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

Issue 771481 link

Starred by 1 user

Issue metadata

Status: Fixed
Merged: issue 771482
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

CHECK failure: buffers_.empty() || CanAppendBuffersToEnd(new_buffers, new_buffers_group_start_p

Project Member Reported by ClusterFuzz, Oct 4 2017

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5113306363985920

Fuzzer: libFuzzer_mediasource_MP4_AACLC_AVC_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  buffers_.empty() || CanAppendBuffersToEnd(new_buffers, new_buffers_group_start_p
  SignalHandler
  media::SourceBufferRangeByPts::AppendBuffersToEnd
  
Sanitizer: memory (MSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=506132:506173

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5113306363985920

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.
 
Cc: msrchandra@chromium.org kkaluri@chromium.org
Components: Infra>Git
Labels: Test-Predator-Wrong M-63
Owner: wolenetz@chromium.org
Status: Assigned (was: Untriaged)
Predator could not provide any possible suspects.
Using CL for the file, "source_buffer_range_by_pts.cc" assigning to the concern owner who might be related.

Suspecting Commit# https://chromium.googlesource.com/chromium/src/+/8efe7217bc8d16be91cc94b0de80f6a5bd5d704f


wolenetz@-- Could you please look into the issue, kindly re-assign if this is not related to your changes.


Thank You.

Cc: chcunningham@chromium.org sande...@chromium.org
Components: -Infra>Git Internals>Media>Source
Mergedinto: 771482
Status: Duplicate (was: Assigned)
Similar root cause as  bug 771482  (out-of-order keyframe PTS versus prior keyframe PTS needs to trigger new coded frame group signalling when buffering by PTS).
Project Member

Comment 3 by ClusterFuzz, Nov 1 2017

ClusterFuzz has detected this issue as fixed in range 512978:513011.

Detailed report: https://clusterfuzz.com/testcase?key=5113306363985920

Fuzzer: libFuzzer_mediasource_MP4_AACLC_AVC_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  buffers_.empty() || CanAppendBuffersToEnd(new_buffers, new_buffers_group_start_p
  SignalHandler
  media::SourceBufferRangeByPts::AppendBuffersToEnd
  
Sanitizer: memory (MSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=506132:506173
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=512978:513011

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5113306363985920

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 14 2017

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

commit 469d91812fee7b2db7fd55d55bcb7663784c507d
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Tue Nov 14 03:28:27 2017

MSE: If timestampOffset change jumps far enough forward, introduce gap

When buffering by either PTS or DTS, and in segments AppendMode, if the
web app sets timestampOffset such that the next coded frame for a track
emitted from the coded frame processing algorithm is far enough in the
future, a buffered range gap can be introduced.

This change makes similar behavior occur when in sequence AppendMode.
Prior to this change, the intent of sequence mode was assumed to be to
not introduce such gaps when jumping forwards due to a newly set
timestampOffset. However, such behavior is neither explicit nor required
in the MSE spec, and implementing it, especially when buffering
compliantly ByPts, is complex and unintuitive. For example, should
previously buffered frames in the "gap" be removed? Worse, the previous
implementation used an implicit expansion of our buffering adjacency
"fudge room" to coalesce such a gap in sequence mode when buffering by
DTS, using inter-frame DTS deltas.

The net of this change also fixes one source of SBRByPts range adjacency
CHECK failures. Subsequent changes will address other sources of
related failures in the referenced bugs, below.

BUG= 771482 ,  771480 ,  771473 ,  771481 ,  771539 ,  781766 ,  775967 
TEST=Updated FrameProcessorTests and added LargeTimestampOffsetJumpForward

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: Iecc30c6ef6bf5c3f7433152cc62793806ecbd653
Reviewed-on: https://chromium-review.googlesource.com/757756
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516169}
[modify] https://crrev.com/469d91812fee7b2db7fd55d55bcb7663784c507d/media/filters/frame_processor.cc
[modify] https://crrev.com/469d91812fee7b2db7fd55d55bcb7663784c507d/media/filters/frame_processor.h
[modify] https://crrev.com/469d91812fee7b2db7fd55d55bcb7663784c507d/media/filters/frame_processor_unittest.cc

Labels: -M-63 M-64
I'm now unable to reproduce this issue; underlying parser change https://chromium-review.googlesource.com/747080 avoids the code fault (that still exists, leading to out-of-order keyframe_map_ in SBRByPts, like  bug 771482  and related). I'll continue working on those fixes. I confirmed that reverting https://chromium-review.googlesource.com/747080 allows repro locally of the CF issue, so I can still use the repro case (with that temporary revert) for deeper verification of the fix for this group of related issues.
Project Member

Comment 6 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

With https://chromium-review.googlesource.com/747080 temporarily reverted locally, this issue is not yet fixed (#4 + #6 + that revert still repro this issue on ToT). Continuing to investigate...
Status: Started (was: Duplicate)
Unduplicating this issue from  bug 771482  since I'm working on a fix specific to the remaining root cause of this bug.
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 22 2017

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

commit 9ef958b55002bbd52bae1f27822c6ba7fa7aaab6
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Wed Nov 22 00:09:24 2017

MSE: Start with a small fudge room that never decreases

This change prevents initial 0-duration buffers from causing a large
default fudge room (used for determining range membership, adjacency and
coalescing) to cause inconsistent "yes, that timestamp for the next buffer
is adjacent" and "oops, now that I see that next buffer has a non-zero
duration smaller than the default fudge room, it's not really adjacent"
scenarios.

Now, the fudge room is prevented from shrinking. As larger frame
durations or non-zero inter-buffer decode timestamp deltas are observed
in a continuous (per MSE coded frame processing algorithm) DTS sequence
for a track, this fudge room grows. This change also changes the default
minimum fudge room to 2 milliseconds.

I confirmed locally this also fixes the underlying issue in  bug 771481 ,
whose repro was hidden by the more recent parser fix in
https://chromium-review.googlesource.com/747080.

BUG= 771481 
TEST=FrameProcessorTest.SegmentsMode_BufferingByPts_InitialZeroDurationBuffers

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: Idb658410fffc9dfd091de35b2d9ff8ee70486dad
Reviewed-on: https://chromium-review.googlesource.com/783331
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518470}
[modify] https://crrev.com/9ef958b55002bbd52bae1f27822c6ba7fa7aaab6/media/filters/frame_processor_unittest.cc
[modify] https://crrev.com/9ef958b55002bbd52bae1f27822c6ba7fa7aaab6/media/filters/source_buffer_stream.cc

Status: Fixed (was: Started)

Sign in to add a comment