FFmpegDemuxer should fall back to disabled streams for seeking

media/filters/ffmpeg_demuxer.cc

Index: media/filters/ffmpeg_demuxer.cc
diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc
index 093e4f539b13847a1f7f6beb0e140674af037f69..871e04f6bd22fff7591362f6da9de6f60ab1daa0 100644
--- a/media/filters/ffmpeg_demuxer.cc
+++ b/media/filters/ffmpeg_demuxer.cc
@@ -1523,6 +1523,21 @@ void FFmpegDemuxer::LogMetadata(AVFormatContext* avctx,
   media_log_->AddEvent(std::move(metadata_event));
 }
 
+FFmpegDemuxerStream* FFmpegDemuxer::FindStreamWithLowestStartPts(bool enabled) {
+  FFmpegDemuxerStream* lowest_start_time_stream = nullptr;
+  for (const auto& stream : streams_) {
+    if (!stream || stream->start_time() == kNoTimestamp)
+      continue;
+    if (stream->enabled() != enabled)
+      continue;
+    if (!lowest_start_time_stream ||
+        stream->start_time() < lowest_start_time_stream->start_time()) {
+      lowest_start_time_stream = stream.get();
+    }
+  }
+  return lowest_start_time_stream;
+}
+
 FFmpegDemuxerStream* FFmpegDemuxer::FindPreferredStreamForSeeking(
     base::TimeDelta seek_time) {
   // If we have a selected/enabled video stream and its start time is lower
@@ -1541,25 +1556,36 @@ FFmpegDemuxerStream* FFmpegDemuxer::FindPreferredStreamForSeeking(
 
   // If video stream is not present or |seek_time| is lower than the video start
   // time, then try to find an enabled stream with the lowest start time.
-  FFmpegDemuxerStream* lowest_start_time_stream = nullptr;
+  FFmpegDemuxerStream* lowest_start_time_enabled_stream =
+      FindStreamWithLowestStartPts(true);
+  if (lowest_start_time_enabled_stream &&
+      lowest_start_time_enabled_stream->start_time() <= seek_time) {
+    return lowest_start_time_enabled_stream;
+  }
+
+  // Fall back to any enabled stream, even if it's start time is unknown.

[servolk, 2016/12/06 18:14:17]

TBH I'm not 100% sure about this. Should we prefer an enabled stream with an
unknown (kNoTimestamp) start time over a disabled stream with a known start
time? I believe we should always prefer enabled streams whenever possible and
having this logic allows us to exactly preserve the old behavior tested by
FFmpegDemuxerTest::Seeking_PreferredStreamSelection. But if e.g. FFmpeg handles
seeking better for streams with known start times, then we can reconsider this.

[DaleCurtis, 2016/12/06 20:18:03]

We shouldn't have any streams with a start time of kNoTimestamp. I think that's
a bug. Today we make the overall start time 0 if no stream has a true start
timestamp. I wonder if we should propagate 0 to all streams with an unknown
start_time_. From a cursory look the only code that cares if it's NoTimestamp is
this code.

[servolk, 2016/12/06 22:05:29]

Are you sure about this? Looking at comment
https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481...
and ExtractStartTime function at
https://cs.chromium.org/chromium/src/media/filters/ffmpeg_demuxer.cc?rcl=1481...
it seems to me like it is possible to have streams with unknown start times
(kNoTimestamp and not a 0 pts).
IIUC if some stream is muxed in such a way that no single packet from it is read
during avformat_find_stream_info, we won't know the start time for it.
When I was refactoring the seeking stream selection in
https://codereview.chromium.org/2281843002 I was trying to carefully preserve
the original logic that we had around handling streams with no start times.

[DaleCurtis, 2016/12/06 22:17:09]

We never correct the start time if this is true, so the stream forever lingers
with kNoTimestamp and the demuxer always marks the start time as zero in this
case.

[servolk, 2016/12/06 23:00:41]

Ok, so in other words this means that it's the right choice here to prefer
enabled streams with an unknown / zero start time for seeking over disabled
streams with known start time, right?
Or are you saying that we should explicitly set start time to 0 for demuxer
streams whose start time could not be determined reliably in
FFmpegDemuxer::OnFindStreamInfoDone and then just DCHECK here and elsewhere that
stream start time is never kNoTimestamp?

[DaleCurtis, 2016/12/06 23:29:05]

I think we can set the start_time to zero when it's unknown. Do you see any
instances other than this code that cares about the kNoTimestamp value for
start_time?

[servolk, 2016/12/06 23:43:34]

You are right, looks like FFmpegDemuxerStream::start_time() is only used  here,
so we can experiment with using 0 instead of kNoTimestamp in ExtractStartTime
and it shouldn't affect anything else, I'll give it a try.

   for (const auto& stream : streams_) {
-    if (!stream || !stream->enabled() || stream->start_time() == kNoTimestamp)
-      continue;
-    if (!lowest_start_time_stream ||
-        stream->start_time() < lowest_start_time_stream->start_time()) {
-      lowest_start_time_stream = stream.get();
-    }
+    if (stream && stream->enabled())
+      return stream.get();
   }
-  // If we found a stream with start time lower than |seek_time|, then use it.
-  if (lowest_start_time_stream &&
-      lowest_start_time_stream->start_time() <= seek_time) {
-    return lowest_start_time_stream;
+
+  // If there's no enabled streams to consider from, try a disabled stream with
+  // the lowest known start time.
+  FFmpegDemuxerStream* lowest_start_time_disabled_stream =
+      FindStreamWithLowestStartPts(false);
+  if (lowest_start_time_disabled_stream &&
+      lowest_start_time_disabled_stream->start_time() <= seek_time) {
+    return lowest_start_time_disabled_stream;
+  }
+
+  // Otherwise fall back to any other stream.
+  for (const auto& stream : streams_) {
+    if (stream)
+      return stream.get();
   }
 
-  // If we couldn't find any streams with the start time lower than |seek_time|
-  // then use either video (if one exists) or any audio stream.
-  return video_stream ? video_stream : static_cast<FFmpegDemuxerStream*>(
-                                           GetStream(DemuxerStream::AUDIO));
+  NOTREACHED();
+  return nullptr;
 }
 
 void FFmpegDemuxer::OnSeekFrameDone(int result) {