1210852 - getUserMedia is doing capability enumeration on the main thread

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Gian-Carlo Pascutto [:gcp]]

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())

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1210852 - do SelectSettings of device capabilities on media thread.

Bug 1210852 - do SelectSettings of device capabilities on media thread.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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...

Comment 3

[Andreas Pehrson [:pehrsons]]

(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.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Yeah I get that too. I messed something up clearly. Will take a look later today.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 8

[Randell Jesup [:jesup] (needinfo me)]

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 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 13

[Randell Jesup [:jesup] (needinfo me)]

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

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

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04f486e2fdfc

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/04f486e2fdfc

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

[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.

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

A patch rebased for beta is available in Bug 1209033 comment 20.

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Beta uplift request made in Bug 1209033.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Not tracking for 42 as we released and I don't think it is worst shipping in a dot release.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Not tracking this for 43 either, since the fix for it is uplifting in bug 1209033.