Closed Bug 1108701 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(3 files)

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.
Absorbing the deps from bug 1097823.
Depends on: 1108707
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)
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+
No longer blocks: 821062
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?
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.