Update AudioRenderer, VideoRenderer, and AudioDecoder Initialize() methods to use PipelineStatusCB.

- Fixes race on state_ in CompositeFilter detected by TSAN
- Moves us one step closer to removing FilterHost::SetError().

BUG=109875
TEST=AudioRendererBaseTest.* VideoRendererBaseTest.*, FFmpegAudioDecoderTest.*, CompositeFilterTest.*


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

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

16 files changed, 50 insertions, 65 deletions

diff --git a/media/base/composite_filter.cc b/media/base/composite_filter.cc
index 2b0166c..5089815 100644
--- a/media/base/composite_filter.cc
+++ b/media/base/composite_filter.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -443,17 +443,6 @@ void CompositeFilter::OnStatusCB(const base::Closure& callback,
 }
 
 void CompositeFilter::SetError(PipelineStatus error) {
-  // TODO(acolwell): Temporary hack to handle errors that occur
-  // during filter initialization. In this case we just forward
-  // the error to the host even if it is on the wrong thread. We
-  // have to do this because if we defer the call, we can't be
-  // sure the host will get the error before the "init done" callback
-  // is executed. This will be cleaned up when filter init is refactored.
-  if (state_ == kCreated) {
-    SendErrorToHost(error);
-    return;
-  }
-
   if (message_loop_ != MessageLoop::current()) {
     message_loop_->PostTask(FROM_HERE,
         base::Bind(&CompositeFilter::SetError, this, error));
@@ -461,6 +450,7 @@ void CompositeFilter::SetError(PipelineStatus error) {
   }
 
   DCHECK_EQ(message_loop_, MessageLoop::current());
+  DCHECK_NE(state_, kCreated);
 
   // Drop errors recieved while stopping or stopped.
   // This shields the owner of this object from having
diff --git a/media/base/composite_filter_unittest.cc b/media/base/composite_filter_unittest.cc
index 6b7f219..b5aacc4 100644
--- a/media/base/composite_filter_unittest.cc
+++ b/media/base/composite_filter_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -748,11 +748,6 @@ TEST_F(CompositeFilterTest, TestErrorWhilePlaying) {
 
   SetupAndAdd2Filters();
 
-  // Simulate an error on |filter_2_| while in kCreated state. This
-  // can happen if an error occurs during filter initialization.
-  EXPECT_CALL(*mock_filter_host_, SetError(PIPELINE_ERROR_OUT_OF_MEMORY));
-  filter_2_->host()->SetError(PIPELINE_ERROR_OUT_OF_MEMORY);
-
   DoPlay();
 
   // Simulate an error on |filter_2_| while playing.
diff --git a/media/base/filters.h b/media/base/filters.h
index cf8bcea..4992723 100644
--- a/media/base/filters.h
+++ b/media/base/filters.h
@@ -174,9 +174,8 @@ class MEDIA_EXPORT AudioDecoder : public Filter {
   // Initialize a AudioDecoder with the given DemuxerStream, executing the
   // callback upon completion.
   // stats_callback is used to update global pipeline statistics.
-  //
-  // TODO(scherkus): switch to PipelineStatus callback.
-  virtual void Initialize(DemuxerStream* stream, const base::Closure& callback,
+  virtual void Initialize(DemuxerStream* stream,
+                          const PipelineStatusCB& callback,
                           const StatisticsCallback& stats_callback) = 0;
 
   // Request samples to be decoded and returned via the provided callback.
@@ -207,7 +206,8 @@ class MEDIA_EXPORT VideoRenderer : public Filter {
  public:
   // Initialize a VideoRenderer with the given VideoDecoder, executing the
   // callback upon completion.
-  virtual void Initialize(VideoDecoder* decoder, const base::Closure& callback,
+  virtual void Initialize(VideoDecoder* decoder,
+                          const PipelineStatusCB& callback,
                           const StatisticsCallback& stats_callback) = 0;
 
   // Returns true if this filter has received and processed an end-of-stream
@@ -225,7 +225,7 @@ class MEDIA_EXPORT AudioRenderer : public Filter {
   // to resume playback. Pause(), Seek(), or Stop() cancels the underflow
   // condition.
   virtual void Initialize(AudioDecoder* decoder,
-                          const base::Closure& init_callback,
+                          const PipelineStatusCB& init_callback,
                           const base::Closure& underflow_callback) = 0;
 
   // Returns true if this filter has received and processed an end-of-stream
diff --git a/media/base/mock_filters.cc b/media/base/mock_filters.cc
index ad3bc38..484fe8d 100644
--- a/media/base/mock_filters.cc
+++ b/media/base/mock_filters.cc
@@ -171,10 +171,9 @@ void RunPipelineStatusCB(PipelineStatus status, const PipelineStatusCB& cb) {
   cb.Run(status);
 }
 
-void RunFilterCallback3(::testing::Unused, const base::Closure& callback,
-                        ::testing::Unused) {
-  callback.Run();
-
+void RunPipelineStatusCB3(::testing::Unused, const PipelineStatusCB& callback,
+                          ::testing::Unused) {
+  callback.Run(PIPELINE_OK);
 }
 
 void RunStopFilterCallback(const base::Closure& callback) {
diff --git a/media/base/mock_filters.h b/media/base/mock_filters.h
index 6ccbc32..b2b12c2 100644
--- a/media/base/mock_filters.h
+++ b/media/base/mock_filters.h
@@ -211,7 +211,7 @@ class MockAudioDecoder : public AudioDecoder {
 
   // AudioDecoder implementation.
   MOCK_METHOD3(Initialize, void(DemuxerStream* stream,
-                                const base::Closure& callback,
+                                const PipelineStatusCB& callback,
                                 const StatisticsCallback& stats_callback));
   MOCK_METHOD1(Read, void(const ReadCB& callback));
   MOCK_METHOD1(ProduceAudioSamples, void(scoped_refptr<Buffer>));
@@ -238,7 +238,7 @@ class MockVideoRenderer : public VideoRenderer {
 
   // VideoRenderer implementation.
   MOCK_METHOD3(Initialize, void(VideoDecoder* decoder,
-                                const base::Closure& callback,
+                                const PipelineStatusCB& callback,
                                 const StatisticsCallback& stats_callback));
   MOCK_METHOD0(HasEnded, bool());
 
@@ -265,7 +265,7 @@ class MockAudioRenderer : public AudioRenderer {
 
   // AudioRenderer implementation.
   MOCK_METHOD3(Initialize, void(AudioDecoder* decoder,
-                                const base::Closure& init_callback,
+                                const PipelineStatusCB& init_callback,
                                 const base::Closure& underflow_callback));
   MOCK_METHOD0(HasEnded, bool());
   MOCK_METHOD1(SetVolume, void(float volume));
@@ -317,8 +317,8 @@ class MockFilterCollection {
 void RunFilterCallback(::testing::Unused, const base::Closure& callback);
 void RunFilterStatusCB(::testing::Unused, const FilterStatusCB& cb);
 void RunPipelineStatusCB(PipelineStatus status, const PipelineStatusCB& cb);
-void RunFilterCallback3(::testing::Unused, const base::Closure& callback,
-                        ::testing::Unused);
+void RunPipelineStatusCB3(::testing::Unused, const PipelineStatusCB& callback,
+                          ::testing::Unused);
 
 // Helper gmock function that immediately executes the Closure on behalf of the
 // provided filter.  Can be used when mocking the Stop() method.
diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc
index fb84f2c..359472f 100644
--- a/media/base/pipeline.cc
+++ b/media/base/pipeline.cc
@@ -1175,7 +1175,7 @@ bool Pipeline::InitializeAudioDecoder(
   pipeline_init_state_->audio_decoder_ = audio_decoder;
   audio_decoder->Initialize(
       stream,
-      base::Bind(&Pipeline::OnFilterInitialize, this, PIPELINE_OK),
+      base::Bind(&Pipeline::OnFilterInitialize, this),
       base::Bind(&Pipeline::OnUpdateStatistics, this));
   return true;
 }
@@ -1231,7 +1231,7 @@ bool Pipeline::InitializeAudioRenderer(
 
   audio_renderer_->Initialize(
       decoder,
-      base::Bind(&Pipeline::OnFilterInitialize, this, PIPELINE_OK),
+      base::Bind(&Pipeline::OnFilterInitialize, this),
       base::Bind(&Pipeline::OnAudioUnderflow, this));
   return true;
 }
@@ -1255,7 +1255,7 @@ bool Pipeline::InitializeVideoRenderer(
 
   video_renderer_->Initialize(
       decoder,
-      base::Bind(&Pipeline::OnFilterInitialize, this, PIPELINE_OK),
+      base::Bind(&Pipeline::OnFilterInitialize, this),
       base::Bind(&Pipeline::OnUpdateStatistics, this));
   return true;
 }
diff --git a/media/base/pipeline_unittest.cc b/media/base/pipeline_unittest.cc
index 325431a..58d1aa8 100644
--- a/media/base/pipeline_unittest.cc
+++ b/media/base/pipeline_unittest.cc
@@ -142,7 +142,7 @@ class PipelineTest : public ::testing::Test {
   // Sets up expectations to allow the audio decoder to initialize.
   void InitializeAudioDecoder(MockDemuxerStream* stream) {
     EXPECT_CALL(*mocks_->audio_decoder(), Initialize(stream, _, _))
-        .WillOnce(Invoke(&RunFilterCallback3));
+        .WillOnce(Invoke(&RunPipelineStatusCB3));
     EXPECT_CALL(*mocks_->audio_decoder(), SetPlaybackRate(0.0f));
     EXPECT_CALL(*mocks_->audio_decoder(), Seek(base::TimeDelta(), _))
         .WillOnce(Invoke(&RunFilterStatusCB));
@@ -154,7 +154,7 @@ class PipelineTest : public ::testing::Test {
   void InitializeVideoRenderer() {
     EXPECT_CALL(*mocks_->video_renderer(),
                 Initialize(mocks_->video_decoder(), _, _))
-        .WillOnce(Invoke(&RunFilterCallback3));
+        .WillOnce(Invoke(&RunPipelineStatusCB3));
     EXPECT_CALL(*mocks_->video_renderer(), SetPlaybackRate(0.0f));
     EXPECT_CALL(*mocks_->video_renderer(),
                 Seek(mocks_->demuxer()->GetStartTime(), _))
@@ -168,12 +168,12 @@ class PipelineTest : public ::testing::Test {
     if (disable_after_init_callback) {
       EXPECT_CALL(*mocks_->audio_renderer(),
                   Initialize(mocks_->audio_decoder(), _, _))
-          .WillOnce(DoAll(Invoke(&RunFilterCallback3),
+          .WillOnce(DoAll(Invoke(&RunPipelineStatusCB3),
                           DisableAudioRenderer(mocks_->audio_renderer())));
     } else {
       EXPECT_CALL(*mocks_->audio_renderer(),
                   Initialize(mocks_->audio_decoder(), _, _))
-          .WillOnce(Invoke(&RunFilterCallback3));
+          .WillOnce(Invoke(&RunPipelineStatusCB3));
     }
     EXPECT_CALL(*mocks_->audio_renderer(), SetPlaybackRate(0.0f));
     EXPECT_CALL(*mocks_->audio_renderer(), SetVolume(1.0f));
diff --git a/media/filters/audio_renderer_base.cc b/media/filters/audio_renderer_base.cc
index 23c17a9..7124408 100644
--- a/media/filters/audio_renderer_base.cc
+++ b/media/filters/audio_renderer_base.cc
@@ -83,7 +83,7 @@ void AudioRendererBase::Seek(base::TimeDelta time, const FilterStatusCB& cb) {
 }
 
 void AudioRendererBase::Initialize(AudioDecoder* decoder,
-                                   const base::Closure& init_callback,
+                                   const PipelineStatusCB& init_callback,
                                    const base::Closure& underflow_callback) {
   DCHECK(decoder);
   DCHECK(!init_callback.is_null());
@@ -108,14 +108,13 @@ void AudioRendererBase::Initialize(AudioDecoder* decoder,
 
   // Give the subclass an opportunity to initialize itself.
   if (!OnInitialize(bits_per_channel, channel_layout, sample_rate)) {
-    host()->SetError(PIPELINE_ERROR_INITIALIZATION_FAILED);
-    init_callback.Run();
+    init_callback.Run(PIPELINE_ERROR_INITIALIZATION_FAILED);
     return;
   }
 
   // Finally, execute the start callback.
   state_ = kPaused;
-  init_callback.Run();
+  init_callback.Run(PIPELINE_OK);
 }
 
 bool AudioRendererBase::HasEnded() {
diff --git a/media/filters/audio_renderer_base.h b/media/filters/audio_renderer_base.h
index 4326901..85549ee 100644
--- a/media/filters/audio_renderer_base.h
+++ b/media/filters/audio_renderer_base.h
@@ -42,7 +42,7 @@ class MEDIA_EXPORT AudioRendererBase : public AudioRenderer {
 
   // AudioRenderer implementation.
   virtual void Initialize(AudioDecoder* decoder,
-                          const base::Closure& init_callback,
+                          const PipelineStatusCB& init_callback,
                           const base::Closure& underflow_callback) OVERRIDE;
   virtual bool HasEnded() OVERRIDE;
   virtual void ResumeAfterUnderflow(bool buffer_more_audio) OVERRIDE;
diff --git a/media/filters/audio_renderer_base_unittest.cc b/media/filters/audio_renderer_base_unittest.cc
index 976743c..c08a314 100644
--- a/media/filters/audio_renderer_base_unittest.cc
+++ b/media/filters/audio_renderer_base_unittest.cc
@@ -95,7 +95,7 @@ class AudioRendererBaseTest : public ::testing::Test {
     EXPECT_CALL(*renderer_, OnInitialize(_, _, _))
         .WillOnce(Return(true));
     renderer_->Initialize(
-        decoder_, NewExpectedClosure(), NewUnderflowClosure());
+        decoder_, NewExpectedStatusCB(PIPELINE_OK), NewUnderflowClosure());
   }
 
   void Preroll() {
@@ -221,8 +221,10 @@ class AudioRendererBaseTest : public ::testing::Test {
 TEST_F(AudioRendererBaseTest, Initialize_Failed) {
   EXPECT_CALL(*renderer_, OnInitialize(_, _, _))
       .WillOnce(Return(false));
-  EXPECT_CALL(host_, SetError(PIPELINE_ERROR_INITIALIZATION_FAILED));
-  renderer_->Initialize(decoder_, NewExpectedClosure(), NewUnderflowClosure());
+  renderer_->Initialize(
+      decoder_,
+      NewExpectedStatusCB(PIPELINE_ERROR_INITIALIZATION_FAILED),
+      NewUnderflowClosure());
 
   // We should have no reads.
   EXPECT_TRUE(read_cb_.is_null());
@@ -231,7 +233,8 @@ TEST_F(AudioRendererBaseTest, Initialize_Failed) {
 TEST_F(AudioRendererBaseTest, Initialize_Successful) {
   EXPECT_CALL(*renderer_, OnInitialize(_, _, _))
       .WillOnce(Return(true));
-  renderer_->Initialize(decoder_, NewExpectedClosure(), NewUnderflowClosure());
+  renderer_->Initialize(decoder_, NewExpectedStatusCB(PIPELINE_OK),
+                        NewUnderflowClosure());
 
   // We should have no reads.
   EXPECT_TRUE(read_cb_.is_null());
diff --git a/media/filters/ffmpeg_audio_decoder.cc b/media/filters/ffmpeg_audio_decoder.cc
index a3ad564..608f330 100644
--- a/media/filters/ffmpeg_audio_decoder.cc
+++ b/media/filters/ffmpeg_audio_decoder.cc
@@ -76,7 +76,7 @@ void FFmpegAudioDecoder::Flush(const base::Closure& callback) {
 
 void FFmpegAudioDecoder::Initialize(
     DemuxerStream* stream,
-    const base::Closure& callback,
+    const PipelineStatusCB& callback,
     const StatisticsCallback& stats_callback) {
   // TODO(scherkus): change Initialize() signature to pass |stream| as a
   // scoped_refptr<>.
@@ -108,7 +108,7 @@ int FFmpegAudioDecoder::samples_per_second() {
 
 void FFmpegAudioDecoder::DoInitialize(
     const scoped_refptr<DemuxerStream>& stream,
-    const base::Closure& callback,
+    const PipelineStatusCB& callback,
     const StatisticsCallback& stats_callback) {
   demuxer_stream_ = stream;
   const AudioDecoderConfig& config = stream->audio_decoder_config();
@@ -123,8 +123,7 @@ void FFmpegAudioDecoder::DoInitialize(
                 << " bits per channel: " << config.bits_per_channel()
                 << " samples per second: " << config.samples_per_second();
 
-    host()->SetError(DECODER_ERROR_NOT_SUPPORTED);
-    callback.Run();
+    callback.Run(DECODER_ERROR_NOT_SUPPORTED);
     return;
   }
 
@@ -137,8 +136,7 @@ void FFmpegAudioDecoder::DoInitialize(
     DLOG(ERROR) << "Could not initialize audio decoder: "
                 << codec_context_->codec_id;
 
-    host()->SetError(DECODER_ERROR_NOT_SUPPORTED);
-    callback.Run();
+    callback.Run(DECODER_ERROR_NOT_SUPPORTED);
     return;
   }
 
@@ -147,7 +145,7 @@ void FFmpegAudioDecoder::DoInitialize(
   channel_layout_ = config.channel_layout();
   samples_per_second_ = config.samples_per_second();
 
-  callback.Run();
+  callback.Run(PIPELINE_OK);
 }
 
 void FFmpegAudioDecoder::DoFlush(const base::Closure& callback) {
diff --git a/media/filters/ffmpeg_audio_decoder.h b/media/filters/ffmpeg_audio_decoder.h
index 2ee96ff..0cfd456 100644
--- a/media/filters/ffmpeg_audio_decoder.h
+++ b/media/filters/ffmpeg_audio_decoder.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -25,7 +25,8 @@ class MEDIA_EXPORT FFmpegAudioDecoder : public AudioDecoder {
   virtual void Flush(const base::Closure& callback) OVERRIDE;
 
   // AudioDecoder implementation.
-  virtual void Initialize(DemuxerStream* stream, const base::Closure& callback,
+  virtual void Initialize(DemuxerStream* stream,
+                          const PipelineStatusCB& callback,
                           const StatisticsCallback& stats_callback) OVERRIDE;
   virtual void Read(const ReadCB& callback) OVERRIDE;
   virtual int bits_per_channel() OVERRIDE;
@@ -35,7 +36,7 @@ class MEDIA_EXPORT FFmpegAudioDecoder : public AudioDecoder {
  private:
   // Methods running on decoder thread.
   void DoInitialize(const scoped_refptr<DemuxerStream>& stream,
-                    const base::Closure& callback,
+                    const PipelineStatusCB& callback,
                     const StatisticsCallback& stats_callback);
   void DoFlush(const base::Closure& callback);
   void DoRead(const ReadCB& callback);
diff --git a/media/filters/ffmpeg_audio_decoder_unittest.cc b/media/filters/ffmpeg_audio_decoder_unittest.cc
index 0efe91e..b9f073d 100644
--- a/media/filters/ffmpeg_audio_decoder_unittest.cc
+++ b/media/filters/ffmpeg_audio_decoder_unittest.cc
@@ -70,7 +70,7 @@ class FFmpegAudioDecoderTest : public testing::Test {
         .WillOnce(ReturnRef(config_));
 
     decoder_->Initialize(demuxer_,
-                         NewExpectedClosure(),
+                         NewExpectedStatusCB(PIPELINE_OK),
                          base::Bind(&MockStatisticsCallback::OnStatistics,
                                     base::Unretained(&statistics_callback_)));
 
diff --git a/media/filters/video_renderer_base.cc b/media/filters/video_renderer_base.cc
index 69a6dac..72c8972 100644
--- a/media/filters/video_renderer_base.cc
+++ b/media/filters/video_renderer_base.cc
@@ -100,7 +100,7 @@ void VideoRendererBase::Seek(base::TimeDelta time, const FilterStatusCB& cb) {
 }
 
 void VideoRendererBase::Initialize(VideoDecoder* decoder,
-                                   const base::Closure& callback,
+                                   const PipelineStatusCB& callback,
                                    const StatisticsCallback& stats_callback) {
   base::AutoLock auto_lock(lock_);
   DCHECK(decoder);
@@ -127,8 +127,7 @@ void VideoRendererBase::Initialize(VideoDecoder* decoder,
   if (!base::PlatformThread::Create(0, this, &thread_)) {
     NOTREACHED() << "Video thread creation failed";
     state_ = kError;
-    host()->SetError(PIPELINE_ERROR_INITIALIZATION_FAILED);
-    callback.Run();
+    callback.Run(PIPELINE_ERROR_INITIALIZATION_FAILED);
     return;
   }
 
@@ -137,7 +136,7 @@ void VideoRendererBase::Initialize(VideoDecoder* decoder,
   // TODO(scherkus): find out if this is necessary, but it seems to help.
   ::SetThreadPriority(thread_, THREAD_PRIORITY_ABOVE_NORMAL);
 #endif  // defined(OS_WIN)
-  callback.Run();
+  callback.Run(PIPELINE_OK);
 }
 
 bool VideoRendererBase::HasEnded() {
diff --git a/media/filters/video_renderer_base.h b/media/filters/video_renderer_base.h
index 047e079..2dd2634 100644
--- a/media/filters/video_renderer_base.h
+++ b/media/filters/video_renderer_base.h
@@ -53,7 +53,7 @@ class MEDIA_EXPORT VideoRendererBase
 
   // VideoRenderer implementation.
   virtual void Initialize(VideoDecoder* decoder,
-                          const base::Closure& callback,
+                          const PipelineStatusCB& callback,
                           const StatisticsCallback& stats_callback) OVERRIDE;
   virtual bool HasEnded() OVERRIDE;
 
diff --git a/media/filters/video_renderer_base_unittest.cc b/media/filters/video_renderer_base_unittest.cc
index 7ff3f41..60f224f 100644
--- a/media/filters/video_renderer_base_unittest.cc
+++ b/media/filters/video_renderer_base_unittest.cc
@@ -102,7 +102,8 @@ class VideoRendererBaseTest : public ::testing::Test {
 
     // Initialize, we shouldn't have any reads.
     renderer_->Initialize(decoder_,
-                          NewExpectedClosure(), NewStatisticsCallback());
+                          NewExpectedStatusCB(PIPELINE_OK),
+                          NewStatisticsCallback());
 
     // Now seek to trigger prerolling.
     Seek(0);