Fix clock update behavior at the end of stream.

Sometimes the audio stream is slightly shorter than the video stream.
This can result in no audio data being written to the audio device if
you seek close to the end of the clip. This causes the pipeline to
hang because the video stream hasn't ended, but the clock hasn't been
started because no audio data is written to the device. This change
makes sure that the clock gets started if the audio stream has ended
and we are still waiting for a clock update.

I've also included a fix for a related problem where all of the audio
data gets written to the device, but clock updates don't occur on
playback_delay changes. This was contributing to the issue mentioned
above because up to a second worth of audio data can be covered by
the playback_delay. If this happens while seeking to within a second
of the clip end you won't get clock updates for the audio data.

BUG=52887
TEST=PipelineImplTest.AudioStreamShorterThanVideo

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

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

6 files changed, 120 insertions, 21 deletions

diff --git a/media/base/clock.h b/media/base/clock.h
index f586b63..52f3d8a 100644
--- a/media/base/clock.h
+++ b/media/base/clock.h
@@ -16,6 +16,7 @@
 #ifndef MEDIA_BASE_CLOCK_H_
 #define MEDIA_BASE_CLOCK_H_
 
+#include "base/scoped_ptr.h"
 #include "base/time.h"
 
 namespace media {
@@ -42,6 +43,8 @@ class Clock {
   virtual base::TimeDelta Elapsed() const = 0;
 
  protected:
+  // Only allow scoped_ptr<> to delete clocks.
+  friend class scoped_ptr<Clock>;
   virtual ~Clock() {}
 };
 
diff --git a/media/base/clock_impl.cc b/media/base/clock_impl.cc
index 6744f09..4b38eb81 100644
--- a/media/base/clock_impl.cc
+++ b/media/base/clock_impl.cc
@@ -19,7 +19,7 @@ ClockImpl::~ClockImpl() {
 
 base::TimeDelta ClockImpl::Play() {
   DCHECK(!playing_);
-  reference_ = time_provider_();
+  reference_ = GetTimeFromProvider();
   playing_ = true;
   return media_time_;
 }
@@ -38,14 +38,14 @@ void ClockImpl::SetTime(const base::TimeDelta& time) {
     return;
   }
   if (playing_) {
-    reference_ = time_provider_();
+    reference_ = GetTimeFromProvider();
   }
   media_time_ = time;
 }
 
 void ClockImpl::SetPlaybackRate(float playback_rate) {
   if (playing_) {
-    base::Time time = time_provider_();
+    base::Time time = GetTimeFromProvider();
     media_time_ = ElapsedViaProvidedTime(time);
     reference_ = time;
   }
@@ -56,7 +56,7 @@ base::TimeDelta ClockImpl::Elapsed() const {
   if (!playing_) {
     return media_time_;
   }
-  return ElapsedViaProvidedTime(time_provider_());
+  return ElapsedViaProvidedTime(GetTimeFromProvider());
 }
 
 base::TimeDelta ClockImpl::ElapsedViaProvidedTime(
@@ -67,4 +67,10 @@ base::TimeDelta ClockImpl::ElapsedViaProvidedTime(
   return media_time_ + base::TimeDelta::FromMicroseconds(now_us);
 }
 
+base::Time ClockImpl::GetTimeFromProvider() const {
+  if (time_provider_) {
+    return time_provider_();
+  }
+  return base::Time();
+}
 }  // namespace media
diff --git a/media/base/clock_impl.h b/media/base/clock_impl.h
index 689d010..ac44b2b 100644
--- a/media/base/clock_impl.h
+++ b/media/base/clock_impl.h
@@ -35,6 +35,8 @@ class ClockImpl : public Clock {
   // value as returned by |time_provider_|.
   base::TimeDelta ElapsedViaProvidedTime(const base::Time& time) const;
 
+  virtual base::Time GetTimeFromProvider() const;
+
   // Function returning current time in base::Time units.
   TimeProvider* time_provider_;
 
diff --git a/media/base/pipeline_impl.cc b/media/base/pipeline_impl.cc
index 7765cd0..c53044f 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -9,6 +9,7 @@
 #include "base/compiler_specific.h"
 #include "base/condition_variable.h"
 #include "base/stl_util-inl.h"
+#include "media/base/clock_impl.h"
 #include "media/base/media_format.h"
 #include "media/base/pipeline_impl.h"
 
@@ -16,7 +17,7 @@ namespace media {
 
 PipelineImpl::PipelineImpl(MessageLoop* message_loop)
     : message_loop_(message_loop),
-      clock_(&base::Time::Now),
+      clock_(new ClockImpl(&base::Time::Now)),
       waiting_for_clock_update_(false),
       state_(kCreated),
       remaining_transitions_(0),
@@ -179,7 +180,7 @@ base::TimeDelta PipelineImpl::GetCurrentTime() const {
 }
 
 base::TimeDelta PipelineImpl::GetCurrentTime_Locked() const {
-  base::TimeDelta elapsed = clock_.Elapsed();
+  base::TimeDelta elapsed = clock_->Elapsed();
   if (state_ == kEnded || elapsed > duration_) {
     return duration_;
   }
@@ -303,7 +304,7 @@ void PipelineImpl::ResetState() {
   error_            = PIPELINE_OK;
   waiting_for_clock_update_ = false;
   audio_disabled_   = false;
-  clock_.SetTime(kZero);
+  clock_->SetTime(kZero);
   rendered_mime_types_.clear();
 }
 
@@ -411,14 +412,14 @@ void PipelineImpl::SetTime(base::TimeDelta time) {
   // 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())
+    if (time < clock_->Elapsed())
       return;
     waiting_for_clock_update_ = false;
-    clock_.SetTime(time);
-    clock_.Play();
+    clock_->SetTime(time);
+    clock_->Play();
     return;
   }
-  clock_.SetTime(time);
+  clock_->SetTime(time);
 }
 
 void PipelineImpl::SetDuration(base::TimeDelta duration) {
@@ -693,7 +694,7 @@ void PipelineImpl::PlaybackRateChangedTask(float playback_rate) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   {
     AutoLock auto_lock(lock_);
-    clock_.SetPlaybackRate(playback_rate);
+    clock_->SetPlaybackRate(playback_rate);
   }
   for (FilterVector::iterator iter = filters_.begin();
        iter != filters_.end();
@@ -745,7 +746,7 @@ void PipelineImpl::SeekTask(base::TimeDelta time,
     AutoLock auto_lock(lock_);
     // If we are waiting for a clock update, the clock hasn't been played yet.
     if (!waiting_for_clock_update_)
-      clock_.Pause();
+      clock_->Pause();
   }
   filters_.front()->Pause(
       NewCallback(this, &PipelineImpl::OnFilterStateTransition));
@@ -762,8 +763,20 @@ void PipelineImpl::NotifyEndedTask() {
   DCHECK(audio_renderer_ || video_renderer_);
 
   // Make sure every extant renderer has ended.
-  if ((audio_renderer_ && !audio_renderer_->HasEnded() && !audio_disabled_) ||
-      (video_renderer_ && !video_renderer_->HasEnded())) {
+  if (audio_renderer_ && !audio_disabled_) {
+    if (!audio_renderer_->HasEnded()) {
+      return;
+    }
+
+    if (waiting_for_clock_update_) {
+      // Start clock since there is no more audio to
+      // trigger clock updates.
+      waiting_for_clock_update_ = false;
+      clock_->Play();
+    }
+  }
+
+  if (video_renderer_ && !video_renderer_->HasEnded()) {
     return;
   }
 
@@ -821,7 +834,7 @@ void PipelineImpl::FilterStateTransitionTask() {
     state_ = FindNextState(state_);
     if (state_ == kSeeking) {
       AutoLock auto_lock(lock_);
-      clock_.SetTime(seek_timestamp_);
+      clock_->SetTime(seek_timestamp_);
     }
 
     if (TransientState(state_)) {
@@ -866,7 +879,7 @@ void PipelineImpl::FilterStateTransitionTask() {
         rendered_mime_types_.find(mime_type::kMajorTypeAudio) !=
         rendered_mime_types_.end();
     if (!waiting_for_clock_update_)
-      clock_.Play();
+      clock_->Play();
 
     if (IsPipelineStopPending()) {
       // We had a pending stop request need to be honored right now.
diff --git a/media/base/pipeline_impl.h b/media/base/pipeline_impl.h
index f0b9c61..86961cf 100644
--- a/media/base/pipeline_impl.h
+++ b/media/base/pipeline_impl.h
@@ -17,7 +17,7 @@
 #include "base/scoped_ptr.h"
 #include "base/thread.h"
 #include "base/time.h"
-#include "media/base/clock_impl.h"
+#include "media/base/clock.h"
 #include "media/base/filter_host.h"
 #include "media/base/pipeline.h"
 
@@ -331,7 +331,7 @@ class PipelineImpl : public Pipeline, public FilterHost {
   // Reference clock.  Keeps track of current playback time.  Uses system
   // clock and linear interpolation, but can have its time manually set
   // by filters.
-  ClockImpl clock_;
+  scoped_ptr<Clock> clock_;
 
   // If this value is set to true, then |clock_| is paused and we are waiting
   // for an update of the clock greater than or equal to the elapsed time to
@@ -416,6 +416,7 @@ class PipelineImpl : public Pipeline, public FilterHost {
   scoped_ptr<PipelineInitState> pipeline_init_state_;
 
   FRIEND_TEST_ALL_PREFIXES(PipelineImplTest, GetBufferedTime);
+  FRIEND_TEST_ALL_PREFIXES(PipelineImplTest, AudioStreamShorterThanVideo);
 
   DISALLOW_COPY_AND_ASSIGN(PipelineImpl);
 };
diff --git a/media/base/pipeline_impl_unittest.cc b/media/base/pipeline_impl_unittest.cc
index d61da09..f465e52 100644
--- a/media/base/pipeline_impl_unittest.cc
+++ b/media/base/pipeline_impl_unittest.cc
@@ -1,3 +1,4 @@
+
 // Copyright (c) 2010 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.
@@ -6,6 +7,7 @@
 
 #include "base/callback.h"
 #include "base/stl_util-inl.h"
+#include "media/base/clock_impl.h"
 #include "media/base/pipeline_impl.h"
 #include "media/base/media_format.h"
 #include "media/base/filters.h"
@@ -632,8 +634,6 @@ TEST_F(PipelineImplTest, DisableAudioRenderer) {
   mocks_->audio_renderer()->SetPlaybackRate(1.0f);
 
   // Verify that ended event is fired when video ends.
-  EXPECT_CALL(*mocks_->audio_renderer(), HasEnded())
-      .WillOnce(Return(false));
   EXPECT_CALL(*mocks_->video_renderer(), HasEnded())
       .WillOnce(Return(true));
   EXPECT_CALL(callbacks_, OnEnded());
@@ -679,4 +679,78 @@ TEST_F(PipelineImplTest, EndedCallback) {
   host->NotifyEnded();
 }
 
+// Static function & time variable used to simulate changes in wallclock time.
+static int64 g_static_clock_time;
+static base::Time StaticClockFunction() {
+  return base::Time::FromInternalValue(g_static_clock_time);
+}
+
+TEST_F(PipelineImplTest, AudioStreamShorterThanVideo) {
+  base::TimeDelta duration = base::TimeDelta::FromSeconds(10);
+
+  CreateAudioStream();
+  CreateVideoStream();
+  MockDemuxerStreamVector streams;
+  streams.push_back(audio_stream());
+  streams.push_back(video_stream());
+
+  InitializeDataSource();
+  InitializeDemuxer(&streams, duration);
+  InitializeAudioDecoder(audio_stream());
+  InitializeAudioRenderer();
+  InitializeVideoDecoder(video_stream());
+  InitializeVideoRenderer();
+  InitializePipeline();
+
+  // For convenience to simulate filters calling the methods.
+  FilterHost* host = pipeline_;
+
+  // Replace the clock so we can simulate wallclock time advancing w/o using
+  // Sleep.
+  pipeline_->clock_.reset(new ClockImpl(&StaticClockFunction));
+
+  EXPECT_EQ(0, host->GetTime().ToInternalValue());
+
+  float playback_rate = 1.0f;
+  EXPECT_CALL(*mocks_->data_source(), SetPlaybackRate(playback_rate));
+  EXPECT_CALL(*mocks_->demuxer(), SetPlaybackRate(playback_rate));
+  EXPECT_CALL(*mocks_->video_decoder(), SetPlaybackRate(playback_rate));
+  EXPECT_CALL(*mocks_->audio_decoder(), SetPlaybackRate(playback_rate));
+  EXPECT_CALL(*mocks_->video_renderer(), SetPlaybackRate(playback_rate));
+  EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(playback_rate));
+  pipeline_->SetPlaybackRate(playback_rate);
+  message_loop_.RunAllPending();
+
+  InSequence s;
+
+  // Verify that the clock doesn't advance since it hasn't been started by
+  // a time update from the audio stream.
+  int64 start_time = host->GetTime().ToInternalValue();
+  g_static_clock_time +=
+      base::TimeDelta::FromMilliseconds(100).ToInternalValue();
+  EXPECT_EQ(host->GetTime().ToInternalValue(), start_time);
+
+  // Signal end of audio stream.
+  EXPECT_CALL(*mocks_->audio_renderer(), HasEnded())
+      .WillOnce(Return(true));
+  EXPECT_CALL(*mocks_->video_renderer(), HasEnded())
+      .WillOnce(Return(false));
+  host->NotifyEnded();
+  message_loop_.RunAllPending();
+
+  // Verify that the clock advances.
+  start_time = host->GetTime().ToInternalValue();
+  g_static_clock_time +=
+      base::TimeDelta::FromMilliseconds(100).ToInternalValue();
+  EXPECT_GT(host->GetTime().ToInternalValue(), start_time);
+
+  // Signal end of video stream and make sure OnEnded() callback occurs.
+  EXPECT_CALL(*mocks_->audio_renderer(), HasEnded())
+      .WillOnce(Return(true));
+  EXPECT_CALL(*mocks_->video_renderer(), HasEnded())
+      .WillOnce(Return(true));
+  EXPECT_CALL(callbacks_, OnEnded());
+  host->NotifyEnded();
+}
+
 }  // namespace media