Allow multiple concurrent Decode() requests in VideoDecoder interface.

media/filters/video_frame_stream_unittest.cc

Index: media/filters/video_frame_stream_unittest.cc
diff --git a/media/filters/video_frame_stream_unittest.cc b/media/filters/video_frame_stream_unittest.cc
index 5225e98a543e9cefc301b42727ac3ea6167e2c13..6b0caa6ef246cb6cb30684b30902bc2e55f53579 100644
--- a/media/filters/video_frame_stream_unittest.cc
+++ b/media/filters/video_frame_stream_unittest.cc
@@ -26,17 +26,34 @@ static const int kDecodingDelay = 7;
 
 namespace media {
 
+struct VideoFrameStreamTestParams {
+  VideoFrameStreamTestParams(bool is_encrypted,
+                             bool enable_get_decode_output,
+                             int decoding_delay,
+                             int parallel_decoding)
+      : is_encrypted(is_encrypted),
+        enable_get_decode_output(enable_get_decode_output),
+        decoding_delay(decoding_delay),
+        parallel_decoding(parallel_decoding) {}
+
+  bool is_encrypted;
+  bool enable_get_decode_output;
+  int decoding_delay;
+  int parallel_decoding;
+};
+
 class VideoFrameStreamTest
     : public testing::Test,
-      public testing::WithParamInterface<std::tr1::tuple<bool, bool> > {
+      public testing::WithParamInterface<VideoFrameStreamTestParams> {
  public:
   VideoFrameStreamTest()
       : demuxer_stream_(new FakeDemuxerStream(kNumConfigs,
                                               kNumBuffersInOneConfig,
-                                              IsEncrypted())),
+                                              GetParam().is_encrypted)),
         decryptor_(new NiceMock<MockDecryptor>()),
-        decoder_(
-            new FakeVideoDecoder(kDecodingDelay, IsGetDecodeOutputEnabled())),
+        decoder_(new FakeVideoDecoder(GetParam().decoding_delay,
+                                      GetParam().enable_get_decode_output,
+                                      GetParam().parallel_decoding)),
         is_initialized_(false),
         num_decoded_frames_(0),
         pending_initialize_(false),
@@ -73,14 +90,6 @@ class VideoFrameStreamTest
     EXPECT_FALSE(is_initialized_);
   }
 
-  bool IsEncrypted() {
-    return std::tr1::get<0>(GetParam());
-  }
-
-  bool IsGetDecodeOutputEnabled() {
-    return std::tr1::get<1>(GetParam());
-  }
-
   MOCK_METHOD1(SetDecryptorReadyCallback, void(const media::DecryptorReadyCB&));
 
   void OnStatistics(const PipelineStatistics& statistics) {
@@ -120,8 +129,8 @@ class VideoFrameStreamTest
     }
 
     DCHECK_EQ(stream_type, Decryptor::kVideo);
-    scoped_refptr<DecoderBuffer> decrypted = DecoderBuffer::CopyFrom(
-        encrypted->data(), encrypted->data_size());
+    scoped_refptr<DecoderBuffer> decrypted =
+        DecoderBuffer::CopyFrom(encrypted->data(), encrypted->data_size());
     decrypted->set_timestamp(encrypted->timestamp());
     decrypted->set_duration(encrypted->duration());
     decrypt_cb.Run(Decryptor::kSuccess, decrypted);
@@ -131,16 +140,19 @@ class VideoFrameStreamTest
   void FrameReady(VideoFrameStream::Status status,
                   const scoped_refptr<VideoFrame>& frame) {
     DCHECK(pending_read_);
-    // TODO(xhwang): Add test cases where the fake decoder returns error or
-    // the fake demuxer aborts demuxer read.
-    ASSERT_TRUE(status == VideoFrameStream::OK ||
-                status == VideoFrameStream::ABORTED) << status;
     frame_read_ = frame;
+    last_read_status_ = status;
     if (frame.get() && !frame->end_of_stream())
       num_decoded_frames_++;
     pending_read_ = false;
   }
 
+  void FrameReadyHoldDemuxer(VideoFrameStream::Status status,
+                             const scoped_refptr<VideoFrame>& frame) {
+    FrameReady(status, frame);
+
+  }
+
   void OnReset() {
     DCHECK(!pending_read_);
     DCHECK(pending_reset_);
@@ -224,7 +236,7 @@ class VideoFrameStreamTest
         break;
 
       case DECODER_DECODE:
-        decoder_->HoldNextDecode();
+        decoder_->HoldDecode();
         ReadUntilPending();
         break;
 
@@ -322,6 +334,7 @@ class VideoFrameStreamTest
   bool pending_stop_;
   int total_bytes_decoded_;
   scoped_refptr<VideoFrame> frame_read_;
+  VideoFrameStream::Status last_read_status_;
 
   // Decryptor has no key to decrypt a frame.
   bool has_no_key_;
@@ -330,14 +343,34 @@ class VideoFrameStreamTest
   DISALLOW_COPY_AND_ASSIGN(VideoFrameStreamTest);
 };
 
-INSTANTIATE_TEST_CASE_P(Clear, VideoFrameStreamTest,
-  testing::Combine(testing::Values(false), testing::Values(false)));
-INSTANTIATE_TEST_CASE_P(Clear_GetDecodeOutput, VideoFrameStreamTest,
-  testing::Combine(testing::Values(false), testing::Values(true)));
-INSTANTIATE_TEST_CASE_P(Encrypted, VideoFrameStreamTest,
-  testing::Combine(testing::Values(true), testing::Values(false)));
-INSTANTIATE_TEST_CASE_P(Encrypted_GetDecodeOutput, VideoFrameStreamTest,
-  testing::Combine(testing::Values(true), testing::Values(true)));
+INSTANTIATE_TEST_CASE_P(
+    Clear,
+    VideoFrameStreamTest,
+    ::testing::Values(
+        VideoFrameStreamTestParams(false, false, kDecodingDelay, 1)));
+INSTANTIATE_TEST_CASE_P(
+    Clear_GetDecodeOutput,
+    VideoFrameStreamTest,
+    ::testing::Values(
+        VideoFrameStreamTestParams(false, true, kDecodingDelay, 1)));
+INSTANTIATE_TEST_CASE_P(
+    Encrypted,
+    VideoFrameStreamTest,
+    ::testing::Values(
+        VideoFrameStreamTestParams(true, false, kDecodingDelay, 1)));
+INSTANTIATE_TEST_CASE_P(
+    Encrypted_GetDecodeOutput,
+    VideoFrameStreamTest,
+    ::testing::Values(
+        VideoFrameStreamTestParams(true, true, kDecodingDelay, 1)));
+
+INSTANTIATE_TEST_CASE_P(
+    Clear_Parallel,
+    VideoFrameStreamTest,
+    ::testing::Values(
+        VideoFrameStreamTestParams(true, false, 0, 3),
+        VideoFrameStreamTestParams(true, false, kDecodingDelay, 3)));
+
 
 TEST_P(VideoFrameStreamTest, Initialization) {
   Initialize();
@@ -366,6 +399,36 @@ TEST_P(VideoFrameStreamTest, Read_AfterReset) {
   Read();
 }
 
+TEST_P(VideoFrameStreamTest, Read_BlockedDemuxer) {
+  Initialize();
+  demuxer_stream_->HoldNextRead();
+  ReadOneFrame();
+  EXPECT_TRUE(pending_read_);
+
+  int demuxed_frames = 0;

[xhwang, 2014/04/29 20:01:26]

nit: s/frame/buffer

Actually you can just use FakeDemuxerStream::num_buffers_returned().

[Sergey Ulanov, 2014/04/30 18:56:52]

Done.

+
+  // Pass frames from the demuxer to the VideoFrameStream until the first read
+  // request is satisfied.
+  while (pending_read_) {
+    ++demuxed_frames;
+    demuxer_stream_->SatisfyReadAndHoldNext();
+    message_loop_.RunUntilIdle();
+  }
+
+  // Stream should have read at least one buffer from the demuxer, but not more
+  // than decoding_delay-1.
+  EXPECT_EQ(std::max(GetParam().decoding_delay - 1, 1), demuxed_frames);

[xhwang, 2014/04/29 20:01:26]

why? For example:
input:  1 2 3 ...
output:   1 2 ...

decoding_delay is 1
so std::max(GetParam().decoding_delay - 1, 1) is 1.
but now we have one frame decoded, so we should have 2 demuxed buffers.
You can change the test parameters to repro this.

[Sergey Ulanov, 2014/04/30 18:56:52]

Yes, you are right. This should be 
  std::min(GetParam().decoding_delay + 1, kNumBuffersInOneConfig + 1);

+
+  // At this point the stream is waiting on read from the demuxer, but there is
+  // no pending read from the stream. The stream should be blocked if we try
+  // reading from it again.
+  ReadUntilPending();

[xhwang, 2014/04/29 20:01:26]

My original proposal is to add a test where we have:
1, multiple decode calls
2, multiple frames decoded and stored in ready_outputs_
3, then we have a demuxer read blocking
4, VideoFrameStream::Read()
We want to make sure the the last VFS::Read() can still be satisfied without
satisfying the blocking demuxer read.

I want to test it because it's one of the motivations of this CL.

The current test doesn't cover this case. I verified that we never accumulate
internal buffers with the current settings.

[Sergey Ulanov, 2014/04/30 18:56:52]

That's a good point. Added another test now.

+
+  demuxer_stream_->SatisfyRead();
+  message_loop_.RunUntilIdle();
+  EXPECT_FALSE(pending_read_);
+}
+
 // No Reset() before initialization is successfully completed.
 
 TEST_P(VideoFrameStreamTest, Reset_AfterInitialization) {
@@ -449,7 +512,7 @@ TEST_P(VideoFrameStreamTest, Stop_BeforeInitialization) {
 }
 
 TEST_P(VideoFrameStreamTest, Stop_DuringSetDecryptor) {
-  if (!IsEncrypted()) {
+  if (!GetParam().is_encrypted) {
     DVLOG(1) << "SetDecryptor test only runs when the stream is encrytped.";
     return;
   }
@@ -555,4 +618,38 @@ TEST_P(VideoFrameStreamTest, Stop_AfterRead_AfterReset) {
   Stop();
 }
 
+TEST_P(VideoFrameStreamTest, DecoderErrorWhenReading) {
+  Initialize();
+  EnterPendingState(DECODER_DECODE);
+  decoder_->SimulateError();
+  message_loop_.RunUntilIdle();
+  ASSERT_FALSE(pending_read_);
+  ASSERT_EQ(VideoFrameStream::DECODE_ERROR, last_read_status_);
+}
+
+TEST_P(VideoFrameStreamTest, DecoderErrorWhenNotReading) {
+  Initialize();
+
+  decoder_->HoldDecode();
+  ReadOneFrame();
+  EXPECT_TRUE(pending_read_);
+
+  // Satisfy decode requests until we get the first frame out.
+  while (pending_read_) {
+    decoder_->SatisfySingleDecode();
+    message_loop_.RunUntilIdle();
+  }
+
+  // Trigger an error in the decoding.
+  decoder_->SimulateError();
+
+  // The error must surface from Read() as DECODE_ERROR.
+  while (last_read_status_ == VideoFrameStream::OK) {
+    ReadOneFrame();
+    message_loop_.RunUntilIdle();
+    EXPECT_FALSE(pending_read_);
+  }
+  EXPECT_EQ(VideoFrameStream::DECODE_ERROR, last_read_status_);
+}
+
 }  // namespace media