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

RESOLVED FIXED in Firefox 36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla37
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Absorbing the deps from bug 1097823.
(Assignee)

Updated

4 years ago
Depends on: 1108707
(Assignee)

Comment 2

4 years ago
Created attachment 8534121 [details] [diff] [review]
Part 1 - Use MediaPromises for RequestAudioData and RequestVideoData. v3 r=cpearce
Attachment #8534121 - Flags: review+
Attachment #8534121 - Flags: feedback?(brsun)
Attachment #8534121 - Flags: feedback?(bechen)
(Assignee)

Comment 3

4 years ago
Created attachment 8534123 [details] [diff] [review]
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.
Attachment #8534123 - Flags: review?(cpearce)
(Assignee)

Comment 4

4 years ago
Created attachment 8534124 [details] [diff] [review]
Part 2 - Replace AudioDecodeRendezvous with promise-y goodness. v2 r=cpearce,karlt
Attachment #8534124 - Flags: review+
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?
Attachment #8534123 - Flags: review?(cpearce) → review-
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.
Attachment #8534123 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/1714687f2c45
https://hg.mozilla.org/mozilla-central/rev/f82dc369ae6d
https://hg.mozilla.org/mozilla-central/rev/6ae6ded2abe5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
No longer blocks: 821062
Duplicate of this bug: 821062

Comment 12

4 years ago
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.
Attachment #8534121 - Flags: feedback?(brsun) → feedback+
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.
Attachment #8534121 - Flags: approval-mozilla-aurora?
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8534121 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8534121 - Flags: feedback?(bechen)
You need to log in before you can comment on or make changes to this bug.