Fix media timeline so that thumb never exceeds buffered data

media/base/pipeline_impl.cc

Index: media/base/pipeline_impl.cc
diff --git a/media/base/pipeline_impl.cc b/media/base/pipeline_impl.cc
index eec4351ecdeeb9b5e994d1e49a1b820a680b2406..060c9fd02354dc69fa49495ee78f38faf9e60092 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -238,11 +238,7 @@ base::TimeDelta PipelineImpl::GetCurrentTime() const {
 
 base::TimeDelta PipelineImpl::GetCurrentTime_Locked() const {
   lock_.AssertAcquired();
-  base::TimeDelta elapsed = clock_->Elapsed();
-  if (elapsed > duration_)
-    return duration_;
-
-  return elapsed;
+  return clock_->Elapsed();
 }
 
 base::TimeDelta PipelineImpl::GetBufferedTime() {
@@ -250,8 +246,8 @@ base::TimeDelta PipelineImpl::GetBufferedTime() {
 
   // If media is fully loaded, then return duration.
   if (local_source_ || total_bytes_ == buffered_bytes_) {
-    max_buffered_time_ = duration_;
-    return duration_;
+    max_buffered_time_ = clock_->media_duration();
+    return max_buffered_time_;
   }
 
   base::TimeDelta current_time = GetCurrentTime_Locked();
@@ -265,7 +261,8 @@ base::TimeDelta PipelineImpl::GetBufferedTime() {
 
   // If buffered time was not set, we use current time, current bytes, and
   // buffered bytes to estimate the buffered time.
-  double estimated_rate = duration_.InMillisecondsF() / total_bytes_;
+  double estimated_rate =
+      clock_->media_duration().InMillisecondsF() / total_bytes_;
   double estimated_current_time = estimated_rate * current_bytes_;
   DCHECK_GE(buffered_bytes_, current_bytes_);
   base::TimeDelta buffered_time = base::TimeDelta::FromMilliseconds(
@@ -273,7 +270,7 @@ base::TimeDelta PipelineImpl::GetBufferedTime() {
                          estimated_current_time));
 
   // Cap approximated buffered time at the length of the video.
-  buffered_time = std::min(buffered_time, duration_);
+  buffered_time = std::min(buffered_time, clock_->media_duration());
 
   // Make sure buffered_time is at least the current time
   buffered_time = std::max(buffered_time, current_time);
@@ -286,7 +283,7 @@ base::TimeDelta PipelineImpl::GetBufferedTime() {
 
 base::TimeDelta PipelineImpl::GetMediaDuration() const {
   base::AutoLock auto_lock(lock_);
-  return duration_;
+  return clock_->media_duration();
 }
 
 int64 PipelineImpl::GetBufferedBytes() const {
@@ -347,7 +344,7 @@ void PipelineImpl::ResetState() {
   tearing_down_     = false;
   error_caused_teardown_ = false;
   playback_rate_change_pending_ = false;
-  duration_         = kZero;
+  clock_->set_media_duration(kZero);

[acolwell GONE FROM CHROMIUM, 2012/01/13 22:51:21]

Replace this and the call below with a clock_->Reset()?

[vrk (LEFT CHROMIUM), 2012/01/21 00:54:14]

Done.

   buffered_time_    = kZero;
   buffered_bytes_   = 0;
   streaming_        = false;
@@ -363,7 +360,7 @@ void PipelineImpl::ResetState() {
   has_video_        = false;
   waiting_for_clock_update_ = false;
   audio_disabled_   = false;
-  clock_->SetTime(kZero);
+  clock_->SetTime(kZero, kZero);
   download_rate_monitor_.Reset();
 }
 
@@ -475,20 +472,31 @@ base::TimeDelta PipelineImpl::GetDuration() const {
   return GetMediaDuration();
 }
 
-void PipelineImpl::SetTime(base::TimeDelta time) {
+void PipelineImpl::OnAudioTimeUpdate(base::TimeDelta time,
+                                     base::TimeDelta max_time) {
+  DCHECK(time <= max_time);
   DCHECK(IsRunning());
   base::AutoLock auto_lock(lock_);
 
-  // If we were waiting for a valid timestamp and such timestamp arrives, we
-  // need to clear the flag for waiting and start the clock.
-  if (waiting_for_clock_update_) {
-    if (time < clock_->Elapsed())
-      return;
-    clock_->SetTime(time);
-    StartClockIfWaitingForTimeUpdate_Locked();
+  if (!has_audio_)
     return;
-  }
-  clock_->SetTime(time);
+  if (waiting_for_clock_update_ && time < clock_->Elapsed())
+    return;
+
+  clock_->SetTime(time, max_time);
+  StartClockIfWaitingForTimeUpdate_Locked();
+}
+
+void PipelineImpl::OnVideoTimeUpdate(base::TimeDelta max_time) {
+  DCHECK(IsRunning());
+  base::AutoLock auto_lock(lock_);
+
+  if (has_audio_)
+    return;
+
+  DCHECK(!waiting_for_clock_update_);
+  DCHECK(clock_->Elapsed() >= max_time);
+  clock_->set_max_time(max_time);
 }
 
 void PipelineImpl::SetDuration(base::TimeDelta duration) {
@@ -499,7 +507,7 @@ void PipelineImpl::SetDuration(base::TimeDelta duration) {
   UMA_HISTOGRAM_LONG_TIMES("Media.Duration", duration);
 
   base::AutoLock auto_lock(lock_);
-  duration_ = duration;
+  clock_->set_media_duration(duration);
 }
 
 void PipelineImpl::SetBufferedTime(base::TimeDelta buffered_time) {
@@ -892,6 +900,7 @@ void PipelineImpl::SeekTask(base::TimeDelta time,
   SetState(kPausing);
   seek_timestamp_ = time;
   seek_callback_ = seek_callback;
+  clock_->set_max_time(seek_timestamp_);

[acolwell GONE FROM CHROMIUM, 2012/01/13 22:51:21]

I'm pretty sure this needs to be inside the locked block below  or TSAN will be
unhappy. 

Why is this needed? It seems like it could cause weird problems if one seeks far
ahead into the future. It would temporarily override the value set by the audio
& video time update calls. I think we still have to worry about those coming in
until the pipeline_filter_->Pause() completes.

[vrk (LEFT CHROMIUM), 2012/01/21 00:54:14]

I believe the reason why I did this was to prevent the thumb from moving forward
in between the start of a seek (where the thumb moves immediately) and the video
catching up to the seek....

... but unfortunately it's hard for me to confirm this remotely :( On second
thought it seems like it would cause problems, as you said. For now I have
deleted the line, and I will test again when I get back to my desk!

 
   // Kick off seeking!
   {
@@ -934,7 +943,7 @@ void PipelineImpl::NotifyEndedTask() {
   {
     base::AutoLock auto_lock(lock_);
     clock_->Pause();
-    clock_->SetTime(duration_);
+    clock_->SetTime(clock_->media_duration(), clock_->media_duration());

[acolwell GONE FROM CHROMIUM, 2012/01/13 22:51:21]

How about clock_->EndOfStream() or something similar? You might consider
transitioning from playing to paused state inside the clock, but I don't know if
that will have an adverse effect elsewhere.

[vrk (LEFT CHROMIUM), 2012/01/21 00:54:14]

Done.
Considering there's a call to Pause() right above this call, looks like it'll be
okay :) I smooshed that into the EndOfStream impl as well.

   }
 
   if (!ended_callback_.is_null()) {
@@ -994,7 +1003,7 @@ void PipelineImpl::FilterStateTransitionTask() {
   SetState(FindNextState(state_));
   if (state_ == kSeeking) {
     base::AutoLock auto_lock(lock_);
-    clock_->SetTime(seek_timestamp_);
+    clock_->SetTime(seek_timestamp_, seek_timestamp_);
   }
 
   // Carry out the action for the current state.
@@ -1033,8 +1042,10 @@ void PipelineImpl::FilterStateTransitionTask() {
     // We use audio stream to update the clock. So if there is such a stream,
     // we pause the clock until we receive a valid timestamp.
     waiting_for_clock_update_ = true;
-    if (!has_audio_)
+    if (!has_audio_) {
+      clock_->set_max_time(clock_->media_duration());
       StartClockIfWaitingForTimeUpdate_Locked();
+    }
 
     // Start monitoring rate of downloading.
     int bitrate = 0;
@@ -1165,7 +1176,7 @@ void PipelineImpl::OnDemuxerBuilt(PipelineStatus status, Demuxer* demuxer) {
     base::AutoLock auto_lock(lock_);
     // We do not want to start the clock running. We only want to set the base
     // media time so our timestamp calculations will be correct.
-    clock_->SetTime(demuxer_->GetStartTime());
+    clock_->SetTime(demuxer_->GetStartTime(), clock_->media_duration());

[acolwell GONE FROM CHROMIUM, 2012/01/13 22:51:21]

I think we might want to use demuxer_->GetStartTime() here for the max too. The
comments above lead me to believe we don't want the clock to move forward so
setting both params to that makes sure that happens.

[vrk (LEFT CHROMIUM), 2012/01/21 00:54:14]

Done.

   }
 
   OnFilterInitialize(PIPELINE_OK);
@@ -1254,7 +1265,9 @@ bool PipelineImpl::InitializeAudioRenderer(
   audio_renderer_->Initialize(
       decoder,
       base::Bind(&PipelineImpl::OnFilterInitialize, this, PIPELINE_OK),
-      base::Bind(&PipelineImpl::OnAudioUnderflow, this));
+      base::Bind(&PipelineImpl::OnAudioUnderflow, this),
+      base::Bind(&PipelineImpl::OnAudioTimeUpdate, this));
+
   return true;
 }
 
@@ -1278,7 +1291,8 @@ bool PipelineImpl::InitializeVideoRenderer(
   video_renderer_->Initialize(
       decoder,
       base::Bind(&PipelineImpl::OnFilterInitialize, this, PIPELINE_OK),
-      base::Bind(&PipelineImpl::OnUpdateStatistics, this));
+      base::Bind(&PipelineImpl::OnUpdateStatistics, this),
+      base::Bind(&PipelineImpl::OnVideoTimeUpdate, this));
   return true;
 }