Deflake FFmpegDemuxer tests by using RunLoop::QuitClosure.

Almost every DemuxerStream::Read() test was using a bare
base::RunLoop::Run() and later posting as deprecated quit closure for
the current loop. This updates all the tests to use a helper method
which properly sets the RunLoop and explicit QuitClosure.

It also cleans up some log spam and clang  warnings at the same
time.

I'm not sure this actually resolves the linked bug though, I think that
was actually a bug in ffmpeg which has subsequently been fixed in a
later ffmpeg roll.

BUG=996040
R=xhwang

Change-Id: I91e2221da5bb95017c82a255a2676a2a6ce5f944
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1919722
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Auto-Submit: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715914}

media/filters/ffmpeg_demuxer.cc

@@ -69,12 +69,12 @@ static base::Time ExtractTimelineOffset(
     const AVFormatContext* format_context) {
   if (container == container_names::CONTAINER_WEBM) {
     const AVDictionaryEntry* entry =
-        av_dict_get(format_context->metadata, "creation_time", NULL, 0);
+        av_dict_get(format_context->metadata, "creation_time", nullptr, 0);
 
     base::Time timeline_offset;
 
     // FFmpegDemuxerTests assume base::Time::FromUTCString() is used here.
-    if (entry != NULL && entry->value != NULL &&
+    if (entry != nullptr && entry->value != nullptr &&
         base::Time::FromUTCString(entry->value, &timeline_offset)) {
       return timeline_offset;
     }
@@ -327,8 +327,8 @@ FFmpegDemuxerStream::FFmpegDemuxerStream(
   duration_ = ConvertStreamTimestamp(stream->time_base, stream->duration);
 
   if (is_encrypted) {
-    AVDictionaryEntry* key = av_dict_get(stream->metadata, "enc_key_id", NULL,
-                                         0);
+    AVDictionaryEntry* key =
+        av_dict_get(stream->metadata, "enc_key_id", nullptr, 0);
     DCHECK(key);
     DCHECK(key->value);
     if (!key || !key->value)
@@ -398,10 +398,10 @@ void FFmpegDemuxerStream::EnqueuePacket(ScopedAVPacket packet) {
   last_packet_dts_ = packet_dts;
 
   if (waiting_for_keyframe_) {
-    if (packet->flags & AV_PKT_FLAG_KEY)
+    if (packet->flags & AV_PKT_FLAG_KEY) {
       waiting_for_keyframe_ = false;
-    else {
-      DLOG(WARNING) << "Dropped non-keyframe pts=" << packet->pts;
+    } else {
+      DVLOG(1) << "Dropped non-keyframe pts=" << packet->pts;
       return;
     }
   }
@@ -881,8 +881,8 @@ size_t FFmpegDemuxerStream::MemoryUsage() const {
 
 std::string FFmpegDemuxerStream::GetMetadata(const char* key) const {
   const AVDictionaryEntry* entry =
-      av_dict_get(stream_->metadata, key, NULL, 0);
-  return (entry == NULL || entry->value == NULL) ? "" : entry->value;
+      av_dict_get(stream_->metadata, key, nullptr, 0);
+  return (entry == nullptr || entry->value == nullptr) ? "" : entry->value;
 }
 
 // static
@@ -905,21 +905,15 @@ FFmpegDemuxer::FFmpegDemuxer(
     const MediaTracksUpdatedCB& media_tracks_updated_cb,
     MediaLog* media_log,
     bool is_local_file)
-    : host_(NULL),
-      task_runner_(task_runner),
+    : task_runner_(task_runner),
       // FFmpeg has no asynchronous API, so we use base::WaitableEvents inside
       // the BlockingUrlProtocol to handle hops to the render thread for network
       // reads and seeks.
       blocking_task_runner_(
           base::CreateSequencedTaskRunner({base::ThreadPool(), base::MayBlock(),
                                            base::TaskPriority::USER_BLOCKING})),
-      stopped_(false),
-      pending_read_(false),
       data_source_(data_source),
       media_log_(media_log),
-      bitrate_(0),
-      start_time_(kNoTimestamp),
-      duration_known_(false),
       encrypted_media_init_data_cb_(encrypted_media_init_data_cb),
       media_tracks_updated_cb_(media_tracks_updated_cb),
       is_local_file_(is_local_file) {
@@ -1035,7 +1029,7 @@ void FFmpegDemuxer::Stop() {
       stream->Stop();
   }
 
-  data_source_ = NULL;
+  data_source_ = nullptr;
 
   // Invalidate WeakPtrs on |task_runner_|, destruction may happen on another
   // thread. We don't need to wait for any outstanding tasks since they will all
@@ -1146,7 +1140,7 @@ FFmpegDemuxerStream* FFmpegDemuxer::GetFirstEnabledFFmpegStream(
       return stream.get();
     }
   }
-  return NULL;
+  return nullptr;
 }
 
 base::TimeDelta FFmpegDemuxer::GetStartTime() const {

media/filters/ffmpeg_demuxer.h

@@ -41,6 +41,7 @@
 #include "media/base/demuxer.h"
 #include "media/base/pipeline_status.h"
 #include "media/base/text_track_config.h"
+#include "media/base/timestamp_constants.h"
 #include "media/base/video_decoder_config.h"
 #include "media/ffmpeg/ffmpeg_deleters.h"
 #include "media/filters/blocking_url_protocol.h"
@@ -337,7 +338,7 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
   // Executes |pending_seek_cb_| with |status| and closes out the async trace.
   void RunPendingSeekCB(PipelineStatus status);
 
-  DemuxerHost* host_;
+  DemuxerHost* host_ = nullptr;
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
@@ -348,13 +349,13 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
   PipelineStatusCallback init_cb_;
 
   // Indicates if Stop() has been called.
-  bool stopped_;
+  bool stopped_ = false;
 
   // Tracks if there's an outstanding av_read_frame() operation.
   //
   // TODO(scherkus): Allow more than one read in flight for higher read
   // throughput using demuxer_bench to verify improvements.
-  bool pending_read_;
+  bool pending_read_ = false;
 
   // Tracks if there's an outstanding av_seek_frame() operation. Used to discard
   // results of pre-seek av_read_frame() operations.
@@ -379,12 +380,12 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
   MediaLog* media_log_;
 
   // Derived bitrate after initialization has completed.
-  int bitrate_;
+  int bitrate_ = 0;
 
   // The first timestamp of the audio or video stream, whichever is lower.  This
   // is used to adjust timestamps so that external consumers always see a zero
   // based timeline.
-  base::TimeDelta start_time_;
+  base::TimeDelta start_time_ = kNoTimestamp;
 
   // The Time associated with timestamp 0. Set to a null
   // time if the file doesn't have an association to Time.
@@ -392,7 +393,7 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
 
   // Set if we know duration of the audio stream. Used when processing end of
   // stream -- at this moment we definitely know duration.
-  bool duration_known_;
+  bool duration_known_ = false;
   base::TimeDelta duration_;
 
   // FFmpegURLProtocol implementation and corresponding glue bits.

media/filters/ffmpeg_demuxer_unittest.cc

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/bind.h"
+#include "base/callback_forward.h"
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ -22,6 +23,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "media/base/decrypt_config.h"
+#include "media/base/demuxer_stream.h"
 #include "media/base/media_client.h"
 #include "media/base/media_tracks.h"
 #include "media/base/media_util.h"
@@ -202,6 +204,7 @@ class FFmpegDemuxerTest : public testing::Test {
   // This makes it easier to track down where test failures occur.
   void OnReadDone(const base::Location& location,
                   const ReadExpectation& read_expectation,
+                  base::OnceClosure quit_closure,
                   DemuxerStream::Status status,
                   scoped_refptr<DecoderBuffer> buffer) {
     std::string location_str = location.ToString();
@@ -218,18 +221,7 @@ class FFmpegDemuxerTest : public testing::Test {
       EXPECT_EQ(read_expectation.is_key_frame, buffer->is_key_frame());
     }
     OnReadDoneCalled(read_expectation.size, read_expectation.timestamp_us);
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::RunLoop::QuitCurrentWhenIdleClosureDeprecated());
-  }
-
-  DemuxerStream::ReadCB NewReadCB(
-      const base::Location& location,
-      int size,
-      int64_t timestamp_us,
-      bool is_key_frame,
-      DemuxerStream::Status status = DemuxerStream::kOk) {
-    return NewReadCBWithCheckedDiscard(location, size, timestamp_us,
-                                       base::TimeDelta(), is_key_frame, status);
+    std::move(quit_closure).Run();
   }
 
   DemuxerStream::ReadCB NewReadCBWithCheckedDiscard(
@@ -238,14 +230,35 @@ class FFmpegDemuxerTest : public testing::Test {
       int64_t timestamp_us,
       base::TimeDelta discard_front_padding,
       bool is_key_frame,
-      DemuxerStream::Status status = DemuxerStream::kOk) {
+      DemuxerStream::Status status,
+      base::OnceClosure quit_closure) {
     EXPECT_CALL(*this, OnReadDoneCalled(size, timestamp_us));
 
     struct ReadExpectation read_expectation(
         size, timestamp_us, discard_front_padding, is_key_frame, status);
 
     return base::BindOnce(&FFmpegDemuxerTest::OnReadDone,
-                          base::Unretained(this), location, read_expectation);
+                          base::Unretained(this), location, read_expectation,
+                          std::move(quit_closure));
+  }
+
+  void Read(DemuxerStream* stream,
+            const base::Location& location,
+            int size,
+            int64_t timestamp_us,
+            bool is_key_frame,
+            DemuxerStream::Status status = DemuxerStream::Status::kOk,
+            base::TimeDelta discard_front_padding = base::TimeDelta()) {
+    base::RunLoop run_loop;
+    stream->Read(NewReadCBWithCheckedDiscard(
+        location, size, timestamp_us, discard_front_padding, is_key_frame,
+        status, run_loop.QuitClosure()));
+    run_loop.Run();
+
+    // Ensure tasks posted after the ReadCB is satisfied run. These are always
+    // tasks posted to FFmpegDemuxer's internal |blocking_task_runner_|, which
+    // the RunLoop above won't pump.
+    task_environment_.RunUntilIdle();
   }
 
   MOCK_METHOD2(OnEncryptedMediaInitData,
@@ -331,9 +344,9 @@ class FFmpegDemuxerTest : public testing::Test {
     EXPECT_TRUE(base::PathService::Get(base::DIR_SOURCE_ROOT, &file_path));
 
     file_path = file_path.Append(FILE_PATH_LITERAL("media"))
-        .Append(FILE_PATH_LITERAL("test"))
-        .Append(FILE_PATH_LITERAL("data"))
-        .AppendASCII(name);
+                    .Append(FILE_PATH_LITERAL("test"))
+                    .Append(FILE_PATH_LITERAL("data"))
+                    .AppendASCII(name);
 
     data_source_.reset(new FileDataSource());
     EXPECT_TRUE(data_source_->Initialize(file_path));
@@ -482,15 +495,18 @@ TEST_F(FFmpegDemuxerTest, AbortPendingReads) {
   // stream to be marked as EOF.  Simulate this here to ensure it is properly
   // cleared by the AbortPendingReads() call.
   format_context()->pb->eof_reached = 1;
-  audio->Read(NewReadCB(FROM_HERE, 29, 0, true, DemuxerStream::kAborted));
-  demuxer_->AbortPendingReads();
-  base::RunLoop().Run();
-  task_environment_.RunUntilIdle();
+  {
+    base::RunLoop run_loop;
+    audio->Read(NewReadCBWithCheckedDiscard(FROM_HERE, 29, 0, base::TimeDelta(),
+                                            true, DemuxerStream::kAborted,
+                                            run_loop.QuitClosure()));
+    demuxer_->AbortPendingReads();
+    run_loop.Run();
+    task_environment_.RunUntilIdle();
+  }
 
   // Additional reads should also be aborted (until a Seek()).
-  audio->Read(NewReadCB(FROM_HERE, 29, 0, true, DemuxerStream::kAborted));
-  base::RunLoop().Run();
-  task_environment_.RunUntilIdle();
+  Read(audio, FROM_HERE, 29, 0, true, DemuxerStream::kAborted);
 
   // Ensure blocking thread has completed outstanding work.
   demuxer_->Stop();
@@ -508,14 +524,8 @@ TEST_F(FFmpegDemuxerTest, Read_Audio) {
 
   // Attempt a read from the audio stream and run the message loop until done.
   DemuxerStream* audio = GetStream(DemuxerStream::AUDIO);
-
-  audio->Read(NewReadCB(FROM_HERE, 29, 0, true));
-  base::RunLoop().Run();
-
-  audio->Read(NewReadCB(FROM_HERE, 27, 3000, true));
-  base::RunLoop().Run();
-  task_environment_.RunUntilIdle();
-
+  Read(audio, FROM_HERE, 29, 0, true);
+  Read(audio, FROM_HERE, 27, 3000, true);
   EXPECT_EQ(166866, demuxer_->GetMemoryUsage());
 }
 
@@ -526,14 +536,8 @@ TEST_F(FFmpegDemuxerTest, Read_Video) {
 
   // Attempt a read from the video stream and run the message loop until done.
   DemuxerStream* video = GetStream(DemuxerStream::VIDEO);
-
-  video->Read(NewReadCB(FROM_HERE, 22084, 0, true));
-  base::RunLoop().Run();
-
-  video->Read(NewReadCB(FROM_HERE, 1057, 33000, false));
-  base::RunLoop().Run();
-  task_environment_.RunUntilIdle();
-
+  Read(video, FROM_HERE, 22084, 0, true);
+  Read(video, FROM_HERE, 1057, 33000, false);
   EXPECT_EQ(148778, demuxer_->GetMemoryUsage());
 }
 
@@ -613,12 +617,8 @@ TEST_F(FFmpegDemuxerTest, Read_VideoPositiveStartTime) {
 
   // Run the test twice with a seek in between.
   for (int i = 0; i < 2; ++i) {
-    video->Read(NewReadCB(FROM_HERE, 5636, video_start_time.InMicroseconds(),
-                          true));
-    base::RunLoop().Run();
-    audio->Read(NewReadCB(FROM_HERE, 165, audio_start_time.InMicroseconds(),
-                          true));
-    base::RunLoop().Run();
+    Read(video, FROM_HERE, 5636, video_start_time.InMicroseconds(), true);
+    Read(audio, FROM_HERE, 165, audio_start_time.InMicroseconds(), true);
 
     // Verify that the start time is equal to the lowest timestamp (ie the
     // audio).
@@ -642,8 +642,7 @@ TEST_F(FFmpegDemuxerTest, Read_AudioNoStartTime) {
 
   // Run the test twice with a seek in between.
   for (int i = 0; i < 2; ++i) {
-    GetStream(DemuxerStream::AUDIO)->Read(NewReadCB(FROM_HERE, 4095, 0, true));
-    base::RunLoop().Run();
+    Read(GetStream(DemuxerStream::AUDIO), FROM_HERE, 4095, 0, true);
     EXPECT_EQ(base::TimeDelta(), demuxer_->start_time());
 
     // Seek back to the beginning and repeat the test.
@@ -678,29 +677,20 @@ TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOggDiscard_Bear) {
   // guarantee the order of demuxed packets in OGG containers. See
   // http://crbug.com/387996.
   for (int i = 0; i < 1; ++i) {
-    audio->Read(
-        NewReadCBWithCheckedDiscard(FROM_HERE, 40, 0, kInfiniteDuration, true));
-    base::RunLoop().Run();
-    audio->Read(NewReadCBWithCheckedDiscard(FROM_HERE, 41, 2903,
-                                            kInfiniteDuration, true));
-    base::RunLoop().Run();
-    audio->Read(NewReadCBWithCheckedDiscard(
-        FROM_HERE, 173, 5805, base::TimeDelta::FromMicroseconds(10159), true));
-    base::RunLoop().Run();
-
-    audio->Read(NewReadCB(FROM_HERE, 148, 18866, true));
-    base::RunLoop().Run();
+    Read(audio, FROM_HERE, 40, 0, true, DemuxerStream::Status::kOk,
+         kInfiniteDuration);
+    Read(audio, FROM_HERE, 41, 2903, true, DemuxerStream::Status::kOk,
+         kInfiniteDuration);
+    Read(audio, FROM_HERE, 173, 5805, true, DemuxerStream::Status::kOk,
+         base::TimeDelta::FromMicroseconds(10159));
+
+    Read(audio, FROM_HERE, 148, 18866, true);
     EXPECT_EQ(base::TimeDelta::FromMicroseconds(-15964),
               demuxer_->start_time());
 
-    video->Read(NewReadCB(FROM_HERE, 5751, 0, true));
-    base::RunLoop().Run();
-
-    video->Read(NewReadCB(FROM_HERE, 846, 33367, false));
-    base::RunLoop().Run();
-
-    video->Read(NewReadCB(FROM_HERE, 1255, 66733, false));
-    base::RunLoop().Run();
+    Read(video, FROM_HERE, 5751, 0, true);
+    Read(video, FROM_HERE, 846, 33367, false);
+    Read(video, FROM_HERE, 1255, 66733, false);
 
     // Seek back to the beginning and repeat the test.
     WaitableMessageLoopEvent event;
@@ -724,27 +714,18 @@ TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOggDiscard_Sync) {
 
   // Run the test twice with a seek in between.
   for (int i = 0; i < 2; ++i) {
-    audio->Read(NewReadCBWithCheckedDiscard(
-        FROM_HERE, 1, 0, base::TimeDelta::FromMicroseconds(2902), true));
-    base::RunLoop().Run();
-
-    audio->Read(NewReadCB(FROM_HERE, 1, 2902, true));
-    base::RunLoop().Run();
-    EXPECT_EQ(base::TimeDelta::FromMicroseconds(-2902),
-              demuxer_->start_time());
+    Read(audio, FROM_HERE, 1, 0, true, DemuxerStream::Status::kOk,
+         base::TimeDelta::FromMicroseconds(2902));
+    Read(audio, FROM_HERE, 1, 2902, true);
+    EXPECT_EQ(base::TimeDelta::FromMicroseconds(-2902), demuxer_->start_time());
 
     // Though the internal start time may be below zero, the exposed media time
     // must always be >= zero.
     EXPECT_EQ(base::TimeDelta(), demuxer_->GetStartTime());
 
-    video->Read(NewReadCB(FROM_HERE, 9997, 0, true));
-    base::RunLoop().Run();
-
-    video->Read(NewReadCB(FROM_HERE, 16, 33241, false));
-    base::RunLoop().Run();
-
-    video->Read(NewReadCB(FROM_HERE, 631, 66482, false));
-    base::RunLoop().Run();
+    Read(video, FROM_HERE, 9997, 0, true);
+    Read(video, FROM_HERE, 16, 33241, false);
+    Read(video, FROM_HERE, 631, 66482, false);
 
     // Seek back to the beginning and repeat the test.
     WaitableMessageLoopEvent event;
@@ -775,23 +756,17 @@ TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOpusDiscard_Sync) {
   // Run the test twice with a seek in between.
   for (int i = 0; i < 2; ++i) {
     for (size_t j = 0; j < base::size(kTestExpectations); ++j) {
-      audio->Read(NewReadCB(FROM_HERE, kTestExpectations[j][0],
-                            kTestExpectations[j][1], true));
-      base::RunLoop().Run();
+      Read(audio, FROM_HERE, kTestExpectations[j][0], kTestExpectations[j][1],
+           true);
     }
 
     // Though the internal start time may be below zero, the exposed media time
     // must always be >= zero.
     EXPECT_EQ(base::TimeDelta(), demuxer_->GetStartTime());
 
-    video->Read(NewReadCB(FROM_HERE, 16009, 0, true));
-    base::RunLoop().Run();
-
-    video->Read(NewReadCB(FROM_HERE, 2715, 1000, false));
-    base::RunLoop().Run();
-
-    video->Read(NewReadCB(FROM_HERE, 427, 33000, false));
-    base::RunLoop().Run();
+    Read(video, FROM_HERE, 16009, 0, true);
+    Read(video, FROM_HERE, 2715, 1000, false);
+    Read(video, FROM_HERE, 427, 33000, false);
 
     // Seek back to the beginning and repeat the test.
     WaitableMessageLoopEvent event;
@@ -812,16 +787,11 @@ TEST_F(FFmpegDemuxerTest, TestAudioNegativeTimestamps) {
   InitializeDemuxer();
 
   DemuxerStream* audio = GetStream(DemuxerStream::AUDIO);
-  audio->Read(NewReadCB(FROM_HERE, 104, 0, true));
-  base::RunLoop().Run();
-  audio->Read(NewReadCB(FROM_HERE, 104, 25873, true));
-  base::RunLoop().Run();
-  audio->Read(NewReadCB(FROM_HERE, 104, 51746, true));
-  base::RunLoop().Run();
-  audio->Read(NewReadCB(FROM_HERE, 104, 77619, true));
-  base::RunLoop().Run();
-  audio->Read(NewReadCB(FROM_HERE, 104, 103492, true));
-  base::RunLoop().Run();
+  Read(audio, FROM_HERE, 104, 0, true);
+  Read(audio, FROM_HERE, 104, 25873, true);
+  Read(audio, FROM_HERE, 104, 51746, true);
+  Read(audio, FROM_HERE, 104, 77619, true);
+  Read(audio, FROM_HERE, 104, 103492, true);
 }
 #endif  // defined(OS_CHROMEOS)
 
@@ -844,30 +814,24 @@ TEST_F(FFmpegDemuxerTest,
 
   // Run the test twice with a seek in between.
   for (int i = 0; i < 2; ++i) {
-    audio->Read(NewReadCBWithCheckedDiscard(
-        FROM_HERE, 408, 0, base::TimeDelta::FromMicroseconds(6500), true));
-    base::RunLoop().Run();
+    Read(audio, FROM_HERE, 408, 0, true, DemuxerStream::Status::kOk,
+         base::TimeDelta::FromMicroseconds(6500));
 
     for (size_t j = 0; j < base::size(kTestExpectations); ++j) {
-      audio->Read(NewReadCB(FROM_HERE, kTestExpectations[j][0],
-                            kTestExpectations[j][1], true));
-      base::RunLoop().Run();
+      Read(audio, FROM_HERE, kTestExpectations[j][0], kTestExpectations[j][1],
+           true);
     }
 
     // Though the internal start time may be below zero, the exposed media time
     // must always be >= zero.
     EXPECT_EQ(base::TimeDelta(), demuxer_->GetStartTime());
 
-    video->Read(NewReadCB(FROM_HERE, 185105, 0, true));
-    base::RunLoop().Run();
-
-    video->Read(NewReadCB(FROM_HERE, 35941, 125000, false));
-    base::RunLoop().Run();
+    Read(video, FROM_HERE, 185105, 0, true);
+    Read(video, FROM_HERE, 35941, 125000, false);
 
     // If things aren't working correctly, this expectation will fail because
     // the chained ogg workaround breaks out of order timestamps.
-    video->Read(NewReadCB(FROM_HERE, 8129, 84000, false));
-    base::RunLoop().Run();
+    Read(video, FROM_HERE, 8129, 84000, false);
 
     // Seek back to the beginning and repeat the test.
     WaitableMessageLoopEvent event;
@@ -887,13 +851,10 @@ TEST_F(FFmpegDemuxerTest, Read_AudioNegativeStartTimeAndOpusSfxDiscard_Sync) {
   DemuxerStream* audio = GetStream(DemuxerStream::AUDIO);
   EXPECT_EQ(audio->audio_decoder_config().codec_delay(), 312);
 
-   // Run the test twice with a seek in between.
+  // Run the test twice with a seek in between.
   for (int i = 0; i < 2; ++i) {
-    audio->Read(NewReadCB(FROM_HERE, 314, 0, true));
-    base::RunLoop().Run();
-
-    audio->Read(NewReadCB(FROM_HERE, 244, 20000, true));
-    base::RunLoop().Run();
+    Read(audio, FROM_HERE, 314, 0, true);
+    Read(audio, FROM_HERE, 244, 20000, true);
 
     // Though the internal start time may be below zero, the exposed media time
     // must always be >= zero.
@@ -923,18 +884,14 @@ TEST_F(FFmpegDemuxerTest, Read_DiscardDisabledVideoStream) {
   CreateDemuxer("bear-vp8-webvtt.webm");
   InitializeDemuxer();
   Seek(seek_target);
-  GetStream(DemuxerStream::AUDIO)
-      ->Read(NewReadCB(FROM_HERE, 163, 1612000, true));
-  base::RunLoop().Run();
+  Read(GetStream(DemuxerStream::AUDIO), FROM_HERE, 163, 1612000, true);
   auto bytes_read_with_video_enabled = data_source_->bytes_read_for_testing();
 
   static_cast<FFmpegDemuxerStream*>(GetStream(DemuxerStream::VIDEO))
       ->SetEnabled(false, base::TimeDelta());
   data_source_->reset_bytes_read_for_testing();
   Seek(seek_target);
-  GetStream(DemuxerStream::AUDIO)
-      ->Read(NewReadCB(FROM_HERE, 156, 1987000, true));
-  base::RunLoop().Run();
+  Read(GetStream(DemuxerStream::AUDIO), FROM_HERE, 156, 1987000, true);
   auto bytes_read_with_video_disabled = data_source_->bytes_read_for_testing();
   EXPECT_LT(bytes_read_with_video_disabled, bytes_read_with_video_enabled);
 }
@@ -997,8 +954,7 @@ TEST_F(FFmpegDemuxerTest, Seek) {
   ASSERT_TRUE(audio);
 
   // Read a video packet and release it.
-  video->Read(NewReadCB(FROM_HERE, 22084, 0, true));
-  base::RunLoop().Run();
+  Read(video, FROM_HERE, 22084, 0, true);
 
   // Issue a simple forward seek, which should discard queued packets.
   WaitableMessageLoopEvent event;
@@ -1007,24 +963,19 @@ TEST_F(FFmpegDemuxerTest, Seek) {
   event.RunAndWaitForStatus(PIPELINE_OK);
 
   // Audio read #1.
-  audio->Read(NewReadCB(FROM_HERE, 145, 803000, true));
-  base::RunLoop().Run();
+  Read(audio, FROM_HERE, 145, 803000, true);
 
   // Audio read #2.
-  audio->Read(NewReadCB(FROM_HERE, 148, 826000, true));
-  base::RunLoop().Run();
+  Read(audio, FROM_HERE, 148, 826000, true);
 
   // Video read #1.
-  video->Read(NewReadCB(FROM_HERE, 5425, 801000, true));
-  base::RunLoop().Run();
+  Read(video, FROM_HERE, 5425, 801000, true);
 
   // Video read #2.
-  video->Read(NewReadCB(FROM_HERE, 1906, 834000, false));
-  base::RunLoop().Run();
+  Read(video, FROM_HERE, 1906, 834000, false);
 }
 
-// TODO(crbug.com/996040): Flaky.
-TEST_F(FFmpegDemuxerTest, DISABLED_CancelledSeek) {
+TEST_F(FFmpegDemuxerTest, CancelledSeek) {
   CreateDemuxer("bear-320x240.webm");
   InitializeDemuxer();
 
@@ -1035,8 +986,7 @@ TEST_F(FFmpegDemuxerTest, DISABLED_CancelledSeek) {
   ASSERT_TRUE(audio);
 
   // Read a video packet and release it.
-  video->Read(NewReadCB(FROM_HERE, 22084, 0, true));
-  base::RunLoop().Run();
+  Read(video, FROM_HERE, 22084, 0, true);
 
   // Issue a simple forward seek, which should discard queued packets.
   WaitableMessageLoopEvent event;
@@ -1066,7 +1016,6 @@ TEST_F(FFmpegDemuxerTest, Stop) {
   // Attempt the read...
   audio->Read(callback.Get());
   task_environment_.RunUntilIdle();
-  ;
 
   // Don't let the test call Stop() again.
   demuxer_.reset();
@@ -1085,8 +1034,7 @@ TEST_F(FFmpegDemuxerTest, SeekWithCuesBeforeFirstCluster) {
   ASSERT_TRUE(audio);
 
   // Read a video packet and release it.
-  video->Read(NewReadCB(FROM_HERE, 22084, 0, true));
-  base::RunLoop().Run();
+  Read(video, FROM_HERE, 22084, 0, true);
 
   // Issue a simple forward seek, which should discard queued packets.
   WaitableMessageLoopEvent event;
@@ -1095,20 +1043,16 @@ TEST_F(FFmpegDemuxerTest, SeekWithCuesBeforeFirstCluster) {
   event.RunAndWaitForStatus(PIPELINE_OK);
 
   // Audio read #1.
-  audio->Read(NewReadCB(FROM_HERE, 40, 2403000, true));
-  base::RunLoop().Run();
+  Read(audio, FROM_HERE, 40, 2403000, true);
 
   // Audio read #2.
-  audio->Read(NewReadCB(FROM_HERE, 42, 2406000, true));
-  base::RunLoop().Run();
+  Read(audio, FROM_HERE, 42, 2406000, true);
 
   // Video read #1.
-  video->Read(NewReadCB(FROM_HERE, 5276, 2402000, true));
-  base::RunLoop().Run();
+  Read(video, FROM_HERE, 5276, 2402000, true);
 
   // Video read #2.
-  video->Read(NewReadCB(FROM_HERE, 1740, 2436000, false));
-  base::RunLoop().Run();
+  Read(video, FROM_HERE, 1740, 2436000, false);
 }
 
 // Ensure ID3v1 tag reading is disabled.  id3_test.mp3 has an ID3v1 tag with the
@@ -1116,7 +1060,7 @@ TEST_F(FFmpegDemuxerTest, SeekWithCuesBeforeFirstCluster) {
 TEST_F(FFmpegDemuxerTest, NoID3TagData) {
   CreateDemuxer("id3_test.mp3");
   InitializeDemuxer();
-  EXPECT_FALSE(av_dict_get(format_context()->metadata, "title", NULL, 0));
+  EXPECT_FALSE(av_dict_get(format_context()->metadata, "title", nullptr, 0));
 }
 
 // Ensure MP3 files with large image/video based ID3 tags demux okay.  FFmpeg
@@ -1184,8 +1128,7 @@ TEST_F(FFmpegDemuxerTest, MP4_ZeroStszEntry) {
 
 class Mp3SeekFFmpegDemuxerTest
     : public FFmpegDemuxerTest,
-      public testing::WithParamInterface<const char*> {
-};
+      public testing::WithParamInterface<const char*> {};
 TEST_P(Mp3SeekFFmpegDemuxerTest, TestFastSeek) {
   // Init demxuer with given MP3 file parameter.
   CreateDemuxer(GetParam());
@@ -1256,10 +1199,8 @@ static void ValidateAnnexB(DemuxerStream* stream,
 }
 
 TEST_F(FFmpegDemuxerTest, IsValidAnnexB) {
-  const char* files[] = {
-    "bear-1280x720-av_frag.mp4",
-    "bear-1280x720-av_with-aud-nalus_frag.mp4"
-  };
+  const char* files[] = {"bear-1280x720-av_frag.mp4",
+                         "bear-1280x720-av_with-aud-nalus_frag.mp4"};
 
   for (size_t i = 0; i < base::size(files); ++i) {
     DVLOG(1) << "Testing " << files[i];
@@ -1368,11 +1309,8 @@ TEST_F(FFmpegDemuxerTest, HEVC_in_MP4_container) {
   DemuxerStream* video = GetStream(DemuxerStream::VIDEO);
   ASSERT_TRUE(video);
 
-  video->Read(NewReadCB(FROM_HERE, 3569, 66733, true));
-  base::RunLoop().Run();
-
-  video->Read(NewReadCB(FROM_HERE, 1042, 200200, false));
-  base::RunLoop().Run();
+  Read(video, FROM_HERE, 3569, 66733, true);
+  Read(video, FROM_HERE, 1042, 200200, false);
 
   SetMediaClient(nullptr);
 #else
@@ -1398,11 +1336,8 @@ TEST_F(FFmpegDemuxerTest, Read_AC3_Audio) {
   DemuxerStream* audio = GetStream(DemuxerStream::AUDIO);
 
   // Read the first two frames and check that we are getting expected data
-  audio->Read(NewReadCB(FROM_HERE, 834, 0, true));
-  base::RunLoop().Run();
-
-  audio->Read(NewReadCB(FROM_HERE, 836, 34830, true));
-  base::RunLoop().Run();
+  Read(audio, FROM_HERE, 834, 0, true);
+  Read(audio, FROM_HERE, 836, 34830, true);
 
   SetMediaClient(nullptr);
 #else
@@ -1428,11 +1363,8 @@ TEST_F(FFmpegDemuxerTest, Read_EAC3_Audio) {
   DemuxerStream* audio = GetStream(DemuxerStream::AUDIO);
 
   // Read the first two frames and check that we are getting expected data
-  audio->Read(NewReadCB(FROM_HERE, 870, 0, true));
-  base::RunLoop().Run();
-
-  audio->Read(NewReadCB(FROM_HERE, 872, 34830, true));
-  base::RunLoop().Run();
+  Read(audio, FROM_HERE, 870, 0, true);
+  Read(audio, FROM_HERE, 872, 34830, true);
 
   SetMediaClient(nullptr);
 #else
@@ -1773,21 +1705,18 @@ TEST_F(FFmpegDemuxerTest, MultitrackMemoryUsage) {
   // streams with available capacity, i.e all enabled streams. By default only
   // the first audio and the first video stream are enabled, so the memory usage
   // shouldn't be too high.
-  audio->Read(NewReadCB(FROM_HERE, 304, 0, true));
-  base::RunLoop().Run();
-  task_environment_.RunUntilIdle();
+  Read(audio, FROM_HERE, 304, 0, true);
   EXPECT_EQ(22134, demuxer_->GetMemoryUsage());
 
   // Now enable all demuxer streams in the file and perform another read, this
   // will buffer the data for additional streams and memory usage will increase.
   std::vector<DemuxerStream*> streams = demuxer_->GetAllStreams();
-  for (auto* stream : streams)
+  for (auto* stream : streams) {
     static_cast<FFmpegDemuxerStream*>(stream)->SetEnabled(true,
                                                           base::TimeDelta());
+  }
+  Read(audio, FROM_HERE, 166, 21000, true);
 
-  audio->Read(NewReadCB(FROM_HERE, 166, 21000, true));
-  base::RunLoop().Run();
-  task_environment_.RunUntilIdle();
   // With newly enabled demuxer streams the amount of memory used by the demuxer
   // is much higher.
   EXPECT_EQ(156011, demuxer_->GetMemoryUsage());