Closed
Bug 1239384
Opened 9 years ago
Closed 9 years ago
Simplify and clean up Cameras IPC code
Categories
(Core :: WebRTC: Audio/Video, enhancement)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: gcp, Assigned: gcp)
Details
Attachments
(2 files, 2 obsolete files)
29.19 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
13.88 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
The current PCameras code contains a fair amount of duplicated code or repeated patterns, and is quite complicated in general. Some of it can probably be cleaned up.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
This removes the static Cameras interface. It was the cause for quite a bit
of code duplication, both in having to repeat the entire API as CamerasChild
methods (that do the IPC), as well as in having to repeat the
obtain-ptr-and-grab-lock sequence to ensure the object is alive throughout
the call.
The solution modifies the client users to add a simple GetChildAndCall() wrapper.
Because it's implemented via perfect forwarding and templates, we need to move
a bit more code to the CamerasChild header.
Attachment #8707874 -
Flags: review?(rjesup)
Assignee | ||
Updated•9 years ago
|
Attachment #8707526 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gpascutto
Comment 4•9 years ago
|
||
Comment on attachment 8707874 [details] [diff] [review]
Remove static interface for Cameras using forwarding
Review of attachment 8707874 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/systemservices/CamerasChild.h
@@ +76,5 @@
> + CamerasSingleton();
> + ~CamerasSingleton();
> +
> + static OffTheBooksMutex& Mutex() {
> + return GetInstance().mCamerasMutex;
So this creates the instance in a non-threadsafe way, without a mutex, before returning the mutex to use in accessing the same object. I think this is problematically unsafe (and makes use of the mutex almost moot), as we're back to relying on C++11's static object guarantees which are broken in MSVC 2013.
Attachment #8707874 -
Flags: review?(rjesup) → review-
Comment 5•9 years ago
|
||
Comment on attachment 8707874 [details] [diff] [review]
Remove static interface for Cameras using forwarding
Review of attachment 8707874 [details] [diff] [review]:
-----------------------------------------------------------------
looking again, this is not a change - same pattern as before, just moved.
Attachment #8707874 -
Flags: review- → review+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f721ce31dc68dd3069ea7bd378c964db9d49da
Bug 1239384 - Remove static interface for Cameras using forwarding. r=jesup
Assignee | ||
Updated•9 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8709323 -
Flags: review?(rjesup)
Assignee | ||
Comment 9•9 years ago
|
||
I still need to find a better way to deal with the magic "mReplyXXXX"
variables (which is why this was using templates), but it's already a nice
simplification.
Attachment #8709326 -
Flags: review?(rjesup)
Assignee | ||
Updated•9 years ago
|
Attachment #8709323 -
Attachment is obsolete: true
Attachment #8709323 -
Flags: review?(rjesup)
Comment 10•9 years ago
|
||
bugherder |
Comment 11•9 years ago
|
||
Comment on attachment 8709326 [details] [diff] [review]
Encapsulate Cameras dispatch, locking and success handling in a class
Review of attachment 8709326 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/systemservices/CamerasChild.cpp
@@ +169,5 @@
> + const T& ReturnValue() {
> + if (mSuccess) {
> + return mSuccessValue;
> + } else {
> + return mFailureValue;
I'd code this as a trinary 1-liner, but this is fine
@@ +192,5 @@
> + // Prevent concurrent use of the reply variables by holding
> + // the mReplyMonitor. Note that this is unlocked while waiting for
> + // the reply to be filled in, necessitating the additional mRequestLock/Mutex;
> + MonitorAutoLock mReplyLock;
> + MutexAutoLock mRequestLock;
What is using mRequestLock now? I see nothing locking it
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #11)
> @@ +192,5 @@
> > + // Prevent concurrent use of the reply variables by holding
> > + // the mReplyMonitor. Note that this is unlocked while waiting for
> > + // the reply to be filled in, necessitating the additional mRequestLock/Mutex;
> > + MonitorAutoLock mReplyLock;
> > + MutexAutoLock mRequestLock;
>
> What is using mRequestLock now? I see nothing locking it
It's in the constructors' intializer list. Being a MutexAutoLock, that locks it.
Updated•9 years ago
|
Attachment #8709326 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5147ec24a77fc7f58bbac3d15d52f572aa66580
Bug 1239384 - Encapsulate Cameras dispatch, locking and success handling in a class. r=jesup
Assignee | ||
Updated•9 years ago
|
Whiteboard: [leave open]
Comment 14•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•