New issue
Advanced search Search tips

Issue 773115 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Deep SAP-Type-2 sequences can trigger CHECK failure when MseBufferByPts feature enabled

Project Member Reported by wolenetz@chromium.org, Oct 9 2017

Issue description

Repro steps:
1. Start M63 Chrome with --enable-features=MseBufferByPts
2. Browse to nest.com, login and view one or more live camera streams.

Observe failed CHECK:

FATAL:source_buffer_range_by_pts.cc(69)] Check failed: buffers_.empty() || CanAppendBuffersToEnd(new_buffers, new_buffers_group_start_pts)

Extra logging shows that the SAP-Type-2 sequence being appended begins with a keyframe too far into the future to be adjacent to the range that is otherwise adjacent (just before) the presentation interval of the GOP that's being appended.

[1:1:1009/155310.927168:VERBOSE1:source_buffer_stream.cc(294)] Append VIDEO: buffers dts=[1200000us;1800000us(last frame dur=66666us)], pts interval=[1200000us,1999999us), buffers:
	dts=1200000 timestamp=1333333 duration=66666 size=74747 side_data_size=0 is_key_frame=1 encrypted=0 discard_padding (ms)=(0, 0)
	dts=1266666 timestamp=1200000 duration=66666 size=137 side_data_size=0 is_key_frame=0 encrypted=0 discard_padding (ms)=(0, 0)
	dts=1333333 timestamp=1266666 duration=66666 size=75 side_data_size=0 is_key_frame=0 encrypted=0 discard_padding (ms)=(0, 0)
	dts=1400000 timestamp=1533333 duration=66666 size=436 side_data_size=0 is_key_frame=0 encrypted=0 discard_padding (ms)=(0, 0)
	dts=1466666 timestamp=1400000 duration=66666 size=74 side_data_size=0 is_key_frame=0 encrypted=0 discard_padding (ms)=(0, 0)
	dts=1533333 timestamp=1466666 duration=66666 size=78 side_data_size=0 is_key_frame=0 encrypted=0 discard_padding (ms)=(0, 0)
	dts=1600000 timestamp=1733333 duration=66666 size=305 side_data_size=0 is_key_frame=0 encrypted=0 discard_padding (ms)=(0, 0)
	dts=1666666 timestamp=1600000 duration=66666 size=74 side_data_size=0 is_key_frame=0 encrypted=0 discard_padding (ms)=(0, 0)
	dts=1733333 timestamp=1666666 duration=66666 size=74 side_data_size=0 is_key_frame=0 encrypted=0 discard_padding (ms)=(0, 0)
	dts=1800000 timestamp=1933333 duration=66666 size=205 side_data_size=0 is_key_frame=0 encrypted=0 discard_padding (ms)=(0, 0)

 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 15 2017

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

commit d5f07c6849faa9a88e6358eb001de5a75443d791
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Nov 15 20:43:05 2017

MSE: When buffering ByPts, handle keyframe with PTS < previous keyframe

If FrameProcessor doesn't signal SourceBufferStream of a new coded frame
group when buffering ByPts and an otherwise continuous (in DTS) frame
sequence has keyframe PTS that does not increase, then code faults are
hit. Specifically,

A) the underlying SourceBufferRangeByPts could have its keyframe map
   become out of sequence with respect to the buffers it indexes,
   violating assumptions of its ordering, and

B) internal adjacency checks for the new frame with the previous append
   can fail.

This change adds such signalling to FrameProcessor to avoid these faults
when keyframe PTS sequence decreases in a CFG when buffering ByPts.
Note that "deep" SAP-Type-2 sequences can still trigger (B), tracked by
 bug 773115 .

TEST=New test cases' ProcessFrames calls triggered A or B previously:
   SegmentsModeNewByPts/FrameProcessorTest.OOOKeyframePts_1/0 (Hit A)
   SequenceModeNewByPts/FrameProcessorTest.OOOKeyframePts_1/0 (Hit A)*
   SegmentsModeNewByPts/FrameProcessorTest.OOOKeyframePts_2/0 (Hit B)
   SequenceModeNewByPts/FrameProcessorTest.OOOKeyframePts_2/0 (Hit B)
   * - this test also triggered (B) prior to landing prereq CL 469d9181

BUG= 771482 ,  771480 ,  771473 ,  771481 ,  771539 ,  781766 ,  775967 ,  773115 

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I80902f3ce1c0a9262459df0a1092d6bb5cbc400f
Reviewed-on: https://chromium-review.googlesource.com/728860
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516814}
[modify] https://crrev.com/d5f07c6849faa9a88e6358eb001de5a75443d791/media/filters/frame_processor.cc
[modify] https://crrev.com/d5f07c6849faa9a88e6358eb001de5a75443d791/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/d5f07c6849faa9a88e6358eb001de5a75443d791/media/filters/source_buffer_range.cc

Status: Started (was: Assigned)
I have a fix I'm about to send for review (https://chromium-review.googlesource.com/c/chromium/src/+/777778).
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 6 2017

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

commit c4c936cf14b8c7f125a6087241b7b7983644ee49
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Dec 06 02:30:27 2017

MSE: Signal SBS of new CFG more granularly when buffering ByPts

This change includes multiple fixes for various cases where
SourceBufferStream requires notification of a new coded frame group when
buffering by PTS (currently available via kMseBufferByPts feature) even
if there is no MSE coded frame processing algorithm DTS discontinuity
detected. This additional granularity allows for SourceBufferStream to
understand that the next buffers appended to it may overlap recently
appended buffers, and also allows for handling cases where the next
keyframe (continuous in DTS) jumps significantly forward in PTS but
still needs to remain continuous with the current append sequence.

On every keyframe, when buffering by PTS, FrameProcessor now considers
doing the extra signalling if DTS is continuous and buffering ByPts,
and:
a) keyframe PTS jumps significantly into the future relative to the
   highest PTS emitted already in the current coded frame group (in
   which case the highest PTS emitted already in the current CFG is used
   as the signalled PTS value), or
b) keyframe PTS is before the highest PTS emitted already in the current
   coded frame group (in which case the keyframe PTS is used as the
   signalled PTS value and tracking of the highest PTS emitted is reset
   to enable correct detection and signalling of both (a) and (b) cases
   for future keyframes in the continuous DTS append sequence.)

This change also includes a fix to SourceBufferStream when buffering
ByPts to appropriately split a range that is being overlap-appended.

A benign DCHECK is also removed from SBS::UpdateLastAppendStateForRemove
which could fail for a SAP Type 2 GOP at the beginning of a range (the
last appended buffer in that range might indeed be a non-keyframe with a
PTS prior to the range start time, which could be as late as the PTS of
the keyframe of that non-keyframe's GOP.)

New and updated unit tests are included. I locally confirmed this fixes
bugs  773115  and  788344 , and appears to fix nest.com and twitch.com
renderer crashes with dcheck_always_on=true.

Pre-existing  bug 791095  is demonstrated, but not yet fixed, by new
unit tests:
  BufferingByPts_ContinuousDts_SapType2_and_PtsJumpForward
  BufferingByPts_ContinuousDts_NewSap2GopEndOverlapsLastGop_1

Pre-existing bug 763620 is demonstrated, but not yet fixed, by new
unit tests:
  BufferingByPts_ContinuousDts_NewGopEndOverlapsLastGop_2
  BufferingByPts_ContinuousDts_NewSap2GopEndOverlapsLastGop_2
  BufferingByPts_ContinuousDts_GopKeyframePtsOrder_2_1_3

BUG= 773115 , 788344 , 791095 ,763620

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I6f95cf85e1d1fa5b5f74ed1d99a3853ec6ccf686
Reviewed-on: https://chromium-review.googlesource.com/777778
Reviewed-by: Sergey Volk <servolk@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521961}
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/base/test_helpers.h
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/filters/chunk_demuxer_unittest.cc
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/filters/frame_processor.cc
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/c4c936cf14b8c7f125a6087241b7b7983644ee49/media/filters/source_buffer_stream_unittest.cc

Status: Fixed (was: Started)
Labels: -M-64 M-65

Sign in to add a comment