Flush and seek after all track changes

FFmpeg demuxer needs to flush all the tracks after a track change or
else it will start trying to enqueue frames on stopped streams. The seek
can also be made optional if a track change event fires but no tracks
were actually changed.

This also removes SeekOnVideoTrackChangeWontSeekIfEmpty, because it
doesn't actually test anything.

Bug: 371024058
Change-Id: Ib6c82be0029af8f7c1def1e0a5c806b9b1dce667
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6300094
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Ted (Chromium) Meyer <tmathmeyer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1424278}

media/filters/ffmpeg_demuxer.cc

@@ -1767,6 +1767,8 @@ void FFmpegDemuxer::FindAndEnableProperTracks(
     TrackChangeCB change_completed_cb) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
+  bool any_track_changed = false;
+
   std::set<FFmpegDemuxerStream*> enabled_streams;
   for (const auto& id : track_ids) {
     auto it = track_id_to_demux_stream_map_.find(id);
@@ -1782,6 +1784,9 @@ void FFmpegDemuxer::FindAndEnableProperTracks(
       continue;
     }
     enabled_streams.insert(stream);
+    if (!stream->IsEnabled()) {
+      any_track_changed = true;
+    }
     stream->SetEnabled(true, curr_time);
   }
 
@@ -1791,13 +1796,35 @@ void FFmpegDemuxer::FindAndEnableProperTracks(
     if (stream && stream->type() == track_type &&
         enabled_streams.find(stream.get()) == enabled_streams.end()) {
       DVLOG(1) << __func__ << ": disabling stream " << stream.get();
+      if (stream->IsEnabled()) {
+        any_track_changed = true;
+      }
       stream->SetEnabled(false, curr_time);
     }
   }
 
   std::vector<DemuxerStream*> streams(enabled_streams.begin(),
                                       enabled_streams.end());
-  std::move(change_completed_cb).Run(streams);
+  base::OnceCallback<void(int)> seek_cb = base::BindOnce(
+      &FFmpegDemuxer::OnTrackChangeSeekComplete, weak_factory_.GetWeakPtr(),
+      base::BindOnce(std::move(change_completed_cb), std::move(streams)));
+
+  if (any_track_changed) {
+    SeekInternal(curr_time, std::move(seek_cb));
+  } else {
+    std::move(seek_cb).Run(0);
+  }
+}
+
+void FFmpegDemuxer::OnTrackChangeSeekComplete(base::OnceClosure cb,
+                                              int seek_status) {
+  for (const auto& stream : streams_) {
+    if (stream && stream->IsEnabled()) {
+      stream->FlushBuffers(true);
+    }
+  }
+  // TODO(crbug.com/40898124): Report seek failures for track changes too.
+  std::move(cb).Run();
 }
 
 void FFmpegDemuxer::OnEnabledAudioTracksChanged(
@@ -1808,49 +1835,12 @@ void FFmpegDemuxer::OnEnabledAudioTracksChanged(
                             std::move(change_completed_cb));
 }
 
-void FFmpegDemuxer::OnVideoSeekedForTrackChange(
-    DemuxerStream* video_stream,
-    base::OnceClosure seek_completed_cb,
-    int result) {
-  for (const auto& stream : streams_) {
-    if (stream && stream->IsEnabled()) {
-      stream->FlushBuffers(true);
-    }
-  }
-  // TODO(crbug.com/40898124): Report seek failures for track changes too.
-  std::move(seek_completed_cb).Run();
-}
-
-void FFmpegDemuxer::SeekOnVideoTrackChange(
-    base::TimeDelta seek_to_time,
-    TrackChangeCB seek_completed_cb,
-    const std::vector<DemuxerStream*>& streams) {
-  if (streams.size() != 1u) {
-    // If FFmpegDemuxer::FindAndEnableProperTracks() was not able to find the
-    // selected streams in the ID->DemuxerStream map, then its possible for
-    // this vector to be empty. If that's the case, we don't want to bother
-    // with seeking, and just call the callback immediately.
-    std::move(seek_completed_cb).Run(streams);
-    return;
-  }
-  SeekInternal(
-      seek_to_time,
-      base::BindOnce(&FFmpegDemuxer::OnVideoSeekedForTrackChange,
-                     weak_factory_.GetWeakPtr(), streams[0],
-                     base::BindOnce(std::move(seek_completed_cb), streams)));
-}
-
 void FFmpegDemuxer::OnSelectedVideoTrackChanged(
     const std::vector<MediaTrack::Id>& track_ids,
     base::TimeDelta curr_time,
     TrackChangeCB change_completed_cb) {
-  // Find tracks -> Seek track -> run callback.
-  FindAndEnableProperTracks(
-      track_ids, curr_time, DemuxerStream::VIDEO,
-      track_ids.empty() ? std::move(change_completed_cb)
-                        : base::BindOnce(&FFmpegDemuxer::SeekOnVideoTrackChange,
-                                         weak_factory_.GetWeakPtr(), curr_time,
-                                         std::move(change_completed_cb)));
+  FindAndEnableProperTracks(track_ids, curr_time, DemuxerStream::VIDEO,
+                            std::move(change_completed_cb));
 }
 
 void FFmpegDemuxer::ReadFrameIfNeeded() {

media/filters/ffmpeg_demuxer.h

@@ -330,12 +330,8 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
 
   void SeekInternal(base::TimeDelta time,
                     base::OnceCallback<void(int)> seek_cb);
-  void OnVideoSeekedForTrackChange(DemuxerStream* video_stream,
-                                   base::OnceClosure seek_completed_cb,
-                                   int result);
-  void SeekOnVideoTrackChange(base::TimeDelta seek_to_time,
-                              TrackChangeCB seek_completed_cb,
-                              const std::vector<DemuxerStream*>& streams);
+  void OnTrackChangeSeekComplete(base::OnceClosure seek_completed_cb,
+                                 int result);
 
   // Executes |init_cb_| with |status| and closes out the async trace.
   void RunInitCB(PipelineStatus status);

media/filters/ffmpeg_demuxer_unittest.cc

@@ -181,15 +181,6 @@ class FFmpegDemuxerTest : public testing::Test {
     InitializeDemuxerInternal(expected_pipeline_status, base::Time());
   }
 
-  void SeekOnVideoTrackChangePassthrough(
-      base::TimeDelta time,
-      base::OnceCallback<void(const std::vector<DemuxerStream*>&)> cb,
-      const std::vector<DemuxerStream*>& streams) {
-    // The tests can't access private methods directly because gtest uses
-    // some magic macros that break the 'friend' declaration.
-    demuxer_->SeekOnVideoTrackChange(time, std::move(cb), streams);
-  }
-
   MOCK_METHOD2(OnReadDoneCalled, void(int, int64_t));
 
   struct ReadExpectation {
@@ -1777,17 +1768,4 @@ TEST_F(FFmpegDemuxerTest, MultitrackMemoryUsage) {
   EXPECT_EQ(GetExpectedMemoryUsage(896, 156011), demuxer_->GetMemoryUsage());
 }
 
-TEST_F(FFmpegDemuxerTest, SeekOnVideoTrackChangeWontSeekIfEmpty) {
-  // We only care about video tracks.
-  CreateDemuxer("bear-320x240-video-only.webm");
-  InitializeDemuxer();
-  std::vector<DemuxerStream*> streams;
-  base::RunLoop loop;
-
-  SeekOnVideoTrackChangePassthrough(
-      base::TimeDelta(), base::BindOnce(QuitLoop, loop.QuitClosure()), streams);
-
-  loop.Run();
-}
-
 }  // namespace media

media/test/pipeline_integration_test.cc

@@ -983,6 +983,21 @@ TEST_F(PipelineIntegrationTest, SwitchVideoTrackDuringPlayback) {
   Stop();
 }
 
+TEST_F(PipelineIntegrationTest, MixSeekAndTrackSwitch) {
+  // We need something with multiple tracks.
+  ASSERT_EQ(PIPELINE_OK, Start("multitrack-3video-2audio.webm", kNoClockless));
+  Pause();
+  Seek(base::Seconds(8));
+  Play();
+  OnSelectedVideoTrackChanged(MediaTrack::Id("3"));
+  OnSelectedVideoTrackChanged(MediaTrack::Id("1"));
+  OnEnabledAudioTracksChanged({MediaTrack::Id("5")});
+  Seek(base::Seconds(0));
+  Play();
+  ASSERT_TRUE(WaitUntilCurrentTimeIsAfter(TimestampMs(100)));
+  Stop();
+}
+
 TEST_F(PipelineIntegrationTest, BasicPlaybackOpusOggTrimmingHashed) {
   ASSERT_EQ(PIPELINE_OK, Start("opus-trimming-test.ogg", kHashed));