MP4Decoder calls blocking TaskQueue::AwaitShutdownAndIdle() on main thread

RESOLVED WONTFIX

Status

()

Core
Audio/Video: Playback
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: bkelly, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

2 years ago
The TaskQueue::AwaitShutdownAndIdle() call blocks on a monitor until the TaskQueue is drained.  AFAICT MP4Decoder uses it on the main thread at these call sites:

  https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Decoder.cpp#257
  https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Decoder.cpp#279
  https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Decoder.cpp#285

Its also used in a few places in media where its not clear what thread its on:

  https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#228
  https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/omx/OmxDataDecoder.cpp#252

In general calling a blocking method on the main thread is very bad for browser responsiveness.  It seems this code should be using the ShutdownPromise instead.
Flags: needinfo?(bobbyholley)
Component: Audio/Video → Audio/Video: Playback
Those functions needs to be synchronous. It's the unfortunate downside of the canPlayType JS API. 

We need to know if the underlying media foundation has support for what we're about to play; and the media framework is mostly asynchronous. Hence the blocking.
(Reporter)

Comment 2

2 years ago
Have we provided spec feedback that these APIs should be async?
There's a few of those HTML5 media element canPlayType or MediaSource isTypeSupported; I don't see why you would want those API to be async simply because of our backend implementation.
(Reporter)

Comment 4

2 years ago
Well I assume these operations are being performed OMT because they do I/O or other blocking badness.  Is that not the case?

If its impractical to implement these sync APIs without blocking, then we should suggest the spec provide an async method instead so we can discourage the use of the sync APIs.  Just like we've built IDB to be async and discourage localStorage use.

Anyway, thats just my opinion.
Flags: needinfo?(bobbyholley)
(In reply to Ben Kelly [:bkelly] from comment #4)
> Well I assume these operations are being performed OMT because they do I/O
> or other blocking badness.  Is that not the case?

Actually, it's more to do with how the underlying media framework works.
Most of them are async to start with.

In regards to detecting if the decoding is HW accelerated, we can often only do so once we've decoded a frame.

But having said that, the results are cached, that code only ever runs once. The next call to it will use the cache result.
We even launch a task 1 minute after startup, when the browser is idle to check and populate the cached results.

So the next call to canPlayType will not block anything.
As such the impact of being synchronous is low IMHO.
(Reporter)

Comment 6

2 years ago
Ok.  Sounds good to me.  I guess a comment would be nice to avoid people copying this to other places by accident.

Thanks for the explanation!
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.