Closed
Bug 1108707
Opened 9 years ago
Closed 9 years ago
Use MediaPromises to make MediaDecoderReader::Shutdown async
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(1 file)
33.49 KB,
patch
|
cpearce
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4bd4053edb6e
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8533419 -
Flags: review?(cpearce)
Comment 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
(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.
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4e3f6b6cabfb
Assignee | ||
Comment 6•9 years ago
|
||
(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.
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/18c302f80bce
Comment 8•9 years ago
|
||
Backed out for nsTArray_base leaks. https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4474474&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/38478b0b00fb
Relanded because it wasn't the cause of the leaks. https://hg.mozilla.org/integration/mozilla-inbound/rev/96b86345fe30
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/96b86345fe30
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 11•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
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.
Description
•