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

media/filters/source_buffer_stream.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/source_buffer_stream.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/bind.h"
@@ skipping 571 matching lines @@
 
 void SourceBufferStream::Remove(base::TimeDelta start, base::TimeDelta end,
                                 base::TimeDelta duration) {
   DVLOG(1) << __FUNCTION__ << "(" << start.InSecondsF()
            << ", " << end.InSecondsF()
            << ", " << duration.InSecondsF() << ")";
   DCHECK(start >= base::TimeDelta()) << start.InSecondsF();
   DCHECK(start < end) << "start " << start.InSecondsF()
                       << " end " << end.InSecondsF();
   DCHECK(duration != kNoTimestamp());
+  DCHECK(start <= duration) << "start " << start.InSecondsF()
+                            << " duration " << duration.InSecondsF();
 
   base::TimeDelta remove_end_timestamp = duration;
   base::TimeDelta keyframe_timestamp = FindKeyframeAfterTimestamp(end);
   if (keyframe_timestamp != kNoTimestamp()) {
     remove_end_timestamp = keyframe_timestamp;

[wolenetz, 2014/06/05 21:38:03]

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?

[acolwell GONE FROM CHROMIUM, 2014/06/05 22:56:25]

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.
No.

   } else if (end < remove_end_timestamp) {
     remove_end_timestamp = end;
   }
 
+  if (start == remove_end_timestamp)

[wolenetz, 2014/06/05 21:38:03]

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:25]

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.

+    return;
+
   BufferQueue deleted_buffers;
   RemoveInternal(start, remove_end_timestamp, false, &deleted_buffers);
 
   if (!deleted_buffers.empty())
     SetSelectedRangeIfNeeded(deleted_buffers.front()->GetDecodeTimestamp());
 }
 
 void SourceBufferStream::RemoveInternal(
     base::TimeDelta start, base::TimeDelta end, bool is_exclusive,
     BufferQueue* deleted_buffers) {
@@ skipping 1625 matching lines @@
     return false;
 
   DCHECK_NE(have_splice_buffers, have_preroll_buffer);
   splice_buffers_index_ = 0;
   pending_buffer_.swap(*out_buffer);
   pending_buffers_complete_ = false;
   return true;
 }
 
 }  // namespace media