MediaFormatReader's internal seek may never complete under some circumstances

RESOLVED FIXED in Firefox 49

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks 2 bugs)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

3 years ago
This one certainly doesn't cause bug 1258922, but I identified the problem investigating it.

When playback reach a discontinuity or gap in the media, the decoder is drained, then the MFR will seek back to the last sample returned during draining.

If the source buffer was cleared in between, this last seek will fail. The internal seek target threshold is cleared.

The MFR has now lost record that it had to seek back to the last sample before continuing. Playback will never resume.

We do not have tests for this case, and I don't think we can write a reliable one as the interval during which we are draining and seek back is very small, leaving the probability that the data was cleared very small.

It is a potential problem however, and the MFR shouldn't clear the internal seek state just because the first seek failed.
Assignee

Comment 2

3 years ago
While a seek is pending, the demuxer has been reset meaning that ShouldSkip() would almost always incorrectly return true and would corrupt the seeking state.

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

Comment 4

3 years ago
It ensure that resume from waiting for data is correct and the MediaFormatReader internal seek can handle partial GOP.

Review commit: https://reviewboard.mozilla.org/r/49949/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49949/
Attachment #8747575 - Flags: review?(gsquelart) → review+
Comment on attachment 8747575 [details]
MozReview Request: Bug 1269178: P1. Retry InternalSeek if previous attempt failed once more data is available. r?gerald

https://reviewboard.mozilla.org/r/49943/#review46657

::: dom/media/MediaFormatReader.cpp:833
(Diff revision 1)
> +
> +  bool hasPendingSeek =
> +    decoder.mTimeThreshold && !decoder.mTimeThreshold.ref().mHasSeeked;
> +  if (hasPendingSeek || decoder.HasWaitingPromise()) {
> +    if (hasPendingSeek) {
> +      LOG("Attempting Internal Seek");

Was that line only needed during development&testing?
If yes, you may remove it.
If not, and you want to keep it there, how about more information, e.g.: "Received new data, pending seek -> attempt internal seek."
Comment on attachment 8747576 [details]
MozReview Request: Bug 1269178: P2. Ensure that no skip to next keyframe logic is activated while there's a pending seek. r?gerald

https://reviewboard.mozilla.org/r/49945/#review46661

::: dom/media/MediaFormatReader.cpp:523
(Diff revision 1)
> -  if (ShouldSkip(aSkipToNextKeyframe, timeThreshold)) {
> +  // Ensure we have no pending seek going as ShouldSkip could return out of data
> +  // information.

"out of data" -> "out of date"?
Attachment #8747576 - Flags: review?(gsquelart) → review+
Comment on attachment 8747577 [details]
MozReview Request: Bug 1269178: P3. Add mochitest. r?gerald

https://reviewboard.mozilla.org/r/49947/#review46663
Attachment #8747577 - Flags: review?(gsquelart) → review+
Comment on attachment 8747578 [details]
MozReview Request: Bug 1269178: P4. Add mochitest. r?gerald

https://reviewboard.mozilla.org/r/49949/#review46665

::: dom/media/mediasource/test/mochitest.ini:43
(Diff revision 1)
>    aac20-48000-64000-1.m4s aac20-48000-64000-1.m4s^headers^
>    aac20-48000-64000-2.m4s aac20-48000-64000-2.m4s^headers^
>    aac51-48000-128000-init.mp4 aac51-48000-128000-init.mp4^headers^
>    aac51-48000-128000-1.m4s aac51-48000-128000-1.m4s^headers^
>    aac51-48000-128000-2.m4s aac51-48000-128000-2.m4s^headers^
> +  bipbop/bipbop_480_624kbps-videoinit.mp4	bipbop/bipbop_480_624kbps-videoinit.mp4^headers^

Is that a tab character in there? Exterminate!
Attachment #8747578 - Flags: review?(gsquelart) → review+
Assignee

Comment 9

3 years ago
https://reviewboard.mozilla.org/r/49943/#review46667

::: dom/media/MediaFormatReader.cpp:833
(Diff revision 1)
> +
> +  bool hasPendingSeek =
> +    decoder.mTimeThreshold && !decoder.mTimeThreshold.ref().mHasSeeked;
> +  if (hasPendingSeek || decoder.HasWaitingPromise()) {
> +    if (hasPendingSeek) {
> +      LOG("Attempting Internal Seek");

the log mac add the name of the class and the name of the function, as such it is obvious that new data has been added
Assignee

Comment 10

3 years ago
Comment on attachment 8747576 [details]
MozReview Request: Bug 1269178: P2. Ensure that no skip to next keyframe logic is activated while there's a pending seek. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49945/diff/1-2/
Assignee

Comment 11

3 years ago
Comment on attachment 8747577 [details]
MozReview Request: Bug 1269178: P3. Add mochitest. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49947/diff/1-2/
Assignee

Comment 12

3 years ago
Comment on attachment 8747578 [details]
MozReview Request: Bug 1269178: P4. Add mochitest. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49949/diff/1-2/
Assignee: nobody → jyavenard
Depends on: 1269857
No longer depends on: 1269857
Depends on: 1269408
Assignee

Updated

3 years ago
See Also: → 1285904
Assignee

Updated

3 years ago
Blocks: 1286115
Jean-Yves - what happened with the backouts above?
Flags: needinfo?(jyavenard)
Assignee

Comment 18

3 years ago
For some reasons all those changes ended up in bug 1269408 instead.

https://hg.mozilla.org/mozilla-central/rev/df20f438502df1a33c71aea95d3ded5b84a2d2a7
and it contains both the P1 and P2 of this bug. Not sure what happened here.
Flags: needinfo?(jyavenard)
Assignee

Updated

3 years ago
See Also: 12859041269408

Updated

3 years ago
Duplicate of this bug: 1285904
You need to log in before you can comment on or make changes to this bug.