1239384 - Simplify and clean up Cameras IPC code

Status: RESOLVED FIXED

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

Description

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Attachment: Remove static interface for Cameras using forwarding

Comment 2

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee81e581b15e

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Attachment: Remove static interface for Cameras using forwarding

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.

Comment 4

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

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.

Comment 5

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

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.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f721ce31dc68dd3069ea7bd378c964db9d49da
Bug 1239384 - Remove static interface for Cameras using forwarding. r=jesup

Comment 7

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef289a285c72

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Attachment: Encapsulate Cameras dispatch, locking and success handling in a class

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Attachment: Encapsulate Cameras dispatch, locking and success handling in a class

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.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9f721ce31dc

Comment 11

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

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

Comment 12

[Gian-Carlo Pascutto [:gcp]]

(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.

Comment 13

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5147ec24a77fc7f58bbac3d15d52f572aa66580
Bug 1239384 - Encapsulate Cameras dispatch, locking and success handling in a class. r=jesup

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b5147ec24a77