SkipVideoDemuxToNextKeyFrame could incorrectly jump over gap in data.

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(6 attachments, 5 obsolete attachments)

58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
gerald
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
gerald
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
gerald
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
Details
(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8753250 [details]
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/
Attachment #8753250 - Flags: review?(gsquelart)
(Assignee)

Updated

3 years ago
Attachment #8753250 - Flags: review?(gsquelart)
(Assignee)

Comment 2

3 years ago
this fix has exposed a bug in MediaFormatReader ; need to look closer into it.
(Assignee)

Comment 3

3 years ago
Created attachment 8753653 [details]
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/
Attachment #8753250 - Attachment description: MozReview Request: Bug 1272964: [MSE] Do not skip over gaps when searching for the next keyframe. r?gerald → MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r?gerald
Attachment #8753653 - Flags: review?(gsquelart)
Attachment #8753654 - Flags: review?(gsquelart)
Attachment #8753250 - Flags: review?(gsquelart)
(Assignee)

Comment 4

3 years ago
Created attachment 8753654 [details]
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/
(Assignee)

Comment 5

3 years ago
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/
Attachment #8753653 - Flags: review?(gsquelart) → review+
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 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?
Attachment #8753654 - Flags: review?(gsquelart) → review+
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
Attachment #8753250 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 9

3 years ago
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/
(Assignee)

Comment 10

3 years ago
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/
(Assignee)

Comment 11

3 years ago
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/
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.
Flags: needinfo?(jyavenard)
(Assignee)

Comment 14

3 years ago
Created attachment 8754248 [details]
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/
Attachment #8754248 - Flags: review?(cpearce)
Attachment #8754249 - Flags: review?(cpearce)
(Assignee)

Comment 15

3 years ago
Created 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

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/
(Assignee)

Updated

3 years ago
Blocks: 1274190
(Assignee)

Comment 16

3 years ago
Created attachment 8754320 [details]
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/
Attachment #8754320 - Flags: review?(cpearce)
(Assignee)

Comment 17

3 years ago
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/
(Assignee)

Comment 18

3 years ago
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/
(Assignee)

Comment 19

3 years ago
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/
(Assignee)

Comment 20

3 years ago
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/
(Assignee)

Comment 21

3 years ago
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/
Attachment #8754320 - Attachment description: MozReview Request: Bug 1272964: P6. Decrement mSizeOfQueue when sample is dropped. r?cpearce → MozReview Request: Bug 1272964: P6. Exclude frames dropped due to internal seeking from calculations. r?cpearce
(Assignee)

Updated

3 years ago
Attachment #8753653 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8753654 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8753250 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8754248 - Attachment is obsolete: true
Attachment #8754248 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8754249 - Attachment is obsolete: true
Attachment #8754249 - Flags: review?(cpearce)
(Assignee)

Comment 22

3 years ago
Created attachment 8754390 [details]
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/
Attachment #8754390 - Flags: review?(gsquelart)
Attachment #8754391 - Flags: review?(gsquelart)
Attachment #8754392 - Flags: review?(gsquelart)
Attachment #8754393 - Flags: review?(cpearce)
Attachment #8754394 - Flags: review?(cpearce)
(Assignee)

Comment 23

3 years ago
Created attachment 8754391 [details]
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/
(Assignee)

Comment 24

3 years ago
Created attachment 8754392 [details]
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/
(Assignee)

Comment 25

3 years ago
Created attachment 8754393 [details]
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/
(Assignee)

Comment 26

3 years ago
Created 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

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/
(Assignee)

Comment 27

3 years ago
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/
Attachment #8754390 - Attachment description: MozReview Request: Bug 1272964: P1. Only activate skip to next keyframe logic when next keyframe time is known. r?gerald → MozReview Request: Bug 1272964: P1. Only activate skip to next keyframe logic when next keyframe time is known. r=gerald
Attachment #8754391 - Attachment description: MozReview Request: Bug 1272964: P2. Don't activate skip to next keyframe until we passed the internal seek target. r?gerald → MozReview Request: Bug 1272964: P2. Don't activate skip to next keyframe until we passed the internal seek target. r=gerald
Attachment #8754392 - Attachment description: MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r?gerald → MozReview Request: Bug 1272964: [MSE] P3. Do not skip over gaps when searching for the next keyframe. r=gerald
(Assignee)

Comment 28

3 years ago
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/
(Assignee)

Comment 29

3 years ago
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/
(Assignee)

Comment 30

3 years ago
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/
(Assignee)

Comment 31

3 years ago
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/
(Assignee)

Comment 32

3 years ago
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/
(Assignee)

Comment 33

3 years ago
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
Attachment #8754390 - Flags: review+
(Assignee)

Comment 34

3 years ago
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
Attachment #8754391 - Flags: review+
(Assignee)

Comment 35

3 years ago
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
Attachment #8754392 - Flags: review+
Attachment #8754390 - Flags: review?(gsquelart)
Attachment #8754391 - Flags: review?(gsquelart)
Attachment #8754392 - Flags: review?(gsquelart)
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
Attachment #8754393 - Flags: review?(cpearce) → review+
Attachment #8754320 - Flags: review?(cpearce) → review+
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.
Attachment #8754394 - Flags: review?(cpearce) → review+
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 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
Attachment #8754390 - Flags: review+
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
Attachment #8754391 - Flags: review+
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
Attachment #8754392 - Flags: review+
(Assignee)

Updated

3 years ago
Flags: needinfo?(jyavenard)
(Assignee)

Comment 42

3 years ago
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/
(Assignee)

Comment 43

3 years ago
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/
(Assignee)

Comment 44

3 years ago
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/
(Assignee)

Comment 45

3 years ago
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/
(Assignee)

Comment 46

3 years ago
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/
(Assignee)

Comment 47

3 years ago
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/
(Assignee)

Updated

3 years ago
Depends on: 1270323
(Assignee)

Updated

a year ago
Depends on: 1400674
You need to log in before you can comment on or make changes to this bug.