New issue
Advanced search Search tips

Issue 798935 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug

Blocking:
issue 804802



Sign in to add a comment

Abrt in media::SourceBufferRangeByPts::AppendBuffersToEnd

Project Member Reported by ClusterFuzz, Jan 4 2018

Issue description

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

Fuzzer: libFuzzer_mediasource_MP2T_AACLC_AVC_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: Abrt
Crash Address: 0x03e900004902
Crash State:
  media::SourceBufferRangeByPts::AppendBuffersToEnd
  media::SourceBufferStream<media::SourceBufferRangeByPts>::Append
  media::ChunkDemuxerStream::Append
  
Sanitizer: memory (MSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=518239:518261

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Jan 4 2018

Components: Internals>Media
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Cc: kkaluri@chromium.org
Labels: M-64 Test-Predator-Wrong
Owner: wolenetz@chromium.org
Status: Assigned (was: Untriaged)
With reference to the following  Issue 793247 , assigning it to wolenetz@

wolenetz@ Could you please look into it
Cc: chcunningham@chromium.org
Components: -Internals>Media Internals>Media>Source
Status: Started (was: Assigned)
I have a local repro in the debug ASAN version of this fuzzer on linux.
Interesting.  SAP-Type-2 and 0 duration buffer is in the repro case. At least the SAP-Type-2 is triggering the reset of |range_for_next_append_|, and then AppendBuffersToEnd is done to the wrong range.

I'll investigate further to see why...
I'm still working on reproducing the fault in a minimized unit test.
I have found, however, that there is an increase in the fudge room at the start of the faulting append operation (and after the OnStartOfCodedFrameGroup's prior signalling), and the first buffer in the new append is at (first range's buffered end time + fudge room + 1) microseconds. So I think the issue is inconsistent |range_for_next_append_| determination (the first range) versus the lack of merging the two ranges (MergeAllAdjacentRanges), probably due to an off-by-one microsecond between the two code paths.
Blocking: 804802
The root cause is https://cs.chromium.org/chromium/src/media/filters/source_buffer_stream.cc?sq=package:chromium&dr&l=700 (and perhaps also https://cs.chromium.org/chromium/src/media/filters/source_buffer_stream.cc?sq=package:chromium&dr&l=652): PotentialNextAppendTimestamp(), in the presence of the last appended buffer in the current CFG being a SAP-Type-2 nonkeyframe prior to the start of |range_for_next_append_|, but within fudge room of some other range, can cause |range_for_next_append_| to be reset incorrectly to that other range. Subsequent attempted append of the new GOP onto that other range fails the adjacency check.

Neither 0-duration, nor increasing fudge room, are required to reproduce this issue.

Investigating fix(es).
Two different rough approaches to fixing this that I'm considering:
1) Modify PotentialNextAppendTimestamp() to have some state from OnStartOfCodedFrameGroup()?? (ugly, fragile, hacky)
2) Take the high road and have SourceBufferRangeByPts move its start time earlier if it contains a SAP-Type-2 nonkeyframe with timestamp prior to its "start time" otherwise. We're already do the associated overlap removals in such situations; this would be a cleaner fix and tie into existing adjacency / fudge room better IIUC.

I'll take a stab at #2 first, and see how well it works.
@#8 if a later GOP besides the first GOP in the range "owns" the nonkeyframe prior to the range start time, then approach #2 might regress the time-range remove operation (which is based on keyframe timestamps). I need to think further about this...
@#8-9: Approach #2's modification of the range preparation and buffer removal code would be extensive in cases like the repro. Note that the repro, if in one bytestream media segment, might not even be considered SAP-2. However, the repro could just as easily be accomplished by splicing a SAP-2 stream over previously buffered media.

In light of the relatively small portion of Chrome SAP-2 playbacks encountered and lack of consensus on the MSE v1 spec on how to interoperably process such frames in the coded frame processing algorithm, I'll try smaller fixes along the lines of approach #1 first. That approach should allow for SAP-2 overlaps of previously buffered media, with the caveat that range start times for a range beginning with a SAP-2 GOP (or containing some extreme SAP 2 GOP with a nonkeyframe PTS well before the range's first GOP's keyframe PTS) would not be adjusted backwards (like current code) to that extremely early PTS. The handling of extremely "early" nonkeyframe PTS for such SAP 2 sequences would still use the range's first GOP's keyframe PTS as the range start time (excepting existing cases where the |range_start_time_| is earlier as a result of muxed CFG signalling or range splitting.)
Labels: -M-64 M-66
This impacts only BufferingByPts mode. => M-66
Fix in CR: https://chromium-review.googlesource.com/c/chromium/src/+/879408

Cc: sande...@chromium.org
Project Member

Comment 13 by bugdroid1@chromium.org, Jan 28 2018

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

commit fdeaf6e25bb2d2596de09eca9a902ea98e70d84c
Author: Matt Wolenetz <wolenetz@chromium.org>
Date: Sun Jan 28 16:44:16 2018

MSE: Use |highest_timestamp_in_append_sequence_| in PotentialNextAppendTimestamp()

SAP-Type-2 sequences may contain a nonkeyframe so far in advance of the
range's first GOP's keyframe timestamp that it may appear to belong to
the presentation interval of some other range. If we continued to use
|last_appended_buffer_timestamp_| when re-determining
|range_for_next_append_| upon consulting PotentialNextAppendTimestamp()
during buffer removal operations that touched the current
|range_for_next_append_|, we could incorrectly append to the earlier
range.

This change uses the |highest_timestamp_in_append_sequence_| in
PotentialNextAppendTimestamp(), enabling correct determination of
|range_for_next_append_| when such SAP-Type-2 scenarios are encountered
when BufferingByPts.

Note: This change continues the direction of the BufferingByPts logic so
far: it doesn't move a range's start time earlier if that range contains
a nonkeyframe from a SAP-Type-2 GOP before the range's starting
keyframe. This greatly simplifies adapting our existing logic to
gracefully handle SAP-Type-2, with the known caveat that range start
times don't indicate any potentially buffered earlier SAP-Type-2
nonkeyframes. See also  https://crbug.com/798935#c10 

BUG= 798935 , 804802 , 795655 
TEST=Added SBST.Sap2GopWithNonKeyframeInEarlierRangesInterval, and the
CF cases for bugs  798935  and  804802  no longer repro locally with this
change.

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: Ieff9b086ec9ac73f02a6bfbfa406765b434d9728
Reviewed-on: https://chromium-review.googlesource.com/879408
Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532280}
[modify] https://crrev.com/fdeaf6e25bb2d2596de09eca9a902ea98e70d84c/media/filters/source_buffer_stream.cc
[modify] https://crrev.com/fdeaf6e25bb2d2596de09eca9a902ea98e70d84c/media/filters/source_buffer_stream_unittest.cc

Project Member

Comment 14 by ClusterFuzz, Jan 29 2018

ClusterFuzz has detected this issue as fixed in range 532267:532326.

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

Fuzzer: libFuzzer_mediasource_MP2T_AACLC_AVC_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_msan
Platform Id: linux

Crash Type: Abrt
Crash Address: 0x03e900004902
Crash State:
  media::SourceBufferRangeByPts::AppendBuffersToEnd
  media::SourceBufferStream<media::SourceBufferRangeByPts>::Append
  media::ChunkDemuxerStream::Append
  
Sanitizer: memory (MSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=518239:518261
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_msan&range=532267:532326

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

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 15 by ClusterFuzz, Jan 29 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
ClusterFuzz testcase 4710837939077120 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Sign in to add a comment