Use maximum capacity instead of a ratio of capacity for BufferedResourceLoader.

This change essentially moves internally buffered network data from WebURLLoader to BufferedResourceLoader as soon as there's capacity instead of waiting for an arbitrary threshold.

There's no fear of spamming progress events at a higher frequency since progress events are limited to firing at a minimum interval of 350ms.

BUG=124719
TEST=none

Review URL: https://chromiumcodereview.appspot.com/10694098

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@146133 0039d316-1c4b-4281-b951-d872f2087c98

webkit/media/buffered_data_source.cc

@@ -72,7 +72,7 @@ BufferedResourceLoader* BufferedDataSource::CreateResourceLoader(
 
   BufferedResourceLoader::DeferStrategy strategy = preload_ == METADATA ?
       BufferedResourceLoader::kReadThenDefer :
-      BufferedResourceLoader::kThresholdDefer;
+      BufferedResourceLoader::kCapacityDefer;
 
   return new BufferedResourceLoader(url_,
                                     cors_mode_,
@@ -309,18 +309,18 @@ void BufferedDataSource::SetPlaybackRateTask(float playback_rate) {
     // 200 responses end up not being reused to satisfy future range requests,
     // and we don't want to get too far ahead of the read-head (and thus require
     // a restart), so keep to the thresholds.
-    loader_->UpdateDeferStrategy(BufferedResourceLoader::kThresholdDefer);
+    loader_->UpdateDeferStrategy(BufferedResourceLoader::kCapacityDefer);
   } else if (media_has_played_ && playback_rate == 0) {
     // If the playback has started (at which point the preload value is ignored)
     // and we're paused, then try to load as much as possible (the loader will
-    // fall back to kThresholdDefer if it knows the current response won't be
+    // fall back to kCapacityDefer if it knows the current response won't be
     // useful from the cache in the future).
     loader_->UpdateDeferStrategy(BufferedResourceLoader::kNeverDefer);
   } else {
     // If media is currently playing or the page indicated preload=auto,
     // use threshold strategy to enable/disable deferring when the buffer
     // is full/depleted.
-    loader_->UpdateDeferStrategy(BufferedResourceLoader::kThresholdDefer);
+    loader_->UpdateDeferStrategy(BufferedResourceLoader::kCapacityDefer);
   }
 }
 

webkit/media/buffered_data_source_unittest.cc

@@ -498,7 +498,7 @@ TEST_F(BufferedDataSourceTest, DefaultValues) {
 
   // Ensure we have sane values for default loading scenario.
   EXPECT_EQ(AUTO, preload());
-  EXPECT_EQ(BufferedResourceLoader::kThresholdDefer, defer_strategy());
+  EXPECT_EQ(BufferedResourceLoader::kCapacityDefer, defer_strategy());
 
   EXPECT_EQ(0, data_source_bitrate());
   EXPECT_EQ(0.0f, data_source_playback_rate());

webkit/media/buffered_resource_loader.cc

@@ -53,13 +53,6 @@ static const int kMaxBufferCapacity = 20 * kMegabyte;
 // location and will instead reset the request.
 static const int kForwardWaitThreshold = 2 * kMegabyte;
 
-// The lower bound on our buffer (expressed as a fraction of the buffer size)
-// where we'll disable deferring and continue downloading data.
-//
-// TODO(scherkus): refer to http://crbug.com/124719 for more discussion on
-// how we could improve our buffering logic.
-static const double kDisableDeferThreshold = 0.9;
-
 // 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.
@@ -575,7 +568,7 @@ bool BufferedResourceLoader::DidPassCORSAccessCheck() const {
 
 void BufferedResourceLoader::UpdateDeferStrategy(DeferStrategy strategy) {
   if (!might_be_reused_from_cache_in_future_ && strategy == kNeverDefer)
-    strategy = kThresholdDefer;
+    strategy = kCapacityDefer;
   defer_strategy_ = strategy;
   UpdateDeferBehavior();
 }
@@ -642,8 +635,8 @@ bool BufferedResourceLoader::ShouldEnableDefer() const {
     case kReadThenDefer:
       return read_cb_.is_null();
 
-    // Defer if we've reached the max capacity of the threshold.
-    case kThresholdDefer:
+    // Defer if we've reached max capacity.
+    case kCapacityDefer:
       return buffer_.forward_bytes() >= buffer_.forward_capacity();
   }
   // Otherwise don't enable defer.
@@ -666,15 +659,9 @@ bool BufferedResourceLoader::ShouldDisableDefer() const {
       return !read_cb_.is_null() && last_offset_ > buffer_.forward_bytes();
 
     // Disable deferring whenever our forward-buffered amount falls beneath our
-    // threshold.
-    //
-    // TODO(scherkus): refer to http://crbug.com/124719 for more discussion on
-    // how we could improve our buffering logic.
-    case kThresholdDefer: {
-      int buffered = buffer_.forward_bytes();
-      int threshold = buffer_.forward_capacity() * kDisableDeferThreshold;
-      return buffered < threshold;
-    }
+    // capacity.
+    case kCapacityDefer:
+      return buffer_.forward_bytes() < buffer_.forward_capacity();
   }
 
   // Otherwise keep deferring.

webkit/media/buffered_resource_loader.h

@@ -31,19 +31,17 @@ const char kHttpScheme[] = "http";
 const char kHttpsScheme[] = "https";
 const char kDataScheme[] = "data";
 
-// This class works inside demuxer thread and render thread. It contains a
-// WebURLLoader and does the actual resource loading. This object does
-// buffering internally, it defers the resource loading if buffer is full
-// and un-defers the resource loading if it is under buffered.
+// Wraps a WebURLLoader to maintain an in-memory buffer of downloaded
+// data according to the current defer strategy.
 class BufferedResourceLoader : public WebKit::WebURLLoaderClient {
  public:
   // kNeverDefer - Aggresively buffer; never defer loading while paused.
   // kReadThenDefer - Request only enough data to fulfill read requests.
-  // kThresholdDefer - Try to keep amount of buffered data at a threshold.
+  // kCapacityDefer - Try to keep amount of buffered data at capacity.
   enum DeferStrategy {
     kNeverDefer,
     kReadThenDefer,
-    kThresholdDefer,
+    kCapacityDefer,
   };
 
   // Status codes for start/read operations on BufferedResourceLoader.
@@ -179,7 +177,7 @@ class BufferedResourceLoader : public WebKit::WebURLLoaderClient {
   bool DidPassCORSAccessCheck() const;
 
   // Sets the defer strategy to the given value unless it seems unwise.
-  // Specifically downgrade kNeverDefer to kThresholdDefer if we know the
+  // Specifically downgrade kNeverDefer to kCapacityDefer if we know the
   // current response will not be used to satisfy future requests (the cache
   // won't help us).
   void UpdateDeferStrategy(DeferStrategy strategy);

webkit/media/buffered_resource_loader_unittest.cc

@@ -87,7 +87,7 @@ class BufferedResourceLoaderTest : public testing::Test {
     loader_.reset(new BufferedResourceLoader(
         gurl_, BufferedResourceLoader::kUnspecified,
         first_position_, last_position_,
-        BufferedResourceLoader::kThresholdDefer, 0, 0,
+        BufferedResourceLoader::kCapacityDefer, 0, 0,
         new media::MediaLog()));
 
     // |test_loader_| will be used when Start() is called.
@@ -397,7 +397,7 @@ TEST_F(BufferedResourceLoaderTest, InvalidPartialResponse) {
 // Tests the logic of sliding window for data buffering and reading.
 TEST_F(BufferedResourceLoaderTest, BufferAndRead) {
   Initialize(kHttpUrl, 10, 29);
-  loader_->UpdateDeferStrategy(BufferedResourceLoader::kThresholdDefer);
+  loader_->UpdateDeferStrategy(BufferedResourceLoader::kCapacityDefer);
   Start();
   PartialResponse(10, 29, 30);
 
@@ -657,7 +657,7 @@ TEST_F(BufferedResourceLoaderTest, ReadThenDeferStrategy) {
   StopWhenLoad();
 }
 
-// Tests the data buffering logic of ThresholdDefer strategy.
+// Tests the data buffering logic of kCapacityDefer strategy.
 TEST_F(BufferedResourceLoaderTest, ThresholdDeferStrategy) {
   Initialize(kHttpUrl, 10, 99);
   SetLoaderBuffer(10, 20);
@@ -667,30 +667,22 @@ TEST_F(BufferedResourceLoaderTest, ThresholdDeferStrategy) {
   uint8 buffer[10];
   InSequence s;
 
-  // Write half of threshold: keep not deferring.
+  // Write half of capacity: keep not deferring.
   WriteData(5);
 
-  // Write rest of space until threshold: start deferring.
+  // Write rest of space until capacity: start deferring.
   EXPECT_CALL(*this, LoadingCallback(BufferedResourceLoader::kLoadingDeferred));
   WriteData(5);
 
-  // Read a little from the buffer: keep deferring.
+  // Read a byte from the buffer: stop deferring.
   EXPECT_CALL(*this, ReadCallback(BufferedResourceLoader::kOk, 1));
-  ReadLoader(10, 1, buffer);
-
-  // Read a little more and go under threshold: stop deferring.
-  EXPECT_CALL(*this, ReadCallback(BufferedResourceLoader::kOk, 4));
   EXPECT_CALL(*this, LoadingCallback(BufferedResourceLoader::kLoading));
-  ReadLoader(12, 4, buffer);
+  ReadLoader(10, 1, buffer);
 
-  // Write rest of space until threshold: start deferring.
+  // Write a byte to hit capacity: start deferring.
   EXPECT_CALL(*this, LoadingCallback(BufferedResourceLoader::kLoadingDeferred));
   WriteData(6);
 
-  // Read a little from the buffer: keep deferring.
-  EXPECT_CALL(*this, ReadCallback(BufferedResourceLoader::kOk, 1));
-  ReadLoader(16, 1, buffer);
-
   StopWhenLoad();
 }
 
@@ -706,6 +698,7 @@ TEST_F(BufferedResourceLoaderTest, Tricky_ReadForwardsPastBuffered) {
   // PRECONDITION
   WriteUntilThreshold();
   EXPECT_CALL(*this, ReadCallback(BufferedResourceLoader::kOk, 1));
+  EXPECT_CALL(*this, LoadingCallback(BufferedResourceLoader::kLoading));
   ReadLoader(10, 1, buffer);
   ConfirmBufferState(1, 10, 9, 10);
   ConfirmLoaderOffsets(11, 0, 0);
@@ -722,7 +715,6 @@ TEST_F(BufferedResourceLoaderTest, Tricky_ReadForwardsPastBuffered) {
   // AFTER
   //   offset=24 [__________]
   //
-  EXPECT_CALL(*this, LoadingCallback(BufferedResourceLoader::kLoading));
   ReadLoader(20, 4, buffer);
 
   // Write a little, make sure we didn't start deferring.
@@ -787,7 +779,7 @@ TEST_F(BufferedResourceLoaderTest, Tricky_SmallReadWithinThreshold) {
   ConfirmLoaderOffsets(10, 0, 0);
 
   // *** TRICKY BUSINESS, PT. III ***
-  // Read past forward capacity but within threshold: stop deferring.
+  // Read past forward capacity but within capacity: stop deferring.
   //
   // In order for the read to complete we must:
   //   1) Adjust offset forward to create capacity.
@@ -836,7 +828,7 @@ TEST_F(BufferedResourceLoaderTest, Tricky_LargeReadWithinThreshold) {
 
   // *** TRICKY BUSINESS, PT. IV ***
   // Read a large amount past forward capacity but within
-  // threshold: stop deferring.
+  // capacity: stop deferring.
   //
   // In order for the read to complete we must:
   //   1) Adjust offset forward to create capacity.