NextFrameSeekTask might decode more video frames than needed

RESOLVED FIXED in Firefox 50

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

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

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
Here is the call flow:

1. Seek() decides not to call EnsureVideoDecodeTaskQueued() for there is already something in the video queue and the queue is not finished yet.

2. Seek() sees there is an audio request pending and needs to wait for it before finishing seek.

3. OnAudioDecoded() calls CheckIfSeekComplete() which calls EnsureVideoDecodeTaskQueued() because mSeekedVideoData is null and IsVideoSeekComplete() returns false.

Audio callbacks shouldn't request new video frames for that is the job of video callbacks.
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
(Assignee)

Updated

2 years ago
Blocks: 1283751
Priority: -- → P2
Mass change P2 -> P3
Priority: P2 → P3
(Assignee)

Comment 3

2 years ago
Created attachment 8769532 [details]
Bug 1283718. Part 1 - fix comments and constify IsAudioSeekComplete().

Review commit: https://reviewboard.mozilla.org/r/63396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63396/
Attachment #8769532 - Flags: review?(kaku)
Attachment #8769533 - Flags: review?(kaku)
Attachment #8769534 - Flags: review?(kaku)
Attachment #8769535 - Flags: review?(kaku)
Attachment #8769536 - Flags: review?(kaku)
Attachment #8769537 - Flags: review?(kaku)
(Assignee)

Comment 4

2 years ago
Created attachment 8769533 [details]
Bug 1283718. Part 2 - fix the logic of IsVideoSeekComplete().

Review commit: https://reviewboard.mozilla.org/r/63398/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63398/
(Assignee)

Comment 5

2 years ago
Created attachment 8769534 [details]
Bug 1283718. Part 3 - replace some code with new helpers.

Review commit: https://reviewboard.mozilla.org/r/63400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63400/
(Assignee)

Comment 7

2 years ago
Created attachment 8769536 [details]
Bug 1283718. Part 5 - move the call to EnsureVideoDecodeTaskQueued() out of MaybeFinishSeek() so we only request video in video callbacks.

Review commit: https://reviewboard.mozilla.org/r/63404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63404/
(Assignee)

Comment 8

2 years ago
Created attachment 8769537 [details]
Bug 1283718. Part 6 - always check NeedMoreVideo() before requesting new video.

We still have a chance to finish seeking even when video promises are rejected
provided we already have video samples in the queue.

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

Comment 9

2 years ago
Comment on attachment 8769532 [details]
Bug 1283718. Part 1 - fix comments and constify IsAudioSeekComplete().

https://reviewboard.mozilla.org/r/63396/#review60268
Attachment #8769532 - Flags: review?(kaku) → review+

Comment 10

2 years ago
Comment on attachment 8769533 [details]
Bug 1283718. Part 2 - fix the logic of IsVideoSeekComplete().

https://reviewboard.mozilla.org/r/63398/#review60270
Attachment #8769533 - Flags: review?(kaku) → review+

Comment 11

2 years ago
Comment on attachment 8769534 [details]
Bug 1283718. Part 3 - replace some code with new helpers.

https://reviewboard.mozilla.org/r/63400/#review60272
Attachment #8769534 - Flags: review?(kaku) → review+

Comment 12

2 years ago
Comment on attachment 8769535 [details]
Bug 1283718. Part 4 - rename some function.

https://reviewboard.mozilla.org/r/63402/#review60274
Attachment #8769535 - Flags: review?(kaku) → review+

Comment 13

2 years ago
Comment on attachment 8769536 [details]
Bug 1283718. Part 5 - move the call to EnsureVideoDecodeTaskQueued() out of MaybeFinishSeek() so we only request video in video callbacks.

https://reviewboard.mozilla.org/r/63404/#review60276
Attachment #8769536 - Flags: review?(kaku) → review+

Comment 14

2 years ago
Comment on attachment 8769537 [details]
Bug 1283718. Part 6 - always check NeedMoreVideo() before requesting new video.

https://reviewboard.mozilla.org/r/63406/#review60278
Attachment #8769537 - Flags: review?(kaku) → review+
(Assignee)

Comment 15

2 years ago
Thanks!

Comment 16

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3262d04cc706
Part 1 - fix comments and constify IsAudioSeekComplete(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/b2d6082f93a4
Part 2 - fix the logic of IsVideoSeekComplete(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/4b0c00af923d
Part 3 - replace some code with new helpers. r=kaku
https://hg.mozilla.org/integration/autoland/rev/10d751f3f2e9
Part 4 - rename some function. r=kaku
https://hg.mozilla.org/integration/autoland/rev/44614f65b3a1
Part 5 - move the call to EnsureVideoDecodeTaskQueued() out of MaybeFinishSeek() so we only request video in video callbacks. r=kaku
https://hg.mozilla.org/integration/autoland/rev/29a5d693613d
Part 6 - always check NeedMoreVideo() before requesting new video. r=kaku
You need to log in before you can comment on or make changes to this bug.