Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2385423002:
MediaSource: Fix CHECK crash in append fudge room edge case. (Closed)
|
Created:
4 years, 2 months ago by chcunningham Modified:
4 years, 2 months ago Reviewers:
wolenetz CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaSource: Fix CHECK crash in append fudge room edge case.
This CHECK is hit by MSE users who make a small Remove() of their last
appended frames and then a subsequent Append() as if the previous remove
had not occurred.
The bug will also manifest if users set duration to just under the
current duration - currently equivalent to a small Remove().
Aside: duration truncation will soon be deprecated.
If the initial Remove() is small enough (less than the fudge room),
RemoveInternal() will fail to see this as causing a discontinuity and will
not reset range_for_next_append_. If the next append is beyond the
newly-shortened-range-end + fudge room, then CanAppendToEnd will CHECK.
BUG=627905
TEST=new unit tests, crash gone from (nsfw) http://ck101.com/thread-3559838-1-1.html?ref=banner
Committed: https://crrev.com/1900dd4377b064eefd4e9a3bcf2e115c638ebb5e
Cr-Commit-Position: refs/heads/master@{#423735}
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 1
Patch Set 3 : Fix new CFG logic #
Total comments: 16
Patch Set 4 : Rebase and feedback #
Messages
Total messages: 17 (8 generated)
Description was changed from ========== MediaSource: Fix CHECK crash in append fudge room edge case. This CHECK is hit by MSE users who make a small Remove() of their last appended frames and then a subsequent Append() as if the previous remove had not occurred. The bug will also manifest if users set duration to just under the current duration - currently equivalent to a small Remove(). If the initial Remove() is small enough (less than the fuge room), RemoveInternal() will fail see this as causing a discontinuity and will not reset range_for_next_append_. If the next append is beyond the newly-shortened-range-end + fudge room, then CanAppendToEnd will check. BUG=627905 TEST=new unit tests, crash gone from (nsfw) http://ck101.com/thread-3559838-1-1.html?ref=banner ========== to ========== MediaSource: Fix CHECK crash in append fudge room edge case. This CHECK is hit by MSE users who make a small Remove() of their last appended frames and then a subsequent Append() as if the previous remove had not occurred. The bug will also manifest if users set duration to just under the current duration - currently equivalent to a small Remove(). If the initial Remove() is small enough (less than the fuge room), RemoveInternal() will fail see this as causing a discontinuity and will not reset range_for_next_append_. If the next append is beyond the newly-shortened-range-end + fudge room, then CanAppendToEnd will CHECK. BUG=627905 TEST=new unit tests, crash gone from (nsfw) http://ck101.com/thread-3559838-1-1.html?ref=banner ==========
chcunningham@chromium.org changed reviewers: + wolenetz@chromium.org
Hey Matt, PTAL
On 2016/10/04 15:03:37, chcunningham wrote: > Hey Matt, PTAL Friendly ping.
Yeah, on my radar. Tomorrow morning if not earlier. On Oct 5, 2016 12:22 PM, <chcunningham@chromium.org> wrote: > On 2016/10/04 15:03:37, chcunningham wrote: > > Hey Matt, PTAL > > Friendly ping. > > https://codereview.chromium.org/2385423002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM % nits: CL description: * Note in the "if users set duration" paragraph that this scenario exercises deprecated, soon-to-be-removed behavior. * s/fuge/fudge/ * s/will fail see/will fail to see/ And: https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1166: if (ranges_.empty()) nit: why remove the DVLOG? Maybe retain it but with lower verbosity if debug spam was concern. https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1173: if (start < end) { aside: this truncate-on-duration-reduction behavior is being deprecated (at blink-side, and already disallowed in spec) https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1178: // Truncation removed current seek position. Clear selected range. nit: let's be careful not to confuse seek position (could be misinterpreted as pending seek to unbuffered position) with current position in the stream. tl;dr: s/seek// FWIW, I think we still have some separate edge cases (not to fix in this CL) around removals, currentTime interpolation, and duration reduction, where a seek to truncated duration might not be done right. However, those edge cases will be simplified soon once we fully deprecate the old "allow automatic range removals and partial frame rendering on duration reduction". Regardless, we shouldn't need to worry about end_of_stream_ being true here (and perhaps having to emit EOS buffers directly from this code path), since the API should guarantee that MediaSource object is in "open" state in this code path. So, please also add DCHECK(!end_of_stream_) to the beginning of this method. https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream.h (left): https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.h:82: // TODO(vrk): Implement garbage collection. (crbug.com/125070) :) https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:3323: TEST_F(SourceBufferStreamTest, SetExplicitDuration_EdgeCase2) { FWIW, EdgeCase2 exercises deprecated path currently. Once "duration reduction can truncate buffered frames" is disallowed (as is already disallowed in spec), this test will need to just be dropped (but keep RemoveWithinFudgeRoom, below). https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4779: LOG(ERROR) << "-------------SECOND APPEND------------"; nit: needed? https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4802: StartCodedFrameGroup_InExisting_RemoveGOP_ThenAppend_2n) { nit: s/2n/2/ ? https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4805: LOG(ERROR) << "---REMOVING FROM 30 - 60 ---"; nit ditto https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4809: LOG(ERROR) << "---SECOND APPEND---"; nit ditto
https://codereview.chromium.org/2385423002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2385423002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:210: // We may not want to not do this. Keeping last append around is important fwiw: I think you answered your own question by removing this comment from more recent patch set, but for posterity, if we foresee discontinuity here, we must ResetLastAppendState(). Otherwise, remove from last appended to currently being appended buffer (or CFG start time, if earlier) could remove some legit GOP(s).
Description was changed from ========== MediaSource: Fix CHECK crash in append fudge room edge case. This CHECK is hit by MSE users who make a small Remove() of their last appended frames and then a subsequent Append() as if the previous remove had not occurred. The bug will also manifest if users set duration to just under the current duration - currently equivalent to a small Remove(). If the initial Remove() is small enough (less than the fuge room), RemoveInternal() will fail see this as causing a discontinuity and will not reset range_for_next_append_. If the next append is beyond the newly-shortened-range-end + fudge room, then CanAppendToEnd will CHECK. BUG=627905 TEST=new unit tests, crash gone from (nsfw) http://ck101.com/thread-3559838-1-1.html?ref=banner ========== to ========== MediaSource: Fix CHECK crash in append fudge room edge case. This CHECK is hit by MSE users who make a small Remove() of their last appended frames and then a subsequent Append() as if the previous remove had not occurred. The bug will also manifest if users set duration to just under the current duration - currently equivalent to a small Remove(). If the initial Remove() is small enough (less than the fudge room), RemoveInternal() will fail to see this as causing a discontinuity and will not reset range_for_next_append_. If the next append is beyond the newly-shortened-range-end + fudge room, then CanAppendToEnd will CHECK. BUG=627905 TEST=new unit tests, crash gone from (nsfw) http://ck101.com/thread-3559838-1-1.html?ref=banner ==========
Description was changed from ========== MediaSource: Fix CHECK crash in append fudge room edge case. This CHECK is hit by MSE users who make a small Remove() of their last appended frames and then a subsequent Append() as if the previous remove had not occurred. The bug will also manifest if users set duration to just under the current duration - currently equivalent to a small Remove(). If the initial Remove() is small enough (less than the fudge room), RemoveInternal() will fail to see this as causing a discontinuity and will not reset range_for_next_append_. If the next append is beyond the newly-shortened-range-end + fudge room, then CanAppendToEnd will CHECK. BUG=627905 TEST=new unit tests, crash gone from (nsfw) http://ck101.com/thread-3559838-1-1.html?ref=banner ========== to ========== MediaSource: Fix CHECK crash in append fudge room edge case. This CHECK is hit by MSE users who make a small Remove() of their last appended frames and then a subsequent Append() as if the previous remove had not occurred. The bug will also manifest if users set duration to just under the current duration - currently equivalent to a small Remove(). Aside: duration truncation will soon be deprecated. If the initial Remove() is small enough (less than the fudge room), RemoveInternal() will fail to see this as causing a discontinuity and will not reset range_for_next_append_. If the next append is beyond the newly-shortened-range-end + fudge room, then CanAppendToEnd will CHECK. BUG=627905 TEST=new unit tests, crash gone from (nsfw) http://ck101.com/thread-3559838-1-1.html?ref=banner ==========
Thanks Matt. Fixed CL description. https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1166: if (ranges_.empty()) On 2016/10/05 22:41:09, wolenetz wrote: > nit: why remove the DVLOG? Maybe retain it but with lower verbosity if debug > spam was concern. Added back. https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1173: if (start < end) { On 2016/10/05 22:41:09, wolenetz wrote: > aside: this truncate-on-duration-reduction behavior is being deprecated (at > blink-side, and already disallowed in spec) Acknowledged. https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream.cc:1178: // Truncation removed current seek position. Clear selected range. On 2016/10/05 22:41:09, wolenetz wrote: > nit: let's be careful not to confuse seek position (could be misinterpreted as > pending seek to unbuffered position) with current position in the stream. tl;dr: > s/seek// > > FWIW, I think we still have some separate edge cases (not to fix in this CL) > around removals, currentTime interpolation, and duration reduction, where a seek > to truncated duration might not be done right. However, those edge cases will be > simplified soon once we fully deprecate the old "allow automatic range removals > and partial frame rendering on duration reduction". > > Regardless, we shouldn't need to worry about end_of_stream_ being true here (and > perhaps having to emit EOS buffers directly from this code path), since the API > should guarantee that MediaSource object is in "open" state in this code path. > > So, please also add DCHECK(!end_of_stream_) to the beginning of this method. Done. https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4779: LOG(ERROR) << "-------------SECOND APPEND------------"; On 2016/10/05 22:41:09, wolenetz wrote: > nit: needed? Woops. Removed. https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4802: StartCodedFrameGroup_InExisting_RemoveGOP_ThenAppend_2n) { On 2016/10/05 22:41:09, wolenetz wrote: > nit: s/2n/2/ ? Done. https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4805: LOG(ERROR) << "---REMOVING FROM 30 - 60 ---"; On 2016/10/05 22:41:09, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/2385423002/diff/40001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:4809: LOG(ERROR) << "---SECOND APPEND---"; On 2016/10/05 22:41:09, wolenetz wrote: > nit ditto Done.
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/2385423002/#ps60001 (title: "Rebase and feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MediaSource: Fix CHECK crash in append fudge room edge case. This CHECK is hit by MSE users who make a small Remove() of their last appended frames and then a subsequent Append() as if the previous remove had not occurred. The bug will also manifest if users set duration to just under the current duration - currently equivalent to a small Remove(). Aside: duration truncation will soon be deprecated. If the initial Remove() is small enough (less than the fudge room), RemoveInternal() will fail to see this as causing a discontinuity and will not reset range_for_next_append_. If the next append is beyond the newly-shortened-range-end + fudge room, then CanAppendToEnd will CHECK. BUG=627905 TEST=new unit tests, crash gone from (nsfw) http://ck101.com/thread-3559838-1-1.html?ref=banner ========== to ========== MediaSource: Fix CHECK crash in append fudge room edge case. This CHECK is hit by MSE users who make a small Remove() of their last appended frames and then a subsequent Append() as if the previous remove had not occurred. The bug will also manifest if users set duration to just under the current duration - currently equivalent to a small Remove(). Aside: duration truncation will soon be deprecated. If the initial Remove() is small enough (less than the fudge room), RemoveInternal() will fail to see this as causing a discontinuity and will not reset range_for_next_append_. If the next append is beyond the newly-shortened-range-end + fudge room, then CanAppendToEnd will CHECK. BUG=627905 TEST=new unit tests, crash gone from (nsfw) http://ck101.com/thread-3559838-1-1.html?ref=banner ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== MediaSource: Fix CHECK crash in append fudge room edge case. This CHECK is hit by MSE users who make a small Remove() of their last appended frames and then a subsequent Append() as if the previous remove had not occurred. The bug will also manifest if users set duration to just under the current duration - currently equivalent to a small Remove(). Aside: duration truncation will soon be deprecated. If the initial Remove() is small enough (less than the fudge room), RemoveInternal() will fail to see this as causing a discontinuity and will not reset range_for_next_append_. If the next append is beyond the newly-shortened-range-end + fudge room, then CanAppendToEnd will CHECK. BUG=627905 TEST=new unit tests, crash gone from (nsfw) http://ck101.com/thread-3559838-1-1.html?ref=banner ========== to ========== MediaSource: Fix CHECK crash in append fudge room edge case. This CHECK is hit by MSE users who make a small Remove() of their last appended frames and then a subsequent Append() as if the previous remove had not occurred. The bug will also manifest if users set duration to just under the current duration - currently equivalent to a small Remove(). Aside: duration truncation will soon be deprecated. If the initial Remove() is small enough (less than the fudge room), RemoveInternal() will fail to see this as causing a discontinuity and will not reset range_for_next_append_. If the next append is beyond the newly-shortened-range-end + fudge room, then CanAppendToEnd will CHECK. BUG=627905 TEST=new unit tests, crash gone from (nsfw) http://ck101.com/thread-3559838-1-1.html?ref=banner Committed: https://crrev.com/1900dd4377b064eefd4e9a3bcf2e115c638ebb5e Cr-Commit-Position: refs/heads/master@{#423735} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1900dd4377b064eefd4e9a3bcf2e115c638ebb5e Cr-Commit-Position: refs/heads/master@{#423735} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
