Modify DecoderPriorityCB so that it's called per-decoder

Currently, `DecoderSelector::decoder_priority_cb_` is called once
per-config, and can only return whether platform decoders specifically
should be prioritized, deprioritized, or left as-is.

This is somewhat inflexible, as it disallows skipping or deprioritizing
individual decoders based on other characteristics. This CL modifies
the decoder selector so that it runs the given predicate/callback once
per decoder, and modifies `DecoderPriority` to be one of
`kDeprioritized`, `kSkipped`, or `kNormal`. kNormal was chosen instead
of `kPrioritized` since it better reflects the mechanism of what's
happening. It's not being prioritized, it's just being left as-is in the
queue.

Change-Id: I9dfcecec5454201501dd1d09c23638da634fe188
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2388269Reviewed-by: Frank Liberato <liberato@chromium.org>
Commit-Queue: Will Cassella <cassew@google.com>
Cr-Commit-Position: refs/heads/master@{#805994}

media/base/test_helpers.cc

@@ -243,6 +243,19 @@ VideoDecoderConfig TestVideoConfig::ExtraLargeEncrypted(VideoCodec codec) {
                        VIDEO_ROTATION_0, kExtraLargeSize, true);
 }
 
+// static
+VideoDecoderConfig TestVideoConfig::Custom(gfx::Size size, VideoCodec codec) {
+  return GetTestConfig(codec, MinProfile(codec), VideoColorSpace::JPEG(),
+                       VIDEO_ROTATION_0, size, false);
+}
+
+// static
+VideoDecoderConfig TestVideoConfig::CustomEncrypted(gfx::Size size,
+                                                    VideoCodec codec) {
+  return GetTestConfig(codec, MinProfile(codec), VideoColorSpace::JPEG(),
+                       VIDEO_ROTATION_0, size, true);
+}
+
 // static
 gfx::Size TestVideoConfig::NormalCodedSize() {
   return kNormalSize;

media/base/test_helpers.h

@@ -110,6 +110,11 @@ class TestVideoConfig {
   static VideoDecoderConfig ExtraLarge(VideoCodec codec = kCodecVP8);
   static VideoDecoderConfig ExtraLargeEncrypted(VideoCodec codec = kCodecVP8);
 
+  static VideoDecoderConfig Custom(gfx::Size size,
+                                   VideoCodec codec = kCodecVP8);
+  static VideoDecoderConfig CustomEncrypted(gfx::Size size,
+                                            VideoCodec codec = kCodecVP8);
+
   // Returns coded size for Normal and Large config.
   static gfx::Size NormalCodedSize();
   static gfx::Size LargeCodedSize();

media/filters/decoder_selector.cc

@@ -32,13 +32,14 @@ namespace {
 
 const char kSelectDecoderTrace[] = "DecoderSelector::SelectDecoder";
 
-template <typename T>
-DecoderPriority UnspecifiedDecoderPriority(const T& /*config*/) {
-  return DecoderPriority::kUnspecified;
+template <typename ConfigT, typename DecoderT>
+DecoderPriority NormalDecoderPriority(const ConfigT& /*config*/,
+                                      const DecoderT& /*decoder*/) {
+  return DecoderPriority::kNormal;
 }
 
-DecoderPriority GetDefaultVideoDecoderPriority(
-    const VideoDecoderConfig& config) {
+DecoderPriority ResolutionBasedDecoderPriority(const VideoDecoderConfig& config,
+                                               const VideoDecoder& decoder) {
 #if defined(OS_ANDROID)
   constexpr auto kSoftwareDecoderHeightCutoff = 360;
 #elif defined(OS_CHROMEOS)
@@ -47,25 +48,31 @@ DecoderPriority GetDefaultVideoDecoderPriority(
   constexpr auto kSoftwareDecoderHeightCutoff = 720;
 #endif
 
-  // We only do a height check to err on the side of hardware decoding
-  return config.visible_rect().height() < kSoftwareDecoderHeightCutoff
-             ? DecoderPriority::kPreferSoftwareDecoders
-             : DecoderPriority::kPreferPlatformDecoders;
+  // We only do a height check to err on the side of prioritizing platform
+  // decoders.
+  const auto at_or_above_software_cutoff =
+      config.visible_rect().height() >= kSoftwareDecoderHeightCutoff;
+
+  // Platform decoders are deprioritized below the cutoff, and non-platform
+  // decoders are deprioritized above it.
+  return at_or_above_software_cutoff == decoder.IsPlatformDecoder()
+             ? DecoderPriority::kNormal
+             : DecoderPriority::kDeprioritized;
 }
 
 void SetDefaultDecoderPriorityCB(VideoDecoderSelector::DecoderPriorityCB* out) {
   if (base::FeatureList::IsEnabled(kResolutionBasedDecoderPriority)) {
-    *out = base::BindRepeating(GetDefaultVideoDecoderPriority);
+    *out = base::BindRepeating(ResolutionBasedDecoderPriority);
   } else {
-    *out = base::BindRepeating<DecoderPriority(const VideoDecoderConfig&)>(
-        UnspecifiedDecoderPriority);
+    *out = base::BindRepeating(
+        NormalDecoderPriority<VideoDecoderConfig, VideoDecoder>);
   }
 }
 
 void SetDefaultDecoderPriorityCB(AudioDecoderSelector::DecoderPriorityCB* out) {
   // Platform audio decoders are not currently prioritized or deprioritized
-  *out = base::BindRepeating<DecoderPriority(const AudioDecoderConfig&)>(
-      UnspecifiedDecoderPriority);
+  *out = base::BindRepeating(
+      NormalDecoderPriority<AudioDecoderConfig, AudioDecoder>);
 }
 
 }  // namespace
@@ -346,31 +353,31 @@ void DecoderSelector<StreamType>::RunSelectDecoderCB() {
 
 template <DemuxerStream::Type StreamType>
 void DecoderSelector<StreamType>::FilterAndSortAvailableDecoders() {
-  // Filter out any decoders that do not support decryption
-  if (config_.is_encrypted()) {
-    auto const non_decrypting = std::remove_if(
-        decoders_.begin(), decoders_.end(),
-        [](auto& decoder) { return !decoder->SupportsDecryption(); });
-    decoders_.erase(non_decrypting, decoders_.end());
+  std::vector<std::unique_ptr<Decoder>> decoders = std::move(decoders_);
+  std::vector<std::unique_ptr<Decoder>> deprioritized_decoders;
+
+  for (auto& decoder : decoders) {
+    // Skip the decoder if this decoder doesn't support encryption for a
+    // decrypting config
+    if (config_.is_encrypted() && !decoder->SupportsDecryption())
+      continue;
+
+    // Run the predicate on this decoder.
+    switch (decoder_priority_cb_.Run(config_, *decoder)) {
+      case DecoderPriority::kSkipped:
+        continue;
+      case DecoderPriority::kNormal:
+        decoders_.push_back(std::move(decoder));
+        break;
+      case DecoderPriority::kDeprioritized:
+        deprioritized_decoders.push_back(std::move(decoder));
+        break;
+    }
   }
 
-  // If platform decoders are prioritized for this config, shift all platform
-  // decoders to the front of the list (retaining their relative order).
-  const auto decoder_priority = decoder_priority_cb_.Run(config_);
-  switch (decoder_priority) {
-    case DecoderPriority::kUnspecified:
-      break;
-    case DecoderPriority::kPreferPlatformDecoders:
-    case DecoderPriority::kPreferSoftwareDecoders: {
-      auto prefer_platform_decoder =
-          decoder_priority == DecoderPriority::kPreferPlatformDecoders;
-      std::stable_partition(decoders_.begin(), decoders_.end(),
-                            [prefer_platform_decoder](auto& decoder) {
-                              return decoder->IsPlatformDecoder() ==
-                                     prefer_platform_decoder;
-                            });
-    } break;
-  }
+  // Post-insert deprioritized decoders
+  std::move(deprioritized_decoders.begin(), deprioritized_decoders.end(),
+            std::inserter(decoders_, decoders_.end()));
 }
 
 // These forward declarations tell the compiler that we will use

media/filters/decoder_selector.h

@@ -28,10 +28,19 @@ class CdmContext;
 class DecryptingDemuxerStream;
 class MediaLog;
 
+// Enum returned by `DecoderSelector::DecoderPriorityCB` to indicate
+// priority of the current decoder.
 enum class DecoderPriority {
-  kUnspecified,
-  kPreferPlatformDecoders,
-  kPreferSoftwareDecoders,
+  // `kNormal` indicates that the current decoder should continue through with
+  // selection in it's current order.
+  kNormal,
+
+  // `kDeprioritized` indicates that the current decoder should only be selected
+  // if other decoders have failed.
+  kDeprioritized,
+
+  // `kSkipped` indicates that the current decoder should not be used at all.
+  kSkipped,
 };
 
 // DecoderSelector handles construction and initialization of Decoders for a
@@ -51,10 +60,11 @@ class MEDIA_EXPORT DecoderSelector {
   using CreateDecodersCB =
       base::RepeatingCallback<std::vector<std::unique_ptr<Decoder>>()>;
 
-  // Evaluates what type of decoders should be prioritized for the given config.
-  // If |kUnspecified| is returned, nothing is prioritized.
+  // Prediate to evaluate whether a decoder should be prioritized,
+  // deprioritized, or skipped.
   using DecoderPriorityCB =
-      base::RepeatingCallback<DecoderPriority(const DecoderConfig&)>;
+      base::RepeatingCallback<DecoderPriority(const DecoderConfig&,
+                                              const Decoder&)>;
 
   // Emits the result of a single call to SelectDecoder(). Parameters are
   //   1: The initialized Decoder. nullptr if selection failed.

media/filters/video_decoder_stream_unittest.cc

@@ -51,11 +51,13 @@ static std::string GetDecoderName(int i) {
   return std::string("VideoDecoder") + base::NumberToString(i);
 }
 
-DecoderPriority MockDecoderPriority(const VideoDecoderConfig& config) {
-  return config.visible_rect().height() >=
-                 TestVideoConfig::LargeCodedSize().height()
-             ? DecoderPriority::kPreferPlatformDecoders
-             : DecoderPriority::kPreferSoftwareDecoders;
+DecoderPriority MockDecoderPriority(const VideoDecoderConfig& config,
+                                    const VideoDecoder& decoder) {
+  auto const at_or_above_cutoff = config.visible_rect().height() >=
+                                  TestVideoConfig::LargeCodedSize().height();
+  return at_or_above_cutoff == decoder.IsPlatformDecoder()
+             ? DecoderPriority::kNormal
+             : DecoderPriority::kDeprioritized;
 }
 
 }  // namespace