Fix media timeline so that thumb never exceeds buffered data

media/base/pipeline_impl.cc

 // Copyright (c) 2012 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.
 //
 // TODO(scherkus): clean up PipelineImpl... too many crazy function names,
 // potential deadlocks, etc...
 
 #include "media/base/pipeline_impl.h"
 
 #include <algorithm>
@@ skipping 220 matching lines @@
   }
 }
 
 base::TimeDelta PipelineImpl::GetCurrentTime() const {
   base::AutoLock auto_lock(lock_);
   return GetCurrentTime_Locked();
 }
 
 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() {
   base::AutoLock auto_lock(lock_);
 
   // 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();
 
   // If buffered time was set, we report that value directly.
   if (buffered_time_.ToInternalValue() > 0)
     return std::max(buffered_time_, current_time);
 
   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 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(
       static_cast<int64>(estimated_rate * (buffered_bytes_ - current_bytes_) +
                          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);
 
   // Only print the max buffered time for smooth buffering.
   max_buffered_time_ = std::max(buffered_time, max_buffered_time_);
 
   return max_buffered_time_;
 }
 
 base::TimeDelta PipelineImpl::GetMediaDuration() const {
   base::AutoLock auto_lock(lock_);
-  return duration_;
+  return clock_->media_duration();
 }
 
 int64 PipelineImpl::GetBufferedBytes() const {
   base::AutoLock auto_lock(lock_);
   return buffered_bytes_;
 }
 
 int64 PipelineImpl::GetTotalBytes() const {
   base::AutoLock auto_lock(lock_);
   return total_bytes_;
@@ skipping 40 matching lines @@
 
 void PipelineImpl::ResetState() {
   base::AutoLock auto_lock(lock_);
   const base::TimeDelta kZero;
   running_          = false;
   stop_pending_     = false;
   seek_pending_     = false;
   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;
   local_source_     = false;
   total_bytes_      = 0;
   natural_size_.SetSize(0, 0);
   volume_           = 1.0f;
   preload_          = AUTO;
   playback_rate_    = 0.0f;
   pending_playback_rate_ = 0.0f;
   status_           = PIPELINE_OK;
   has_audio_        = false;
   has_video_        = false;
   waiting_for_clock_update_ = false;
   audio_disabled_   = false;
-  clock_->SetTime(kZero);
+  clock_->SetTime(kZero, kZero);
   download_rate_monitor_.Reset();
 }
 
 void PipelineImpl::SetState(State next_state) {
   if (state_ != kStarted && next_state == kStarted &&
       !creation_time_.is_null()) {
     UMA_HISTOGRAM_TIMES(
         "Media.TimeToPipelineStarted", base::Time::Now() - creation_time_);
     creation_time_ = base::Time();
   }
@@ skipping 91 matching lines @@
 base::TimeDelta PipelineImpl::GetTime() const {
   DCHECK(IsRunning());
   return GetCurrentTime();
 }
 
 base::TimeDelta PipelineImpl::GetDuration() const {
   DCHECK(IsRunning());
   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) {
   DCHECK(IsRunning());
   media_log_->AddEvent(
       media_log_->CreateTimeEvent(
           MediaLogEvent::DURATION_SET, "duration", 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) {
   DCHECK(IsRunning());
   base::AutoLock auto_lock(lock_);
   buffered_time_ = buffered_time;
 }
 
 void PipelineImpl::SetTotalBytes(int64 total_bytes) {
   DCHECK(IsRunning());
@@ skipping 372 matching lines @@
   // We'll need to pause every filter before seeking.  The state transition
   // is as follows:
   //   kStarted/kEnded
   //   kPausing (for each filter)
   //   kSeeking (for each filter)
   //   kStarting (for each filter)
   //   kStarted
   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!
   {
     base::AutoLock auto_lock(lock_);
     if (clock_->IsPlaying())
       clock_->Pause();
   }
   pipeline_filter_->Pause(
       base::Bind(&PipelineImpl::OnFilterStateTransition, this));
 }
@@ skipping 22 matching lines @@
 
   if (video_renderer_ && !video_renderer_->HasEnded()) {
     return;
   }
 
   // Transition to ended, executing the callback if present.
   SetState(kEnded);
   {
     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()) {
     ended_callback_.Run(status_);
   }
 }
 
 void PipelineImpl::NotifyNetworkEventTask(NetworkEvent type) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   if (!network_callback_.is_null())
@@ skipping 39 matching lines @@
     NOTREACHED() << "Invalid current state: " << state_;
     SetError(PIPELINE_ERROR_ABORT);
     return;
   }
 
   // Decrement the number of remaining transitions, making sure to transition
   // to the next state if needed.
   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.
   if (TransientState(state_)) {
     if (state_ == kPausing) {
       pipeline_filter_->Pause(
           base::Bind(&PipelineImpl::OnFilterStateTransition, this));
     } else if (state_ == kFlushing) {
       pipeline_filter_->Flush(
           base::Bind(&PipelineImpl::OnFilterStateTransition, this));
@@ skipping 18 matching lines @@
     // the seek has compelted.
     if (playback_rate_change_pending_) {
       playback_rate_change_pending_ = false;
       PlaybackRateChangedTask(pending_playback_rate_);
     }
 
     base::AutoLock auto_lock(lock_);
     // 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;
     if (demuxer_.get()) {
       bitrate = demuxer_->GetBitrate();
       local_source_ = demuxer_->IsLocalSource();
       streaming_ = !demuxer_->IsSeekable();
     }
     // Needs to be locked because most other calls to |download_rate_monitor_|
     // occur on the renderer thread.
@@ skipping 110 matching lines @@
     return;
   }
 
   demuxer_ = demuxer;
   demuxer_->set_host(this);
 
   {
     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);
 }
 
 bool PipelineImpl::InitializeAudioDecoder(
     const scoped_refptr<Demuxer>& demuxer) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   DCHECK(IsPipelineOk());
 
@@ skipping 68 matching lines @@
     SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
     return false;
   }
 
   if (!PrepareFilter(audio_renderer_))
     return false;
 
   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;
 }
 
 bool PipelineImpl::InitializeVideoRenderer(
     const scoped_refptr<VideoDecoder>& decoder) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   DCHECK(IsPipelineOk());
 
   if (!decoder)
     return false;
 
   filter_collection_->SelectVideoRenderer(&video_renderer_);
   if (!video_renderer_) {
     SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
     return false;
   }
 
   if (!PrepareFilter(video_renderer_))
     return false;
 
   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;
 }
 
 void PipelineImpl::TearDownPipeline() {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   DCHECK_NE(kStopped, state_);
 
   DCHECK(!tearing_down_ ||  // Teardown on Stop().
          (tearing_down_ && error_caused_teardown_) ||  // Teardown on error.
          (tearing_down_ && stop_pending_));  // Stop during teardown by error.
@@ skipping 144 matching lines @@
 void PipelineImpl::StartClockIfWaitingForTimeUpdate_Locked() {
   lock_.AssertAcquired();
   if (!waiting_for_clock_update_)
     return;
 
   waiting_for_clock_update_ = false;
   clock_->Play();
 }
 
 }  // namespace media