Closed Bug 1299021 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

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
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
Attachment #8786178 - Flags: review?(kikuo)
Attachment #8786179 - Flags: review?(kikuo)
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 ?
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 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 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+
Thanks!
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
https://hg.mozilla.org/mozilla-central/rev/0b46962a4b12
https://hg.mozilla.org/mozilla-central/rev/6f6e6eb573d3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.