From baf56e2f0bef2b7373117aee8388325cf2ab4852 Mon Sep 17 00:00:00 2001 From: "xhwang@chromium.org" Date: Wed, 26 Sep 2012 23:42:00 +0000 Subject: Enforce Stop() is always called before dtor in FFmepgVideoDecoder. Enforce this because Stop() needs to do some shutdown work asynchronously. - Updated comments in VideoDecoder interface. - Added DCHECK in dtor. - Updated FFmpegVideoDecoderTest to call Stop() in dtor. BUG=93932,152262 TEST=Updated media_unittest Review URL: https://chromiumcodereview.appspot.com/10990039 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@158931 0039d316-1c4b-4281-b951-d872f2087c98 --- media/base/video_decoder.h | 15 ++++--- media/filters/ffmpeg_video_decoder.cc | 16 +++++--- media/filters/ffmpeg_video_decoder.h | 2 +- media/filters/ffmpeg_video_decoder_unittest.cc | 55 +++++++++++++++----------- 4 files changed, 53 insertions(+), 35 deletions(-) (limited to 'media') diff --git a/media/base/video_decoder.h b/media/base/video_decoder.h index b22b144..4c86217 100644 --- a/media/base/video_decoder.h +++ b/media/base/video_decoder.h @@ -26,9 +26,9 @@ class MEDIA_EXPORT VideoDecoder kDecryptError // Decrypting error happened. }; - // Initialize a VideoDecoder with the given DemuxerStream, executing the - // callback upon completion. - // statistics_cb is used to update global pipeline statistics. + // Initializes a VideoDecoder with the given DemuxerStream, executing the + // |status_cb| upon completion. + // |statistics_cb| is used to update global pipeline statistics. virtual void Initialize(const scoped_refptr& stream, const PipelineStatusCB& status_cb, const StatisticsCB& statistics_cb) = 0; @@ -50,13 +50,16 @@ class MEDIA_EXPORT VideoDecoder typedef base::Callback&)> ReadCB; virtual void Read(const ReadCB& read_cb) = 0; - // Reset decoder state, fulfilling all pending ReadCB and dropping extra + // Resets decoder state, fulfilling all pending ReadCB and dropping extra // queued decoded data. After this call, the decoder is back to an initialized // clean state. virtual void Reset(const base::Closure& closure) = 0; - // Stop decoder and set it to an uninitialized state. Note that a VideoDecoder - // should/could not be re-initialized after it has been stopped. + // Stops decoder, fires any pending callbacks and sets the decoder to an + // uninitialized state. A VideoDecoder cannot be re-initialized after it has + // been stopped. + // Note that if Initialize() has been called, Stop() must be called and + // complete before deleting the decoder. virtual void Stop(const base::Closure& closure) = 0; // Returns true if the output format has an alpha channel. Most formats do not diff --git a/media/filters/ffmpeg_video_decoder.cc b/media/filters/ffmpeg_video_decoder.cc index f05fa3f..ff44bf8 100644 --- a/media/filters/ffmpeg_video_decoder.cc +++ b/media/filters/ffmpeg_video_decoder.cc @@ -207,6 +207,11 @@ void FFmpegVideoDecoder::Stop(const base::Closure& closure) { return; } + if (state_ == kUninitialized) { + closure.Run(); + return; + } + if (decryptor_) decryptor_->CancelDecrypt(); @@ -226,7 +231,9 @@ void FFmpegVideoDecoder::DoStop() { } FFmpegVideoDecoder::~FFmpegVideoDecoder() { - ReleaseFFmpegResources(); + DCHECK_EQ(kUninitialized, state_); + DCHECK(!codec_context_); + DCHECK(!av_frame_); } void FFmpegVideoDecoder::DoRead(const ReadCB& read_cb) { @@ -528,11 +535,10 @@ bool FFmpegVideoDecoder::ConfigureDecoder() { codec_context_->release_buffer = ReleaseVideoBufferImpl; AVCodec* codec = avcodec_find_decoder(codec_context_->codec_id); - if (!codec) - return false; - - if (avcodec_open2(codec_context_, codec, NULL) < 0) + if (!codec || avcodec_open2(codec_context_, codec, NULL) < 0) { + ReleaseFFmpegResources(); return false; + } av_frame_ = avcodec_alloc_frame(); return true; diff --git a/media/filters/ffmpeg_video_decoder.h b/media/filters/ffmpeg_video_decoder.h index 340864d..292adf1 100644 --- a/media/filters/ffmpeg_video_decoder.h +++ b/media/filters/ffmpeg_video_decoder.h @@ -50,7 +50,7 @@ class MEDIA_EXPORT FFmpegVideoDecoder : public VideoDecoder { kUninitialized, kNormal, kFlushCodec, - kDecodeFinished, + kDecodeFinished }; // Carries out the reading operation scheduled by Read(). diff --git a/media/filters/ffmpeg_video_decoder_unittest.cc b/media/filters/ffmpeg_video_decoder_unittest.cc index d97d7a0..00be225 100644 --- a/media/filters/ffmpeg_video_decoder_unittest.cc +++ b/media/filters/ffmpeg_video_decoder_unittest.cc @@ -6,6 +6,7 @@ #include #include "base/bind.h" +#include "base/callback_helpers.h" #include "base/message_loop.h" #include "base/memory/singleton.h" #include "base/string_util.h" @@ -25,6 +26,8 @@ #include "testing/gmock/include/gmock/gmock.h" using ::testing::_; +using ::testing::AtMost; +using ::testing::Invoke; using ::testing::IsNull; using ::testing::Return; using ::testing::ReturnRef; @@ -63,10 +66,6 @@ ACTION_P2(RunDecryptCB, status, buffer) { arg1.Run(status, buffer); } -ACTION_P3(RunDecryptCB3, decrypt_cb, status, buffer) { - decrypt_cb.Run(status, buffer); -} - class FFmpegVideoDecoderTest : public testing::Test { public: FFmpegVideoDecoderTest() @@ -90,7 +89,9 @@ class FFmpegVideoDecoderTest : public testing::Test { encrypted_i_frame_buffer_ = CreateFakeEncryptedBuffer(); } - virtual ~FFmpegVideoDecoderTest() {} + virtual ~FFmpegVideoDecoderTest() { + Stop(); + } void Initialize() { config_.Initialize(kCodecVP8, VIDEO_CODEC_PROFILE_UNKNOWN, kVideoFormat, @@ -122,12 +123,27 @@ class FFmpegVideoDecoderTest : public testing::Test { InitializeWithConfigAndStatus(config, PIPELINE_OK); } + void CancelDecrypt() { + if (!decrypt_cb_.is_null()) { + base::ResetAndReturn(&decrypt_cb_).Run( + Decryptor::kError, scoped_refptr(NULL)); + } + } + void Reset() { + EXPECT_CALL(*decryptor_, CancelDecrypt()) + .WillOnce(Invoke(this, &FFmpegVideoDecoderTest::CancelDecrypt)); decoder_->Reset(NewExpectedClosure()); message_loop_.RunAllPending(); } void Stop() { + // Use AtMost(1) here because CancelDecrypt() will be called once if the + // decoder was initialized and has not been stopped, and will not be + // called otherwise. + EXPECT_CALL(*decryptor_, CancelDecrypt()) + .Times(AtMost(1)) + .WillRepeatedly(Invoke(this, &FFmpegVideoDecoderTest::CancelDecrypt)); decoder_->Stop(NewExpectedClosure()); message_loop_.RunAllPending(); } @@ -229,6 +245,7 @@ class FFmpegVideoDecoderTest : public testing::Test { VideoDecoderConfig config_; VideoDecoder::ReadCB read_cb_; + Decryptor::DecryptCB decrypt_cb_; // Various buffers for testing. scoped_array frame_buffer_; @@ -517,6 +534,9 @@ TEST_F(FFmpegVideoDecoderTest, DecodeEncryptedFrame_CorruptedBufferReturned) { EXPECT_CALL(*decryptor_, Decrypt(encrypted_i_frame_buffer_, _)) .WillRepeatedly(RunDecryptCB(Decryptor::kSuccess, corrupt_i_frame_buffer_)); + // The decoder only detects the error at the second decoding call. So + // |statistics_cb_| still gets called once. + EXPECT_CALL(statistics_cb_, OnStatistics(_)); // Our read should still get satisfied with end of stream frame during an // error. @@ -580,19 +600,14 @@ TEST_F(FFmpegVideoDecoderTest, Reset_DuringPendingDecrypt) { EXPECT_CALL(*demuxer_, Read(_)) .WillRepeatedly(ReturnBuffer(encrypted_i_frame_buffer_)); - - Decryptor::DecryptCB decrypt_cb; EXPECT_CALL(*decryptor_, Decrypt(encrypted_i_frame_buffer_, _)) - .WillOnce(SaveArg<1>(&decrypt_cb)); + .WillOnce(SaveArg<1>(&decrypt_cb_)); decoder_->Read(read_cb_); message_loop_.RunAllPending(); // Make sure the Read() on the decoder triggers a Decrypt() on the decryptor. - EXPECT_FALSE(decrypt_cb.is_null()); + EXPECT_FALSE(decrypt_cb_.is_null()); - EXPECT_CALL(*decryptor_, CancelDecrypt()) - .WillOnce(RunDecryptCB3(decrypt_cb, Decryptor::kError, - scoped_refptr(NULL))); EXPECT_CALL(*this, FrameReady(VideoDecoder::kOk, IsNull())); Reset(); message_loop_.RunAllPending(); @@ -647,19 +662,14 @@ TEST_F(FFmpegVideoDecoderTest, Stop_DuringPendingDecrypt) { EXPECT_CALL(*demuxer_, Read(_)) .WillRepeatedly(ReturnBuffer(encrypted_i_frame_buffer_)); - - Decryptor::DecryptCB decrypt_cb; EXPECT_CALL(*decryptor_, Decrypt(encrypted_i_frame_buffer_, _)) - .WillOnce(SaveArg<1>(&decrypt_cb)); + .WillOnce(SaveArg<1>(&decrypt_cb_)); decoder_->Read(read_cb_); message_loop_.RunAllPending(); // Make sure the Read() on the decoder triggers a Decrypt() on the decryptor. - EXPECT_FALSE(decrypt_cb.is_null()); + EXPECT_FALSE(decrypt_cb_.is_null()); - EXPECT_CALL(*decryptor_, CancelDecrypt()) - .WillOnce(RunDecryptCB3(decrypt_cb, Decryptor::kError, - scoped_refptr(NULL))); EXPECT_CALL(*this, FrameReady(VideoDecoder::kOk, IsNull())); Stop(); message_loop_.RunAllPending(); @@ -682,7 +692,7 @@ TEST_F(FFmpegVideoDecoderTest, AbortPendingRead) { } // Test aborted read on the demuxer stream. -TEST_F(FFmpegVideoDecoderTest, AbortPendingReadDuringFlush) { +TEST_F(FFmpegVideoDecoderTest, AbortPendingReadDuringReset) { Initialize(); DemuxerStream::ReadCB read_cb; @@ -695,9 +705,8 @@ TEST_F(FFmpegVideoDecoderTest, AbortPendingReadDuringFlush) { message_loop_.RunAllPending(); ASSERT_FALSE(read_cb.is_null()); - // Flush while there is still an outstanding read on the demuxer. - decoder_->Reset(NewExpectedClosure()); - message_loop_.RunAllPending(); + // Reset while there is still an outstanding read on the demuxer. + Reset(); // Signal an aborted demuxer read. read_cb.Run(DemuxerStream::kAborted, NULL); -- cgit v1.1