Improving WebM video duration estimation.

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 a80d629fe8a620d18177e5ecd33aac34811894c9..e57efec75bbe452ce0fab968d09e707c0ea4445c 100644
--- a/media/filters/source_buffer_range.cc
+++ b/media/filters/source_buffer_range.cc
@@ -48,6 +48,9 @@ void SourceBufferRange::AppendBuffersToEnd(const BufferQueue& new_buffers) {
   DCHECK(media_segment_start_time_ == kNoDecodeTimestamp() ||
          media_segment_start_time_ <=
              new_buffers.front()->GetDecodeTimestamp());
+
+  AdjustEstimatedDurationForNewAppend(new_buffers);
+
   for (BufferQueue::const_iterator itr = new_buffers.begin();
        itr != new_buffers.end();
        ++itr) {
@@ -63,11 +66,36 @@ void SourceBufferRange::AppendBuffersToEnd(const BufferQueue& new_buffers) {
   }
 }
 
+void SourceBufferRange::AdjustEstimatedDurationForNewAppend(

[wolenetz, 2015/03/28 00:26:06]

Does adjustment to duration correctly include when SBS::track_buffer_ ends with
the last appended buffer and needs to have its estimated duration adjusted based
on |new_buffers|?

[chcunningham, 2015/04/13 23:25:17]

This is a great question and I've given it a lot of thought. At present I am
*not* adjusting the duration for the buffer at the tail of the
SBS::track_buffer_. I'm thinking of leaving it this way...

There are a few ways I've pondered this:

1. What is the likelihood that adjusting the track buffer like we adjust in
SourceBufferedRange will correct a bad estimate (which is the whole point of the
adjustment)? I would say the likelihood is zero. The buffers put into the track
buffer are not just a little overlapped (due to an off estimate). They are
entirely overlapped (at least their start times are) by buffers that are now
sitting in our tracked ranges.

2. But maybe its worth adjusting the duration anyway? My opinion at this point
is it doesn't make sense to adjust (regardless of whether the tail is
estimated). I came to this conclusion after considering how we might do the
adjusting. 

I considered searching the ranges for a buffer (lets call it  NEXT_IN_TIME)
who's start time most closely follows the tail of the track_buffer_. We could
then just take the delta's in these start times and assign the track_buffer_'s
tail this delta as its duration.

The thing is, not only is that probably not the encoded duration for the buffer,
but also, its not even consistent with how the timeline behaves after we've
exhausted the track buffer. As you know, once the track buffer is exhausted the
next frame we play is the next KEY frame in our ranges who's DTS follows the
track buffer's last DTS. This key frame may be any number of frames apart from
that NEXT_IN_TIME buffer we used to compute the duration. 

little diagram:
https://drive.google.com/file/d/0BwlI4ijQnVJRa3l5VUZ4Y2xONFk/view?usp=sharing

[wolenetz, 2015/04/15 02:55:23]

I agree.
I agree.

Thanks for looking into this track_buffer_ case.

+    const BufferQueue& new_buffers) {
+  if (buffers_.empty() || new_buffers.empty()) {

[wolenetz, 2015/03/28 00:26:06]

nit: Change call to only occur when buffers_ is not empty. Then DCHECK both
!buffers_.empty() and !new_buffers.empty() here. Note that AppendBuffersToEnd
already protects against new_buffers_ being empty when buffers_ is not empty
(see CanAppendBuffersToEnd, which should fail hard if the new buffers are
empty).

[chcunningham, 2015/04/13 23:25:18]

Let me push back a little here (or at least argue the other side). I think the
way I have it is a simpler contract. Its easy to reason about, callers have less
overhead this way, and its overall less lines of code. If you see major upside
in the other approach LMK.

In classes that are this complex I'm sensitive to having contracts like "dont
call if empty" which are subject subtly breaking in some later refactoring 

[wolenetz, 2015/04/15 02:55:23]

That's fine. If this method really needed to require non-empty buffers, then
DCHECK makes sense. Otherwise, it was only a nit :)

+    return;
+  }
+
+  // Only adjust the last of the previously appended buffers. The last buffer

[wolenetz, 2015/03/28 00:26:05]

nit: s/buffers./buffers if its duration was previously estimated.

[chcunningham, 2015/04/13 23:25:17]

Done.

+  // in a given append is an edge case where the parsers (upstream) cannot know
+  // the implicit PTS-delta duration because the do not know the PTS of the

[wolenetz, 2015/03/28 00:26:05]

nit: s/because the/because they/.. Actually, is this too much info? Maybe reduce
some of this to "If estimated, refine the estimation as necessary using the PTS
of the first new buffer being appended that has been deemed to be next in this
range."

[chcunningham, 2015/04/13 23:25:17]

Done. Changed a little from what you wrote, but did make it much shorter. I'm
hoping that the reason for focusing on the last of the estimated buffers is well
documented.

+  // frame that may follow the last for that append. The PTS of this new append
+  // supplies the missing information for a better duration estimate.
+  scoped_refptr<StreamParserBuffer> last_appended_buffer = buffers_.back();

[wolenetz, 2015/03/28 00:26:05]

nit: const& the smart pointer to reduce unnecessary refcount churn locally here.

[chcunningham, 2015/04/13 23:25:18]

Done. Made auto too.

+  if (last_appended_buffer->is_duration_estimated()) {
+    base::TimeDelta timestamp_delta =
+        new_buffers.front()->timestamp() - last_appended_buffer->timestamp();
+    if (last_appended_buffer->duration() != timestamp_delta) {
+      DVLOG(1) << "Replacing estimated duration ("
+               << last_appended_buffer->duration()
+               << ") from last append with derived duration ("

[wolenetz, 2015/03/28 00:26:06]

nit: may not have been from "last" append.

[chcunningham, 2015/04/13 23:25:17]

Done. Now reads "Replacing estimated duration (X) from previous range-end with
derived duration (Y)."

+               << timestamp_delta << ").";
+      last_appended_buffer->set_duration(timestamp_delta);
+    }
+  }
+}
+
 void SourceBufferRange::Seek(DecodeTimestamp timestamp) {
   DCHECK(CanSeekTo(timestamp));
   DCHECK(!keyframe_map_.empty());
 
-  KeyframeMap::iterator result = GetFirstKeyframeBefore(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()));
 }
@@ -175,7 +203,7 @@ SourceBufferRange::GetFirstKeyframeAt(DecodeTimestamp timestamp,
 }
 
 SourceBufferRange::KeyframeMap::iterator
-SourceBufferRange::GetFirstKeyframeBefore(DecodeTimestamp timestamp) {
+SourceBufferRange::GetFirstKeyframeAtOrBefore(DecodeTimestamp timestamp) {
   KeyframeMap::iterator result = keyframe_map_.lower_bound(timestamp);
   // lower_bound() returns the first element >= |timestamp|, so we want the
   // previous element if it did not return the element exactly equal to
@@ -288,7 +316,7 @@ int SourceBufferRange::GetRemovalGOP(
   BufferQueue::iterator buffer_itr = buffers_.begin() + keyframe_index;
   KeyframeMap::iterator gop_end = keyframe_map_.end();
   if (end_timestamp < GetBufferedEndTimestamp())
-    gop_end = GetFirstKeyframeBefore(end_timestamp);
+    gop_end = GetFirstKeyframeAtOrBefore(end_timestamp);
 
   // Check if the removal range is within a GOP and skip the loop if so.
   // [keyframe]...[start_timestamp]...[end_timestamp]...[keyframe]
@@ -528,7 +556,7 @@ DecodeTimestamp SourceBufferRange::KeyframeBeforeTimestamp(
   if (timestamp < GetStartTimestamp() || timestamp >= GetBufferedEndTimestamp())
     return kNoDecodeTimestamp();
 
-  return GetFirstKeyframeBefore(timestamp)->first;
+  return GetFirstKeyframeAtOrBefore(timestamp)->first;
 }
 
 bool SourceBufferRange::IsNextInSequence(