Use MediaPromises to make MediaDecoderReader::Shutdown async

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla37
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(1 attachment)

cpearce and I hatched this plan last night.

The basic problem is that shutdown is one long process that blocks on all kinds of things. This means that it doesn't play well with any asynchronous operations that might want to happen during shutdown (such as rejecting other promises), because everything has been torn down by the time those async operations round-trip through the event loop.

Since we're trying to make everything more asynchronous, it seems reasonable to try to make Shutdown async as well. Luckily, we have just the right tools to do that now. I've got mostly-working patches.
Depends on: 1108767
Attachment #8533419 - Flags: review?(cpearce)
Comment on attachment 8533419 [details] [diff] [review]
Make reader shutdown asynchronous. v1

Review of attachment 8533419 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: dom/media/MediaTaskQueue.h
@@ +19,5 @@
>  namespace mozilla {
>  
>  class SharedThreadPool;
>  
> +typedef MediaPromise<bool, bool> ShutdownPromise;

You never resolve a ShutdownPromise with anything other than "true", yet you always assert that true is passed to the resolve/reject method, so why have a bool resolve type anyway? Is it analogous to resolving a JS promise with "undefined"? i.e. an artifact to give the type system something to pass around?

@@ +51,5 @@
>  
>    // Puts the queue in a shutdown state and returns immediately. The queue will
>    // remain alive at least until all the events are drained, because the Runners
>    // hold a strong reference to the task queue, and one of them is always held
>    // by the threadpool event queue when the task queue is non-empty.

Comment should mention that the promise resovles when the last task in the queue has completed.

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +250,5 @@
> +MediaSourceReader::ContinueShutdown(bool aSuccess)
> +{
> +  MOZ_ASSERT(aSuccess);
> +  if (mTrackBuffers.Length()) {
> +    mTrackBuffers[0]->Shutdown()->Then(GetTaskQueue(), __func__, this,

It would be nice if we had a Promises.All() equivalent here. Please don't feel obliged to go write one before landing though!
Attachment #8533419 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #3)
> You never resolve a ShutdownPromise with anything other than "true", yet you
> always assert that true is passed to the resolve/reject method, so why have
> a bool resolve type anyway? Is it analogous to resolving a JS promise with
> "undefined"? i.e. an artifact to give the type system something to pass
> around?

Correct. I'm open to better solutions, I just couldn't think of one that didn't involve a lot of code duplication.

> Comment should mention that the promise resovles when the last task in the
> queue has completed.

Will do.

> ::: dom/media/mediasource/MediaSourceReader.cpp
> @@ +250,5 @@
> > +MediaSourceReader::ContinueShutdown(bool aSuccess)
> > +{
> > +  MOZ_ASSERT(aSuccess);
> > +  if (mTrackBuffers.Length()) {
> > +    mTrackBuffers[0]->Shutdown()->Then(GetTaskQueue(), __func__, this,
> 
> It would be nice if we had a Promises.All() equivalent here. Please don't
> feel obliged to go write one before landing though!

Yeah I pondered that too. It would also be nice if we could destroy these objects off-main-thread - do you know of any actual blockers for doing this? The only ones I ran into in practice were a few objects with non-threadsafe refcounts.

If we had both of those things, we could make this very elegant and parallel.
(In reply to Bobby Holley (:bholley) from comment #5)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4e3f6b6cabfb

This was green modulo b2g, which I've now fixed here:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=682083200c4a

Filing a followup to make the MediaOmxReader situation a little bit nicer.
Blocks: 1109216
Relanded because it wasn't the cause of the leaks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/96b86345fe30
https://hg.mozilla.org/mozilla-central/rev/96b86345fe30
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Blocks: 1109954
Depends on: 1113282
Comment on attachment 8533419 [details] [diff] [review]
Make reader shutdown asynchronous. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, sites more likely to serve flash video.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Low. This affects normal media playback, but has been working fine on nightly for two weeks now and the new design is more robust.
[String/UUID change made/needed]: None.
Attachment #8533419 - Flags: approval-mozilla-aurora?
Attachment #8533419 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.