1111413 - Use MediaPromise for seeking

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Use MediaPromise for seeking instead of a callback

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Remove RequestSampleCallback

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8536282 [details] [diff] [review]
Part 2: Remove RequestSampleCallback

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

\o/

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Use MediaPromise for seeking instead of a callback v2

Fixed review comments, carrying r=bholley.

Comment 6

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99b9c2fca066
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f738423336a

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99b9c2fca066
https://hg.mozilla.org/mozilla-central/rev/7f738423336a

Comment 8

[Ralph Giles (:rillian)]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9eb09ba7451a
https://hg.mozilla.org/releases/mozilla-aurora/rev/7e2a075d8906