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 descriptionUserAgent: 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.
,
Jan 17
(5 days ago)
,
Jan 17
(5 days ago)
,
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.
,
Jan 17
(5 days ago)
->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.)
,
Jan 17
(5 days ago)
,
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).
,
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!
,
Jan 17
(5 days ago)
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.
,
Jan 17
(5 days ago)
,
Jan 17
(5 days ago)
,
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
,
Jan 17
(5 days ago)
CL in progress: https://chromium-review.googlesource.com/c/chromium/src/+/1419102
,
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
,
Jan 18
(4 days ago)
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.
,
Jan 18
(4 days ago)
+govind@ also per abdulsyed@ is currently OoO Please see c#15.
,
Jan 18
(4 days ago)
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.
,
Jan 18
(4 days ago)
@#17 Will do. Thank you, govind@.
,
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
,
Today
(18 hours ago)
The NextAction date has arrived: 2019-01-22
,
Today
(10 hours ago)
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.
,
Today
(10 hours ago)
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
,
Today
(10 hours ago)
Approved for M72. branch:3626
,
Today
(10 hours ago)
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
,
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
,
Today
(10 hours ago)
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}
,
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}
,
Today
(8 hours ago)
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.
,
Today
(8 hours ago)
|
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by dtapu...@chromium.org
, Jan 17 (5 days ago)