Stop using TYPE_MOZILLA_NONMAINUITHREAD thread type for the background media manager thread

RESOLVED FIXED in Firefox 68

Status

()

defect
P1
normal
RESOLVED FIXED
3 months ago
Last month

People

(Reporter: jimm, Assigned: jimm)

Tracking

(Blocks 2 bugs)

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 months ago

This was introduced back in 2014 when we were doing everything in content, including capturing window contents and various other UI type activity.

We should be able to switch to TYPE_MOZILLA_NONMAINTHREAD here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db8054e9f57e6c2947c68e158138dc07b90dbe44

Assignee

Comment 1

3 months ago

s/everything in content/everything in a single app/

Assignee

Comment 2

3 months ago

Pushed a try build with asserts in a couple media related native event processing routines - didn't get any hits for content. I think it's safe to remove the ui loop here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6876287fb194c18a96d5f922c0671913c1577fd

Assignee

Updated

3 months ago
Assignee: nobody → jmathies
Priority: -- → P1
Assignee

Updated

3 months ago
Blocks: 1381022

jimm - so the mediamanager thread is made a non-UI thread by this, but it appears to do so universally, not just in the content process. The main question is if this affects GetUserMedia streams initiated by Chrome content (which Hello used to do), and if this affects the remote (in the Master process) capture of screen data on behalf of the Content process.

Someone should set up these two experiments (screen capture from content, and screen capture from Chrome, which would require code insertion to do) and verify if the actual calls that caused us to switch to a UI thread are either a) being done from a UI thread, or b) aren't being done anymore. The original failures that required this were very random and not easily reproducible.

Flags: needinfo?(jmathies)
Flags: needinfo?(jib)
Flags: needinfo?(dminor)

We have tests in the tree for verifying that screen capture from content works. Screen capture is easier to test than cameras.

Our screen-capture prompts' preview area is implemented using screen capture from chrome, i.e. verifiable through manual testing—We have automated tests for these capture calls succeeding, but not for verifying that actual pixels appear. In any case, it should be doable without code insertion.

I don't know which calls specifically needed to be on the UI thread, maybe others recall?

Flags: needinfo?(jib)
Assignee

Comment 6

2 months ago

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #5)

We have [tests in the
tree](https://searchfox.org/mozilla-central/rev/
d302c3058330a57f238be4062fddea629311ce66/dom/media/tests/mochitest/
test_getUserMedia_basicScreenshare.html#100,119) for verifying that screen
capture from content works. Screen capture is easier to test than cameras.

Our screen-capture prompts' preview area is implemented using screen capture
from chrome, i.e. verifiable through manual testing—We have automated tests
for these capture calls succeeding, but not for verifying that actual pixels
appear. In any case, it should be doable without code insertion.

I don't know which calls specifically needed to be on the UI thread, maybe
others recall?

I don't think any of these calls need to be on a ui thread. We originally implemented this thinking some of this capture stuff needed a ui pump. After implementing it we realized the real culprit was the compositor background thread, which created windows but didn't pump.

Flags: needinfo?(jmathies)

Even with these changes, the desktop capture code will still be running on a UI Thread due to our PlatformUIThread implementation [1]. As far as I know, all capture calls still end up spinning up that thread, so I think these changes should be safe.

[1] https://searchfox.org/mozilla-central/rev/ee3905439acbf81e9c829ece0b46d09d2fa26c5c/dom/media/systemservices/video_engine/desktop_capture_impl.cc#411

Flags: needinfo?(dminor)

Comment 8

Last month
Pushed by jmathies@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13d43fe66cae
Switch to using TYPE_MOZILLA_NONMAINTHREAD for MediaManager. r=jesup

Comment 9

Last month
bugherder
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.