media: Support better decoder switching

media/filters/decoder_selector.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/decoder_selector.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ skipping 25 matching lines @@
     case DemuxerStream::TEXT:
     case DemuxerStream::UNKNOWN:
       NOTREACHED();
   }
   return false;
 }
 
 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) {}
 
 template <DemuxerStream::Type StreamType>
 DecoderSelector<StreamType>::~DecoderSelector() {
   DVLOG(2) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (!select_decoder_cb_.is_null())
     ReturnNullDecoder();
 
   decoder_.reset();
   decrypted_stream_.reset();
 }
 
 template <DemuxerStream::Type StreamType>
 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) {
   DVLOG(2) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(traits);
   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);
 
   if (!HasValidStreamConfig(stream)) {
     DLOG(ERROR) << "Invalid stream config.";
     ReturnNullDecoder();
     return;
   }
 
   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
     InitializeDecryptingDemuxerStream();
 #endif
     return;
   }
 
-  config_ = StreamTraits::GetDecoderConfig(input_stream_);
-
-  // If the input stream is encrypted, CdmContext must be non-null.
-  DCHECK(!config_.is_encrypted());
-
   InitializeDecoder();
 }
 
 #if !defined(DISABLE_FFMPEG_VIDEO_DECODERS)
 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_,
       base::Bind(&DecoderSelector<StreamType>::DecryptingDecoderInitDone,
                  weak_ptr_factory_.GetWeakPtr()),
       output_cb_);
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderSelector<StreamType>::DecryptingDecoderInitDone(bool success) {
@@ skipping 38 matching lines @@
   // decryption and use a decoder to decode the clear stream. Otherwise, we'll
   // try to see whether any decoder can decrypt-and-decode the encrypted stream
   // directly. So in both cases, we'll initialize the decoders.
 
   if (status == PIPELINE_OK) {
     input_stream_ = decrypted_stream_.get();
     config_ = StreamTraits::GetDecoderConfig(input_stream_);
     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();
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderSelector<StreamType>::InitializeDecoder() {
   DVLOG(2) << __func__;
   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_,
       base::Bind(&DecoderSelector<StreamType>::DecoderInitDone,
                  weak_ptr_factory_.GetWeakPtr()),
       output_cb_);
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderSelector<StreamType>::DecoderInitDone(bool success) {
@@ skipping 25 matching lines @@
 
 // These forward declarations tell the compiler that we will use
 // DecoderSelector with these arguments, allowing us to keep these definitions
 // in our .cc without causing linker errors. This also means if anyone tries to
 // instantiate a DecoderSelector with anything but these two specializations
 // they'll most likely get linker errors.
 template class DecoderSelector<DemuxerStream::AUDIO>;
 template class DecoderSelector<DemuxerStream::VIDEO>;
 
 }  // namespace media