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

media/filters/source_buffer_range.cc

 // Copyright 2014 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_range.h"
 
 #include <algorithm>
 
 #include "media/base/timestamp_constants.h"
 
 namespace media {
 
 // Comparison operators for std::upper_bound() and std::lower_bound().
 static bool CompareTimeDeltaToStreamParserBuffer(
     const DecodeTimestamp& decode_timestamp,
     const scoped_refptr<StreamParserBuffer>& buffer) {
   return decode_timestamp < buffer->GetDecodeTimestamp();
 }
 static bool CompareStreamParserBufferToTimeDelta(
     const scoped_refptr<StreamParserBuffer>& buffer,
     const DecodeTimestamp& decode_timestamp) {
   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(
     GapPolicy gap_policy, const BufferQueue& new_buffers,
     DecodeTimestamp media_segment_start_time,
     const InterbufferDistanceCB& interbuffer_distance_cb)
     : gap_policy_(gap_policy),
       keyframe_map_index_base_(0),
       next_buffer_index_(-1),
       media_segment_start_time_(media_segment_start_time),
@@ skipping 53 matching lines @@
     }
   }
 }
 
 void SourceBufferRange::Seek(DecodeTimestamp timestamp) {
   DCHECK(CanSeekTo(timestamp));
   DCHECK(!keyframe_map_.empty());
 
   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) {
   SeekAhead(timestamp, false);
 }
 
 void SourceBufferRange::SeekAheadPast(DecodeTimestamp timestamp) {
   SeekAhead(timestamp, true);
 }
 
 void SourceBufferRange::SeekAhead(DecodeTimestamp timestamp,
                                   bool skip_given_timestamp) {
   DCHECK(!keyframe_map_.empty());
 
   KeyframeMap::iterator result =
       GetFirstKeyframeAt(timestamp, skip_given_timestamp);
 
   // If there isn't a keyframe after |timestamp|, then seek to end and return
   // kNoTimestamp to signal such.
   if (result == keyframe_map_.end()) {
     next_buffer_index_ = -1;
     return;
   }
   next_buffer_index_ = result->second - keyframe_map_index_base_;
   DCHECK_LT(next_buffer_index_, static_cast<int>(buffers_.size()));
 }
 
 void SourceBufferRange::SeekToStart() {
-  DCHECK(!buffers_.empty());
+  CHECK(!buffers_.empty());
   next_buffer_index_ = 0;
 }
 
 SourceBufferRange* SourceBufferRange::SplitRange(DecodeTimestamp timestamp) {
   CHECK(!buffers_.empty());
 
   // Find the first keyframe at or after |timestamp|.
   KeyframeMap::iterator new_beginning_keyframe =
       GetFirstKeyframeAt(timestamp, false);
 
   // If there is no keyframe after |timestamp|, we can't split the range.
   if (new_beginning_keyframe == keyframe_map_.end())
     return NULL;
 
   // Remove the data beginning at |keyframe_index| from |buffers_| and save it
   // into |removed_buffers|.
   int keyframe_index =
       new_beginning_keyframe->second - keyframe_map_index_base_;
   DCHECK_LT(keyframe_index, static_cast<int>(buffers_.size()));
   BufferQueue::iterator starting_point = buffers_.begin() + keyframe_index;
   BufferQueue removed_buffers(starting_point, buffers_.end());
 
   DecodeTimestamp new_range_start_timestamp = kNoDecodeTimestamp();
   if (GetStartTimestamp() < buffers_.front()->GetDecodeTimestamp() &&
       timestamp < removed_buffers.front()->GetDecodeTimestamp()) {
     // The split is in the gap between |media_segment_start_time_| and
     // the first buffer of the new range so we should set the start
     // time of the new range to |timestamp| so we preserve part of the
     // gap in the new range.
     new_range_start_timestamp = timestamp;

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

I might be missing something here, but this sounds like a recipe for making
*this* range become an empty range. If so, I don't see any handling (removal) of
the empty range in SourceBufferStream. 

If this is effectively creating a new range that has all the content of the
existing range.. maybe the right thing to do there is just return null? ie -
this range cannot be split. 

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

Hmm. This doesn't appear to be broken, though is a little confusing and out of
scope of this CL: SourceBufferStream calls SBR::TruncateAt(timestamp, a boolean)
shortly after SBR::SplitRange(timestamp). If the split caused |buffers_| to
become empty, then the truncate range should also be empty and TruncateAt should
return true (meaning, the range is empty and should be deleted), regardless of
the TruncateAt boolean parameter value. I added a code comment at least to
SplitRange to indicate that |this| might become empty.

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

Acknowledged. 

   }
 
   keyframe_map_.erase(new_beginning_keyframe, keyframe_map_.end());
   FreeBufferRange(starting_point, buffers_.end());
 
   // Create a new range with |removed_buffers|.
   SourceBufferRange* split_range =
       new SourceBufferRange(
           gap_policy_, removed_buffers, new_range_start_timestamp,
           interbuffer_distance_cb_);
 
   // If the next buffer position is now in |split_range|, update the state of
   // 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();
   }
 
   return split_range;
 }
 
 SourceBufferRange::BufferQueue::iterator SourceBufferRange::GetBufferItrAt(
     DecodeTimestamp timestamp,
     bool skip_given_timestamp) {
   return skip_given_timestamp
@@ skipping 70 matching lines @@
     deleted_buffers->push_back(buffers_.front());
     buffers_.pop_front();
     ++buffers_deleted;
   }
 
   // Update |keyframe_map_index_base_| to account for the deleted buffers.
   keyframe_map_index_base_ += buffers_deleted;
 
   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
   // the range.
   if (buffers_deleted > 0)
     media_segment_start_time_ = kNoDecodeTimestamp();
 
   return total_bytes_deleted;
 }
 
@@ skipping 159 matching lines @@
   next_buffer_index_++;
   return true;
 }
 
 bool SourceBufferRange::HasNextBuffer() const {
   return next_buffer_index_ >= 0 &&
       next_buffer_index_ < static_cast<int>(buffers_.size());
 }
 
 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();
   }
 
   return buffers_[next_buffer_index_]->GetDecodeTimestamp();
 }
 
 bool SourceBufferRange::HasNextBufferPosition() const {
   return next_buffer_index_ >= 0;
@@ skipping 15 matching lines @@
 }
 
 bool SourceBufferRange::CanAppendRangeToEnd(
     const SourceBufferRange& range) const {
   return CanAppendBuffersToEnd(range.buffers_);
 }
 
 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()));
 }
 
 bool SourceBufferRange::CanSeekTo(DecodeTimestamp timestamp) const {
   DecodeTimestamp start_timestamp =
       std::max(DecodeTimestamp(), GetStartTimestamp() - GetFudgeRoom());
   return !keyframe_map_.empty() && start_timestamp <= timestamp &&
       timestamp < GetBufferedEndTimestamp();
 }
 
@@ skipping 55 matching lines @@
 DecodeTimestamp SourceBufferRange::KeyframeBeforeTimestamp(
     DecodeTimestamp timestamp) {
   DCHECK(!keyframe_map_.empty());
 
   if (timestamp < GetStartTimestamp() || timestamp >= GetBufferedEndTimestamp())
     return kNoDecodeTimestamp();
 
   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 {
   // Because we do not know exactly when is the next timestamp, any buffer
   // that starts within 2x the approximate duration of a buffer is considered
   // within this range.
   return 2 * GetApproximateDuration();
 }
 
 base::TimeDelta SourceBufferRange::GetApproximateDuration() const {
@@ skipping 27 matching lines @@
     }
 
     if (buffer->timestamp() + buffer->duration() <= start.ToPresentationTime())
       continue;
     buffers->push_back(buffer);
   }
   return previous_size < buffers->size();
 }
 
 }  // namespace media