1108701 - Use MediaPromises for MediaDecoderReader::Request{Audio,Video}Data

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

I decided to split this out of bug 1097823 so that we can get the basic infrastructure landed. The stuff that caused the failures on bug 1097823 will move to this patch.

Comment 1

[Bobby Holley (:bholley)]

Absorbing the deps from bug 1097823.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Use MediaPromises for RequestAudioData and RequestVideoData. v3 r=cpearce

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1.5 - Stop sharing promise members between subclass and superclass. v1

This gives us better invariants in terms of which promises should already be
cleared by shutdown and which ones should be rejected during shutdown.

I plan to fold this into part 1 before landing, but I'm keeping it as a
separate interdiff to make it easier for cpearce to review.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Replace AudioDecodeRendezvous with promise-y goodness. v2 r=cpearce,karlt

Comment 5

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4aa4a7c0ee42
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d3ead3647f88

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8534123 [details] [diff] [review]
Part 1.5 - Stop sharing promise members between subclass and superclass. v1

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

::: dom/media/fmp4/MP4Reader.h
@@ +143,5 @@
>      MP4Reader* mReader;
>      mp4_demuxer::TrackType mType;
>    };
>  
> +  struct DecoderDataBase {

I'm not keen on creating a DecoderDataBase/DecoderData inheritance hierarchy here...

I deliberately want DecoderData to be inspecific as to whether it's decoding a video or audio stream, so that (as much as possible) we can write general stream handling code...

So... it's a bit gross, but can the promise live inside the MP4Reader instead of inside MP4Reader::DecoderData?

Or... instead of creating a DecoderDataBase class, push the inheritance into a delegate of DecoderData? Something like:

class MediaDataPromiseHolder {
public:
  virtual void Resolve(MediaData*) = 0;
  virtual void Reject(reason...) = 0;
  virtual bool HasPromise() = 0;
};

.. and then have audio/video specific implementations that own the specific typed promise, and assert that we're resolving with a MediaData that matches the stream type, etc?

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8534123 [details] [diff] [review]
Part 1.5 - Stop sharing promise members between subclass and superclass. v1

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

Discussed on IRC, will accept keeping DecoderData named DecoderData, and having the subclass named DecoderDataWithPromise, or something like that.

I was thinking that with the inheritance hierarchy I proposed we could then resolve the promises with a MediaData rather than having to cast to the specific type, but I don't think that pattern will work well as we'd need to have an Ensure function that knows the specific promise type.

Comment 8

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1714687f2c45
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f82dc369ae6d

Comment 9

[Bobby Holley (:bholley)]

Sigh, followup jb bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae6ded2abe5

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1714687f2c45
https://hg.mozilla.org/mozilla-central/rev/f82dc369ae6d
https://hg.mozilla.org/mozilla-central/rev/6ae6ded2abe5

Comment 12

[Bruce Sun [:brsun]]

Comment on attachment 8534121 [details] [diff] [review]
Part 1 - Use MediaPromises for RequestAudioData and RequestVideoData. v3 r=cpearce

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

Sorry for the late response. It seems good.

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8534121 [details] [diff] [review]
Part 1 - Use MediaPromises for RequestAudioData and RequestVideoData. v3 r=cpearce

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]: This affects non-MSE media playback and is a significant refactoring. Risk is moderate, but it's been stable on m-c and later MSE changes require this.
[String/UUID change made/needed]: None.

This request applies to all patches on this bug.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/daaeccd4975e
https://hg.mozilla.org/releases/mozilla-aurora/rev/e39c7a02ccd1