MSE: More cleanly handle stall resolution edge cases

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 05f23379ddcd07a9cf365ba6d8653ecedd2ecd62..2f90bd74ea787bc4778035c97f99379e1a8a2451 100644
--- a/media/filters/source_buffer_stream.cc
+++ b/media/filters/source_buffer_stream.cc
@@ -1600,68 +1600,33 @@ DecodeTimestamp SourceBufferStream::FindNewSelectedRangeSeekTimestamp(
 
   RangeList::iterator itr = ranges_.begin();
 
+  // When checking a range to see if it has or begins soon enough after
+  // |start_timestamp|, use the fudge room to determine "soon enough".
+  DecodeTimestamp start_timestamp_plus_fudge =
+      start_timestamp + ComputeFudgeRoom(GetMaxInterbufferDistance());
+
+  // Multiple ranges could be within the fudge room, because the fudge room is
+  // dynamic based on max inter-buffer distance seen so far. Optimistically
+  // check the earliest ones first.
   for (; itr != ranges_.end(); ++itr) {
-    if ((*itr)->GetEndTimestamp() >= start_timestamp) {
+    DecodeTimestamp range_start = (*itr)->GetStartTimestamp();
+    if (range_start >= start_timestamp_plus_fudge)

[chcunningham, 2016/04/20 20:14:27]

Do you want this comparison to be GE vs GT? IIUC GE is probably right - we
typically consider a range's end timestamp to not be part of the range...and in
a since start_timestamp_plus_fudge is the *end timestamp* of the fudge space.
Just want to confirm my reasoning.

[wolenetz, 2016/04/20 21:28:01]

Hmm. I had based this on the previous logic in l1652.
For all the "kNoDecodeTimestamp()" arg of SetSelectedRangeIfNeeded, only if
there is no track_buffer_ or selected_range_, and only if there has been a
buffer output since the last seek, then that last output buffer's DTS + 1
microsecond is passed as |start_timestamp| to
FindNewSelectedRangeSeekTimestamp().
Search termination on GE in this case is correct IMO (there's no range starting
at or before last_output_buffer_timestamp_ + fudge_room.

However, we have some pre-existing path(s) with SetSelectedRangeIfNeeded()
called with a non-kNoDecodeTimestamp(), and with no track_buffer_ or
selected_range_, where that timestamp is used without the +1us adjustment in
this method. I think those cases have greater questions such as "should the
fudge room apply to the DTS of a previously next-buffer, but now removed?
(Remove() case, specifically)" Out of scope of this CL; I've filed a crbug to
track that investigation (https://crbug.com/605274)

       break;
+    if ((*itr)->GetEndTimestamp() < start_timestamp)
+      continue;
+    DecodeTimestamp search_timestamp = start_timestamp;
+    if (start_timestamp < range_start &&
+        start_timestamp_plus_fudge >= range_start) {
+      search_timestamp = range_start;
     }
+    DecodeTimestamp keyframe_timestamp =
+        (*itr)->NextKeyframeTimestamp(search_timestamp);
+    if (keyframe_timestamp != kNoDecodeTimestamp())
+      return keyframe_timestamp;
   }
 
-  if (itr == ranges_.end()) {
-    DVLOG(2) << __FUNCTION__ << " " << GetStreamTypeName()
-             << " no buffered data for dts=" << start_timestamp.InSecondsF();
-    return kNoDecodeTimestamp();
-  }
-
-  // First check for a keyframe timestamp >= |start_timestamp|
-  // in the current range.
-  DecodeTimestamp keyframe_timestamp =
-      (*itr)->NextKeyframeTimestamp(start_timestamp);
-
-  if (keyframe_timestamp != kNoDecodeTimestamp())
-    return keyframe_timestamp;
-
-  // If a keyframe was not found then look for a keyframe that is
-  // "close enough" in the current or next range.
-  DecodeTimestamp end_timestamp =
-      start_timestamp + ComputeFudgeRoom(GetMaxInterbufferDistance());
-  DCHECK(start_timestamp < end_timestamp);
-
-  // Make sure the current range doesn't start beyond |end_timestamp|.
-  if ((*itr)->GetStartTimestamp() >= end_timestamp)
-    return kNoDecodeTimestamp();
-
-  keyframe_timestamp = (*itr)->KeyframeBeforeTimestamp(end_timestamp);
-
-  // Check to see if the keyframe is within the acceptable range
-  // (|start_timestamp|, |end_timestamp|].
-  if (keyframe_timestamp != kNoDecodeTimestamp() &&
-      start_timestamp < keyframe_timestamp  &&
-      keyframe_timestamp <= end_timestamp) {
-    return keyframe_timestamp;
-  }
-
-  // If |end_timestamp| is within this range, then no other checks are
-  // necessary.
-  if (end_timestamp <= (*itr)->GetEndTimestamp())
-    return kNoDecodeTimestamp();
-
-  // Move on to the next range.
-  ++itr;
-
-  // Return early if the next range does not contain |end_timestamp|.
-  if (itr == ranges_.end() || (*itr)->GetStartTimestamp() >= end_timestamp)
-    return kNoDecodeTimestamp();
-
-  keyframe_timestamp = (*itr)->KeyframeBeforeTimestamp(end_timestamp);
-
-  // Check to see if the keyframe is within the acceptable range
-  // (|start_timestamp|, |end_timestamp|].
-  if (keyframe_timestamp != kNoDecodeTimestamp() &&
-      start_timestamp < keyframe_timestamp  &&
-      keyframe_timestamp <= end_timestamp) {
-    return keyframe_timestamp;
-  }
-
+  DVLOG(2) << __FUNCTION__ << " " << GetStreamTypeName()
+           << " no buffered data for dts=" << start_timestamp.InSecondsF();
   return kNoDecodeTimestamp();
 }