Closed
Bug 1053130
Opened 10 years ago
Closed 10 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)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: ctai, Assigned: ayang)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 8 obsolete files)
99.27 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
3.77 KB,
patch
|
ayang
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•10 years ago
|
||
Extract the base class "MediaEngineCameraVideoSource" from "MediaEngineWebRTCVideoSource" and "MediaEngineGonkVideoSource".
Reporter | ||
Comment 2•10 years ago
|
||
Fix the comment in MediaEngineGonkVideoSource.h#29.
Attachment #8474447 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Attachment #8474451 -
Flags: feedback?(jolin)
Reporter | ||
Updated•10 years ago
|
Attachment #8474451 -
Flags: feedback?(slee)
Reporter | ||
Comment 3•10 years ago
|
||
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)
Reporter | ||
Comment 4•10 years ago
|
||
fix some -Werror=reorder.
Attachment #8474451 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Attachment #8474921 -
Flags: feedback?(slee)
Reporter | ||
Updated•10 years ago
|
Attachment #8474921 -
Flags: feedback?(jolin)
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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+
Reporter | ||
Comment 7•10 years ago
|
||
Rebased and addressed John and Steven's comments. Carry John and Steven's f+
Attachment #8474921 -
Attachment is obsolete: true
Attachment #8475031 -
Flags: feedback+
Reporter | ||
Comment 8•10 years ago
|
||
Fix some nits, like &/* goes with type and some missing argument names in declarations.
Attachment #8475031 -
Attachment is obsolete: true
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8475053 [details] [diff] [review] bug-1053130.patch v1.4 Carry John and Steven's f+.
Attachment #8475053 -
Flags: feedback+
Reporter | ||
Comment 10•10 years ago
|
||
Fix reviewer typo. Carry f+ from John and Steven.
Attachment #8475053 -
Attachment is obsolete: true
Attachment #8475054 -
Flags: feedback+
Reporter | ||
Comment 11•10 years ago
|
||
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)
Reporter | ||
Updated•10 years ago
|
Attachment #8475054 -
Flags: feedback?(rjesup) → review?(rjesup)
Comment 12•10 years ago
|
||
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)
Reporter | ||
Comment 13•10 years ago
|
||
Attachment #8475054 -
Attachment is obsolete: true
Attachment #8475054 -
Flags: review?(rjesup)
Reporter | ||
Comment 14•10 years ago
|
||
Attachment #8483979 -
Attachment is obsolete: true
Reporter | ||
Comment 15•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
QA Contact: ayang
Assignee | ||
Updated•10 years ago
|
QA Contact: ayang
Assignee | ||
Updated•10 years ago
|
Assignee: ctai → ayang
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(rjesup)
Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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+
Comment 18•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=541062befa48 (previous Try was on top of M1 bustage)
Comment 21•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f2e822c02c8b
Updated•10 years ago
|
Attachment #8503720 -
Flags: review?(ayang)
Assignee | ||
Updated•10 years ago
|
Attachment #8503720 -
Flags: review?(ayang) → review+
Comment 24•10 years ago
|
||
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•10 years ago
|
||
Bustage fix; MediaEngineGonkVideoSource needs mozilla/dom/File.h https://hg.mozilla.org/integration/mozilla-inbound/rev/42f385becea7
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c033be95eb1f https://hg.mozilla.org/mozilla-central/rev/42f385becea7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•