Numerous fixes to audio/video buffered resource loading.

webkit/glue/media/buffered_resource_loader_unittest.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 <algorithm>
 #include <string>
 
 #include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/stringprintf.h"
 #include "media/base/media_log.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_util.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebString.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURLError.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURLRequest.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURLResponse.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebView.h"
 #include "webkit/glue/media/buffered_resource_loader.h"
 #include "webkit/mocks/mock_webframeclient.h"
 #include "webkit/mocks/mock_weburlloader.h"
 
-using ::testing::_;
-using ::testing::Assign;
-using ::testing::AtLeast;
-using ::testing::DeleteArg;
-using ::testing::DoAll;
 using ::testing::InSequence;
-using ::testing::Invoke;
-using ::testing::InvokeWithoutArgs;
-using ::testing::NotNull;
 using ::testing::Return;
-using ::testing::ReturnRef;
-using ::testing::SetArgumentPointee;
-using ::testing::StrictMock;
 using ::testing::Truly;
 using ::testing::NiceMock;
-using ::testing::WithArgs;
 
 using WebKit::WebString;
 using WebKit::WebURLError;
 using WebKit::WebURLResponse;
 using WebKit::WebView;
 
 namespace webkit_glue {
 
 static const char* kHttpUrl = "http://test";
 static const char kHttpRedirectToSameDomainUrl1[] = "http://test/ing";
@@ skipping 42 matching lines @@
   virtual ~BufferedResourceLoaderTest() {
     view_->close();
   }
 
   void Initialize(const char* url, int first_position, int last_position) {
     gurl_ = GURL(url);
     first_position_ = first_position;
     last_position_ = last_position;
 
     url_loader_ = new NiceMock<MockWebURLLoader>();
-    loader_ = new BufferedResourceLoader(gurl_,
-                                         first_position_, last_position_,
-                                         new media::MediaLog());
+    loader_ = new BufferedResourceLoader(
+        gurl_, first_position_, last_position_,
+        BufferedResourceLoader::kThresholdDefer, 0, 0,
+        new media::MediaLog());
     loader_->SetURLLoaderForTest(url_loader_);
   }
 
   void SetLoaderBuffer(size_t forward_capacity, size_t backward_capacity) {
     loader_->buffer_.reset(
         new media::SeekableBuffer(backward_capacity, forward_capacity));
   }
 
   void Start() {
     InSequence s;
@@ skipping 113 matching lines @@
   void ReadLoader(int64 position, int size, uint8* buffer) {
     loader_->Read(position, size, buffer,
                   NewCallback(this, &BufferedResourceLoaderTest::ReadCallback));
   }
 
   // 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 ConfirmLoaderBufferBackwardCapacity(size_t expected_backward_capacity) {
+    EXPECT_EQ(loader_->buffer_->backward_capacity(),
+              expected_backward_capacity);
+  }
+
   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);
   }
 
-  void CheckBufferWindowIsNotTooBig() {
-    // An arbitrarily chosen, reasonable limit.
+  // Makes sure the |loader_| buffer window is in a reasonable range.
+  void CheckBufferWindowBounds() {
+    // Corresponds to value defined in buffered_resource_loader.cc.
+    static const size_t kMinBufferCapacity = 2 * 1024 * 1024;
+    EXPECT_GE(loader_->buffer_->forward_capacity(), kMinBufferCapacity);
+    EXPECT_GE(loader_->buffer_->backward_capacity(), kMinBufferCapacity);
+
+    // Corresponds to value defined in buffered_resource_loader.cc.
     static const size_t kMaxBufferCapacity = 20 * 1024 * 1024;
     EXPECT_LE(loader_->buffer_->forward_capacity(), kMaxBufferCapacity);
     EXPECT_LE(loader_->buffer_->backward_capacity(), kMaxBufferCapacity);
   }
 
-  void CheckBufferWindowIsNotTooSmall() {
-    EXPECT_GT(loader_->buffer_->forward_capacity(), 0u);
-    EXPECT_GT(loader_->buffer_->backward_capacity(), 0u);
-  }
-
-  // Makes sure the |loader_| buffer window is in a reasonable range.
-  void CheckBufferWindowBounds() {
-    CheckBufferWindowIsNotTooSmall();
-    CheckBufferWindowIsNotTooBig();
-  }
-
-  // Updates the |loader_|'s |playback_rate| and |bitrate|, then returns via the
-  // output parameters the resultant change in the forward and backward capacity
-  // of |loader_|'s buffer window.
-  void InvokeChangeInBufferWindow(float playback_rate, int bitrate,
-                                  int* out_forward_capacity_delta,
-                                  int* out_backward_capacity_delta) {
-    CheckBufferWindowBounds();
-    size_t old_forward_capacity = loader_->buffer_->forward_capacity();
-    size_t old_backward_capacity = loader_->buffer_->backward_capacity();
-
-    loader_->SetPlaybackRate(playback_rate);
-    loader_->SetBitrate(bitrate);
-    CheckBufferWindowBounds();
-
-    *out_forward_capacity_delta =
-        loader_->buffer_->forward_capacity() - old_forward_capacity;
-    *out_backward_capacity_delta =
-        loader_->buffer_->backward_capacity() - old_backward_capacity;
-  }
-
   MOCK_METHOD1(StartCallback, void(int error));
   MOCK_METHOD1(ReadCallback, void(int error));
   MOCK_METHOD0(NetworkCallback, void());
 
  protected:
   GURL gurl_;
   int64 first_position_;
   int64 last_position_;
 
   scoped_refptr<BufferedResourceLoader> loader_;
@@ skipping 143 matching lines @@
   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);
 
+  // Don't test for network callbacks (covered by *Strategy tests).
+  EXPECT_CALL(*this, NetworkCallback())
+      .WillRepeatedly(Return());
+
   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);
   Start();
   PartialResponse(10, 0x00FFFFFF, 0x01000000);
 
   uint8 buffer[10];
   InSequence s;
 
   // Read very far ahead will get a cache miss.
   EXPECT_CALL(*this, ReadCallback(net::ERR_CACHE_MISS));
   ReadLoader(0x00FFFFFF, 1, buffer);
 
@@ skipping 180 matching lines @@
 
   // Test redirect to the same domain and then to a different domain.
   Initialize(kHttpUrl, -1, -1);
   Start();
   Redirect(kHttpRedirectToSameDomainUrl1);
   Redirect(kHttpRedirectToDifferentDomainUrl1);
   EXPECT_FALSE(loader_->HasSingleOrigin());
   StopWhenLoad();
 }
 
-TEST_F(BufferedResourceLoaderTest, BufferWindow_Bitrate_Set) {
-  Initialize(kHttpUrl, -1, -1);
-  Start();
-  loader_->SetBitrate(1024 * 8);
-  CheckBufferWindowBounds();
-  StopWhenLoad();
-}
-
-TEST_F(BufferedResourceLoaderTest, BufferWindow_Bitrate_Increase) {
+TEST_F(BufferedResourceLoaderTest, BufferWindow_Default) {
   Initialize(kHttpUrl, -1, -1);
   Start();
 
-  static const int kBitrate = 1024 * 8;
-  static const float kPlaybackRate = 1.0;
-  loader_->SetBitrate(kBitrate);
-  loader_->SetPlaybackRate(kPlaybackRate);
-
-  static const int kNewBitrate = 1024 * 1024 * 8;
-  int forward_capacity_delta;
-  int backward_capacity_delta;
-  InvokeChangeInBufferWindow(kPlaybackRate, kNewBitrate,
-                             &forward_capacity_delta, &backward_capacity_delta);
-
-  EXPECT_GT(forward_capacity_delta, 0);
-  EXPECT_GT(backward_capacity_delta, 0);
+  // Test ensures that default construction of a BufferedResourceLoader has sane
+  // values.
+  //
+  // Please do not change these values in order to make a test pass! Instead,
+  // start a conversation on what the default buffer window capacities should
+  // be.
+  ConfirmLoaderBufferBackwardCapacity(2 * 1024 * 1024);
+  ConfirmLoaderBufferForwardCapacity(2 * 1024 * 1024);
 
   StopWhenLoad();
 }
 
-TEST_F(BufferedResourceLoaderTest, BufferWindow_Bitrate_Decrease) {
-  Initialize(kHttpUrl, -1, -1);
-  Start();
-
-  static const int kBitrate = 1024 * 1024 * 8;
-  static const float kPlaybackRate = 1.0;
-  loader_->SetBitrate(kBitrate);
-  loader_->SetPlaybackRate(kPlaybackRate);
-
-  static const int kNewBitrate = 1024 * 8;
-  int forward_capacity_delta;
-  int backward_capacity_delta;
-  InvokeChangeInBufferWindow(kPlaybackRate, kNewBitrate,
-                             &forward_capacity_delta, &backward_capacity_delta);
-
-  EXPECT_LT(forward_capacity_delta, 0);
-  EXPECT_LT(backward_capacity_delta, 0);
-
-  StopWhenLoad();
-}
-
-TEST_F(BufferedResourceLoaderTest, BufferWindow_SetVeryLarge) {
-  Initialize(kHttpUrl, -1, -1);
-  Start();
-
-  loader_->SetPlaybackRate(1.0);
-  loader_->SetBitrate(100 * 1024 * 1024);
-  CheckBufferWindowBounds();
-
-  loader_->SetBitrate(1024 * 1024 * 8);
-  loader_->SetPlaybackRate(10000.0);
-  CheckBufferWindowBounds();
-
-  loader_->SetPlaybackRate(-10000.0);
-  CheckBufferWindowBounds();
-  StopWhenLoad();
-}
-
-TEST_F(BufferedResourceLoaderTest, BufferWindow_SetVerySmall) {
-  Initialize(kHttpUrl, -1, -1);
-  Start();
-  loader_->SetBitrate(1);
-  CheckBufferWindowBounds();
-
-  // Only check for too big of buffer size in very small playback case; a buffer
-  // window of 0 is reasonable if playback is crawling.
-  loader_->SetPlaybackRate(-0.01f);
-  CheckBufferWindowIsNotTooBig();
-  loader_->SetPlaybackRate(0.01f);
-  CheckBufferWindowIsNotTooBig();
-  StopWhenLoad();
-}
-
-TEST_F(BufferedResourceLoaderTest, BufferWindow_SetUnknownBitrate) {
+TEST_F(BufferedResourceLoaderTest, BufferWindow_Bitrate_Unknown) {
   Initialize(kHttpUrl, -1, -1);
   Start();
   loader_->SetBitrate(0);
   CheckBufferWindowBounds();
   StopWhenLoad();
 }
 
-TEST_F(BufferedResourceLoaderTest, BufferWindow_PlaybackRate_Increase) {
+TEST_F(BufferedResourceLoaderTest, BufferWindow_Bitrate_TooLow) {
   Initialize(kHttpUrl, -1, -1);
   Start();
-
-  static const int kBitrate = 1024 * 8;
-  static const float kPlaybackRate = 1.0;
-  loader_->SetBitrate(kBitrate);
-  loader_->SetPlaybackRate(kPlaybackRate);
-
-  static const float kNewPlaybackRate = 1.5;
-  int forward_capacity_delta;
-  int backward_capacity_delta;
-  InvokeChangeInBufferWindow(kNewPlaybackRate, kBitrate,
-                             &forward_capacity_delta, &backward_capacity_delta);
-
-  EXPECT_GT(forward_capacity_delta, 0);
-  EXPECT_GT(backward_capacity_delta, 0);
-
-  StopWhenLoad();
-}
-
-TEST_F(BufferedResourceLoaderTest, BufferWindow_PlaybackRate_Decrease) {

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

I like the clean-up!

One comment is that the unit tests now only test the clamping behavior of the
buffer window when adjusting the playback rate/bitrate. Is that the only thing
we care to test?

(For example, the old tests included sanity check on increasing/decreasing
window sizes and going from play to pause. Is the ugliness/usefulness trade-off
not worth it, do you think?)

-  Initialize(kHttpUrl, -1, -1);
-  Start();
-
-  static const int kBitrate = 1024 * 8;
-  static const float kPlaybackRate = 1.0;
-  loader_->SetBitrate(kBitrate);
-  loader_->SetPlaybackRate(kPlaybackRate);
-
-  static const float kNewPlaybackRate = 0.5;
-  int forward_capacity_delta;
-  int backward_capacity_delta;
-  InvokeChangeInBufferWindow(kNewPlaybackRate, kBitrate,
-                             &forward_capacity_delta, &backward_capacity_delta);
-
-  EXPECT_LT(forward_capacity_delta, 0);
-  EXPECT_LT(backward_capacity_delta, 0);
-
-  StopWhenLoad();
-}
-
-TEST_F(BufferedResourceLoaderTest, BufferWindow_PlaybackRate_Backwards) {
-  Initialize(kHttpUrl, -1, -1);
-  Start();
-  loader_->SetPlaybackRate(-1.0);
+  loader_->SetBitrate(1024 * 8);  // 1 Kbps.
   CheckBufferWindowBounds();
   StopWhenLoad();
 }
 
-TEST_F(BufferedResourceLoaderTest, BufferWindow_PlayPause) {
+TEST_F(BufferedResourceLoaderTest, BufferWindow_Bitrate_JustRight) {
   Initialize(kHttpUrl, -1, -1);
   Start();
-
-  static const int kBitrate = 1024 * 8;
-  static const float kPlaybackRate = 1.0;
-  loader_->SetBitrate(kBitrate);
-  loader_->SetPlaybackRate(kPlaybackRate);
-
-  static const float kPausedPlaybackRate = 0.0;
-  int forward_capacity_delta;
-  int backward_capacity_delta;
-  InvokeChangeInBufferWindow(kPausedPlaybackRate, kBitrate,
-                             &forward_capacity_delta, &backward_capacity_delta);
-
-  EXPECT_EQ(forward_capacity_delta, 0);
-  EXPECT_EQ(backward_capacity_delta, 0);
-
+  loader_->SetBitrate(2 * 1024 * 1024 * 8);  // 2 Mbps.
+  CheckBufferWindowBounds();
   StopWhenLoad();
 }
 
-// TODO(hclam): add unit test for defer loading.
+TEST_F(BufferedResourceLoaderTest, BufferWindow_Bitrate_TooHigh) {
+  Initialize(kHttpUrl, -1, -1);
+  Start();
+  loader_->SetBitrate(100 * 1024 * 1024 * 8);  // 100 Mbps.
+  CheckBufferWindowBounds();
+  StopWhenLoad();
+}
+
+TEST_F(BufferedResourceLoaderTest, BufferWindow_PlaybackRate_Negative) {
+  Initialize(kHttpUrl, -1, -1);
+  Start();
+  loader_->SetPlaybackRate(-10);
+  CheckBufferWindowBounds();
+  StopWhenLoad();
+}
+
+TEST_F(BufferedResourceLoaderTest, BufferWindow_PlaybackRate_Zero) {
+  Initialize(kHttpUrl, -1, -1);
+  Start();
+  loader_->SetPlaybackRate(0);
+  CheckBufferWindowBounds();
+  StopWhenLoad();
+}
+
+TEST_F(BufferedResourceLoaderTest, BufferWindow_PlaybackRate_TooLow) {

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

Not a fan of the _TooLow, _JustRight, and _TooHigh suffixes. They make me think
"Compared to what?" These tests are to verify clamping behavior so perhaps
something like _BelowLowerBound, _WithinBounds, _AboveUpperBound would be
better.

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

Done.

+  Initialize(kHttpUrl, -1, -1);
+  Start();
+  loader_->SetPlaybackRate(0.1);
+  CheckBufferWindowBounds();
+  StopWhenLoad();
+}
+
+TEST_F(BufferedResourceLoaderTest, BufferWindow_PlaybackRate_JustRight) {
+  Initialize(kHttpUrl, -1, -1);
+  Start();
+  loader_->SetPlaybackRate(10);
+  CheckBufferWindowBounds();
+  StopWhenLoad();
+}
+
+TEST_F(BufferedResourceLoaderTest, BufferWindow_PlaybackRate_TooHigh) {
+  Initialize(kHttpUrl, -1, -1);
+  Start();
+  loader_->SetPlaybackRate(100);
+  CheckBufferWindowBounds();
+  StopWhenLoad();
+}
 
 }  // namespace webkit_glue