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

Status: RESOLVED WONTFIX

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Jean-Yves Avenard [:jya]]

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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Have we provided spec feedback that these APIs should be async?

Comment 3

[Jean-Yves Avenard [:jya]]

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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 5

[Jean-Yves Avenard [:jya]]

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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!