Fixing rounding error in buffering bar for fully loaded HTTP videos

Added logic in the buffering code to no longer estimate buffered time if
the video is fully loaded over HTTP. Also corrected some erroneous buffering
logic and fixed tests to match the revised expectations.

BUG=50570
TEST=media_unittests

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

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

2 files changed, 30 insertions, 14 deletions

diff --git a/media/base/pipeline_impl.cc b/media/base/pipeline_impl.cc
index 73af164..75c522f 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -215,21 +215,30 @@ base::TimeDelta PipelineImpl::GetBufferedTime() {
   AutoLock auto_lock(lock_);
 
   // If media is fully loaded, then return duration.
-  if (loaded_)
+  if (loaded_ || total_bytes_ == buffered_bytes_) {
+    max_buffered_time_ = duration_;
     return duration_;
+  }
 
   // If buffered time was set, we report that value directly.
   if (buffered_time_.ToInternalValue() > 0)
     return buffered_time_;
 
-  if (total_bytes_ == 0 || current_bytes_ == 0)
+  if (total_bytes_ == 0)
     return base::TimeDelta();
 
   // If buffered time was not set, we use current time, current bytes, and
   // buffered bytes to estimate the buffered time.
   double current_time = static_cast<double>(current_bytes_) / total_bytes_ *
                         duration_.InMilliseconds();
-  double rate = current_time / current_bytes_;
+  double rate = 0.0;
+  if (current_bytes_ == 0) {
+    // If we haven't read any bytes, use the length/size of entire video to
+    // estimate rate.
+    rate = static_cast<double>(duration_.InMilliseconds()) / total_bytes_;
+  } else {
+    rate = current_time / current_bytes_;
+  }
   DCHECK_GE(buffered_bytes_, current_bytes_);
   base::TimeDelta buffered_time = base::TimeDelta::FromMilliseconds(
       static_cast<int64>(rate * (buffered_bytes_ - current_bytes_) +
diff --git a/media/base/pipeline_impl_unittest.cc b/media/base/pipeline_impl_unittest.cc
index 36c8019..181f07d 100644
--- a/media/base/pipeline_impl_unittest.cc
+++ b/media/base/pipeline_impl_unittest.cc
@@ -497,7 +497,10 @@ TEST_F(PipelineImplTest, Properties) {
             pipeline_->GetMediaDuration().ToInternalValue());
   EXPECT_EQ(kTotalBytes, pipeline_->GetTotalBytes());
   EXPECT_EQ(kBufferedBytes, pipeline_->GetBufferedBytes());
-  EXPECT_EQ(0,
+
+  // Because kTotalBytes and kBufferedBytes are equal to each other,
+  // the entire video should be buffered.
+  EXPECT_EQ(kDuration.ToInternalValue(),
             pipeline_->GetBufferedTime().ToInternalValue());
 }
 
@@ -516,25 +519,29 @@ TEST_F(PipelineImplTest, GetBufferedTime) {
   EXPECT_TRUE(pipeline_->IsInitialized());
   EXPECT_EQ(PIPELINE_OK, pipeline_->GetError());
 
-  // If media is loaded, we should return duration of media.
-  pipeline_->SetLoaded(true);
-  EXPECT_EQ(kDuration.ToInternalValue(),
-            pipeline_->GetBufferedTime().ToInternalValue());
-  pipeline_->SetLoaded(false);
+  // TODO(vrk): The following mini-test cases are order-dependent, and should
+  // probably be separated into independent test cases.
 
-  // Buffered time is 0 if no bytes are buffered or read.
+  // Buffered time is 0 if no bytes are buffered.
   pipeline_->SetBufferedBytes(0);
   EXPECT_EQ(0, pipeline_->GetBufferedTime().ToInternalValue());
-  pipeline_->SetBufferedBytes(kBufferedBytes);
-
-  pipeline_->SetCurrentReadPosition(0);
-  EXPECT_EQ(0, pipeline_->GetBufferedTime().ToInternalValue());
 
   // We should return buffered_time_ if it is set and valid.
   const base::TimeDelta buffered = base::TimeDelta::FromSeconds(10);
   pipeline_->SetBufferedTime(buffered);
   EXPECT_EQ(buffered.ToInternalValue(),
             pipeline_->GetBufferedTime().ToInternalValue());
+
+  // If media has been fully received, we should return the duration
+  // of the media.
+  pipeline_->SetBufferedBytes(kTotalBytes);
+  EXPECT_EQ(kDuration.ToInternalValue(),
+            pipeline_->GetBufferedTime().ToInternalValue());
+
+  // If media is loaded, we should return duration of media.
+  pipeline_->SetLoaded(true);
+  EXPECT_EQ(kDuration.ToInternalValue(),
+            pipeline_->GetBufferedTime().ToInternalValue());
 }
 
 TEST_F(PipelineImplTest, DisableAudioRenderer) {