getUserMedia is doing capability enumeration on the main thread

RESOLVED FIXED in Firefox 44

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gcp, Assigned: jib)

Tracking

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 wontfix, firefox42- wontfix, firefox43- affected, firefox44 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

4 years ago
https://bugzilla.mozilla.org/show_bug.cgi?id=1070216#c32

<jesup> so why is it on MainThread?
<jesup> MediaManager.cpp:1441
<jesup> So, that lambda then DispatchToMainThread()s to resolve the pledges... let's see if that's it
<jesup> Yeah, it's in the Pledge stuff
<jesup> Aha.  In processing the Fitness distance code, it does  size_t num = NumCapabilities(); - but it's now back on mainthread
<jesup> It needs to cache the capabilities while still on the MediaManager thread I think, and just use them in the thing we send back to MainThread
<jesup> Ok, we need a bug on this, assigned to jib.  And we should add thread-asserts to a few more MediaEngine APIs (at least !Is_MainThread())
Reporter

Updated

4 years ago
Assignee: nobody → jib
Blocks: 1070216
https://reviewboard.mozilla.org/r/21215/#review19073

Should work, but not done.

::: dom/media/MediaManager.cpp:1147
(Diff revision 1)
> +  MediaManager::PostTask(FROM_HERE, NewTaskFrom([id, aConstraints,
> +                                                 aSources]() mutable {

Note to self: need to add reference counting or something here, since the algorithm modifies the passed-in array. The caller keeps it alive normally, but if things are torn down at the wrong time...
(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> https://reviewboard.mozilla.org/r/21215/#review19073
> 
> Should work, but not done.

Just tested, the deadlock is gone! Thanks :-)

It seems like I get an audio track when I request video-only gUM() though, but I guess you're on top of that.
Yeah I get that too. I messed something up clearly. Will take a look later today.
Summary: EnumerateRawDevices is doing capability enumeration on the main thread → getUserMedia is doing capability enumeration on the main thread
Component: Audio/Video → WebRTC
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
Comment on attachment 8669486 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.

Bug 1210852 - do SelectSettings of device capabilities on media thread.
Attachment #8669486 - Flags: review?(rjesup)
Comment on attachment 8669486 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.

Bug 1210852 - do SelectSettings of device capabilities on media thread.
Comment on attachment 8669486 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.

Bug 1210852 - do SelectSettings of device capabilities on media thread.
Attachment #8669486 - Flags: review?(rjesup) → review+
Comment on attachment 8669486 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.

https://reviewboard.mozilla.org/r/21215/#review19259

::: dom/media/systemservices/MediaUtils.h:325
(Diff revision 4)
> +/* media::Refcountable - Add threadsafe ref-counting to something that hasn't.

hasn't -> isn't

Consider proposing this for mfbt somewhere
Comment on attachment 8669486 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.

Bug 1210852 - do SelectSettings of device capabilities on media thread.
Comment on attachment 8669486 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.

Bug 1210852 - do SelectSettings of device capabilities on media thread.
Comment on attachment 8669486 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.

Thanks! I've incorporated your feedback, and I also did some minor changes to the tests for whether MediaManager is still around in the callbacks.

If you could take another look that would be great.
Attachment #8669486 - Flags: review+ → review?(rjesup)
Comment on attachment 8669486 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.

Bug 1210852 - do SelectSettings of device capabilities on media thread.
Comment on attachment 8669486 [details]
MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.

https://reviewboard.mozilla.org/r/21215/#review19289
Attachment #8669486 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/04f486e2fdfc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
[Tracking Requested - why for this release]:

It deadlocks. Bug 1209033 comment 13 demonstrates this in the wild, so we should probably uplift this if we can. This is likely a regression from 39.
A patch rebased for beta is available in Bug 1209033 comment 20.
Beta uplift request made in Bug 1209033.
Not tracking for 42 as we released and I don't think it is worst shipping in a dot release.
Not tracking this for 43 either, since the fix for it is uplifting in bug 1209033.
You need to log in before you can comment on or make changes to this bug.