Numerous fixes to audio/video buffered resource loading.

webkit/glue/media/buffered_resource_loader.cc

Index: webkit/glue/media/buffered_resource_loader.cc
diff --git a/webkit/glue/media/buffered_resource_loader.cc b/webkit/glue/media/buffered_resource_loader.cc
index 07f5729693b20b4701209bbb2603b857595a390a..2ce07fa87dc33a90c504a220a623fc24edd254ba 100644
--- a/webkit/glue/media/buffered_resource_loader.cc
+++ b/webkit/glue/media/buffered_resource_loader.cc
@@ -35,11 +35,10 @@ static const int kHttpPartialContent = 206;
 // Define the number of bytes in a megabyte.
 static const size_t kMegabyte = 1024 * 1024;
 
-// Default backward capacity of the buffer.
-static const size_t kBackwardCapacity = 2 * kMegabyte;
-
-// Default forward capacity of the buffer.
-static const size_t kForwardCapacity = 10 * kMegabyte;
+// Minimum capacity of the buffer in forward or backward direction.
+//
+// 2MB is an arbitrary limit; it just seems to be "good enough" in practice.
+static const size_t kMinBufferCapacity = 2 * kMegabyte;
 
 // Maximum capacity of the buffer in forward or backward direction. This is
 // effectively the largest single read the code path can handle.
@@ -52,14 +51,55 @@ static const size_t kMaxBufferCapacity = 20 * kMegabyte;
 // location and will instead reset the request.
 static const int kForwardWaitThreshold = 2 * kMegabyte;
 
+// Computes the suggested backward and forward capacity for the buffer
+// if one wants to play at |playback_rate| * the natural playback speed.
+// Use a value of 0 for |bitrate| if it is unknown.
+static void ComputeTargetBufferWindow(float playback_rate, int bitrate,
+                                      size_t* out_backward_capacity,
+                                      size_t* out_forward_capacity) {
+  static const size_t kDefaultBitrate = 200 * 1024 * 8;  // 200 Kbps.
+  static const size_t kMaxBitrate = 20 * kMegabyte * 8;  // 20 Mbps.
+  static const float kMaxPlaybackRate = 25.0;
+  static const size_t kTargetSecondsBufferedAhead = 10;
+  static const size_t kTargetSecondsBufferedBehind = 2;
+
+  // Use a default bit rate if unknown and clamp to prevent overflow.
+  if (bitrate <= 0)
+    bitrate = kDefaultBitrate;
+  bitrate = std::min(static_cast<size_t>(bitrate), kMaxBitrate);
+
+  // Only scale the buffer window for higher playback rates and clamp to prevent
+  // overflow.
+  bool backward_playback = playback_rate < 0.0f;
+  playback_rate = std::max(playback_rate, 1.0f);

[acolwell GONE FROM CHROMIUM, 2011/10/11 16:18:01]

Why are we mapping 0 & all negative playback_rates to 1 here? If so the comment
should be updated to reflect why.

If playback rate is 0 do we really want to adjust to 1x size? It seems like we'd
want to keep the buffer the same size since it is more likely that we'll unpause
at the same rate before the pause.

[scherkus (not reviewing), 2011/10/11 20:13:19]

whoops missed absolute value of playback_rate!

it's the values from [-1..1] that we shouldn't bother scaling

I removed the check for playback_rate == 0 because I view this function as
something that should always give us the most sensible buffer size, even if we
don't have the best information on hand (i.e., during construction when
bitrate==0, playback_rate==0)

Initializing buffer_ in the constructor with canned values as opposed to using
this function could lead to an immediate buffer resize as soon as
bitrate/playback rate is set, which is why we now always use this function when
calculating sizes for buffer_

+  playback_rate = std::min(playback_rate, kMaxPlaybackRate);
+
+  size_t bytes_per_second = static_cast<size_t>(playback_rate * bitrate / 8.0);
+
+  // Clamp between kMinBufferCapacity and kMaxBufferCapacity.
+  *out_forward_capacity = std::max(
+      kTargetSecondsBufferedAhead * bytes_per_second, kMinBufferCapacity);
+  *out_backward_capacity = std::max(
+      kTargetSecondsBufferedBehind * bytes_per_second, kMinBufferCapacity);
+
+  *out_forward_capacity = std::min(*out_forward_capacity, kMaxBufferCapacity);
+  *out_backward_capacity = std::min(*out_backward_capacity, kMaxBufferCapacity);
+
+  if (backward_playback)
+    std::swap(*out_forward_capacity, *out_backward_capacity);
+}
+
+
 BufferedResourceLoader::BufferedResourceLoader(
     const GURL& url,
     int64 first_byte_position,
     int64 last_byte_position,
+    DeferStrategy strategy,
+    int bitrate,
+    float playback_rate,
     media::MediaLog* media_log)
-    : buffer_(new media::SeekableBuffer(kBackwardCapacity, kForwardCapacity)),
-      deferred_(false),
-      defer_strategy_(kReadThenDefer),
+    : deferred_(false),
+      defer_strategy_(strategy),
       completed_(false),
       range_requested_(false),
       range_supported_(false),
@@ -79,9 +119,15 @@ BufferedResourceLoader::BufferedResourceLoader(
       first_offset_(0),
       last_offset_(0),
       keep_test_loader_(false),
-      bitrate_(0),
-      playback_rate_(0.0),
+      bitrate_(bitrate),
+      playback_rate_(playback_rate),
       media_log_(media_log) {
+
+  size_t backward_capacity;
+  size_t forward_capacity;
+  ComputeTargetBufferWindow(
+      playback_rate_, bitrate_, &backward_capacity, &forward_capacity);
+  buffer_.reset(new media::SeekableBuffer(backward_capacity, forward_capacity));
 }
 
 BufferedResourceLoader::~BufferedResourceLoader() {
@@ -232,7 +278,7 @@ void BufferedResourceLoader::Read(int64 position,
       saved_forward_capacity_ = buffer_->forward_capacity();
       buffer_->set_forward_capacity(read_size_);
     }
-   return;
+    return;
   }
 
   // Make a callback to report failure.
@@ -510,49 +556,10 @@ void BufferedResourceLoader::SetBitrate(int bitrate) {
 /////////////////////////////////////////////////////////////////////////////
 // Helper methods.
 
-// Computes the suggested backward and forward capacity for the buffer
-// if one wants to play at |playback_rate| * the natural playback speed.
-// Use a value of 0 for |bitrate| if it is unknown.
-static void ComputeTargetBufferWindow(float playback_rate, int bitrate,
-                                      size_t* out_backward_capacity,
-                                      size_t* out_forward_capacity) {
-  DCHECK_GE(bitrate, 0);
-  DCHECK_NE(playback_rate, 0.0);
-  static const size_t kDefaultBitrate = kMegabyte;
-  static const size_t kMaxBitrate = 50 * kMegabyte;
-  static const float kMaxPlaybackRate = 25.0;
-  static const size_t kTargetSecondsBufferedAhead = 10;
-  static const size_t kTargetSecondsBufferedBehind = 2;
-
-  if (bitrate <= 0)
-    bitrate = kDefaultBitrate;
-
-  bool backward_playback = playback_rate < 0.0;
-  if (backward_playback)
-    playback_rate *= -1.0;
-
-  // Cap playback rate and bitrate to prevent overflow.
-  playback_rate = std::min(kMaxPlaybackRate, playback_rate);
-  bitrate  = std::min(kMaxBitrate, static_cast<size_t>(bitrate));
-
-  size_t bytes_per_second = static_cast<size_t>(playback_rate * bitrate / 8.0);
-
-  *out_forward_capacity = std::min(
-      kTargetSecondsBufferedAhead * bytes_per_second, kMaxBufferCapacity);
-  *out_backward_capacity = std::min(
-      kTargetSecondsBufferedBehind * bytes_per_second, kMaxBufferCapacity);
-  if (backward_playback)
-    std::swap(*out_forward_capacity, *out_backward_capacity);
-}
-
 void BufferedResourceLoader::UpdateBufferWindow() {
   if (!buffer_.get())
     return;
 
-  // Don't adjust buffer window if video is paused.
-  if (playback_rate_ == 0.0)

[acolwell GONE FROM CHROMIUM, 2011/10/11 16:18:01]

Sanity check. This was removed because of the new playback_rate clamping logic
in ComputeTargetBufferWindow()? Do we really want to adjust the buffer size on
pause?

[vrk (LEFT CHROMIUM), 2011/10/11 18:23:21]

+1 what acolwell said; I don't think it makes sense to adjust buffer window on
pause.

[scherkus (not reviewing), 2011/10/11 20:13:19]

Moved to SetPlaybackRate()

[scherkus (not reviewing), 2011/10/11 20:13:19]

Done.

-    return;
-
   size_t backward_capacity;
   size_t forward_capacity;
   ComputeTargetBufferWindow(