Simplify and clean up Cameras IPC code

RESOLVED FIXED in Firefox 46

Status

()

Core
WebRTC: Audio/Video
--
enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gcp, Assigned: gcp)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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

2 years ago
Created attachment 8707526 [details] [diff] [review]
Remove static interface for Cameras using forwarding
(Assignee)

Comment 2

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee81e581b15e
(Assignee)

Comment 3

2 years ago
Created attachment 8707874 [details] [diff] [review]
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.
Attachment #8707874 - Flags: review?(rjesup)
(Assignee)

Updated

2 years ago
Attachment #8707526 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 6

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

Updated

2 years ago
Whiteboard: [leave open]
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef289a285c72
(Assignee)

Comment 8

2 years ago
Created attachment 8709323 [details] [diff] [review]
Encapsulate Cameras dispatch, locking and success handling in a class
Attachment #8709323 - Flags: review?(rjesup)
(Assignee)

Comment 9

2 years ago
Created attachment 8709326 [details] [diff] [review]
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.
Attachment #8709326 - Flags: review?(rjesup)
(Assignee)

Updated

2 years ago
Attachment #8709323 - Attachment is obsolete: true
Attachment #8709323 - Flags: review?(rjesup)

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f9f721ce31dc
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

2 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

2 years ago
Attachment #8709326 - Flags: review?(rjesup) → review+
(Assignee)

Comment 13

2 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

2 years ago
Whiteboard: [leave open]

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5147ec24a77
Status: NEW → RESOLVED
Last Resolved: 2 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.