Use ffmpeg packet.pos for restarting reading after reenabling video

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 849 matching lines @@
       stopped_(false),
       pending_read_(false),
       data_source_(data_source),
       media_log_(media_log),
       bitrate_(0),
       start_time_(kNoTimestamp),
       text_enabled_(false),
       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());
   DCHECK(data_source_);
   DCHECK(!media_tracks_updated_cb_.is_null());
 }
 
 FFmpegDemuxer::~FFmpegDemuxer() {
   // NOTE: This class is not destroyed on |task_runner|, so we must ensure that
   // there are no outstanding WeakPtrs by the time we reach here.
@@ skipping 111 matching lines @@
   } else {
     // Don't use GetWeakPtr() here since we are on the wrong thread.
     task_runner_->PostTask(
         FROM_HERE, base::Bind(&FFmpegDemuxer::AbortPendingReads, weak_this_));
   }
 }
 
 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()));
 }
 
 base::Time FFmpegDemuxer::GetTimelineOffset() const {
   return timeline_offset_;
 }
 
 std::vector<DemuxerStream*> FFmpegDemuxer::GetAllStreams() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   std::vector<DemuxerStream*> result;
@@ skipping 593 matching lines @@
     return;
   }
 
   if (result < 0) {
     // Use VLOG(1) instead of NOTIMPLEMENTED() to prevent the message being
     // captured from stdout and contaminates testing.
     // TODO(scherkus): Implement this properly and signal error (BUG=23447).
     VLOG(1) << "Not implemented";
   }
 
+  last_packet_pos_ = -1;
+
   // Tell streams to flush buffers due to seeking.
   for (const auto& stream : streams_) {
     if (stream)
       stream->FlushBuffers();
   }
 
   // Resume reading until capacity.
   ReadFrameIfNeeded();
 
   // Notify we're finished seeking.
@@ skipping 53 matching lines @@
   for (const auto& stream : streams_) {
     if (stream && stream->type() == DemuxerStream::VIDEO &&
         stream.get() != selected_stream) {
       DVLOG(1) << __func__ << ": disabling stream " << stream.get();
       stream->SetEnabled(false, curr_time);
     }
   }
   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() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // Make sure we have work to do before reading.
   if (stopped_ || !StreamsHaveAvailableCapacity() || pending_read_ ||
       !pending_seek_cb_.is_null()) {
     return;
   }
 
   // Allocate and read an AVPacket from the media. Save |packet_ptr| since
   // evaluation order of packet.get() and base::Passed(&packet) is
   // undefined.
   ScopedAVPacket packet(new AVPacket());
   AVPacket* packet_ptr = packet.get();
 
   pending_read_ = true;
   base::PostTaskAndReplyWithResult(
       blocking_task_runner_.get(), FROM_HERE,
       base::Bind(&av_read_frame, glue_->format_context(), packet_ptr),
       base::Bind(&FFmpegDemuxer::OnReadFrameDone, weak_factory_.GetWeakPtr(),
                  base::Passed(&packet)));
 }
 
 void FFmpegDemuxer::OnReadFrameDone(ScopedAVPacket packet, int result) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(pending_read_);
   pending_read_ = false;
 
   if (stopped_ || !pending_seek_cb_.is_null())

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

probably need to also cancel the read result that arrives while a restart-seek
is in progress

     return;
 
   // Consider the stream as ended if:
   // - either underlying ffmpeg returned an error
   // - or FFMpegDemuxer reached the maximum allowed memory usage.
   if (result < 0 || IsMaxMemoryUsageReached()) {
     if (result < 0) {
       MEDIA_LOG(DEBUG, media_log_)
           << GetDisplayName()
           << ": av_read_frame(): " << AVErrorToString(result);
@@ skipping 35 matching lines @@
     //
     // https://code.google.com/p/chromium/issues/detail?id=169133#c10
     if (!packet->data) {
       ScopedAVPacket new_packet(new AVPacket());
       av_new_packet(new_packet.get(), 0);
       av_packet_copy_props(new_packet.get(), packet.get());
       packet.swap(new_packet);
     }
 
     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.
     if (duration_known_) {
       const base::TimeDelta duration = demuxer_stream->duration();
       if (duration != kNoTimestamp && duration > duration_) {
         duration_ = duration;
         host_->SetDuration(duration_);
       }
     }
@@ skipping 51 matching lines @@
 }
 
 void FFmpegDemuxer::SetLiveness(DemuxerStream::Liveness liveness) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   for (const auto& stream : streams_) {
     if (stream)
       stream->SetLiveness(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