NextFrameSeekTask will never finish while waiting for audio

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

(3 attachments)

(Assignee)

Description

2 years ago
https://hg.mozilla.org/mozilla-central/file/b69a5bbb5e40bd426e35222baa600b481e50d265/dom/media/NextFrameSeekTask.cpp#l174

Suppose we have |mVideoQueue.GetSize() > 0| and |!mReader->IsRequestingVideoData() && !mReader->IsWaitingVideoData() && mReader->IsWaitingAudioData()|, Seek() will return without calling EnsureVideoDecodeTaskQueued().

CheckIfSeekComplete() will never be called and seek will not finish. We need to call CheckIfSeekComplete() when AudioWaitCallback is resolved.
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
(Assignee)

Updated

2 years ago
Blocks: 1283751
Priority: -- → P2
(Assignee)

Comment 2

2 years ago
Created attachment 8768265 [details]
Bug 1283394. Part 1 - rename some functions.

Review commit: https://reviewboard.mozilla.org/r/62578/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62578/
Attachment #8768265 - Flags: review?(kaku)
Attachment #8768266 - Flags: review?(kaku)
Attachment #8768267 - Flags: review?(kaku)
(Assignee)

Comment 4

2 years ago
Created attachment 8768267 [details]
Bug 1283394. Part 3 - check if seek is completed in AudioWaitCallback().

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

Comment 5

2 years ago
Comment on attachment 8768265 [details]
Bug 1283394. Part 1 - rename some functions.

https://reviewboard.mozilla.org/r/62578/#review59294
Attachment #8768265 - Flags: review?(kaku) → review+

Comment 6

2 years ago
Comment on attachment 8768266 [details]
Bug 1283394. Part 2 - remove some null-checks.

https://reviewboard.mozilla.org/r/62580/#review59296
Attachment #8768266 - Flags: review?(kaku) → review+

Comment 7

2 years ago
Comment on attachment 8768267 [details]
Bug 1283394. Part 3 - check if seek is completed in AudioWaitCallback().

https://reviewboard.mozilla.org/r/62582/#review59290

::: dom/media/NextFrameSeekTask.cpp:174
(Diff revision 1)
> -  if ((mVideoQueue.GetSize() > 0
> -       && !mReader->IsRequestingAudioData() && !mReader->IsWaitingAudioData()
> -       && !mReader->IsRequestingVideoData() && !mReader->IsWaitingVideoData())
> -      || mVideoQueue.AtEndOfStream()) {
> -    UpdateSeekTargetTime();
> -    SeekTaskResolveValue val = {};  // Zero-initialize data members.
> +  bool hasPendingRequests = mReader->IsRequestingAudioData() ||
> +                            mReader->IsWaitingAudioData() ||
> +                            mReader->IsRequestingVideoData() ||
> +                            mReader->IsWaitingVideoData();
> +
> +  bool needMoreVideo = mVideoQueue.GetSize() == 0 && !mVideoQueue.IsFinished();

I think we could constifiy these two variables.
Attachment #8768267 - Flags: review?(kaku) → review+
(Assignee)

Comment 8

2 years ago
https://reviewboard.mozilla.org/r/62582/#review59290

> I think we could constifiy these two variables.

We can. But it won't be as helpful as to class members because it is almost impossible to modify these local bools accidentally. On the other hand, the extra "const" will not add as much information as noise in such a short code.
(Assignee)

Comment 9

2 years ago
Thanks for the review!

Comment 10

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9580ebae07e5
Part 1 - rename some functions. r=kaku
https://hg.mozilla.org/integration/autoland/rev/711a051269d5
Part 2 - remove some null-checks. r=kaku
https://hg.mozilla.org/integration/autoland/rev/28441ef38d79
Part 3 - check if seek is completed in AudioWaitCallback(). r=kaku
Mass change P2 -> P3
Priority: P2 → P3

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9580ebae07e5
https://hg.mozilla.org/mozilla-central/rev/711a051269d5
https://hg.mozilla.org/mozilla-central/rev/28441ef38d79
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.