Closed Bug 1239384 Opened 4 years ago Closed 4 years ago

Simplify and clean up Cameras IPC code

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: gcp, Assigned: gcp)

Details

Attachments

(2 files, 2 obsolete files)

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.
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)
Attachment #8707526 - Attachment is obsolete: true
Assignee: nobody → gpascutto
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 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+
Whiteboard: [leave open]
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)
Attachment #8709323 - Attachment is obsolete: true
Attachment #8709323 - Flags: review?(rjesup)
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
(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.
Attachment #8709326 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5147ec24a77fc7f58bbac3d15d52f572aa66580
Bug 1239384 - Encapsulate Cameras dispatch, locking and success handling in a class. r=jesup
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/b5147ec24a77
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.