Closed Bug 1276272 Opened 8 years ago Closed 8 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files)

Bug 1276272 part 1 - add Promise::MaybeRejectWithVode() utility method;
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1276272 part 2 - modify the webidl and preference setting;
58 bytes, text/x-review-board-request
ehsan.akhgari
: review+
Details
Bug 1276272 part 3 - implement promise-based HTMLMediaElement::seekToNextFrame();
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1276272 part 4 - modify the mochitest;
58 bytes, text/x-review-board-request
jwwang
: review+
Details 
This is a follow-up bug of bug 1235301.

This bug intends to make the HTMLMediaElement::SeekToNextFrame() as a promise-based asynchronous API.
Assignee: nobody → kaku
Depends on: 1235301
Attachment #8760069 - Flags: review?(jwwang)
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 on attachment 8760070 [details]
Bug 1276272 part 4 - modify the mochitest;

https://reviewboard.mozilla.org/r/57806/#review54664
Attachment #8760070 - Flags: review?(jwwang) → review+
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.
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.
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 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/
Attachment #8760069 - Flags: review?(jwwang)
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 on attachment 8760069 [details]
Bug 1276272 part 3 - implement promise-based HTMLMediaElement::seekToNextFrame();

https://reviewboard.mozilla.org/r/57804/#review54696
Attachment #8760069 - Flags: review?(jwwang) → review+
Attachment #8760067 - Flags: review?(ehsan) → review+
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()?
Attachment #8760068 - Flags: review?(ehsan) → review+
Comment on attachment 8760068 [details]
Bug 1276272 part 2 - modify the webidl and preference setting;

https://reviewboard.mozilla.org/r/57802/#review55538
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 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 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 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/
https://reviewboard.mozilla.org/r/57800/#review55536

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

Hmm, modified, thanks.
Keywords: checkin-needed
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
Flags: needinfo?(kaku)
Keywords: checkin-needed
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 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 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 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/
Rebased and please help to checkin them, thanks!
Flags: needinfo?(kaku)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
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?
Flags: needinfo?(ehsan)
See Also: → 1293546
@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
It seems like you can return InvalidStateError for those cases...
Flags: needinfo?(ehsan)
Blocks: 1295440
(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!
Thanks for updating the document!
Depends on: 1344357
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: