Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1898373002:
MSE: More cleanly handle stall resolution edge cases (Closed)
|
Created:
4 years, 8 months ago by wolenetz Modified:
4 years, 8 months ago Reviewers:
chcunningham CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMSE: More cleanly handle stall resolution edge cases
Refactors SourceBufferStream::FindNewSelectedRangeSeekTimestamp() to be more
readable and to handle a few edge cases when the last read position has been
removed and new buffers are appended. Includes new tests for the edge cases.
BUG=140875
Committed: https://crrev.com/345657b744f3b6d15b93f500a366d95769166b9d
Cr-Commit-Position: refs/heads/master@{#388582}
Patch Set 1 #
Total comments: 10
Messages
Total messages: 15 (7 generated)
wolenetz@chromium.org changed reviewers: + chcunningham@chromium.org
Please take a look. This is the CL promised during our offline chat and mentioned in https://codereview.chromium.org/1903503003/#msg8 Thanks! https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3551: CheckExpectedBuffers("120K"); This line fails prior to the SBS.cc change also in this CL. https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3569: CheckExpectedBuffers("120K 121"); This line fails prior to the SBS.cc change also in this CL. https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3587: CheckExpectedBuffers("150K 151"); This line fails prior to the SBS.cc change also in this CL. https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3604: CheckNoNextBuffer(); This test confirms stall behavior that used to be more common prior to this CL (see above).
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1898373002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1898373002/1
Description was changed from ========== MSE: More cleanly handle stall resolution edge cases Refactors SourceBufferStream::FindNewSelectedRangeSeekTimestamp() to be more readable and to handle a few edge cases when the last read position has been removed and new buffers are appended. Includes tests for the new edge cases. BUG=140875 ========== to ========== MSE: More cleanly handle stall resolution edge cases Refactors SourceBufferStream::FindNewSelectedRangeSeekTimestamp() to be more readable and to handle a few edge cases when the last read position has been removed and new buffers are appended. Includes new tests for the edge cases. BUG=140875 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream.cc:1613: if (range_start >= start_timestamp_plus_fudge) Do you want this comparison to be GE vs GT? IIUC GE is probably right - we typically consider a range's end timestamp to not be part of the range...and in a since start_timestamp_plus_fudge is the *end timestamp* of the fudge space. Just want to confirm my reasoning. https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3551: CheckExpectedBuffers("120K"); On 2016/04/20 00:38:24, wolenetz wrote: > This line fails prior to the SBS.cc change also in this CL. Acknowledged. https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3569: CheckExpectedBuffers("120K 121"); On 2016/04/20 00:38:24, wolenetz wrote: > This line fails prior to the SBS.cc change also in this CL. Done. https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3587: CheckExpectedBuffers("150K 151"); On 2016/04/20 00:38:24, wolenetz wrote: > This line fails prior to the SBS.cc change also in this CL. Acknowledged. https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:3604: CheckNoNextBuffer(); On 2016/04/20 00:38:24, wolenetz wrote: > This test confirms stall behavior that used to be more common prior to this CL > (see above). Acknowledged.
Thanks for review. https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1898373002/diff/1/media/filters/source_buffer... media/filters/source_buffer_stream.cc:1613: if (range_start >= start_timestamp_plus_fudge) On 2016/04/20 20:14:27, chcunningham wrote: > Do you want this comparison to be GE vs GT? IIUC GE is probably right - we > typically consider a range's end timestamp to not be part of the range...and in > a since start_timestamp_plus_fudge is the *end timestamp* of the fudge space. > Just want to confirm my reasoning. Hmm. I had based this on the previous logic in l1652. For all the "kNoDecodeTimestamp()" arg of SetSelectedRangeIfNeeded, only if there is no track_buffer_ or selected_range_, and only if there has been a buffer output since the last seek, then that last output buffer's DTS + 1 microsecond is passed as |start_timestamp| to FindNewSelectedRangeSeekTimestamp(). Search termination on GE in this case is correct IMO (there's no range starting at or before last_output_buffer_timestamp_ + fudge_room. However, we have some pre-existing path(s) with SetSelectedRangeIfNeeded() called with a non-kNoDecodeTimestamp(), and with no track_buffer_ or selected_range_, where that timestamp is used without the +1us adjustment in this method. I think those cases have greater questions such as "should the fudge room apply to the DTS of a previously next-buffer, but now removed? (Remove() case, specifically)" Out of scope of this CL; I've filed a crbug to track that investigation (https://crbug.com/605274)
The CQ bit was checked by wolenetz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1898373002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1898373002/1
Message was sent while issue was closed.
Description was changed from ========== MSE: More cleanly handle stall resolution edge cases Refactors SourceBufferStream::FindNewSelectedRangeSeekTimestamp() to be more readable and to handle a few edge cases when the last read position has been removed and new buffers are appended. Includes new tests for the edge cases. BUG=140875 ========== to ========== MSE: More cleanly handle stall resolution edge cases Refactors SourceBufferStream::FindNewSelectedRangeSeekTimestamp() to be more readable and to handle a few edge cases when the last read position has been removed and new buffers are appended. Includes new tests for the edge cases. BUG=140875 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== MSE: More cleanly handle stall resolution edge cases Refactors SourceBufferStream::FindNewSelectedRangeSeekTimestamp() to be more readable and to handle a few edge cases when the last read position has been removed and new buffers are appended. Includes new tests for the edge cases. BUG=140875 ========== to ========== MSE: More cleanly handle stall resolution edge cases Refactors SourceBufferStream::FindNewSelectedRangeSeekTimestamp() to be more readable and to handle a few edge cases when the last read position has been removed and new buffers are appended. Includes new tests for the edge cases. BUG=140875 Committed: https://crrev.com/345657b744f3b6d15b93f500a366d95769166b9d Cr-Commit-Position: refs/heads/master@{#388582} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/345657b744f3b6d15b93f500a366d95769166b9d Cr-Commit-Position: refs/heads/master@{#388582} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
