Closed
Bug 1209033
Opened 9 years ago
Closed 9 years ago
WebRTC getUserMedia with constraints leads to InternalError or hang in 43
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Tracking
()
RESOLVED
FIXED
backlog | webrtc/webaudio+ |
People
(Reporter: akvakh, Assigned: jib)
References
Details
(Keywords: hang)
Attachments
(5 files)
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
lizzard
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-review-board-request
|
lizzard
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-review-board-request
|
lizzard
:
approval-mozilla-beta+
|
Details |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36
Steps to reproduce:
1. Open web page using https (not to show devices access dialog in case of device change)
2. Use getUserMedia with constraints to select audio and video recording device (using deviceId)
3. Change audio device id only by passing new constraints to getUserMedia (leave video constraints the same)
Actual results:
errorCallback fired with name: InternalError , message: "Starting video failed”
Expected results:
successCallback fired
Updated•9 years ago
|
Component: Untriaged → WebRTC: Audio/Video
Product: Firefox → Core
Comment 1•9 years ago
|
||
Jib: please verify and update.
Reporter: Can you provide any more details or logs?
Flags: needinfo?(jib)
Flags: needinfo?(gingerbread_man)
Comment 2•9 years ago
|
||
Sorry, but I'm unable to test this (which is presumably what was requested). Clearing needinfo flag.
Flags: needinfo?(gingerbread_man)
Assignee | ||
Comment 3•9 years ago
|
||
Looks like the needinfo was meant for the reporter.
I'm unable to reproduce. Here's the fiddle I used: https://jsfiddle.net/77bcfthz/
Andrey, some questions:
1. does it work in http?
2. What OS is this on?
3. What audio devices do you have attached?
4. Is one of the audio devices the "built-in" version of the other, or are you specifically using two unrelated audio devices in your test?
5. Are you able to reproduce with the above fiddle?
Note to self: We should at least change InternalError to either NotReadableError or AbortError to be spec compliant.
Spec [1] says:
"If the user grants permission but a hardware error such as an OS/program/webpage lock prevents access, reject p with a new DOMException object whose name attribute has the value NotReadableError and abort these steps.
If the user grants permission but device access fails for any reason other than those listed above, reject p with a new DOMException object whose name attribute has the value AbortError and abort these steps."
[1] http://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-getusermedia
Flags: needinfo?(akvakh)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(jib)
Reporter | ||
Comment 4•9 years ago
|
||
Hello,
1. does it work in http?
Yes, but in case of http it shows dialog to choose the recording device.
2. What OS is this on?
Mac OS 10.11
3. What audio devices do you have attached?
Enum shows 3 devices: Default, Built-in Microphone and Display Audio (I have Cinema Display connected to my macbook)
4. Is one of the audio devices the "built-in" version of the other, or are you specifically using two unrelated audio devices in your test?
It doesn't matter, it's related to switch between any 2 devices
5. Are you able to reproduce with the above fiddle?
Fiddle works well. But this app doesn't https://demos.zingaya.com/rtctests/index.html?username=testuser&pwd=testpass&appname=rtctest&accname=aylarov , just allow access and then click "Enable Self View" and then change audio recording device in combobox
Flags: needinfo?(akvakh)
Assignee | ||
Comment 5•9 years ago
|
||
The problem doesn't reproduce for me in your app (I'm on OSX 10.9.5. and tried FF41 and 42). Can you modify the fiddle until it breaks for you?
Also, if you could test this in Firefox Nightly, that would be great (we fixed a race there that may be related - bug 1103188 comment 92).
Flags: needinfo?(akvakh)
Reporter | ||
Comment 6•9 years ago
|
||
Updated to the latest version of Nightly, the problem is still there, I recorded video:
https://www.youtube.com/watch?v=tWWBaJF3D6U&feature=youtu.be
Flags: needinfo?(akvakh)
Assignee | ||
Comment 7•9 years ago
|
||
I still can't reproduce. I tried to follow the code, but got lost. If you could reproduce it by modifying the minimal fiddle in comment 3 that would help a lot.
(Btw I recommend the actively developed adapter.js, the official WebRTC polyfill, to abstract out browser differences, as it lets you program against the latest standard, with simpler code on your end. https://github.com/webrtc/adapter/blob/master/adapter.js )
This looks to me like either:
A) A race in our code or in the hardware device between calling stream.stop() on the previous gum stream and obtaining the new one. Have you tried putting in a 200 ms delay? It would be interesting to see if it helps. You could also try to switch from stream.stop() to stream.getTracks().forEach(track => track.stop()), which is the new standard, and another difference from the fiddle.
I wonder, does it reproduce in http if you select the device in the permission prompt to match the device you select in-content? (sorry, we have bug 1213046 which is messing up the default choice here). Btw, you can avoid offering the redundant user-choice in the first place by using the 'exact' keyword in constraints [1] (available if you use adapter.js).
If you could try that it would rule out a race, and it might instead be...
B) a hardware error of some kind, and the spec allows us to return NotReadableError when that happens.
Maybe it's not liking combinations of video and audio from different devices? But in your video it
looks like you're selecting the same one. hmm.
The error comes from here:
http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp?rev=8f665fef765b&mark=279-279#254
[1] https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getUserMedia#Parameters
Reporter | ||
Comment 8•9 years ago
|
||
It's not B) , since if I change the device one more time after the error it works well.
Looks like I found the reason - I changed MediaStream.stop to track.stop and it doesn't happen anymore.
Comment 9•9 years ago
|
||
(In reply to Andrey Kovalenko from comment #8)
> Looks like I found the reason - I changed MediaStream.stop to track.stop and
> it doesn't happen anymore.
FYI - could this be the order-of-calling-stop issue we recently discovered with pehrson's patches?
Flags: needinfo?(jib)
Assignee | ||
Comment 10•9 years ago
|
||
FWIW the order didn't matter. That turned out to be a pilot error on my part.
Flags: needinfo?(jib)
Comment 11•9 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #10)
> FWIW the order didn't matter. That turned out to be a pilot error on my part.
Where does this stand?
Assignee | ||
Comment 13•9 years ago
|
||
I was able to modify the fiddle to reproduce it: https://jsfiddle.net/jib1/nh3pp8g2/ It doesn't happen on every run, but 30-60% of the time, so reliably enough to find a regression-range.
It happens in 42 and 43 (and probably earlier back to 39), but was fixed in 44 here: https://hg.mozilla.org/mozilla-central/rev/2f4ae4d923bc which I verified by manual regression testing (mozregression isn't working for me atm).
I got it working in 43 with the following patches (I ran into Bug 1210852 with this test, a deadlock, which should be uplifted IMHO):
> 2015-10-14 11:27:15 +0800 | pehrsons: Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib
> 2015-10-03 20:42:26 -0400 | jib: Bug 1210852 - do SelectSettings of device capabilities on media thread.
> 2015-09-30 00:39:38 +0800 | pehrsons: Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib
> 2015-09-30 00:39:08 +0800 | pehrsons: Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib
Assignee: nobody → jib
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(jib)
Assignee | ||
Comment 14•9 years ago
|
||
[Tracking Requested - why for this release]:
This request is for pehrsons's patches (3 of the 4 patches mentioned in comment 13), because we cherry-picked them from huge patch sets in those bugs mentioned (and I've flagged Bug 1210852 - the hang - separately since it should be considered on its own).
The regression here (the non-hang-part) is that the camera does not come on in all situations when it should. The STR is to have multiple back-to-back gUM requests with persistent permissions granted (a timing issue).
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox43:
--- → affected
status-firefox44:
--- → unaffected
tracking-firefox42:
--- → ?
tracking-firefox43:
--- → ?
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 15•9 years ago
|
||
To clarify: this bug has no patches, but requests uplift of cherry-picked patches from other bugs.
The patches don't apply cleanly, so I tried to push my patch-set to mozreview from my local beta, but stopped when it started preparing 1000s of commits. Am I doing something wrong?
Comment 16•9 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #15)
> To clarify: this bug has no patches, but requests uplift of cherry-picked
> patches from other bugs.
>
> The patches don't apply cleanly, so I tried to push my patch-set to
> mozreview from my local beta, but stopped when it started preparing 1000s of
> commits. Am I doing something wrong?
Use splinter ;-) File a bug on Mozreview
Comment 17•9 years ago
|
||
Updated status/tracking. Not changing the 43 entries until I look at what patches we're considering, how changed they are, and see some try results with them. (Since these are being cherry-picked out of much larger sets.)
backlog: --- → webrtc/webaudio+
Rank: 15
tracking-firefox42:
? → ---
Flags: needinfo?(jib)
Priority: -- → P1
Assignee | ||
Comment 18•9 years ago
|
||
Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib
Assignee | ||
Comment 19•9 years ago
|
||
Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib
This is needed to avoid something like:
* [old stream] stop track 1 -> deallocate MediaDevice for track 1
* [new stream] gUM() -> allocate MediaDevice for track 1
* [old stream] stop stream -> deallocate MediaDevice for track 1
* [new stream] gUM() -> start MediaDevice for track 1 (oops, MediaDevice was no more!)
Assignee | ||
Comment 20•9 years ago
|
||
Bug 1210852 - do SelectSettings of device capabilities on media thread.
Assignee | ||
Comment 21•9 years ago
|
||
Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib
We currently avoid Deallocating a CaptureDevice used for multiple gUMStreams
when one of them calls Deallocate() by keeping track of how many called
Start().
The normal lifetime sequence however, is:
Allocate()
Start()
Stop()
Deallocate()
This patches fixes the lifetime management by keeping track of how many
users of the CaptureDevice called Allocate().
Assignee | ||
Comment 22•9 years ago
|
||
I asked on #mozreview, and was told not to be alarmed, so I went ahead, and the patches for beta are above. I launched a try run in mozreview.
Andreas, could you look over these patches for any missing dependencies or potential problems for beta?
Flags: needinfo?(rjesup)
Flags: needinfo?(pehrsons)
Flags: needinfo?(jib)
Comment 23•9 years ago
|
||
I don't really want to vouch for moving SelectSettings to the main thread, haven't been involved much there. For the rest I think we're good. That code has been pretty stable since beta.
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 24•9 years ago
|
||
Of course. I'm vouching for that one.
Assignee | ||
Updated•9 years ago
|
Summary: WebRTC getUserMedia with constraints leads to InternalError → WebRTC getUserMedia with constraints leads to InternalError or hang in 43
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8681981 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.
Approval Request Comment
[Feature/regressing bug #]: Bug 1046245
[User impact if declined]:
Calling getUserMedia twice in a row with persistent permission (i.e. without a permission prompt), where the first call succeeded, leads to a race which depending on timing can cause the second stream to either fail with InternalError or deadlock between the main thread and the mediaManager thread. The deadlock seems to happen in non-e10s mode.
The deadlock is fixed by this patch which is from Bug 1210852 and rebased for beta.
[Describe test coverage new/current, TreeHerder]:
Bug 1210852 landed in Nightly almost a month ago and seems to be working well, passing all existing tests.
[Risks and why]:
Low. The code change follows an established pattern in this file for thread dispatching over to the media thread, where the code was always meant to be run (and used to run before 39).
[String/UUID change made/needed]: none
Attachment #8681981 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8681982 [details]
MozReview Request: Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib
(This request is for the remaining 3 patches fixing the InternalError.)
Approval Request Comment
[Feature/regressing bug #]: Bug 1046245
[User impact if declined]:
Calling getUserMedia twice in a row with persistent permission (i.e. without a permission prompt), where the first call succeeded, leads to a race which depending on timing can cause the second stream to fail with InternalError.
The three patches are from Bug 1103188 and Bug 1070216.
[Describe test coverage new/current, TreeHerder]:
Bug 1103188 landed in Nightly over a month ago, and Bug 1070216 half-a-month ago, and seem to be working well, passing all existing and new tests. These cherry-picked patches are vouched for by pehrsons in comment 23 as being simple and standalone from the other patches in those bugs (they were problems discovered along the way of an implementation feature).
[Risks and why]:
Low. The first two patches fixed some faulty assumptions about when streams are marked as stopped in comment 19. The third patch fixes the tracking code for when resources for track members can be relinquished (opencount in Start/Stop when it should have been Allocate/Deallcoate) in comment 21.
All of these a bugs in their own right that contributed to this resource overlap problem.
[String/UUID change made/needed]: none
Attachment #8681982 -
Flags: approval-mozilla-beta?
Tracking for 43 and 44 for now. I'll look at uplifting this next week or the end of this week once we have beta 1 in place.
Comment 28•9 years ago
|
||
The patches we've requested uplift on (to Beta - Fx43) are already in Fx44. So I'm clearing tracking-firefox44. Also, I think all the needinfo's have been answered; so I'm clearing the remaining one.
tracking-firefox44:
+ → ---
Flags: needinfo?(rjesup)
Comment on attachment 8681981 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.
Fix for hang and other video issues. OK to uplift to beta.
Attachment #8681981 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8681982 [details]
MozReview Request: Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib
OK to uplift to beta, this is the 2nd of 4 patches to fix a video/audio hang.
Attachment #8681982 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Wes, can you uplift these and the other two patches attached here that jib rebased for beta. Thanks.
Flags: needinfo?(wkocher)
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/b199ea9f265c
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/1babaebeccc6
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/c458ad434a12
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/3705005d2190
Flags: needinfo?(wkocher)
Backed out in https://hg.mozilla.org/releases/mozilla-beta/rev/4e86a0d1261a for various crashes:
https://treeherder.mozilla.org/logviewer.html#?job_id=628390&repo=mozilla-beta
https://treeherder.mozilla.org/logviewer.html#?job_id=628244&repo=mozilla-beta
https://treeherder.mozilla.org/logviewer.html#?job_id=627973&repo=mozilla-beta
https://treeherder.mozilla.org/logviewer.html#?job_id=627999&repo=mozilla-beta
Flags: needinfo?(jib)
Assignee | ||
Comment 35•9 years ago
|
||
Comment on attachment 8681979 [details]
MozReview Request: Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23937/diff/1-2/
Attachment #8681979 -
Flags: review?(jib)
Assignee | ||
Updated•9 years ago
|
Attachment #8681980 -
Flags: review?(jib)
Assignee | ||
Comment 36•9 years ago
|
||
Comment on attachment 8681980 [details]
MozReview Request: Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23939/diff/1-2/
Assignee | ||
Comment 37•9 years ago
|
||
Comment on attachment 8681981 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23941/diff/1-2/
Assignee | ||
Updated•9 years ago
|
Attachment #8681982 -
Flags: review?(jib)
Assignee | ||
Comment 38•9 years ago
|
||
Comment on attachment 8681982 [details]
MozReview Request: Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23943/diff/1-2/
Assignee | ||
Comment 39•9 years ago
|
||
Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib
Attachment #8685107 -
Flags: review?(jib)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(jib)
Attachment #8681979 -
Flags: review?(jib)
Assignee | ||
Updated•9 years ago
|
Attachment #8681980 -
Flags: review?(jib)
Assignee | ||
Updated•9 years ago
|
Attachment #8681982 -
Flags: review?(jib)
Assignee | ||
Updated•9 years ago
|
Attachment #8685107 -
Flags: review?(jib)
Assignee | ||
Comment 40•9 years ago
|
||
Comment on attachment 8685107 [details]
MozReview Request: Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib
I forgot to stay on top of try's here, sorry. Got wrong thread asserts:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=3705005d2190
We need one more patch (this one) from Bug 1103188 to fix this. Doing new try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f98856a73ac
Andreas, please let me know if you see any problems with using this patch on beta.
Worst case, I also did a try with just the lone Bug 1210852 patch (the hang):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=804a226b61b0
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 41•9 years ago
|
||
Comment on attachment 8681979 [details]
MozReview Request: Bug 1103188 - Keep track of capture stop only in gUM stream listener. r?jib
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23937/diff/2-3/
Assignee | ||
Comment 42•9 years ago
|
||
Comment on attachment 8681980 [details]
MozReview Request: Bug 1103188 - Keep track of stopped tracks in gUM stream listener. r?jib
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23939/diff/2-3/
Assignee | ||
Comment 43•9 years ago
|
||
Comment on attachment 8681982 [details]
MozReview Request: Bug 1070216 - Properly manage lifetime of allocated CaptureDevices. r?jib
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23943/diff/2-3/
Assignee | ||
Comment 44•9 years ago
|
||
Comment on attachment 8685107 [details]
MozReview Request: Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24729/diff/1-2/
Assignee | ||
Comment 45•9 years ago
|
||
Comment on attachment 8685107 [details]
MozReview Request: Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib
Fixed kdiff error.
Assignee | ||
Comment 46•9 years ago
|
||
New try will all patches https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ed2d8b2281c
Assignee | ||
Comment 48•9 years ago
|
||
Comment on attachment 8685107 [details]
MozReview Request: Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib
See comment 26.
Last beta landing bounced w/oranges about NotifyFinished being on the wrong thread, because we left out this patch in our cherry-picking from bug 1103188 which fixes that.
Try seems fine now in comment 46 (though it's hard to know what passes for green these days with a lot of oranges still, but they seem unrelated).
Attachment #8685107 -
Flags: approval-mozilla-beta?
Comment on attachment 8685107 [details]
MozReview Request: Bug 1103188 - Always call MediaManager::NotifyFinished/NotifyRemoved on main thread. r?jib
Let's try uplifting to beta again for the beta 4 build today.
Attachment #8685107 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/1540124e58cd
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/e3fad0bd414e
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/1ffe42de58bd
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/98d9576c7d13
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/5ca6857c26e5
Assignee | ||
Comment 52•9 years ago
|
||
Yes, thanks for catching that it wasn't.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: needinfo?(jib)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•