[media] Maintain DecoderSelector state throughout decoder selection.

This CL changes DecoderStream to maintain an instance of DecoderSelector
continually. This enabled DecoderSelector to maintain
DecryptingDemuxerStream state and the blacklist internally.

An immediate consequence is that DecoderSelector can now always try the
full list of potential decoders each time that selection is triggered.
This fixes the GPU->GPU changeType() transition, and provides a
foundation to build GPU fallforward on top of.

Another consequence is that fallback can occur more than once for a
stream. It is no longer possible to loop forever doing fallbacks, so this
is a robustness improvement in addition to being a requirement for
changeType().

Bug: 877673
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I76d1bed1c914e1ec58a6653d7200fbb67f70971a
Reviewed-on: https://chromium-review.googlesource.com/1188978
Commit-Queue: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587908}

media/base/callback_holder.h

@@ -17,8 +17,6 @@ template <typename CB> class CallbackHolder {
   CallbackHolder() : hold_(false) {}
 
   ~CallbackHolder() {
-    // Make sure all callbacks are satisfied!
-    DCHECK(!hold_);
     DCHECK(original_cb_.is_null());
     DCHECK(held_cb_.is_null());
   }

media/base/test_helpers.cc

@@ -200,6 +200,12 @@ AudioDecoderConfig TestAudioConfig::Normal() {
                             Unencrypted());
 }
 
+AudioDecoderConfig TestAudioConfig::NormalEncrypted() {
+  return AudioDecoderConfig(kCodecVorbis, kSampleFormatPlanarF32,
+                            CHANNEL_LAYOUT_STEREO, 44100, EmptyExtraData(),
+                            AesCtrEncryptionScheme());
+}
+
 // static
 AudioParameters TestAudioParameters::Normal() {
   return AudioParameters(AudioParameters::AUDIO_PCM_LOW_LATENCY,

media/base/test_helpers.h

@@ -110,6 +110,7 @@ class TestVideoConfig {
 class TestAudioConfig {
  public:
   static AudioDecoderConfig Normal();
+  static AudioDecoderConfig NormalEncrypted();
 };
 
 // Provides pre-canned AudioParameters objects.

media/filters/BUILD.gn

@@ -250,10 +250,10 @@ source_set("unit_tests") {
   sources = [
     "audio_buffer_stream_unittest.cc",
     "audio_clock_unittest.cc",
-    "audio_decoder_selector_unittest.cc",
     "audio_renderer_algorithm_unittest.cc",
     "audio_timestamp_validator_unittest.cc",
     "chunk_demuxer_unittest.cc",
+    "decoder_selector_unittest.cc",
     "decrypting_audio_decoder_unittest.cc",
     "decrypting_demuxer_stream_unittest.cc",
     "decrypting_video_decoder_unittest.cc",
@@ -271,7 +271,6 @@ source_set("unit_tests") {
     "source_buffer_state_unittest.cc",
     "source_buffer_stream_unittest.cc",
     "video_cadence_estimator_unittest.cc",
-    "video_decoder_selector_unittest.cc",
     "video_frame_stream_unittest.cc",
     "video_renderer_algorithm_unittest.cc",
     "vp8_bool_decoder_unittest.cc",

media/filters/decoder_selector.h

@@ -26,9 +26,8 @@ class CdmContext;
 class DecryptingDemuxerStream;
 class MediaLog;
 
-// DecoderSelector (creates if necessary and) initializes the proper
-// Decoder for a given DemuxerStream. If the given DemuxerStream is
-// encrypted, a DecryptingDemuxerStream may also be created.
+// DecoderSelector handles construction and initialization of Decoders for a
+// DemuxerStream, and maintains the state required for decoder fallback.
 // The template parameter |StreamType| is the type of stream we will be
 // selecting a decoder for.
 template<DemuxerStream::Type StreamType>
@@ -44,87 +43,76 @@ class MEDIA_EXPORT DecoderSelector {
   using CreateDecodersCB =
       base::RepeatingCallback<std::vector<std::unique_ptr<Decoder>>()>;
 
-  // Indicates completion of Decoder selection.
-  // - First parameter: The initialized Decoder. If it's set to NULL, then
-  // Decoder initialization failed.
-  // - Second parameter: The initialized DecryptingDemuxerStream. If it's not
-  // NULL, then a DecryptingDemuxerStream is created and initialized to do
-  // decryption for the initialized Decoder.
-  // Note: The caller owns selected Decoder and DecryptingDemuxerStream.
+  // Emits the result of a single call to SelectDecoder(). Parameters are
+  //   1: The initialized Decoder. nullptr if selection failed.
+  //   2: The initialized DecryptingDemuxerStream, if one was created. This
+  //      happens at most once for a DecoderSelector instance.
+  // The caller owns the Decoder and DecryptingDemuxerStream.
+  //
   // The caller should call DecryptingDemuxerStream::Reset() before
   // calling Decoder::Reset() to release any pending decryption or read.
   using SelectDecoderCB =
-      base::Callback<void(std::unique_ptr<Decoder>,
-                          std::unique_ptr<DecryptingDemuxerStream>)>;
+      base::OnceCallback<void(std::unique_ptr<Decoder>,
+                              std::unique_ptr<DecryptingDemuxerStream>)>;
 
-  DecoderSelector(
-      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-      CreateDecodersCB create_decoders_cb,
-      MediaLog* media_log);
+  DecoderSelector(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                  CreateDecodersCB create_decoders_cb,
+                  MediaLog* media_log);
 
-  // Aborts pending Decoder selection and fires |select_decoder_cb| with
-  // null and null immediately if it's pending.
+  // Aborts any pending decoder selection.
   ~DecoderSelector();
 
-  // Initializes and selects the first Decoder that can decode the |stream|.
-  // The selected Decoder (and DecryptingDemuxerStream) is returned via
-  // the |select_decoder_cb|.
-  // Notes:
-  // 1. This must not be called again before |select_decoder_cb| is run.
-  // 2. |create_decoders_cb| will be called to create a list of candidate
-  //    decoders to select from.
-  // 3. The |blacklisted_decoder| will be skipped in the decoder selection
-  //    process, unless DecryptingDemuxerStream is chosen. This is because
-  //    DecryptingDemuxerStream updates the |config_|, and the blacklist should
-  //    only be applied to the original |stream| config.
-  // 4. All decoders that are not selected will be deleted upon returning
-  //    |select_decoder_cb|.
-  // 5. |cdm_context| is optional. If |cdm_context| is null, no CDM will be
-  //    available to perform decryption.
-  void SelectDecoder(StreamTraits* traits,
-                     DemuxerStream* stream,
-                     CdmContext* cdm_context,
-                     const std::string& blacklisted_decoder,
-                     const SelectDecoderCB& select_decoder_cb,
-                     const typename Decoder::OutputCB& output_cb,
-                     const base::Closure& waiting_for_decryption_key_cb);
+  // Initialize with stream parameters. Should be called exactly once.
+  void Initialize(StreamTraits* traits,
+                  DemuxerStream* stream,
+                  CdmContext* cdm_context,
+                  base::RepeatingClosure waiting_for_decryption_key_cb);
+
+  // Selects and initializes a decoder, which will be returned via
+  // |select_decoder_cb| posted to |task_runner|. Subsequent calls to
+  // SelectDecoder() will return different decoder instances, until all
+  // potential decoders have been exhausted.
+  //
+  // When the caller determines that decoder selection has succeeded (eg.
+  // because the decoder decoded a frame successfully), it should call
+  // FinalizeDecoderSelection().
+  //
+  // Must not be called while another selection is pending.
+  void SelectDecoder(SelectDecoderCB select_decoder_cb,
+                     typename Decoder::OutputCB output_cb);
+
+  // Signals that decoder selection has been completed (successfully). Future
+  // calls to SelectDecoder() will select from the full list of decoders.
+  void FinalizeDecoderSelection();
 
  private:
   void InitializeDecoder();
-  void DecoderInitDone(bool success);
+  void OnDecoderInitializeDone(bool success);
   void ReturnNullDecoder();
   void InitializeDecryptingDemuxerStream();
-  void DecryptingDemuxerStreamInitDone(PipelineStatus status);
+  void OnDecryptingDemuxerStreamInitializeDone(PipelineStatus status);
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   CreateDecodersCB create_decoders_cb_;
   MediaLog* media_log_;
 
-  StreamTraits* traits_;
+  StreamTraits* traits_ = nullptr;
+  DemuxerStream* stream_ = nullptr;
+  CdmContext* cdm_context_ = nullptr;
+  base::RepeatingClosure waiting_for_decryption_key_cb_;
 
-  // Could be the |stream| passed in SelectDecoder, or |decrypted_stream_| when
-  // a DecryptingDemuxerStream is selected.
-  DemuxerStream* input_stream_ = nullptr;
+  // Overall decoder selection state.
+  DecoderConfig config_;
+  bool is_selecting_decoders_ = false;
+  std::vector<std::unique_ptr<Decoder>> decoders_;
 
-  CdmContext* cdm_context_;
-  std::string blacklisted_decoder_;
+  // State for a single SelectDecoder() invocation.
   SelectDecoderCB select_decoder_cb_;
   typename Decoder::OutputCB output_cb_;
-  base::Closure waiting_for_decryption_key_cb_;
-
-  std::vector<std::unique_ptr<Decoder>> decoders_;
-
   std::unique_ptr<Decoder> decoder_;
-  std::unique_ptr<DecryptingDemuxerStream> decrypted_stream_;
-
-  // Config of the |input_stream| used to initialize decoders.
-  DecoderConfig config_;
-
-  // Indicates if we tried to initialize |decrypted_stream_|.
-  bool tried_decrypting_demuxer_stream_ = false;
+  std::unique_ptr<DecryptingDemuxerStream> decrypting_demuxer_stream_;
 
-  // NOTE: Weak pointers must be invalidated before all other member variables.
-  base::WeakPtrFactory<DecoderSelector> weak_ptr_factory_;
+  base::WeakPtrFactory<DecoderSelector> weak_this_factory_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(DecoderSelector);
 };

media/filters/decoder_stream.cc

@@ -48,13 +48,12 @@ DecoderStream<StreamType>::DecoderStream(
     MediaLog* media_log)
     : traits_(std::move(traits)),
       task_runner_(task_runner),
-      create_decoders_cb_(std::move(create_decoders_cb)),
       media_log_(media_log),
       state_(STATE_UNINITIALIZED),
       stream_(nullptr),
       cdm_context_(nullptr),
       decoder_produced_a_frame_(false),
-      has_fallen_back_once_on_decode_error_(false),
+      decoder_selector_(task_runner, std::move(create_decoders_cb), media_log),
       decoding_eos_(false),
       preparing_output_(false),
       pending_decode_requests_(0),
@@ -110,9 +109,11 @@ void DecoderStream<StreamType>::Initialize(
   init_cb_ = std::move(init_cb);
   cdm_context_ = cdm_context;
   statistics_cb_ = std::move(statistics_cb);
-  waiting_for_decryption_key_cb_ = std::move(waiting_for_decryption_key_cb);
+  waiting_for_decryption_key_cb_ = waiting_for_decryption_key_cb;
 
   traits_->OnStreamReset(stream_);
+  decoder_selector_.Initialize(traits_.get(), stream, cdm_context,
+                               std::move(waiting_for_decryption_key_cb));
 
   state_ = STATE_INITIALIZING;
   SelectDecoder();
@@ -266,23 +267,11 @@ void DecoderStream<StreamType>::SkipPrepareUntil(
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::SelectDecoder() {
-  // If we are already using DecryptingDemuxerStream (DDS), e.g. during
-  // fallback, the |stream_| will always be clear. In this case, no need pass in
-  // the |cdm_context_|. This will also help prevent creating a new DDS on top
-  // of the current DDS.
-  CdmContext* cdm_context = decrypting_demuxer_stream_ ? nullptr : cdm_context_;
-  std::string blacklisted_decoder = decoder_ ? decoder_->GetDisplayName() : "";
-
-  decoder_selector_ = std::make_unique<DecoderSelector<StreamType>>(
-      task_runner_, create_decoders_cb_, media_log_);
-
-  decoder_selector_->SelectDecoder(
-      traits_.get(), stream_, cdm_context, blacklisted_decoder,
+  decoder_selector_.SelectDecoder(
       base::BindRepeating(&DecoderStream<StreamType>::OnDecoderSelected,
                           weak_factory_.GetWeakPtr()),
       base::BindRepeating(&DecoderStream<StreamType>::OnDecodeOutputReady,
-                          fallback_weak_factory_.GetWeakPtr()),
-      waiting_for_decryption_key_cb_);
+                          fallback_weak_factory_.GetWeakPtr()));
 }
 
 template <DemuxerStream::Type StreamType>
@@ -296,8 +285,6 @@ void DecoderStream<StreamType>::OnDecoderSelected(
   DCHECK(state_ == STATE_INITIALIZING || state_ == STATE_REINITIALIZING_DECODER)
       << state_;
 
-  decoder_selector_.reset();
-
   if (state_ == STATE_INITIALIZING) {
     DCHECK(init_cb_);
     DCHECK(!read_cb_);
@@ -310,6 +297,9 @@ void DecoderStream<StreamType>::OnDecoderSelected(
   if (decrypting_demuxer_stream) {
     decrypting_demuxer_stream_ = std::move(decrypting_demuxer_stream);
     stream_ = decrypting_demuxer_stream_.get();
+    // Also clear |cdm_context_|, it shouldn't be passed during reinitialize for
+    // a sream that isn't encrypted.
+    cdm_context_ = nullptr;
   }
   if (decoder_change_observer_cb_)
     decoder_change_observer_cb_.Run(decoder_.get());
@@ -463,11 +453,7 @@ void DecoderStream<StreamType>::OnDecodeDone(int buffer_size,
 
   switch (status) {
     case DecodeStatus::DECODE_ERROR:
-      // Only fall back to a new decoder after failing to decode the first
-      // buffer, and if we have not fallen back before.
-      if (!decoder_produced_a_frame_ &&
-          !has_fallen_back_once_on_decode_error_) {
-        has_fallen_back_once_on_decode_error_ = true;
+      if (!decoder_produced_a_frame_) {
         pending_decode_requests_ = 0;
 
         // Prevent all pending decode requests and outputs from those requests
@@ -541,10 +527,13 @@ void DecoderStream<StreamType>::OnDecodeOutputReady(
   // fallback decoder.
   // Note: |fallback_buffers_| might still have buffers, and we will keep
   // reading from there before requesting new buffers from |stream_|.
-  pending_buffers_.clear();
+  if (!decoder_produced_a_frame_) {
+    decoder_produced_a_frame_ = true;
+    decoder_selector_.FinalizeDecoderSelection();
+    pending_buffers_.clear();
+  }
 
   // If the frame should be dropped, exit early and decode another frame.
-  decoder_produced_a_frame_ = true;
   if (traits_->OnDecodeDone(output) == PostDecodeAction::DROP)
     return;
 
@@ -740,7 +729,9 @@ void DecoderStream<StreamType>::ReinitializeDecoder() {
   DCHECK_EQ(pending_decode_requests_, 0);
 
   state_ = STATE_REINITIALIZING_DECODER;
-  // Decoders should not need a new CDM during reinitialization.
+
+  // TODO(sandersd): Detect whether a new decoder is required before
+  // attempting reinitialization.
   traits_->InitializeDecoder(
       decoder_.get(), traits_->GetDecoderConfig(stream_),
       stream_->liveness() == DemuxerStream::LIVENESS_LIVE, cdm_context_,

media/filters/decoder_stream.h

@@ -216,7 +216,6 @@ class MEDIA_EXPORT DecoderStream {
   std::unique_ptr<DecoderStreamTraits<StreamType>> traits_;
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
-  CreateDecodersCB create_decoders_cb_;
   MediaLog* media_log_;
 
   State state_;
@@ -237,20 +236,11 @@ class MEDIA_EXPORT DecoderStream {
   // Whether |decoder_| has produced a frame yet. Reset on fallback.
   bool decoder_produced_a_frame_;
 
-  // Whether we have already fallen back once on decode error, used to prevent
-  // issues like infinite fallback like:
-  // 1. select decoder 1
-  // 2. decode error on decoder 1
-  // 3. black list decoder 1 and select decoder 2
-  // 4. decode error again on decoder 2
-  // 5. black list decoder 2 and select decoder 1
-  // 6. go to (2)
-  bool has_fallen_back_once_on_decode_error_;
-
   std::unique_ptr<DecryptingDemuxerStream> decrypting_demuxer_stream_;
 
-  // Destruct before |decrypting_demuxer_stream_| or |decoder_|.
-  std::unique_ptr<DecoderSelector<StreamType>> decoder_selector_;
+  // Note: Holds pointers to |traits_|, |stream_|, |decrypting_demuxer_stream_|,
+  // and |cdm_context_|.
+  DecoderSelector<StreamType> decoder_selector_;
 
   ConfigChangeObserverCB config_change_observer_cb_;
   DecoderChangeObserverCB decoder_change_observer_cb_;

media/filters/fake_video_decoder.cc

@@ -48,6 +48,10 @@ void FakeVideoDecoder::EnableEncryptedConfigSupport() {
   supports_encrypted_config_ = true;
 }
 
+base::WeakPtr<FakeVideoDecoder> FakeVideoDecoder::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 std::string FakeVideoDecoder::GetDisplayName() const {
   return decoder_name_;
 }

media/filters/fake_video_decoder.h

@@ -57,6 +57,8 @@ class FakeVideoDecoder : public VideoDecoder {
   void Reset(const base::Closure& closure) override;
   int GetMaxDecodeRequests() const override;
 
+  base::WeakPtr<FakeVideoDecoder> GetWeakPtr();
+
   // Holds the next init/decode/reset callback from firing.
   void HoldNextInit();
   void HoldDecode();

media/filters/video_frame_stream_unittest.cc

@@ -179,9 +179,8 @@ class VideoFrameStreamTest
       if (GetParam().is_encrypted && !GetParam().has_decryptor)
         decoder->EnableEncryptedConfigSupport();
 
-      // Keep a copy of the raw pointers so we can change the behavior of each
-      // decoder.
-      decoders_.push_back(decoder.get());
+      // Keep a reference so we can change the behavior of each decoder.
+      decoders_.push_back(decoder->GetWeakPtr());
 
       decoders.push_back(std::move(decoder));
     }
@@ -208,12 +207,20 @@ class VideoFrameStreamTest
   // |decoder_indices|.
   void FailDecoderInitOnSelection(const std::vector<int>& decoder_indices) {
     decoder_indices_to_fail_init_ = decoder_indices;
+    for (int i : decoder_indices) {
+      if (!decoders_.empty() && decoders_[i] && decoders_[i].get() != decoder_)
+        decoders_[i]->SimulateFailureToInit();
+    }
   }
 
   // On next decoder selection, hold initialization on decoders specified by
   // |decoder_indices|.
   void HoldDecoderInitOnSelection(const std::vector<int>& decoder_indices) {
     decoder_indices_to_hold_init_ = decoder_indices;
+    for (int i : decoder_indices) {
+      if (!decoders_.empty() && decoders_[i] && decoders_[i].get() != decoder_)
+        decoders_[i]->HoldNextInit();
+    }
   }
 
   // After next decoder selection, hold decode on decoders specified by
@@ -221,6 +228,10 @@ class VideoFrameStreamTest
   // may be resumed immediately and it'll be too late to hold decode then.
   void HoldDecodeAfterSelection(const std::vector<int>& decoder_indices) {
     decoder_indices_to_hold_decode_ = decoder_indices;
+    for (int i : decoder_indices) {
+      if (!decoders_.empty() && decoders_[i] && decoders_[i].get() != decoder_)
+        decoders_[i]->HoldDecode();
+    }
   }
 
   // Updates the |decoder_| currently being used by VideoFrameStream.
@@ -458,10 +469,10 @@ class VideoFrameStreamTest
   // e.g. RegisterNewKeyCB().
   std::unique_ptr<NiceMock<MockDecryptor>> decryptor_;
 
-  // Raw pointers to the list of decoders to be select from by DecoderSelector.
+  // References to the list of decoders to be select from by DecoderSelector.
   // Three decoders are needed to test that decoder fallback can occur more than
   // once on a config change. They are owned by |video_frame_stream_|.
-  std::vector<FakeVideoDecoder*> decoders_;
+  std::vector<base::WeakPtr<FakeVideoDecoder>> decoders_;
 
   std::vector<int> decoder_indices_to_fail_init_;
   std::vector<int> decoder_indices_to_hold_init_;
@@ -968,25 +979,31 @@ TEST_P(VideoFrameStreamTest, FallbackDecoder_DoesReinitializeStompPendingRead) {
   ASSERT_GT(decoder_->total_bytes_decoded(), first_decoded_bytes);
 }
 
-TEST_P(VideoFrameStreamTest, FallbackDecoder_DecodeErrorTwice) {
+TEST_P(VideoFrameStreamTest, FallbackDecoder_DecodeErrorRepeated) {
   Initialize();
 
+  // Hold other decoders to simuate errors.
+  HoldDecodeAfterSelection({1, 2});
+
   // Simulate decode error to trigger the fallback path.
   decoder_->SimulateError();
-
-  // Decoder 0 should be blacklisted and never tried. Hold decode on decoder 1
-  // and simulate decode error again.
-  HoldDecodeAfterSelection({1});
   ReadOneFrame();
+  base::RunLoop().RunUntilIdle();
+
+  // Expect decoder 1 to be tried.
   ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
   decoder_->SimulateError();
-  decoder_->SatisfyDecode();
   base::RunLoop().RunUntilIdle();
 
-  // Only one fallback is allowed so we are not falling back to other decoders.
-  ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
+  // Then decoder 2.
+  ASSERT_EQ(GetDecoderName(2), decoder_->GetDisplayName());
+  decoder_->SimulateError();
+  base::RunLoop().RunUntilIdle();
+
+  // No decoders left, expect failure.
+  EXPECT_EQ(decoder_, nullptr);
   EXPECT_FALSE(pending_read_);
-  ASSERT_EQ(VideoFrameStream::DECODE_ERROR, last_read_status_);
+  EXPECT_EQ(VideoFrameStream::DECODE_ERROR, last_read_status_);
 }
 
 // This tests verifies that we properly fallback to a new decoder if the first
@@ -1029,40 +1046,46 @@ TEST_P(VideoFrameStreamTest,
   ReadOneFrame();
 
   // Verify that fallback happened.
-  EXPECT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
+  EXPECT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
   EXPECT_FALSE(pending_read_);
   EXPECT_EQ(VideoFrameStream::OK, last_read_status_);
   EXPECT_GT(decoder_->total_bytes_decoded(), 0);
 }
 
 TEST_P(VideoFrameStreamTest,
-       FallbackDecoder_DecodeErrorTwice_AfterReinitialization) {
+       FallbackDecoder_DecodeErrorRepeated_AfterReinitialization) {
   Initialize();
 
-  // Simulate decode error to trigger the fallback path.
+  // Simulate decode error to trigger fallback.
   decoder_->SimulateError();
   ReadOneFrame();
-  ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
+  base::RunLoop().RunUntilIdle();
 
   // Simulate reinitialize error of decoder 1.
+  ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
   decoder_->SimulateFailureToInit();
-
-  // Decoder 0 should be selected again. Simulate immediate decode error after
-  // reinitialization.
-  HoldDecodeAfterSelection({0});
+  HoldDecodeAfterSelection({0, 1, 2});
   ReadUntilDecoderReinitialized();
+
+  // Decoder 0 should be selected again.
   ASSERT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
   decoder_->SimulateError();
-  decoder_->SatisfyDecode();
   base::RunLoop().RunUntilIdle();
 
-  // VideoDecoderStream has produced video frames, so we are not trying fallback
-  // again.
-  // TODO(xhwang): Revisit this behavior, e.g. always try to fallback if a newly
-  // selected decoder has not produced any video frames before.
-  ASSERT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
+  // Decoder 1.
+  ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
+  decoder_->SimulateError();
+  base::RunLoop().RunUntilIdle();
+
+  // Decoder 2.
+  ASSERT_EQ(GetDecoderName(2), decoder_->GetDisplayName());
+  decoder_->SimulateError();
+  base::RunLoop().RunUntilIdle();
+
+  // No decoders left.
+  EXPECT_EQ(decoder_, nullptr);
   EXPECT_FALSE(pending_read_);
-  ASSERT_EQ(VideoFrameStream::DECODE_ERROR, last_read_status_);
+  EXPECT_EQ(VideoFrameStream::DECODE_ERROR, last_read_status_);
 }
 
 TEST_P(VideoFrameStreamTest, FallbackDecoder_ConfigChangeClearsPendingBuffers) {
@@ -1232,8 +1255,8 @@ TEST_P(VideoFrameStreamTest, FallbackDecoder_SelectedOnInitThenDecodeErrors) {
   decoder_->SimulateError();
   base::RunLoop().RunUntilIdle();
 
-  // |video_frame_stream_| should have fallen back to decoder 0.
-  ASSERT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
+  // |video_frame_stream_| should have fallen back to decoder 2.
+  ASSERT_EQ(GetDecoderName(2), decoder_->GetDisplayName());
 
   ASSERT_FALSE(pending_read_);
   ASSERT_EQ(VideoFrameStream::OK, last_read_status_);
@@ -1298,7 +1321,8 @@ TEST_P(VideoFrameStreamTest, ReinitializeFailure_Once) {
   Initialize();
   decoder_->SimulateFailureToInit();
   ReadUntilDecoderReinitialized();
-  ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
+  // Should have fallen back to a new instance of decoder 0.
+  ASSERT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
   ReadAllFrames();
   ASSERT_GT(decoder_->total_bytes_decoded(), 0);
 }
@@ -1306,14 +1330,15 @@ TEST_P(VideoFrameStreamTest, ReinitializeFailure_Once) {
 TEST_P(VideoFrameStreamTest, ReinitializeFailure_Twice) {
   Initialize();
 
-  // Trigger reinitialization error, and fallback to decoder 1.
+  // Trigger reinitialization error, and fallback to a new instance.
   decoder_->SimulateFailureToInit();
   ReadUntilDecoderReinitialized();
-  ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
+  ASSERT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
 
   ReadOneFrame();
 
-  // Trigger reinitialization error again, and fallback back to decoder 0.
+  // Trigger reinitialization error again. Since a frame was output, this will
+  // be a new instance of decoder 0 again.
   decoder_->SimulateFailureToInit();
   ReadUntilDecoderReinitialized();
   ASSERT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
@@ -1323,11 +1348,11 @@ TEST_P(VideoFrameStreamTest, ReinitializeFailure_Twice) {
 TEST_P(VideoFrameStreamTest, ReinitializeFailure_OneUnsupportedDecoder) {
   Initialize();
 
-  // The current decoder will fail to reinitialize and will be blacklisted.
+  // The current decoder will fail to reinitialize.
   decoder_->SimulateFailureToInit();
 
   // Decoder 1 will also fail to initialize on decoder selection.
-  FailDecoderInitOnSelection({1});
+  FailDecoderInitOnSelection({0, 1});
 
   ReadUntilDecoderReinitialized();
 
@@ -1340,11 +1365,12 @@ TEST_P(VideoFrameStreamTest, ReinitializeFailure_OneUnsupportedDecoder) {
 TEST_P(VideoFrameStreamTest, ReinitializeFailure_NoSupportedDecoder) {
   Initialize();
 
-  // The current decoder will fail to reinitialize and will be blacklisted.
+  // The current decoder will fail to reinitialize, triggering decoder
+  // selection.
   decoder_->SimulateFailureToInit();
 
-  // Decoder 1 and 2 will also fail to initialize on decoder selection.
-  FailDecoderInitOnSelection({1, 2});
+  // All of the decoders will fail in decoder selection.
+  FailDecoderInitOnSelection({0, 1, 2});
 
   ReadUntilDecoderReinitialized();