Unexpected stall can result because internal result of MergeWithAdjacentRangeIfNecessary needs to consider range_start_time |
||||
Issue description
Some types of overlap appends can result in distinct internal buffered ranges which otherwise should be continuous (and merged). When reported to Blink, the TimeRanges construction coalesces these internally distinct ranges, incorrectly reporting availability of continuous playback across the resulting coalesced range.
Initial inspection of SourceBufferStream::MergeWithAdjacentRangeIfNecessary and SBRByPts(and SBRByDts) indicates that SBRBy{Pts,Dts}::CanAppendRangeToEnd(r) needs to consider the possibility that |r| may have a start time earlier than the first buffer's time in |r| (rather than hardcoding the kNoTimestamp as that alternate start time).
I expect to land a unit test soon (via https://chromium-review.googlesource.com/c/chromium/src/+/777778 or derivative) which refers to this bug to more clearly demonstrate the incorrect stall in a range which otherwise would be continuous; [a,b)[b,c) should really be just [a,c) in actual demux continuity.
,
Dec 6 2017
I have a repro using unit tests, and understand a way to fix it. Work is in progress on the tests and the fix at https://chromium-review.googlesource.com/c/chromium/src/+/812424
,
Dec 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2249115a998973ba1ee60ca776d108bf4f33e0f2 commit 2249115a998973ba1ee60ca776d108bf4f33e0f2 Author: Matt Wolenetz <wolenetz@chromium.org> Date: Fri Dec 15 18:50:17 2017 MSE: Increase const usage in SourceBufferRange classes Increases usage of const where appropriate in SourceBufferRange{,ByDts,ByPts} classes. I found and am fixing this while working on patches to fix bug 791095 . BUG= 791095 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: If8d70f80e36462fce62451a3a189485bb9774eef Reviewed-on: https://chromium-review.googlesource.com/825965 Reviewed-by: Dan Sanders <sandersd@chromium.org> Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> Cr-Commit-Position: refs/heads/master@{#524422} [modify] https://crrev.com/2249115a998973ba1ee60ca776d108bf4f33e0f2/media/filters/source_buffer_range.cc [modify] https://crrev.com/2249115a998973ba1ee60ca776d108bf4f33e0f2/media/filters/source_buffer_range.h [modify] https://crrev.com/2249115a998973ba1ee60ca776d108bf4f33e0f2/media/filters/source_buffer_range_by_dts.cc [modify] https://crrev.com/2249115a998973ba1ee60ca776d108bf4f33e0f2/media/filters/source_buffer_range_by_dts.h [modify] https://crrev.com/2249115a998973ba1ee60ca776d108bf4f33e0f2/media/filters/source_buffer_range_by_pts.cc [modify] https://crrev.com/2249115a998973ba1ee60ca776d108bf4f33e0f2/media/filters/source_buffer_range_by_pts.h
,
Jan 9 2018
,
Jan 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3d4779ec6766d8e4b9ca81f107d63d458e79050b commit 3d4779ec6766d8e4b9ca81f107d63d458e79050b Author: Matt Wolenetz <wolenetz@chromium.org> Date: Thu Jan 11 20:00:24 2018 MSE: Preserve front-adjacency of middle/end-overlapping appends Adjusts an overlapping coded frame group's "start time" backwards to make it continuous with the highest buffered frame prior to the overlap in the overlapped range. This helps prevent discontinuity from being introduced by the SBS::RemoveInternal processing during the next ::Append call. Note that discontinuities can still be introduced by app or GC eviction removing data from the overlapped range prior to that ::Append call. Follow-up CL(s) will fix the other part of bug 791095 where the end of an overlapping append might become erroneously discontinuous. BUG= 791095 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: I449f9e063a8e4bde70f10a943a5b664f1b044b59 Reviewed-on: https://chromium-review.googlesource.com/860971 Reviewed-by: Dan Sanders <sandersd@chromium.org> Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> Cr-Commit-Position: refs/heads/master@{#528718} [modify] https://crrev.com/3d4779ec6766d8e4b9ca81f107d63d458e79050b/media/filters/frame_processor_unittest.cc [modify] https://crrev.com/3d4779ec6766d8e4b9ca81f107d63d458e79050b/media/filters/source_buffer_range_by_dts.cc [modify] https://crrev.com/3d4779ec6766d8e4b9ca81f107d63d458e79050b/media/filters/source_buffer_range_by_dts.h [modify] https://crrev.com/3d4779ec6766d8e4b9ca81f107d63d458e79050b/media/filters/source_buffer_range_by_pts.cc [modify] https://crrev.com/3d4779ec6766d8e4b9ca81f107d63d458e79050b/media/filters/source_buffer_range_by_pts.h [modify] https://crrev.com/3d4779ec6766d8e4b9ca81f107d63d458e79050b/media/filters/source_buffer_stream.cc [modify] https://crrev.com/3d4779ec6766d8e4b9ca81f107d63d458e79050b/media/filters/source_buffer_stream.h [modify] https://crrev.com/3d4779ec6766d8e4b9ca81f107d63d458e79050b/media/filters/source_buffer_stream_unittest.cc
,
Jan 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4a1c11475b4c8457a0f7dcb2cc3cc3e2a1fdbf2a commit 4a1c11475b4c8457a0f7dcb2cc3cc3e2a1fdbf2a Author: Matt Wolenetz <wolenetz@chromium.org> Date: Fri Jan 12 21:42:56 2018 MSE: Preserve end-adjacency of overlapping appends When determining if the current range can be merged with the next range, consider that the next range might have an earlier start time than its first buffer's time that allows for adjacency within fudge room and merging the two ranges. Also, when splitting off the end of a range as part of buffer removal from the range, use the later of the split time and the range's start time for the range start time of the range that is split off. Because range start times are used in reporting buffered ranges, this change also fixes (except from some ByPts SAP-Type-2 cases) an issue where there could be distinct buffered ranges that are reported (due to coalescing of overlapping or immediately adjacent times by TimeRanges) as continuous, resulting in unexpected playback stalls at the internal range boundaries. Includes further test comments noting further work needed to fix bug 791095 for some SAP-Type-2 cases. BUG= 791095 TEST=New SourceBufferStreamTests, multiple updated tests 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: Id2bac409ebdea5d9d9dbc3d40b80b81183666d8c Reviewed-on: https://chromium-review.googlesource.com/812424 Reviewed-by: Dan Sanders <sandersd@chromium.org> Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> Cr-Commit-Position: refs/heads/master@{#529076} [modify] https://crrev.com/4a1c11475b4c8457a0f7dcb2cc3cc3e2a1fdbf2a/media/filters/chunk_demuxer_unittest.cc [modify] https://crrev.com/4a1c11475b4c8457a0f7dcb2cc3cc3e2a1fdbf2a/media/filters/frame_processor_unittest.cc [modify] https://crrev.com/4a1c11475b4c8457a0f7dcb2cc3cc3e2a1fdbf2a/media/filters/source_buffer_range_by_dts.cc [modify] https://crrev.com/4a1c11475b4c8457a0f7dcb2cc3cc3e2a1fdbf2a/media/filters/source_buffer_range_by_dts.h [modify] https://crrev.com/4a1c11475b4c8457a0f7dcb2cc3cc3e2a1fdbf2a/media/filters/source_buffer_range_by_pts.cc [modify] https://crrev.com/4a1c11475b4c8457a0f7dcb2cc3cc3e2a1fdbf2a/media/filters/source_buffer_range_by_pts.h [modify] https://crrev.com/4a1c11475b4c8457a0f7dcb2cc3cc3e2a1fdbf2a/media/filters/source_buffer_stream_unittest.cc
,
Jan 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b160add3de8981a9de325096b78e3d38a9e3bc4b commit b160add3de8981a9de325096b78e3d38a9e3bc4b Author: Matt Wolenetz <wolenetz@chromium.org> Date: Sat Jan 13 00:47:09 2018 MSE: Coalesce adjacent ranges in a ByPts coded frame group with SAP-2 When a continuation of a coded frame group is a SAP-Type-2 GOP, and when buffering ByPts, SourceBufferStream's logic may result in a discontinuous (although directly abutting) set of 2 ranges: the range from the previous append in the group, and the range beginning with a new GOP. This change checks for this condition and merges those two ranges, if necessary. BUG= 791095 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: I938abd68ed356103236a4828bb0ab022a2d642b6 Reviewed-on: https://chromium-review.googlesource.com/865849 Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Cr-Commit-Position: refs/heads/master@{#529120} [modify] https://crrev.com/b160add3de8981a9de325096b78e3d38a9e3bc4b/media/filters/chunk_demuxer_unittest.cc [modify] https://crrev.com/b160add3de8981a9de325096b78e3d38a9e3bc4b/media/filters/frame_processor_unittest.cc [modify] https://crrev.com/b160add3de8981a9de325096b78e3d38a9e3bc4b/media/filters/source_buffer_stream.cc
,
Jan 13 2018
Once https://chromium-review.googlesource.com/c/chromium/src/+/865826 lands, this bug will be fixed. If clusterfuzz finds issues via the strengthened debug IsRangeListSorted() checks in that CL, those will be new bug(s).
,
Jan 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/03c6fb32c54b64bb9424b7e2dbdf8753e409e7ce commit 03c6fb32c54b64bb9424b7e2dbdf8753e409e7ce Author: Matt Wolenetz <wolenetz@chromium.org> Date: Sat Jan 13 01:29:17 2018 MSE: Improve sorted range checking in debug builds Now that known range adjacency issues have been fixed (previous fixes in bug 791095 ), this change increases the strength of IsRangeListSorted() to ensure that the ranges' presentation intervals (in DTS if buffering ByDTS, in PTS if buffering ByPTS) are sorted and disjoint. This strengthened logic caught the known errors fixed by prereq CL https://chromium-review.googlesource.com/c/chromium/src/+/865849. Also, the check for sorted ranges is moved from ::RemoveInternal to ::Remove, because it is possible that interim |ranges_| state may have abutting, non-overlapping, ranges during the scope of ::Append processing. BUG= 791095 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: Iccd018f88ba773a8e8ec21b97f975c2f872024b9 Reviewed-on: https://chromium-review.googlesource.com/865826 Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Cr-Commit-Position: refs/heads/master@{#529134} [modify] https://crrev.com/03c6fb32c54b64bb9424b7e2dbdf8753e409e7ce/media/filters/source_buffer_stream.cc
,
Jan 13 2018
,
Jan 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cfe1e5f7d7aafa697a51d137b955fd0c40667c1d commit cfe1e5f7d7aafa697a51d137b955fd0c40667c1d Author: Matt Wolenetz <wolenetz@chromium.org> Date: Wed Jan 17 05:32:18 2018 MSE: Prevent disjoint (but adjacent) ranges in a ByDts case Like many cases fixed by bug 791095 , this change fixes another case where a set of buffered ranges [a,b) [b,c) might result from a sequence of appends to the MSE API. This fix expands an additional MergeWithAdjacentRangeIfNecessary() call in SBS::Append() to occur not just when buffering by PTS, but also when buffering by DTS. New unit tests are included that demonstrated the fault prior to the rest of this change, in both sequence and segments mode when buffering by DTS. With this change, the clusterfuzz case in bug 801796 no longer repros. BUG= 801796 , 791095 TEST=SourceBufferStreamTest.PreciselyOverlapLastAudioFrameAppended_* 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: Icf103726b5220d021519c1fce086a06f87c48161 Reviewed-on: https://chromium-review.googlesource.com/867972 Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> Reviewed-by: Chrome Cunningham <chcunningham@chromium.org> Cr-Commit-Position: refs/heads/master@{#529628} [modify] https://crrev.com/cfe1e5f7d7aafa697a51d137b955fd0c40667c1d/media/filters/source_buffer_stream.cc [modify] https://crrev.com/cfe1e5f7d7aafa697a51d137b955fd0c40667c1d/media/filters/source_buffer_stream_unittest.cc [modify] https://crrev.com/cfe1e5f7d7aafa697a51d137b955fd0c40667c1d/media/test/pipeline_integration_fuzzertest.cc
,
Jan 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ef5f357c1f1375f3167bc1d9ecc9b5188ad421f6 commit ef5f357c1f1375f3167bc1d9ecc9b5188ad421f6 Author: Matt Wolenetz <wolenetz@chromium.org> Date: Mon Jan 22 21:13:14 2018 MSE: Fix a couple fudge room usage faults When fudge room increases, MergeAllAdjacentRanges should merge all ranges that are adjacent based on the new fudge room. Previous code would not try to merge sets of more than 2 adjacent ranges, resulting in disjoint ranges which should have been coalesced. This change updates MergeAllAdjacentRanges to try to merge the current range with the next one repeatedly if the previous merge was successful. This change also stops using the approximate buffer duration (based on the maximum duration or continuous interbuffer DTS distance seen so far) as the reported range end time for ranges ending with a zero duration buffer. In combination with the previously broken MergeAllAdjacentRanges logic, approximated range end times that grow dynamically as a side effect of fudge room growth led to previously disjoint ranges overlapping each other (and failing the strengthened IsRangeListSorted() debug build verifications added as part of fixing bug 791095 .) This change uses a constant 1 microsecond for the duration of a buffer that is reported as zero duration by the stream parsing and frame processor when calculating the buffered end time of a range. Since public base::TimeDelta::FromInternalValue usage is deprecated, this change also switches usage to FromMicroseconds in SBS and SBR. BUG= 803018 , 803293 , 791095 TEST=SBS.*ZeroDurationBuffersThenIncreasingFudgeRoom, and removal of a FrameProcessorTest that verified the obsolete fudge-room-based buffered range reporting. The clusterfuzz cases in bugs 803018 and 803293 also 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: I8b4adfeb5064c15e57211d2f07132e7723f7f66a Reviewed-on: https://chromium-review.googlesource.com/871517 Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> Reviewed-by: Chrome Cunningham <chcunningham@chromium.org> Cr-Commit-Position: refs/heads/master@{#530992} [modify] https://crrev.com/ef5f357c1f1375f3167bc1d9ecc9b5188ad421f6/media/filters/frame_processor_unittest.cc [modify] https://crrev.com/ef5f357c1f1375f3167bc1d9ecc9b5188ad421f6/media/filters/source_buffer_range_by_dts.cc [modify] https://crrev.com/ef5f357c1f1375f3167bc1d9ecc9b5188ad421f6/media/filters/source_buffer_range_by_pts.cc [modify] https://crrev.com/ef5f357c1f1375f3167bc1d9ecc9b5188ad421f6/media/filters/source_buffer_stream.cc [modify] https://crrev.com/ef5f357c1f1375f3167bc1d9ecc9b5188ad421f6/media/filters/source_buffer_stream.h [modify] https://crrev.com/ef5f357c1f1375f3167bc1d9ecc9b5188ad421f6/media/filters/source_buffer_stream_unittest.cc |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, Dec 6 2017