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..f2fcd112c8bb68dafaa0e677c33e954d2a3a13e9 100644
--- a/media/filters/ffmpeg_demuxer.cc
+++ b/media/filters/ffmpeg_demuxer.cc
@@ -1557,9 +1557,24 @@ FFmpegDemuxerStream* FFmpegDemuxer::FindPreferredStreamForSeeking(
   }
 
   // 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));
+  // then use either video (if one exists) or any enabled audio stream.
+  if (video_stream)
+    return video_stream;
+
+  for (const auto& stream : streams_) {
+    if (stream && stream->type() == DemuxerStream::AUDIO && stream->enabled())
+      return stream.get();
+  }
+
+  // If there's no enabled audio/video streams, then just fall back to any

[DaleCurtis, 2016/12/05 23:11:27]

Seems like instead this code should loop the whole process again with
ignore_disabled_streams or something instead of trying to add a new possible
path. 

[servolk, 2016/12/06 00:36:05]

I agree that it would be nice and more consistent to have similar code structure
for handling the two cases (enabled and non-enabled streams), but on the other
hand those two cases are meaningfully different so repeating the same steps
while disregarding the |enabled| status is not guaranteed to give us the best
result.
Upon thinking about this some more I believe the best course of action is to:
1. If there's an enabled video stream with start_pts <= seek target, then we
should always prefer that (since only video streams have key frames and thus are
more sensitive to the actual seek position, and there can be only one enabled
video stream per HTML5MediaElement).
2. If step #1 didn't yield the result, then we should try to find the stream
with lowest start pts among enabled audio/video streams (this is essentially
what lines 1542-1557 are currently doing).
3. But then, if step #2 didn't yield a result, we should repeat only step #2
again while disregarding the enabled status. I.e. we should try again to find a
stream with the lowest start pts this time among the disabled streams.

IMHO there's no point in repeating step #1 if there's no enabled video stream,
since disabled video streams may be read from, but that data will be discarded
and so we don't need to be concerned about key frames for disabled video
streams.
I'll do a bit more refactoring in the next patchset to extract the logic for
finding stream with lowest start pts among enabled/disabled streams.

+  // non-null stream, even if it's disabled.
+  for (const auto& stream : streams_) {
+    if (stream)
+      return stream.get();
+  }
+
+  NOTREACHED();
+  return nullptr;
 }
 
 void FFmpegDemuxer::OnSeekFrameDone(int result) {