MediaSource: Fix CHECK crash in append fudge room edge case.

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 <sstream>
 
@@ skipping 189 matching lines @@
   new_coded_frame_group_ = true;
 
   RangeList::iterator last_range = range_for_next_append_;
   range_for_next_append_ = FindExistingRangeFor(coded_frame_group_start_time);
 
   // Only reset |last_appended_buffer_timestamp_| if this new coded frame group
   // is not adjacent to the previous coded frame group appended to the stream.
   if (range_for_next_append_ == ranges_.end() ||
       !AreAdjacentInSequence(last_appended_buffer_timestamp_,
                              coded_frame_group_start_time)) {
-    last_appended_buffer_timestamp_ = kNoDecodeTimestamp();
-    last_appended_buffer_duration_ = kNoTimestamp;
-    last_appended_buffer_is_keyframe_ = false;
+    ResetLastAppendedState();
     DVLOG(3) << __func__ << " next appended buffers will "
              << (range_for_next_append_ == ranges_.end()
                      ? "be in a new range"
                      : "overlap an existing range");
   } else if (last_range != ranges_.end()) {
     DCHECK(last_range == range_for_next_append_);
     DVLOG(3) << __func__ << " next appended buffers will continue range unless "
              << "intervening remove makes discontinuity";
   }
 }
@@ skipping 224 matching lines @@
   if (new_coded_frame_group_)
     return coded_frame_group_start_time_;
 
   // If we still don't know a potential next append timestamp, then we have
   // removed the ranged to which it previously belonged and have not completed a
   // subsequent append or received a subsequent OnStartOfCodedFrameGroup()
   // signal.
   return kNoDecodeTimestamp();
 }
 
+void SourceBufferStream::UpdateLastAppendStateForRemove(
+    DecodeTimestamp remove_start,
+    DecodeTimestamp remove_end,
+    bool exclude_start) {
+  // TODO(chcunningham): change exclude_start to include_start in this class and
+  // SourceBufferRange. Negatives are hard to reason about.
+  bool include_start = !exclude_start;
+
+  // No need to check previous append's GOP if starting a new CFG. New CFG is
+  // already required to begin with a key frame.
+  if (new_coded_frame_group_)
+    return;
+
+  if (range_for_next_append_ != ranges_.end()) {
+    if (last_appended_buffer_timestamp_ != kNoDecodeTimestamp()) {
+      DCHECK((*range_for_next_append_)
+                 ->BelongsToRange(last_appended_buffer_timestamp_));
+
+      // Note start and end of last appended GOP.
+      DecodeTimestamp gop_end = last_appended_buffer_timestamp_;
+      DecodeTimestamp gop_start =
+          (*range_for_next_append_)->KeyframeBeforeTimestamp(gop_end);
+
+      // If last append is about to be disrupted, reset associated state so we
+      // know to create a new range for future appends and require an initial
+      // key frame.
+      if (((include_start && remove_start == gop_end) ||
+           remove_start < gop_end) &&
+          remove_end > gop_start) {
+        DVLOG(2) << __func__ << " " << GetStreamTypeName()
+                 << " Resetting next append state for remove ("
+                 << remove_start.InSecondsF() << ", " << remove_end.InSecondsF()
+                 << ", " << exclude_start << ")";
+        range_for_next_append_ = ranges_.end();
+        ResetLastAppendedState();
+      }
+    } else {
+      NOTREACHED() << __func__ << " " << GetStreamTypeName()
+                   << " range_for_next_append_ set, but not tracking last"
+                   << " append nor new coded frame group.";
+    }
+  }
+}
+
 void SourceBufferStream::RemoveInternal(DecodeTimestamp start,
                                         DecodeTimestamp end,
                                         bool exclude_start,
                                         BufferQueue* deleted_buffers) {
   DVLOG(2) << __func__ << " " << GetStreamTypeName() << " ("
            << start.InSecondsF() << ", " << end.InSecondsF() << ", "
            << exclude_start << ")";
   DVLOG(3) << __func__ << " " << GetStreamTypeName()
            << ": before remove ranges_=" << RangesToString(ranges_);
 
   DCHECK(start >= DecodeTimestamp());
   DCHECK(start < end) << "start " << start.InSecondsF()
                       << " end " << end.InSecondsF();
   DCHECK(deleted_buffers);
 
+  // Doing this upfront simplifies decisions about range_for_next_append_ below.
+  UpdateLastAppendStateForRemove(start, end, exclude_start);
+
   RangeList::iterator itr = ranges_.begin();
-
   while (itr != ranges_.end()) {
     SourceBufferRange* range = *itr;
     if (range->GetStartTimestamp() >= end)
       break;
 
     // Split off any remaining GOPs starting at or after |end| and add it to
     // |ranges_|.
     SourceBufferRange* new_range = range->SplitRange(end);
     if (new_range) {
       itr = ranges_.insert(++itr, new_range);
@@ skipping 73 matching lines @@
   SetSelectedRange(NULL);
   track_buffer_.clear();
   config_change_pending_ = false;
   last_output_buffer_timestamp_ = kNoDecodeTimestamp();
   just_exhausted_track_buffer_ = false;
   splice_buffers_index_ = 0;
   pending_buffer_ = NULL;
   pending_buffers_complete_ = false;
 }
 
+void SourceBufferStream::ResetLastAppendedState() {
+  last_appended_buffer_timestamp_ = kNoDecodeTimestamp();
+  last_appended_buffer_duration_ = kNoTimestamp;
+  last_appended_buffer_is_keyframe_ = false;
+}
+
 bool SourceBufferStream::ShouldSeekToStartOfBuffered(
     base::TimeDelta seek_timestamp) const {
   if (ranges_.empty())
     return false;
   base::TimeDelta beginning_of_buffered =
       ranges_.front()->GetStartTimestamp().ToPresentationTime();
   return (seek_timestamp <= beginning_of_buffered &&
           beginning_of_buffered < kSeekToStartFudgeRoom());
 }
 
@@ skipping 529 matching lines @@
 
   SeekAndSetSelectedRange(*itr, seek_dts);
   seek_pending_ = false;
 }
 
 bool SourceBufferStream::IsSeekPending() const {
   return seek_pending_ && !IsEndOfStreamReached();
 }
 
 void SourceBufferStream::OnSetDuration(base::TimeDelta duration) {
-  DecodeTimestamp duration_dts =
-      DecodeTimestamp::FromPresentationTime(duration);
-  DVLOG(1) << __func__ << " " << GetStreamTypeName() << " ("
-           << duration.InSecondsF() << ")";
-
-  RangeList::iterator itr = ranges_.end();
-  for (itr = ranges_.begin(); itr != ranges_.end(); ++itr) {
-    if ((*itr)->GetEndTimestamp() > duration_dts)
-      break;
-  }
-  if (itr == ranges_.end())
+  if (ranges_.empty())

[wolenetz, 2016/10/05 22:41:09]

nit: why remove the DVLOG? Maybe retain it but with lower verbosity if debug
spam was concern.

[chcunningham, 2016/10/06 21:57:33]

Added back.

     return;
 
-  // Need to partially truncate this range.
-  if ((*itr)->GetStartTimestamp() < duration_dts) {
-    bool delete_range = (*itr)->TruncateAt(duration_dts, NULL, false);
-    if ((*itr == selected_range_) && !selected_range_->HasNextBufferPosition())
+  DecodeTimestamp start = DecodeTimestamp::FromPresentationTime(duration);
+  DecodeTimestamp end = ranges_.back()->GetBufferedEndTimestamp();
+
+  // Trim the end if it exceeds the new duration.
+  if (start < end) {

[wolenetz, 2016/10/05 22:41:09]

aside: this truncate-on-duration-reduction behavior is being deprecated (at
blink-side, and already disallowed in spec)

[chcunningham, 2016/10/06 21:57:32]

Acknowledged.

+    BufferQueue deleted_buffers;
+    RemoveInternal(start, end, false, &deleted_buffers);
+
+    if (!deleted_buffers.empty()) {
+      // Truncation removed current seek position. Clear selected range.

[wolenetz, 2016/10/05 22:41:09]

nit: let's be careful not to confuse seek position (could be misinterpreted as
pending seek to unbuffered position) with current position in the stream. tl;dr:
s/seek//

FWIW, I think we still have some separate edge cases (not to fix in this CL)
around removals, currentTime interpolation, and duration reduction, where a seek
to truncated duration might not be done right. However, those edge cases will be
simplified soon once we fully deprecate the old "allow automatic range removals
and partial frame rendering on duration reduction".

Regardless, we shouldn't need to worry about end_of_stream_ being true here (and
perhaps having to emit EOS buffers directly from this code path), since the API
should guarantee that MediaSource object is in "open" state in this code path.

So, please also add DCHECK(!end_of_stream_) to the beginning of this method.

[chcunningham, 2016/10/06 21:57:32]

Done.

       SetSelectedRange(NULL);
-
-    if (delete_range) {
-      DeleteAndRemoveRange(&itr);
-    } else {
-      ++itr;
     }
   }
-
-  // Delete all ranges that begin after |duration_dts|.
-  while (itr != ranges_.end()) {
-    // If we're about to delete the selected range, also reset the seek state.
-    DCHECK((*itr)->GetStartTimestamp() >= duration_dts);
-    if (*itr == selected_range_)
-      ResetSeekState();
-    DeleteAndRemoveRange(&itr);
-  }
 }
 
 SourceBufferStream::Status SourceBufferStream::GetNextBuffer(
     scoped_refptr<StreamParserBuffer>* out_buffer) {
   DVLOG(2) << __func__ << " " << GetStreamTypeName();
   if (!pending_buffer_.get()) {
     const SourceBufferStream::Status status = GetNextBufferInternal(out_buffer);
     if (status != SourceBufferStream::kSuccess ||
         !SetPendingBuffer(out_buffer)) {
       DVLOG(2) << __func__ << " " << GetStreamTypeName()
@@ skipping 517 matching lines @@
 
   DCHECK(*itr != ranges_.end());
   if (**itr == selected_range_) {
     DVLOG(1) << __func__ << " deleting selected range.";
     SetSelectedRange(NULL);
   }
 
   if (*itr == range_for_next_append_) {
     DVLOG(1) << __func__ << " deleting range_for_next_append_.";
     range_for_next_append_ = ranges_.end();
-    last_appended_buffer_timestamp_ = kNoDecodeTimestamp();
-    last_appended_buffer_is_keyframe_ = false;
+    ResetLastAppendedState();
   }
 
   delete **itr;
   *itr = ranges_.erase(*itr);
 }
 
 void SourceBufferStream::GenerateSpliceFrame(const BufferQueue& new_buffers) {
   DCHECK(!new_buffers.empty());
 
   // Splice frames are only supported for audio.
@@ skipping 111 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