Added extending forward_capacity for large reads.

BUG=42956
TEST=test_shell_tests


Review URL: http://codereview.chromium.org/7227016

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

3 files changed, 104 insertions, 19 deletions

diff --git a/webkit/glue/media/buffered_resource_loader.cc b/webkit/glue/media/buffered_resource_loader.cc
index 7829448..e262233 100644
--- a/webkit/glue/media/buffered_resource_loader.cc
+++ b/webkit/glue/media/buffered_resource_loader.cc
@@ -33,29 +33,33 @@ static const int kHttpPartialContent = 206;
 static const size_t kMegabyte = 1024 * 1024;
 
 // Backward capacity of the buffer, by default 2MB.
-static const size_t kBackwardCapcity = 2 * kMegabyte;
+static const size_t kBackwardCapacity = 2 * kMegabyte;
 
 // Forward capacity of the buffer, by default 10MB.
 static const size_t kForwardCapacity = 10 * kMegabyte;
 
-// The threshold of bytes that we should wait until the data arrives in the
-// future instead of restarting a new connection. This number is defined in the
-// number of bytes, we should determine this value from typical connection speed
-// and amount of time for a suitable wait. Now I just make a guess for this
-// number to be 2MB.
-// TODO(hclam): determine a better value for this.
+// Maximum forward capacity of the buffer, by default 20MB. This is effectively
+// the largest single read teh code path can handle. 20MB is an arbitrary limit;
+// it just seems to be "good enough" in practice.
+static const size_t kMaxForwardCapacity = 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;
 
 BufferedResourceLoader::BufferedResourceLoader(
     const GURL& url,
     int64 first_byte_position,
     int64 last_byte_position)
-    : buffer_(new media::SeekableBuffer(kBackwardCapcity, kForwardCapacity)),
+    : buffer_(new media::SeekableBuffer(kBackwardCapacity, kForwardCapacity)),
       deferred_(false),
       defer_strategy_(kReadThenDefer),
       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),
@@ -161,6 +165,7 @@ void BufferedResourceLoader::Read(int64 position,
   DCHECK(buffer_.get());
   DCHECK(read_callback);
   DCHECK(buffer);
+  DCHECK_GT(read_size, 0);
 
   // Save the parameter of reading.
   read_callback_.reset(read_callback);
@@ -183,6 +188,13 @@ void BufferedResourceLoader::Read(int64 position,
     return;
   }
 
+  // Make sure |read_size_| is not too large for the buffer to ever be able to
+  // fulfill the read request.
+  if (read_size_ > kMaxForwardCapacity) {
+    DoneRead(net::ERR_FAILED);
+    return;
+  }
+
   // Prepare the parameters.
   first_offset_ = static_cast<int>(read_position_ - offset_);
   last_offset_ = first_offset_ + read_size_;
@@ -199,10 +211,17 @@ void BufferedResourceLoader::Read(int64 position,
   // Update defer behavior to re-enable deferring if need be.
   UpdateDeferBehavior();
 
-  // If we expected the read request to be fulfilled later, returns
-  // immediately and let more data to flow in.
-  if (WillFulfillRead())
-    return;
+  // 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;
+  }
 
   // Make a callback to report failure.
   DoneRead(net::ERR_CACHE_MISS);
@@ -562,13 +581,14 @@ bool BufferedResourceLoader::CanFulfillRead() {
 }
 
 bool BufferedResourceLoader::WillFulfillRead() {
-  // Reading too far in the backward direction.
+  // Trying to read too far behind.
   if (first_offset_ < 0 &&
       first_offset_ + static_cast<int>(buffer_->backward_bytes()) < 0)
     return false;
 
-  // Try to read too far ahead.
-  if (last_offset_ > kForwardWaitThreshold)
+  // Trying to read too far ahead.
+  if (first_offset_ - static_cast<int>(buffer_->forward_bytes()) >
+      kForwardWaitThreshold)
     return false;
 
   // The resource request has completed, there's no way we can fulfill the
@@ -641,6 +661,10 @@ std::string BufferedResourceLoader::GenerateHeaders(
 void BufferedResourceLoader::DoneRead(int error) {
   read_callback_->RunWithParams(Tuple1<int>(error));
   read_callback_.reset();
+  if (buffer_.get() && saved_forward_capacity_) {
+    buffer_->set_forward_capacity(saved_forward_capacity_);
+    saved_forward_capacity_ = 0;
+  }
   read_position_ = 0;
   read_size_ = 0;
   read_buffer_ = NULL;
diff --git a/webkit/glue/media/buffered_resource_loader.h b/webkit/glue/media/buffered_resource_loader.h
index 39e89d8..43a17d6 100644
--- a/webkit/glue/media/buffered_resource_loader.h
+++ b/webkit/glue/media/buffered_resource_loader.h
@@ -79,7 +79,8 @@ class BufferedResourceLoader :
 
   // Reads the specified |read_size| from |position| into |buffer| and when
   // the operation is done invoke |callback| with number of bytes read or an
-  // error code.
+  // error code. If necessary, will temporarily increase forward capacity of
+  // buffer to accomodate an unusually large read.
   // |callback| is called with the following values:
   // - (Anything greater than or equal 0)
   //   Read was successful with the indicated number of bytes read.
@@ -228,6 +229,9 @@ class BufferedResourceLoader :
   // True if Range header is supported.
   bool range_supported_;
 
+  // Forward capacity to reset to after an extension.
+  size_t saved_forward_capacity_;
+
   // Does the work of loading and sends data back to this client.
   scoped_ptr<WebKit::WebURLLoader> url_loader_;
 
@@ -249,7 +253,7 @@ class BufferedResourceLoader :
   // read has completed or failed.
   scoped_ptr<net::CompletionCallback> read_callback_;
   int64 read_position_;
-  int read_size_;
+  size_t read_size_;
   uint8* read_buffer_;
 
   // Offsets of the requested first byte and last byte in |buffer_|. They are
diff --git a/webkit/glue/media/buffered_resource_loader_unittest.cc b/webkit/glue/media/buffered_resource_loader_unittest.cc
index df44c48..034c1b3 100644
--- a/webkit/glue/media/buffered_resource_loader_unittest.cc
+++ b/webkit/glue/media/buffered_resource_loader_unittest.cc
@@ -227,11 +227,15 @@ class BufferedResourceLoaderTest : public testing::Test {
                   NewCallback(this, &BufferedResourceLoaderTest::ReadCallback));
   }
 
-  // Verifis that data in buffer[0...size] is equal to data_[pos...pos+size].
+  // Verifies that data in buffer[0...size] is equal to data_[pos...pos+size].
   void VerifyBuffer(uint8* buffer, int pos, int size) {
     EXPECT_EQ(0, memcmp(buffer, data_ + pos, size));
   }
 
+  void ConfirmLoaderBufferForwardCapacity(size_t expected_forward_capacity) {
+    EXPECT_EQ(loader_->buffer_->forward_capacity(), expected_forward_capacity);
+  }
+
   void ConfirmLoaderDeferredState(bool expectedVal) {
     EXPECT_EQ(loader_->deferred_, expectedVal);
   }
@@ -392,6 +396,59 @@ TEST_F(BufferedResourceLoaderTest, BufferAndRead) {
   ReadLoader(30, 10, buffer);
 }
 
+// Tests the logic of expanding the data buffer for large reads.
+TEST_F(BufferedResourceLoaderTest, ReadExtendBuffer) {
+  Initialize(kHttpUrl, 10, 0x014FFFFFF);
+  SetLoaderBuffer(10, 20);
+  Start();
+  PartialResponse(10, 0x014FFFFFF, 0x01500000);
+
+  uint8 buffer[20];
+  InSequence s;
+
+  // Write more than forward capacity and read it back. Ensure forward capacity
+  // gets reset.
+  EXPECT_CALL(*this, NetworkCallback());
+  WriteLoader(10, 20);
+  EXPECT_CALL(*this, ReadCallback(20));
+  ReadLoader(10, 20, buffer);
+
+  VerifyBuffer(buffer, 10, 20);
+  ConfirmLoaderBufferForwardCapacity(10);
+
+  // Make and outstanding read request larger than forward capacity. Ensure
+  // forward capacity gets extended.
+  EXPECT_CALL(*this, NetworkCallback());
+  ReadLoader(30, 20, buffer);
+
+  ConfirmLoaderBufferForwardCapacity(20);
+
+  // Fulfill outstanding request. Ensure forward capacity gets reset.
+  EXPECT_CALL(*this, ReadCallback(20));
+  EXPECT_CALL(*this, NetworkCallback());
+  WriteLoader(30, 20);
+
+  VerifyBuffer(buffer, 30, 20);
+  ConfirmLoaderBufferForwardCapacity(10);
+
+  // Try to read further ahead than kForwardWaitThreshold allows. Ensure
+  // forward capacity is not changed.
+  EXPECT_CALL(*this, NetworkCallback());
+  EXPECT_CALL(*this, ReadCallback(net::ERR_CACHE_MISS));
+  ReadLoader(0x00300000, 1, buffer);
+
+  ConfirmLoaderBufferForwardCapacity(10);
+
+  // Try to read more than maximum forward capacity. Ensure forward capacity is
+  // not changed.
+  EXPECT_CALL(*this, ReadCallback(net::ERR_FAILED));
+  ReadLoader(30, 0x01400001, buffer);
+
+  ConfirmLoaderBufferForwardCapacity(10);
+
+  StopWhenLoad();
+}
+
 TEST_F(BufferedResourceLoaderTest, ReadOutsideBuffer) {
   Initialize(kHttpUrl, 10, 0x00FFFFFF);
   loader_->UpdateDeferStrategy(BufferedResourceLoader::kThresholdDefer);
@@ -401,7 +458,7 @@ TEST_F(BufferedResourceLoaderTest, ReadOutsideBuffer) {
   uint8 buffer[10];
   InSequence s;
 
-  // Read very far aheard will get a cache miss.
+  // Read very far ahead will get a cache miss.
   EXPECT_CALL(*this, ReadCallback(net::ERR_CACHE_MISS));
   ReadLoader(0x00FFFFFF, 1, buffer);