media: Support better decoder switching

media/filters/decoder_selector.cc

Index: media/filters/decoder_selector.cc
diff --git a/media/filters/decoder_selector.cc b/media/filters/decoder_selector.cc
index c4271060efc28567097de1866657f761c830bdb8..ddbc4c3ab9162210eab31001260850692cb70584 100644
--- a/media/filters/decoder_selector.cc
+++ b/media/filters/decoder_selector.cc
@@ -43,10 +43,10 @@ static bool HasValidStreamConfig(DemuxerStream* stream) {
 template <DemuxerStream::Type StreamType>
 DecoderSelector<StreamType>::DecoderSelector(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-    ScopedVector<Decoder> decoders,
+    const CreateDecodersCB& create_decoders_cb,
     const scoped_refptr<MediaLog>& media_log)
     : task_runner_(task_runner),
-      decoders_(std::move(decoders)),
+      create_decoders_cb_(create_decoders_cb),
       media_log_(media_log),
       input_stream_(nullptr),
       weak_ptr_factory_(this) {}
@@ -68,6 +68,7 @@ void DecoderSelector<StreamType>::SelectDecoder(
     StreamTraits* traits,
     DemuxerStream* stream,
     CdmContext* cdm_context,
+    const std::string& blacklisted_decoder,

[sandersd (OOO until July 31), 2017/04/24 20:55:14]

I worry that blacklisting a single decoder may be insufficient in some cases.
(Example: same decoder available via GpuVideoDecoder and MojoVideoDecoder.) The
cases I can come up with are all temporary and shouldn't be shipped to users,
but I'm not convinced the behavior is obvious in all the possible cases.

Perhaps the selection state can be reified and handled more explicitly?

[xhwang, 2017/04/25 00:43:30]

We "could" host the same decoder via GVD and MVD, but I don't see why we want to
do that in production... That being said, it should be trivial to extend the
"blacklisted_decoder" to a "blacklisted_decoder_list".
 
Could you please elaborate a bit more on what "selection state" you are
referring to?

     const SelectDecoderCB& select_decoder_cb,
     const typename Decoder::OutputCB& output_cb,
     const base::Closure& waiting_for_decryption_key_cb) {
@@ -77,9 +78,6 @@ void DecoderSelector<StreamType>::SelectDecoder(
   DCHECK(stream);
   DCHECK(select_decoder_cb_.is_null());
 
-  cdm_context_ = cdm_context;
-  waiting_for_decryption_key_cb_ = waiting_for_decryption_key_cb;
-
   // Make sure |select_decoder_cb| runs on a different execution stack.
   select_decoder_cb_ = BindToCurrentLoop(select_decoder_cb);
 
@@ -91,11 +89,20 @@ void DecoderSelector<StreamType>::SelectDecoder(
 
   traits_ = traits;
   input_stream_ = stream;
+  cdm_context_ = cdm_context;
+  blacklisted_decoder_ = blacklisted_decoder;
   output_cb_ = output_cb;
+  waiting_for_decryption_key_cb_ = waiting_for_decryption_key_cb;
+
+  decoders_ = create_decoders_cb_.Run();
+  config_ = StreamTraits::GetDecoderConfig(input_stream_);
 
   // When there is a CDM attached, always try the decrypting decoder or
   // demuxer-stream first.
-  if (cdm_context_) {
+  if (config_.is_encrypted()) {
+    DCHECK(cdm_context_);
+// TODO(xhwang): This if-defined doesn't make a lot of sense. It should be
+// replaced by some better checks.
 #if !defined(DISABLE_FFMPEG_VIDEO_DECODERS)
     InitializeDecryptingDecoder();
 #else
@@ -104,11 +111,6 @@ void DecoderSelector<StreamType>::SelectDecoder(
     return;
   }
 
-  config_ = StreamTraits::GetDecoderConfig(input_stream_);
-
-  // If the input stream is encrypted, CdmContext must be non-null.
-  DCHECK(!config_.is_encrypted());
-
   InitializeDecoder();
 }
 
@@ -116,9 +118,15 @@ void DecoderSelector<StreamType>::SelectDecoder(
 template <DemuxerStream::Type StreamType>
 void DecoderSelector<StreamType>::InitializeDecryptingDecoder() {
   DVLOG(2) << __func__;
+
   decoder_.reset(new typename StreamTraits::DecryptingDecoderType(
       task_runner_, media_log_, waiting_for_decryption_key_cb_));
 
+  if (decoder_->GetDisplayName() == blacklisted_decoder_) {

[sandersd (OOO until July 31), 2017/04/24 20:55:14]

This implies some new semantics for the DisplayName of a decoder, which should
be documented. (Specifically, it now needs to be stable and defined before
Initialize().)

I'm actually not sure what should be done for MojoVideoDecoder here. I do want
to proxy the remote decoder name for logging purposes, but depending on exactly
how that is implemented it may only be reasonable after Initialize().

[xhwang, 2017/04/25 00:43:30]

Good point. Will do!
This could be useful in the decode-error-fallback case when MojoVideoDecoder
hosts multiple decoders. But in that case, MojoVideoDecoder can have it's own
fallback logic when decode-error happens, such that if it really fails (even
after trying fallback), putting it in the blacklist is still the right choice.

+    DecryptingDecoderInitDone(false);
+    return;
+  }
+
   traits_->InitializeDecoder(
       decoder_.get(), StreamTraits::GetDecoderConfig(input_stream_),
       input_stream_->liveness() == DemuxerStream::LIVENESS_LIVE, cdm_context_,
@@ -177,11 +185,7 @@ void DecoderSelector<StreamType>::DecryptingDemuxerStreamInitDone(
     DCHECK(!config_.is_encrypted());
   } else {
     decrypted_stream_.reset();
-    config_ = StreamTraits::GetDecoderConfig(input_stream_);
-
-    // Prefer decrypting decoder by using an encrypted config.
-    if (!config_.is_encrypted())
-      config_.SetIsEncrypted(true);
+    DCHECK(config_.is_encrypted());
   }
 
   InitializeDecoder();
@@ -193,14 +197,24 @@ void DecoderSelector<StreamType>::InitializeDecoder() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(!decoder_);
 
-  if (decoders_.empty()) {
+  // Select the next non-blacklisted decoder.
+  while (!decoders_.empty()) {
+    std::unique_ptr<Decoder> decoder(decoders_.front());
+    decoders_.weak_erase(decoders_.begin());
+    // When |decrypted_stream_| is selected, the |config_| has changed so ignore

[DaleCurtis, 2017/04/24 19:39:39]

Can you explain this comment a bit more? The code above seems to prevent vending
if blacklist match, so how are we hitting this point?

[DaleCurtis, 2017/04/24 19:41:04]

Ah just saw your entry in the CL description now. Should this bypass be in the
DecoderStream instead? It seems more informed about making this decision.

[xhwang, 2017/04/25 00:43:29]

From my understanding, DecoderStream doesn't know that a DecryptingDemuxerStream
could be selected in DecoderSelector. So  there's no easy path for DecoderStream
to bypass this. Am I missing something here? :)

+    // the blacklist.
+    if (decrypted_stream_ ||
+        decoder->GetDisplayName() != blacklisted_decoder_) {
+      decoder_ = std::move(decoder);
+      break;
+    }
+  }
+
+  if (!decoder_) {
     ReturnNullDecoder();
     return;
   }
 
-  decoder_.reset(decoders_.front());
-  decoders_.weak_erase(decoders_.begin());
-
   traits_->InitializeDecoder(
       decoder_.get(), config_,
       input_stream_->liveness() == DemuxerStream::LIVENESS_LIVE, cdm_context_,