Closed Bug 1053130 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: ctai, Assigned: ayang)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 8 obsolete files)

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
Attached patch bug-1053130.patch v1.0 (obsolete) — Splinter Review
Extract the base class "MediaEngineCameraVideoSource" from "MediaEngineWebRTCVideoSource" and "MediaEngineGonkVideoSource".
Attached patch bug-1053130.patch v1.1 (obsolete) — Splinter Review
Fix the comment in MediaEngineGonkVideoSource.h#29.
Attachment #8474447 - Attachment is obsolete: true
Attachment #8474451 - Flags: feedback?(jolin)
Attachment #8474451 - Flags: feedback?(slee)
Comment on attachment 8474451 [details] [diff] [review]
bug-1053130.patch v1.1

Need to fix some bugs. Cancel the feedback?
Attachment #8474451 - Flags: feedback?(slee)
Attachment #8474451 - Flags: feedback?(jolin)
Attached patch bug-1053130.patch v1.2 (obsolete) — Splinter Review
fix some -Werror=reorder.
Attachment #8474451 - Attachment is obsolete: true
Attachment #8474921 - Flags: feedback?(slee)
Attachment #8474921 - Flags: feedback?(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?
Attachment #8474921 - Flags: feedback?(jolin) → feedback+
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.
Attachment #8474921 - Flags: feedback?(slee) → feedback+
Attached patch bug-1053130.patch v1.3 (obsolete) — Splinter Review
Rebased and addressed John and Steven's comments.
Carry John and Steven's f+
Attachment #8474921 - Attachment is obsolete: true
Attachment #8475031 - Flags: feedback+
Attached patch bug-1053130.patch v1.4 (obsolete) — Splinter Review
Fix some nits, like &/* goes with type and some missing argument names in declarations.
Attachment #8475031 - Attachment is obsolete: true
Comment on attachment 8475053 [details] [diff] [review]
bug-1053130.patch v1.4

Carry John and Steven's f+.
Attachment #8475053 - Flags: feedback+
Attached patch bug-1053130.patch v1.5 (obsolete) — Splinter Review
Fix reviewer typo.
Carry f+ from John and Steven.
Attachment #8475053 - Attachment is obsolete: true
Attachment #8475054 - Flags: feedback+
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
Attachment #8475054 - Flags: feedback?(rjesup)
Attachment #8475054 - Flags: feedback?(rjesup) → review?(rjesup)
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)
Attached patch bug-1053130.patch v1.6 (obsolete) — Splinter Review
Attachment #8475054 - Attachment is obsolete: true
Attachment #8475054 - Flags: review?(rjesup)
Attached patch bug-1053130.patch v1.7 (obsolete) — Splinter Review
Attachment #8483979 - Attachment is obsolete: true
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.
Flags: needinfo?(rjesup)
Blocks: 1074675
QA Contact: ayang
QA Contact: ayang
Assignee: ctai → ayang
Flags: needinfo?(rjesup)
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.
Attachment #8483983 - Attachment is obsolete: true
Attachment #8499451 - Flags: review?(rjesup)
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
Attachment #8499451 - Flags: review?(rjesup) → review+
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.
 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))
Note there were already a bunch of failures that were not due to the M1 bustage, so we'll need to look at this.
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)
Attachment #8503720 - Flags: review?(ayang)
Attachment #8503720 - Flags: review?(ayang) → review+
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.
Bustage fix; MediaEngineGonkVideoSource needs mozilla/dom/File.h
https://hg.mozilla.org/integration/mozilla-inbound/rev/42f385becea7
https://hg.mozilla.org/mozilla-central/rev/c033be95eb1f
https://hg.mozilla.org/mozilla-central/rev/42f385becea7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1089096
You need to log in before you can comment on or make changes to this bug.