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..238c31eeb6a53c3b3cf0b3977ec91a01ec1b482a 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());
@@ -1655,6 +1657,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,9 +1732,38 @@ void FFmpegDemuxer::OnSelectedVideoTrackChanged(
   if (selected_stream) {
     DVLOG(1) << __func__ << ": enabling stream " << selected_stream;
     selected_stream->SetEnabled(true, curr_time);
+    selected_stream->FlushBuffers();
+
+    base::TimeDelta seek_time =

[DaleCurtis, 2017/05/04 18:53:03]

Extract common code from Seek() and put in an internal helper which we can use
here. Perhaps it can take a done_cb to reuse almost all the code too.

[servolk, 2017/05/04 19:15:16]

Will do.

[servolk, 2017/05/04 22:13:16]

Done (I've extracted most of the common seeking logic into SeekInternal in
patchset #3).

+        start_time_ < base::TimeDelta()
+            ? curr_time + start_time_
+            : curr_time < start_time_ ? start_time_ : curr_time;
+
+    base::PostTaskAndReplyWithResult(
+        blocking_task_runner_.get(), FROM_HERE,
+        base::Bind(&av_seek_frame, glue_->format_context(),
+                   selected_stream->av_stream()->index,
+                   ConvertToTimeBase(selected_stream->av_stream()->time_base,
+                                     seek_time),
+                   // Always seek to a timestamp <= to the desired timestamp.
+                   AVSEEK_FLAG_BACKWARD),
+        base::Bind(&FFmpegDemuxer::OnSeekDoneForRestartingStream,
+                   weak_factory_.GetWeakPtr(), selected_stream));
   }
 }
 
+void FFmpegDemuxer::OnSeekDoneForRestartingStream(FFmpegDemuxerStream* stream,
+                                                  int result) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  if (result < 0) {
+    DVLOG(1) << __func__ << ": seek failed: " << AVErrorToString(result);
+    return;
+  }
+  DVLOG(2) << __func__
+           << ": will drop packets until last_packet_pos_=" << last_packet_pos_;
+  restarting_stream_ = stream;

[DaleCurtis, 2017/05/04 18:53:03]

What happens if another track is enabled and tramples restarting_stream_ before
we finish the track change? Are we guaranteed this won't happen?

[servolk, 2017/05/04 19:15:16]

Great question. I think it could happen, but we should be fine. If we got here
when the restarting_stream_ is non-null, it means that we are still in the
'stream restart' mode, i.e. we are reading packets for only one stream (the
current restarting_stream_) and dropping packets for other streams. We'll simply
remain in the stream restart mode, but now we'll be reading only packets for the
new |stream|. if stream == restarting_stream_, then it means we've already done
FlushBuffers in the OnSelectedVideoStream changed. Otherwise I guess there might
be some packets left in the previous restarting stream, but it shouldn't
interfere with anything, only slightly affect memory usage. I guess we can do
restarting_stream_->FlushBuffers (if restarting_stream_ is non-null already)
here before overwriting it, to make a small improvement. But it shouldn't affect
playback behavior.

[DaleCurtis, 2017/05/04 21:21:06]

Isn't that wrong though? I.e. the stream we restarted first may have 0 or more
packets, so we need to deliver all packets to that stream, but only
restarting_stream gets the special logic to ensure it gets all packets.

[servolk, 2017/05/04 21:38:30]

I think that's ok, because we only use this logic for video streams and there
can be only one selected video stream at a time. So we need to handle only two
cases:
1. Video stream A was deselected (disabled), then re-enabled, at that point
we'll enter the restarting mode and will start reading packets for it, then the
same stream A gets disabled and re-enabled again before we finished handling the
first operation. In this case we should be fine, because we did FlushBuffers on
stream A when it got re-enabled the second time, and now we've just done a seek
to a new restart position and will resume reading more frames for stream A from
the new restart position. There's no need to deliver old frames as we never
finished the first restart, and they would have been dropped by the video
renderer anyway (because it also does a flush every time a stream is restarted).
2. If we have more than one video stream, let's say we have video streams A and
B. First the stream A gets re-enabled and we start buffering data for it. But
then the selected video stream is changed to B. Now we don't need to finish
delivering any read packets for A, because A is automatically
deselected/disabled when the video stream B is selected.

[DaleCurtis, 2017/05/04 21:41:12]

Even if we just consider audio, depending on how the file is muxed if you drop
packets like this you are not guaranteed to have audio for the time you need.
I.e. with a bad muxing the audio packets you may need for the video time T are
before last packet pos and thus get dropped.

[DaleCurtis, 2017/05/04 21:46:12]

Oh right I see what you mean, we only run the seek on video so should be okay on
audio packets since their buffers are guaranteed to contain the current time.
This is a lot of assumptions that need to be documented and have tests / DCHECKS
present.

+}
+
 void FFmpegDemuxer::ReadFrameIfNeeded() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
@@ -1815,7 +1848,25 @@ 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 =

[DaleCurtis, 2017/05/04 18:53:03]

Drop unnecessary parens.

[servolk, 2017/05/04 19:15:16]

Done.

+        (restarting_stream_ && demuxer_stream != restarting_stream_ &&
+         last_packet_pos_ >= 0 && packet.get()->pos <= last_packet_pos_);

[DaleCurtis, 2017/05/04 18:53:03]

Is pos monotonically increasing? I.e. this won't be changed by badly muxed
files?

[servolk, 2017/05/04 19:15:15]

I don't know for sure, but I think it should be monotonically increasing even
for badly muxed files. IIUC timestamps (pts) might be all over the place for
badly muxed files, but file read position will still increase monotonically.

+    if (drop_seen_packet) {
+      DVLOG(4) << "Dropping packet: pos=" << packet.get()->pos;

[DaleCurtis, 2017/05/04 18:53:03]

"already seen packet"

[servolk, 2017/05/04 19:15:16]

Done.

+    } 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.