Use ffmpeg packet.pos for restarting reading after reenabling video

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 081bcbf1a452be23ee45adb6a080e11bd5b55c77..64777991f274418fab5c14072462c7a2f9c392ed 100644
--- a/media/filters/ffmpeg_demuxer.cc
+++ b/media/filters/ffmpeg_demuxer.cc
@@ -867,6 +867,8 @@ FFmpegDemuxer::FFmpegDemuxer(
       duration_known_(false),
       encrypted_media_init_data_cb_(encrypted_media_init_data_cb),
       media_tracks_updated_cb_(media_tracks_updated_cb),
+      last_packet_pos_(-1),
+      restarting_stream_(nullptr),
       cancel_pending_seek_factory_(this),
       weak_factory_(this) {
   DCHECK(task_runner_.get());
@@ -998,50 +1000,9 @@ void FFmpegDemuxer::CancelPendingSeek(base::TimeDelta seek_time) {
 void FFmpegDemuxer::Seek(base::TimeDelta time, const PipelineStatusCB& cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   CHECK(pending_seek_cb_.is_null());
-
-  // FFmpeg requires seeks to be adjusted according to the lowest starting time.
-  // Since EnqueuePacket() rebased negative timestamps by the start time, we
-  // must correct the shift here.
-  //
-  // Additionally, to workaround limitations in how we expose seekable ranges to
-  // Blink (http://crbug.com/137275), we also want to clamp seeks before the
-  // start time to the start time.
-  base::TimeDelta seek_time = start_time_ < base::TimeDelta()
-                                  ? time + start_time_
-                                  : time < start_time_ ? start_time_ : time;
-
-  // When seeking in an opus stream we need to ensure we deliver enough data to
-  // satisfy the seek preroll; otherwise the audio at the actual seek time will
-  // not be entirely accurate.
-  FFmpegDemuxerStream* audio_stream =
-      GetFirstEnabledFFmpegStream(DemuxerStream::AUDIO);
-  if (audio_stream) {
-    const AudioDecoderConfig& config = audio_stream->audio_decoder_config();
-    if (config.codec() == kCodecOpus)
-      seek_time = std::max(start_time_, seek_time - config.seek_preroll());
-  }
-
-  // Choose the seeking stream based on whether it contains the seek time, if no
-  // match can be found prefer the preferred stream.
-  //
-  // TODO(dalecurtis): Currently FFmpeg does not ensure that all streams in a
-  // given container will demux all packets after the seek point.  Instead it
-  // only guarantees that all packets after the file position of the seek will
-  // be demuxed.  It's an open question whether FFmpeg should fix this:
-  // http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2014-June/159212.html
-  // Tracked by http://crbug.com/387996.
-  FFmpegDemuxerStream* demux_stream = FindPreferredStreamForSeeking(seek_time);
-  DCHECK(demux_stream);
-  const AVStream* seeking_stream = demux_stream->av_stream();
-  DCHECK(seeking_stream);
-
   pending_seek_cb_ = cb;
-  base::PostTaskAndReplyWithResult(
-      blocking_task_runner_.get(), FROM_HERE,
-      base::Bind(&av_seek_frame, glue_->format_context(), seeking_stream->index,
-                 ConvertToTimeBase(seeking_stream->time_base, seek_time),
-                 // Always seek to a timestamp <= to the desired timestamp.
-                 AVSEEK_FLAG_BACKWARD),
+  SeekInternal(
+      time, nullptr,
       base::Bind(&FFmpegDemuxer::OnSeekFrameDone, weak_factory_.GetWeakPtr()));
 }
 
@@ -1655,6 +1616,8 @@ void FFmpegDemuxer::OnSeekFrameDone(int result) {
     VLOG(1) << "Not implemented";
   }
 
+  last_packet_pos_ = -1;
+
   // Tell streams to flush buffers due to seeking.
   for (const auto& stream : streams_) {
     if (stream)
@@ -1728,7 +1691,25 @@ void FFmpegDemuxer::OnSelectedVideoTrackChanged(
   if (selected_stream) {
     DVLOG(1) << __func__ << ": enabling stream " << selected_stream;
     selected_stream->SetEnabled(true, curr_time);
+    selected_stream->FlushBuffers();
+    SeekInternal(curr_time, selected_stream,

[wolenetz, 2017/05/08 20:55:33]

could we already be in the middle of either a normal seek or another
deselect->reselect video track seek? If so, it seems this needs to protected
against (no more that 1 seek of any kind should be allowed to be in progress
against the ffmpeg API).

[servolk, 2017/05/12 00:55:37]

Yeah, I looked closer at how FFmpegDemuxer is called and looks like it's
possible, which is unfortunate, since we'll need to make the logic here more
complicated to handle this. I think we need to consider two cases here:
1. A track is re-enabled while we have a pending seek in progress. In this case
we'll need to re-start the pending seek to the same position immediately after
it completes, since the newly enabled stream will have its avdiscard flags
updated and the actual seek target position in the input stream might change.
2. If seek is initiated while the video stream restart is in progress (i.e. the
|restarting_stream_| is not null and we are still reading past data for the
video stream). In this case I believe we only need to reset the
restarting_stream_ to null and proceed with the seek. The reading is guaranteed
to restart from the correct file position for the new seek_target, because by
that point the avdiscard flags for the re-enabled streams are updated.

I'll see if I can find a way to add new test cases for these.

+                 base::Bind(&FFmpegDemuxer::OnSeekDoneForRestartingStream,
+                            weak_factory_.GetWeakPtr(), selected_stream));
+  }
+}
+
+void FFmpegDemuxer::OnSeekDoneForRestartingStream(FFmpegDemuxerStream* stream,

[wolenetz, 2017/05/08 20:55:33]

What if ::Stop() occurs while this seek was in progress? Also, what if this
FFmpegDemuxer is destructed prior to this callback occurring? There's quite a
bit of machinery already in the normal ::Seek() process to handle such problems;
similar is likely necessary for these "restart" seeks. Would re-using the same
machinery be feasible?

[servolk, 2017/05/12 00:55:37]

I believe the destruction of FFmpegDemuxer should be ok due to using weak_ptrs
here (any pending callbacks will just be canceled).
Re. ::Stop - that's a good question, but I not sure what machinery you are
referring to, could you point me to it please? I don't see pending_seek_cb_
being checked in FFmpegDemuxerStream::Stop. Should there be at least a
DCHECK(!pending_seek_cb_) perhaps?

+                                                  int result) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  if (result < 0) {
+    DVLOG(1) << __func__ << ": seek failed: " << AVErrorToString(result);

[sandersd (OOO until July 31), 2017/05/10 22:27:49]

Looks like this should probably be logged to the media log?

[servolk, 2017/05/12 00:55:37]

Done.

+    return;
   }
+  DVLOG(2) << __func__
+           << ": will drop packets until last_packet_pos_=" << last_packet_pos_;
+  if (restarting_stream_)
+    restarting_stream_->FlushBuffers();
+  restarting_stream_ = stream;
 }
 
 void FFmpegDemuxer::ReadFrameIfNeeded() {
@@ -1815,7 +1796,26 @@ void FFmpegDemuxer::OnReadFrameDone(ScopedAVPacket packet, int result) {
     }
 
     FFmpegDemuxerStream* demuxer_stream = streams_[packet->stream_index].get();
-    if (demuxer_stream->IsEnabled())
+
+    // If |restarting_stream_| is not null, we are in stream restart mode, which
+    // means we seeked back the ffmpeg reading position and now we need to drop
+    // packets from other streams until we reach the previously seen read
+    // position |last_packet_pos_|.
+    bool drop_seen_packet =
+        restarting_stream_ && demuxer_stream != restarting_stream_ &&
+        last_packet_pos_ >= 0 && packet.get()->pos <= last_packet_pos_;

[wolenetz, 2017/05/08 20:55:33]

Is pos non-decreasing (during normal sequence of reads, and can only go
backwards on seek), and on the same dimension across all streams (do all streams
share a common pos?)

[servolk, 2017/05/12 00:55:37]

IIUC the answer is yes to both questions.
AVPacket::pos is the byte position of the AVPacket in stream (see
https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavcodec/avcodec.h?...).
AFAIK FFmpeg reads packets from the stream sequentially in their bytestream
order. That's why we chose to use it here (I'll add you to the mail thread where
we discussed this with Dale).

+    if (drop_seen_packet) {
+      DVLOG(4) << "Dropping already seen packet packet: pos="
+               << packet.get()->pos;
+    } else if (restarting_stream_ && demuxer_stream != restarting_stream_) {
+      DVLOG(2) << "Restarting reading packets: pos=" << packet.get()->pos;
+      restarting_stream_ = nullptr;
+    }
+
+    if (!restarting_stream_)
+      last_packet_pos_ = packet.get()->pos;
+
+    if (demuxer_stream->IsEnabled() && !drop_seen_packet)
       demuxer_stream->EnqueuePacket(std::move(packet));
 
     // If duration estimate was incorrect, update it and tell higher layers.
@@ -1887,4 +1887,56 @@ void FFmpegDemuxer::SetLiveness(DemuxerStream::Liveness liveness) {
   }
 }
 
+void FFmpegDemuxer::SeekInternal(base::TimeDelta time,
+                                 FFmpegDemuxerStream* preferred_stream,
+                                 const FFmpegSeekDoneCB& ffmpeg_seek_done_cb) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  // FFmpeg requires seeks to be adjusted according to the lowest starting time.
+  // Since EnqueuePacket() rebased negative timestamps by the start time, we
+  // must correct the shift here.
+  //
+  // Additionally, to workaround limitations in how we expose seekable ranges to
+  // Blink (http://crbug.com/137275), we also want to clamp seeks before the
+  // start time to the start time.
+  base::TimeDelta seek_time = start_time_ < base::TimeDelta()
+                                  ? time + start_time_
+                                  : time < start_time_ ? start_time_ : time;
+
+  // When seeking in an opus stream we need to ensure we deliver enough data to
+  // satisfy the seek preroll; otherwise the audio at the actual seek time will
+  // not be entirely accurate.
+  FFmpegDemuxerStream* audio_stream =
+      GetFirstEnabledFFmpegStream(DemuxerStream::AUDIO);
+  if (audio_stream) {
+    const AudioDecoderConfig& config = audio_stream->audio_decoder_config();
+    if (config.codec() == kCodecOpus)
+      seek_time = std::max(start_time_, seek_time - config.seek_preroll());
+  }
+
+  if (!preferred_stream) {
+    // Choose the seeking stream based on whether it contains the seek time, if
+    // no match can be found prefer the preferred stream.
+    //
+    // TODO(dalecurtis): Currently FFmpeg does not ensure that all streams in a
+    // given container will demux all packets after the seek point.  Instead it
+    // only guarantees that all packets after the file position of the seek will
+    // be demuxed.  It's an open question whether FFmpeg should fix this:
+    // http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2014-June/159212.html
+    // Tracked by http://crbug.com/387996.
+    preferred_stream = FindPreferredStreamForSeeking(seek_time);
+    DCHECK(preferred_stream);
+  }
+  const AVStream* ffmpeg_stream = preferred_stream->av_stream();
+  DCHECK(ffmpeg_stream);
+
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner_.get(), FROM_HERE,
+      base::Bind(&av_seek_frame, glue_->format_context(), ffmpeg_stream->index,
+                 ConvertToTimeBase(ffmpeg_stream->time_base, seek_time),
+                 // Always seek to a timestamp <= to the desired timestamp.
+                 AVSEEK_FLAG_BACKWARD),
+      ffmpeg_seek_done_cb);
+}
+
 }  // namespace media