1269178 - MediaFormatReader's internal seek may never complete under some circumstances

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1269178: P1. Retry InternalSeek if previous attempt failed once more data is available. r?gerald

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

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1269178: P2. Ensure that no skip to next keyframe logic is activated while there's a pending seek. r?gerald

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/

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1269178: P3. Add mochitest. r?gerald

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

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1269178: P4. Add mochitest. r?gerald

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/

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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"?

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

https://reviewboard.mozilla.org/r/49947/#review46663

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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!

Comment 9

[Jean-Yves Avenard [:jya]]

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

Comment 10

[Jean-Yves Avenard [:jya]]

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/

Comment 11

[Jean-Yves Avenard [:jya]]

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/

Comment 12

[Jean-Yves Avenard [:jya]]

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/

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecb87e0e1cab
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f6d645e9e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b7c393b0d06
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa7871c000f

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ecb87e0e1cab
https://hg.mozilla.org/mozilla-central/rev/31f6d645e9e5
https://hg.mozilla.org/mozilla-central/rev/7b7c393b0d06
https://hg.mozilla.org/mozilla-central/rev/7aa7871c000f

Comment 15

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97acde75c885
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8cf9ffd16e

Comment 16

[Carsten Book [:Tomcat]]

backout from m-c
https://hg.mozilla.org/mozilla-central/rev/97acde75c885 backout from m-c
https://hg.mozilla.org/mozilla-central/rev/2d8cf9ffd16e backout from m-c

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Jean-Yves - what happened with the backouts above?

Comment 18

[Jean-Yves Avenard [:jya]]

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.