diff options
author | scherkus@chromium.org <scherkus@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-04-29 20:10:15 +0000 |
---|---|---|
committer | scherkus@chromium.org <scherkus@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-04-29 20:10:15 +0000 |
commit | 7b025aff7f4a5f513cc93e801a14028abb8582cd (patch) | |
tree | 7eaebd0ad40b944ca00aafb8c4b997c88c5d9023 | |
parent | 7ea8eb0b4356d7fd248adf37acff9c77e0bc6cc4 (diff) | |
download | chromium_src-7b025aff7f4a5f513cc93e801a14028abb8582cd.zip chromium_src-7b025aff7f4a5f513cc93e801a14028abb8582cd.tar.gz chromium_src-7b025aff7f4a5f513cc93e801a14028abb8582cd.tar.bz2 |
Forward decoder errors from FFmpegVideoDecodeEngine to FFmpegVideoDecoder.
Previously they were getting dropped on the floor, which could leave the media pipeline in lame duck mode.
BUG=80187
TEST=media_unittests
Review URL: http://codereview.chromium.org/6899058
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@83577 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | media/base/filters.h | 2 | ||||
-rw-r--r-- | media/filters/ffmpeg_video_decoder.cc | 2 | ||||
-rw-r--r-- | media/filters/ffmpeg_video_decoder_unittest.cc | 18 | ||||
-rw-r--r-- | media/filters/video_renderer_base.cc | 59 | ||||
-rw-r--r-- | media/filters/video_renderer_base.h | 4 | ||||
-rw-r--r-- | media/filters/video_renderer_base_unittest.cc | 40 | ||||
-rw-r--r-- | media/video/ffmpeg_video_decode_engine.cc | 14 | ||||
-rw-r--r-- | media/video/ffmpeg_video_decode_engine_unittest.cc | 6 |
8 files changed, 113 insertions, 32 deletions
diff --git a/media/base/filters.h b/media/base/filters.h index a3e5afb..7c13bea 100644 --- a/media/base/filters.h +++ b/media/base/filters.h @@ -204,6 +204,8 @@ class VideoDecoder : public Filter { virtual void ProduceVideoFrame(scoped_refptr<VideoFrame> frame) = 0; // Installs a permanent callback for passing decoded video output. + // + // A NULL frame represents a decoding error. typedef base::Callback<void(scoped_refptr<VideoFrame>)> ConsumeVideoFrameCB; void set_consume_video_frame_callback(const ConsumeVideoFrameCB& callback) { consume_video_frame_callback_ = callback; diff --git a/media/filters/ffmpeg_video_decoder.cc b/media/filters/ffmpeg_video_decoder.cc index 81af909..2120022 100644 --- a/media/filters/ffmpeg_video_decoder.cc +++ b/media/filters/ffmpeg_video_decoder.cc @@ -209,7 +209,7 @@ void FFmpegVideoDecoder::OnSeekComplete() { } void FFmpegVideoDecoder::OnError() { - NOTIMPLEMENTED(); + VideoFrameReady(NULL); } void FFmpegVideoDecoder::OnFormatChange(VideoStreamInfo stream_info) { diff --git a/media/filters/ffmpeg_video_decoder_unittest.cc b/media/filters/ffmpeg_video_decoder_unittest.cc index 75c1c2d..54fb1be 100644 --- a/media/filters/ffmpeg_video_decoder_unittest.cc +++ b/media/filters/ffmpeg_video_decoder_unittest.cc @@ -145,10 +145,11 @@ class FFmpegVideoDecoderTest : public testing::Test { renderer_ = new MockVideoRenderer(); engine_ = new StrictMock<MockVideoDecodeEngine>(); - DCHECK(decoder_); - // Inject mocks and prepare a demuxer stream. decoder_->set_host(&host_); + decoder_->set_consume_video_frame_callback( + base::Bind(&MockVideoRenderer::ConsumeVideoFrame, + base::Unretained(renderer_.get()))); decoder_->SetVideoDecodeEngineForTest(engine_); demuxer_ = new StrictMock<MockFFmpegDemuxerStream>(); @@ -269,6 +270,15 @@ TEST_F(FFmpegVideoDecoderTest, Initialize_Successful) { EXPECT_EQ(kHeight, height); } +TEST_F(FFmpegVideoDecoderTest, OnError) { + InitializeDecoderSuccessfully(); + + scoped_refptr<VideoFrame> null_frame; + EXPECT_CALL(*renderer_, ConsumeVideoFrame(null_frame)); + engine_->event_handler_->OnError(); +} + + ACTION_P2(ReadFromDemux, decoder, buffer) { decoder->ProduceVideoSample(buffer); } @@ -311,10 +321,6 @@ TEST_F(FFmpegVideoDecoderTest, DoDecode_TestStateTransition) { // 5) All state transitions happen as expected. InitializeDecoderSuccessfully(); - decoder_->set_consume_video_frame_callback( - base::Bind(&MockVideoRenderer::ConsumeVideoFrame, - base::Unretained(renderer_.get()))); - // Setup initial state and check that it is sane. ASSERT_EQ(FFmpegVideoDecoder::kNormal, decoder_->state_); ASSERT_TRUE(base::TimeDelta() == decoder_->pts_stream_.current_pts()); diff --git a/media/filters/video_renderer_base.cc b/media/filters/video_renderer_base.cc index 78fa6f4..92bedba 100644 --- a/media/filters/video_renderer_base.cc +++ b/media/filters/video_renderer_base.cc @@ -175,8 +175,7 @@ void VideoRendererBase::Initialize(VideoDecoder* decoder, &surface_type_, &surface_format_, &width_, &height_)) { - host()->SetError(PIPELINE_ERROR_INITIALIZATION_FAILED); - state_ = kError; + EnterErrorState_Locked(PIPELINE_ERROR_INITIALIZATION_FAILED); return; } host()->SetVideoSize(width_, height_); @@ -185,8 +184,7 @@ void VideoRendererBase::Initialize(VideoDecoder* decoder, // TODO(scherkus): do we trust subclasses not to do something silly while // we're holding the lock? if (!OnInitialize(decoder)) { - host()->SetError(PIPELINE_ERROR_INITIALIZATION_FAILED); - state_ = kError; + EnterErrorState_Locked(PIPELINE_ERROR_INITIALIZATION_FAILED); return; } @@ -199,8 +197,7 @@ void VideoRendererBase::Initialize(VideoDecoder* decoder, // Create our video thread. if (!base::PlatformThread::Create(0, this, &thread_)) { NOTREACHED() << "Video thread creation failed"; - host()->SetError(PIPELINE_ERROR_INITIALIZATION_FAILED); - state_ = kError; + EnterErrorState_Locked(PIPELINE_ERROR_INITIALIZATION_FAILED); return; } @@ -389,12 +386,19 @@ void VideoRendererBase::PutCurrentFrame(scoped_refptr<VideoFrame> frame) { } void VideoRendererBase::ConsumeVideoFrame(scoped_refptr<VideoFrame> frame) { - PipelineStatistics statistics; - statistics.video_frames_decoded = 1; - statistics_callback_->Run(statistics); + if (frame) { + PipelineStatistics statistics; + statistics.video_frames_decoded = 1; + statistics_callback_->Run(statistics); + } base::AutoLock auto_lock(lock_); + if (!frame) { + EnterErrorState_Locked(PIPELINE_ERROR_DECODE); + return; + } + // Decoder could reach seek state before our Seek() get called. // We will enter kSeeking if (kFlushed == state_) @@ -570,4 +574,41 @@ base::TimeDelta VideoRendererBase::CalculateSleepDuration( static_cast<int64>(sleep.InMicroseconds() / playback_rate)); } +void VideoRendererBase::EnterErrorState_Locked(PipelineStatus status) { + lock_.AssertAcquired(); + + scoped_ptr<FilterCallback> callback; + switch (state_) { + case kUninitialized: + case kPrerolled: + case kPaused: + case kFlushed: + case kEnded: + case kPlaying: + break; + + case kFlushing: + CHECK(flush_callback_.get()); + callback.swap(flush_callback_); + break; + + case kSeeking: + CHECK(seek_callback_.get()); + callback.swap(seek_callback_); + break; + + case kStopped: + NOTREACHED() << "Should not error if stopped."; + return; + + case kError: + return; + } + state_ = kError; + host()->SetError(status); + + if (callback.get()) + callback->Run(); +} + } // namespace media diff --git a/media/filters/video_renderer_base.h b/media/filters/video_renderer_base.h index 5047191..66eba6e 100644 --- a/media/filters/video_renderer_base.h +++ b/media/filters/video_renderer_base.h @@ -22,6 +22,7 @@ #include "base/synchronization/lock.h" #include "base/threading/platform_thread.h" #include "media/base/filters.h" +#include "media/base/pipeline_status.h" #include "media/base/video_frame.h" namespace media { @@ -130,6 +131,9 @@ class VideoRendererBase : public VideoRenderer, base::TimeDelta CalculateSleepDuration(VideoFrame* next_frame, float playback_rate); + // Safely handles entering to an error state. + void EnterErrorState_Locked(PipelineStatus status); + // Used for accessing data members. base::Lock lock_; diff --git a/media/filters/video_renderer_base_unittest.cc b/media/filters/video_renderer_base_unittest.cc index 3e0578c..c21d6c7 100644 --- a/media/filters/video_renderer_base_unittest.cc +++ b/media/filters/video_renderer_base_unittest.cc @@ -110,16 +110,20 @@ class VideoRendererBaseTest : public ::testing::Test { Seek(0); } - void Seek(int64 timestamp) { - EXPECT_CALL(*renderer_, OnFrameAvailable()); + void StartSeeking(int64 timestamp) { EXPECT_FALSE(seeking_); - // Now seek to trigger prerolling. + // Seek to trigger prerolling. seeking_ = true; renderer_->Seek(base::TimeDelta::FromMicroseconds(timestamp), NewCallback(this, &VideoRendererBaseTest::OnSeekComplete)); + } + + void FinishSeeking() { + EXPECT_CALL(*renderer_, OnFrameAvailable()); + EXPECT_TRUE(seeking_); - // Now satisfy the read requests. The callback must be executed in order + // Satisfy the read requests. The callback must be executed in order // to exit the loop since VideoRendererBase can read a few extra frames // after |timestamp| in order to preroll. for (int64 i = 0; seeking_; ++i) { @@ -127,6 +131,11 @@ class VideoRendererBaseTest : public ::testing::Test { } } + void Seek(int64 timestamp) { + StartSeeking(timestamp); + FinishSeeking(); + } + void Flush() { renderer_->Pause(NewExpectedCallback()); @@ -136,6 +145,10 @@ class VideoRendererBaseTest : public ::testing::Test { renderer_->Flush(NewExpectedCallback()); } + void CreateError() { + decoder_->VideoFrameReadyForTest(NULL); + } + void CreateFrame(int64 timestamp, int64 duration) { const base::TimeDelta kZero; scoped_refptr<VideoFrame> frame; @@ -244,6 +257,25 @@ TEST_F(VideoRendererBaseTest, Play) { Flush(); } +TEST_F(VideoRendererBaseTest, Error_Playing) { + Initialize(); + renderer_->Play(NewExpectedCallback()); + + EXPECT_CALL(host_, SetError(PIPELINE_ERROR_DECODE)); + CreateError(); + Flush(); +} + +TEST_F(VideoRendererBaseTest, Error_Seeking) { + Initialize(); + Flush(); + StartSeeking(0); + + EXPECT_CALL(host_, SetError(PIPELINE_ERROR_DECODE)); + CreateError(); + Flush(); +} + TEST_F(VideoRendererBaseTest, Seek_Exact) { Initialize(); Flush(); diff --git a/media/video/ffmpeg_video_decode_engine.cc b/media/video/ffmpeg_video_decode_engine.cc index 4ebfb30..0413092 100644 --- a/media/video/ffmpeg_video_decode_engine.cc +++ b/media/video/ffmpeg_video_decode_engine.cc @@ -244,12 +244,11 @@ void FFmpegVideoDecodeEngine::DecodeFrame(scoped_refptr<Buffer> buffer) { // Log the problem if we can't decode a video frame and exit early. if (result < 0) { - VLOG(1) << "Error decoding a video frame with timestamp: " - << buffer->GetTimestamp().InMicroseconds() << " us, duration: " - << buffer->GetDuration().InMicroseconds() << " us, packet size: " - << buffer->GetDataSize() << " bytes"; - // TODO(jiesun): call event_handler_->OnError() instead. - event_handler_->ConsumeVideoFrame(video_frame, statistics); + LOG(ERROR) << "Error decoding a video frame with timestamp: " + << buffer->GetTimestamp().InMicroseconds() << " us, duration: " + << buffer->GetDuration().InMicroseconds() << " us, packet size: " + << buffer->GetDataSize() << " bytes"; + event_handler_->OnError(); return; } @@ -274,8 +273,7 @@ void FFmpegVideoDecodeEngine::DecodeFrame(scoped_refptr<Buffer> buffer) { if (!av_frame_->data[VideoFrame::kYPlane] || !av_frame_->data[VideoFrame::kUPlane] || !av_frame_->data[VideoFrame::kVPlane]) { - // TODO(jiesun): call event_handler_->OnError() instead. - event_handler_->ConsumeVideoFrame(video_frame, statistics); + event_handler_->OnError(); return; } diff --git a/media/video/ffmpeg_video_decode_engine_unittest.cc b/media/video/ffmpeg_video_decode_engine_unittest.cc index 994d2c5..8a53f6d 100644 --- a/media/video/ffmpeg_video_decode_engine_unittest.cc +++ b/media/video/ffmpeg_video_decode_engine_unittest.cc @@ -290,11 +290,9 @@ TEST_F(FFmpegVideoDecodeEngineTest, DecodeFrame_DecodeError) { EXPECT_CALL(*this, ProduceVideoSample(_)) .WillOnce(DemuxComplete(test_engine_.get(), buffer_)); - EXPECT_CALL(*this, ConsumeVideoFrame(_, _)) - .WillOnce(DecodeComplete(this)); - test_engine_->ProduceVideoFrame(video_frame_); + EXPECT_CALL(*this, OnError()); - EXPECT_FALSE(video_frame_.get()); + test_engine_->ProduceVideoFrame(video_frame_); } TEST_F(FFmpegVideoDecodeEngineTest, DecodeFrame_LargerWidth) { |