More media::PipelineImpl cleanup, this time focusing on not taking down the render process.

Specifically, if the pipeline hasn't started we should fail gracefully and not DCHECK.  Volume and playback rate are now allowed to be set if the pipeline isn't running, and filters will now receive a call to SetVolume() and SetPlaybackRate() with the initial values when the pipeline has fully initialized.

Added tests and expectations for all of this and ran valgrind to verify that we were indeed leaking memory (now fixed).

BUG=16009, 13902
TEST=media_unittests, layout tests

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


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

3 files changed, 145 insertions, 64 deletions

diff --git a/media/base/pipeline_impl.cc b/media/base/pipeline_impl.cc
index 68d509c..098fb18 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -78,8 +78,8 @@ PipelineImpl::~PipelineImpl() {
 bool PipelineImpl::Start(FilterFactory* factory,
                          const std::string& url,
                          PipelineCallback* start_callback) {
-  DCHECK(!pipeline_internal_);
-  DCHECK(factory);
+  DCHECK(!pipeline_internal_) << "PipelineInternal already exists";
+  scoped_ptr<PipelineCallback> callback(start_callback);
   if (pipeline_internal_ || !factory) {
     return false;
   }
@@ -90,29 +90,29 @@ bool PipelineImpl::Start(FilterFactory* factory,
     NOTREACHED() << "Could not create PipelineInternal";
     return false;
   }
-  pipeline_internal_->Start(factory, url, start_callback);
+  pipeline_internal_->Start(factory, url, callback.release());
   return true;
 }
 
 // Stop the PipelineInternal who will NULL our reference to it and reset our
 // state to a newly created PipelineImpl object.
 void PipelineImpl::Stop(PipelineCallback* stop_callback) {
+  scoped_ptr<PipelineCallback> callback(stop_callback);
   if (pipeline_internal_) {
-    pipeline_internal_->Stop(stop_callback);
+    pipeline_internal_->Stop(callback.release());
   }
 }
 
 void PipelineImpl::Seek(base::TimeDelta time,
                         PipelineCallback* seek_callback) {
-  if (IsPipelineOk()) {
-    pipeline_internal_->Seek(time, seek_callback);
-  } else {
-    NOTREACHED();
+  scoped_ptr<PipelineCallback> callback(seek_callback);
+  if (pipeline_internal_) {
+    pipeline_internal_->Seek(time, callback.release());
   }
 }
 
 bool PipelineImpl::IsRunning() const {
-  AutoLock auto_lock(const_cast<Lock&>(lock_));
+  AutoLock auto_lock(lock_);
   return pipeline_internal_ != NULL;
 }
 
@@ -133,12 +133,15 @@ float PipelineImpl::GetPlaybackRate() const {
   return playback_rate_;
 }
 
-void PipelineImpl::SetPlaybackRate(float rate) {
-  if (IsPipelineOk() && rate >= 0.0f) {
-    pipeline_internal_->SetPlaybackRate(rate);
-  } else {
-    // It's OK for a client to call SetPlaybackRate(0.0f) if we're stopped.
-    DCHECK(rate == 0.0f && playback_rate_ == 0.0f);
+void PipelineImpl::SetPlaybackRate(float playback_rate) {
+  if (playback_rate < 0.0f) {
+    return;
+  }
+
+  AutoLock auto_lock(lock_);
+  playback_rate_ = playback_rate;
+  if (pipeline_internal_) {
+    pipeline_internal_->PlaybackRateChanged(playback_rate);
   }
 }
 
@@ -148,10 +151,14 @@ float PipelineImpl::GetVolume() const {
 }
 
 void PipelineImpl::SetVolume(float volume) {
-  if (IsPipelineOk() && volume >= 0.0f && volume <= 1.0f) {
-    pipeline_internal_->SetVolume(volume);
-  } else {
-    NOTREACHED();
+  if (volume < 0.0f || volume > 1.0f) {
+    return;
+  }
+
+  AutoLock auto_lock(lock_);
+  volume_ = volume;
+  if (pipeline_internal_) {
+    pipeline_internal_->VolumeChanged(volume);
   }
 }
 
@@ -244,11 +251,6 @@ void PipelineImpl::SetTime(base::TimeDelta time) {
   time_ = time;
 }
 
-void PipelineImpl::InternalSetPlaybackRate(float rate) {
-  AutoLock auto_lock(lock_);
-  playback_rate_ = rate;
-}
-
 bool PipelineImpl::InternalSetError(PipelineError error) {
   // Don't want callers to set an error of "OK".  STOPPING is a special value
   // that should only be used internally by the StopTask() method.
@@ -306,15 +308,16 @@ void PipelineInternal::Seek(base::TimeDelta time,
 }
 
 // Called on client's thread.
-void PipelineInternal::SetPlaybackRate(float rate) {
+void PipelineInternal::PlaybackRateChanged(float playback_rate) {
   message_loop_->PostTask(FROM_HERE,
-      NewRunnableMethod(this, &PipelineInternal::SetPlaybackRateTask, rate));
+      NewRunnableMethod(this, &PipelineInternal::PlaybackRateChangedTask,
+                        playback_rate));
 }
 
 // Called on client's thread.
-void PipelineInternal::SetVolume(float volume) {
+void PipelineInternal::VolumeChanged(float volume) {
   message_loop_->PostTask(FROM_HERE,
-      NewRunnableMethod(this, &PipelineInternal::SetVolumeTask, volume));
+      NewRunnableMethod(this, &PipelineInternal::VolumeChangedTask, volume));
 }
 
 // Called from any thread.
@@ -436,6 +439,10 @@ void PipelineInternal::InitializeTask() {
       return;
     }
 
+    // Initialization was successful, set the volume and playback rate.
+    PlaybackRateChangedTask(pipeline_->GetPlaybackRate());
+    VolumeChangedTask(pipeline_->GetVolume());
+
     state_ = kStarted;
     filter_factory_ = NULL;
     if (start_callback_.get()) {
@@ -506,14 +513,23 @@ void PipelineInternal::ErrorTask(PipelineError error) {
   DestroyFilters();
 }
 
-void PipelineInternal::SetPlaybackRateTask(float rate) {
+void PipelineInternal::PlaybackRateChangedTask(float playback_rate) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  pipeline_->InternalSetPlaybackRate(rate);
   for (FilterHostVector::iterator iter = filter_hosts_.begin();
        iter != filter_hosts_.end();
        ++iter) {
-    (*iter)->media_filter()->SetPlaybackRate(rate);
+    (*iter)->media_filter()->SetPlaybackRate(playback_rate);
+  }
+}
+
+void PipelineInternal::VolumeChangedTask(float volume) {
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
+
+  scoped_refptr<AudioRenderer> audio_renderer;
+  GetFilter(&audio_renderer);
+  if (audio_renderer) {
+    audio_renderer->SetVolume(volume);
   }
 }
 
@@ -522,6 +538,11 @@ void PipelineInternal::SeekTask(base::TimeDelta time,
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   seek_callback_.reset(seek_callback);
 
+  // Supress seeking if we haven't fully started.
+  if (state_ != kStarted) {
+    return;
+  }
+
   for (FilterHostVector::iterator iter = filter_hosts_.begin();
        iter != filter_hosts_.end();
        ++iter) {
@@ -542,17 +563,6 @@ void PipelineInternal::SeekTask(base::TimeDelta time,
   }
 }
 
-void PipelineInternal::SetVolumeTask(float volume) {
-  DCHECK_EQ(MessageLoop::current(), message_loop_);
-
-  pipeline_->volume_ = volume;
-  scoped_refptr<AudioRenderer> audio_renderer;
-  GetFilter(&audio_renderer);
-  if (audio_renderer) {
-    audio_renderer->SetVolume(volume);
-  }
-}
-
 template <class Filter, class Source>
 void PipelineInternal::CreateFilter(FilterFactory* filter_factory,
                                     Source source,
diff --git a/media/base/pipeline_impl.h b/media/base/pipeline_impl.h
index 2d6b988..911d253 100644
--- a/media/base/pipeline_impl.h
+++ b/media/base/pipeline_impl.h
@@ -73,7 +73,6 @@ class PipelineImpl : public Pipeline {
   void SetBufferedBytes(int64 buffered_bytes);
   void SetVideoSize(size_t width, size_t height);
   void SetTime(base::TimeDelta time);
-  void InternalSetPlaybackRate(float rate);
 
   // Sets the error to the new error code only if the current error state is
   // PIPELINE_OK. Returns true if error set, otherwise leaves current error
@@ -121,17 +120,14 @@ class PipelineImpl : public Pipeline {
   size_t video_width_;
   size_t video_height_;
 
-  // Current volume level (from 0.0f to 1.0f).  The volume reflects the last
-  // value the audio filter was called with SetVolume, so there will be a short
-  // period of time between the client calling SetVolume on the pipeline and
-  // this value being updated.  Set by the PipelineInternal just prior to
-  // calling the audio renderer.
+  // Current volume level (from 0.0f to 1.0f).  This value is set immediately
+  // via SetVolume() and a task is dispatched on the message loop to notify the
+  // filters.
   float volume_;
 
-  // Current playback rate (>= 0.0f).  This member reflects the last value
-  // that the filters in the pipeline were called with, so there will be a short
-  // period of time between the client calling SetPlaybackRate and this value
-  // being updated.  Set by the PipelineInternal just prior to calling filters.
+  // Current playback rate (>= 0.0f).  This value is set immediately via
+  // SetPlaybackRate() and a task is dispatched on the message loop to notify
+  // the filters.
   float playback_rate_;
 
   // Current playback time.  Set by a FilterHostImpl object on behalf of the
@@ -187,8 +183,10 @@ class PipelineInternal : public base::RefCountedThreadSafe<PipelineInternal> {
              PipelineCallback* start_callback);
   void Stop(PipelineCallback* stop_callback);
   void Seek(base::TimeDelta time, PipelineCallback* seek_callback);
-  void SetPlaybackRate(float rate);
-  void SetVolume(float volume);
+
+  // Notifies that the client has changed the playback rate/volume.
+  void PlaybackRateChanged(float playback_rate);
+  void VolumeChanged(float volume);
 
   // Methods called by a FilterHostImpl object.  These methods may be called
   // on any thread, either the pipeline's thread or any other.
@@ -270,9 +268,12 @@ class PipelineInternal : public base::RefCountedThreadSafe<PipelineInternal> {
   void StopTask(PipelineCallback* stop_callback);
   void ErrorTask(PipelineError error);
 
-  void SetPlaybackRateTask(float rate);
+  // Carries out notifying filters that the playback rate/volume has changed,
+  void PlaybackRateChangedTask(float playback_rate);
+  void VolumeChangedTask(float volume);
+
+  // Carries out notifying filters that we are seeking to a new timestamp.
   void SeekTask(base::TimeDelta time, PipelineCallback* seek_callback);
-  void SetVolumeTask(float volume);
 
   // Internal methods used in the implementation of the pipeline thread.  All
   // of these methods are only called on the pipeline thread.
diff --git a/media/base/pipeline_impl_unittest.cc b/media/base/pipeline_impl_unittest.cc
index 1a550eb..8d8053d 100644
--- a/media/base/pipeline_impl_unittest.cc
+++ b/media/base/pipeline_impl_unittest.cc
@@ -27,7 +27,7 @@ class CallbackHelper {
   CallbackHelper() {}
   virtual ~CallbackHelper() {}
 
-  MOCK_METHOD1(OnInitialize, void(bool result));
+  MOCK_METHOD1(OnStart, void(bool result));
   MOCK_METHOD1(OnSeek, void(bool result));
   MOCK_METHOD1(OnStop, void(bool result));
 
@@ -66,6 +66,7 @@ class PipelineImplTest : public ::testing::Test {
     EXPECT_CALL(*mocks_->data_source(), Initialize(""))
         .WillOnce(DoAll(InitializationComplete(mocks_->data_source()),
                         Return(true)));
+    EXPECT_CALL(*mocks_->data_source(), SetPlaybackRate(0.0f));
     EXPECT_CALL(*mocks_->data_source(), Stop());
   }
 
@@ -77,6 +78,7 @@ class PipelineImplTest : public ::testing::Test {
                         Return(true)));
     EXPECT_CALL(*mocks_->demuxer(), GetNumberOfStreams())
         .WillRepeatedly(Return(streams->size()));
+    EXPECT_CALL(*mocks_->demuxer(), SetPlaybackRate(0.0f));
     EXPECT_CALL(*mocks_->demuxer(), Stop());
 
     // Configure the demuxer to return the streams.
@@ -92,6 +94,7 @@ class PipelineImplTest : public ::testing::Test {
     EXPECT_CALL(*mocks_->video_decoder(), Initialize(stream))
         .WillOnce(DoAll(InitializationComplete(mocks_->video_decoder()),
                         Return(true)));
+    EXPECT_CALL(*mocks_->video_decoder(), SetPlaybackRate(0.0f));
     EXPECT_CALL(*mocks_->video_decoder(), Stop());
   }
 
@@ -100,6 +103,7 @@ class PipelineImplTest : public ::testing::Test {
     EXPECT_CALL(*mocks_->audio_decoder(), Initialize(stream))
         .WillOnce(DoAll(InitializationComplete(mocks_->audio_decoder()),
                         Return(true)));
+    EXPECT_CALL(*mocks_->audio_decoder(), SetPlaybackRate(0.0f));
     EXPECT_CALL(*mocks_->audio_decoder(), Stop());
   }
 
@@ -108,6 +112,7 @@ class PipelineImplTest : public ::testing::Test {
     EXPECT_CALL(*mocks_->video_renderer(), Initialize(mocks_->video_decoder()))
         .WillOnce(DoAll(InitializationComplete(mocks_->video_renderer()),
                         Return(true)));
+    EXPECT_CALL(*mocks_->video_renderer(), SetPlaybackRate(0.0f));
     EXPECT_CALL(*mocks_->video_renderer(), Stop());
   }
 
@@ -116,6 +121,8 @@ class PipelineImplTest : public ::testing::Test {
     EXPECT_CALL(*mocks_->audio_renderer(), Initialize(mocks_->audio_decoder()))
         .WillOnce(DoAll(InitializationComplete(mocks_->audio_renderer()),
                         Return(true)));
+    EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(0.0f));
+    EXPECT_CALL(*mocks_->audio_renderer(), SetVolume(0.0f));
     EXPECT_CALL(*mocks_->audio_renderer(), Stop());
   }
 
@@ -123,10 +130,10 @@ class PipelineImplTest : public ::testing::Test {
   // afters tests have set expectations any filters they wish to use.
   void InitializePipeline(bool callback_result) {
     // Expect an initialization callback.
-    EXPECT_CALL(callbacks_, OnInitialize(callback_result));
+    EXPECT_CALL(callbacks_, OnStart(callback_result));
     pipeline_.Start(mocks_, "",
                     NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
-                                &CallbackHelper::OnInitialize));
+                                &CallbackHelper::OnStart));
     message_loop_.RunAllPending();
   }
 
@@ -140,6 +147,59 @@ class PipelineImplTest : public ::testing::Test {
   DISALLOW_COPY_AND_ASSIGN(PipelineImplTest);
 };
 
+// Test that playback controls methods no-op when the pipeline hasn't been
+// started.
+TEST_F(PipelineImplTest, NotStarted) {
+  const base::TimeDelta kZero;
+
+  // StrictMock<> will ensure these never get called, and valgrind/purify will
+  // make sure the callbacks are instantly deleted.
+  pipeline_.Start(NULL, "",
+                  NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
+                              &CallbackHelper::OnStart));
+  pipeline_.Stop(NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
+                             &CallbackHelper::OnStop));
+  pipeline_.Seek(kZero,
+                 NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
+                             &CallbackHelper::OnSeek));
+
+  EXPECT_FALSE(pipeline_.IsRunning());
+  EXPECT_FALSE(pipeline_.IsInitialized());
+  EXPECT_FALSE(pipeline_.IsRendered(""));
+  EXPECT_FALSE(pipeline_.IsRendered(AudioDecoder::major_mime_type()));
+  EXPECT_FALSE(pipeline_.IsRendered(VideoDecoder::major_mime_type()));
+
+  // Setting should still work.
+  EXPECT_EQ(0.0f, pipeline_.GetPlaybackRate());
+  pipeline_.SetPlaybackRate(-1.0f);
+  EXPECT_EQ(0.0f, pipeline_.GetPlaybackRate());
+  pipeline_.SetPlaybackRate(1.0f);
+  EXPECT_EQ(1.0f, pipeline_.GetPlaybackRate());
+
+  // Setting should still work.
+  EXPECT_EQ(0.0f, pipeline_.GetVolume());
+  pipeline_.SetVolume(-1.0f);
+  EXPECT_EQ(0.0f, pipeline_.GetVolume());
+  pipeline_.SetVolume(1.0f);
+  EXPECT_EQ(1.0f, pipeline_.GetVolume());
+
+  EXPECT_TRUE(kZero == pipeline_.GetTime());
+  EXPECT_TRUE(kZero == pipeline_.GetBufferedTime());
+  EXPECT_TRUE(kZero == pipeline_.GetDuration());
+
+  EXPECT_EQ(0, pipeline_.GetBufferedBytes());
+  EXPECT_EQ(0, pipeline_.GetTotalBytes());
+
+  // Should always get set to zero.
+  size_t width = 1u;
+  size_t height = 1u;
+  pipeline_.GetVideoSize(&width, &height);
+  EXPECT_EQ(0u, width);
+  EXPECT_EQ(0u, height);
+
+  EXPECT_EQ(PIPELINE_OK, pipeline_.GetError());
+}
+
 TEST_F(PipelineImplTest, NeverInitializes) {
   EXPECT_CALL(*mocks_->data_source(), Initialize(""))
       .WillOnce(Return(true));
@@ -150,7 +210,7 @@ TEST_F(PipelineImplTest, NeverInitializes) {
   // never executed.
   pipeline_.Start(mocks_, "",
                   NewCallback(reinterpret_cast<CallbackHelper*>(&callbacks_),
-                              &CallbackHelper::OnInitialize));
+                              &CallbackHelper::OnStart));
   message_loop_.RunAllPending();
 
   EXPECT_FALSE(pipeline_.IsInitialized());
@@ -160,7 +220,7 @@ TEST_F(PipelineImplTest, NeverInitializes) {
   // verify that nothing has been called, then set our expectation for the call
   // made during tear down.
   Mock::VerifyAndClear(&callbacks_);
-  EXPECT_CALL(callbacks_, OnInitialize(false));
+  EXPECT_CALL(callbacks_, OnStart(false));
 }
 
 TEST_F(PipelineImplTest, RequiredFilterMissing) {
@@ -185,9 +245,19 @@ TEST_F(PipelineImplTest, URLNotFound) {
 }
 
 TEST_F(PipelineImplTest, NoStreams) {
-  MockDemuxerStreamVector streams;
-  InitializeDataSource();
-  InitializeDemuxer(&streams);
+  // Manually set these expecations because SetPlaybackRate() is not called if
+  // we cannot fully initialize the pipeline.
+  EXPECT_CALL(*mocks_->data_source(), Initialize(""))
+      .WillOnce(DoAll(InitializationComplete(mocks_->data_source()),
+                      Return(true)));
+  EXPECT_CALL(*mocks_->data_source(), Stop());
+
+  EXPECT_CALL(*mocks_->demuxer(), Initialize(mocks_->data_source()))
+      .WillOnce(DoAll(InitializationComplete(mocks_->demuxer()),
+                      Return(true)));
+  EXPECT_CALL(*mocks_->demuxer(), GetNumberOfStreams())
+      .WillRepeatedly(Return(0));
+  EXPECT_CALL(*mocks_->demuxer(), Stop());
 
   InitializePipeline(false);
   EXPECT_FALSE(pipeline_.IsInitialized());