media: Support better decoder switching

Today on reinitialization error, or decode error of the first buffer,
DecoderStream will fall back to decoders left in the DecoderSelector.
Since we will have less and less decoders in the DecoderSelector, the
ability to switch decoders is limited.

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Blacklist

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fall back at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinite
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.

Review-Url: https://codereview.chromium.org/2837613004
Cr-Commit-Position: refs/heads/master@{#469579}

media/base/audio_decoder.h

@@ -43,7 +43,11 @@ class MEDIA_EXPORT AudioDecoder {
   // depends on |this|.
   virtual ~AudioDecoder();
 
-  // Returns the name of the decoder for logging purpose.
+  // Returns the name of the decoder for logging and decoder selection purposes.
+  // This name should be available immediately after construction (e.g. before
+  // Initialize() is called). It should also be stable in the sense that the
+  // name does not change across multiple constructions.
+  // TODO(xhwang): Rename this method since the name is not only for display.
   virtual std::string GetDisplayName() const = 0;
 
   // Initializes an AudioDecoder with |config|, executing the |init_cb| upon

media/base/audio_decoder_config.h

@@ -88,9 +88,7 @@ class MEDIA_EXPORT AudioDecoderConfig {
   }
 
   // Sets the config to be encrypted or not encrypted manually. This can be
-  // useful for decryptors that decrypts an encrypted stream to a clear stream,
-  // or for decoder selectors that wants to select decrypting decoders instead
-  // of clear decoders.
+  // useful for decryptors that decrypts an encrypted stream to a clear stream.
   void SetIsEncrypted(bool is_encrypted);
 
  private:

media/base/mock_filters.cc

@@ -88,24 +88,26 @@ VideoRotation MockDemuxerStream::video_rotation() {
   return VIDEO_ROTATION_0;
 }
 
-std::string MockVideoDecoder::GetDisplayName() const {
-  return "MockVideoDecoder";
-}
-
-MockVideoDecoder::MockVideoDecoder() {
+MockVideoDecoder::MockVideoDecoder(const std::string& decoder_name)
+    : decoder_name_(decoder_name) {
   ON_CALL(*this, CanReadWithoutStalling()).WillByDefault(Return(true));
 }
 
-std::string MockAudioDecoder::GetDisplayName() const {
-  return "MockAudioDecoder";
-}
-
 MockVideoDecoder::~MockVideoDecoder() {}
 
-MockAudioDecoder::MockAudioDecoder() {}
+std::string MockVideoDecoder::GetDisplayName() const {
+  return decoder_name_;
+}
+
+MockAudioDecoder::MockAudioDecoder(const std::string& decoder_name)
+    : decoder_name_(decoder_name) {}
 
 MockAudioDecoder::~MockAudioDecoder() {}
 
+std::string MockAudioDecoder::GetDisplayName() const {
+  return decoder_name_;
+}
+
 MockRendererClient::MockRendererClient() {}
 
 MockRendererClient::~MockRendererClient() {}

media/base/mock_filters.h

@@ -179,7 +179,8 @@ class MockDemuxerStream : public DemuxerStream {
 
 class MockVideoDecoder : public VideoDecoder {
  public:
-  MockVideoDecoder();
+  explicit MockVideoDecoder(
+      const std::string& decoder_name = "MockVideoDecoder");
   virtual ~MockVideoDecoder();
 
   // VideoDecoder implementation.
@@ -197,12 +198,14 @@ class MockVideoDecoder : public VideoDecoder {
   MOCK_CONST_METHOD0(CanReadWithoutStalling, bool());
 
  private:
+  std::string decoder_name_;
   DISALLOW_COPY_AND_ASSIGN(MockVideoDecoder);
 };
 
 class MockAudioDecoder : public AudioDecoder {
  public:
-  MockAudioDecoder();
+  explicit MockAudioDecoder(
+      const std::string& decoder_name = "MockAudioDecoder");
   virtual ~MockAudioDecoder();
 
   // AudioDecoder implementation.
@@ -218,6 +221,7 @@ class MockAudioDecoder : public AudioDecoder {
   MOCK_METHOD1(Reset, void(const base::Closure&));
 
  private:
+  std::string decoder_name_;
   DISALLOW_COPY_AND_ASSIGN(MockAudioDecoder);
 };
 

media/base/video_decoder.h

@@ -44,7 +44,10 @@ class MEDIA_EXPORT VideoDecoder {
   // depends on |this|.
   virtual ~VideoDecoder();
 
-  // Returns the name of the decoder for logging purpose.
+  // Returns the name of the decoder for logging and decoder selection purposes.
+  // This name should be available immediately after construction (e.g. before
+  // Initialize() is called). It should also be stable in the sense that the
+  // name does not change across multiple constructions.
   virtual std::string GetDisplayName() const = 0;
 
   // Initializes a VideoDecoder with the given |config|, executing the

media/base/video_decoder_config.h

@@ -120,9 +120,7 @@ class MEDIA_EXPORT VideoDecoderConfig {
   base::Optional<HDRMetadata> hdr_metadata() const;
 
   // Sets the config to be encrypted or not encrypted manually. This can be
-  // useful for decryptors that decrypts an encrypted stream to a clear stream,
-  // or for decoder selectors that wants to select decrypting decoders instead
-  // of clear decoders.
+  // useful for decryptors that decrypts an encrypted stream to a clear stream.
   void SetIsEncrypted(bool is_encrypted);
 
  private:

media/filters/audio_decoder_selector_unittest.cc

@@ -42,6 +42,9 @@ MATCHER(ClearConfig, "") {
 
 namespace media {
 
+const char kDecoder1[] = "Decoder1";
+const char kDecoder2[] = "Decoder2";
+
 class AudioDecoderSelectorTest : public ::testing::Test {
  public:
   enum DecryptorCapability {
@@ -55,8 +58,8 @@ class AudioDecoderSelectorTest : public ::testing::Test {
       : traits_(&media_log_),
         demuxer_stream_(
             new StrictMock<MockDemuxerStream>(DemuxerStream::AUDIO)),
-        decoder_1_(new StrictMock<MockAudioDecoder>()),
-        decoder_2_(new StrictMock<MockAudioDecoder>()) {
+        decoder_1_(new StrictMock<MockAudioDecoder>(kDecoder1)),
+        decoder_2_(new StrictMock<MockAudioDecoder>(kDecoder2)) {
     all_decoders_.push_back(decoder_1_);
     all_decoders_.push_back(decoder_2_);
     // |cdm_context_| and |decryptor_| are conditionally created in
@@ -88,6 +91,10 @@ class AudioDecoderSelectorTest : public ::testing::Test {
     demuxer_stream_->set_audio_decoder_config(encrypted_audio_config);
   }
 
+  ScopedVector<AudioDecoder> CreateVideoDecodersForTest() {
+    return std::move(all_decoders_);
+  }
+
   void InitializeDecoderSelector(DecryptorCapability decryptor_capability,
                                  int num_decoders) {
     if (decryptor_capability != kNoCdm) {
@@ -111,19 +118,25 @@ class AudioDecoderSelectorTest : public ::testing::Test {
         all_decoders_.begin() + num_decoders, all_decoders_.end());
 
     decoder_selector_.reset(new AudioDecoderSelector(
-        message_loop_.task_runner(), std::move(all_decoders_), &media_log_));
+        message_loop_.task_runner(),
+        base::Bind(&AudioDecoderSelectorTest::CreateVideoDecodersForTest,
+                   base::Unretained(this)),
+        &media_log_));
   }
 
-  void SelectDecoder() {
+  void SelectDecoderWithBlacklist(const std::string& blacklisted_decoder) {
     decoder_selector_->SelectDecoder(
         &traits_, demuxer_stream_.get(), cdm_context_.get(),
+        blacklisted_decoder,
         base::Bind(&AudioDecoderSelectorTest::MockOnDecoderSelected,
                    base::Unretained(this)),
         base::Bind(&AudioDecoderSelectorTest::OnDecoderOutput),
         base::Bind(&AudioDecoderSelectorTest::OnWaitingForDecryptionKey));
     base::RunLoop().RunUntilIdle();
   }
 
+  void SelectDecoder() { SelectDecoderWithBlacklist(""); }
+
   void SelectDecoderAndDestroy() {
     SelectDecoder();
 
@@ -168,18 +181,22 @@ class AudioDecoderSelectorTest : public ::testing::Test {
   DISALLOW_COPY_AND_ASSIGN(AudioDecoderSelectorTest);
 };
 
-// Tests for clear streams.
+// Tests for clear streams. The CDM will not be used for clear streams so
+// DecryptorCapability doesn't really matter.
 
-TEST_F(AudioDecoderSelectorTest, ClearStream_NoDecryptor_NoClearDecoder) {
+TEST_F(AudioDecoderSelectorTest, ClearStream_NoClearDecoder) {
   UseClearStream();
-  InitializeDecoderSelector(kNoDecryptor, 0);
+
+  // DecoderSelector will not try decrypting decoders for clear stream, even
+  // if the CDM is capable of doing decrypt and decode.
+  InitializeDecoderSelector(kDecryptAndDecode, 0);
 
   EXPECT_CALL(*this, OnDecoderSelected(IsNull(), IsNull()));
 
   SelectDecoder();
 }
 
-TEST_F(AudioDecoderSelectorTest, ClearStream_NoCdm_OneClearDecoder) {
+TEST_F(AudioDecoderSelectorTest, ClearStream_OneClearDecoder) {
   UseClearStream();
   InitializeDecoderSelector(kNoCdm, 1);
 
@@ -190,7 +207,7 @@ TEST_F(AudioDecoderSelectorTest, ClearStream_NoCdm_OneClearDecoder) {
   SelectDecoder();
 }
 
-TEST_F(AudioDecoderSelectorTest, Destroy_ClearStream_NoCdm_OneClearDecoder) {
+TEST_F(AudioDecoderSelectorTest, Destroy_ClearStream_OneClearDecoder) {
   UseClearStream();
   InitializeDecoderSelector(kNoCdm, 1);
 
@@ -199,7 +216,7 @@ TEST_F(AudioDecoderSelectorTest, Destroy_ClearStream_NoCdm_OneClearDecoder) {
   SelectDecoderAndDestroy();
 }
 
-TEST_F(AudioDecoderSelectorTest, ClearStream_NoCdm_MultipleClearDecoder) {
+TEST_F(AudioDecoderSelectorTest, ClearStream_MultipleClearDecoder) {
   UseClearStream();
   InitializeDecoderSelector(kNoCdm, 2);
 
@@ -212,8 +229,7 @@ TEST_F(AudioDecoderSelectorTest, ClearStream_NoCdm_MultipleClearDecoder) {
   SelectDecoder();
 }
 
-TEST_F(AudioDecoderSelectorTest,
-       Destroy_ClearStream_NoCdm_MultipleClearDecoder) {
+TEST_F(AudioDecoderSelectorTest, Destroy_ClearStream_MultipleClearDecoder) {
   UseClearStream();
   InitializeDecoderSelector(kNoCdm, 2);
 
@@ -224,90 +240,16 @@ TEST_F(AudioDecoderSelectorTest,
   SelectDecoderAndDestroy();
 }
 
-TEST_F(AudioDecoderSelectorTest, ClearStream_NoDecryptor_OneClearDecoder) {
+TEST_F(AudioDecoderSelectorTest, ClearStream_BlackListedDecoder) {
   UseClearStream();
-  InitializeDecoderSelector(kNoDecryptor, 1);
-
-  EXPECT_CALL(*decoder_1_, Initialize(EncryptedConfig(), _, _, _))
-      .WillOnce(RunCallback<2>(false));
-  EXPECT_CALL(*this, OnDecoderSelected(IsNull(), IsNull()));
-
-  SelectDecoder();
-}
-
-TEST_F(AudioDecoderSelectorTest,
-       Destroy_ClearStream_NoDecryptor_OneClearDecoder) {
-  UseClearStream();
-  InitializeDecoderSelector(kNoDecryptor, 1);
-
-  EXPECT_CALL(*decoder_1_, Initialize(EncryptedConfig(), _, _, _));
-
-  SelectDecoderAndDestroy();
-}
-
-TEST_F(AudioDecoderSelectorTest, ClearStream_NoDecryptor_MultipleClearDecoder) {
-  UseClearStream();
-  InitializeDecoderSelector(kNoDecryptor, 2);
+  InitializeDecoderSelector(kNoCdm, 2);
 
-  EXPECT_CALL(*decoder_1_, Initialize(EncryptedConfig(), _, _, _))
-      .WillOnce(RunCallback<2>(false));
-  EXPECT_CALL(*decoder_2_, Initialize(EncryptedConfig(), _, _, _))
+  // Decoder 1 is blacklisted and will not even be tried.
+  EXPECT_CALL(*decoder_2_, Initialize(ClearConfig(), _, _, _))
       .WillOnce(RunCallback<2>(true));
   EXPECT_CALL(*this, OnDecoderSelected(decoder_2_, IsNull()));
 
-  SelectDecoder();
-}
-
-TEST_F(AudioDecoderSelectorTest,
-       Destroy_ClearStream_NoDecryptor_MultipleClearDecoder) {
-  UseClearStream();
-  InitializeDecoderSelector(kNoDecryptor, 2);
-
-  EXPECT_CALL(*decoder_1_, Initialize(EncryptedConfig(), _, _, _))
-      .WillOnce(RunCallback<2>(false));
-  EXPECT_CALL(*decoder_2_, Initialize(EncryptedConfig(), _, _, _));
-
-  SelectDecoderAndDestroy();
-}
-
-TEST_F(AudioDecoderSelectorTest, ClearStream_DecryptOnly) {
-  UseClearStream();
-  InitializeDecoderSelector(kDecryptOnly, 1);
-
-  EXPECT_CALL(*decoder_1_, Initialize(ClearConfig(), _, _, _))
-      .WillOnce(RunCallback<2>(true));
-  EXPECT_CALL(*this, OnDecoderSelected(decoder_1_, NotNull()));
-
-  SelectDecoder();
-}
-
-TEST_F(AudioDecoderSelectorTest, Destroy_ClearStream_DecryptOnly) {
-  UseClearStream();
-  InitializeDecoderSelector(kDecryptOnly, 1);
-
-  EXPECT_CALL(*decoder_1_, Initialize(ClearConfig(), _, _, _));
-
-  SelectDecoderAndDestroy();
-}
-
-TEST_F(AudioDecoderSelectorTest, ClearStream_DecryptAndDecode) {
-  UseClearStream();
-  InitializeDecoderSelector(kDecryptAndDecode, 1);
-
-#if !defined(OS_ANDROID)
-  // A DecryptingVideoDecoder will be created and selected. The clear decoder
-  // should not be touched at all. No DecryptingDemuxerStream should be
-  // created.
-  EXPECT_CALL(*this, OnDecoderSelected(NotNull(), IsNull()));
-#else
-  // A DecryptingDemuxerStream will be created. The clear decoder will be
-  // initialized and returned.
-  EXPECT_CALL(*decoder_1_, Initialize(ClearConfig(), _, _, _))
-      .WillOnce(RunCallback<2>(true));
-  EXPECT_CALL(*this, OnDecoderSelected(NotNull(), NotNull()));
-#endif
-
-  SelectDecoder();
+  SelectDecoderWithBlacklist(kDecoder1);
 }
 
 // Tests for encrypted streams.
@@ -434,4 +376,48 @@ TEST_F(AudioDecoderSelectorTest, EncryptedStream_DecryptAndDecode) {
   SelectDecoder();
 }
 
+TEST_F(AudioDecoderSelectorTest,
+       EncryptedStream_NoDecryptor_BlackListedDecoder) {
+  UseEncryptedStream();
+  InitializeDecoderSelector(kNoDecryptor, 2);
+
+  EXPECT_CALL(*decoder_2_, Initialize(EncryptedConfig(), _, _, _))
+      .WillOnce(RunCallback<2>(true));
+  EXPECT_CALL(*this, OnDecoderSelected(decoder_2_, IsNull()));
+
+  SelectDecoderWithBlacklist(kDecoder1);
+}
+
+TEST_F(AudioDecoderSelectorTest,
+       EncryptedStream_DecryptOnly_BlackListedDecoder) {
+  UseEncryptedStream();
+  InitializeDecoderSelector(kDecryptOnly, 2);
+
+  // When DecryptingDemuxerStream is chosen, the blacklist is ignored.
+  EXPECT_CALL(*decoder_1_, Initialize(ClearConfig(), _, _, _))
+      .WillOnce(RunCallback<2>(false));
+  EXPECT_CALL(*decoder_2_, Initialize(ClearConfig(), _, _, _))
+      .WillOnce(RunCallback<2>(true));
+  EXPECT_CALL(*this, OnDecoderSelected(decoder_2_, NotNull()));
+
+  SelectDecoderWithBlacklist(kDecoder2);
+}
+
+TEST_F(AudioDecoderSelectorTest,
+       EncryptedStream_DecryptAndDecode_BlackListedDecoder) {
+  UseEncryptedStream();
+  InitializeDecoderSelector(kDecryptAndDecode, 2);
+
+  // DecryptingAudioDecoder is blacklisted so we'll fall back to use
+  // DecryptingDemuxerStream to do decrypt-only.
+  EXPECT_CALL(*decoder_1_, Initialize(ClearConfig(), _, _, _))
+      .WillOnce(RunCallback<2>(false));
+  EXPECT_CALL(*decoder_2_, Initialize(ClearConfig(), _, _, _))
+      .WillOnce(RunCallback<2>(true));
+  EXPECT_CALL(*this, OnDecoderSelected(decoder_2_, NotNull()));
+
+  // TODO(xhwang): Avoid the hardcoded string here.
+  SelectDecoderWithBlacklist("DecryptingAudioDecoder");
+}
+
 }  // namespace media