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::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
   // 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;
       }
       break;
     }
   }
 
   // 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;
-  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 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;
+  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;
   }
 
-  // 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));
+  // 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())
+      return stream.get();
+  }
+
+  // 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();
+  }
+
+  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