Revert "[media] Maintain DecoderSelector state throughout decoder selection."

This reverts commit 9e96a32b.

Reason for revert: Likely culprit of https://crbug.com/879469.

Original change's description:
> [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}

TBR=dalecurtis@chromium.org,xhwang@chromium.org,sandersd@chromium.org

Change-Id: I6836b3e04b56591289fd973a5e89092b70e923fe
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
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
Reviewed-on: https://chromium-review.googlesource.com/1199082Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587959}

media/base/callback_holder.h

@@ -17,6 +17,8 @@ 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,12 +200,6 @@ 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,7 +110,6 @@ 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,6 +271,7 @@ 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,8 +26,9 @@ class CdmContext;
 class DecryptingDemuxerStream;
 class MediaLog;
 
-// DecoderSelector handles construction and initialization of Decoders for a
-// DemuxerStream, and maintains the state required for decoder fallback.
+// 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.
 // The template parameter |StreamType| is the type of stream we will be
 // selecting a decoder for.
 template<DemuxerStream::Type StreamType>
@@ -43,76 +44,87 @@ class MEDIA_EXPORT DecoderSelector {
   using CreateDecodersCB =
       base::RepeatingCallback<std::vector<std::unique_ptr<Decoder>>()>;
 
-  // 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.
-  //
+  // 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.
   // The caller should call DecryptingDemuxerStream::Reset() before
   // calling Decoder::Reset() to release any pending decryption or read.
   using SelectDecoderCB =
-      base::OnceCallback<void(std::unique_ptr<Decoder>,
+      base::Callback<void(std::unique_ptr<Decoder>,
                           std::unique_ptr<DecryptingDemuxerStream>)>;
 
-  DecoderSelector(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+  DecoderSelector(
+      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
       CreateDecodersCB create_decoders_cb,
       MediaLog* media_log);
 
-  // Aborts any pending decoder selection.
+  // Aborts pending Decoder selection and fires |select_decoder_cb| with
+  // null and null immediately if it's pending.
   ~DecoderSelector();
 
-  // Initialize with stream parameters. Should be called exactly once.
-  void Initialize(StreamTraits* traits,
+  // 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,
-                  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();
+                     const std::string& blacklisted_decoder,
+                     const SelectDecoderCB& select_decoder_cb,
+                     const typename Decoder::OutputCB& output_cb,
+                     const base::Closure& waiting_for_decryption_key_cb);
 
  private:
   void InitializeDecoder();
-  void OnDecoderInitializeDone(bool success);
+  void DecoderInitDone(bool success);
   void ReturnNullDecoder();
   void InitializeDecryptingDemuxerStream();
-  void OnDecryptingDemuxerStreamInitializeDone(PipelineStatus status);
+  void DecryptingDemuxerStreamInitDone(PipelineStatus status);
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   CreateDecodersCB create_decoders_cb_;
   MediaLog* media_log_;
 
-  StreamTraits* traits_ = nullptr;
-  DemuxerStream* stream_ = nullptr;
-  CdmContext* cdm_context_ = nullptr;
-  base::RepeatingClosure waiting_for_decryption_key_cb_;
+  StreamTraits* traits_;
 
-  // Overall decoder selection state.
-  DecoderConfig config_;
-  bool is_selecting_decoders_ = false;
-  std::vector<std::unique_ptr<Decoder>> decoders_;
+  // Could be the |stream| passed in SelectDecoder, or |decrypted_stream_| when
+  // a DecryptingDemuxerStream is selected.
+  DemuxerStream* input_stream_ = nullptr;
 
-  // State for a single SelectDecoder() invocation.
+  CdmContext* cdm_context_;
+  std::string blacklisted_decoder_;
   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> decrypting_demuxer_stream_;
+  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;
 
-  base::WeakPtrFactory<DecoderSelector> weak_this_factory_;
+  // NOTE: Weak pointers must be invalidated before all other member variables.
+  base::WeakPtrFactory<DecoderSelector> weak_ptr_factory_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(DecoderSelector);
 };

media/filters/decoder_stream.cc

@@ -48,12 +48,13 @@ 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),
-      decoder_selector_(task_runner, std::move(create_decoders_cb), media_log),
+      has_fallen_back_once_on_decode_error_(false),
       decoding_eos_(false),
       preparing_output_(false),
       pending_decode_requests_(0),
@@ -109,11 +110,9 @@ 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_ = waiting_for_decryption_key_cb;
+  waiting_for_decryption_key_cb_ = std::move(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();
@@ -267,11 +266,23 @@ void DecoderStream<StreamType>::SkipPrepareUntil(
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::SelectDecoder() {
-  decoder_selector_.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,
       base::BindRepeating(&DecoderStream<StreamType>::OnDecoderSelected,
                           weak_factory_.GetWeakPtr()),
       base::BindRepeating(&DecoderStream<StreamType>::OnDecodeOutputReady,
-                          fallback_weak_factory_.GetWeakPtr()));
+                          fallback_weak_factory_.GetWeakPtr()),
+      waiting_for_decryption_key_cb_);
 }
 
 template <DemuxerStream::Type StreamType>
@@ -285,6 +296,8 @@ 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_);
@@ -297,9 +310,6 @@ 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());
@@ -453,7 +463,11 @@ void DecoderStream<StreamType>::OnDecodeDone(int buffer_size,
 
   switch (status) {
     case DecodeStatus::DECODE_ERROR:
-      if (!decoder_produced_a_frame_) {
+      // 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;
         pending_decode_requests_ = 0;
 
         // Prevent all pending decode requests and outputs from those requests
@@ -527,13 +541,10 @@ 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_|.
-  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;
 
@@ -729,9 +740,7 @@ void DecoderStream<StreamType>::ReinitializeDecoder() {
   DCHECK_EQ(pending_decode_requests_, 0);
 
   state_ = STATE_REINITIALIZING_DECODER;
-
-  // TODO(sandersd): Detect whether a new decoder is required before
-  // attempting reinitialization.
+  // Decoders should not need a new CDM during reinitialization.
   traits_->InitializeDecoder(
       decoder_.get(), traits_->GetDecoderConfig(stream_),
       stream_->liveness() == DemuxerStream::LIVENESS_LIVE, cdm_context_,

media/filters/decoder_stream.h

@@ -216,6 +216,7 @@ 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_;
@@ -236,11 +237,20 @@ 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_;
 
-  // Note: Holds pointers to |traits_|, |stream_|, |decrypting_demuxer_stream_|,
-  // and |cdm_context_|.
-  DecoderSelector<StreamType> decoder_selector_;
+  // Destruct before |decrypting_demuxer_stream_| or |decoder_|.
+  std::unique_ptr<DecoderSelector<StreamType>> decoder_selector_;
 
   ConfigChangeObserverCB config_change_observer_cb_;
   DecoderChangeObserverCB decoder_change_observer_cb_;

media/filters/fake_video_decoder.cc

@@ -48,10 +48,6 @@ 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,8 +57,6 @@ 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,8 +179,9 @@ class VideoFrameStreamTest
       if (GetParam().is_encrypted && !GetParam().has_decryptor)
         decoder->EnableEncryptedConfigSupport();
 
-      // Keep a reference so we can change the behavior of each decoder.
-      decoders_.push_back(decoder->GetWeakPtr());
+      // Keep a copy of the raw pointers so we can change the behavior of each
+      // decoder.
+      decoders_.push_back(decoder.get());
 
       decoders.push_back(std::move(decoder));
     }
@@ -207,20 +208,12 @@ 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
@@ -228,10 +221,6 @@ 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.
@@ -469,10 +458,10 @@ class VideoFrameStreamTest
   // e.g. RegisterNewKeyCB().
   std::unique_ptr<NiceMock<MockDecryptor>> decryptor_;
 
-  // References to the list of decoders to be select from by DecoderSelector.
+  // Raw pointers 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<base::WeakPtr<FakeVideoDecoder>> decoders_;
+  std::vector<FakeVideoDecoder*> decoders_;
 
   std::vector<int> decoder_indices_to_fail_init_;
   std::vector<int> decoder_indices_to_hold_init_;
@@ -979,31 +968,25 @@ TEST_P(VideoFrameStreamTest, FallbackDecoder_DoesReinitializeStompPendingRead) {
   ASSERT_GT(decoder_->total_bytes_decoded(), first_decoded_bytes);
 }
 
-TEST_P(VideoFrameStreamTest, FallbackDecoder_DecodeErrorRepeated) {
+TEST_P(VideoFrameStreamTest, FallbackDecoder_DecodeErrorTwice) {
   Initialize();
 
-  // Hold other decoders to simuate errors.
-  HoldDecodeAfterSelection({1, 2});
-
   // Simulate decode error to trigger the fallback path.
   decoder_->SimulateError();
-  ReadOneFrame();
-  base::RunLoop().RunUntilIdle();
 
-  // Expect decoder 1 to be tried.
+  // Decoder 0 should be blacklisted and never tried. Hold decode on decoder 1
+  // and simulate decode error again.
+  HoldDecodeAfterSelection({1});
+  ReadOneFrame();
   ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
   decoder_->SimulateError();
+  decoder_->SatisfyDecode();
   base::RunLoop().RunUntilIdle();
 
-  // Then decoder 2.
-  ASSERT_EQ(GetDecoderName(2), decoder_->GetDisplayName());
-  decoder_->SimulateError();
-  base::RunLoop().RunUntilIdle();
-
-  // No decoders left, expect failure.
-  EXPECT_EQ(decoder_, nullptr);
+  // Only one fallback is allowed so we are not falling back to other decoders.
+  ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
   EXPECT_FALSE(pending_read_);
-  EXPECT_EQ(VideoFrameStream::DECODE_ERROR, last_read_status_);
+  ASSERT_EQ(VideoFrameStream::DECODE_ERROR, last_read_status_);
 }
 
 // This tests verifies that we properly fallback to a new decoder if the first
@@ -1046,46 +1029,40 @@ TEST_P(VideoFrameStreamTest,
   ReadOneFrame();
 
   // Verify that fallback happened.
-  EXPECT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
+  EXPECT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
   EXPECT_FALSE(pending_read_);
   EXPECT_EQ(VideoFrameStream::OK, last_read_status_);
   EXPECT_GT(decoder_->total_bytes_decoded(), 0);
 }
 
 TEST_P(VideoFrameStreamTest,
-       FallbackDecoder_DecodeErrorRepeated_AfterReinitialization) {
+       FallbackDecoder_DecodeErrorTwice_AfterReinitialization) {
   Initialize();
 
-  // Simulate decode error to trigger fallback.
+  // Simulate decode error to trigger the fallback path.
   decoder_->SimulateError();
   ReadOneFrame();
-  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
 
   // Simulate reinitialize error of decoder 1.
-  ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
   decoder_->SimulateFailureToInit();
-  HoldDecodeAfterSelection({0, 1, 2});
-  ReadUntilDecoderReinitialized();
 
-  // Decoder 0 should be selected again.
+  // Decoder 0 should be selected again. Simulate immediate decode error after
+  // reinitialization.
+  HoldDecodeAfterSelection({0});
+  ReadUntilDecoderReinitialized();
   ASSERT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
   decoder_->SimulateError();
+  decoder_->SatisfyDecode();
   base::RunLoop().RunUntilIdle();
 
-  // 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);
+  // 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());
   EXPECT_FALSE(pending_read_);
-  EXPECT_EQ(VideoFrameStream::DECODE_ERROR, last_read_status_);
+  ASSERT_EQ(VideoFrameStream::DECODE_ERROR, last_read_status_);
 }
 
 TEST_P(VideoFrameStreamTest, FallbackDecoder_ConfigChangeClearsPendingBuffers) {
@@ -1255,8 +1232,8 @@ TEST_P(VideoFrameStreamTest, FallbackDecoder_SelectedOnInitThenDecodeErrors) {
   decoder_->SimulateError();
   base::RunLoop().RunUntilIdle();
 
-  // |video_frame_stream_| should have fallen back to decoder 2.
-  ASSERT_EQ(GetDecoderName(2), decoder_->GetDisplayName());
+  // |video_frame_stream_| should have fallen back to decoder 0.
+  ASSERT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
 
   ASSERT_FALSE(pending_read_);
   ASSERT_EQ(VideoFrameStream::OK, last_read_status_);
@@ -1321,8 +1298,7 @@ TEST_P(VideoFrameStreamTest, ReinitializeFailure_Once) {
   Initialize();
   decoder_->SimulateFailureToInit();
   ReadUntilDecoderReinitialized();
-  // Should have fallen back to a new instance of decoder 0.
-  ASSERT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
+  ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
   ReadAllFrames();
   ASSERT_GT(decoder_->total_bytes_decoded(), 0);
 }
@@ -1330,15 +1306,14 @@ TEST_P(VideoFrameStreamTest, ReinitializeFailure_Once) {
 TEST_P(VideoFrameStreamTest, ReinitializeFailure_Twice) {
   Initialize();
 
-  // Trigger reinitialization error, and fallback to a new instance.
+  // Trigger reinitialization error, and fallback to decoder 1.
   decoder_->SimulateFailureToInit();
   ReadUntilDecoderReinitialized();
-  ASSERT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
+  ASSERT_EQ(GetDecoderName(1), decoder_->GetDisplayName());
 
   ReadOneFrame();
 
-  // Trigger reinitialization error again. Since a frame was output, this will
-  // be a new instance of decoder 0 again.
+  // Trigger reinitialization error again, and fallback back to decoder 0.
   decoder_->SimulateFailureToInit();
   ReadUntilDecoderReinitialized();
   ASSERT_EQ(GetDecoderName(0), decoder_->GetDisplayName());
@@ -1348,11 +1323,11 @@ TEST_P(VideoFrameStreamTest, ReinitializeFailure_Twice) {
 TEST_P(VideoFrameStreamTest, ReinitializeFailure_OneUnsupportedDecoder) {
   Initialize();
 
-  // The current decoder will fail to reinitialize.
+  // The current decoder will fail to reinitialize and will be blacklisted.
   decoder_->SimulateFailureToInit();
 
   // Decoder 1 will also fail to initialize on decoder selection.
-  FailDecoderInitOnSelection({0, 1});
+  FailDecoderInitOnSelection({1});
 
   ReadUntilDecoderReinitialized();
 
@@ -1365,12 +1340,11 @@ TEST_P(VideoFrameStreamTest, ReinitializeFailure_OneUnsupportedDecoder) {
 TEST_P(VideoFrameStreamTest, ReinitializeFailure_NoSupportedDecoder) {
   Initialize();
 
-  // The current decoder will fail to reinitialize, triggering decoder
-  // selection.
+  // The current decoder will fail to reinitialize and will be blacklisted.
   decoder_->SimulateFailureToInit();
 
-  // All of the decoders will fail in decoder selection.
-  FailDecoderInitOnSelection({0, 1, 2});
+  // Decoder 1 and 2 will also fail to initialize on decoder selection.
+  FailDecoderInitOnSelection({1, 2});
 
   ReadUntilDecoderReinitialized();