New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 922893 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Today
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug

Blocking:
issue 760264



Sign in to add a comment

When MseBufferByPts, SBS::IsDtsMonotonicallyIncreasing shouldn't compare current GOP frames' DTS to previous GOPs' DTS

Reported by lunaz...@gmail.com, Jan 17 (5 days ago)

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36

Steps to reproduce the problem:
1. navigator to https://luna2216.github.io/#/sample/appenderror/
2. open the console
3. An error "CHUNK_DEMUXER_ERROR_APPEND_FAILED: Buffers did not monotonically increase." is shown

What is the expected behavior?
the video can play without any error

What went wrong?
The video element throws an error: CHUNK_DEMUXER_ERROR_APPEND_FAILED: Buffers did not monotonically increase.

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 71.0.3578.98  Channel: stable
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
Flash Version: 

I'm able to reproduce this on some of Win 7 PCs, not able to reproduce this on Win 10 and OSX.
 

Comment 1 by dtapu...@chromium.org, Jan 17 (5 days ago)

Components: Internals>Media>Video

Comment 2 by vamshi.kommuri@chromium.org, Jan 17 (5 days ago)

Labels: Needs-Triage-M71

Comment 3 by dalecur...@chromium.org, Jan 17 (5 days ago)

Cc: wolenetz@chromium.org

Comment 4 by wolenetz@chromium.org, Jan 17 (5 days ago)

I have a repro on tip of tree on linux, with --enable-features=MseBufferByPts. Note, on current stable without MseBufferByPts enabled, I could not obtain a repro.

From this small dataset, this looks like one potential source of increased CHUNK_DEMUXER_ERROR_APPEND_FAILED with MseBufferByPts.

lunazzlb@, would you please check a repro's media player log in chrome://media-internals and report which of the following lines you see:
info	ChunkDemuxer: buffering by DTS
or
info	ChunkDemuxer: buffering by PTS
If I understand correctly, unless you're somewhow forcing --enable-features=MseBufferByPts, you shouldn't be seeing the latter line in a repro on Chrome 71.0.3578.98 stable -- are you enabling that feature in your repros?

Meanwhile, I do have a repro at least when buffering by PTS, so I'll use this to investigate.

Comment 5 by wolenetz@chromium.org, Jan 17 (5 days ago)

Blocking: 760264
Labels: -Pri-2 Needs-Feedback Pri-1
Owner: wolenetz@chromium.org
Status: Assigned (was: Unconfirmed)
->P1 since this is one route exposing an append failure seen with MseBufferByPts at a layer lower than the parser (the parser and frame processor should protect against this particular "Buffers did not monotonically increase" error occurring, except I think I may see how in ByPts logic, that protection may be broken.)

Comment 6 by wolenetz@chromium.org, Jan 17 (5 days ago)

Components: -Internals>Media>Video Internals>Media>Source

Comment 7 by wolenetz@chromium.org, Jan 17 (5 days ago)

Pertinent info from ByPts local repro using custom build with extra logging:

1. Append init segment:
  source_buffer_state.cc(682)] Video track_id=1 config: codec: h264, format: PIXEL_FORMAT_I420, profile: h264 baseline, coded size: [640,360], visible rect: [0,0,640,360], natural size: [640,360], has extra data: false, encryption scheme: Unencrypted, rotation: 0°

2. Append (and process in chunks of up to 128KB) a media segment:
  source_buffer.cc(367)] appendBuffer this=xxx media_time=0 size=163936
  ... 1st chunk ...
  frame_processor.cc(518)] NotifyStartOfCodedFrameGroup(dts 4782930946509us, pts 4782930946509us)
  chunk_demuxer.cc(258)] ChunkDemuxerStream::OnStartOfCodedFrameGroup(dts 4.78293e+06, pts 4.78293e+06)
  source_buffer_stream.cc(254)] OnStartOfCodedFrameGroup VIDEO (dts 4782930946509us, pts 4782930946509us)
  source_buffer_stream.cc(277)] OnStartOfCodedFrameGroupInternal next appended buffers will be in a new range
  ...
  source_buffer_stream.cc(317)] Append VIDEO: buffers dts=[4782930946509us;4782932386509us(last frame dur=40000us)], pts interval=[4782930946509us,4782932426509us)
  ...
  source_buffer_stream.cc(500)] Append VIDEO: done. ranges_=[4782930946509us;4782932386509us(4782932426509us)]

  ... 2nd chunk ...
  source_buffer_stream.cc(317)] Append VIDEO: buffers dts=[4782932426509us;4782932906509us(last frame dur=40000us)], pts interval=[4782932426509us,4782932946509us)
  ...
  source_buffer_stream.cc(500)] Append VIDEO: done. ranges_=[4782930946509us;4782932906509us(4782932946509us)]

(All looks ok so far)

2. Append init segment
  source_buffer_state.cc(682)] Video track_id=1 config: codec: h264, format: PIXEL_FORMAT_I420, profile: h264 high, coded size: [1920,1080], visible rect: [0,0,1920,1080], natural size: [1920,1080], has extra data: false, encryption scheme: Unencrypted, rotation: 0°

3. Append (and process in chunks of up to 128KB) more media (at least the first part is the beginning of a media segment):
  source_buffer.cc(367)] appendBuffer this=xxx media_time=4.78293e+06 size=1336079
  ... 1st chunk ...
  mp4_stream_parser.cc(920)] Emit video frame:  track_id=1, key=1, dur=40, dts=4782932866, cts=4782932946, size=228326
  source_buffer_state.cc(910)] OnNewBuffers buffer_queues=1
  frame_processor.cc(671)] ProcessFrame: Processing frame Type=2, TrackID=1, PTS=4782932946509us, DTS=4782932866509us, DUR=40000us, RAP=1
  frame_processor.cc(473)] Reset()
  frame_processor.cc(207)] Reset()
  frame_processor.cc(825)] ProcessFrame: Discontinuity: reprocessing frame
  frame_processor.cc(671)] ProcessFrame: Processing frame Type=2, TrackID=1, PTS=4782932946509us, DTS=4782932866509us, DUR=40000us, RAP=1
  frame_processor.cc(527)] FlushProcessedFrames()
  frame_processor.cc(518)] NotifyStartOfCodedFrameGroup(dts 4782932866509us, pts 4782932946509us)
  chunk_demuxer.cc(258)] ChunkDemuxerStream::OnStartOfCodedFrameGroup(dts 4.78293e+06, pts 4.78293e+06)
  source_buffer_stream.cc(254)] OnStartOfCodedFrameGroup VIDEO (dts 4782932866509us, pts 4782932946509us)
  source_buffer_range_by_pts.cc(517)] BelongsToRange
  source_buffer_stream.cc(302)] OnStartOfCodedFrameGroupInternal next appended buffers will continue range unless intervening remove makes discontinuity
  frame_processor.cc(996)] ProcessFrame: Sending processed frame to stream, PTS=4782932946509us, DTS=4782932866509us
  frame_processor.cc(527)] FlushProcessedFrames()
  source_buffer_stream.cc(317)] Append VIDEO: buffers dts=[4782932866509us;4782932866509us(last frame dur=40000us)], pts interval=[4782932946509us,4782932986509us)
  ERROR:render_media_log.cc(30)] MediaEvent: MEDIA_ERROR_LOG_ENTRY {"error":"Buffers did not monotonically increase."}


Analysis of this will be in next comment(s).

Comment 8 by wolenetz@chromium.org, Jan 17 (5 days ago)

@#7:
1st media segment ends with
        DTS: 4782932906509
highest PTS: 4782932906509

2nd media segment begins with:
        DTS: 4782932866509
        PTS: 4782932946509

Correctly, SourceBufferStream determines the 2nd media segment to be continuous with the end of the first (byPts), but the DTS across the two is *not* continuous.

However, when buffering-by-PTS, DTS continuity should only matter *within* a GOP, not between two GOPs. In buffering-by-PTS, the SourceBufferStream's OnStartOfCodedFrameGroupInternal (or more precisely), IsDtsMonotonicallyIncreasing should not compare one GOP's DTS with the previous GOP's DTS. (Certainly in legacy buffering-by-dts mode, it *should*, but not in ByPts).

It looks like this is an implementation issue for MseBufferByPts, *thank you* lunazzlb@ for assisting by providing such a timely repro!

Comment 9 by wolenetz@chromium.org, Jan 17 (5 days ago)

Cc: -wolenetz@chromium.org hbengali@chromium.org xhw...@chromium.org dalecur...@chromium.org
Folks on cc:, this issue may be the root cause of CHUNK_DEMUXER_ERROR_APPEND PipelineStatus increase in MseBufferByPts experimentation, or at least one source of such increase.

Comment 10 by wolenetz@chromium.org, Jan 17 (5 days ago)

Summary: When MseBufferByPts, SBS::IsDtsMonotonicallyIncreasing shouldn't compare current GOP frames' DTS to previous GOPs' DTS (was: video give "Buffers did not monotonically increase." error when append segments on some PC)

Comment 11 by wolenetz@chromium.org, Jan 17 (5 days ago)

Cc: sande...@chromium.org

Comment 12 by wolenetz@chromium.org, Jan 17 (5 days ago)

@#7-8, when not MseBufferByPts (when instead, using legacy buffering-by-DTS logic), the append in part 3 succeeds because buffering-by-DTS detects the dts discontinuity and handles the buffering overlap:
  source_buffer_stream.cc(243)] OnStartOfCodedFrameGroup VIDEO (dts 4782932866509us, pts 4782932946509us)
  source_buffer_stream.cc(277)] OnStartOfCodedFrameGroupInternal next appended buffers will overlap an existing range

Comment 13 by wolenetz@chromium.org, Jan 17 (5 days ago)

Status: Started (was: Assigned)
CL in progress: https://chromium-review.googlesource.com/c/chromium/src/+/1419102

Project Member

Comment 14 by bugdroid1@chromium.org, Jan 18 (4 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf05d740a3b21c514a689fd331051701b04293a6

commit bf05d740a3b21c514a689fd331051701b04293a6
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Fri Jan 18 20:55:41 2019

MSE-BufferByPts: Do not require DTS monotonicity across GOPs, just within GOPs

When buffering by PTS (e.g., --enable-features=MseBufferByPts),
monotonically increasing DTS should only be required within GOPs in
SourceBufferStream::Append() operations. This change relaxes existing
checks for DTS monotonicity in SBS::IsDtsMonotonicallyIncreasing() to be
within GOPs, not across GOPs, when buffering by PTS.

This change also makes failure of SBS::IsDtsMonotonicallyIncreasing() cause
crash because any such failure is an implementation issue that would need
fixing rather than a parse error.

While updating other existing test cases based on the new behavior, I
included in this CL a further update to require all frames to be keyframes in
audio SourceBufferStreams. FrameProcessor already forces this. Audio SBS
tests now use only keyframes.

BUG= 922893 
TEST=new */FrameProcessorTest.ContinuousPts_DiscontinousDts_AcrossGops* tests,
and updated SourceBufferStreamTest.SameTimestamp_*

Change-Id: Iac4177679b605cdeb1bd8b0e1d7b5b76853d645a
Reviewed-on: https://chromium-review.googlesource.com/c/1419102
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624277}
[modify] https://crrev.com/bf05d740a3b21c514a689fd331051701b04293a6/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/bf05d740a3b21c514a689fd331051701b04293a6/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/bf05d740a3b21c514a689fd331051701b04293a6/media/filters/source_buffer_stream_unittest.cc

Comment 15 by wolenetz@google.com, Jan 18 (4 days ago)

Cc: abdulsyed@chromium.org
Labels: M-72
Follow-up CL to eliminate another (hopefully less-likely) source of CHUNK_DEMUXER_ERROR_APPEND_FAILED is https://chromium-review.googlesource.com/c/chromium/src/+/1423357

+abdulsyed@ - I expect to request merge-to-M72 ASAP once these two bake this weekend. I understand this is very close to the upcoming Stable M72 cut. At least #14 should fix what is highly suspected as being the root cause of the only significant regression seen in experimentation of MseBufferByPts feature (versus existing/legacy ByDts). I can share stats, but the number of broken playbacks (under MseBufferByPts) without #14 is significant, so earlier verification in a stable channel that the issue is fixed would really be helpful.

Comment 16 by wolenetz@google.com, Jan 18 (4 days ago)

Cc: gov...@chromium.org
+govind@ also per abdulsyed@ is currently OoO
Please see c#15.

Comment 17 by gov...@chromium.org, Jan 18 (4 days ago)

NextAction: 2019-01-22
Pls update bug with canary result on Tuesday morning. I will let abdulsyed@ to do M72 merge review as this is regressed in M71 and M72 is very close to stable promotion. Thank you.

Comment 18 by wolenetz@google.com, Jan 18 (4 days ago)

@#17 Will do. Thank you, govind@.
Project Member

Comment 19 by bugdroid1@chromium.org, Jan 18 (4 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9963e505940bcc41f290b19c29a18b4030e80fbe

commit 9963e505940bcc41f290b19c29a18b4030e80fbe
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Fri Jan 18 23:35:47 2019

MSE-BufferByPts(andByDts): Enforce non-negative timestamp assumptions in SBS::Append

Once FrameProcessor has a frame that has survived processing (e.g.,
timestampOffset application, satisfying any needs-random-access-point
condition for the frame's track, append-window trimming and filtering,
and failing parse for resulting negative DTS if buffering-by-DTS), it
enqueues such a frame for later SourceBufferStream::Append. If it is the
first frame that FrameProcessor thinks may begin a new coded frame
group, it also signals SourceBufferStream::OnStartOfNewCodedFrameGroup
with the frame's DTS and PTS.

The net result is that two conditions (basically that the DTS or PTS,
depending on runtime buffering model, is non-negative in the result of
OnStartOfNewCodedFrameGroup processing and for the first frame in the
queue of frames in an Append), for which SourceBufferStream would
MEDIA_LOG and emit a parse error if they were not met, should never
fail.

This change makes those conditions CHECKS instead of parse errors, to
enable fuzzing to help verify those two conditions are invariant.

BUG= 922893 

Change-Id: Icae9fdc908943103b8ffea717c35060deeaef0d4
Reviewed-on: https://chromium-review.googlesource.com/c/1423357
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624352}
[modify] https://crrev.com/9963e505940bcc41f290b19c29a18b4030e80fbe/media/filters/source_buffer_stream.cc

Comment 20 by monor...@bugs.chromium.org, Today (18 hours ago)

The NextAction date has arrived: 2019-01-22

Comment 21 by wolenetz@google.com, Today (10 hours ago)

Labels: Merge-Request-72
abdulsyed@ (and govind@): Please review merge request of both #14 and #19 to M72. They baked over the weekend. None of the crashes in Chrome >= 73.0.3677.0 are failures of these more strict CHECKs. None of my local fuzzing of 19 mediasource fuzzers over the long weekend detected any regression with those two changes. Clusterfuzz hasn't yet picked those 2 changes up, but I'd expect that these are safe to merge to M72 given this data.
Project Member

Comment 22 by sheriffbot@chromium.org, Today (10 hours ago)

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: We are only 6 days from stable.
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 23 by abdulsyed@google.com, Today (10 hours ago)

Labels: -Merge-Review-72 Merge-Approved-72
Approved for M72. branch:3626
Project Member

Comment 24 by bugdroid1@chromium.org, Today (10 hours ago)

Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f15abc56d3851ef8407fa3e36ea5723f2a83e80a

commit f15abc56d3851ef8407fa3e36ea5723f2a83e80a
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Tue Jan 22 20:07:23 2019

To M72: MSE-BufferByPts: Do not require DTS monotonicity across GOPs, just within GOPs

When buffering by PTS (e.g., --enable-features=MseBufferByPts),
monotonically increasing DTS should only be required within GOPs in
SourceBufferStream::Append() operations. This change relaxes existing
checks for DTS monotonicity in SBS::IsDtsMonotonicallyIncreasing() to be
within GOPs, not across GOPs, when buffering by PTS.

This change also makes failure of SBS::IsDtsMonotonicallyIncreasing() cause
crash because any such failure is an implementation issue that would need
fixing rather than a parse error.

While updating other existing test cases based on the new behavior, I
included in this CL a further update to require all frames to be keyframes in
audio SourceBufferStreams. FrameProcessor already forces this. Audio SBS
tests now use only keyframes.

BUG= 922893 
TEST=new */FrameProcessorTest.ContinuousPts_DiscontinousDts_AcrossGops* tests,
and updated SourceBufferStreamTest.SameTimestamp_*

Change-Id: Iac4177679b605cdeb1bd8b0e1d7b5b76853d645a
Reviewed-on: https://chromium-review.googlesource.com/c/1419102
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624277}(cherry picked from commit bf05d740a3b21c514a689fd331051701b04293a6)

TBR=sandersd@chromium.org

Change-Id: Ib781f983332a588deb60f338b4c7a9aaac84fa7a
Reviewed-on: https://chromium-review.googlesource.com/c/1427601
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#759}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/f15abc56d3851ef8407fa3e36ea5723f2a83e80a/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/f15abc56d3851ef8407fa3e36ea5723f2a83e80a/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/f15abc56d3851ef8407fa3e36ea5723f2a83e80a/media/filters/source_buffer_stream_unittest.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Today (10 hours ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8cd13d90b0a07e240e8bb398fae8d46609d564c4

commit 8cd13d90b0a07e240e8bb398fae8d46609d564c4
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Tue Jan 22 20:09:27 2019

To M72: MSE-BufferByPts(andByDts): Enforce non-negative timestamp assumptions in SBS::Append

Once FrameProcessor has a frame that has survived processing (e.g.,
timestampOffset application, satisfying any needs-random-access-point
condition for the frame's track, append-window trimming and filtering,
and failing parse for resulting negative DTS if buffering-by-DTS), it
enqueues such a frame for later SourceBufferStream::Append. If it is the
first frame that FrameProcessor thinks may begin a new coded frame
group, it also signals SourceBufferStream::OnStartOfNewCodedFrameGroup
with the frame's DTS and PTS.

The net result is that two conditions (basically that the DTS or PTS,
depending on runtime buffering model, is non-negative in the result of
OnStartOfNewCodedFrameGroup processing and for the first frame in the
queue of frames in an Append), for which SourceBufferStream would
MEDIA_LOG and emit a parse error if they were not met, should never
fail.

This change makes those conditions CHECKS instead of parse errors, to
enable fuzzing to help verify those two conditions are invariant.

BUG= 922893 

Change-Id: Icae9fdc908943103b8ffea717c35060deeaef0d4
Reviewed-on: https://chromium-review.googlesource.com/c/1423357
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624352}(cherry picked from commit 9963e505940bcc41f290b19c29a18b4030e80fbe)

TBR=sandersd@chromium.org

Change-Id: I711333870a1b76aa293846142db53ac9cbb28882
Reviewed-on: https://chromium-review.googlesource.com/c/1427603
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#760}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/8cd13d90b0a07e240e8bb398fae8d46609d564c4/media/filters/source_buffer_stream.cc

Project Member

Comment 26 by cr-audit...@appspot.gserviceaccount.com, Today (10 hours ago)

Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/8cd13d90b0a07e240e8bb398fae8d46609d564c4

Commit: 8cd13d90b0a07e240e8bb398fae8d46609d564c4
Author: wolenetz@chromium.org
Commiter: wolenetz@chromium.org
Date: 2019-01-22 20:09:27 +0000 UTC

To M72: MSE-BufferByPts(andByDts): Enforce non-negative timestamp assumptions in SBS::Append

Once FrameProcessor has a frame that has survived processing (e.g.,
timestampOffset application, satisfying any needs-random-access-point
condition for the frame's track, append-window trimming and filtering,
and failing parse for resulting negative DTS if buffering-by-DTS), it
enqueues such a frame for later SourceBufferStream::Append. If it is the
first frame that FrameProcessor thinks may begin a new coded frame
group, it also signals SourceBufferStream::OnStartOfNewCodedFrameGroup
with the frame's DTS and PTS.

The net result is that two conditions (basically that the DTS or PTS,
depending on runtime buffering model, is non-negative in the result of
OnStartOfNewCodedFrameGroup processing and for the first frame in the
queue of frames in an Append), for which SourceBufferStream would
MEDIA_LOG and emit a parse error if they were not met, should never
fail.

This change makes those conditions CHECKS instead of parse errors, to
enable fuzzing to help verify those two conditions are invariant.

BUG= 922893 

Change-Id: Icae9fdc908943103b8ffea717c35060deeaef0d4
Reviewed-on: https://chromium-review.googlesource.com/c/1423357
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624352}(cherry picked from commit 9963e505940bcc41f290b19c29a18b4030e80fbe)

TBR=sandersd@chromium.org

Change-Id: I711333870a1b76aa293846142db53ac9cbb28882
Reviewed-on: https://chromium-review.googlesource.com/c/1427603
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#760}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Project Member

Comment 27 by cr-audit...@appspot.gserviceaccount.com, Today (10 hours ago)

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/f15abc56d3851ef8407fa3e36ea5723f2a83e80a

Commit: f15abc56d3851ef8407fa3e36ea5723f2a83e80a
Author: wolenetz@chromium.org
Commiter: wolenetz@chromium.org
Date: 2019-01-22 20:07:23 +0000 UTC

To M72: MSE-BufferByPts: Do not require DTS monotonicity across GOPs, just within GOPs

When buffering by PTS (e.g., --enable-features=MseBufferByPts),
monotonically increasing DTS should only be required within GOPs in
SourceBufferStream::Append() operations. This change relaxes existing
checks for DTS monotonicity in SBS::IsDtsMonotonicallyIncreasing() to be
within GOPs, not across GOPs, when buffering by PTS.

This change also makes failure of SBS::IsDtsMonotonicallyIncreasing() cause
crash because any such failure is an implementation issue that would need
fixing rather than a parse error.

While updating other existing test cases based on the new behavior, I
included in this CL a further update to require all frames to be keyframes in
audio SourceBufferStreams. FrameProcessor already forces this. Audio SBS
tests now use only keyframes.

BUG= 922893 
TEST=new */FrameProcessorTest.ContinuousPts_DiscontinousDts_AcrossGops* tests,
and updated SourceBufferStreamTest.SameTimestamp_*

Change-Id: Iac4177679b605cdeb1bd8b0e1d7b5b76853d645a
Reviewed-on: https://chromium-review.googlesource.com/c/1419102
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624277}(cherry picked from commit bf05d740a3b21c514a689fd331051701b04293a6)

TBR=sandersd@chromium.org

Change-Id: Ib781f983332a588deb60f338b4c7a9aaac84fa7a
Reviewed-on: https://chromium-review.googlesource.com/c/1427601
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#759}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Comment 28 by wolenetz@google.com, Today (8 hours ago)

Status: Fixed (was: Started)
lunazzlb@ : if you retry your repro with Chrome Canary version >= 73.0.3677.0 and --enable-features=MseBufferByPts on command line, you should no longer get the append error. Aside: if you instead force old buffering mode using --disable-features=MseBufferByPts on command line, at the resolution transition in your repro, there is a noticeably larger frozen frame than when MseBufferByPts is enabled. This is because the older ByDts buffering model uses decode timestamp intervals for buffering (and buffered range reporting), and the DTS at the beginning of the higher resolution latter portion of the repro media overlaps the end of the lower resolution media. So the overlapped media is removed from being fed to the decoder. In both cases, the actual presentation timestamp (not decode timestamp) of the media is used to play decoded results. This explains the lack of a buffered range gap in ByDts and the presence of a noticeable pause during playback in ByDts.

Note that the ByPts behavior is compliant with spec, also, hence we're focused on getting it shipped. Your repro (this bug) is hopefully allowing us to fix the last significant issue blocking such shipping. Again, thank you.

Comment 29 by wolenetz@chromium.org, Today (8 hours ago)

NextAction: ----

Sign in to add a comment