VDA unittest: test for calling Reset before NotifyFlushDone

media/gpu/video_decode_accelerator_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // The bulk of this file is support code; sorry about that.  Here's an overview
 // to hopefully help readers of this code:
 // - RenderingHelper is charged with interacting with X11/{EGL/GLES2,GLX/GL} or
 //   Win/EGL.
 // - ClientState is an enum for the state of the decode client used by the test.
 // - ClientStateNotification is a barrier abstraction that allows the test code
@@ skipping 132 matching lines @@
 // working directory.
 base::FilePath g_test_file_path;
 
 // Environment to store rendering thread.
 class VideoDecodeAcceleratorTestEnvironment;
 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,
   MID_STREAM_RESET = -2,
   END_OF_STREAM_RESET = -1
 };
 
 const int kMaxResetAfterFrameNum = 100;
 const int kMaxFramesToDelayReuse = 64;
 const base::TimeDelta kReuseDelay = base::TimeDelta::FromSeconds(1);
@@ skipping 292 matching lines @@
   void NotifyResetDone() override;
   void NotifyError(VideoDecodeAccelerator::Error error) override;
 
   void OutputFrameDeliveryTimes(base::File* output);
 
   // Simple getters for inspecting the state of the Client.
   int num_done_bitstream_buffers() { return num_done_bitstream_buffers_; }
   int num_skipped_fragments() { return num_skipped_fragments_; }
   int num_queued_fragments() { return num_queued_fragments_; }
   int num_decoded_frames() { return num_decoded_frames_; }
+  int num_decoded_frames_before_reset() {
+    return num_decoded_frames_before_reset_;
+  }
   double frames_per_second();
   // Return the median of the decode time of all decoded frames.
   base::TimeDelta decode_time_median();
   bool decoder_deleted() { return !decoder_.get(); }
 
  private:
   typedef std::map<int32_t, scoped_refptr<TextureRef>> TextureRefMap;
 
   void SetState(ClientState new_state);
   void FinishInitialization();
   void ReturnPicture(int32_t picture_buffer_id);
 
   // Delete the associated decoder helper.
   void DeleteDecoder();
+  // Reset the associated decoder helpr.

[wuchengli, 2017/03/02 09:44:04]

helper

[johnylin1, 2017/03/07 07:44:16]

Done.

+  void ResetDecoder();
 
   // Compute & return the first encoded bytes (including a start frame) to send
   // to the decoder, starting at |start_pos| and returning one fragment. Skips
   // to the first decodable position.
   std::string GetBytesForFirstFragment(size_t start_pos, size_t* end_pos);
   // Compute & return the encoded bytes of next fragment to send to the decoder
   // (based on |start_pos|).
   std::string GetBytesForNextFragment(size_t start_pos, size_t* end_pos);
   // Helpers for GetBytesForNextFragment above.
   void GetBytesForNextNALU(size_t start_pos, size_t* end_pos);  // For h.264.
@@ skipping 17 matching lines @@
   std::unique_ptr<base::WeakPtrFactory<VideoDecodeAccelerator>>
       weak_vda_ptr_factory_;
   std::unique_ptr<GpuVideoDecodeAcceleratorFactory> vda_factory_;
   int remaining_play_throughs_;
   int reset_after_frame_num_;
   int delete_decoder_state_;
   ClientState state_;
   int num_skipped_fragments_;
   int num_queued_fragments_;
   int num_decoded_frames_;
+  int num_decoded_frames_before_reset_;
   int num_done_bitstream_buffers_;
   base::TimeTicks initialize_done_ticks_;
   VideoCodecProfile profile_;
   int fake_decoder_;
   GLenum texture_target_;
   VideoPixelFormat pixel_format_;
   bool suppress_rendering_;
   std::vector<base::TimeTicks> frame_delivery_times_;
   int delay_reuse_after_frame_num_;
   // A map from bitstream buffer id to the decode start time of the buffer.
@@ skipping 61 matching lines @@
       encoded_data_next_pos_to_decode_(0),
       next_bitstream_buffer_id_(0),
       note_(note),
       remaining_play_throughs_(num_play_throughs),
       reset_after_frame_num_(reset_after_frame_num),
       delete_decoder_state_(delete_decoder_state),
       state_(CS_CREATED),
       num_skipped_fragments_(0),
       num_queued_fragments_(0),
       num_decoded_frames_(0),
+      num_decoded_frames_before_reset_(0),
       num_done_bitstream_buffers_(0),
       fake_decoder_(fake_decoder),
       texture_target_(0),
       pixel_format_(PIXEL_FORMAT_UNKNOWN),
       suppress_rendering_(suppress_rendering),
       delay_reuse_after_frame_num_(delay_reuse_after_frame_num),
       decode_calls_per_second_(decode_calls_per_second),
       render_as_thumbnails_(render_as_thumbnails),
       next_picture_buffer_id_(1),
       weak_this_factory_(this) {
@@ skipping 193 matching lines @@
   if (num_decoded_frames_ > delay_reuse_after_frame_num_) {
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE, base::Bind(&VideoDecodeAccelerator::ReusePictureBuffer,
                               weak_vda_, picture_buffer_id),
         kReuseDelay);
   } else {
     decoder_->ReusePictureBuffer(picture_buffer_id);
   }
 }
 
+void GLRenderingVDAClient::ResetDecoder() {

[wuchengli, 2017/03/02 09:44:05]

The name is not very good. There are three other decoder_->Reset(); not calling
this function. Is it possible to call ResetDecoder() in every
decoder_->Reset();? If not, num_decoded_frames_before_reset_ is only set
correctly sometimes. I also feel num_decoded_frames_before_reset_ is not the
main thing we want to test here. Removing it can simplify the code a bit.

[wuchengli, 2017/03/02 10:07:12]

As discussed, let's use ResetDecoderAfterFlush.

[johnylin1, 2017/03/07 07:44:16]

Done.

+  --remaining_play_throughs_;
+  DCHECK_GE(remaining_play_throughs_, 0);
+  num_decoded_frames_before_reset_ = num_decoded_frames_;

[wuchengli, 2017/03/02 10:07:12]

It's more consistent to set num_decoded_frames_before_reset_ every time we call
decoder_->Reset(). But I feel it's lots of code. Pawel. How about removing this?

[johnylin1, 2017/03/07 07:44:16]

After discussed, we gonna add this check after refactoring VDA unittest. Remove
it in this CL.

+  decoder_->Reset();
+  if (state_ == CS_FLUSHED)

[wuchengli, 2017/03/02 09:44:04]

I don't understand. Why do we need this line?

[wuchengli, 2017/03/02 10:07:11]

As discussed, this can be removed.

[johnylin1, 2017/03/07 07:44:16]

Done.

+    SetState(CS_RESETTING);
+}
+
 void GLRenderingVDAClient::NotifyEndOfBitstreamBuffer(
     int32_t bitstream_buffer_id) {
   if (decoder_deleted())
     return;
 
   // TODO(fischman): this test currently relies on this notification to make
   // forward progress during a Reset().  But the VDA::Reset() API doesn't
   // guarantee this, so stop relying on it (and remove the notifications from
   // VaapiVideoDecodeAccelerator::FinishReset()).
   ++num_done_bitstream_buffers_;
   --outstanding_decodes_;
 
   // Flush decoder after all BitstreamBuffers are processed.
   if (encoded_data_next_pos_to_decode_ == encoded_data_.size()) {
     if (state_ != CS_FLUSHING) {
       decoder_->Flush();
       SetState(CS_FLUSHING);
+      if (reset_after_frame_num_ == RESET_BEFORE_NOTIFY_FLUSH_DONE) {
+        SetState(CS_FLUSHED);
+        ResetDecoder();
+      }
     }
   } else if (decode_calls_per_second_ == 0) {
     DecodeNextFragment();
   }
 }
 
 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

[wuchengli, 2017/03/02 09:44:05]

s/gets/gets here/

[johnylin1, 2017/03/07 07:44:16]

Done.

+    // NotifyResetDone().
+    LOG_ASSERT(state_ == CS_RESETTING);

[wuchengli, 2017/03/02 09:44:04]

When state_ != CS_RESETTING, it can be a bug of driver. We shouldn't crash the
test. Just ASSERT_EQ it and remove the return.

[johnylin1, 2017/03/07 07:44:16]

Done.

+    return;
+  }
+
   SetState(CS_FLUSHED);
-  --remaining_play_throughs_;
-  DCHECK_GE(remaining_play_throughs_, 0);
-  if (decoder_deleted())
-    return;
-  decoder_->Reset();
-  SetState(CS_RESETTING);
+  ResetDecoder();
 }
 
 void GLRenderingVDAClient::NotifyResetDone() {
   if (decoder_deleted())
     return;
 
   if (reset_after_frame_num_ == MID_STREAM_RESET) {
     reset_after_frame_num_ = END_OF_STREAM_RESET;
     DecodeNextFragment();
     return;
@@ skipping 202 matching lines @@
   next_bitstream_buffer_id_ = (next_bitstream_buffer_id_ + 1) & 0x3FFFFFFF;
   decoder_->Decode(bitstream_buffer);
   ++outstanding_decodes_;
   if (!remaining_play_throughs_ &&
       -delete_decoder_state_ == next_bitstream_buffer_id_) {
     DeleteDecoder();
   }
 
   if (reset_here) {
     reset_after_frame_num_ = MID_STREAM_RESET;
     decoder_->Reset();

[wuchengli, 2017/03/02 09:44:04]

I'm curious. Why the state doesn't need to be set to CS_RESETTING here?

     // Restart from the beginning to re-Decode() the SPS we just sent.
     encoded_data_next_pos_to_decode_ = 0;
   } else {
     encoded_data_next_pos_to_decode_ = end_pos;
   }
 
   if (decode_calls_per_second_ > 0) {
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
         base::Bind(&GLRenderingVDAClient::DecodeNextFragment, AsWeakPtr()),
@@ skipping 379 matching lines @@
     if (delete_decoder_state < CS_FLUSHED)
       continue;
     GLRenderingVDAClient* client = clients_[i].get();
     TestVideoFile* video_file =
         test_video_files_[i % test_video_files_.size()].get();
     if (video_file->num_frames > 0) {
       // Expect the decoded frames may be more than the video frames as frames
       // 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 !=
+               RESET_BEFORE_NOTIFY_FLUSH_DONE)
         EXPECT_EQ(client->num_decoded_frames(), video_file->num_frames);
+      EXPECT_GE(client->num_decoded_frames(),
+                client->num_decoded_frames_before_reset());
     }
     if (reset_point == END_OF_STREAM_RESET) {
       EXPECT_EQ(video_file->num_fragments, client->num_skipped_fragments() +
                                                client->num_queued_fragments());
       EXPECT_EQ(client->num_done_bitstream_buffers(),
                 client->num_queued_fragments());
     }
     LOG(INFO) << "Decoder " << i << " fps: " << client->frames_per_second();
     if (!render_as_thumbnails) {
       int min_fps = suppress_rendering ? video_file->min_fps_no_render
@@ skipping 81 matching lines @@
     ResetAfterFirstConfigInfo,
     VideoDecodeAcceleratorParamTest,
     ::testing::Values(std::make_tuple(1,
                                       1,
                                       1,
                                       RESET_AFTER_FIRST_CONFIG_INFO,
                                       CS_RESET,
                                       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(
     MidStreamReset,
     VideoDecodeAcceleratorParamTest,
     ::testing::Values(
         std::make_tuple(1, 1, 1, MID_STREAM_RESET, CS_RESET, false, false)));
 
 INSTANTIATE_TEST_CASE_P(
     SlowRendering,
@@ skipping 270 matching lines @@
       continue;
     }
   }
 
   base::ShadowingAtExitManager at_exit_manager;
 
   return base::LaunchUnitTestsSerially(
       argc, argv,
       base::Bind(&media::VDATestSuite::Run, base::Unretained(&test_suite)));
 }