Fix crashes caused when SourceBuffer.remove() is called starting at the presentation duration.

Fix crashes caused when SourceBuffer.remove() is called starting at the presentation duration.

BUG=381302
TEST=ChunkDemuxerTest.Remove_StartAtDuration

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

[wolenetz, 2014-06-05 21:38:04]

https://codereview.chromium.org/312403002/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/312403002/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:598: remove_end_timestamp =
keyframe_timestamp;
nit: ILTM like adding DCHECK(keyframe_timestamp > start) here would also be a
good idea, since if not kNoTimestamp(), keyframe_timestamp must be > end, which
must be > start.
Also, may keyframe_timestamp legally exceed |duration| here?

https://codereview.chromium.org/312403002/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:603: if (start == remove_end_timestamp)
remove_end_timestamp is either duration (in which case, if start is also
duration, we should fail DCHECK above, already); or |end| which is less than
|duration| (in which case, if start is == end, we should fail the same DCHECK
above, already); or a keyframe timestamp beyond the original end and DCHECK
passes and start < remove_end_timestamp.

So, does this change focus only on fixing release mode crashes where either:
1. start == duration (caller can protect against this)
2. start == end (caller can protect against this)
3. start > end (and a keyframe exists beyond end at 'start' time): caller can
protect against this, too.

I think I'm missing the reason why caller isn't doing the work here.

[acolwell GONE FROM CHROMIUM, 2014-06-05 22:56:26]

https://codereview.chromium.org/312403002/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/312403002/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:598: remove_end_timestamp =
keyframe_timestamp;
On 2014/06/05 21:38:03, wolenetz wrote:
> nit: ILTM like adding DCHECK(keyframe_timestamp > start) here would also be a
> good idea, since if not kNoTimestamp(), keyframe_timestamp must be > end,
which
> must be > start.
I don't think we need this here since the DCHECK in RemoveInternal() should
catch that case and it would mean that FindKeyFrameAfterTimestamp() would not be
delivering on its intended behavior.

> Also, may keyframe_timestamp legally exceed |duration| here?
No.

https://codereview.chromium.org/312403002/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:603: if (start == remove_end_timestamp)
On 2014/06/05 21:38:03, wolenetz wrote:
> remove_end_timestamp is either duration (in which case, if start is also
> duration, we should fail DCHECK above, already); or |end| which is less than
> |duration| (in which case, if start is == end, we should fail the same DCHECK
> above, already); or a keyframe timestamp beyond the original end and DCHECK
> passes and start < remove_end_timestamp.
> 
> So, does this change focus only on fixing release mode crashes where either:
> 1. start == duration (caller can protect against this)
> 2. start == end (caller can protect against this)
> 3. start > end (and a keyframe exists beyond end at 'start' time): caller can
> protect against this, too.
> 
> I think I'm missing the reason why caller isn't doing the work here.

I moved this to ChunkDemuxer::Remove(). I can't go any higher than that right
now because that is where the authoritative duration information lives.

[wolenetz, 2014-06-05 23:27:32]

looking pretty good. Thanks for moving this up to ChunkDemuxer.

https://codereview.chromium.org/312403002/diff/20001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/312403002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3283:
base::TimeDelta::FromSecondsD(demuxer_->GetDuration()),
demuxer_->GetDuration() will be much larger than 160ms. It's actually
kDefaultDuration() from InitDemuxer(), above. So I don't think this tests the
exact desired scenario of removal beginning at duration and buffered ends at
duration. Perhaps do a demuxer_->SetDuration(140) after InitDemuxer, add an
expectation of a call to SetDuration(160) prior to appending the cluster, and
confirm demuxer->GetDuration() is 160 prior to Remove()?

[acolwell GONE FROM CHROMIUM, 2014-06-06 00:40:07]

https://codereview.chromium.org/312403002/diff/20001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/312403002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3283:
base::TimeDelta::FromSecondsD(demuxer_->GetDuration()),
On 2014/06/05 23:27:32, wolenetz wrote:
> demuxer_->GetDuration() will be much larger than 160ms. It's actually
> kDefaultDuration() from InitDemuxer(), above. So I don't think this tests the
> exact desired scenario of removal beginning at duration and buffered ends at
> duration. Perhaps do a demuxer_->SetDuration(140) after InitDemuxer, add an
> expectation of a call to SetDuration(160) prior to appending the cluster, and
> confirm demuxer->GetDuration() is 160 prior to Remove()?

Done.

[wolenetz, 2014-06-06 00:52:01]

lgtm % removal of RemoveBeforeInitSegment test. Also, possibility of spec
bug/clarification. Details:

PS2 failed on try bots due to: ChunkDemuxerTest.RemoveBeforeInitSegment.

IIUC, spec disallows remove() prior to init segment because duration is still
NaN and comparison of start < NaN implicitly fails.

However, this seems a bit vague to me: should spec call out that
SourceBuffer.remove() is *explicitly* disallowed if duration is NaN, as
currently implemented by SourceBuffer::remove():
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
?

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

Thanks for the review.

On 2014/06/06 00:52:01, wolenetz wrote:
> lgtm % removal of RemoveBeforeInitSegment test. Also, possibility of spec
> bug/clarification. Details:
> 
> PS2 failed on try bots due to: ChunkDemuxerTest.RemoveBeforeInitSegment.
Removed.

> 
> IIUC, spec disallows remove() prior to init segment because duration is still
> NaN and comparison of start < NaN implicitly fails.
> 
> However, this seems a bit vague to me: should spec call out that
> SourceBuffer.remove() is *explicitly* disallowed if duration is NaN, as
> currently implemented by SourceBuffer::remove():
Spec bug filed.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26000

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

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-06-06 02:02:10]

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

[commit-bot: I haz the power, 2014-06-06 11:23:10]

Change committed as 275405