Handle config change during pending reset correctly in FFmpegVideoDecoder.

media/filters/ffmpeg_video_decoder_unittest.cc

Index: media/filters/ffmpeg_video_decoder_unittest.cc
diff --git a/media/filters/ffmpeg_video_decoder_unittest.cc b/media/filters/ffmpeg_video_decoder_unittest.cc
index fa1cfade1a39795df06d68853293af6400bb7216..df2fd61951cff1b7fb7be069542c82460ef53018 100644
--- a/media/filters/ffmpeg_video_decoder_unittest.cc
+++ b/media/filters/ffmpeg_video_decoder_unittest.cc
@@ -495,7 +495,7 @@ TEST_F(FFmpegVideoDecoderTest, Stop_DuringPendingRead) {
 }
 
 // Test aborted read on the demuxer stream.
-TEST_F(FFmpegVideoDecoderTest, AbortPendingRead) {
+TEST_F(FFmpegVideoDecoderTest, DemuxerRead_Aborted) {
   Initialize();
 
   EXPECT_CALL(*demuxer_, Read(_))
@@ -510,8 +510,8 @@ TEST_F(FFmpegVideoDecoderTest, AbortPendingRead) {
   EXPECT_FALSE(video_frame);
 }
 
-// Test aborted read on the demuxer stream.
-TEST_F(FFmpegVideoDecoderTest, AbortPendingReadDuringReset) {
+// Test aborted read on the demuxer stream during pending reset.
+TEST_F(FFmpegVideoDecoderTest, DemuxerRead_AbortedDuringReset) {
   Initialize();
 
   // Request a read on the decoder and ensure the demuxer has been called.
@@ -527,6 +527,78 @@ TEST_F(FFmpegVideoDecoderTest, AbortPendingReadDuringReset) {
   // Signal an aborted demuxer read: a NULL video frame should be returned.
   EXPECT_CALL(*this, FrameReady(VideoDecoder::kOk, IsNull()));
   read_cb.Run(DemuxerStream::kAborted, NULL);
+  message_loop_.RunUntilIdle();
+}
+
+// Test config change on the demuxer stream.
+TEST_F(FFmpegVideoDecoderTest, DemuxerRead_ConfigChanged) {
+  Initialize();
+
+  EXPECT_CALL(*demuxer_, Read(_))

[acolwell GONE FROM CHROMIUM, 2013/01/03 17:53:31]

It would be nice if this was slightly reworked so that it verifies that at least
one video_decoder_config() call happens between the kConfigChanged callback and
the next read.

[xhwang, 2013/01/04 01:28:05]

Hmm, this is harder than what I thought. Actually
FFmpegVideoDecoder::GetVideoBuffer() also calls video_decoder_config() (for the
natural size). So it's hard to insert a EXPECT_CALL() for video_decoder_config()
in a specific place.

But I do improved the test such that it's initialized with a random config
first. The config change has to pick up the new config to be able to decode the
frame.

+      .WillOnce(RunCallback<0>(DemuxerStream::kConfigChanged,
+                               scoped_refptr<DecoderBuffer>()))
+      .WillOnce(RunCallback<0>(DemuxerStream::kOk, i_frame_buffer_))
+      .WillRepeatedly(RunCallback<0>(DemuxerStream::kOk,
+                                     end_of_stream_buffer_));
+  EXPECT_CALL(statistics_cb_, OnStatistics(_));
+
+  VideoDecoder::Status status;
+  scoped_refptr<VideoFrame> video_frame;
+
+  Read(&status, &video_frame);
+
+  EXPECT_EQ(VideoDecoder::kOk, status);
+  ASSERT_TRUE(video_frame);
+  EXPECT_FALSE(video_frame->IsEndOfStream());
+}
+
+// Test config change on the demuxer stream during pending reset.
+TEST_F(FFmpegVideoDecoderTest, DemuxerRead_ConfigChangedDuringReset) {
+  static const gfx::Size kInitCodedSize(100, 100);
+  static const gfx::Rect kInitVisibleRect(100, 100);
+  static const gfx::Size kInitNaturalSize(100, 100);
+  VideoDecoderConfig init_config(
+      kCodecH264, VIDEO_CODEC_PROFILE_UNKNOWN, VideoFrame::RGB32,
+      kInitCodedSize, kInitVisibleRect, kInitNaturalSize, NULL, 0, false);
+  InitializeWithConfig(init_config);
+
+  // Request a read on the decoder and ensure the demuxer has been called.
+  DemuxerStream::ReadCB read_cb;
+  EXPECT_CALL(*demuxer_, Read(_))
+      .WillOnce(SaveArg<0>(&read_cb));
+  decoder_->Read(read_cb_);
+  ASSERT_FALSE(read_cb.is_null());
+
+  // Reset while there is still an outstanding read on the demuxer.
+  Reset();
+
+  VideoDecoderConfig new_config(
+      kCodecVP8, VIDEO_CODEC_PROFILE_UNKNOWN, kVideoFormat,
+      kCodedSize, kVisibleRect, kNaturalSize, NULL, 0, false);
+  EXPECT_CALL(*demuxer_, video_decoder_config())
+      .WillRepeatedly(ReturnRef(new_config));
+  EXPECT_CALL(*this, FrameReady(VideoDecoder::kOk, IsNull()));
+
+  // Signal a config change.
+  base::ResetAndReturn(&read_cb).Run(DemuxerStream::kConfigChanged, NULL);
+  message_loop_.RunUntilIdle();
+
+  // Now the decoder should be in a clean initialized state (initialized with
+  // |new_config|) and be ready to decode |i_frame_buffer_|.
+  EXPECT_CALL(*demuxer_, Read(_))
+      .WillOnce(RunCallback<0>(DemuxerStream::kOk, i_frame_buffer_))
+      .WillRepeatedly(RunCallback<0>(DemuxerStream::kOk,
+                                     end_of_stream_buffer_));
+  EXPECT_CALL(statistics_cb_, OnStatistics(_));
+
+  VideoDecoder::Status status;
+  scoped_refptr<VideoFrame> video_frame;
+
+  Read(&status, &video_frame);
+
+  EXPECT_EQ(VideoDecoder::kOk, status);
+  ASSERT_TRUE(video_frame);
+  EXPECT_FALSE(video_frame->IsEndOfStream());
 }

[acolwell GONE FROM CHROMIUM, 2013/01/03 17:53:31]

A test should also be added to test the config failure with pending reset path.

[xhwang, 2013/01/04 01:28:05]

Done.

 
 }  // namespace media