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)
Tracking
()
RESOLVED
FIXED
mozilla59
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)
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.
Assignee | ||
Comment 1•7 years ago
|
||
It seems Windows doesn't allow multiple processes access the same camera hardware simultaneously.
Assignee | ||
Comment 2•7 years ago
|
||
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.
Comment 3•7 years ago
|
||
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)
Updated•7 years ago
|
Rank: 13
Priority: -- → P1
Comment 4•7 years ago
|
||
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.
Comment 5•7 years ago
|
||
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Assignee | ||
Comment 6•7 years ago
|
||
(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)
Assignee | ||
Comment 7•7 years ago
|
||
I can also reproduce the issue on Windows 10.
OS: Windows 7 → Windows
Updated•7 years ago
|
Comment 9•7 years ago
|
||
[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
tracking-firefox57:
--- → ?
tracking-firefox58:
--- → ?
Flags: needinfo?(jib) → needinfo?(mchiang)
Keywords: regression
Comment 10•7 years ago
|
||
FWIW MediaParent shares a singleton if we're looking for code to model on [1].
[1] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/media/systemservices/MediaParent.cpp#41
Updated•7 years ago
|
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
Updated•7 years ago
|
Flags: needinfo?(gpascutto)
Comment 11•7 years ago
|
||
(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 | ||
Updated•7 years ago
|
Assignee: nobody → mchiang
Flags: needinfo?(mchiang)
Updated•7 years ago
|
Comment 12•7 years ago
|
||
(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.
Assignee | ||
Comment 14•7 years ago
|
||
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)
Comment 15•7 years ago
|
||
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 16•7 years ago
|
||
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.
Thank you everyone for weighing in the risk and reward on this one. Let's try to get this fixed in 58
Flags: needinfo?(rkothari)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
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 20•7 years ago
|
||
mozreview-review |
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]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
Revision 2: rebase & fix static analysis defect.
I have verified the patch in Windows.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•7 years ago
|
||
Revision 3: support desktop sharing in multiple tabs (content processes)
Comment 25•7 years ago
|
||
mozreview-review |
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]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•7 years ago
|
||
Revision 4: fix static analysis defect.
Remaining job:
Fix memory leak found in try server (https://treeherder.mozilla.org/#/jobs?repo=try&revision=09edf3fffbaf75052fde0d6a22c100c728d6bf16)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
Revision 4-6: fix memory leakage
Comment 31•7 years ago
|
||
mozreview-review |
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]
Assignee | ||
Comment 32•7 years ago
|
||
Comment 33•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8916027 -
Flags: review?(gpascutto)
Comment 34•7 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 35•7 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 36•7 years ago
|
||
mozreview-review |
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?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 39•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•7 years ago
|
||
Revision 8-9: rebase
Assignee | ||
Comment 42•7 years ago
|
||
Please ignore comment 41.
Revision 8-9: We need to make capnum unique in order to use it as an index in bug 1388667.
Assignee | ||
Updated•7 years ago
|
tracking-firefox58:
+ → ---
Comment 43•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 44•7 years ago
|
||
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)
Assignee | ||
Comment 45•7 years ago
|
||
(In reply to Munro Mengjue Chiang [:mchiang] from comment #44)
> CamerasParent will notify after VideoCapture thread has been created and
s/CamerasParent/CamerasParent ctor
Comment 46•7 years ago
|
||
(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 hidden (mozreview-request) |
Assignee | ||
Comment 48•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8916027 -
Flags: review?(jib)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 50•7 years ago
|
||
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.
Comment 51•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 53•7 years ago
|
||
Revision 11-12: fix Linux build error
Assignee | ||
Comment 54•7 years ago
|
||
I have verified the fiddles in comment 50 in Windows / Linux / OSX.
All of them work properly.
The next step is implementing the automation.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8925863 -
Attachment is obsolete: true
Attachment #8925863 -
Flags: review?(jib)
Comment hidden (mozreview-request) |
Comment 58•7 years ago
|
||
mozreview-review |
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 59•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 62•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 65•7 years ago
|
||
https://reviewboard.mozilla.org/r/187290/diff/14-15/
fix mochitest failure & rebase
https://reviewboard.mozilla.org/r/197462/diff/2-3/
rebase
Assignee | ||
Comment 66•7 years ago
|
||
Comment 67•7 years ago
|
||
mozreview-review |
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 68•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 69•7 years ago
|
||
mozreview-review-reply |
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.
Comment 70•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 75•7 years ago
|
||
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
Comment 76•7 years ago
|
||
bugherder |
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
Comment 77•7 years ago
|
||
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)
Comment 78•7 years ago
|
||
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.
Description
•