1053130 - [Refactor] Extract a base class for B2G camera video source and WebRTC Video source to inherit from.

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Initial from [1]. We should identify the major base functionality of WebRTCVideoSource and GonkVideoSource and refactor that into a base class both inherit from. Too much duplicated code will be a maintenance nightmare.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=938034#c44

Comment 1

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1053130.patch v1.0

Extract the base class "MediaEngineCameraVideoSource" from "MediaEngineWebRTCVideoSource" and "MediaEngineGonkVideoSource".

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1053130.patch v1.1

Fix the comment in MediaEngineGonkVideoSource.h#29.

Comment 3

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8474451 [details] [diff] [review]
bug-1053130.patch v1.1

Need to fix some bugs. Cancel the feedback?

Comment 4

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1053130.patch v1.2

fix some -Werror=reorder.

Comment 5

[John Lin [:jhlin][:jolin]]

Comment on attachment 8474921 [details] [diff] [review]
bug-1053130.patch v1.2

Review of attachment 8474921 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM except for the possible leakage.

::: content/media/webrtc/MediaEngineCameraVideoSource.h
@@ +66,5 @@
> +    tmp->Append(filename);
> +    rv = tmp->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    mSnapshotPath = new nsString();

Couldn't find delete mSnapshotPath anywhere... Probably leak?

Comment 6

[StevenLee[:slee]]

Comment on attachment 8474921 [details] [diff] [review]
bug-1053130.patch v1.2

Review of attachment 8474921 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. :)

::: content/media/webrtc/MediaEngineGonkVideoSource.cpp
@@ +140,5 @@
> +nsresult
> +MediaEngineGonkVideoSource::Start(SourceMediaStream* aStream, TrackID aID)
> +{
> +  LOG((__FUNCTION__));
> +  int error = 0;

This variable is unused. Please remove it.

@@ +187,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  {
> +    MonitorAutoLock lock(mMonitor);

Could you move mMonitor to protect mImage only? mState is protected by mCallbackMonitor.

::: content/media/webrtc/MediaEngineGonkVideoSource.h
@@ +29,5 @@
> + * CameraThread:
> + * mDOMCameraControl, mCaptureIndex, mCameraThread, mWindowId, mCameraManager,
> + * mNativeCameraControl, mPreviewStream, mState, mLastCapture, mWidth, mHeight
> + *
> + * Where mWidth, mHeight, mImage are protected by mMonitor

mRotation, mCameraAngle and mBackCamera are also protected by mMonitor.

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1053130.patch v1.3

Rebased and addressed John and Steven's comments.
Carry John and Steven's f+

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1053130.patch v1.4

Fix some nits, like &/* goes with type and some missing argument names in declarations.

Comment 9

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8475053 [details] [diff] [review]
bug-1053130.patch v1.4

Carry John and Steven's f+.

Comment 10

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1053130.patch v1.5

Fix reviewer typo.
Carry f+ from John and Steven.

Comment 11

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8475054 [details] [diff] [review]
bug-1053130.patch v1.5

Hi, Randell,
Could you please give me some feedback for this patch?
I extract a base class for "MediaEngineWebRTCVideoSource" and "MediaEngineGonkVideoSource".
Thanks

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8475054 [details] [diff] [review]
bug-1053130.patch v1.5

Review of attachment 8475054 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this is much better.  I want to see an answer about MediaEngineWebRTCVideo and can likely r+ at that time.  The snapshot functionality can be removed later.

::: content/media/webrtc/MediaEngine.h
@@ +144,5 @@
>    /* It is an error to call Start() before an Allocate(), and Stop() before
>     * a Start(). Only Allocate() may be called after a Deallocate(). */
>  
>  protected:
> +  // Only class' own members can be initialized in constructor initializer lis.

"list"

::: content/media/webrtc/MediaEngineCameraVideoSource.h
@@ +55,5 @@
> +  }
> +
> +  // This runnable is for creating a temporary file on the main thread.
> +  NS_IMETHODIMP
> +  Run()

This runnable could be a separate class with an nsRefPtr back to this...  But I question if this is needed at all.  Snapshot functionality should likely be dropped

@@ +78,5 @@
> +
> +protected:
> +  ~MediaEngineCameraVideoSource()
> +  {
> +    delete mSnapshotPath;

mSnapshotPath shouldn't be a raw ptr

@@ +91,5 @@
> +  void GuessCapability(const VideoTrackConstraintsN& aConstraints,
> +                       const MediaEnginePrefs& aPrefs);
> +
> +  static const unsigned int KMaxDeviceNameLength = 128;
> +  static const unsigned int KMaxUniqueIdLength = 256;

These should not be camera-specific (or even video-specific).

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +60,2 @@
>   */
> +class MediaEngineWebRTCVideoSource : public MediaEngineCameraVideoSource

MediaEngineVideoSource?

With the changes, what is this now used for?

::: content/media/webrtc/moz.build
@@ +38,5 @@
> +    if CONFIG['MOZ_B2G_CAMERA']:
> +        EXPORTS += ['MediaEngineGonkVideoSource.h']
> +        UNIFIED_SOURCES += [
> +            'MediaEngineGonkVideoSource.cpp',
> +        ]

a build peer is needed for these changes (easy review)

Comment 13

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1053130.patch v1.6

Comment 14

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1053130.patch v1.7

Comment 15

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Thanks for your comments, just have one question need to make clear. About the run function for snapshot.
(In reply to Randell Jesup [:jesup] from comment #12)
> Comment on attachment 8475054 [details] [diff] [review]
> bug-1053130.patch v1.5
> 
> Review of attachment 8475054 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this is much better.  I want to see an answer about
> MediaEngineWebRTCVideo and can likely r+ at that time.  The snapshot
> functionality can be removed later.
> 
> ::: content/media/webrtc/MediaEngine.h
> @@ +144,5 @@
> >    /* It is an error to call Start() before an Allocate(), and Stop() before
> >     * a Start(). Only Allocate() may be called after a Deallocate(). */
> >  
> >  protected:
> > +  // Only class' own members can be initialized in constructor initializer lis.
> 
> "list"
> 
> ::: content/media/webrtc/MediaEngineCameraVideoSource.h
> @@ +55,5 @@
> > +  }
> > +
> > +  // This runnable is for creating a temporary file on the main thread.
> > +  NS_IMETHODIMP
> > +  Run()
> 
> This runnable could be a separate class with an nsRefPtr back to this... 
> But I question if this is needed at all.  Snapshot functionality should
> likely be dropped

Do you mean we just remove the |run| from MediaEngineCameraVideoSource?
Actually I don't see any usage of this function |run|, do I miss anything?

> 
> @@ +78,5 @@
> > +
> > +protected:
> > +  ~MediaEngineCameraVideoSource()
> > +  {
> > +    delete mSnapshotPath;
> 
> mSnapshotPath shouldn't be a raw ptr
> 
> @@ +91,5 @@
> > +  void GuessCapability(const VideoTrackConstraintsN& aConstraints,
> > +                       const MediaEnginePrefs& aPrefs);
> > +
> > +  static const unsigned int KMaxDeviceNameLength = 128;
> > +  static const unsigned int KMaxUniqueIdLength = 256;
> 
> These should not be camera-specific (or even video-specific).
> 
> ::: content/media/webrtc/MediaEngineWebRTC.h
> @@ +60,2 @@
> >   */
> > +class MediaEngineWebRTCVideoSource : public MediaEngineCameraVideoSource
> 
> MediaEngineVideoSource?
> 
> With the changes, what is this now used for?

MediaEngineCameraVideoSource is also inherited by MediaEngineDefaultVideoSource and MediaEngineTabVideoSource. |Allocate| is the key purpose of this class. It is used in |GetUserMediaRunnable::ProcessGetUserMedia|.
If we remove MediaEngineCameraVideoSource, then it means that we need to make MediaEngineCameraVideoSource be the parent class of MediaEngineDefaultVideoSource and MediaEngineTabVideoSource. I don't think it is a good idea.

> 
> ::: content/media/webrtc/moz.build
> @@ +38,5 @@
> > +    if CONFIG['MOZ_B2G_CAMERA']:
> > +        EXPORTS += ['MediaEngineGonkVideoSource.h']
> > +        UNIFIED_SOURCES += [
> > +            'MediaEngineGonkVideoSource.cpp',
> > +        ]
> 
> a build peer is needed for these changes (easy review)

Thanks for reminding, will ask build peer once get r+ from you.

Comment 16

[Alfredo Yang (:alfredo)]

Attachment: refactory_media_engine

Remove Snapshot() function and address the review comments in comment 12, except for keeping MediaEngineVideoSource. Please let me know if you think it'd be better to remove it.
Thanks.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8499451 [details] [diff] [review]
refactory_media_engine

Review of attachment 8499451 [details] [diff] [review]:
-----------------------------------------------------------------

I can probably resolve the nits and land this today so it can ride 35.

Thanks ctai and alfredo!!  This *really* does a great job of cleaning up and untangling this code.  Great work, and it will make working on this code much less error-prone and faster.

::: content/media/webrtc/MediaEngineCameraVideoSource.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef MediaEngineCameraVideoSource_h
> +#define MediaEngineCameraVideoSource_h

case (nit)

@@ +78,5 @@
> +  Monitor mMonitor; // Monitor for processing Camera frames.
> +  webrtc::CaptureCapability mCapability; // Doesn't work on OS X.
> +  int mWidth, mHeight;
> +
> +  nsRefPtr<layers::Image> mImage;

Put in a block with mMonitor and anything else protected by it (and mention mMonitor is used by child classes to protect things as well)

::: content/media/webrtc/MediaEngineGonkVideoSource.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef MediaEngineGonkVideoSource_h_
> +#define MediaEngineGonkVideoSource_h_

include guards are normally all-caps (minor nit)

@@ +30,5 @@
> + * mDOMCameraControl, mCaptureIndex, mCameraThread, mWindowId, mCameraManager,
> + * mNativeCameraControl, mPreviewStream, mState, mLastCapture, mWidth, mHeight
> + *
> + * Where mWidth, mHeight, mImage, mPhotoCallbacks, mRotation, mCameraAngle and
> + * mBackCamera are protected by mMonitor.

This is now wrong, and references mMonitor which isn't here (we get it from our parent MediaEngineCameraVideoSource)

@@ +100,5 @@
> +  nsRefPtr<ICameraControl> mCameraControl;
> +  nsCOMPtr<nsIDOMFile> mLastCapture;
> +  nsTArray<nsRefPtr<PhotoCallback>> mPhotoCallbacks;
> +
> +  // These are protected by mMonitor below

This comment is now wrong

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Also, I noted that the section copied to the new GonkVideo file has a few updates since this patch was made; I'm merging that in to resolve the conflict.

hg diff -r 7c24470b6b3aace8d17bb3c218cd8c6598017245 content/media/webrtc/MediaEngineWebRTCVideo.cpp shows the changes

I'll push a try.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

 https://tbpl.mozilla.org/?tree=Try&rev=11a051638cca
(Note: this has the patch merged, but not the nits above resolved (which are mostly comment changes))

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

 https://tbpl.mozilla.org/?tree=Try&rev=541062befa48
(previous Try was on top of M1 bustage)

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Note there were already a bunch of failures that were not due to the M1 bustage, so we'll need to look at this.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Attachment: restore MediaSource property lost in refactor

The screen/windowsharing failures were because the refactor probably started from a base without all the Screen/WindowSharing code, so they ended up removed.  I simply put them back (really correctly merged it forward - these were likely flagged as conflicts and I copy-pasted the new code in without verifying if the old code had changed)

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

https://tbpl.mozilla.org/?tree=Try&rev=f2e822c02c8b

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c033be95eb1f

Note: I resolved the nits and folded the patches.  Also I added "hg cp"'s to the new files from the files they were refactored from, so you can see which bits ended up where and preserve a bit of the hg history/blame.

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Bustage fix; MediaEngineGonkVideoSource needs mozilla/dom/File.h
https://hg.mozilla.org/integration/mozilla-inbound/rev/42f385becea7

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c033be95eb1f
https://hg.mozilla.org/mozilla-central/rev/42f385becea7