Closed Bug 1399413 Opened 7 years ago Closed 7 years ago

Multiple content process getUserMedia is rejected (all platforms except OSX)

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

Product:
Core
Component:
WebRTC: Audio/Video
Version:
57 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
thunderbird_esr52 --- unaffected
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: mchiang, Assigned: mchiang)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1399413 - add mochitests to check multi-tabs gUM.
59 bytes, text/x-review-board-request
jib
: review+
Details 
Steps:

1. Ensure multiple content process is enabled by checking the value of dom.ipc.processCount (should be 4).

2. open a tab, go to https://jsfiddle.net/qregmce3/. Choose a camera device and grant the permission.

3. open another tab, go to the same website. Choose the same camera device and grant the permission.

Expect:
gUM and get a mediastream

Actual:
gUM is rejected.
It seems Windows doesn't allow multiple processes access the same camera hardware simultaneously.
For multiple content processes case, if we open two tabs calling gUM simultaneously, there will be two corresponding instances of CamerasParent / VideoEngine / VideoCaptureModule. This model barely works on Mac (with some limitation) but doesn't work on Windows / Linux. We probably need to refactor the design and make CamerasParent / VideoEngine / VideoCaptureModule as a singleton.
I'm surprised this fails... all capture is done in the Master process via CamerasParent.  There should be a single CamerasParent in the Master.  I suppose there could be a race and we got two?  Seems unlikely given how this was tested.

Munro, what evidence do you ahve for multiple camerasparents?  Logs? (MediaManager:5,GetUserMedia:5,CamerasParent:5)
Flags: needinfo?(mchiang)
Flags: needinfo?(jib)
Rank: 13
Priority: -- → P1
There's nothing enforcing a single CamerasParent, i.e. CamerasParent is made on demand when a Child is created, 

http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/dom/media/systemservices/CamerasChild.cpp#117.

See also bug 1194699 and bug 1073017.
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
(In reply to Randell Jesup [:jesup] from comment #3)
> Munro, what evidence do you ahve for multiple camerasparents?  Logs?
> (MediaManager:5,GetUserMedia:5,CamerasParent:5)
I use lldb to check 'this' in VideoEngine::CreateVideoCapture and VideoCaptureMacAVFoundation ctor.

> I suppose there could be a race and we got two?  Seems unlikely given how this
> was tested.
There are some problems caused by this. bug 1388667 is one of them. On Mac, avfounation will do its best to fulfill each VideoCaptureModule (VideoCaptureMacAVFoundation)'s configuration to the same hardware. On Windows, the second request to access the same hardware is denied.

If we can't enforce a single CamerasParent, perhaps at least we should make VideoEngine and VideoCaptureModule as a singleton.
Flags: needinfo?(mchiang)
I can also reproduce the issue on Windows 10.
OS: Windows 7 → Windows
[Tracking Requested - why for this release]: regression + combined with bug 1400488 this effectively means cam+mic only works in one tab at a time (!) on all platforms but OSX.

I've verified this on Windows 10 as well, and the regression range points to bug 1303113 (same as on linux in bug 1402905).

The only OS this seems to work with is OSX. People expect this to work in multiple tabs, and we seem to have the design in place to do this right.

(In reply to Munro Mengjue Chiang [:mchiang] from comment #6)
> If we can't enforce a single CamerasParent, perhaps at least we should make
> VideoEngine and VideoCaptureModule as a singleton.

That sounds like a reasonable approach to me, or maybe even add a single layer on top, a CamerasSingleton if you will, that the various CamerasParents create and share?

Gcp, anything we should know before attempting this?

Munro, would it make sense for you to look at this, or do you need help? I know you're pretty busy with the downscaling work in and around this area.
Blocks: 1303113
Rank: 13 → 10
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox-esr52: --- → unaffected
status-thunderbird_esr52: --- → unaffected
tracking-firefox57: --- → ?
tracking-firefox58: --- → ?
Flags: needinfo?(jib) → needinfo?(mchiang)
Keywords: regression
OS: Windows → All
Hardware: x86_64 → All
Summary: [Windows] multiple content process getUserMedia is rejected → Multiple content process getUserMedia is rejected (all platforms except OSX)
Version: unspecified → 57 Branch
Flags: needinfo?(gpascutto)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #4)
> There's nothing enforcing a single CamerasParent, i.e. CamerasParent is made
> on demand when a Child is created, 

Are all CamerasParents processed on the same thread, or do they each have their own thread?
Assignee: nobody → mchiang
Flags: needinfo?(mchiang)
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #11)
> Are all CamerasParents processed on the same thread, or do they each have
> their own thread?

http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/ipc/glue/BackgroundParentImpl.cpp#434

I think they share the PBackground thread in the parent.
Flags: needinfo?(gpascutto)
Bad regression, tracked for 57. I hope we can get a fix ready in a week or two so as to not destabilize beta57.
tracking-firefox57: ? → +
I am working on this with the highest priority, but I can foresee the fix won't be trivial.
Comparing to calling gUM in one tab, calling gUM in different tabs is still edge case.
The current code performs well in the main case.
Do we really need to fix this bug in 57?
Flags: needinfo?(rkothari)
Flags: needinfo?(jib)
I'll note that a common usecase for developers is to call from one tab to another (I do that frequently, and I suspect it's even more common for webrtc application developers - anytime they don't have two machines sitting next to them).  Normal users probably only do this occasionally, generally to test if it works, or "try it out".  There are some other usecases that are even less common or virtually never happen (yet) (recording in one tab, while having a call in another, or using one to listen for voice commands while having a call in another, etc).

Things that push developers away from using Firefox (or make it hard for them to verify their changes to their site still work on Firefox) can hurt us.  That said, I don't think it's mandatory it be fixed in 57, though I would very much like it.
Comment 15 is why I thought it important to see if we could get this in to 57, but it sounds from comment 14 that it'll be too risky at this point. I'm still getting used to only having one beta cycle effectively before release. :-(

Bug 1400488 has already been marked wontfix for 57, for the same reason of being too risky, and it regressed more recently than this one.

Thanks Ritu for considering it. Hopefully, developers will use Firefox Developer Edition and be on 58 soon.
status-firefox57: affected → wontfix
tracking-firefox57: + → ---
Flags: needinfo?(jib)
Thank you everyone for weighing in the risk and reward on this one. Let's try to get this fixed in 58
tracking-firefox58: ? → +
Flags: needinfo?(rkothari)
This is only a draft for reference.
I only test it on osx.
There are some unfinished refactoring in vcm_capturer.cc and desktop_capture_impl.cc
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review192330


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/systemservices/VideoEngine.cpp:85
(Diff revision 1)
> -  WithEntry(id, [&found](CaptureEntry& cap) {
> +    WithEntry(id, [&found](CaptureEntry& cap) {
> -         cap.mVideoCaptureModule = nullptr;
> +      cap.mVideoCaptureModule = nullptr;
> -        found = true;
> +      found = true;
> -   });
> +    });
> -  return found ? 0 : (-1);
> +    return found ? 0 : (-1);
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Revision 2: rebase & fix static analysis defect.

I have verified the patch in Windows.
Revision 3: support desktop sharing in multiple tabs (content processes)
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review194376


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/systemservices/CamerasParent.cpp:397
(Diff revision 3)
> -        device_info->DeRegisterVideoInputFeedBack();
> +      device_info->DeRegisterVideoInputFeedBack(mCameraObserver.get());
> -      }
> +    }
> +  }
> +
> +  if (0 == --sNumOfCamerasParent) {
> +    for (int i = 0; i < CaptureEngine::MaxEngine; i++) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]
Revision 4: fix static analysis defect.

Remaining job:
Fix memory leak found in try server (https://treeherder.mozilla.org/#/jobs?repo=try&revision=09edf3fffbaf75052fde0d6a22c100c728d6bf16)
Revision 4-6: fix memory leakage
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review194804


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/systemservices/CamerasParent.cpp:398
(Diff revision 5)
> -      }
> +    }
> +    mCameraObserver = nullptr;
> +  }
> +
> +  if (0 == --sNumOfCamerasParent) {
> +    for (int i = 0; i < CaptureEngine::MaxEngine; i++) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]
In discussing this, due to size of patch and difference in design, it may make sense to hold off landing to the very beginning of 59 cycle to maximize testing for this, and live with the existing 57 behavior in 58.
status-firefox58: affected → fix-optional
Attachment #8916027 - Flags: review?(gpascutto)
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review197472

I think we need a static mutex as well, unless I'm missing something?

Gcp left several scary threading comments in this code when he wrote it, so I'd like him to review this as well, in case there are gotchas I don't understand  here.

::: commit-message-8e052:1
(Diff revision 6)
> +Bug 1399413 - Make VideoEngine & VideoCaptureModule as singleton. r?jib

s/as singleton/singletons/

::: dom/media/systemservices/CamerasParent.h:138
(Diff revision 6)
> -  RefPtr<VideoEngine> mEngines[CaptureEngine::MaxEngine];
> +  static RefPtr<VideoEngine> sEngines[CaptureEngine::MaxEngine];
> +  static Atomic<int32_t> sNumOfCamerasParent;

Maybe add a comment on thread access of these?

::: dom/media/systemservices/CamerasParent.h:148
(Diff revision 6)
>    // Monitors creation of the thread below
>    Monitor mThreadMonitor;
>  
>    // video processing thread - where webrtc.org capturer code runs
> -  base::Thread* mVideoCaptureThread;
> +  static base::Thread* sVideoCaptureThread;

How does a per-instance monitor protect a single-instance thread?

Do we need a StaticMutex here, or is there some clever invariant that multiple CamerasParent instances never run simultaneously on PBackground? Does the thread lock all the monitors?

Is the comment outdated? The monitor's use seems to go way beyond thread creation.

Some comments on the threading guarantees would help.

::: dom/media/systemservices/CamerasParent.cpp:208
(Diff revision 6)
>    while (mWebRTCAlive) {
>      mThreadMonitor.Wait();
>    }
>    // After closing the WebRTC stack, clean up the
>    // VideoCapture thread.
> -  if (self->mVideoCaptureThread) {
> +  if ((0 == sNumOfCamerasParent) && self->sVideoCaptureThread) {

Style nit: not a fan of reverse order comparison. We have warnings for this. Also parens seem redundant

    if (sNumOfCamerasParent == 0 && self->sVideoCaptureThread) {

Applies throughout.

::: dom/media/systemservices/CamerasParent.cpp
(Diff revision 6)
> -      if (engine->IsRunning()) {
> -        LOG(("Being closed down while engine %d is running!", i));
> -      }

Is this check and log message no longer useful? Can it not happen? Should we MOZ_ASSERT?

::: dom/media/systemservices/CamerasParent.cpp:398
(Diff revision 6)
> +    for (auto& engine_ : sEngines) {
> +      engine = engine_.get();
> +      if (engine) {
> -      mozilla::camera::VideoEngine::Delete(engine);
> +        mozilla::camera::VideoEngine::Delete(engine);
> -      mEngines[i] = nullptr;
> +        engine_ = nullptr;

Nit: avoid confusing reuse of `engine` here, as well as unusual trailing underscore (foo_) naming convention. Using `auto& engine` and `Delete(engine.get())` should suffice.

::: dom/media/systemservices/CamerasParent.cpp
(Diff revision 6)
>  
>            if (!error) {
>              error = cap.VideoCapture()->StartCapture(capability);
>            }
>            if (!error) {
> -            engine->Startup();

Why do we no longer need this?

::: dom/media/systemservices/CamerasParent.cpp:867
(Diff revision 6)
>            mCallbacks[i - 1]->mStreamId == (uint32_t)capnum) {
> +
> +        CallbackHelper* cbh = mCallbacks[i-1];
> +        engine->WithEntry(capnum,[cbh](VideoEngine::CaptureEntry& cap) {
> +          if (cap.VideoCapture()) {
> +            cap.VideoCapture()->DeRegisterCaptureDataCallback(static_cast<rtc::VideoSinkInterface<webrtc::VideoFrame>*>(cbh));

wordwrap somehow

::: media/webrtc/trunk/webrtc/modules/video_capture/video_capture.h:97
(Diff revision 6)
> -    virtual void DeRegisterVideoInputFeedBack() {
> -     _inputCallBack = NULL;
> +    virtual void RegisterVideoInputFeedBack(VideoInputFeedBack* callBack) {
> +      _inputCallBacks.insert(callBack);
> +    }
> +
> +    virtual void DeRegisterVideoInputFeedBack(VideoInputFeedBack* callBack) {
> +      std::set<VideoInputFeedBack*>::iterator it = _inputCallBacks.find(callBack);

auto

::: media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc:118
(Diff revision 6)
>  
> -void VideoCaptureImpl::DeRegisterCaptureDataCallback() {
> +void VideoCaptureImpl::DeRegisterCaptureDataCallback(
> +  rtc::VideoSinkInterface<VideoFrame>* dataCallBack) {
> -    CriticalSectionScoped cs(&_apiCs);
> +  CriticalSectionScoped cs(&_apiCs);
> -    _dataCallBack = NULL;
> +
> +  std::set<rtc::VideoSinkInterface<VideoFrame>*>::iterator it = _dataCallBacks.find(dataCallBack);

auto

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc:484
(Diff revision 6)
> -void DesktopCaptureImpl::DeRegisterCaptureDataCallback()
> +void DesktopCaptureImpl::DeRegisterCaptureDataCallback(
> +  rtc::VideoSinkInterface<VideoFrame> *dataCallback)
>  {
>    CriticalSectionScoped cs(&_apiCs);
>    CriticalSectionScoped cs2(&_callBackCs);
> -  _dataCallBack = nullptr;
> +  std::set<rtc::VideoSinkInterface<VideoFrame>*>::iterator it = _dataCallBacks.find(dataCallback);

auto
Attachment #8916027 - Flags: review?(jib) → review-
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review197472

> Is this check and log message no longer useful? Can it not happen? Should we MOZ_ASSERT?

It's no longer useful.
Please see my below comment.

> Why do we no longer need this?

Startup() and Shutdown() [1] maintain a flag mIsRunning.
The purpose is to print a warning log while closing engines [2].
Since we plan to make videoengine a singleton, video engine may possibly still running for another content process while this tab is closed. So this checking is no longer useful and I removed all three functions.

[1] http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/dom/media/systemservices/VideoEngine.h#62-72

[2] http://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/dom/media/systemservices/CamerasParent.cpp#386-388
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review198514

::: dom/media/systemservices/CamerasParent.h:148
(Diff revision 6)
>    // Monitors creation of the thread below
>    Monitor mThreadMonitor;
>  
>    // video processing thread - where webrtc.org capturer code runs
> -  base::Thread* mVideoCaptureThread;
> +  static base::Thread* sVideoCaptureThread;

Yes, we need a StaticMutex.
And, is there any static version of monitor or condVar?
Please ignore comment 41.
Revision 8-9: We need to make capnum unique in order to use it as an index in bug 1388667.
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review199644

::: dom/media/systemservices/CamerasParent.cpp:170
(Diff revision 9)
>    MonitorAutoLock lock(mThreadMonitor);
> +  StaticMutexAutoLock slock(sMutex);
>  
>    while(mChildIsAlive && mWebRTCAlive &&
> -        (!mVideoCaptureThread || !mVideoCaptureThread->IsRunning())) {
> +        (!sVideoCaptureThread || !sVideoCaptureThread->IsRunning())) {
> +    sMutex.Unlock();

This code screams design issue.

Now that all the "s" parts are handled on another level, is there no way to pull this loop apart and only have to take one of the locks?

IIRC (but it's been long) the checks are here to ensure that the VideoCaptureThread startup has finished.

Maybe there should be a Monitor on the "s" level that you can take instead (thus automaticlly handling the unlock/wait/lock sequence), and that can "simply" be signaled too whenever mThreadMonitor is?

::: dom/media/systemservices/CamerasParent.cpp:216
(Diff revision 9)
>      mThreadMonitor.Wait();
> +    sMutex.Lock();
>    }
>    // After closing the WebRTC stack, clean up the
>    // VideoCapture thread.
> -  if (self->mVideoCaptureThread) {
> +  if (sNumOfCamerasParent == 0 && self->sVideoCaptureThread) {

This is used as a refcount, but the decrement isn't done here, but in another part of the code. Is it guaranteed that this function can't be called (or be running) inbetween the decrement of the refcount and this line?

::: dom/media/systemservices/CamerasParent.cpp:988
(Diff revision 9)
>        // Start the thread
>        MonitorAutoLock lock(self->mThreadMonitor);
> -      self->mVideoCaptureThread = new base::Thread("VideoCapture");
> +      StaticMutexAutoLock slock(self->sMutex);
> +      sNumOfCamerasParent++;
> +      if (!self->sVideoCaptureThread) {
> +        MOZ_ASSERT(sNumOfCamerasParent == 1);

Note that sNumOfCamerasParent is an atomic, and the other code accessing it does not take locks (or it wouldn't be an atomic). So taking the sMutex here cannot and will not protect you from this value changing underneath you.

(This doesn't mean there's a bug. It just means it's very hard to reason about the correctness of this code)

::: dom/media/systemservices/VideoEngine.cpp:50
(Diff revision 9)
>    LOG((__PRETTY_FUNCTION__));
> +
> +  for (auto &it : mCaps) {
> +    if (strcmp(it.second.VideoCapture()->CurrentDeviceName(), deviceUniqueIdUTF8) == 0) {
> +      MOZ_ASSERT(mClientCounter[id] < 0x100);
> +      id = it.first + mClientCounter[it.first]++;

This line looks very weird until you realize that id contains multiple values in the high and low bits. Maybe an | instead of + makes that clearer?
Attachment #8916027 - Flags: review?(gpascutto)
Hi gcp,

I am thinking if we can eliminate the using of Monitor?
Because there is no static version of Monitor.
We only have staticMutex.

For example,

DispatchToVideoCaptureThread wait for VideoCapture thread running. [1]
CamerasParent will notify after VideoCapture thread has been created and started. [2]

I am wondering if we can call NS_DispatchToMainThread(threadStart) with the parameter NS_DISPATCH_SYNC instead of using Monitor? [3]

[1] http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/dom/media/systemservices/CamerasParent.cpp#164

[2] http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/dom/media/systemservices/CamerasParent.cpp#984

[3] http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/dom/media/systemservices/CamerasParent.cpp#987
Flags: needinfo?(gpascutto)
(In reply to Munro Mengjue Chiang [:mchiang] from comment #44)
> CamerasParent will notify after VideoCapture thread has been created and
s/CamerasParent/CamerasParent ctor
(In reply to Munro Mengjue Chiang [:mchiang] from comment #44)
> I am thinking if we can eliminate the using of Monitor?
> Because there is no static version of Monitor.
> We only have staticMutex.

Perhaps we can add it?

> I am wondering if we can call NS_DispatchToMainThread(threadStart) with the
> parameter NS_DISPATCH_SYNC instead of using Monitor? [3]

I think the problem is that that will block PBackground.
Flags: needinfo?(gpascutto)
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review201006

::: dom/media/systemservices/VideoEngine.cpp:50
(Diff revisions 9 - 10)
>    LOG((__PRETTY_FUNCTION__));
>  
>    for (auto &it : mCaps) {
>      if (strcmp(it.second.VideoCapture()->CurrentDeviceName(), deviceUniqueIdUTF8) == 0) {
>        MOZ_ASSERT(mClientCounter[id] < 0x100);
> -      id = it.first + mClientCounter[it.first]++;
> +      id = (it.first & 0xFFFFFF00) | (mClientCounter[it.first] & 0x000000FF);

Using mClientCounter as part of capnum is not a gread idea. It may lead to duplicated capnum.
I have test the patch with following fiddles on OSX.

gUM Camera
https://jsfiddle.net/h7e163k9/

gUM Desktop
https://jsfiddle.net/45pofqxL/

ondevicechange
https://jsfiddle.net/hsg9t3kf/

Open these pages in multiple tabs and verify different combination.

I will add more test and verify Windows & Linux.
I'd really like a test for this in automation. I'm just not sure how one would do to open two tabs in different processes. If we can do that the rest should be easy.
Revision 11-12: fix Linux build error
I have verified the fiddles in comment 50 in Windows / Linux / OSX.
All of them work properly.
The next step is implementing the automation.
Attachment #8925863 - Attachment is obsolete: true
Attachment #8925863 - Flags: review?(jib)
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review202690


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review202692


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review204320

Looks good. Just curious about some use of sets and maps instead of simpler open-counts, and an Atomic that no longer seems needed. Would like to see us use the simplest constructs necessary. r=me with that.

::: dom/media/systemservices/CamerasParent.h:138
(Diff revisions 6 - 15)
> +  // sEngines will be accessed by VideoCapture thread only
> +  // sNumOfCamerasParent and sVideoCaptureThread will be accessed by
> +  // main thread / PBackground thread / VideoCapture thread

This comment is great, but leaves out a couple of variables.

I'm particularly interested in what threads access sLiveCamerasParents and what protects it.

It would also help to describe which locks (since there are multiple now) protect each variable.

::: dom/media/systemservices/CamerasParent.h:142
(Diff revisions 6 - 15)
>  
> +  // sEngines will be accessed by VideoCapture thread only
> +  // sNumOfCamerasParent and sVideoCaptureThread will be accessed by
> +  // main thread / PBackground thread / VideoCapture thread
>    static RefPtr<VideoEngine> sEngines[CaptureEngine::MaxEngine];
> -  static Atomic<int32_t> sNumOfCamerasParent;
> +  static std::set<CamerasParent*> sLiveCamerasParents;

(discussed on slack) Since this set<> is basically a glorified open-count, maybe use a simpler sNumOfOpenCamerasParentEngines instead?

(Name suggestion is based on CamerasParent::ClosedEngines() being where we decrement this count).

::: dom/media/systemservices/CamerasParent.h:143
(Diff revisions 6 - 15)
> +  // sEngines will be accessed by VideoCapture thread only
> +  // sNumOfCamerasParent and sVideoCaptureThread will be accessed by
> +  // main thread / PBackground thread / VideoCapture thread
>    static RefPtr<VideoEngine> sEngines[CaptureEngine::MaxEngine];
> -  static Atomic<int32_t> sNumOfCamerasParent;
> +  static std::set<CamerasParent*> sLiveCamerasParents;
> +  static Atomic<int32_t> sNumOfCamerasParents;

Now that sNumOfCamerasParents is protected by sMutex, does it still need to be Atomic?

::: dom/media/systemservices/CamerasParent.cpp:202
(Diff revisions 6 - 15)
>    // Hold here until the WebRTC thread is gone. We need to dispatch
>    // the thread deletion *now*, or there will be no more possibility
>    // to get to the main thread.

I'm a bit confused by this comment. Is "WebRTC thread" and "VideoCapture thread" the same thing here?

Maybe s/gone/done/ and move the talk of deletion down into the if() where we conditionally delete the sVideoCaptureThread now?

::: dom/media/systemservices/CamerasParent.cpp:400
(Diff revisions 6 - 15)
> -  if (0 == --sNumOfCamerasParent) {
> -    for (auto& engine_ : sEngines) {
> +  sLiveCamerasParents.erase(this);
> +  if (sLiveCamerasParents.empty()) {

Maybe add a comment that we're protected by sThreadMonitor here?

::: dom/media/systemservices/VideoEngine.cpp:79
(Diff revisions 6 - 15)
> -  if (mClientCounter[id] == 0) {
> +  {
> +    auto it = mIdMap.find(id);
> +    MOZ_ASSERT(it != mIdMap.end());
> +    Unused << it;
> +  }

#ifdef DEBUG

::: dom/media/systemservices/VideoEngine.cpp:85
(Diff revisions 6 - 15)
> +  for (auto &it : mIdMap) {
> +    if (it.first != id && it.second == mIdMap[id]) {
> +      // There are other tracks still using this hardware.
> +      found = true;

What does this solve that mClientCounter didn't?

::: dom/media/systemservices/VideoEngine.cpp:202
(Diff revisions 6 - 15)
> -  auto it = mCaps.find(entryCapnum);
> +  {
> +    auto it = mIdMap.find(entryCapnum);
> +    MOZ_ASSERT(it != mIdMap.end());
> +    Unused << it;
> +  }

#ifdef DEBUG

::: dom/media/systemservices/VideoEngine.cpp:221
(Diff revisions 6 - 15)
>  
>  int32_t
>  VideoEngine::GenerateId() {
>    // XXX Something better than this (a map perhaps, or a simple boolean TArray, given
>    // the number in-use is O(1) normally!)
> -  return mId = sId++;
> +  return mId = sId++;;

s/;;/;/
Attachment #8916027 - Flags: review?(jib) → review+
Comment on attachment 8926211 [details]
Bug 1399413 - add mochitests to check multi-tabs gUM.

https://reviewboard.mozilla.org/r/197462/#review204376

lgtm.
Attachment #8926211 - Flags: review?(jib) → review+
Comment on attachment 8916027 [details]
Bug 1399413 - Make VideoEngine & VideoCaptureModule singletons.

https://reviewboard.mozilla.org/r/187290/#review204320

> I'm a bit confused by this comment. Is "WebRTC thread" and "VideoCapture thread" the same thing here?
> 
> Maybe s/gone/done/ and move the talk of deletion down into the if() where we conditionally delete the sVideoCaptureThread now?

> Is "WebRTC thread" and "VideoCapture thread" the same thing here?
IIRC, yes. But this comment was made by gcp.

> What does this solve that mClientCounter didn't?

As discussed in IRC, mClientCounter may lead to duplcated capnum.
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #67)
> ::: dom/media/systemservices/CamerasParent.cpp:202
> (Diff revisions 6 - 15)
> >    // Hold here until the WebRTC thread is gone. We need to dispatch
> >    // the thread deletion *now*, or there will be no more possibility
> >    // to get to the main thread.
> 
> I'm a bit confused by this comment. Is "WebRTC thread" and "VideoCapture
> thread" the same thing here?
> 
> Maybe s/gone/done/ and move the talk of deletion down into the if() where we
> conditionally delete the sVideoCaptureThread now?

See also bug 1407415 which deals with this code.
Pushed by mchiang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08ae44b6836c
Make VideoEngine & VideoCaptureModule singletons. r=jib
https://hg.mozilla.org/integration/autoland/rev/269e4e79e32c
add mochitests to check multi-tabs gUM. r=jib
https://hg.mozilla.org/mozilla-central/rev/08ae44b6836c
https://hg.mozilla.org/mozilla-central/rev/269e4e79e32c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1418331
We got a crash report when trying to join a WebRTC call on appear.in https://crash-stats.mozilla.com/report/index/b35bf60f-217c-4fea-8d16-e6dd90171121
Appears to crash in the code which landed here. Munro: can you please take a look at it?
Flags: needinfo?(mchiang)
Never mind. I didn't see that bug 1418331 got created for this crash already.
Flags: needinfo?(mchiang)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: