Fire canplaythrough event at the proper time for audio/video

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 cebab46c7f542a2586a2069a4e7766a4fbccfc53..271906cd7fe4984b472524136e97b54d9234072f 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -184,9 +184,8 @@ float PipelineImpl::GetPlaybackRate() const {
 }
 
 void PipelineImpl::SetPlaybackRate(float playback_rate) {
-  if (playback_rate < 0.0f) {
+  if (playback_rate < 0.0f)
     return;
-  }
 
   base::AutoLock auto_lock(lock_);
   playback_rate_ = playback_rate;
@@ -202,9 +201,8 @@ float PipelineImpl::GetVolume() const {
 }
 
 void PipelineImpl::SetVolume(float volume) {
-  if (volume < 0.0f || volume > 1.0f) {
+  if (volume < 0.0f || volume > 1.0f)
     return;
-  }
 
   base::AutoLock auto_lock(lock_);
   volume_ = volume;
@@ -237,6 +235,7 @@ base::TimeDelta PipelineImpl::GetCurrentTime() const {
 }
 
 base::TimeDelta PipelineImpl::GetCurrentTime_Locked() const {
+  lock_.AssertAcquired();
   base::TimeDelta elapsed = clock_->Elapsed();
   if (state_ == kEnded || elapsed > duration_) {
     return duration_;
@@ -246,7 +245,11 @@ base::TimeDelta PipelineImpl::GetCurrentTime_Locked() const {
 
 base::TimeDelta PipelineImpl::GetBufferedTime() {
   base::AutoLock auto_lock(lock_);
+  return GetBufferedTime_Locked();
+}
 
+base::TimeDelta PipelineImpl::GetBufferedTime_Locked() {
+  lock_.AssertAcquired();
   // If media is fully loaded, then return duration.
   if (loaded_ || total_bytes_ == buffered_bytes_) {
     max_buffered_time_ = duration_;
@@ -369,6 +372,11 @@ void PipelineImpl::ResetState() {
   waiting_for_clock_update_ = false;
   audio_disabled_   = false;
   clock_->SetTime(kZero);
+  starting_bytes_loaded_ = 0;
+  starting_time_    = base::Time();
+  has_notified_can_play_through_ = false;
+  is_downloading_data_ = false;
+  last_approximate_download_rate_ = -1;
 }
 
 void PipelineImpl::SetState(State next_state) {
@@ -414,6 +422,9 @@ void PipelineImpl::FinishInitialization() {
     seek_callback_.Run(status_);
     seek_callback_.Reset();
   }
+  is_downloading_data_ = true;
+  starting_time_ = base::Time::Now();
+  NotifyCanPlayThroughIfNeeded();
 }
 
 // static
@@ -514,12 +525,14 @@ void PipelineImpl::SetTotalBytes(int64 total_bytes) {
 
 void PipelineImpl::SetBufferedBytes(int64 buffered_bytes) {
   DCHECK(IsRunning());
-  base::AutoLock auto_lock(lock_);
-
-  // See comments in SetCurrentReadPosition() about capping.
-  if (buffered_bytes < current_bytes_)
-    current_bytes_ = buffered_bytes;
-  buffered_bytes_ = buffered_bytes;
+  {
+    base::AutoLock auto_lock(lock_);
+    // See comments in SetCurrentReadPosition() about capping.
+    if (buffered_bytes < current_bytes_)
+      current_bytes_ = buffered_bytes;
+    buffered_bytes_ = buffered_bytes;
+  }
+  NotifyCanPlayThroughIfNeeded();
 }
 
 void PipelineImpl::SetNaturalVideoSize(const gfx::Size& size) {
@@ -546,6 +559,7 @@ void PipelineImpl::NotifyEnded() {
   message_loop_->PostTask(FROM_HERE,
       base::Bind(&PipelineImpl::NotifyEndedTask, this));
   media_log_->AddEvent(media_log_->CreateEvent(MediaLogEvent::ENDED));
+  NotifyCanPlayThroughIfNeeded();
 }
 
 void PipelineImpl::SetLoaded(bool loaded) {
@@ -560,9 +574,22 @@ void PipelineImpl::SetLoaded(bool loaded) {
 
 void PipelineImpl::SetNetworkActivity(bool is_downloading_data) {
   DCHECK(IsRunning());
+
+  base::AutoLock auto_lock(lock_);

[acolwell GONE FROM CHROMIUM, 2011/10/28 18:24:13]

Why are we doing these operations here instead of in NotifyNetworkEventTask?

[vrk (LEFT CHROMIUM), 2011/11/01 21:57:34]

Hmm, no real reason; I guess I was doing this here because the
NotifyNetworkEventTask didn't take the "is_downloading_data" param.

Updated NotifyNetworkEventTask accordingly.

+  if (is_downloading_data_ == is_downloading_data)
+    return;
+
+  is_downloading_data_ = is_downloading_data;
+  NetworkEvent type = DOWNLOAD_PAUSED;
+  if (is_downloading_data) {
+    type = DOWNLOAD_CONTINUED;
+    // Reset CanPlayThrough-related state when downloading continues.
+    starting_bytes_loaded_ = buffered_bytes_;
+    starting_time_ = base::Time::Now();
+  }
   message_loop_->PostTask(FROM_HERE,
       base::Bind(
-          &PipelineImpl::NotifyNetworkEventTask, this, is_downloading_data));
+          &PipelineImpl::NotifyNetworkEventTask, this, type));
   media_log_->AddEvent(
       media_log_->CreateBooleanEvent(
           MediaLogEvent::NETWORK_ACTIVITY_SET,
@@ -668,7 +695,6 @@ void PipelineImpl::InitializeTask() {
          state_ == kInitVideoDecoder ||
          state_ == kInitVideoRenderer);
 
-
   // Demuxer created, create audio decoder.
   if (state_ == kInitDemuxer) {
     SetState(kInitAudioDecoder);
@@ -926,10 +952,10 @@ void PipelineImpl::NotifyEndedTask() {
   }
 }
 
-void PipelineImpl::NotifyNetworkEventTask(bool is_downloading_data) {
+void PipelineImpl::NotifyNetworkEventTask(NetworkEvent type) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   if (!network_callback_.is_null())
-    network_callback_.Run(is_downloading_data);
+    network_callback_.Run(type);
 }
 
 void PipelineImpl::DisableAudioRendererTask() {
@@ -1375,4 +1401,71 @@ void PipelineImpl::OnDemuxerSeekDone(base::TimeDelta seek_timestamp,
   done_cb.Run(status);
 }
 
+void PipelineImpl::NotifyCanPlayThroughIfNeeded() {
+  if (!IsRunning() || !IsInitialized())
+    return;
+
+  base::AutoLock auto_lock(lock_);
+  if (!ShouldNotifyCanPlayThrough_Locked())
+    return;
+
+  has_notified_can_play_through_ = true;
+  message_loop_->PostTask(FROM_HERE,

[acolwell GONE FROM CHROMIUM, 2011/10/28 18:24:13]

Do we need this PostTask? Aren't we always on the message_loop_ when this is
called? If not, what would we have to do to make it true.

[vrk (LEFT CHROMIUM), 2011/11/01 21:57:34]

I believe we are not on this thread for 2 out of 3 call sites:
SetBufferedBytes() (render thread) and NotifyEnded (audio thread).
Post NotifyCanPlayThrough to message_loop_ at the calling sites?

+      base::Bind(
+         &PipelineImpl::NotifyNetworkEventTask, this, CAN_PLAY_THROUGH));
+}
+
+int PipelineImpl::ApproximateDownloadRate_Locked() {
+  lock_.AssertAcquired();
+
+  // Playback hasn't started yet.
+  if (starting_time_.is_null())
+    return -1;
+
+  float seconds_elapsed = (base::Time::Now() - starting_time_).InSecondsF();
+  DCHECK(seconds_elapsed >= 0);
+  // Update approximation if pipeline is downloading data and has been
+  // downloading data long enough to get an accurate estimate.
+  // XXX: How many seconds should we wait to get an accurate reading?
+  if (is_downloading_data_ && seconds_elapsed > 1) {

[acolwell GONE FROM CHROMIUM, 2011/10/28 18:24:13]

Make the time elapsed threshold a constant and lets use something like 5 seconds
to start with. Also include a comment above the constant indicating that this
value was pulled out of thin error and will need tuning

[vrk (LEFT CHROMIUM), 2011/11/01 21:57:34]

Done.

+    int bytes_downloaded = buffered_bytes_ - starting_bytes_loaded_;
+    DCHECK(bytes_downloaded > 0);

[acolwell GONE FROM CHROMIUM, 2011/10/28 18:24:13]

Should this really be a DCHECK? Shouldn't we just put this as part of the
condition above. What if there happens to be some sort of network stall where we
don't get bytes for a little bit? I don't think we want to crash in that case.
We just want to keep the old estimate.

[vrk (LEFT CHROMIUM), 2011/11/01 21:57:34]

Oops, the ">" was meant to be a ">=". Am fine with changing it to an if test
instead.

+    last_approximate_download_rate_ = bytes_downloaded / seconds_elapsed;
+  }
+  return last_approximate_download_rate_;
+}
+
+bool PipelineImpl::ShouldNotifyCanPlayThrough_Locked() {
+  lock_.AssertAcquired();
+  if (has_notified_can_play_through_ || total_bytes_ == 0)

[acolwell GONE FROM CHROMIUM, 2011/10/28 18:24:13]

Might want to add a comment explaining what total_bytes_ == 0 reflects (ie live
streaming or that we were unable to determine the total size of the stream)

[vrk (LEFT CHROMIUM), 2011/11/01 21:57:34]

Done.

+    return false;
+  if (loaded_ || buffered_bytes_ == total_bytes_)
+    return true;
+
+  int download_rate = ApproximateDownloadRate_Locked();
+  // If download rate is unknown, cannot approximate when the media can play
+  // through.
+  if (download_rate == -1)
+    return false;
+
+  // If we are downloading at or faster than the media's bitrate, then we can
+  // play through to the end of the media without stopping to buffer.
+  int bitrate = demuxer_->GetBitrate();
+  if (download_rate > bitrate * 8)

[acolwell GONE FROM CHROMIUM, 2011/10/28 18:24:13]

Shouldn't this be (8 * download_rate > bitrate) or (download_rate > bitrate / 8)
?

[vrk (LEFT CHROMIUM), 2011/11/01 21:57:34]

D'OH! Yes, absolutely. Done.

+    return true;
+
+  // If the rate of downloading the media is slower than the rate at which the
+  // media is being played back, see if there's enough data buffered to let the
+  // media play through until the end without buffering.
+  base::TimeDelta current_time = GetCurrentTime_Locked();
+  base::TimeDelta buffered_time = GetBufferedTime_Locked();
+  DCHECK(buffered_time >= current_time);
+  float seconds_buffered = (buffered_time - current_time).InSecondsF();

[acolwell GONE FROM CHROMIUM, 2011/10/28 18:24:13]

std::max(..., 0) to protect the computation in release builds.

[vrk (LEFT CHROMIUM), 2011/11/01 21:57:34]

Done.

+  int bytes_downloaded_after_buffer_is_consumed =

[acolwell GONE FROM CHROMIUM, 2011/10/28 18:24:13]

s/after/while/ ?

[vrk (LEFT CHROMIUM), 2011/11/01 21:57:34]

Done.

+      static_cast<int>(seconds_buffered * download_rate);
+  int bytes_left_to_download = total_bytes_ - buffered_bytes_;
+
+  return bytes_downloaded_after_buffer_is_consumed >= bytes_left_to_download;
+}
+
 }  // namespace media