Reland: MSE: Relax the 'media segment must begin with keyframe' requirement

media/filters/source_buffer_range.cc

Index: media/filters/source_buffer_range.cc
diff --git a/media/filters/source_buffer_range.cc b/media/filters/source_buffer_range.cc
index 562e14e0964a4ebdaf438598a6de4670732b2b79..821e2a788b4c6fe2839cf46370f350e48c05a608 100644
--- a/media/filters/source_buffer_range.cc
+++ b/media/filters/source_buffer_range.cc
@@ -22,9 +22,10 @@ static bool CompareStreamParserBufferToTimeDelta(
   return buffer->GetDecodeTimestamp() < decode_timestamp;
 }
 
-bool SourceBufferRange::AllowSameTimestamp(
-    bool prev_is_keyframe, bool current_is_keyframe) {
-  return prev_is_keyframe || !current_is_keyframe;
+bool SourceBufferRange::IsUncommonSameTimestampSequence(
+    bool prev_is_keyframe,
+    bool current_is_keyframe) {
+  return current_is_keyframe && !prev_is_keyframe;
 }
 
 SourceBufferRange::SourceBufferRange(
@@ -98,7 +99,8 @@ void SourceBufferRange::Seek(DecodeTimestamp timestamp) {
 
   KeyframeMap::iterator result = GetFirstKeyframeAtOrBefore(timestamp);
   next_buffer_index_ = result->second - keyframe_map_index_base_;
-  DCHECK_LT(next_buffer_index_, static_cast<int>(buffers_.size()));
+  CHECK_LT(next_buffer_index_, static_cast<int>(buffers_.size()))
+      << next_buffer_index_ << ", size = " << buffers_.size();
 }
 
 void SourceBufferRange::SeekAheadTo(DecodeTimestamp timestamp) {
@@ -127,7 +129,7 @@ void SourceBufferRange::SeekAhead(DecodeTimestamp timestamp,
 }
 
 void SourceBufferRange::SeekToStart() {
-  DCHECK(!buffers_.empty());
+  CHECK(!buffers_.empty());
   next_buffer_index_ = 0;
 }
 
@@ -173,6 +175,8 @@ SourceBufferRange* SourceBufferRange::SplitRange(DecodeTimestamp timestamp) {
   // this range and |split_range| accordingly.
   if (next_buffer_index_ >= static_cast<int>(buffers_.size())) {
     split_range->next_buffer_index_ = next_buffer_index_ - keyframe_index;
+    CHECK_GE(split_range->next_buffer_index_, 0)

[chcunningham, 2016/02/12 22:46:40]

You might also add a check for next_buffer_index < split_range_->size()

[wolenetz, 2016/02/24 00:34:46]

Done.... BUT: the new CHECK_LT triggered a crash in
SourceBufferStreamTest.RemoveShouldAlwaysExcludeEnd !!
I'm investigating and will post a further patch set once I figure that out.

[chcunningham, 2016/02/24 19:06:26]

Acknowledged. Will be interesting to see whats going on with that test

[chcunningham, 2016/02/24 19:12:54]

We chatted. For posterity: You made needed to make the  CHECK_LE rather than
strictly _LT because SBR has subtle behavior where the next_buffer_index_ will
be increased until it equals size.

+        << split_range->next_buffer_index_;
     ResetNextBufferPosition();
   }
 
@@ -263,7 +267,8 @@ size_t SourceBufferRange::DeleteGOPFromFront(BufferQueue* deleted_buffers) {
 
   if (next_buffer_index_ > -1) {
     next_buffer_index_ -= buffers_deleted;
-    DCHECK_GE(next_buffer_index_, 0);
+    CHECK_GE(next_buffer_index_, 0) << next_buffer_index_ << ", deleted "
+                                    << buffers_deleted;
   }
 
   // Invalidate media segment start time if we've deleted the first buffer of
@@ -443,15 +448,16 @@ bool SourceBufferRange::HasNextBuffer() const {
 }
 
 int SourceBufferRange::GetNextConfigId() const {
-  DCHECK(HasNextBuffer());
+  CHECK(HasNextBuffer()) << next_buffer_index_;
   // If the next buffer is an audio splice frame, the next effective config id
   // comes from the first fade out preroll buffer.
   return buffers_[next_buffer_index_]->GetSpliceBufferConfigId(0);
 }
 
 DecodeTimestamp SourceBufferRange::GetNextTimestamp() const {
-  DCHECK(!buffers_.empty());
-  DCHECK(HasNextBufferPosition());
+  CHECK(!buffers_.empty()) << next_buffer_index_;
+  CHECK(HasNextBufferPosition()) << next_buffer_index_
+                                 << ", size=" << buffers_.size();
 
   if (next_buffer_index_ >= static_cast<int>(buffers_.size())) {
     return kNoDecodeTimestamp();
@@ -487,14 +493,13 @@ bool SourceBufferRange::CanAppendRangeToEnd(
 bool SourceBufferRange::CanAppendBuffersToEnd(
     const BufferQueue& buffers) const {
   DCHECK(!buffers_.empty());
-  return IsNextInSequence(buffers.front()->GetDecodeTimestamp(),
-                          buffers.front()->is_key_frame());
+  return IsNextInSequence(buffers.front()->GetDecodeTimestamp());
 }
 
 bool SourceBufferRange::BelongsToRange(DecodeTimestamp timestamp) const {
   DCHECK(!buffers_.empty());
 
-  return (IsNextInSequence(timestamp, false) ||
+  return (IsNextInSequence(timestamp) ||
           (GetStartTimestamp() <= timestamp && timestamp <= GetEndTimestamp()));
 }
 
@@ -570,17 +575,11 @@ DecodeTimestamp SourceBufferRange::KeyframeBeforeTimestamp(
   return GetFirstKeyframeAtOrBefore(timestamp)->first;
 }
 
-bool SourceBufferRange::IsNextInSequence(
-    DecodeTimestamp timestamp, bool is_key_frame) const {
+bool SourceBufferRange::IsNextInSequence(DecodeTimestamp timestamp) const {
   DecodeTimestamp end = buffers_.back()->GetDecodeTimestamp();
-  if (end < timestamp &&
-      (gap_policy_ == ALLOW_GAPS ||
-       timestamp <= end + GetFudgeRoom())) {
-    return true;
-  }
-
-  return timestamp == end && AllowSameTimestamp(
-      buffers_.back()->is_key_frame(), is_key_frame);
+  return (end == timestamp ||
+          (end < timestamp &&
+           (gap_policy_ == ALLOW_GAPS || timestamp <= end + GetFudgeRoom())));
 }
 
 base::TimeDelta SourceBufferRange::GetFudgeRoom() const {