Add runtime tunables based on finch.

This CL introduces media::Tuneable, which creates a tuneable parameter
based on a finch field trial parameter.  It caches the value to avoid
redundant string conversions, and also has handy helpers for types
that aren't supported by field trial params (base::TimeDelta).

Change-Id: I8b1a6ec3ab0d7f5e36f8ebd496ae2c44e8cd414b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2164227Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792391}

media/base/BUILD.gn

@@ -295,6 +295,8 @@ jumbo_source_set("base") {
     "time_delta_interpolator.h",
     "time_source.h",
     "timestamp_constants.h",
+    "tuneable.cc",
+    "tuneable.h",
     "unaligned_shared_memory.cc",
     "unaligned_shared_memory.h",
     "user_input_monitor.cc",
@@ -573,6 +575,7 @@ source_set("unit_tests") {
     "text_ranges_unittest.cc",
     "text_renderer_unittest.cc",
     "time_delta_interpolator_unittest.cc",
+    "tuneable_unittest.cc",
     "unaligned_shared_memory_unittest.cc",
     "user_input_monitor_unittest.cc",
     "vector_math_unittest.cc",

media/base/media_switches.cc

@@ -717,6 +717,11 @@ const base::Feature kMediaLearningFramework{"MediaLearningFramework",
 const base::Feature kMediaLearningSmoothnessExperiment{
     "MediaLearningSmoothnessExperiment", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Enable the prototype global optimization of tuneables via finch.  See
+// media/base/tuneable.h for how to create tuneable parameters.
+const base::Feature kMediaOptimizer{"JointMediaOptimizer",
+                                    base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Enable aggregate power measurement for media playback.
 const base::Feature kMediaPowerExperiment{"MediaPowerExperiment",
                                           base::FEATURE_DISABLED_BY_DEFAULT};

media/base/media_switches.h

@@ -146,6 +146,7 @@ MEDIA_EXPORT extern const base::Feature kMediaInspectorLogging;
 MEDIA_EXPORT extern const base::Feature kMediaLearningExperiment;
 MEDIA_EXPORT extern const base::Feature kMediaLearningFramework;
 MEDIA_EXPORT extern const base::Feature kMediaLearningSmoothnessExperiment;
+MEDIA_EXPORT extern const base::Feature kMediaOptimizer;
 MEDIA_EXPORT extern const base::Feature kMediaPowerExperiment;
 MEDIA_EXPORT extern const base::Feature kMemoryPressureBasedSourceBufferGC;
 MEDIA_EXPORT extern const base::Feature kNewEncodeCpuLoadEstimator;

media/base/tuneable.cc

+// Copyright 2020 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/base/tuneable.h"
+
+#include <random>
+
+#include "base/hash/hash.h"
+#include "base/metrics/field_trial_params.h"
+#include "base/no_destructor.h"
+#include "base/numerics/ranges.h"
+#include "base/strings/string_number_conversions.h"
+#include "media/base/media_switches.h"
+
+namespace {
+
+// Random seed for all tuneables.
+static std::string* GetRandomSeedPtr() {
+  static base::NoDestructor<std::string> s_random_seed;
+  return &(*s_random_seed);
+}
+
+// Generate a pseudorandom number that depends only on the random seed provided
+// to SetRandomSeedForTuneables and the name provided as an argument.
+//
+// We cast |T| to / from int for the non-specialized implementation, because the
+// underlying parameters sent by finch are ints anyway.  One can't really do
+// too much better.  Since specific types must be declared explicitly for the
+// Tuneable specializations anyway (see the bottom of this file), there's no
+// chance of somebody picking something we haven't thought of and getting an
+// unexpected specialization of GenerateRandom.
+template <typename T>
+T GenerateRandom(const char* name, T minimum_value, T maximum_value) {
+  return static_cast<T>(GenerateRandom<int>(
+      name, static_cast<int>(minimum_value), static_cast<int>(maximum_value)));
+}
+
+template <>
+int GenerateRandom<int>(const char* name,
+                        int minimum_value,
+                        int maximum_value) {
+  // It's okay if this isn't terribly random.
+  auto name_hash =
+      base::PersistentHash(std::string(name) + *GetRandomSeedPtr());
+  // Limit to the range [min, max].
+  // TODO(liberato): I think that this is biased.
+  return name_hash % (maximum_value - minimum_value + 1) + minimum_value;
+}
+
+template <>
+base::TimeDelta GenerateRandom<base::TimeDelta>(const char* name,
+                                                base::TimeDelta minimum_value,
+                                                base::TimeDelta maximum_value) {
+  return base::TimeDelta::FromMilliseconds(GenerateRandom<int>(
+      name, minimum_value.InMilliseconds(), maximum_value.InMilliseconds()));
+}
+
+// Get the finch parameter `name`_`suffix`, and clamp it to the given values.
+// Return `default_value` if there is no parameter, or if the experiment is off.
+template <typename T>
+T GetParam(const char* name,
+           const char* suffix,
+           T minimum_value,
+           T default_value,
+           T maximum_value) {
+  return static_cast<T>(GetParam<int>(
+      name, suffix, static_cast<int>(minimum_value),
+      static_cast<int>(default_value), static_cast<int>(maximum_value)));
+}
+
+template <>
+int GetParam<int>(const char* name,
+                  const char* suffix,
+                  int minimum_value,
+                  int default_value,
+                  int maximum_value) {
+  // TODO: "media_" # `name` ?  Seems like a good idea, since finch params are
+  // not local to any finch feature.  For now, we let consumers do this.
+  std::string param_name = std::string(name) + std::string(suffix);
+  return base::ClampToRange(
+      base::FeatureParam<int>(&::media::kMediaOptimizer, param_name.c_str(),
+                              default_value)
+          .Get(),
+      minimum_value, maximum_value);
+}
+
+template <>
+base::TimeDelta GetParam<base::TimeDelta>(const char* name,
+                                          const char* suffix,
+                                          base::TimeDelta minimum_value,
+                                          base::TimeDelta default_value,
+                                          base::TimeDelta maximum_value) {
+  return base::TimeDelta::FromMilliseconds(GetParam<int>(
+      name, suffix, minimum_value.InMilliseconds(),
+      default_value.InMilliseconds(), maximum_value.InMilliseconds()));
+}
+
+}  // namespace
+
+namespace media {
+
+template <typename T>
+Tuneable<T>::Tuneable(const char* name,
+                      T minimum_value,
+                      T default_value,
+                      T maximum_value) {
+  // Fetch the finch-provided range, clamped to the min, max and defaulted to
+  // the hardcoded default.  This way, if it's unset, min == max == default.
+  T finch_minimum =
+      GetParam<T>(name, "_min", minimum_value, default_value, maximum_value);
+  T finch_maximum =
+      GetParam<T>(name, "_max", minimum_value, default_value, maximum_value);
+
+  if (finch_minimum > finch_maximum) {
+    // Bad parameters.  They're all in range, so we could pick any, but we
+    // assume that the finch range has no meaning and just use the (hopefully)
+    // saner default.
+    t_ = default_value;
+    return;
+  }
+
+  t_ = GenerateRandom<T>(name, finch_minimum, finch_maximum);
+}
+
+template <typename T>
+Tuneable<T>::~Tuneable() = default;
+
+void MEDIA_EXPORT
+SetRandomSeedForTuneables(const base::UnguessableToken& seed) {
+  *GetRandomSeedPtr() = seed.ToString();
+}
+
+// All allowed Tuneable types.  Be sure that GenerateRandom() and GetParam()
+// do something sane for any type you add.
+template class MEDIA_EXPORT Tuneable<int>;
+template class MEDIA_EXPORT Tuneable<base::TimeDelta>;
+template class MEDIA_EXPORT Tuneable<size_t>;
+
+}  // namespace media

media/base/tuneable.h

+// Copyright 2020 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.
+
+#ifndef MEDIA_BASE_TUNEABLE_H_
+#define MEDIA_BASE_TUNEABLE_H_
+
+#include "base/macros.h"
+#include "base/time/time.h"
+#include "base/unguessable_token.h"
+#include "media/base/media_export.h"
+
+namespace media {
+
+// One tuneable value.  Each Tuneable has:
+//  name - Used to set the finch parameter name.
+//  value - Current runtime value.  All Tuneable instances with the same name
+//          will return the same value.  This value will be a constant over the
+//          lifetime of the process as well.
+//  min / default / max values - hardcoded range and default for this tuneable.
+//
+// Via finch, one may enable randomization of the Tuneables, such that a value
+// will be chosen at random between a finch-provided miniumum and maximum.  This
+// minumum and maximum will be constrained by the hardcoded one provided during
+// construction.  Different Tuneable instances for the same name are still
+// guaranteed to be equal, as described above.
+template <typename T>
+class MEDIA_EXPORT Tuneable {
+ public:
+  Tuneable(const char* name, T minimum_value, T default_value, T maximum_value);
+  Tuneable(const Tuneable&) = delete;
+  Tuneable(Tuneable&&) = delete;
+  ~Tuneable();
+
+  Tuneable& operator=(const Tuneable&) = delete;
+  Tuneable& operator=(Tuneable&&) = delete;
+
+  T value() const { return t_; }
+
+  void set_for_testing(T value) { t_ = value; }
+
+ private:
+  T t_;
+};
+
+// Set the random number seed that the tuneable will use to choose its value.
+// Must be called before the first tuneable is constructed.
+void MEDIA_EXPORT SetRandomSeedForTuneables(const base::UnguessableToken& seed);
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_TUNEABLE_H_

media/base/tuneable_unittest.cc

+// Copyright 2020 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/base/tuneable.h"
+
+#include "base/strings/string_number_conversions.h"
+#include "base/test/scoped_feature_list.h"
+#include "media/base/media_switches.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace media {
+
+class TuneableTest : public ::testing::Test {
+ public:
+  TuneableTest() = default;
+
+  void SetUp() override {
+    // Note that we might need to call value() to cache `tuneable_cached_` here.
+    // We don't currently, since it's not needed.
+
+    // Set everything else to non-default values.  We do this because we can't
+    // do it in the tests without it doing odd things.
+    // params[kTuneableIntSetToNot123] = "124";  // Not 123.
+    // params[kTuneableInt0] = "0";
+    // params[kTuneableInt5] = "5";
+    // params[kTuneableInt10] = "10";
+
+    // Set some tuneables to fixed values.
+    SetFinchParameters(kTuneableIntSetToNot123, 124, 124);
+    SetFinchParameters(kTuneableInt0, 0, 0);
+    SetFinchParameters(kTuneableInt5, 5, 5);
+    SetFinchParameters(kTuneableInt10, 10, 10);
+    // TimeDelta should be given in milliseconds.
+    SetFinchParameters(kTuneableTimeDeltaFiveSeconds, 5000, 5000);
+
+    // Let some vary via finch trial.
+    SetFinchParameters(kTuneableInt5To10, 5, 10);
+    // Create 100 identical tuneables, except for their names.
+    for (int i = 0; i < 100; i++) {
+      SetFinchParameters(GetNameForNumberedTuneable(k100Tuneables, i).c_str(),
+                         1, 100);
+    }
+
+    scoped_feature_list_.InitAndEnableFeatureWithParameters(kMediaOptimizer,
+                                                            params_);
+  }
+
+  // Set the finch-chosen parameters for tuneable `name`.
+  void SetFinchParameters(const char* name, int min_value, int max_value) {
+    std::string min_name = std::string(name) + "_min";
+    params_[min_name] = base::NumberToString(min_value);
+    std::string max_name = std::string(name) + "_max";
+    params_[max_name] = base::NumberToString(max_value);
+  }
+
+  // Return the tuneable name for the `x`-th numbered tuneable.
+  std::string GetNameForNumberedTuneable(const char* basename, int x) {
+    std::string name(basename);
+    name[0] = "0123456789ABCDEF"[x & 15];
+    name[1] = "0123456789ABCDEF"[(x >> 4) & 15];
+    return name;
+  }
+
+  base::test::ScopedFeatureList scoped_feature_list_;
+  base::FieldTrialParams params_;
+
+  // Params will set this to not 123.  We default it to 123 before Setup runs,
+  // so we make sure that it uses the hardcoded defaults properly.
+  static constexpr const char* kTuneableIntSetToNot123 = "t_int_not_123";
+  Tuneable<int> tuneable_cached_{kTuneableIntSetToNot123, 0, 123, 200};
+
+  static constexpr const char* kTuneableIntUnset = "t_int_unset";
+  static constexpr const char* kTuneableInt0 = "t_int_0";
+  static constexpr const char* kTuneableInt5 = "t_int_5";
+  static constexpr const char* kTuneableInt10 = "t_int_10";
+  static constexpr const char* kTuneableTimeDeltaFiveSeconds = "t_time_5s";
+
+  static constexpr const char* kTuneableInt5To10 = "t_int_5to10";
+
+  // Initialize 100 of these with different names.
+  static constexpr const char* k100Tuneables = "XX_tuneable";
+
+  DISALLOW_COPY_AND_ASSIGN(TuneableTest);
+};
+
+TEST_F(TuneableTest, IntTuneableCached) {
+  // Verify that `tuneable_cached_` is, in fact, 123 even though the params try
+  // to set it to something else.  This kind of, sort of, guarantees that it's
+  // cached properly.
+  EXPECT_EQ(tuneable_cached_.value(), 123);
+}
+
+TEST_F(TuneableTest, IntTuneableFromDefaultWithClamps) {
+  // The default value should be used, and correctly clamped.
+  constexpr int min_value = 0;
+  constexpr int default_value = 4;
+  constexpr int max_value = 10;
+  Tuneable<int> t_min(kTuneableIntUnset, min_value, min_value - 1, max_value);
+  EXPECT_EQ(t_min.value(), min_value);
+  Tuneable<int> t_default(kTuneableIntUnset, min_value, default_value,
+                          max_value);
+  EXPECT_EQ(t_default.value(), default_value);
+  Tuneable<int> t_max(kTuneableIntUnset, min_value, max_value + 1, max_value);
+  EXPECT_EQ(t_max.value(), max_value);
+}
+
+TEST_F(TuneableTest, IntTuneableFromParams) {
+  // Verify that params override the defaults, and are clamped correctly.
+  constexpr int min_value = 1;
+  constexpr int default_value = 4;  // That's not the same as the param.
+  constexpr int max_value = 9;
+  Tuneable<int> t_min(kTuneableInt0, min_value, default_value, max_value);
+  EXPECT_EQ(t_min.value(), min_value);
+  Tuneable<int> t_param(kTuneableInt5, min_value, default_value, max_value);
+  EXPECT_EQ(t_param.value(), 5);
+  Tuneable<int> t_max(kTuneableInt10, min_value, default_value, max_value);
+  EXPECT_EQ(t_max.value(), max_value);
+}
+
+TEST_F(TuneableTest, OtherSpecializationsCompile) {
+  // Since it's all templated, just be happy if it compiles and does something
+  // somewhat sane.
+  constexpr base::TimeDelta min_value = base::TimeDelta::FromSeconds(0);
+  constexpr base::TimeDelta default_value = base::TimeDelta::FromSeconds(5);
+  constexpr base::TimeDelta max_value = base::TimeDelta::FromSeconds(10);
+  Tuneable<base::TimeDelta> time_delta_tuneable("whatever", min_value,
+                                                default_value, max_value);
+  // Since the tuneable is not provided in the finch parameters, it should
+  // equal the default.
+  EXPECT_EQ(time_delta_tuneable.value(), default_value);
+
+  Tuneable<size_t> size_t_tuneable("whatever_else", 0u, 100u, 500u);
+  EXPECT_EQ(size_t_tuneable.value(), 100u);
+}
+
+TEST_F(TuneableTest, TimeDeltaIsSpecifiedInMilliseconds) {
+  // Since the finch params are constructed with the assumption that the value
+  // will be interpreted as milliseconds, make sure that the Tuneable actually
+  // does interpret it that way.
+  constexpr base::TimeDelta min_value = base::TimeDelta::FromSeconds(0);
+  constexpr base::TimeDelta max_value = base::TimeDelta::FromSeconds(100);
+  Tuneable<base::TimeDelta> t(kTuneableTimeDeltaFiveSeconds, min_value,
+                              min_value, max_value);
+  EXPECT_EQ(t.value(), base::TimeDelta::FromSeconds(5));
+}
+
+TEST_F(TuneableTest, MultipleTuneablesGetTheSameRandomValue) {
+  // Multiple copies of the same tuneable should get the same value.
+  Tuneable<int> t0(kTuneableInt5To10, 0, 2, 100);
+  Tuneable<int> t1(kTuneableInt5To10, 0, 2, 100);
+  EXPECT_GE(t0.value(), 0);
+  EXPECT_LE(t0.value(), 100);
+  EXPECT_EQ(t0.value(), t1.value());
+}
+
+TEST_F(TuneableTest, DifferentSeedsProduceDifferentValues) {
+  // Also verify that they stay bounded.
+  SetRandomSeedForTuneables(base::UnguessableToken::Create());
+  Tuneable<int> t0(kTuneableInt5To10, 0, 2, 100);
+  bool found_different = false;
+  for (int i = 1; i < 100; i++) {
+    SetRandomSeedForTuneables(base::UnguessableToken::Create());
+    Tuneable<int> t1(kTuneableInt5To10, 0, 2, 100);
+    EXPECT_GE(t1.value(), 0);
+    EXPECT_LE(t1.value(), 100);
+    if (t1.value() != t0.value())
+      found_different = true;
+  }
+  EXPECT_TRUE(found_different);
+}
+
+TEST_F(TuneableTest, DifferentNamesProduceDifferentValues) {
+  // For the same seed, we expect different parameter names to sometimes get
+  // different values.
+  Tuneable<int> t0(GetNameForNumberedTuneable(k100Tuneables, 0).c_str(), 0, 50,
+                   100);
+  bool found_different = false;
+  for (int i = 1; i < 100; i++) {
+    Tuneable<int> t1(GetNameForNumberedTuneable(k100Tuneables, i).c_str(), 0,
+                     50, 100);
+    EXPECT_GE(t1.value(), 0);
+    EXPECT_LE(t1.value(), 100);
+    if (t1.value() != t0.value())
+      found_different = true;
+  }
+  EXPECT_TRUE(found_different);
+}
+
+}  // namespace media

media/blink/multibuffer_data_source.cc

@@ -34,7 +34,7 @@ const int64_t kMaxBufferPreload = 50 << 20;  // 50 Mb
 const int64_t kMetadataShift = 6;
 
 // Preload this much extra, then stop preloading until we fall below the
-// kTargetSecondsBufferedAhead.
+// preload_seconds_.value().
 const int64_t kPreloadHighExtra = 1 << 20;  // 1 Mb
 
 // Default pin region size.
@@ -50,12 +50,6 @@ const int64_t kMaxBitrate = 20 * 8 << 20;  // 20 Mbps.
 // Maximum playback rate for buffer calculations.
 const double kMaxPlaybackRate = 25.0;
 
-// Preload this many seconds of data by default.
-const int64_t kTargetSecondsBufferedAhead = 10;
-
-// Keep this many seconds of data for going back by default.
-const int64_t kTargetSecondsBufferedBehind = 2;
-
 // Extra buffer accumulation speed, in terms of download buffer.
 const int kSlowPreloadPercentage = 10;
 
@@ -721,7 +715,7 @@ void MultibufferDataSource::UpdateBufferSizes() {
 
   // Preload 10 seconds of data, clamped to some min/max value.
   int64_t preload =
-      base::ClampToRange(kTargetSecondsBufferedAhead * bytes_per_second,
+      base::ClampToRange(preload_seconds_.value() * bytes_per_second,
                          kMinBufferPreload, kMaxBufferPreload);
 
   // Increase buffering slowly at a rate of 10% of data downloaded so
@@ -736,9 +730,9 @@ void MultibufferDataSource::UpdateBufferSizes() {
   int64_t preload_high = preload + kPreloadHighExtra;
 
   // We pin a few seconds of data behind the current reading position.
-  int64_t pin_backward =
-      base::ClampToRange(kTargetSecondsBufferedBehind * bytes_per_second,
-                         kMinBufferPreload, kMaxBufferPreload);
+  int64_t pin_backward = base::ClampToRange(
+      keep_after_playback_seconds_.value() * bytes_per_second,
+      kMinBufferPreload, kMaxBufferPreload);
 
   // We always pin at least kDefaultPinSize ahead of the read position.
   // Normally, the extra space between preload_high and kDefaultPinSize will
@@ -750,11 +744,11 @@ void MultibufferDataSource::UpdateBufferSizes() {
   // to be thrown away. Most of the time we pin a region that is larger than
   // |buffer_size|, which only makes sense because most of the time, some of
   // the data in pinned region is not present in the cache.
-  int64_t buffer_size =
-      std::min((kTargetSecondsBufferedAhead + kTargetSecondsBufferedBehind) *
-                       bytes_per_second +
-                   extra_buffer * 3,
-               preload_high + pin_backward + extra_buffer);
+  int64_t buffer_size = std::min(
+      (preload_seconds_.value() + keep_after_playback_seconds_.value()) *
+              bytes_per_second +
+          extra_buffer * 3,
+      preload_high + pin_backward + extra_buffer);
 
   if (url_data_->FullyCached() ||
       (url_data_->length() != kPositionNotSpecified &&

media/blink/multibuffer_data_source.h

@@ -17,6 +17,7 @@
 #include "base/synchronization/lock.h"
 #include "media/base/data_source.h"
 #include "media/base/ranges.h"
+#include "media/base/tuneable.h"
 #include "media/blink/media_blink_export.h"
 #include "media/blink/url_index.h"
 #include "url/gurl.h"
@@ -267,6 +268,14 @@ class MEDIA_BLINK_EXPORT MultibufferDataSource : public DataSource {
 
   DownloadingCB downloading_cb_;
 
+  // Preload this many seconds of data by default.
+  media::Tuneable<int> preload_seconds_ = {"SrcMediaMultibufferPreloadSeconds",
+                                           0, 10, 60};
+
+  // Keep this many seconds of data for going back by default.
+  media::Tuneable<int> keep_after_playback_seconds_ = {
+      "SrcMediaMultibufferKeepAfterPlaybackSeconds", 0, 2, 60};
+
   // Disallow rebinding WeakReference ownership to a different thread by keeping
   // a persistent reference. This avoids problems with the thread-safety of
   // reaching into this class from multiple threads to attain a WeakPtr.

media/gpu/android/codec_buffer_wait_coordinator.cc

@@ -51,12 +51,9 @@ bool CodecBufferWaitCoordinator::IsExpectingFrameAvailable() {
 void CodecBufferWaitCoordinator::WaitForFrameAvailable() {
   DCHECK(!release_time_.is_null());
 
-  // 5msec covers >99.9% of cases, so just wait for up to that much before
-  // giving up. If an error occurs, we might not ever get a notification.
-  const base::TimeDelta max_wait = base::TimeDelta::FromMilliseconds(5);
   const base::TimeTicks call_time = base::TimeTicks::Now();
   const base::TimeDelta elapsed = call_time - release_time_;
-  const base::TimeDelta remaining = max_wait - elapsed;
+  const base::TimeDelta remaining = max_wait_.value() - elapsed;
   release_time_ = base::TimeTicks();
   bool timed_out = false;
 
@@ -67,7 +64,7 @@ void CodecBufferWaitCoordinator::WaitForFrameAvailable() {
       timed_out = true;
     }
   } else {
-    DCHECK_LE(remaining, max_wait);
+    DCHECK_LE(remaining, max_wait_.value());
     SCOPED_UMA_HISTOGRAM_TIMER(
         "Media.CodecImage.CodecBufferWaitCoordinator.WaitTimeForFrame");
     if (!frame_available_event_->event.TimedWait(remaining)) {

media/gpu/android/codec_buffer_wait_coordinator.h

@@ -10,6 +10,7 @@
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "gpu/command_buffer/service/texture_owner.h"
+#include "media/base/tuneable.h"
 #include "media/gpu/media_gpu_export.h"
 
 namespace media {
@@ -58,6 +59,13 @@ class MEDIA_GPU_EXPORT CodecBufferWaitCoordinator
   scoped_refptr<FrameAvailableEvent> frame_available_event_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
+  // 5msec covers >99.9% of cases, so just wait for up to that much before
+  // giving up. If an error occurs, we might not ever get a notification.
+  Tuneable<base::TimeDelta> max_wait_ = {"MediaCodecOutputBufferMaxWaitTime",
+                                         base::TimeDelta::FromMilliseconds(0),
+                                         base::TimeDelta::FromMilliseconds(5),
+                                         base::TimeDelta::FromMilliseconds(20)};
+
   DISALLOW_COPY_AND_ASSIGN(CodecBufferWaitCoordinator);
 };
 

media/renderers/renderer_impl.cc

@@ -30,9 +30,6 @@
 
 namespace media {
 
-// See |video_underflow_threshold_|.
-static const int kDefaultVideoUnderflowThresholdMs = 3000;
-
 class RendererImpl::RendererClientInternal final : public RendererClient {
  public:
   RendererClientInternal(DemuxerStream::Type type,
@@ -105,23 +102,10 @@ RendererImpl::RendererImpl(
       cdm_context_(nullptr),
       underflow_disabled_for_testing_(false),
       clockless_video_playback_enabled_for_testing_(false),
-      video_underflow_threshold_(
-          base::TimeDelta::FromMilliseconds(kDefaultVideoUnderflowThresholdMs)),
       pending_audio_track_change_(false),
       pending_video_track_change_(false) {
   weak_this_ = weak_factory_.GetWeakPtr();
   DVLOG(1) << __func__;
-
-  // TODO(dalecurtis): Remove once experiments for http://crbug.com/470940 are
-  // complete.
-  int threshold_ms = 0;
-  std::string threshold_ms_str(
-      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-          switches::kVideoUnderflowThresholdMs));
-  if (base::StringToInt(threshold_ms_str, &threshold_ms) && threshold_ms > 0) {
-    video_underflow_threshold_ =
-        base::TimeDelta::FromMilliseconds(threshold_ms);
-  }
 }
 
 RendererImpl::~RendererImpl() {
@@ -743,7 +727,7 @@ void RendererImpl::OnBufferingStateChange(DemuxerStream::Type type,
                               type, new_buffering_state, reason));
       task_runner_->PostDelayedTask(FROM_HERE,
                                     deferred_video_underflow_cb_.callback(),
-                                    video_underflow_threshold_);
+                                    video_underflow_threshold_.value());
       return;
     }
 

media/renderers/renderer_impl.h

@@ -24,6 +24,7 @@
 #include "media/base/media_export.h"
 #include "media/base/pipeline_status.h"
 #include "media/base/renderer.h"
+#include "media/base/tuneable.h"
 #include "media/base/video_decoder_config.h"
 #include "media/base/waiting.h"
 #include "ui/gfx/geometry/size.h"
@@ -78,7 +79,7 @@ class MEDIA_EXPORT RendererImpl : public Renderer {
     time_source_ = time_source;
   }
   void set_video_underflow_threshold_for_testing(base::TimeDelta threshold) {
-    video_underflow_threshold_ = threshold;
+    video_underflow_threshold_.set_for_testing(threshold);
   }
 
  private:
@@ -252,7 +253,10 @@ class MEDIA_EXPORT RendererImpl : public Renderer {
 
   // The amount of time to wait before declaring underflow if the video renderer
   // runs out of data but the audio renderer still has enough.
-  base::TimeDelta video_underflow_threshold_;
+  Tuneable<base::TimeDelta> video_underflow_threshold_ = {
+      "MediaVideoUnderflowThreshold", base::TimeDelta::FromMilliseconds(1000),
+      base::TimeDelta::FromMilliseconds(3000),
+      base::TimeDelta::FromMilliseconds(8000)};
 
   // Lock used to protect access to the |restarting_audio_| flag and
   // |restarting_audio_time_|.

media/renderers/video_renderer_impl.cc

@@ -18,7 +18,6 @@
 #include "base/time/default_tick_clock.h"
 #include "base/trace_event/trace_event.h"
 #include "media/base/bind_to_current_loop.h"
-#include "media/base/limits.h"
 #include "media/base/media_log.h"
 #include "media/base/media_switches.h"
 #include "media/base/pipeline_status.h"
@@ -73,8 +72,8 @@ VideoRendererImpl::VideoRendererImpl(
       have_renderered_frames_(false),
       last_frame_opaque_(false),
       painted_first_frame_(false),
-      min_buffered_frames_(limits::kMaxVideoFrames),
-      max_buffered_frames_(limits::kMaxVideoFrames) {
+      min_buffered_frames_(initial_buffering_size_.value()),
+      max_buffered_frames_(initial_buffering_size_.value()) {
   DCHECK(create_video_decoders_cb_);
 }
 
@@ -132,8 +131,10 @@ void VideoRendererImpl::Flush(base::OnceClosure callback) {
 
   // Reset preroll capacity so seek time is not penalized. |latency_hint_|
   // and |low_delay_| mode disable automatic preroll adjustments.
-  if (!latency_hint_.has_value() && !low_delay_)
-    min_buffered_frames_ = max_buffered_frames_ = limits::kMaxVideoFrames;
+  if (!latency_hint_.has_value() && !low_delay_) {
+    min_buffered_frames_ = max_buffered_frames_ =
+        initial_buffering_size_.value();
+  }
 }
 
 void VideoRendererImpl::StartPlayingFrom(base::TimeDelta timestamp) {
@@ -433,7 +434,7 @@ void VideoRendererImpl::OnTimeStopped() {
     // |low_delay_| mode disables automatic increases. In these cases the site
     // is expressing a desire to manually control/minimize the buffering
     // threshold for HAVE_ENOUGH.
-    const size_t kMaxUnderflowGrowth = 2 * limits::kMaxVideoFrames;
+    const size_t kMaxUnderflowGrowth = 2 * initial_buffering_size_.value();
     if (!latency_hint_.has_value() && !low_delay_) {
       DCHECK_EQ(min_buffered_frames_, max_buffered_frames_);
 
@@ -471,8 +472,9 @@ void VideoRendererImpl::SetLatencyHint(
 
   if (!latency_hint_.has_value()) {
     // Restore default values.
-    // NOTE kMaxVideoFrames the default max, not the max overall.
-    min_buffered_frames_ = max_buffered_frames_ = limits::kMaxVideoFrames;
+    // NOTE |initial_buffering_size_| the default max, not the max overall.
+    min_buffered_frames_ = max_buffered_frames_ =
+        initial_buffering_size_.value();
     MEDIA_LOG(DEBUG, media_log_)
         << "Video latency hint cleared. Default buffer size ("
         << min_buffered_frames_ << " frames) restored";
@@ -482,7 +484,7 @@ void VideoRendererImpl::SetLatencyHint(
     // to avoid needless churn since the "bare minimum" buffering doesn't
     // fluctuate with changes to FPS.
     min_buffered_frames_ = 1;
-    max_buffered_frames_ = limits::kMaxVideoFrames;
+    max_buffered_frames_ = initial_buffering_size_.value();
     MEDIA_LOG(DEBUG, media_log_)
         << "Video latency hint set:" << *latency_hint << ". "
         << "Effective buffering latency: 1 frame";
@@ -530,8 +532,8 @@ void VideoRendererImpl::UpdateLatencyHintBufferingCaps_Locked(
   }
 
   // Use initial capacity limit if possible. Increase if needed.
-  max_buffered_frames_ = std::max(min_buffered_frames_,
-                                  static_cast<size_t>(limits::kMaxVideoFrames));
+  max_buffered_frames_ =
+      std::max(min_buffered_frames_, initial_buffering_size_.value());
 
   if (!is_latency_hint_media_logged_) {
     is_latency_hint_media_logged_ = true;

media/renderers/video_renderer_impl.h

@@ -19,8 +19,10 @@
 #include "media/base/decryptor.h"
 #include "media/base/demuxer_stream.h"
 #include "media/base/frame_rate_estimator.h"
+#include "media/base/limits.h"
 #include "media/base/media_log.h"
 #include "media/base/pipeline_status.h"
+#include "media/base/tuneable.h"
 #include "media/base/video_decoder.h"
 #include "media/base/video_decoder_config.h"
 #include "media/base/video_frame.h"
@@ -315,6 +317,10 @@ class MEDIA_EXPORT VideoRendererImpl
   // Indicates if we've painted the first valid frame after StartPlayingFrom().
   bool painted_first_frame_;
 
+  // The initial value for |min_buffered_frames_| and |max_buffered_frames_|.
+  Tuneable<size_t> initial_buffering_size_ = {
+      "MediaInitialBufferingSizeForHaveEnough", 3, limits::kMaxVideoFrames, 10};
+
   // The number of frames required to transition from BUFFERING_HAVE_NOTHING to
   // BUFFERING_HAVE_ENOUGH.
   size_t min_buffered_frames_;