From 5badb08f6f862645e950d97c7a58995724cd0c42 Mon Sep 17 00:00:00 2001 From: "boliu@chromium.org" Date: Fri, 11 Jun 2010 17:40:15 +0000 Subject: Make MediaFilter::Stop() asynchronous. This is the second issue regarding making Stop() asynchronous. All Stop() in subclasses of MediaFilter is made to accept a callback and runs the callback at the end of its stop. BUG=16059 TEST=media_unittest, unit_tests, test_shell_tests still passes Review URL: http://codereview.chromium.org/2541003 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@49548 0039d316-1c4b-4281-b951-d872f2087c98 --- media/base/filters.h | 14 ++++++-------- media/filters/audio_renderer_base.cc | 14 ++++++++++---- media/filters/audio_renderer_base.h | 2 +- media/filters/audio_renderer_base_unittest.cc | 4 +++- media/filters/decoder_base.h | 11 ++++++++--- media/filters/decoder_base_unittest.cc | 4 +++- media/filters/ffmpeg_demuxer.cc | 10 +++++++--- media/filters/ffmpeg_demuxer.h | 4 ++-- media/filters/ffmpeg_demuxer_unittest.cc | 18 ++++++++++++------ media/filters/file_data_source.cc | 10 +++++++--- media/filters/file_data_source.h | 4 ++-- media/filters/file_data_source_unittest.cc | 12 ++++++++++++ media/filters/null_audio_renderer.cc | 4 ++-- media/filters/omx_video_decode_engine.h | 2 +- media/filters/omx_video_decoder.cc | 7 ++----- media/filters/omx_video_decoder.h | 2 +- media/filters/video_decoder_impl_unittest.cc | 4 +++- media/filters/video_renderer_base.cc | 7 ++++++- media/filters/video_renderer_base.h | 2 +- media/filters/video_renderer_base_unittest.cc | 4 +++- 20 files changed, 92 insertions(+), 47 deletions(-) (limited to 'media') diff --git a/media/base/filters.h b/media/base/filters.h index 141a753..80a3bd2 100644 --- a/media/base/filters.h +++ b/media/base/filters.h @@ -91,7 +91,9 @@ class MediaFilter : public base::RefCountedThreadSafe { // The pipeline has resumed playback. Filters can continue requesting reads. // Filters may implement this method if they need to respond to this call. + // TODO(boliu): Check that callback is not NULL in sublcasses. virtual void Play(FilterCallback* callback) { + DCHECK(callback); if (callback) { callback->Run(); delete callback; @@ -101,24 +103,20 @@ class MediaFilter : public base::RefCountedThreadSafe { // The pipeline has paused playback. Filters should fulfill any existing read // requests and then idle. Filters may implement this method if they need to // respond to this call. + // TODO(boliu): Check that callback is not NULL in sublcasses. virtual void Pause(FilterCallback* callback) { + DCHECK(callback); if (callback) { callback->Run(); delete callback; } } - // TODO(boliu): Remove once Stop() is asynchronous in subclasses. - virtual void Stop() {} - // The pipeline is being stopped either as a result of an error or because // the client called Stop(). - // TODO(boliu): No implementation in subclasses yet. + // TODO(boliu): Check that callback is not NULL in sublcasses. virtual void Stop(FilterCallback* callback) { - // TODO(boliu): Call the synchronous version for now. Remove once - // all filters have asynchronous stop. - Stop(); - + DCHECK(callback); if (callback) { callback->Run(); delete callback; diff --git a/media/filters/audio_renderer_base.cc b/media/filters/audio_renderer_base.cc index 7e530a8..f3f9294 100644 --- a/media/filters/audio_renderer_base.cc +++ b/media/filters/audio_renderer_base.cc @@ -48,11 +48,17 @@ void AudioRendererBase::Pause(FilterCallback* callback) { } } -void AudioRendererBase::Stop() { +void AudioRendererBase::Stop(FilterCallback* callback) { OnStop(); - AutoLock auto_lock(lock_); - state_ = kStopped; - algorithm_.reset(NULL); + { + AutoLock auto_lock(lock_); + state_ = kStopped; + algorithm_.reset(NULL); + } + if (callback) { + callback->Run(); + delete callback; + } } void AudioRendererBase::Seek(base::TimeDelta time, FilterCallback* callback) { diff --git a/media/filters/audio_renderer_base.h b/media/filters/audio_renderer_base.h index 3762679..279d59f 100644 --- a/media/filters/audio_renderer_base.h +++ b/media/filters/audio_renderer_base.h @@ -33,7 +33,7 @@ class AudioRendererBase : public AudioRenderer { // MediaFilter implementation. virtual void Play(FilterCallback* callback); virtual void Pause(FilterCallback* callback); - virtual void Stop(); + virtual void Stop(FilterCallback* callback); virtual void Seek(base::TimeDelta time, FilterCallback* callback); diff --git a/media/filters/audio_renderer_base_unittest.cc b/media/filters/audio_renderer_base_unittest.cc index 064f7a6..20d2912 100644 --- a/media/filters/audio_renderer_base_unittest.cc +++ b/media/filters/audio_renderer_base_unittest.cc @@ -70,7 +70,9 @@ class AudioRendererBaseTest : public ::testing::Test { virtual ~AudioRendererBaseTest() { // Expect a call into the subclass. EXPECT_CALL(*renderer_, OnStop()); - renderer_->Stop(); + EXPECT_CALL(callback_, OnFilterCallback()); + EXPECT_CALL(callback_, OnCallbackDestroyed()); + renderer_->Stop(callback_.NewCallback()); } protected: diff --git a/media/filters/decoder_base.h b/media/filters/decoder_base.h index cc0e3cb..82f767e 100644 --- a/media/filters/decoder_base.h +++ b/media/filters/decoder_base.h @@ -30,9 +30,9 @@ class DecoderBase : public Decoder { typedef CallbackRunner< Tuple1 > ReadCallback; // MediaFilter implementation. - virtual void Stop() { + virtual void Stop(FilterCallback* callback) { this->message_loop()->PostTask(FROM_HERE, - NewRunnableMethod(this, &DecoderBase::StopTask)); + NewRunnableMethod(this, &DecoderBase::StopTask, callback)); } virtual void Seek(base::TimeDelta time, @@ -146,7 +146,7 @@ class DecoderBase : public Decoder { NewRunnableMethod(this, &DecoderBase::ReadCompleteTask, buffer_ref)); } - void StopTask() { + void StopTask(FilterCallback* callback) { DCHECK_EQ(MessageLoop::current(), this->message_loop()); // Delegate to the subclass first. @@ -155,6 +155,11 @@ class DecoderBase : public Decoder { // Throw away all buffers in all queues. result_queue_.clear(); state_ = kStopped; + + if (callback) { + callback->Run(); + delete callback; + } } void SeekTask(base::TimeDelta time, FilterCallback* callback) { diff --git a/media/filters/decoder_base_unittest.cc b/media/filters/decoder_base_unittest.cc index e771989..4bc0531 100644 --- a/media/filters/decoder_base_unittest.cc +++ b/media/filters/decoder_base_unittest.cc @@ -153,7 +153,9 @@ TEST(DecoderBaseTest, FlowControl) { // Stop. EXPECT_CALL(*decoder, DoStop()); - decoder->Stop(); + EXPECT_CALL(callback, OnFilterCallback()); + EXPECT_CALL(callback, OnCallbackDestroyed()); + decoder->Stop(callback.NewCallback()); message_loop.RunAllPending(); } diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc index c472504..27d8878 100644 --- a/media/filters/ffmpeg_demuxer.cc +++ b/media/filters/ffmpeg_demuxer.cc @@ -265,10 +265,10 @@ void FFmpegDemuxer::PostDemuxTask() { NewRunnableMethod(this, &FFmpegDemuxer::DemuxTask)); } -void FFmpegDemuxer::Stop() { +void FFmpegDemuxer::Stop(FilterCallback* callback) { // Post a task to notify the streams to stop as well. message_loop()->PostTask(FROM_HERE, - NewRunnableMethod(this, &FFmpegDemuxer::StopTask)); + NewRunnableMethod(this, &FFmpegDemuxer::StopTask, callback)); // Then wakes up the thread from reading. SignalReadCompleted(DataSource::kReadError); @@ -561,12 +561,16 @@ void FFmpegDemuxer::DemuxTask() { } } -void FFmpegDemuxer::StopTask() { +void FFmpegDemuxer::StopTask(FilterCallback* callback) { DCHECK_EQ(MessageLoop::current(), message_loop()); StreamVector::iterator iter; for (iter = streams_.begin(); iter != streams_.end(); ++iter) { (*iter)->Stop(); } + if (callback) { + callback->Run(); + delete callback; + } } void FFmpegDemuxer::DisableAudioStreamTask() { diff --git a/media/filters/ffmpeg_demuxer.h b/media/filters/ffmpeg_demuxer.h index bc5cc62..c43bd20 100644 --- a/media/filters/ffmpeg_demuxer.h +++ b/media/filters/ffmpeg_demuxer.h @@ -131,7 +131,7 @@ class FFmpegDemuxer : public Demuxer, virtual void PostDemuxTask(); // MediaFilter implementation. - virtual void Stop(); + virtual void Stop(FilterCallback* callback); virtual void Seek(base::TimeDelta time, FilterCallback* callback); virtual void OnAudioRendererDisabled(); @@ -166,7 +166,7 @@ class FFmpegDemuxer : public Demuxer, void DemuxTask(); // Carries out stopping the demuxer streams on the demuxer thread. - void StopTask(); + void StopTask(FilterCallback* callback); // Carries out disabling the audio stream on the demuxer thread. void DisableAudioStreamTask(); diff --git a/media/filters/ffmpeg_demuxer_unittest.cc b/media/filters/ffmpeg_demuxer_unittest.cc index ec263a6..c6c8e74 100644 --- a/media/filters/ffmpeg_demuxer_unittest.cc +++ b/media/filters/ffmpeg_demuxer_unittest.cc @@ -117,7 +117,9 @@ class FFmpegDemuxerTest : public testing::Test { virtual ~FFmpegDemuxerTest() { // Call Stop() to shut down internal threads. - demuxer_->Stop(); + EXPECT_CALL(callback_, OnFilterCallback()); + EXPECT_CALL(callback_, OnCallbackDestroyed()); + demuxer_->Stop(callback_.NewCallback()); // Finish up any remaining tasks. message_loop_.RunAllPending(); @@ -623,7 +625,9 @@ TEST_F(FFmpegDemuxerTest, Stop) { ASSERT_TRUE(audio); // Stop the demuxer. - demuxer_->Stop(); + EXPECT_CALL(callback_, OnFilterCallback()); + EXPECT_CALL(callback_, OnCallbackDestroyed()); + demuxer_->Stop(callback_.NewCallback()); // Expect all calls in sequence. InSequence s; @@ -732,9 +736,6 @@ TEST_F(FFmpegDemuxerTest, ProtocolRead) { EXPECT_CALL(*data_source_, GetSize(_)) .WillOnce(DoAll(SetArgumentPointee<0>(1024), Return(true))); - // This read complete signal is generated when demuxer is stopped. - EXPECT_CALL(*demuxer, SignalReadCompleted(DataSource::kReadError)); - // First read. EXPECT_EQ(512, demuxer->Read(512, kBuffer)); int64 position; @@ -749,7 +750,12 @@ TEST_F(FFmpegDemuxerTest, ProtocolRead) { // Third read will get an end-of-file error. EXPECT_EQ(AVERROR_EOF, demuxer->Read(512, kBuffer)); - demuxer->Stop(); + // This read complete signal is generated when demuxer is stopped. + EXPECT_CALL(*demuxer, SignalReadCompleted(DataSource::kReadError)); + EXPECT_CALL(callback_, OnFilterCallback()); + EXPECT_CALL(callback_, OnCallbackDestroyed()); + demuxer->Stop(callback_.NewCallback()); + message_loop_.RunAllPending(); } TEST_F(FFmpegDemuxerTest, ProtocolGetSetPosition) { diff --git a/media/filters/file_data_source.cc b/media/filters/file_data_source.cc index 901e107..181149d 100644 --- a/media/filters/file_data_source.cc +++ b/media/filters/file_data_source.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// 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. @@ -19,7 +19,7 @@ FileDataSource::FileDataSource() } FileDataSource::~FileDataSource() { - Stop(); + DCHECK(!file_); } void FileDataSource::Initialize(const std::string& url, @@ -48,13 +48,17 @@ void FileDataSource::Initialize(const std::string& url, callback->Run(); } -void FileDataSource::Stop() { +void FileDataSource::Stop(FilterCallback* callback) { AutoLock l(lock_); if (file_) { file_util::CloseFile(file_); file_ = NULL; file_size_ = 0; } + if (callback) { + callback->Run(); + delete callback; + } } const MediaFormat& FileDataSource::media_format() { diff --git a/media/filters/file_data_source.h b/media/filters/file_data_source.h index 164e74c..182d5be 100644 --- a/media/filters/file_data_source.h +++ b/media/filters/file_data_source.h @@ -1,4 +1,4 @@ -// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// 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. @@ -23,7 +23,7 @@ class FileDataSource : public DataSource { } // Implementation of MediaFilter. - virtual void Stop(); + virtual void Stop(FilterCallback* callback); // Implementation of DataSource. virtual void Initialize(const std::string& url, FilterCallback* callback); diff --git a/media/filters/file_data_source_unittest.cc b/media/filters/file_data_source_unittest.cc index 80df9b7..48d999e 100644 --- a/media/filters/file_data_source_unittest.cc +++ b/media/filters/file_data_source_unittest.cc @@ -65,6 +65,10 @@ TEST(FileDataSourceTest, OpenFile) { scoped_refptr filter = new FileDataSource(); filter->set_host(&host); filter->Initialize(TestFileURL(), callback.NewCallback()); + + EXPECT_CALL(callback, OnFilterCallback()); + EXPECT_CALL(callback, OnCallbackDestroyed()); + filter->Stop(callback.NewCallback()); } // Use the mock filter host to directly call the Read and GetPosition methods. @@ -99,6 +103,10 @@ TEST(FileDataSourceTest, ReadData) { filter->Read(5, 10, ten_bytes, NewCallback(&handler, &ReadCallbackHandler::ReadCallback)); EXPECT_EQ('5', ten_bytes[0]); + + EXPECT_CALL(callback, OnFilterCallback()); + EXPECT_CALL(callback, OnCallbackDestroyed()); + filter->Stop(callback.NewCallback()); } // Test that FileDataSource does nothing on Seek(). @@ -110,6 +118,10 @@ TEST(FileDataSourceTest, Seek) { scoped_refptr filter = new FileDataSource(); filter->Seek(kZero, callback.NewCallback()); + + EXPECT_CALL(callback, OnFilterCallback()); + EXPECT_CALL(callback, OnCallbackDestroyed()); + filter->Stop(callback.NewCallback()); } } // namespace media diff --git a/media/filters/null_audio_renderer.cc b/media/filters/null_audio_renderer.cc index 544b588..62cbeee 100644 --- a/media/filters/null_audio_renderer.cc +++ b/media/filters/null_audio_renderer.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// 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. @@ -23,7 +23,7 @@ NullAudioRenderer::NullAudioRenderer() } NullAudioRenderer::~NullAudioRenderer() { - Stop(); + DCHECK_EQ(kNullThreadHandle, thread_); } // static diff --git a/media/filters/omx_video_decode_engine.h b/media/filters/omx_video_decode_engine.h index 6cdfe73..d1c5619 100644 --- a/media/filters/omx_video_decode_engine.h +++ b/media/filters/omx_video_decode_engine.h @@ -53,6 +53,7 @@ class OmxVideoDecodeEngine : // // TODO(ajwong): Normalize this interface with Task like the others, and // promote to the abstract interface. + // TODO(boliu): Should the callback type be FilterCallback*? virtual void Stop(Callback0::Type* done_cb); // Subclass can provide a different value. @@ -258,4 +259,3 @@ class OmxVideoDecodeEngine : } // namespace media #endif // MEDIA_FILTERS_OMX_VIDEO_DECODE_ENGINE_H_ - diff --git a/media/filters/omx_video_decoder.cc b/media/filters/omx_video_decoder.cc index d969cb6..48d2cb6 100644 --- a/media/filters/omx_video_decoder.cc +++ b/media/filters/omx_video_decoder.cc @@ -72,11 +72,8 @@ void OmxVideoDecoder::FillThisBuffer(scoped_refptr frame) { &OmxVideoDecodeEngine::FillThisBuffer, frame)); } -void OmxVideoDecoder::Stop() { - // TODO(ajwong): This is a total hack. Make async. - base::WaitableEvent event(false, false); - omx_engine_->Stop(NewCallback(&event, &base::WaitableEvent::Signal)); - event.Wait(); +void OmxVideoDecoder::Stop(FilterCallback* callback) { + omx_engine_->Stop(callback); } void OmxVideoDecoder::DoInitialize(DemuxerStream* demuxer_stream, diff --git a/media/filters/omx_video_decoder.h b/media/filters/omx_video_decoder.h index 18c8b6b..236130b 100644 --- a/media/filters/omx_video_decoder.h +++ b/media/filters/omx_video_decoder.h @@ -28,7 +28,7 @@ class OmxVideoDecoder : public VideoDecoder { virtual ~OmxVideoDecoder(); virtual void Initialize(DemuxerStream* stream, FilterCallback* callback); - virtual void Stop(); + virtual void Stop(FilterCallback* callback); virtual void FillThisBuffer(scoped_refptr frame); virtual const MediaFormat& media_format() { return media_format_; } diff --git a/media/filters/video_decoder_impl_unittest.cc b/media/filters/video_decoder_impl_unittest.cc index f2161ed..d84067c 100644 --- a/media/filters/video_decoder_impl_unittest.cc +++ b/media/filters/video_decoder_impl_unittest.cc @@ -139,7 +139,9 @@ class VideoDecoderImplTest : public testing::Test { virtual ~VideoDecoderImplTest() { // Call Stop() to shut down internal threads. - decoder_->Stop(); + EXPECT_CALL(callback_, OnFilterCallback()); + EXPECT_CALL(callback_, OnCallbackDestroyed()); + decoder_->Stop(callback_.NewCallback()); // Finish up any remaining tasks. message_loop_.RunAllPending(); diff --git a/media/filters/video_renderer_base.cc b/media/filters/video_renderer_base.cc index d3c1daa..13b6dee 100644 --- a/media/filters/video_renderer_base.cc +++ b/media/filters/video_renderer_base.cc @@ -99,7 +99,7 @@ void VideoRendererBase::Pause(FilterCallback* callback) { } } -void VideoRendererBase::Stop() { +void VideoRendererBase::Stop(FilterCallback* callback) { AutoLock auto_lock(lock_); state_ = kStopped; @@ -119,6 +119,11 @@ void VideoRendererBase::Stop() { } thread_ = kNullThreadHandle; } + + if (callback) { + callback->Run(); + delete callback; + } } void VideoRendererBase::SetPlaybackRate(float playback_rate) { diff --git a/media/filters/video_renderer_base.h b/media/filters/video_renderer_base.h index 551d234..ba30f52 100644 --- a/media/filters/video_renderer_base.h +++ b/media/filters/video_renderer_base.h @@ -41,7 +41,7 @@ class VideoRendererBase : public VideoRenderer, // MediaFilter implementation. virtual void Play(FilterCallback* callback); virtual void Pause(FilterCallback* callback); - virtual void Stop(); + virtual void Stop(FilterCallback* callback); virtual void SetPlaybackRate(float playback_rate); virtual void Seek(base::TimeDelta time, FilterCallback* callback); diff --git a/media/filters/video_renderer_base_unittest.cc b/media/filters/video_renderer_base_unittest.cc index 111df56..81af7d7 100644 --- a/media/filters/video_renderer_base_unittest.cc +++ b/media/filters/video_renderer_base_unittest.cc @@ -65,7 +65,9 @@ class VideoRendererBaseTest : public ::testing::Test { // Expect a call into the subclass. EXPECT_CALL(*renderer_, OnStop()); - renderer_->Stop(); + EXPECT_CALL(callback_, OnFilterCallback()); + EXPECT_CALL(callback_, OnCallbackDestroyed()); + renderer_->Stop(callback_.NewCallback()); } protected: -- cgit v1.1