|
|
DescriptionMSE - Fix crash caused by incorrect GC of GOP with next buffer position
Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if
the GOP contains the seek position. This may be only one of many current
crashing scenarios; this is intended to fix specifically bug 586611
though the other known bugs are also listed since the bug 586611 could
lead to an empty SourceBufferRange persisting within
SourceBufferStream's |ranges_|, and fail in many ways subsequently.
R=chcunningham@chromium.org
BUG=586611, 569755, 584422
Also, internal b/26908337
TEST=SourceBufferStreamTest.GarbageCollection_DeleteFront_PreserveSeekedGOP, now unable to repro crash on linux debug or release build with YT or via b/26908337
Committed: https://crrev.com/228eb2e7e66e83b3b767625fc13dbeebf2118050
Cr-Commit-Position: refs/heads/master@{#375732}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Stick with DCHECKs for this to ease merging. +unit test #
Total comments: 16
Patch Set 3 : Fixed typo in new unit test's comment per CR #Patch Set 4 : Increased hardening + more code comments per chat w/chcunningham@ #
Messages
Total messages: 37 (14 generated)
Description was changed from ========== MSE - Fix crash caused by incorrect GC of GOP with next buffer position Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if the GOP contains the seek position. This may be only one of many current crashing scenarios; this is intended to fix specifically bug 586611 though the other known bugs are also listed since the bug 586611 could lead to an empty SourceBufferRange persisting within SourceBufferStream's |ranges_|, and fail in many ways subsequently. Also includes a few further changes of DCHECKs to CHECKs in SourceBufferRange to further harden the class against misuse. R=chcunningham@chromium.org BUG=586611,569755,584422,internal b/26908337 TEST=Now unable to repro crash on linux debug or release build with YT or via b/26908337 ========== to ========== MSE - Fix crash caused by incorrect GC of GOP with next buffer position Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if the GOP contains the seek position. This may be only one of many current crashing scenarios; this is intended to fix specifically bug 586611 though the other known bugs are also listed since the bug 586611 could lead to an empty SourceBufferRange persisting within SourceBufferStream's |ranges_|, and fail in many ways subsequently. Also includes a few further changes of DCHECKs to CHECKs in SourceBufferRange to further harden the class against misuse. R=chcunningham@chromium.org BUG=586611,569755,584422 Also, internal b/26908337 TEST=Now unable to repro crash on linux debug or release build with YT or via b/26908337 ==========
Please take a look. I'd like to land this, then bake and merge back as far as stable. cc'ed servolk@ as FYI, since his GC rewrite seems to have caused the particular regression this CL fixes.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Do you always want these CHECKs enabled? Earlier I was just talking about a temporary debugging CL. I'd land them separately so they are easy to revert otherwise.
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
On 2016/02/12 23:43:25, wolenetz wrote: > Please take a look. I'd like to land this, then bake and merge back as far as > stable. > > cc'ed servolk@ as FYI, since his GC rewrite seems to have caused the particular > regression this CL fixes. Also, I can put together a unit test which previously would only hit DCHECK. Let me know if you want that in this CL, too, please.
On 2016/02/12 23:44:48, DaleCurtis wrote: > Do you always want these CHECKs enabled? Earlier I was just talking about a > temporary debugging CL. I'd land them separately so they are easy to revert > otherwise. SGTM. I'll land the further CHECKs separately since there could be other broken paths still.
https://codereview.chromium.org/1692403002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1692403002/diff/1/media/filters/source_buffer... media/filters/source_buffer_range.cc:358: return (GetEndTimestamp() < media_time); Alternatively, s/GetEndTimestamp()/GetBufferedEndTimestamp()/ here might similarly fix the bug. I prefer, though, to explicitly check !FirstGOPContainsNextBufferPosition() prior to calling DeleteGOPFromFront(), since that more clearly protects against this problem.
Description was changed from ========== MSE - Fix crash caused by incorrect GC of GOP with next buffer position Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if the GOP contains the seek position. This may be only one of many current crashing scenarios; this is intended to fix specifically bug 586611 though the other known bugs are also listed since the bug 586611 could lead to an empty SourceBufferRange persisting within SourceBufferStream's |ranges_|, and fail in many ways subsequently. Also includes a few further changes of DCHECKs to CHECKs in SourceBufferRange to further harden the class against misuse. R=chcunningham@chromium.org BUG=586611,569755,584422 Also, internal b/26908337 TEST=Now unable to repro crash on linux debug or release build with YT or via b/26908337 ========== to ========== MSE - Fix crash caused by incorrect GC of GOP with next buffer position Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if the GOP contains the seek position. This may be only one of many current crashing scenarios; this is intended to fix specifically bug 586611 though the other known bugs are also listed since the bug 586611 could lead to an empty SourceBufferRange persisting within SourceBufferStream's |ranges_|, and fail in many ways subsequently. R=chcunningham@chromium.org BUG=586611,569755,584422 Also, internal b/26908337 TEST=Now unable to repro crash on linux debug or release build with YT or via b/26908337 ==========
Description was changed from ========== MSE - Fix crash caused by incorrect GC of GOP with next buffer position Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if the GOP contains the seek position. This may be only one of many current crashing scenarios; this is intended to fix specifically bug 586611 though the other known bugs are also listed since the bug 586611 could lead to an empty SourceBufferRange persisting within SourceBufferStream's |ranges_|, and fail in many ways subsequently. R=chcunningham@chromium.org BUG=586611,569755,584422 Also, internal b/26908337 TEST=Now unable to repro crash on linux debug or release build with YT or via b/26908337 ========== to ========== MSE - Fix crash caused by incorrect GC of GOP with next buffer position Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if the GOP contains the seek position. This may be only one of many current crashing scenarios; this is intended to fix specifically bug 586611 though the other known bugs are also listed since the bug 586611 could lead to an empty SourceBufferRange persisting within SourceBufferStream's |ranges_|, and fail in many ways subsequently. R=chcunningham@chromium.org BUG=586611,569755,584422 Also, internal b/26908337 TEST=SourceBufferStreamTest.GarbageCollection_DeleteFront_PreserveSeekedGOP, now unable to repro crash on linux debug or release build with YT or via b/26908337 ==========
Please take a look at patch set 2. Thanks! https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:859: current_range->FirstGOPContainsNextBufferPosition()) { This is the core of the fix... https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:2454: TEST_F(SourceBufferStreamTest, Note: I tried this test alone against ToT, and it fails as expected in DCHECK(!FirstGOPContainsNextBufferPosition()). https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:2460: NewSegmentAppend("1000K 1010 1020 1030 1040"); Aside: A subsequent range was necessary to ensure that the "last appended GOP" isn't re-added back into the ranges after being collected (yep, there's pre-existing logic in the GC heuristic that preserves the last appended GOP)...
https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:2460: NewSegmentAppend("1000K 1010 1020 1030 1040"); On 2016/02/13 01:35:07, wolenetz wrote: > Aside: A subsequent range was necessary to ensure that the "last appended GOP" > isn't re-added back into the ranges after being collected (yep, there's > pre-existing logic in the GC heuristic that preserves the last appended GOP)... Comment clarification: the last appended GOP is still observably preserved in this test, but having it in a distinct range lets us ALSO see that the seeked GOP is also preserved.
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/1692403002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692403002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
servolk@chromium.org changed reviewers: + servolk@chromium.org
https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:859: current_range->FirstGOPContainsNextBufferPosition()) { On 2016/02/13 01:35:07, wolenetz wrote: > This is the core of the fix... Have you been able to figure out exactly how this happens? TBH I don't understand how that's possible. I think media_time (the current playback position on the timeline) should be always <= next_buffer_position (the demuxer read position). So having this additional check here shouldn't change anything, imho. Perhaps we should also add a dcheck here that media_time <= next_buffer_position?
https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:859: current_range->FirstGOPContainsNextBufferPosition()) { On 2016/02/16 19:28:38, servolk wrote: > On 2016/02/13 01:35:07, wolenetz wrote: > > This is the core of the fix... > > Have you been able to figure out exactly how this happens? TBH I don't > understand how that's possible. I think media_time (the current playback > position on the timeline) should be always <= next_buffer_position (the demuxer > read position). So having this additional check here shouldn't change anything, > imho. Perhaps we should also add a dcheck here that media_time <= > next_buffer_position? Yes. I've confirmed at least one way this happens via log inspection and a unit test (see the new unit test in this CL). tl;dr: have some later buffered range than the current range. Seek to a time within the duration of the very last frame in the current range, but after the DTS of that frame. Garbage collect that range from the beginning. BOOM. This is also why I added the "alternatively..." CR comment in this CL's earlier patch set 1 at SBR::FirstGOPEarlierThanMediaTime().
1 significant comment, 1 nit. https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:845: if (reverse_direction) { I don't see that there is any check in the reverse_direction case that "The GOP being deleted must NOT contain the next buffer position." as required by the new comments in s_b_r.h I also don't see that media_time is at all used in this case in general. Why not? I think documentation for media_time in .h file should better explain what its implications are for this methods algorithm https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:2456: // Set memory limit to 10 buffers. typo: 15 buffers
Please take a look at patch set 3. https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:845: if (reverse_direction) { On 2016/02/16 21:27:02, chcunningham wrote: > I don't see that there is any check in the reverse_direction case that "The GOP > being deleted must NOT contain the next buffer position." as required by the new > comments in s_b_r.h > > I also don't see that media_time is at all used in this case in general. Why > not? > > I think documentation for media_time in .h file should better explain what its > implications are for this methods algorithm l.848 is that check. if last append is reached first in reverse (or forward) collection, then collection stops (l.873, l.879, l.899). If media_time is reached first, l.848 should cover that case already. IIUC, media_time is most useful to the GC heuristic when pending_seek_ is true. Otherwise, the First/LastGOPContainsNextBufferPosition() are more meaningful. The latter regressed and this change seeks to fix. Whether or not media_time in the !seek_pending_ case is meaningful to the GC heuristic is out of scope of this CL. https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream_unittest.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream_unittest.cc:2456: // Set memory limit to 10 buffers. On 2016/02/16 21:27:02, chcunningham wrote: > typo: 15 buffers Done.
On 2016/02/16 22:05:05, wolenetz wrote: > Please take a look at patch set 3. > > https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... > File media/filters/source_buffer_stream.cc (right): > > https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... > media/filters/source_buffer_stream.cc:845: if (reverse_direction) { > On 2016/02/16 21:27:02, chcunningham wrote: > > I don't see that there is any check in the reverse_direction case that "The > GOP > > being deleted must NOT contain the next buffer position." as required by the > new > > comments in s_b_r.h > > > > I also don't see that media_time is at all used in this case in general. Why > > not? > > > > I think documentation for media_time in .h file should better explain what its > > implications are for this methods algorithm > > l.848 is that check. > if last append is reached first in reverse (or forward) collection, then > collection stops (l.873, l.879, l.899). If media_time is reached first, l.848 > should cover that case already. > > IIUC, media_time is most useful to the GC heuristic when pending_seek_ is true. > Otherwise, the First/LastGOPContainsNextBufferPosition() are more meaningful. > The latter regressed and this change seeks to fix. Whether or not media_time in > the !seek_pending_ case is meaningful to the GC heuristic is out of scope of > this CL. > > https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... > File media/filters/source_buffer_stream_unittest.cc (right): > > https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... > media/filters/source_buffer_stream_unittest.cc:2456: // Set memory limit to 10 > buffers. > On 2016/02/16 21:27:02, chcunningham wrote: > > typo: 15 buffers > > Done. LGTM % comments
https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:845: if (reverse_direction) { On 2016/02/16 21:27:02, chcunningham wrote: > I don't see that there is any check in the reverse_direction case that "The GOP > being deleted must NOT contain the next buffer position." as required by the new > comments in s_b_r.h > > I also don't see that media_time is at all used in this case in general. Why > not? > > I think documentation for media_time in .h file should better explain what its > implications are for this methods algorithm The reason why the next buffer position (i.e. demuxer read position) and not the media_time is used here is because media_time is that the next buffer position is generally slighly ahead of the media_time. And if I understand this piece of code correctly (I'm not the original author), I think most of the time during normal playback the GC in the reverse direction will stop due to reaching the last appended buffer position (see the check at line 874). We should only ever hit this in cases when there's a pending seek backwards on the timeline and for those cases we want to keep a small amount of data to allow the playback to continue while the seeking is still in progress. https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:845: if (reverse_direction) { On 2016/02/16 22:05:05, wolenetz wrote: > On 2016/02/16 21:27:02, chcunningham wrote: > > I don't see that there is any check in the reverse_direction case that "The > GOP > > being deleted must NOT contain the next buffer position." as required by the > new > > comments in s_b_r.h > > > > I also don't see that media_time is at all used in this case in general. Why > > not? > > > > I think documentation for media_time in .h file should better explain what its > > implications are for this methods algorithm > > l.848 is that check. > if last append is reached first in reverse (or forward) collection, then > collection stops (l.873, l.879, l.899). If media_time is reached first, l.848 > should cover that case already. > > IIUC, media_time is most useful to the GC heuristic when pending_seek_ is true. > Otherwise, the First/LastGOPContainsNextBufferPosition() are more meaningful. > The latter regressed and this change seeks to fix. Whether or not media_time in > the !seek_pending_ case is meaningful to the GC heuristic is out of scope of > this CL. media_time is certainly useful for pending seeks, but not only for that! Using media_time instead of the demuxer read position for the GC algorithms makes things clearer for apps and keeps our GC compliant to the MSE spec. The demuxer read position is purely internal thing that is not visible to the Javascript player/apps (whereas the media_time position is known in Javascript as HTMLMediaElement::currentTime). So we had bugs in the past where apps would notice that the current playback position (media_time) had been evicted by the GC algorithm and would try to re-buffer that media range being unaware that those buffers have already been consumed by the media pipeline and are no longer necessary. By using the media_time we'll keep the data that's already been read from the demuxer slightly longer, but will avoid confusing the apps. https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:859: current_range->FirstGOPContainsNextBufferPosition()) { On 2016/02/16 20:12:47, wolenetz wrote: > On 2016/02/16 19:28:38, servolk wrote: > > On 2016/02/13 01:35:07, wolenetz wrote: > > > This is the core of the fix... > > > > Have you been able to figure out exactly how this happens? TBH I don't > > understand how that's possible. I think media_time (the current playback > > position on the timeline) should be always <= next_buffer_position (the > demuxer > > read position). So having this additional check here shouldn't change > anything, > > imho. Perhaps we should also add a dcheck here that media_time <= > > next_buffer_position? > > Yes. I've confirmed at least one way this happens via log inspection and a unit > test (see the new unit test in this CL). > tl;dr: have some later buffered range than the current range. Seek to a time > within the duration of the very last frame in the current range, but after the > DTS of that frame. Garbage collect that range from the beginning. BOOM. > This is also why I added the "alternatively..." CR comment in this CL's earlier > patch set 1 at SBR::FirstGOPEarlierThanMediaTime(). ok, but this requires seeking to a very precise position (within a duration of the video frame!) to cause any problems, so I think it's unlikely to be the root cause of the crashes being investigated, although I agree we can harden things here as well.
errr... sorry, these comments didn't post last time. Still LGTM https://codereview.chromium.org/1692403002/diff/1/media/filters/source_buffer... File media/filters/source_buffer_range.cc (right): https://codereview.chromium.org/1692403002/diff/1/media/filters/source_buffer... media/filters/source_buffer_range.cc:358: return (GetEndTimestamp() < media_time); On 2016/02/13 00:51:06, wolenetz wrote: > Alternatively, s/GetEndTimestamp()/GetBufferedEndTimestamp()/ here might > similarly fix the bug. I prefer, though, to explicitly check > !FirstGOPContainsNextBufferPosition() prior to calling DeleteGOPFromFront(), > since that more clearly protects against this problem. Acknowledged. https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:845: if (reverse_direction) { On 2016/02/16 22:05:05, wolenetz wrote: > On 2016/02/16 21:27:02, chcunningham wrote: > > I don't see that there is any check in the reverse_direction case that "The > GOP > > being deleted must NOT contain the next buffer position." as required by the > new > > comments in s_b_r.h > > > > I also don't see that media_time is at all used in this case in general. Why > > not? > > > > I think documentation for media_time in .h file should better explain what its > > implications are for this methods algorithm > > l.848 is that check. > if last append is reached first in reverse (or forward) collection, then > collection stops (l.873, l.879, l.899). If media_time is reached first, l.848 > should cover that case already. > > IIUC, media_time is most useful to the GC heuristic when pending_seek_ is true. > Otherwise, the First/LastGOPContainsNextBufferPosition() are more meaningful. > The latter regressed and this change seeks to fix. Whether or not media_time in > the !seek_pending_ case is meaningful to the GC heuristic is out of scope of > this CL. Sorry missed that check https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:859: current_range->FirstGOPContainsNextBufferPosition()) { On 2016/02/16 20:12:47, wolenetz wrote: > On 2016/02/16 19:28:38, servolk wrote: > > On 2016/02/13 01:35:07, wolenetz wrote: > > > This is the core of the fix... > > > > Have you been able to figure out exactly how this happens? TBH I don't > > understand how that's possible. I think media_time (the current playback > > position on the timeline) should be always <= next_buffer_position (the > demuxer > > read position). So having this additional check here shouldn't change > anything, > > imho. Perhaps we should also add a dcheck here that media_time <= > > next_buffer_position? > > Yes. I've confirmed at least one way this happens via log inspection and a unit > test (see the new unit test in this CL). > tl;dr: have some later buffered range than the current range. Seek to a time > within the duration of the very last frame in the current range, but after the > DTS of that frame. Garbage collect that range from the beginning. BOOM. > This is also why I added the "alternatively..." CR comment in this CL's earlier > patch set 1 at SBR::FirstGOPEarlierThanMediaTime(). After chatting f2f we settled on doing the "alternative" fix as well, as we agree FirstGOPEarlierThanMediaTime is not quite correct if it fails to considered the duration of the last frame.
servolk@, did you want to CR this or just be cc'ed? If the former, it's pending your review at the moment (it == patch set 4). https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... media/filters/source_buffer_stream.cc:859: current_range->FirstGOPContainsNextBufferPosition()) { On 2016/02/16 23:11:24, servolk wrote: > On 2016/02/16 20:12:47, wolenetz wrote: > > On 2016/02/16 19:28:38, servolk wrote: > > > On 2016/02/13 01:35:07, wolenetz wrote: > > > > This is the core of the fix... > > > > > > Have you been able to figure out exactly how this happens? TBH I don't > > > understand how that's possible. I think media_time (the current playback > > > position on the timeline) should be always <= next_buffer_position (the > > demuxer > > > read position). So having this additional check here shouldn't change > > anything, > > > imho. Perhaps we should also add a dcheck here that media_time <= > > > next_buffer_position? > > > > Yes. I've confirmed at least one way this happens via log inspection and a > unit > > test (see the new unit test in this CL). > > tl;dr: have some later buffered range than the current range. Seek to a time > > within the duration of the very last frame in the current range, but after the > > DTS of that frame. Garbage collect that range from the beginning. BOOM. > > This is also why I added the "alternatively..." CR comment in this CL's > earlier > > patch set 1 at SBR::FirstGOPEarlierThanMediaTime(). > > ok, but this requires seeking to a very precise position (within a duration of > the video frame!) to cause any problems, so I think it's unlikely to be the root > cause of the crashes being investigated, although I agree we can harden things > here as well. Acknowledged. Unfortunately, I have been hitting this and only this in my attempts to repro the crashes using the steps from the internal b/. I've changed some of SourceBufferRange's DCHECKs to CHECKs temporarily already (https://codereview.chromium.org/1673293003/), and may do more of the same if we continue to see crashes in SourceBufferRange after this fix has landed. I suspect that the repro involves or is exacerbated by apps clamping seeks to end of buffered ranges (either by interception of seeks or by doing some explicit remove starting at the seek target time prior to appending data to satisfy the seek there and invoking the implicit GC). Note that buffered end time of a range is what is visible to apps, and that is very likely beyond the last DTS of the last coded frame in the range.
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/1692403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692403002/60001
On 2016/02/16 23:37:55, wolenetz wrote: > servolk@, did you want to CR this or just be cc'ed? If the former, it's pending > your review at the moment (it == patch set 4). > > https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... > File media/filters/source_buffer_stream.cc (right): > > https://codereview.chromium.org/1692403002/diff/20001/media/filters/source_bu... > media/filters/source_buffer_stream.cc:859: > current_range->FirstGOPContainsNextBufferPosition()) { > On 2016/02/16 23:11:24, servolk wrote: > > On 2016/02/16 20:12:47, wolenetz wrote: > > > On 2016/02/16 19:28:38, servolk wrote: > > > > On 2016/02/13 01:35:07, wolenetz wrote: > > > > > This is the core of the fix... > > > > > > > > Have you been able to figure out exactly how this happens? TBH I don't > > > > understand how that's possible. I think media_time (the current playback > > > > position on the timeline) should be always <= next_buffer_position (the > > > demuxer > > > > read position). So having this additional check here shouldn't change > > > anything, > > > > imho. Perhaps we should also add a dcheck here that media_time <= > > > > next_buffer_position? > > > > > > Yes. I've confirmed at least one way this happens via log inspection and a > > unit > > > test (see the new unit test in this CL). > > > tl;dr: have some later buffered range than the current range. Seek to a time > > > within the duration of the very last frame in the current range, but after > the > > > DTS of that frame. Garbage collect that range from the beginning. BOOM. > > > This is also why I added the "alternatively..." CR comment in this CL's > > earlier > > > patch set 1 at SBR::FirstGOPEarlierThanMediaTime(). > > > > ok, but this requires seeking to a very precise position (within a duration of > > the video frame!) to cause any problems, so I think it's unlikely to be the > root > > cause of the crashes being investigated, although I agree we can harden things > > here as well. > > Acknowledged. Unfortunately, I have been hitting this and only this in my > attempts to repro the crashes using the steps from the internal b/. I've changed > some of SourceBufferRange's DCHECKs to CHECKs temporarily already > (https://codereview.chromium.org/1673293003/), and may do more of the same if we > continue to see crashes in SourceBufferRange after this fix has landed. > > I suspect that the repro involves or is exacerbated by apps clamping seeks to > end of buffered ranges (either by interception of seeks or by doing some > explicit remove starting at the seek target time prior to appending data to > satisfy the seek there and invoking the implicit GC). Note that buffered end > time of a range is what is visible to apps, and that is very likely beyond the > last DTS of the last coded frame in the range. lgtm
The CQ bit was unchecked by wolenetz@chromium.org
The CQ bit was checked by wolenetz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org Link to the patchset: https://codereview.chromium.org/1692403002/#ps60001 (title: "Increased hardening + more code comments per chat w/chcunningham@")
Thank you for the detailed review.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1692403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1692403002/60001
Message was sent while issue was closed.
Description was changed from ========== MSE - Fix crash caused by incorrect GC of GOP with next buffer position Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if the GOP contains the seek position. This may be only one of many current crashing scenarios; this is intended to fix specifically bug 586611 though the other known bugs are also listed since the bug 586611 could lead to an empty SourceBufferRange persisting within SourceBufferStream's |ranges_|, and fail in many ways subsequently. R=chcunningham@chromium.org BUG=586611,569755,584422 Also, internal b/26908337 TEST=SourceBufferStreamTest.GarbageCollection_DeleteFront_PreserveSeekedGOP, now unable to repro crash on linux debug or release build with YT or via b/26908337 ========== to ========== MSE - Fix crash caused by incorrect GC of GOP with next buffer position Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if the GOP contains the seek position. This may be only one of many current crashing scenarios; this is intended to fix specifically bug 586611 though the other known bugs are also listed since the bug 586611 could lead to an empty SourceBufferRange persisting within SourceBufferStream's |ranges_|, and fail in many ways subsequently. R=chcunningham@chromium.org BUG=586611,569755,584422 Also, internal b/26908337 TEST=SourceBufferStreamTest.GarbageCollection_DeleteFront_PreserveSeekedGOP, now unable to repro crash on linux debug or release build with YT or via b/26908337 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== MSE - Fix crash caused by incorrect GC of GOP with next buffer position Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if the GOP contains the seek position. This may be only one of many current crashing scenarios; this is intended to fix specifically bug 586611 though the other known bugs are also listed since the bug 586611 could lead to an empty SourceBufferRange persisting within SourceBufferStream's |ranges_|, and fail in many ways subsequently. R=chcunningham@chromium.org BUG=586611,569755,584422 Also, internal b/26908337 TEST=SourceBufferStreamTest.GarbageCollection_DeleteFront_PreserveSeekedGOP, now unable to repro crash on linux debug or release build with YT or via b/26908337 ========== to ========== MSE - Fix crash caused by incorrect GC of GOP with next buffer position Fixes SourceBufferStream's usage of DeleteGOPFromFront() to not occur if the GOP contains the seek position. This may be only one of many current crashing scenarios; this is intended to fix specifically bug 586611 though the other known bugs are also listed since the bug 586611 could lead to an empty SourceBufferRange persisting within SourceBufferStream's |ranges_|, and fail in many ways subsequently. R=chcunningham@chromium.org BUG=586611,569755,584422 Also, internal b/26908337 TEST=SourceBufferStreamTest.GarbageCollection_DeleteFront_PreserveSeekedGOP, now unable to repro crash on linux debug or release build with YT or via b/26908337 Committed: https://crrev.com/228eb2e7e66e83b3b767625fc13dbeebf2118050 Cr-Commit-Position: refs/heads/master@{#375732} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/228eb2e7e66e83b3b767625fc13dbeebf2118050 Cr-Commit-Position: refs/heads/master@{#375732} |