Use MediaPromise for seeking

RESOLVED FIXED in Firefox 36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug)

29 Branch
mozilla37
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 8536281 [details] [diff] [review]
Part 1: Use MediaPromise for seeking instead of a callback
Attachment #8536281 - Flags: review?(bobbyholley)
(Assignee)

Comment 2

4 years ago
Created attachment 8536282 [details] [diff] [review]
Part 2: Remove RequestSampleCallback
Attachment #8536282 - Flags: review?(bobbyholley)
Comment on attachment 8536281 [details] [diff] [review]
Part 1: Use MediaPromise for seeking instead of a callback

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

Nice! Thanks for doing this.

This patch seems to be a really compelling use-case for static methods that create already-resolved or already-rejected promises (à la Promise.{resolve,reject} in js), since that would remove the need to declare the temporaries here, and allow these sites to just do:

return SeekPromise::CreateAndResolve(true);
and
return SeekPromise::CreateAndReject(rv);

Those methods should be trivial to implement in MediaPromise.h, and would make this patch a good deal simpler. Can I persuade you to do that? If so, please do. But you're also welcome to land this patch as-is if you'd prefer (but please file a followup, and I'll try to do it).

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2339,4 @@
>  {
>    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>    mWaitingForDecoderSeek = false;
>    if (NS_FAILED(aResult)) {

Add a comment here indicating that we reject in non-failure cases, specifically when we seek twice, referencing MediaSourceReader.

::: dom/media/android/AndroidMediaReader.h
@@ +69,5 @@
>  
>    virtual nsresult ReadMetadata(MediaInfo* aInfo,
>                                  MetadataTags** aTags);
> +  virtual nsRefPtr<SeekPromise>
> +  Seek(int64_t aTime, int64_t aStartTime, int64_t aEndTime, int64_t aCurrentTime);

Please mark this and all over the other codec-specific overrides as MOZ_OVERRIDE.

::: dom/media/mediasource/MediaSourceReader.h
@@ +54,5 @@
>    void OnAudioNotDecoded(NotDecodedReason aReason);
>    void OnVideoDecoded(VideoData* aSample);
>    void OnVideoNotDecoded(NotDecodedReason aReason);
>  
> +  void OnSeekCompleted(bool aIgnored);

You shouldn't need aIgnored with bug 1109954.
Attachment #8536281 - Flags: review?(bobbyholley) → review+
Comment on attachment 8536282 [details] [diff] [review]
Part 2: Remove RequestSampleCallback

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

\o/
Attachment #8536282 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 5

4 years ago
Created attachment 8537131 [details] [diff] [review]
Part 1: Use MediaPromise for seeking instead of a callback v2

Fixed review comments, carrying r=bholley.
Attachment #8536281 - Attachment is obsolete: true
Attachment #8537131 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/99b9c2fca066
https://hg.mozilla.org/mozilla-central/rev/7f738423336a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8536282 [details] [diff] [review]
Part 2: Remove RequestSampleCallback

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, passes mochitests.
[Risks and why]: This affects non-MSE media playback, but issues would have turned up by now.
[String/UUID change made/needed]: None.

This request applies to all patches on this bug.
Attachment #8536282 - Flags: approval-mozilla-aurora?
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8536282 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.