Never call RemoteDecoderManagerChild::Supports on the main thread
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr78 | --- | unaffected |
firefox81 | --- | unaffected |
firefox82 | --- | unaffected |
firefox83 | --- | unaffected |
firefox84 | + | fixed |
People
(Reporter: jya, Assigned: jya)
References
(Regressed 1 open bug, Regression)
Details
(Keywords: regression)
Attachments
(6 files)
In bug 1595994 ; by design we moved the decision to check if a particular decoder was available to the process where said decoder will run.
So if in JS canPlayType was called; we would query via PRemoteDecoderManager if such decoder was available through the RemoteDecoderManagerChild::Support
With the RDD process being launched on demand, the first time you would call RemoteDecoderManagerChild::Support it would launch the RDD process.
Launching the RDD process is a sync method.
The primary reason for moving the decision on which codec we can play to the PRemoteDecoderManager is for the elegance in the design, there's a single actor who knows it all. We can determine fully if the codec is indeed available in that particular process.
We had identified potential of deadlock with the main thread before, and moved the launch RDD method to the PBackground protocol. Unfortunately this isn't sufficient and the architecture chosen in bug 1595994 doesn't scale to real human's usage (it worked for me, but I'm an AI drone).
So we have the main thread, blocking on JS' canPlayType waiting for the sync IPC EnsureRDDProcessAndCreateBridge to complete.
In bug 1673565 we see bugs where the parent process has shutdown, the PBackground is being torn down, but the main thread being blocked has no ability to process that message and EnsureRDDProcessAndCreateBridge waiting forever.
In bug 1673192 we see the parent process background thread, waiting for a sync dispatch to launch the RDD while a11y is itself doing a sync call to the content process awaiting the main thread to complete its current action.
We have discussed various strategies on how to bypass this problem,
One is bug 1672072 which makes the creation of decoder asynchronous; while nice it wouldn't fully resolve the two problems seen above (just make them less likely)
The other is caching the result of RemoteDecoderManagerChild::Support and sync this cache across all content processes.
It's a nice solution, but also rather complex to implement, and wouldn't fix the above issues fully; they could still happen, albeit very rarely.
So the current thought is a simple one. The only case where we need to know for sure if a codec is available or not is for those relying on an external framework: currently AAC and H264.
We only need to know if we can probably play them.
We can determine this information on startup in the parent process, and pass that information to the content processes as they are launched.
Supports will reply true immediately when called in the parent process as we only need to know if the framework is available.
MediaCapabilities is also running on the main thread, and requires more details such as if a particular resolution is available or not, or if hardware decoding is available.
So this needs to be moved outside the main thread; luckily MediaCapabilities is an async API so it's a straightforward thing to do.
Assignee | ||
Comment 1•4 years ago
|
||
P1 because we can potentially completely make the browser unresponsive if a11y services are enabled.
Updated•4 years ago
|
Comment 2•4 years ago
|
||
Set release status flags based on info from the regressing bug 1595994
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
In a coming change, when called on the main thread Supports will be made to return a true/false based on only the codec mimetype. We need a greater level of details with MediaCapabilities.
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
Depends on D95240
Assignee | ||
Comment 6•4 years ago
|
||
There's no need to start the whole PDMFactory machinery if we know it's already disabled.
Additionally, fix tests making assumptions on the changes above.
Depends on D95241
Assignee | ||
Comment 7•4 years ago
|
||
Should we compile without ffvpx support, we would get compilation error otherwise.
Depends on D95242
Assignee | ||
Comment 8•4 years ago
|
||
Initialization of the ContentParent could modify some preferences, and we wouldn't be ready to send them to the ContentChild yet.
So we split a ALIVE into another INITIALIZED state to distinguist the two.
Depends on D95243
Assignee | ||
Comment 9•4 years ago
|
||
In bug 1595994 we attempted to streamline the ability to determine which decoder was available regardless of the process they would be running in. This was subsequently done via the PDMFactory.
As there are several JS API that can query which codec are supported, it requires a synchronous mechanism.
Having a synchronous IPC call to the RemoteDecoderManagerParent has too many caveats to be workable.
So what we do instead is first determine at launch if the required external framework are available and pass this information to each content process.
When checking if a decoder is available, we make a best guess at determining if the PDM would support such codec, without actually loading such framework when running in the content process.
As such PDM::CreateAudio/VideoDecoder using an optional system framework now need to further check the validity of the CreateDecoderParam argument.
Depends on D95244
Comment 10•4 years ago
|
||
Comment 11•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/628a9eab4a5b
https://hg.mozilla.org/mozilla-central/rev/370bc9020130
https://hg.mozilla.org/mozilla-central/rev/38a978b0d588
https://hg.mozilla.org/mozilla-central/rev/b3f458896a5f
https://hg.mozilla.org/mozilla-central/rev/33563a16a5c3
https://hg.mozilla.org/mozilla-central/rev/3f08ccb1f141
Updated•4 years ago
|
Description
•