1276272 - Make HTMLMediaElement::SeekToNextFrame() return a Promise<void>.

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Tzuhao Kuo [:kaku]]

This is a follow-up bug of bug 1235301.

This bug intends to make the HTMLMediaElement::SeekToNextFrame() as a promise-based asynchronous API.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1276272 part 1 - add Promise::MaybeRejectWithVode() utility method;

Review commit: https://reviewboard.mozilla.org/r/57800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57800/

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1276272 part 2 - modify the webidl and preference setting;

Review commit: https://reviewboard.mozilla.org/r/57802/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57802/

Comment 3

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1276272 part 3 - implement promise-based HTMLMediaElement::seekToNextFrame();

Review commit: https://reviewboard.mozilla.org/r/57804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57804/

Comment 4

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1276272 part 4 - modify the mochitest;

Review commit: https://reviewboard.mozilla.org/r/57806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57806/

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8760069 [details]
Bug 1276272 part 3 - implement promise-based HTMLMediaElement::seekToNextFrame();

https://reviewboard.mozilla.org/r/57804/#review54662

::: dom/html/HTMLMediaElement.cpp:1586
(Diff revision 1)
>  
>    // Clamp the seek target to inside the seekable ranges.
>    RefPtr<dom::TimeRanges> seekable = new dom::TimeRanges(ToSupports(OwnerDoc()));
>    media::TimeIntervals seekableIntervals = mDecoder->GetSeekable();
>    if (seekableIntervals.IsInvalid()) {
> -    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); // This will reject the promise.

How will Throw() reject the promise?

::: dom/media/MediaDecoder.cpp:845
(Diff revision 1)
>    }
>    return NS_OK;
>  }
>  
>  void
> -MediaDecoder::CallSeek(const SeekTarget& aTarget)
> +MediaDecoder::AsyncResolveSeekDOMPromiseIfExists()

Why do we resolve the dom promise asynchronously?

::: dom/media/MediaDecoder.cpp:886
(Diff revision 1)
> +MediaDecoder::CallSeek(const SeekTarget& aTarget, dom::Promise* aPromise)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  DiscardOngoingSeekIfExists();
> +
> +  mSeekDOMPromise = aPromise;

Can you capture the promise in a lambda without storing it in a member?

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8760070 [details]
Bug 1276272 part 4 - modify the mochitest;

https://reviewboard.mozilla.org/r/57806/#review54664

Comment 7

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/57804/#review54662

> How will Throw() reject the promise?

For those methods that return a promise, the bindings automatically reject the prmise if any errors were occured. In specific, the bindings call into this function https://dxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.cpp?from=ConvertExceptionToPromise#2858 . 

For exmaple, the HTMLMediaElement::setMediaKey() has this behavior here: https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dom/bindings/HTMLMediaElementBinding.cpp#2358 .

> Why do we resolve the dom promise asynchronously?

For guaranteeing that the promise is resolved/rejected after the "seeking" event callback is invoked. 

The promise resolving/rejection is queued as a "micro-task" which will be handled immediately after the current JS task and before any pending JS tasks. So, at the time we are going to resolve/reject a promise, the "seeking" event task should already be queued but might yet be processed, and if we file the promise resolving/rejection micro-task here, then the promise resolving/rejection micro-task will be processed before the seeking event task. 

That's why I instead file another task which will be queued after the seeking event task.

> Can you capture the promise in a lambda without storing it in a member?

No, we cannot, we need to store it as a member for handling the consecutive seeking operations. In this case, the orginal mSeekRequest is discconected then the callback lambda is never invoked. So, if the promise is stored in the lambda only, we then have no way to reject it.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

https://reviewboard.mozilla.org/r/57804/#review54676

::: dom/media/MediaDecoder.cpp:1311
(Diff revision 1)
> +MediaDecoder::OnSeekRejected()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  mSeekRequest.Complete();
> +
> +  if (mShuttingDown)

The original OnSeekRejected() didn't return early for mShuttingDown is true.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

https://reviewboard.mozilla.org/r/57804/#review54662

> For guaranteeing that the promise is resolved/rejected after the "seeking" event callback is invoked. 
> 
> The promise resolving/rejection is queued as a "micro-task" which will be handled immediately after the current JS task and before any pending JS tasks. So, at the time we are going to resolve/reject a promise, the "seeking" event task should already be queued but might yet be processed, and if we file the promise resolving/rejection micro-task here, then the promise resolving/rejection micro-task will be processed before the seeking event task. 
> 
> That's why I instead file another task which will be queued after the seeking event task.

OK. Add these comments to the functions to make the code readable.

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8760069 [details]
Bug 1276272 part 3 - implement promise-based HTMLMediaElement::seekToNextFrame();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57804/diff/1-2/

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8760070 [details]
Bug 1276272 part 4 - modify the mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57806/diff/1-2/

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8760069 [details]
Bug 1276272 part 3 - implement promise-based HTMLMediaElement::seekToNextFrame();

https://reviewboard.mozilla.org/r/57804/#review54696

Comment 13

[(no longer active)]

Comment on attachment 8760067 [details]
Bug 1276272 part 1 - add Promise::MaybeRejectWithVode() utility method;

https://reviewboard.mozilla.org/r/57800/#review55536

::: dom/promise/Promise.h:153
(Diff revision 1)
>  
>    void MaybeReject(const RefPtr<MediaStreamError>& aArg);
>  
>    void MaybeRejectWithNull();
>  
> +  void MaybeRejectWithVoid();

Why don't you call this MaybeRejectWithUndefined()?

Comment 14

[(no longer active)]

Comment on attachment 8760068 [details]
Bug 1276272 part 2 - modify the webidl and preference setting;

https://reviewboard.mozilla.org/r/57802/#review55538

Comment 15

[Tzuhao Kuo [:kaku]]

Comment on attachment 8760067 [details]
Bug 1276272 part 1 - add Promise::MaybeRejectWithVode() utility method;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57800/diff/1-2/

Comment 16

[Tzuhao Kuo [:kaku]]

Comment on attachment 8760068 [details]
Bug 1276272 part 2 - modify the webidl and preference setting;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57802/diff/1-2/

Comment 17

[Tzuhao Kuo [:kaku]]

Comment on attachment 8760069 [details]
Bug 1276272 part 3 - implement promise-based HTMLMediaElement::seekToNextFrame();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57804/diff/2-3/

Comment 18

[Tzuhao Kuo [:kaku]]

Comment on attachment 8760070 [details]
Bug 1276272 part 4 - modify the mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57806/diff/2-3/

Comment 19

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/57800/#review55536

> Why don't you call this MaybeRejectWithUndefined()?

Hmm, modified, thanks.

Comment 20

[Tzuhao Kuo [:kaku]]

Try looks ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89d1321562e7

Thanks for the review!

Comment 21

[Carsten Book [:Tomcat]]

has problems to apply: could you take a look (thanks)

Hunk #1 FAILED at 934
1 out of 1 hunks FAILED -- saving rejects to file dom/promise/Promise.cpp.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 22

[Tzuhao Kuo [:kaku]]

Comment on attachment 8760067 [details]
Bug 1276272 part 1 - add Promise::MaybeRejectWithVode() utility method;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57800/diff/2-3/

Comment 23

[Tzuhao Kuo [:kaku]]

Comment on attachment 8760068 [details]
Bug 1276272 part 2 - modify the webidl and preference setting;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57802/diff/2-3/

Comment 24

[Tzuhao Kuo [:kaku]]

Comment on attachment 8760069 [details]
Bug 1276272 part 3 - implement promise-based HTMLMediaElement::seekToNextFrame();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57804/diff/3-4/

Comment 25

[Tzuhao Kuo [:kaku]]

Comment on attachment 8760070 [details]
Bug 1276272 part 4 - modify the mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57806/diff/3-4/

Comment 26

[Tzuhao Kuo [:kaku]]

Rebased and please help to checkin them, thanks!

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ec9ca3e6d92
part 1 - add Promise::MaybeRejectWithVode() utility method; r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9925ad8dc9e
part 2 - modify the webidl and preference setting; r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a366d8ef5e9f
part 3 - implement promise-based HTMLMediaElement::seekToNextFrame(); r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/beb4b214b6b7
part 4 - modify the mochitest; r=jwwang

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ec9ca3e6d92
https://hg.mozilla.org/mozilla-central/rev/c9925ad8dc9e
https://hg.mozilla.org/mozilla-central/rev/a366d8ef5e9f
https://hg.mozilla.org/mozilla-central/rev/beb4b214b6b7

Comment 29

[Boris Zbarsky [:bzbarsky]]

Rejecting promises with undefined is a pretty weird thing to do.  Normally you'd reject them with some sort of exception value...  Is there a reason we're doing that here?

Comment 30

[Tzuhao Kuo [:kaku]]

@bz, the HTMLMediaElement::seekToNextFrame() is an experimental feature which was implemented based on the seek code path. In the current WHATWG spec, the seek operation is event-based (not promise-based) and the operation might be discard without any message (where [1][2][3][4] used to be). While I was implementing the seekToNextFrame() API, I replaced [1][2][3][4] with "rejecting the promise with undefined".

I think you are right that we should always be able to return meaningful error messages while rejecting promises. The good news is that the community is considering making the seek operation be promise-based (https://github.com/whatwg/html/issues/553) and I expect that what DOM exceptions should be returned will be well-defined then. For now, should we have our own definition to return something for all [1][2][3][4] cases?

[1] http://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1728
[2] http://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1746
[3] http://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1753
[4] http://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1768

Comment 31

[(no longer active)]

It seems like you can return InvalidStateError for those cases...

Comment 32

[Eric Shepherd [:sheppy]]

(In reply to :Ehsan Akhgari from comment #31)
> It seems like you can return InvalidStateError for those cases...

If a bug is or will be filed to change this, please make sure it's set to dev-doc-needed for me. Thanks!

Comment 33

[Eric Shepherd [:sheppy]]

Documentation has been updated:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/seekToNextFrame

The change is also listed on Firefox 50 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/50

Comment 34

[Tzuhao Kuo [:kaku]]

Thanks for updating the document!