Closed Bug 1210852 Opened 6 years ago Closed 6 years ago

getUserMedia is doing capability enumeration on the main thread

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- wontfix
firefox42 - wontfix
firefox43 - affected
firefox44 --- fixed
Blocking Flags:

People

(Reporter: gcp, Assigned: jib)

References

Details

Attachments

(1 file)

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())
Assignee: nobody → jib
Blocks: 1070216
Bug 1210852 - do SelectSettings of device capabilities on media thread.
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: 6 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.