VDA unittest: test for calling Reset before NotifyFlushDone

media/gpu/video_decode_accelerator_unittest.cc

Index: media/gpu/video_decode_accelerator_unittest.cc
diff --git a/media/gpu/video_decode_accelerator_unittest.cc b/media/gpu/video_decode_accelerator_unittest.cc
index cb1bed901e50501a91a6555ef0af4702444692a7..9ee671b3e0726be76c9acedbef39167068e23fcd 100644
--- a/media/gpu/video_decode_accelerator_unittest.cc
+++ b/media/gpu/video_decode_accelerator_unittest.cc
@@ -150,6 +150,8 @@ VideoDecodeAcceleratorTestEnvironment* g_env;
 // Magic constants for differentiating the reasons for NotifyResetDone being
 // called.
 enum ResetPoint {
+  // Reset() right after calling Flush() (before getting NotifyFlushDone()).
+  RESET_BEFORE_NOTIFY_FLUSH_DONE = -5,
   // Reset() just after calling Decode() with a fragment containing config info.
   RESET_AFTER_FIRST_CONFIG_INFO = -4,
   START_OF_STREAM_RESET = -3,
@@ -824,6 +826,13 @@ void GLRenderingVDAClient::NotifyEndOfBitstreamBuffer(
     if (state_ != CS_FLUSHING) {
       decoder_->Flush();
       SetState(CS_FLUSHING);
+      if (reset_after_frame_num_ == RESET_BEFORE_NOTIFY_FLUSH_DONE) {
+        SetState(CS_FLUSHED);

[Pawel Osciak, 2017/02/24 07:49:42]

Would it make sense to unify this code with other similar Reset() cases in a
helper method?

[johnylin1, 2017/03/01 07:17:08]

Done.

+        --remaining_play_throughs_;
+        DCHECK_GE(remaining_play_throughs_, 0);
+        decoder_->Reset();
+        SetState(CS_RESETTING);
+      }
     }
   } else if (decode_calls_per_second_ == 0) {
     DecodeNextFragment();
@@ -834,11 +843,17 @@ void GLRenderingVDAClient::NotifyFlushDone() {
   if (decoder_deleted())
     return;
 
+  if (reset_after_frame_num_ == RESET_BEFORE_NOTIFY_FLUSH_DONE) {
+    // In ResetBeforeNotifyFlushDone case client is not necessary to wait for
+    // NotifyFlushDone(). But if client gets, it should be always before
+    // NotifyResetDone().
+    LOG_ASSERT(state_ == CS_RESETTING);
+    return;
+  }
+
   SetState(CS_FLUSHED);
   --remaining_play_throughs_;
   DCHECK_GE(remaining_play_throughs_, 0);
-  if (decoder_deleted())
-    return;
   decoder_->Reset();
   SetState(CS_RESETTING);
 }
@@ -1463,7 +1478,10 @@ TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
       // could still be returned until resetting done.
       if (video_file->reset_after_frame_num > 0)
         EXPECT_GE(client->num_decoded_frames(), video_file->num_frames);
-      else
+      // In ResetBeforeNotifyFlushDone case the decoded frames may be less than
+      // the video frames because decoder is reset before flush done.
+      else if (video_file->reset_after_frame_num !=

[Pawel Osciak, 2017/02/24 07:49:41]

Would it perhaps be possible to improve both checks to check whether we decoded
at least the number of frames returned/decoded before calling the reset plus
num_frames (as we replay the whole stream from the beginning to the end after
reset)?

[johnylin1, 2017/03/01 07:17:09]

Done.

+               RESET_BEFORE_NOTIFY_FLUSH_DONE)
         EXPECT_EQ(client->num_decoded_frames(), video_file->num_frames);
     }
     if (reset_point == END_OF_STREAM_RESET) {
@@ -1566,6 +1584,18 @@ INSTANTIATE_TEST_CASE_P(
                                       false,
                                       false)));
 
+// Test Reset() immediately after Flush() and before NotifyFlushDone().
+INSTANTIATE_TEST_CASE_P(
+    ResetBeforeNotifyFlushDone,
+    VideoDecodeAcceleratorParamTest,
+    ::testing::Values(std::make_tuple(1,
+                                      1,
+                                      1,
+                                      RESET_BEFORE_NOTIFY_FLUSH_DONE,
+                                      CS_RESET,
+                                      false,
+                                      false)));
+
 // Test that Reset() mid-stream works fine and doesn't affect decoding even when
 // Decode() calls are made during the reset.
 INSTANTIATE_TEST_CASE_P(