Sometimes VideoSink fails to resolve the end promise and 'ended' is never fired

RESOLVED FIXED in Firefox 51

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla51
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox50 unaffected, firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a year ago
https://hg.mozilla.org/mozilla-central/file/tip/dom/media/mediasink/VideoSink.cpp#l184

It is possible that when mVideoSinkEndRequest is resolved, the queue is empty and TryUpdateRenderedVideoFrames() will not call UpdateRenderedVideoFrames() to resolve the end promise [1].

[1] https://hg.mozilla.org/mozilla-central/file/tip/dom/media/mediasink/VideoSink.cpp#l424
(Assignee)

Comment 1

a year ago
A regression from bug 1258870 which is also responsible for the timeout in bug 1198168.
Assignee: nobody → jwwang
Blocks: 1258870, 1198168
Keywords: regression
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8786178 - Flags: review?(kikuo)
Attachment #8786179 - Flags: review?(kikuo)

Comment 4

a year ago
mozreview-review
Comment on attachment 8786178 [details]
Bug 1299021. Part 1 - extract the code about resolving the end promise.

https://reviewboard.mozilla.org/r/75140/#review73098

::: dom/media/mediasink/VideoSink.cpp:187
(Diff revision 1)
>          [self] () {
>            self->mVideoSinkEndRequest.Complete();
>            self->TryUpdateRenderedVideoFrames();
>          }, [self] () {
>            self->mVideoSinkEndRequest.Complete();
>            self->TryUpdateRenderedVideoFrames();

Can't we just call on |MaybeResolveEndPromise()| after the these two |self->TryUpdateRenderVideoFrames()| in  resolve/reject functions here ?

Becuase all the other call sites for |MaybeResolveEndPromise()| are right after a call on |UpdateRenderedVideoFrames()|.   For me, it seems to increase the uncertainty of code logic. I think that we resovle the EndPromise inside |UpdateRenderedVideoFrames| is because we are controlling & verifying the status of VideoQueue to make sure each video frame is played or is discraded. So we should keep  |MaybeResolveEndPromise()| inside  |UpdateRenderedVideoFrames()|.

Does that make sense to you ?
(Assignee)

Comment 5

a year ago
mozreview-review-reply
Comment on attachment 8786178 [details]
Bug 1299021. Part 1 - extract the code about resolving the end promise.

https://reviewboard.mozilla.org/r/75140/#review73098

> Can't we just call on |MaybeResolveEndPromise()| after the these two |self->TryUpdateRenderVideoFrames()| in  resolve/reject functions here ?
> 
> Becuase all the other call sites for |MaybeResolveEndPromise()| are right after a call on |UpdateRenderedVideoFrames()|.   For me, it seems to increase the uncertainty of code logic. I think that we resovle the EndPromise inside |UpdateRenderedVideoFrames| is because we are controlling & verifying the status of VideoQueue to make sure each video frame is played or is discraded. So we should keep  |MaybeResolveEndPromise()| inside  |UpdateRenderedVideoFrames()|.
> 
> Does that make sense to you ?

Good point! Patches coming.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a year ago
mozreview-review
Comment on attachment 8786178 [details]
Bug 1299021. Part 1 - extract the code about resolving the end promise.

https://reviewboard.mozilla.org/r/75140/#review73480
Attachment #8786178 - Flags: review?(kikuo) → review+

Comment 9

a year ago
mozreview-review
Comment on attachment 8786179 [details]
Bug 1299021. Part 2 - ensure resolving the end promise when mVideoSinkEndRequest is resolved.

https://reviewboard.mozilla.org/r/75142/#review73482
Attachment #8786179 - Flags: review?(kikuo) → review+
(Assignee)

Comment 10

a year ago
Thanks!

Comment 11

a year ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b46962a4b12
Part 1 - extract the code about resolving the end promise. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/6f6e6eb573d3
Part 2 - ensure resolving the end promise when mVideoSinkEndRequest is resolved. r=kikuo

Updated

a year ago
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
status-firefox50: --- → ?
status-firefox51: --- → affected
https://hg.mozilla.org/mozilla-central/rev/0b46962a4b12
https://hg.mozilla.org/mozilla-central/rev/6f6e6eb573d3
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
status-firefox50: ? → unaffected
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.