Prevent incorrect start time from being used during new range creation.

Prevent incorrect start time from being used during new range creation.

If a SourceBuffer::Remove() call removed the GOP that is currently being
appended, an incorrect start timestamp would be used when the next GOP was
appended. This ultimately led to a crash if a followup Remove() call
ended up deleting the range. This change fixes the code that determines
the start timestamp for the range created for the second GOP.

BUG=382815
TEST=SourceBufferStreamTest, Remove_WholeGOPBeingAppended

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276471

[acolwell GONE FROM CHROMIUM, 2014-06-10 23:11:14]

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:1624: last_appended_buffer_timestamp_ =
kNoTimestamp();
This prevents the interbuffer distance calculation from going crazy when the
range we are appending to get destroyed.

[wolenetz, 2014-06-11 01:53:54]

First pass through. Looking pretty good.
nit: s/an/a/ in the description.

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:675: if
(!range->BelongsToRange(potential_next_append_timestamp)) {
I'm unclear how this path could be reached if range's buffers_ is empty().
Shouldn't the earlier range->TruncateAt have returned true in this case?

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:2018: return false;
Should this return true? (see my other comment)

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream_unittest.cc:3514:
TEST_F(SourceBufferStreamTest, Remove_WholeGOPBeingAppended) {
I tried this test without the SBS.cc change applied and no repro resulted
(though the test failed). Does another remove need to be included to trigger the
issue?

[acolwell GONE FROM CHROMIUM, 2014-06-11 16:47:51]

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:675: if
(!range->BelongsToRange(potential_next_append_timestamp)) {
On 2014/06/11 01:53:54, wolenetz wrote:
> I'm unclear how this path could be reached if range's buffers_ is empty().
> Shouldn't the earlier range->TruncateAt have returned true in this case?
Yes. TruncateAt() should return true. I'm still investigating that for a
followup patch. I'll change the description to make it clear that this simply
prevents the wrong start time from being used when creating the new range.

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:2018: return false;
On 2014/06/11 01:53:54, wolenetz wrote:
> Should this return true? (see my other comment)
I believe this should return buffers_.empty(). I was investigating this and was
planning on putting this in a separate CL. This is the true reason for the
crash, but I'm still investigating whether it is still possible to get into this
situation with the fix in this patch in place. I believe the answer is "yes",
but I haven't created a test to prove it yet.

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/324223003/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream_unittest.cc:3514:
TEST_F(SourceBufferStreamTest, Remove_WholeGOPBeingAppended) {
On 2014/06/11 01:53:54, wolenetz wrote:
> I tried this test without the SBS.cc change applied and no repro resulted
> (though the test failed). Does another remove need to be included to trigger
the
> issue?
I'm just fixing the range creating problem here. This avoids the crash in the
bug, but like my other comment stated I believe there is still another
underlying issue.

[wolenetz, 2014-06-11 16:56:22]

lgtm

[acolwell GONE FROM CHROMIUM, 2014-06-11 17:02:01]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-06-11 17:05:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/324223003/20001

[commit-bot: I haz the power, 2014-06-11 19:12:18]

Change committed as 276471