FFmpegDemuxer should fall back to disabled streams for seeking

media/filters/ffmpeg_demuxer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/ffmpeg_demuxer.h"
 
 #include <algorithm>
 #include <memory>
 #include <set>
 #include <utility>
@@ skipping 1505 matching lines @@
     }
   }
   params.SetBoolean("found_audio_stream", (audio_track_count > 0));
   params.SetBoolean("found_video_stream", (video_track_count > 0));
   SetTimeProperty(metadata_event.get(), "max_duration", max_duration);
   SetTimeProperty(metadata_event.get(), "start_time", start_time_);
   metadata_event->params.SetInteger("bitrate", bitrate_);
   media_log_->AddEvent(std::move(metadata_event));
 }
 
 FFmpegDemuxerStream* FFmpegDemuxer::FindPreferredStreamForSeeking(

[DaleCurtis, 2016/12/05 22:18:08]

Does this code really need to care about disabled streams? I think it might be
simpler to just remove all the enabled/disabled stream code from here.

[servolk, 2016/12/05 22:24:00]

I think we do need to care about disabled streams here. FFmpeg demuxer has only
a single read position across all streams, so ideally we want to select the
enabled/selected video stream stream for seeking. If there are multiple video
streams in a given file/URL and we use one of the disabled  video streams for
seeking, then seeking might be imprecise for the enabled video stream (e.g. it
might miss the nearest keyframe where we could restart playback after seeking,
and instead we might need to drop data until the next key frame arrives for the
enabled video stream).

     base::TimeDelta seek_time) {
   // If we have a selected/enabled video stream and its start time is lower
   // than the |seek_time| or unknown, then always prefer it for seeking.
   FFmpegDemuxerStream* video_stream = nullptr;
   for (const auto& stream : streams_) {
     if (stream && stream->type() == DemuxerStream::VIDEO && stream->enabled()) {
       video_stream = stream.get();
       if (video_stream->start_time() == kNoTimestamp ||
           video_stream->start_time() <= seek_time) {
         return video_stream;
@@ skipping 13 matching lines @@
       lowest_start_time_stream = 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 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) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   CHECK(!pending_seek_cb_.is_null());
 
   if (!blocking_thread_.IsRunning()) {
     MEDIA_LOG(ERROR, media_log_) << GetDisplayName() << ": bad state";
     base::ResetAndReturn(&pending_seek_cb_).Run(PIPELINE_ERROR_ABORT);
     return;
@@ skipping 210 matching lines @@
 
 void FFmpegDemuxer::SetLiveness(DemuxerStream::Liveness liveness) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   for (const auto& stream : streams_) {
     if (stream)
       stream->SetLiveness(liveness);
   }
 }
 
 }  // namespace media