Remove buffer duration calculation from WebMClusterParser and update SourceBufferStream accordingly

media/filters/source_buffer_stream.cc

Index: media/filters/source_buffer_stream.cc
diff --git a/media/filters/source_buffer_stream.cc b/media/filters/source_buffer_stream.cc
index 0176aa9c254f13c057f471e4bac1c7b934137f1f..93063a9ae93295e71d420d85418bbaecf309c1d9 100644
--- a/media/filters/source_buffer_stream.cc
+++ b/media/filters/source_buffer_stream.cc
@@ -46,7 +46,11 @@ class SourceBufferRange {
 
   // Updates |next_buffer_index_| to point to next keyframe after or equal to
   // |timestamp|.
-  void SeekAfter(base::TimeDelta timestamp);
+  void SeekAheadTo(base::TimeDelta timestamp);
+
+  // Updates |next_buffer_index_| to point to next keyframe strictly after
+  // |timestamp|.
+  void SeekAheadPast(base::TimeDelta timestamp);
 
   // Finds the next keyframe from |buffers_| after |timestamp|, and creates and
   // returns a new SourceBufferRange with the buffers from that keyframe onward.
@@ -62,12 +66,14 @@ class SourceBufferRange {
   // were removed.
   // |deleted_buffers| contains the buffers that were deleted from this range,
   // starting at the buffer that had been at |next_buffer_index_|.
-  // |deleted_buffers| is empty if the buffer at |next_buffer_index_| was not
-  // deleted.
-  void DeleteAfter(scoped_refptr<StreamParserBuffer> buffer,
-                   BufferQueue* deleted_buffers);
+  // Returns true if the |next_buffer_index_| is reset. Note that this method
+  // may return true even if it does not add any buffers to |deleted_buffers|.
+  // This indicates that the range had not buffered |next_buffer_index_|, but
+  // a buffer at that position would have been deleted.
+  bool TruncateAt(scoped_refptr<StreamParserBuffer> buffer,
+                  BufferQueue* deleted_buffers);
   // Deletes all buffers in range.
-  void DeleteAll(BufferQueue* deleted_buffers);
+  bool DeleteAll(BufferQueue* deleted_buffers);
 
   // Updates |out_buffer| with the next buffer in presentation order. Seek()
   // must be called before calls to GetNextBuffer(), and buffers are returned
@@ -90,8 +96,7 @@ class SourceBufferRange {
   // Returns the start timestamp of the range.
   base::TimeDelta GetStartTimestamp() const;
 
-  // Returns the end timestamp of the buffered data. (Note that this is equal to
-  // the last buffer's timestamp + its duration.)
+  // Returns the end timestamp of the buffered data.
   base::TimeDelta GetEndTimestamp() const;
 
   // Returns whether a buffer with a starting timestamp of |timestamp| would
@@ -111,17 +116,37 @@ class SourceBufferRange {
   // the beginning of |range|.
   bool EndOverlaps(const SourceBufferRange& range) const;
 
+  // Returns true if |timestamp| is the timestamp of the next buffer in
+  // sequence after |buffer|, false otherwise.
+  bool IsNextInSequence(
+      const scoped_refptr<media::StreamParserBuffer>& buffer,
+      base::TimeDelta timestamp) const;
+
+  // Returns the distance in time estimating how far from the beginning or end
+  // of this range a buffer can be to considered in the range.
+  base::TimeDelta GetFudgeRoom() const;
+
  private:
-  // Helper method to delete buffers in |buffers_| starting from
+  typedef std::map<base::TimeDelta, size_t> KeyframeMap;
+
+  // Seeks the range to the next keyframe after |timestamp|. If
+  // |skip_given_timestamp| is true, the seek will go to a keyframe with a
+  // timestamp strictly greater than |timestamp|.
+  void SeekAhead(base::TimeDelta timestamp, bool skip_given_timestamp);
+  KeyframeMap::iterator GetFirstKeyframeAfter(
+      base::TimeDelta timestamp, bool skip_given_timestamp);
+
+  // Helper method to delete buffers in |buffers_| starting at
   // |starting_point|, an iterator in |buffers_|.
-  void DeleteAfter(const BufferQueue::iterator& starting_point,
-                   BufferQueue* deleted_buffers);
+  bool TruncateAt(const BufferQueue::iterator& starting_point,
+                  BufferQueue* deleted_buffers);
 
+  // Returns true if |timestamp| is the timestamp of the next buffer in
+  // sequence after |buffer|, false otherwise.

[acolwell GONE FROM CHROMIUM, 2012/07/02 17:12:33]

? Is this supposed to be above the TruncateAt() declaration?

[vrk (LEFT CHROMIUM), 2012/07/04 02:50:18]

Oops wow, not sure why this is here! (Was the comment for IsNextInSequence()).
Deleted :)

   // An ordered list of buffers in this range.
   BufferQueue buffers_;
 
   // Maps keyframe timestamps to its index position in |buffers_|.
-  typedef std::map<base::TimeDelta, size_t> KeyframeMap;
   KeyframeMap keyframe_map_;
 
   // Index into |buffers_| for the next buffer to be returned by
@@ -146,6 +171,9 @@ class SourceBufferRange {
   // range).
   base::TimeDelta media_segment_start_time_;
 
+  // Stores the largest distance between two adjacent buffers in this range.
+  base::TimeDelta max_interbuffer_distance_;

[acolwell GONE FROM CHROMIUM, 2012/07/02 17:12:33]

Do we really want to keep track of this on a per-SourceBufferRange basis? I
thought this would be a per-SourceBufferStream value.

[vrk (LEFT CHROMIUM), 2012/07/04 02:50:18]

Moved to SourceBufferStream and added a callback for the Ranges to use to access
this value.

+
   DISALLOW_COPY_AND_ASSIGN(SourceBufferRange);
 };
 
@@ -157,7 +185,7 @@ static bool IsRangeListSorted(
     const std::list<media::SourceBufferRange*>& ranges) {
   base::TimeDelta prev = media::kNoTimestamp();
   for (std::list<media::SourceBufferRange*>::const_iterator itr =
-       ranges.begin(); itr != ranges.end(); itr++) {
+       ranges.begin(); itr != ranges.end(); ++itr) {
     if (prev != media::kNoTimestamp() && prev >= (*itr)->GetStartTimestamp())
       return false;
     prev = (*itr)->GetEndTimestamp();
@@ -172,48 +200,37 @@ static bool BufferComparator(
   return first->GetTimestamp() < second->GetTimestamp();
 }
 
-// Returns the upper bound for the starting timestamp for the next buffer
-// in sequence after |buffer|. Assumes |buffer|'s timestamp and
-// duration are valid.
-static base::TimeDelta MaxNextTimestamp(
-    const scoped_refptr<media::StreamParserBuffer>& buffer) {
-  // Because we do not know exactly when is the next timestamp, any buffer
-  // that starts within 1/3 of the duration past the end of this buffer
-  // is considered the next buffer in the sequence.
-  return buffer->GetEndTimestamp() + buffer->GetDuration() / 3;
-}
-
-// Returns true if |timestamp| is the timestamp of the next buffer in
-// sequence after |buffer|, false otherwise.
-static bool IsNextInSequence(
-    const scoped_refptr<media::StreamParserBuffer>& buffer,
-    base::TimeDelta timestamp) {
-  return timestamp >= buffer->GetEndTimestamp() &&
-      timestamp <= MaxNextTimestamp(buffer);
-}
-
 namespace media {
 
 SourceBufferStream::SourceBufferStream()

[vrk (LEFT CHROMIUM), 2012/07/04 02:50:18]

FYI: Also deleted this constructor; it was only being used in the unittest.

     : seek_pending_(false),
-      seek_buffer_timestamp_(base::TimeDelta()),
+      seek_buffer_timestamp_(kNoTimestamp()),
       selected_range_(NULL),
-      end_of_stream_(false) {
+      end_of_stream_(false),
+      deleted_next_buffer_(false),
+      media_segment_start_time_(kNoTimestamp()),
+      range_for_next_append_(ranges_.end()) {
 }
 
 SourceBufferStream::SourceBufferStream(const AudioDecoderConfig& audio_config)
     : seek_pending_(false),
-      seek_buffer_timestamp_(base::TimeDelta()),
+      seek_buffer_timestamp_(kNoTimestamp()),
       selected_range_(NULL),
-      end_of_stream_(false) {
+      end_of_stream_(false),
+      deleted_next_buffer_(false),
+      media_segment_start_time_(kNoTimestamp()),
+      range_for_next_append_(ranges_.end()) {
   audio_config_.CopyFrom(audio_config);
 }
 
 SourceBufferStream::SourceBufferStream(const VideoDecoderConfig& video_config)
     : seek_pending_(false),
-      seek_buffer_timestamp_(base::TimeDelta()),
+      seek_buffer_timestamp_(kNoTimestamp()),
       selected_range_(NULL),
-      end_of_stream_(false) {
+      end_of_stream_(false),
+      deleted_next_buffer_(false),
+      media_segment_start_time_(kNoTimestamp()),
+      range_for_next_append_(ranges_.end()) {
   video_config_.CopyFrom(video_config);
 }
 
@@ -224,14 +241,26 @@ SourceBufferStream::~SourceBufferStream() {
   }
 }
 
-bool SourceBufferStream::Append(
-    const SourceBufferStream::BufferQueue& buffers,
+void SourceBufferStream::OnNewMediaSegment(
     base::TimeDelta media_segment_start_time) {
+  media_segment_start_time_ = media_segment_start_time;
+  range_for_next_append_ = ranges_.end();
+}
+
+bool SourceBufferStream::Append(
+    const SourceBufferStream::BufferQueue& buffers) {
   DCHECK(!buffers.empty());
-  DCHECK(media_segment_start_time != kNoTimestamp());
+  DCHECK(media_segment_start_time_ != kNoTimestamp());
+
+  // Save a snapshot of the |next_buffer_timestamp| before range modifications
+  // are made.
+  base::TimeDelta next_buffer_timestamp = GetNextBufferTimestamp();
 
-  // Find a range into which we'll append |buffers|.
-  RangeList::iterator range_for_new_buffers = FindExistingRangeFor(buffers);
+  // Find a range into which we'll append |buffers|. Use the last range if a new
+  // media segment hasn't been signalled since the last Append() call.
+  RangeList::iterator range_for_new_buffers = range_for_next_append_;
+  if (range_for_next_append_ == ranges_.end())
+    range_for_new_buffers = FindExistingRangeFor(buffers);

[acolwell GONE FROM CHROMIUM, 2012/07/02 17:12:33]

Could this line be moved to OnNewMediaSegment() and the conditional removed? It
seems like that would be more straight-forward.

[vrk (LEFT CHROMIUM), 2012/07/04 02:50:18]

Good idea! Done.

 
   // If there's a range for |buffers|, insert |buffers| accordingly. Otherwise,
   // create a new range with |buffers|.
@@ -242,9 +271,11 @@ bool SourceBufferStream::Append(
     if (!buffers.front()->IsKeyframe())
       return false;
     range_for_new_buffers =
-        AddToRanges(new SourceBufferRange(buffers, media_segment_start_time));
+        AddToRanges(new SourceBufferRange(buffers, media_segment_start_time_));
   }
 
+  range_for_next_append_ = range_for_new_buffers;
+
   // Resolve overlaps.
   ResolveCompleteOverlaps(range_for_new_buffers);
   ResolveEndOverlap(range_for_new_buffers);
@@ -258,10 +289,34 @@ bool SourceBufferStream::Append(
     selected_range_->Seek(buffers.front()->GetTimestamp());
   }
 
-  // Finally, try to complete pending seek if one exists.
-  if (seek_pending_)
+  // Seek to try to fulfill a previous call to Seek().
+  if (seek_pending_) {
+    DCHECK(!selected_range_);
+    DCHECK(!deleted_next_buffer_);
     Seek(seek_buffer_timestamp_);
+  }
+
+  // Seek because the Append() has deleted the buffer that would have been
+  // returned in the next call to GetNextBuffer().
+  if (deleted_next_buffer_) {

[acolwell GONE FROM CHROMIUM, 2012/07/02 17:12:33]

I'm not thrilled about deleted_next_buffer_ & deleted_buffers_ being updated as
side-effects and then used here. WDYT of making these locals here and making
deleted_buffers an out parameter of the appropriate methods and
deleted_next_buffer be the return value of those methods?

[vrk (LEFT CHROMIUM), 2012/07/04 02:50:18]

Yeah, I knew it was a wrong thing to do but was thinking it might be worth it if
the code could be simpler.
I initially didn't do this because I was trying to avoid the parameter bloat I
had in previous versions of the class, but now that I've coded it, I think
output params is still better than having fields. I made both things output
parameters instead of returning a bool because I'd like to be able to check that
deleted_next_buffer is set exactly once (if it's set at all), and that's easier
to do if it's an output parameter.

+    DCHECK(!seek_pending_);
+    selected_range_ = *range_for_new_buffers;
+    if (!deleted_buffers_.empty()) {
+      // Seek the range to the keyframe at or after |next_buffer_timestamp|.
+      selected_range_->SeekAheadTo(next_buffer_timestamp);
+    } else {
+      // If we've deleted the next buffer but |deleted_buffers_| is empty,
+      // that means |next_buffer_timestamp| represents the time we need to
+      // seek past (i.e. the keyframe timestamp needs to be strictly greater
+      // than |next_buffer_timestamp|).
+      selected_range_->SeekAheadPast(next_buffer_timestamp);
+    }
+    UpdateTrackBuffer();
+    deleted_buffers_.clear();
+    deleted_next_buffer_ = false;
+  }
 
+  DCHECK(!deleted_next_buffer_);
   DCHECK(IsRangeListSorted(ranges_));
   return true;
 }
@@ -282,39 +337,29 @@ void SourceBufferStream::InsertIntoExistingRange(
 
   // In case this is a middle overlap, save the buffers that come after the end
   // of |new_buffers|, and add them into a new range.
-  base::TimeDelta next_buffer_timestamp = GetNextBufferTimestamp();
-  bool had_next_buffer = range_for_new_buffers->HasNextBuffer();
   SourceBufferRange* new_next_range =
-      range_for_new_buffers->SplitRange(new_buffers.back()->GetEndTimestamp());
+      range_for_new_buffers->SplitRange(new_buffers.back()->GetTimestamp());
 
   if (new_next_range)
     AddToRanges(new_next_range);
 
   // Delete the buffers that are overlapped by |new_buffers|, then append
   // |new_buffers| to the end of the range.
-  BufferQueue deleted_buffers;
-  range_for_new_buffers->DeleteAfter(new_buffers.front(), &deleted_buffers);
+  bool deleted_next_buffer =
+      range_for_new_buffers->TruncateAt(new_buffers.front(), &deleted_buffers_);
   range_for_new_buffers->AppendToEnd(new_buffers);
 
-  // If |new_buffers| doesn't overlap the selected range, no need to do anything
-  // more.
-  if (selected_range_ != range_for_new_buffers || !had_next_buffer ||
-      next_buffer_timestamp < new_buffers.front()->GetTimestamp()) {
-    return;
-  }
-
   // If this was a middle overlap resulting in a new range, and the next buffer
   // position has been transferred to the newly created range, update the
   // |selected_range_| accordingly.
   if (new_next_range && new_next_range->HasNextBufferPosition()) {
     DCHECK(!range_for_new_buffers->HasNextBufferPosition());
+    DCHECK(!deleted_next_buffer);
     selected_range_ = new_next_range;
-    return;
   }
 
-  selected_range_ = range_for_new_buffers;
-  selected_range_->SeekAfter(next_buffer_timestamp);
-  UpdateTrackBuffer(deleted_buffers);
+  DCHECK(!deleted_next_buffer_);
+  deleted_next_buffer_ = deleted_next_buffer;
 }
 
 void SourceBufferStream::ResolveCompleteOverlaps(
@@ -322,21 +367,16 @@ void SourceBufferStream::ResolveCompleteOverlaps(
   SourceBufferRange* range_with_new_buffers = *range_with_new_buffers_itr;
   RangeList::iterator next_range_itr = range_with_new_buffers_itr;
   next_range_itr++;
-  base::TimeDelta next_buffer_timestamp = GetNextBufferTimestamp();
 
   while (next_range_itr != ranges_.end() &&
          range_with_new_buffers->CompletelyOverlaps(**next_range_itr)) {
     if (*next_range_itr == selected_range_) {
-      // Transfer the next buffer position from the old selected range to the
-      // range with the new buffers.
-      selected_range_ = range_with_new_buffers;
-      selected_range_->SeekAfter(next_buffer_timestamp);
-
-      // Delete everything from the old selected range and save the next
-      // buffers.
-      BufferQueue deleted_buffers;
-      (*next_range_itr)->DeleteAll(&deleted_buffers);
-      UpdateTrackBuffer(deleted_buffers);
+      selected_range_ = NULL;
+      bool deleted_next_buffer =
+          (*next_range_itr)->DeleteAll(&deleted_buffers_);
+      DCHECK(deleted_next_buffer);
+      DCHECK(!deleted_next_buffer_);
+      deleted_next_buffer_ = true;
     }
     delete *next_range_itr;
     next_range_itr = ranges_.erase(next_range_itr);
@@ -348,7 +388,6 @@ void SourceBufferStream::ResolveEndOverlap(
   SourceBufferRange* range_with_new_buffers = *range_with_new_buffers_itr;
   RangeList::iterator next_range_itr = range_with_new_buffers_itr;
   next_range_itr++;
-  base::TimeDelta next_buffer_timestamp = GetNextBufferTimestamp();
 
   if (next_range_itr == ranges_.end() ||
       !range_with_new_buffers->EndOverlaps(**next_range_itr)) {
@@ -381,22 +420,19 @@ void SourceBufferStream::ResolveEndOverlap(
     return;
   }
 
-  // Transfer the next buffer position from the old range to the range with
-  // the new buffers.
-  selected_range_ = range_with_new_buffers;
-  selected_range_->SeekAfter(next_buffer_timestamp);
+  // Save the buffers in |overlapped_range|.
+  bool deleted_next_buffer = overlapped_range->DeleteAll(&deleted_buffers_);
+  DCHECK(deleted_next_buffer);
 
-  // Update track buffer with overlapped buffers.
-  BufferQueue deleted_buffers;
-  scoped_refptr<StreamParserBuffer> buffer;
-  while (overlapped_range->GetNextBuffer(&buffer)) {
-    deleted_buffers.push_back(buffer);
-  }
-  UpdateTrackBuffer(deleted_buffers);
+  // If we end-overlap the |selected_range_|, the next buffer is either
+  // in |new_next_range| or |deleted_buffers_|. Because we've already returned
+  // early in the former case, we set |deleted_next_buffer_| to true.
+  DCHECK(!deleted_next_buffer_);
+  deleted_next_buffer_ = true;
 }
 
-void SourceBufferStream::UpdateTrackBuffer(const BufferQueue& deleted_buffers) {
-  if (!track_buffer_.empty() || deleted_buffers.empty())
+void SourceBufferStream::UpdateTrackBuffer() {
+  if (!track_buffer_.empty() || deleted_buffers_.empty())
     return;
 
   DCHECK(selected_range_);
@@ -407,17 +443,17 @@ void SourceBufferStream::UpdateTrackBuffer(const BufferQueue& deleted_buffers) {
   // If there is no gap between what was deleted and what was added, nothing
   // should be added to the track buffer.
   if (selected_range_->HasNextBuffer() &&
-      next_keyframe_timestamp == deleted_buffers.front()->GetTimestamp()) {
+      next_keyframe_timestamp == deleted_buffers_.front()->GetTimestamp()) {
     return;
   }
 
-  DCHECK(next_keyframe_timestamp >= deleted_buffers.front()->GetTimestamp());
+  DCHECK(next_keyframe_timestamp >= deleted_buffers_.front()->GetTimestamp());
 
   // If the |selected_range_| is ready to return data, fill the track buffer
   // with all buffers that come before |next_keyframe_timestamp| and return.
   if (selected_range_->HasNextBuffer()) {
-    for (BufferQueue::const_iterator itr = deleted_buffers.begin();
-         itr != deleted_buffers.end() &&
+    for (BufferQueue::const_iterator itr = deleted_buffers_.begin();
+         itr != deleted_buffers_.end() &&
          (*itr)->GetTimestamp() < next_keyframe_timestamp; ++itr) {
       track_buffer_.push_back(*itr);
     }
@@ -426,16 +462,16 @@ void SourceBufferStream::UpdateTrackBuffer(const BufferQueue& deleted_buffers) {
 
   // Otherwise, the |selected_range_| is not ready to return data, so add all
   // the deleted buffers into the |track_buffer_|.
-  track_buffer_ = deleted_buffers;
+  track_buffer_.swap(deleted_buffers_);
 
   // See if the next range contains the keyframe after the end of the
   // |track_buffer_|, and if so, change |selected_range_|.
   RangeList::iterator next_range_itr = ++(GetSelectedRangeItr());
   if (next_range_itr != ranges_.end()) {
-    (*next_range_itr)->SeekAfter(track_buffer_.back()->GetEndTimestamp());
+    (*next_range_itr)->SeekAheadPast(track_buffer_.back()->GetTimestamp());
     if ((*next_range_itr)->HasNextBuffer() &&
-        IsNextInSequence(track_buffer_.back(),
-                         (*next_range_itr)->GetNextTimestamp())) {
+        selected_range_->IsNextInSequence(
+            track_buffer_.back(), (*next_range_itr)->GetNextTimestamp())) {
       selected_range_ = *next_range_itr;
     }
   }
@@ -463,6 +499,8 @@ void SourceBufferStream::MergeWithAdjacentRangeIfNecessary(
 }
 
 void SourceBufferStream::Seek(base::TimeDelta timestamp) {
+  DCHECK(!deleted_next_buffer_);
+
   selected_range_ = NULL;
   track_buffer_.clear();
 
@@ -570,18 +608,38 @@ SourceBufferRange::SourceBufferRange(const BufferQueue& new_buffers,
     : next_buffer_index_(-1),
       waiting_for_keyframe_(false),
       next_keyframe_timestamp_(kNoTimestamp()),
-      media_segment_start_time_(media_segment_start_time) {
+      media_segment_start_time_(media_segment_start_time),
+      max_interbuffer_distance_(kNoTimestamp()) {
   DCHECK(!new_buffers.empty());
   DCHECK(new_buffers.front()->IsKeyframe());
   AppendToEnd(new_buffers);
 }
 
 void SourceBufferRange::AppendToEnd(const BufferQueue& new_buffers) {
+  DCHECK(!new_buffers.empty());
+  base::TimeDelta prev_timestamp = kNoTimestamp();
+  if (!buffers_.empty())
+    prev_timestamp = buffers_.back()->GetTimestamp();
+
   for (BufferQueue::const_iterator itr = new_buffers.begin();
        itr != new_buffers.end(); itr++) {
-    DCHECK((*itr)->GetDuration() > base::TimeDelta());
-    DCHECK((*itr)->GetTimestamp() != kNoTimestamp());
+    base::TimeDelta current_timestamp = (*itr)->GetTimestamp();
+    DCHECK(current_timestamp != kNoTimestamp());
     buffers_.push_back(*itr);
+
+    // Save the max interbuffer distance seen in the range.
+    if (prev_timestamp != kNoTimestamp()) {
+      base::TimeDelta interbuffer_distance = current_timestamp - prev_timestamp;
+      if (max_interbuffer_distance_ == kNoTimestamp()) {
+        max_interbuffer_distance_ = interbuffer_distance;
+      } else {
+        max_interbuffer_distance_ =
+            std::max(max_interbuffer_distance_, interbuffer_distance);
+      }
+    }
+    prev_timestamp = current_timestamp;
+
+    // Update keyframe index with new entry if applicable.
     if ((*itr)->IsKeyframe()) {
       keyframe_map_.insert(
           std::make_pair((*itr)->GetTimestamp(), buffers_.size() - 1));
@@ -615,12 +673,20 @@ void SourceBufferRange::Seek(base::TimeDelta timestamp) {
   DCHECK_LT(next_buffer_index_, static_cast<int>(buffers_.size()));
 }
 
-void SourceBufferRange::SeekAfter(base::TimeDelta timestamp) {
+void SourceBufferRange::SeekAheadTo(base::TimeDelta timestamp) {
+  SeekAhead(timestamp, false);
+}
+
+void SourceBufferRange::SeekAheadPast(base::TimeDelta timestamp) {
+  SeekAhead(timestamp, true);
+}
+
+void SourceBufferRange::SeekAhead(base::TimeDelta timestamp,
+                                  bool skip_given_timestamp) {
   DCHECK(!keyframe_map_.empty());
 
-  // lower_bound() returns the first element >= |timestamp|, so |result| is the
-  // value that we want.
-  KeyframeMap::iterator result = keyframe_map_.lower_bound(timestamp);
+  KeyframeMap::iterator result =
+      GetFirstKeyframeAfter(timestamp, skip_given_timestamp);
 
   // If there isn't a keyframe after |timestamp|, then seek to end and return
   // kNoTimestamp to signal such.
@@ -630,15 +696,14 @@ void SourceBufferRange::SeekAfter(base::TimeDelta timestamp) {
     next_keyframe_timestamp_ = timestamp;
     return;
   }
-
   next_buffer_index_ = result->second;
   DCHECK_LT(next_buffer_index_, static_cast<int>(buffers_.size()));
 }
 
 SourceBufferRange* SourceBufferRange::SplitRange(base::TimeDelta timestamp) {
-  // Find the first keyframe after |timestamp|.
+  // Find the first keyframe after |timestamp|, not including |timestamp|.
   KeyframeMap::iterator new_beginning_keyframe =
-      keyframe_map_.lower_bound(timestamp);
+      GetFirstKeyframeAfter(timestamp, true);
 
   // If there is no keyframe after |timestamp|, we can't split the range.
   if (new_beginning_keyframe == keyframe_map_.end())
@@ -666,38 +731,57 @@ SourceBufferRange* SourceBufferRange::SplitRange(base::TimeDelta timestamp) {
   return split_range;
 }
 
-void SourceBufferRange::DeleteAll(BufferQueue* removed_buffers) {
-  DeleteAfter(buffers_.begin(), removed_buffers);
+SourceBufferRange::KeyframeMap::iterator
+SourceBufferRange::GetFirstKeyframeAfter(base::TimeDelta timestamp,
+                                         bool skip_given_timestamp) {
+  KeyframeMap::iterator result = keyframe_map_.lower_bound(timestamp);
+  // lower_bound() returns the first element >= |timestamp|, so if we don't want
+  // to include keyframes == |timestamp|, we have to increment the iterator
+  // accordingly.
+  if (skip_given_timestamp &&
+      result != keyframe_map_.end() && result->first == timestamp) {
+    ++result;
+  }
+  return result;
+}
+
+bool SourceBufferRange::DeleteAll(BufferQueue* removed_buffers) {
+  return TruncateAt(buffers_.begin(), removed_buffers);
 }
 
-void SourceBufferRange::DeleteAfter(
+bool SourceBufferRange::TruncateAt(
     scoped_refptr<StreamParserBuffer> buffer, BufferQueue* removed_buffers) {
   // Find the place in |buffers_| where we will begin deleting data.
   BufferQueue::iterator starting_point =
       std::lower_bound(buffers_.begin(), buffers_.end(),
                        buffer,
                        BufferComparator);
-  DeleteAfter(starting_point, removed_buffers);
+  return TruncateAt(starting_point, removed_buffers);
 }
 
-void SourceBufferRange::DeleteAfter(
+bool SourceBufferRange::TruncateAt(
     const BufferQueue::iterator& starting_point, BufferQueue* removed_buffers) {
   DCHECK(removed_buffers);
+  DCHECK(removed_buffers->empty());
+
   // Return if we're not deleting anything.
   if (starting_point == buffers_.end())
-    return;
+    return false;
 
+  bool removed_next_buffer = false;
   // Reset the next buffer index if we will be deleting the buffer that's next
   // in sequence.
-  if (HasNextBuffer() &&
+  if (HasNextBufferPosition() &&
       GetNextTimestamp() >= (*starting_point)->GetTimestamp()) {
-    // Save the buffers we're about to delete if the output parameter is valid.
-    int starting_offset = starting_point - buffers_.begin();
-    int next_buffer_offset = next_buffer_index_ - starting_offset;
-    DCHECK_GE(next_buffer_offset, 0);
-    BufferQueue saved(starting_point + next_buffer_offset, buffers_.end());
-    removed_buffers->swap(saved);
+    if (HasNextBuffer()) {
+      int starting_offset = starting_point - buffers_.begin();
+      int next_buffer_offset = next_buffer_index_ - starting_offset;
+      DCHECK_GE(next_buffer_offset, 0);
+      BufferQueue saved(starting_point + next_buffer_offset, buffers_.end());
+      removed_buffers->swap(saved);
+    }
     next_buffer_index_ = -1;
+    removed_next_buffer = true;
   }
 
   // Remove keyframes from |starting_point| onward.
@@ -707,6 +791,7 @@ void SourceBufferRange::DeleteAfter(
 
   // Remove everything from |starting_point| onward.
   buffers_.erase(starting_point, buffers_.end());
+  return removed_next_buffer;
 }
 
 bool SourceBufferRange::GetNextBuffer(
@@ -735,7 +820,7 @@ base::TimeDelta SourceBufferRange::GetNextTimestamp() const {
     return next_keyframe_timestamp_;
 
   if (next_buffer_index_ >= static_cast<int>(buffers_.size()))
-    return buffers_.back()->GetEndTimestamp();
+    return buffers_.back()->GetTimestamp();

[acolwell GONE FROM CHROMIUM, 2012/07/02 17:12:33]

Why is this ok? Wouldn't we want to use fudge room here if we don't want to
require duration?

[vrk (LEFT CHROMIUM), 2012/07/04 02:50:18]

Talked offline, returning kNoTimestamp() instead and made necessary changes in
SourceBufferStream::Append() and SourceBufferRange::TruncateAt()

 
   return buffers_.at(next_buffer_index_)->GetTimestamp();
 }
@@ -772,8 +857,10 @@ bool SourceBufferRange::BelongsToRange(base::TimeDelta timestamp) const {
 }
 
 bool SourceBufferRange::CanSeekTo(base::TimeDelta timestamp) const {
-  return !keyframe_map_.empty() && GetStartTimestamp() <= timestamp &&
-      GetEndTimestamp() > timestamp;
+  base::TimeDelta start_range =
+      std::max(base::TimeDelta(), GetStartTimestamp() - GetFudgeRoom());
+  return !keyframe_map_.empty() && start_range <= timestamp &&
+      timestamp <= GetEndTimestamp();
 }
 
 bool SourceBufferRange::CompletelyOverlaps(
@@ -783,7 +870,7 @@ bool SourceBufferRange::CompletelyOverlaps(
 }
 
 bool SourceBufferRange::EndOverlaps(const SourceBufferRange& range) const {
-  return range.GetStartTimestamp() < GetEndTimestamp() &&
+  return range.GetStartTimestamp() <= GetEndTimestamp() &&
       GetEndTimestamp() < range.GetEndTimestamp();
 }
 
@@ -797,7 +884,27 @@ base::TimeDelta SourceBufferRange::GetStartTimestamp() const {
 
 base::TimeDelta SourceBufferRange::GetEndTimestamp() const {
   DCHECK(!buffers_.empty());
-  return buffers_.back()->GetEndTimestamp();
+  return buffers_.back()->GetTimestamp();

[acolwell GONE FROM CHROMIUM, 2012/07/02 17:12:33]

Shouldn't we be using fudge here?

[vrk (LEFT CHROMIUM), 2012/07/04 02:50:18]

I avoided it here because this was being used in the end-overlap scenarios. I
added a separate method GetBufferedEndTimestamp() for GetBufferedTime() and
CanSeekTo().

+}
+
+bool SourceBufferRange::IsNextInSequence(
+    const scoped_refptr<media::StreamParserBuffer>& buffer,
+    base::TimeDelta timestamp) const {
+  return buffer->GetTimestamp() < timestamp &&
+      timestamp <= buffer->GetTimestamp() + GetFudgeRoom();
+}
+
+base::TimeDelta SourceBufferRange::GetFudgeRoom() const {
+  // Use a constant default fudge room of 250ms if we've seen fewer than 2
+  // buffers in this range.
+  base::TimeDelta fudge = base::TimeDelta::FromMilliseconds(250);
+
+  // Because we do not know exactly when is the next timestamp, any buffer
+  // that starts within 2x the |max_interbuffer_distance_| seen so far in this
+  // range is considered the next buffer in the sequence.
+  if (max_interbuffer_distance_ != kNoTimestamp())

[acolwell GONE FROM CHROMIUM, 2012/07/02 17:12:33]

nit: Reverse check and early return default then just return the
2*max_interbuffer_distance_ below.

[vrk (LEFT CHROMIUM), 2012/07/04 02:50:18]

Done.

+    fudge = 2 * max_interbuffer_distance_;
+  return fudge;
 }
 
 }  // namespace media