Closed Bug 1674043 Opened 4 years ago Closed 4 years ago

Never call RemoteDecoderManagerChild::Supports on the main thread

Categories

(Core :: Audio/Video: Playback, defect, P1)

defect

Tracking

()

RESOLVED FIXED
84 Branch
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.

P1 because we can potentially completely make the browser unresponsive if a11y services are enabled.

Severity: -- → S1
Priority: -- → P1

Set release status flags based on info from the regressing bug 1595994

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.

Assignee: nobody → jyavenard
Status: NEW → ASSIGNED

Depends on D95240

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

Should we compile without ffvpx support, we would get compilation error otherwise.

Depends on D95242

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

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

Regressions: 1674685
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/628a9eab4a5b P1. Call PDMFactory::Support off the main thread. r=bryce https://hg.mozilla.org/integration/autoland/rev/370bc9020130 P2. Remove stray header declaration. r=mattwoodrow https://hg.mozilla.org/integration/autoland/rev/38a978b0d588 P3. Early reject AV1 in mp4 decoding if decoder is turned off. r=bryce https://hg.mozilla.org/integration/autoland/rev/b3f458896a5f P4. Always have the ffvpx StaticPref. r=bryce https://hg.mozilla.org/integration/autoland/rev/33563a16a5c3 P5. Don't attempt to synchronize preferences until ContentParent has compleded initialization. r=nika https://hg.mozilla.org/integration/autoland/rev/3f08ccb1f141 P6. Remove sync PDecoderMananger::Supports API. r=mattwoodrow,bryce,mjf,ipc-reviewers,nika
Regressions: 1681043
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: