Numerous fixes to audio/video buffered resource loading.

webkit/glue/media/buffered_resource_loader.cc

 // Copyright (c) 2011 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 "webkit/glue/media/buffered_resource_loader.h"
 
 #include "base/format_macros.h"
 #include "base/stringprintf.h"
 #include "base/string_util.h"
 #include "media/base/media_log.h"
@@ skipping 17 matching lines @@
 using webkit_glue::MultipartResponseDelegate;
 
 namespace webkit_glue {
 
 static const int kHttpOK = 200;
 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.
 // 20MB is an arbitrary limit; it just seems to be "good enough" in practice.
 static const size_t kMaxBufferCapacity = 20 * kMegabyte;
 
 // Maximum number of bytes outside the buffer we will wait for in order to
 // fulfill a read. If a read starts more than 2MB away from the data we
 // currently have in the buffer, we will not wait for buffer to reach the read's
 // 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),
       saved_forward_capacity_(0),
       url_(url),
       first_byte_position_(first_byte_position),
       last_byte_position_(last_byte_position),
       single_origin_(true),
       start_callback_(NULL),
       offset_(0),
       content_length_(kPositionNotSpecified),
       instance_size_(kPositionNotSpecified),
       read_callback_(NULL),
       read_position_(0),
       read_size_(0),
       read_buffer_(NULL),
       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() {
   if (!completed_ && url_loader_.get())
     url_loader_->cancel();
 }
 
 void BufferedResourceLoader::Start(net::OldCompletionCallback* start_callback,
                                    const base::Closure& event_callback,
                                    WebFrame* frame) {
@@ skipping 130 matching lines @@
 
   // If we expect the read request to be fulfilled later, return
   // and let more data to flow in.
   if (WillFulfillRead()) {
     // If necessary, expand the forward capacity of the buffer to accomodate an
     // unusually large read.
     if (read_size_ > buffer_->forward_capacity()) {
       saved_forward_capacity_ = buffer_->forward_capacity();
       buffer_->set_forward_capacity(read_size_);
     }
-   return;
+    return;
   }
 
   // Make a callback to report failure.
   DoneRead(net::ERR_CACHE_MISS);
 }
 
 int64 BufferedResourceLoader::GetBufferedPosition() {
   if (buffer_.get())
     return offset_ + static_cast<int>(buffer_->forward_bytes()) - 1;
   return kPositionNotSpecified;
@@ skipping 257 matching lines @@
 
 void BufferedResourceLoader::SetBitrate(int bitrate) {
   DCHECK(bitrate >= 0);
   bitrate_ = bitrate;
   UpdateBufferWindow();
 }
 
 /////////////////////////////////////////////////////////////////////////////
 // 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(
       playback_rate_, bitrate_, &backward_capacity, &forward_capacity);
 
   // This does not evict data from the buffer if the new capacities are less
   // than the current capacities; the new limits will be enforced after the
   // existing excess buffered data is consumed.
   buffer_->set_backward_capacity(backward_capacity);
   buffer_->set_forward_capacity(forward_capacity);
@@ skipping 205 matching lines @@
   if (buffer_.get()) {
     media_log_->AddEvent(
         media_log_->CreateBufferedExtentsChangedEvent(
             offset_ - buffer_->backward_bytes(),
             offset_,
             offset_ + buffer_->forward_bytes()));
   }
 }
 
 }  // namespace webkit_glue