1272964 - SkipVideoDemuxToNextKeyFrame could incorrectly jump over gap in data.

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

SkipVideoDemuxToNextKeyFrame check that the time threshold is buffered is incorrect, as it's the next keyframe time we should check (but that we don't know).

SkipVideoDemuxToNextKeyFrame could incorrectly jump over any gap and end up well pass the time threshold.

So we should use the same logic as per bug 1272916, and ensure we don't go over gaps ; however stalling may not be the best thing to do as currentTime would be incorrectly in the past of where we've actually stalled.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r?gerald

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

Comment 2

[Jean-Yves Avenard [:jya]]

this fix has exposed a bug in MediaFormatReader ; need to look closer into it.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272964: P1. Only activate skip to next keyframe logic when next keyframe time is known. r?gerald

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

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272964: P2. Don't activate skip to next keyframe until we passed the internal seek target. r?gerald

currentTime may be passed the start of the internal seek time, but before it end, which would have triggered a false positive: we do not want to jump to the next key frame here.

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

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8753250 [details]
MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r?gerald

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

Comment 6

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

Comment on attachment 8753653 [details]
MozReview Request: Bug 1272964: P1. Only activate skip to next keyframe logic when next keyframe time is known. r?gerald

https://reviewboard.mozilla.org/r/53428/#review50204

Comment 7

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

Comment on attachment 8753654 [details]
MozReview Request: Bug 1272964: P2. Don't activate skip to next keyframe until we passed the internal seek target. r?gerald

https://reviewboard.mozilla.org/r/53430/#review50208

r+, but please review the commit description:
'may be passed the start' -> past
'before it end' -> its? ends?

Comment 8

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

Comment on attachment 8753250 [details]
MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r?gerald

https://reviewboard.mozilla.org/r/53146/#review50210

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8753654 [details]
MozReview Request: Bug 1272964: P2. Don't activate skip to next keyframe until we passed the internal seek target. r?gerald

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

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8753250 [details]
MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53146/diff/2-3/

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8753250 [details]
MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53146/diff/3-4/

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d95abc81b04
https://hg.mozilla.org/integration/mozilla-inbound/rev/993c685703fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2425c95f50f

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing mda test_BufferingWait_mp4.html, at least on Linux x64:

https://hg.mozilla.org/integration/mozilla-inbound/rev/75834b774528083e9173aca0a1b74b27f3d44e19
https://hg.mozilla.org/integration/mozilla-inbound/rev/542ecb1c7022d7cb5375154960d4b2f1b8501a91
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd613abb770f52a7dbf27682cce9d9038272808

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f2425c95f50f7c83e45d023cfaa5ac0151ff9d65
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=28108885&repo=mozilla-inbound
01:04:52     INFO -  27 INFO TEST-PASS | dom/media/mediasource/test/test_BufferingWait_mp4.html | Reached target time of: 1.57895
01:04:52     INFO -  28 INFO TEST-PASS | dom/media/mediasource/test/test_BufferingWait_mp4.html | fetchWithXHR load uri='bipbop/bipbop3.m4s' status=200
01:04:52     INFO -  29 INFO Loading buffer: [0, 24013)
01:04:52     INFO -  30 INFO SourceBuffer buffered ranges grew from TimeRanges: [0.095, 1.625396)[3.298333, 4.040272) to TimeRanges: [0.095, 2.414875)[3.298333, 4.040272)
01:04:52     INFO -  31 INFO TEST-UNEXPECTED-FAIL | dom/media/mediasource/test/test_BufferingWait_mp4.html | Test timed out.

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272964: P4. Only flush decoder if skip to next keyframe actually succeeds. r?cpearce

As the decoder was flushed and reset prior the skip to next keyframe started, and future error would be unrecoverable. So only reset the decoder once the skip completes and succeeded. Otherwise we default back to normal decoding.

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

Comment 15

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272964: [MSE] P5. Default to skipping to the next keyframe if no keyframe was found past currentTime. r?cpearce

If no keyframe are found after our time threshold, we can still skip to another keyframe (despite being prior the desired time).
So this is just a workaround for our inability to tell the MDSM when to enter buffering mode and instead the MDSM incorrectly uses the time of the last frame returned.

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

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272964: P6. Exclude frames dropped due to internal seeking from calculations. r?cpearce

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

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8753654 [details]
MozReview Request: Bug 1272964: P2. Don't activate skip to next keyframe until we passed the internal seek target. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53430/diff/2-3/

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8753250 [details]
MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53146/diff/4-5/

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754248 [details]
MozReview Request: Bug 1272964: P4. Only flush decoder if skip to next keyframe actually succeeds. r?cpearce

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

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754249 [details]
MozReview Request: Bug 1272964: [MSE] P5. Default to skipping to the next keyframe if no keyframe was found past currentTime. r?cpearce

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

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754320 [details]
MozReview Request: Bug 1272964: P6. Exclude frames dropped due to internal seeking from calculations. r?cpearce

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

Comment 22

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272964: P1. Only activate skip to next keyframe logic when next keyframe time is known. r=gerald

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

Comment 23

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272964: P2. Don't activate skip to next keyframe until we passed the internal seek target. r=gerald

currentTime may be past the start of the internal seek time, but before its end, which would have triggered a false positive: we do not want to jump to the next key frame here.

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

Comment 24

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r=gerald

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

Comment 25

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272964: P4. Only flush decoder if skip to next keyframe actually succeeds. r?cpearce

As the decoder was flushed and reset prior the skip to next keyframe started, and future error would be unrecoverable. So only reset the decoder once the skip completes and succeeded. Otherwise we default back to normal decoding.

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

Comment 26

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1272964: [MSE] P5. Default to skipping to the next keyframe if no keyframe was found past currentTime. r?cpearce

If no keyframe are found after our time threshold, we can still skip to another keyframe (despite being prior the desired time).
So this is just a workaround for our inability to tell the MDSM when to enter buffering mode and instead the MDSM incorrectly uses the time of the last frame returned.

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

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754390 [details]
MozReview Request: Bug 1272964: P1. Only activate skip to next keyframe logic when next keyframe time is known. r=gerald

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

Comment 28

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754391 [details]
MozReview Request: Bug 1272964: P2. Don't activate skip to next keyframe until we passed the internal seek target. r=gerald

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

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754392 [details]
MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r=gerald

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

Comment 30

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754393 [details]
MozReview Request: Bug 1272964: P4. Only flush decoder if skip to next keyframe actually succeeds. r?cpearce

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

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754394 [details]
MozReview Request: Bug 1272964: [MSE] P5. Default to skipping to the next keyframe if no keyframe was found past currentTime. r?cpearce

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

Comment 32

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754320 [details]
MozReview Request: Bug 1272964: P6. Exclude frames dropped due to internal seeking from calculations. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53904/diff/2-3/

Comment 33

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754390 [details]
MozReview Request: Bug 1272964: P1. Only activate skip to next keyframe logic when next keyframe time is known. r=gerald

https://reviewboard.mozilla.org/r/53934/#review50652

carrying r+ from incorrect mozreview push

Comment 34

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754391 [details]
MozReview Request: Bug 1272964: P2. Don't activate skip to next keyframe until we passed the internal seek target. r=gerald

https://reviewboard.mozilla.org/r/53936/#review50654

carrying r+ from incorrect mozreview push

Comment 35

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754392 [details]
MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r=gerald

https://reviewboard.mozilla.org/r/53938/#review50656

carrying r+ from incorrect mozreview push

Comment 36

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8754393 [details]
MozReview Request: Bug 1272964: P4. Only flush decoder if skip to next keyframe actually succeeds. r?cpearce

https://reviewboard.mozilla.org/r/53940/#review50804

Comment 37

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8754320 [details]
MozReview Request: Bug 1272964: P6. Exclude frames dropped due to internal seeking from calculations. r?cpearce

https://reviewboard.mozilla.org/r/53904/#review50806

::: dom/media/MediaFormatReader.cpp:1165
(Diff revision 3)
> -          decoder.mNumSamplesOutputTotal - mLastReportedNumDecodedFrames;
> +        decoder.mNumSamplesOutputTotal - mLastReportedNumDecodedFrames;
>          a.mDecoded = static_cast<uint32_t>(delta);
>          mLastReportedNumDecodedFrames = decoder.mNumSamplesOutputTotal;
>          nsCString error;
>          mVideo.mIsHardwareAccelerated =
> -          mVideo.mDecoder && mVideo.mDecoder->IsHardwareAccelerated(error);
> +        mVideo.mDecoder && mVideo.mDecoder->IsHardwareAccelerated(error);

"mVideo.mDecoder && mVideo.mDecoder->IsHardwareAccelerated(error);" should be indented.

Comment 38

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8754394 [details]
MozReview Request: Bug 1272964: [MSE] P5. Default to skipping to the next keyframe if no keyframe was found past currentTime. r?cpearce

https://reviewboard.mozilla.org/r/53942/#review50808

Comment 39

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

Comment on attachment 8754390 [details]
MozReview Request: Bug 1272964: P1. Only activate skip to next keyframe logic when next keyframe time is known. r=gerald

https://reviewboard.mozilla.org/r/53934/#review50874

Comment 40

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

Comment on attachment 8754391 [details]
MozReview Request: Bug 1272964: P2. Don't activate skip to next keyframe until we passed the internal seek target. r=gerald

https://reviewboard.mozilla.org/r/53936/#review50876

Comment 41

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

Comment on attachment 8754392 [details]
MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r=gerald

https://reviewboard.mozilla.org/r/53938/#review50878

Comment 42

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754390 [details]
MozReview Request: Bug 1272964: P1. Only activate skip to next keyframe logic when next keyframe time is known. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53934/diff/2-3/

Comment 43

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754391 [details]
MozReview Request: Bug 1272964: P2. Don't activate skip to next keyframe until we passed the internal seek target. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53936/diff/2-3/

Comment 44

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754392 [details]
MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r=gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53938/diff/2-3/

Comment 45

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754393 [details]
MozReview Request: Bug 1272964: P4. Only flush decoder if skip to next keyframe actually succeeds. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53940/diff/2-3/

Comment 46

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754394 [details]
MozReview Request: Bug 1272964: [MSE] P5. Default to skipping to the next keyframe if no keyframe was found past currentTime. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53942/diff/2-3/

Comment 47

[Jean-Yves Avenard [:jya]]

Comment on attachment 8754320 [details]
MozReview Request: Bug 1272964: P6. Exclude frames dropped due to internal seeking from calculations. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53904/diff/3-4/

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f15eecb6ba1
https://hg.mozilla.org/integration/mozilla-inbound/rev/13fd3929f83a
https://hg.mozilla.org/integration/mozilla-inbound/rev/937ac53c342e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c6bfbf57df9
https://hg.mozilla.org/integration/mozilla-inbound/rev/7725328be4f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da7e78b6d35

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f15eecb6ba1
https://hg.mozilla.org/mozilla-central/rev/13fd3929f83a
https://hg.mozilla.org/mozilla-central/rev/937ac53c342e
https://hg.mozilla.org/mozilla-central/rev/5c6bfbf57df9
https://hg.mozilla.org/mozilla-central/rev/7725328be4f5
https://hg.mozilla.org/mozilla-central/rev/1da7e78b6d35