vda unittest: delete GLRenderingVDAClient in correct thread

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 188 matching lines @@
                                    base::SPLIT_WANT_ALL);
   // Check these are legitimate MD5s.
   for (const std::string& md5_string : *md5_strings) {
     // Ignore the empty string added by SplitString
     if (!md5_string.length())
       continue;
     // Ignore comments
     if (md5_string.at(0) == '#')
       continue;
 
-    LOG_ASSERT(static_cast<int>(md5_string.length()) == kMD5StringLength)
-        << md5_string;
+    LOG_IF(ERROR, static_cast<int>(md5_string.length()) != kMD5StringLength)
+        << "MD5 length error: " << md5_string;
     bool hex_only = std::count_if(md5_string.begin(), md5_string.end(),
                                   isxdigit) == kMD5StringLength;
-    LOG_ASSERT(hex_only) << md5_string;
+    LOG_IF(ERROR, !hex_only) << "MD5 includes non-hex char: " << md5_string;
   }
-  LOG_ASSERT(md5_strings->size() >= 1U) << "  MD5 checksum file ("
-                                        << filepath.MaybeAsASCII()
-                                        << ") missing or empty.";
+  LOG_IF(ERROR, md5_strings->empty()) << "  MD5 checksum file ("
+                                      << filepath.MaybeAsASCII()
+                                      << ") missing or empty.";
 }
 
 // State of the GLRenderingVDAClient below.  Order matters here as the test
 // makes assumptions about it.
 enum ClientState {
   CS_CREATED = 0,
   CS_DECODER_SET = 1,
   CS_INITIALIZED = 2,
   CS_FLUSHING = 3,
   CS_FLUSHED = 4,
@@ skipping 547 matching lines @@
 }
 
 void GLRenderingVDAClient::ReturnPicture(int32_t picture_buffer_id) {
   if (decoder_deleted())
     return;
   LOG_ASSERT(1U == pending_textures_.erase(picture_buffer_id));
 
   if (pending_textures_.empty() && state_ == CS_RESETTING) {
     SetState(CS_RESET);
     DeleteDecoder();
+    // Cascade through the rest of the states to simplify test code below.
+    for (int i = state_ + 1; i < CS_MAX; ++i)
+      SetState(static_cast<ClientState>(i));
     return;
   }
 
   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::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()) {
     // TODO(owenlin): We should not have to check the number of
@@ skipping 40 matching lines @@
     encoded_data_next_pos_to_decode_ = 0;
     FinishInitialization();
     return;
   }
 
   rendering_helper_->Flush(window_id_);
 
   if (pending_textures_.empty()) {
     SetState(CS_RESET);
     DeleteDecoder();
+    // Cascade through the rest of the states to simplify test code below.
+    for (int i = state_ + 1; i < CS_MAX; ++i)
+      SetState(static_cast<ClientState>(i));
   }
 }
 
 void GLRenderingVDAClient::NotifyError(VideoDecodeAccelerator::Error error) {
   SetState(CS_ERROR);
 }
 
 void GLRenderingVDAClient::OutputFrameDeliveryTimes(base::File* output) {
   std::string s = base::StringPrintf("frame count: %" PRIuS "\n",
                                      frame_delivery_times_.size());
@@ skipping 11 matching lines @@
   return encoded[pos] == 0 && encoded[pos + 1] == 0 && encoded[pos + 2] == 0 &&
          encoded[pos + 3] == 1;
 }
 
 void GLRenderingVDAClient::SetState(ClientState new_state) {
   note_->Notify(new_state);
   state_ = new_state;
   if (!remaining_play_throughs_ && new_state == delete_decoder_state_) {
     LOG_ASSERT(!decoder_deleted());
     DeleteDecoder();
+    // Cascade through the rest of the states to simplify test code below.
+    for (int i = delete_decoder_state_ + 1; i < CS_MAX; ++i)

[johnylin1, 2017/01/10 14:01:55]

This change makes us need to call SetState() inside SetState(), although it can
be guaranteed there is no endless recursive possibility, it still looks not
pretty.

One thought is we could do this way:

void GLRenderingVDAClient::SetState(ClientState new_state) {
  note_->Notify(new_state);
  state_ = new_state;
  if (!remaining_play_throughs_ && new_state == delete_decoder_state_) {
    LOG_ASSERT(!decoder_deleted());
    DeleteDecoder();
    // Cascade through the rest of the states to simplify test code below.
    for (int i = delete_decoder_state_ + 1; i < CS_MAX; ++i) {
      note_->Notify(static_cast<ClientState>(i));
      state_ = static_cast<ClientState>(i);
    }
  }
}

We can avoid any recursion but spending more lines.

+      SetState(static_cast<ClientState>(i));
   }
 }
 
 void GLRenderingVDAClient::FinishInitialization() {
   SetState(CS_INITIALIZED);
   initialize_done_ticks_ = base::TimeTicks::Now();
 
   if (reset_after_frame_num_ == START_OF_STREAM_RESET) {
     reset_after_frame_num_ = MID_STREAM_RESET;
     decoder_->Reset();
     return;
   }
 
   for (int i = 0; i < num_in_flight_decodes_; ++i)
     DecodeNextFragment();
   DCHECK_EQ(outstanding_decodes_, num_in_flight_decodes_);
 }
 
 void GLRenderingVDAClient::DeleteDecoder() {
   if (decoder_deleted())
     return;
   weak_vda_ptr_factory_->InvalidateWeakPtrs();
   decoder_.reset();
   base::STLClearObject(&encoded_data_);
   active_textures_.clear();
-
-  // Cascade through the rest of the states to simplify test code below.
-  for (int i = state_ + 1; i < CS_MAX; ++i)
-    SetState(static_cast<ClientState>(i));
 }
 
 std::string GLRenderingVDAClient::GetBytesForFirstFragment(size_t start_pos,
                                                            size_t* end_pos) {
   if (profile_ < H264PROFILE_MAX) {
     *end_pos = start_pos;
     while (*end_pos + 4 < encoded_data_.size()) {
       if ((encoded_data_[*end_pos + 4] & 0x1f) == 0x7)  // SPS start frame
         return GetBytesForNextFragment(*end_pos, end_pos);
       GetBytesForNextNALU(*end_pos, end_pos);
@@ skipping 111 matching lines @@
   BitstreamBuffer bitstream_buffer(next_bitstream_buffer_id_, dup_handle,
                                    next_fragment_size);
   decode_start_time_[next_bitstream_buffer_id_] = base::TimeTicks::Now();
   // Mask against 30 bits, to avoid (undefined) wraparound on signed integer.
   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();
+    // Cascade through the rest of the states to simplify test code below.
+    for (int i = state_ + 1; i < CS_MAX; ++i)
+      SetState(static_cast<ClientState>(i));
   }
 
   if (reset_here) {
     reset_after_frame_num_ = MID_STREAM_RESET;
     decoder_->Reset();
     // 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;
   }
@@ skipping 50 matching lines @@
   void WaitUntilIdle();
   void OutputLogFile(const base::FilePath::CharType* log_path,
                      const std::string& content);
 
   TestFilesVector test_video_files_;
   RenderingHelper rendering_helper_;
 
  protected:
   // Must be static because this method may run after the destructor.
   template <typename T>
-  static void Delete(std::unique_ptr<T> item) {
+  static void Delete(T item) {
     // |item| is cleared when the scope of this function is left.
   }
 
  private:
   // Required for Thread to work.  Not used otherwise.
   base::ShadowingAtExitManager at_exit_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(VideoDecodeAcceleratorTest);
 };
 
 VideoDecodeAcceleratorTest::VideoDecodeAcceleratorTest() {}
 
 void VideoDecodeAcceleratorTest::SetUp() {
   ParseAndReadTestVideoData(g_test_video_data, &test_video_files_);
 }
 
 void VideoDecodeAcceleratorTest::TearDown() {
-  std::unique_ptr<TestFilesVector> test_video_files(new TestFilesVector);
-  test_video_files->swap(test_video_files_);
-
   g_env->GetRenderingTaskRunner()->PostTask(
       FROM_HERE,
-      base::Bind(&Delete<TestFilesVector>, base::Passed(&test_video_files)));
+      base::Bind(&Delete<TestFilesVector>, base::Passed(&test_video_files_)));
 
   base::WaitableEvent done(base::WaitableEvent::ResetPolicy::AUTOMATIC,
                            base::WaitableEvent::InitialState::NOT_SIGNALED);
   g_env->GetRenderingTaskRunner()->PostTask(
       FROM_HERE, base::Bind(&RenderingHelper::UnInitialize,
                             base::Unretained(&rendering_helper_), &done));
   done.Wait();
 
   rendering_helper_.TearDown();
 }
@@ skipping 118 matching lines @@
 //   instantiated.
 // - Number of concurrent in-flight Decode() calls per decoder.
 // - Number of play-throughs.
 // - reset_after_frame_num: see GLRenderingVDAClient ctor.
 // - delete_decoder_phase: see GLRenderingVDAClient ctor.
 // - whether to test slow rendering by delaying ReusePictureBuffer().
 // - whether the video frames are rendered as thumbnails.
 class VideoDecodeAcceleratorParamTest
     : public VideoDecodeAcceleratorTest,
       public ::testing::WithParamInterface<
-          std::tuple<int, int, int, ResetPoint, ClientState, bool, bool>> {};
+          std::tuple<int, int, int, ResetPoint, ClientState, bool, bool>> {
+ protected:
+  using NotesVector =
+      std::vector<std::unique_ptr<ClientStateNotification<ClientState>>>;
+  using ClientsVector = std::vector<std::unique_ptr<GLRenderingVDAClient>>;
+
+  void TearDown() override;
+
+  NotesVector notes_;
+  ClientsVector clients_;
+};
+
+void VideoDecodeAcceleratorParamTest::TearDown() {
+  // |clients_| must be deleted first because |clients_| use |notes_|.
+  g_env->GetRenderingTaskRunner()->PostTask(
+      FROM_HERE, base::Bind(&Delete<ClientsVector>, base::Passed(&clients_)));
+
+  g_env->GetRenderingTaskRunner()->PostTask(
+      FROM_HERE, base::Bind(&Delete<NotesVector>, base::Passed(&notes_)));
+
+  WaitUntilIdle();
+
+  // Do VideoDecodeAcceleratorTest clean-up after deleting clients and notes.
+  VideoDecodeAcceleratorTest::TearDown();
+}
 
 // Wait for |note| to report a state and if it's not |expected_state| then
 // assert |client| has deleted its decoder.
 static void AssertWaitForStateOrDeleted(
     ClientStateNotification<ClientState>* note,
     GLRenderingVDAClient* client,
     ClientState expected_state) {
   ClientState state = note->Wait();
   if (state == expected_state)
     return;
@@ skipping 23 matching lines @@
 
   if (g_num_play_throughs > 0)
     num_play_throughs = g_num_play_throughs;
 
   UpdateTestVideoFileParams(num_concurrent_decoders, reset_point,
                             &test_video_files_);
 
   // Suppress GL rendering for all tests when the "--rendering_fps" is 0.
   const bool suppress_rendering = g_rendering_fps == 0;
 
-  using NotesVector =
-      std::vector<std::unique_ptr<ClientStateNotification<ClientState>>>;
-  using ClientsVector = std::vector<std::unique_ptr<GLRenderingVDAClient>>;
-  NotesVector notes(num_concurrent_decoders);
-  ClientsVector clients(num_concurrent_decoders);
+  notes_.resize(num_concurrent_decoders);
+  clients_.resize(num_concurrent_decoders);
 
   RenderingHelperParams helper_params;
   helper_params.rendering_fps = g_rendering_fps;
   helper_params.warm_up_iterations = g_rendering_warm_up;
   helper_params.render_as_thumbnails = render_as_thumbnails;
   if (render_as_thumbnails) {
     // Only one decoder is supported with thumbnail rendering
     LOG_ASSERT(num_concurrent_decoders == 1U);
     helper_params.thumbnails_page_size = kThumbnailsPageSize;
     helper_params.thumbnail_size = kThumbnailSize;
   }
 
   // First kick off all the decoders.
   for (size_t index = 0; index < num_concurrent_decoders; ++index) {
     TestVideoFile* video_file =
         test_video_files_[index % test_video_files_.size()].get();
     std::unique_ptr<ClientStateNotification<ClientState>> note =
         base::MakeUnique<ClientStateNotification<ClientState>>();
-    notes[index] = std::move(note);
+    notes_[index] = std::move(note);
 
     int delay_after_frame_num = std::numeric_limits<int>::max();
     if (test_reuse_delay &&
         kMaxFramesToDelayReuse * 2 < video_file->num_frames) {
       delay_after_frame_num = video_file->num_frames - kMaxFramesToDelayReuse;
     }
 
     std::unique_ptr<GLRenderingVDAClient> client =
         base::MakeUnique<GLRenderingVDAClient>(
-            index, &rendering_helper_, notes[index].get(), video_file->data_str,
-            num_in_flight_decodes, num_play_throughs,
+            index, &rendering_helper_, notes_[index].get(),
+            video_file->data_str, num_in_flight_decodes, num_play_throughs,
             video_file->reset_after_frame_num, delete_decoder_state,
             video_file->width, video_file->height, video_file->profile,
             g_fake_decoder, suppress_rendering, delay_after_frame_num, 0,
             render_as_thumbnails);
 
-    clients[index] = std::move(client);
+    clients_[index] = std::move(client);
     helper_params.window_sizes.push_back(
         render_as_thumbnails
             ? kThumbnailsPageSize
             : gfx::Size(video_file->width, video_file->height));
   }
 
   InitializeRenderingHelper(helper_params);
 
   for (size_t index = 0; index < num_concurrent_decoders; ++index) {
-    CreateAndStartDecoder(clients[index].get(), notes[index].get());
+    CreateAndStartDecoder(clients_[index].get(), notes_[index].get());
   }
 
   // Then wait for all the decodes to finish.
   // Only check performance & correctness later if we play through only once.
   bool skip_performance_and_correctness_checks = num_play_throughs > 1;
   for (size_t i = 0; i < num_concurrent_decoders; ++i) {
-    ClientStateNotification<ClientState>* note = notes[i].get();
+    ClientStateNotification<ClientState>* note = notes_[i].get();
     ClientState state = note->Wait();
     if (state != CS_INITIALIZED) {
       skip_performance_and_correctness_checks = true;
       // We expect initialization to fail only when more than the supported
       // number of decoders is instantiated.  Assert here that something else
       // didn't trigger failure.
       ASSERT_GT(num_concurrent_decoders,
                 static_cast<size_t>(kMinSupportedNumConcurrentDecoders));
       continue;
     }
-    ASSERT_EQ(state, CS_INITIALIZED);
     for (int n = 0; n < num_play_throughs; ++n) {
       // For play-throughs other than the first, we expect initialization to
       // succeed unconditionally.
       if (n > 0) {
         ASSERT_NO_FATAL_FAILURE(AssertWaitForStateOrDeleted(
-            note, clients[i].get(), CS_INITIALIZED));
+            note, clients_[i].get(), CS_INITIALIZED));
       }
       // InitializeDone kicks off decoding inside the client, so we just need to
       // wait for Flush.
       ASSERT_NO_FATAL_FAILURE(
-          AssertWaitForStateOrDeleted(note, clients[i].get(), CS_FLUSHING));
+          AssertWaitForStateOrDeleted(note, clients_[i].get(), CS_FLUSHING));
       ASSERT_NO_FATAL_FAILURE(
-          AssertWaitForStateOrDeleted(note, clients[i].get(), CS_FLUSHED));
+          AssertWaitForStateOrDeleted(note, clients_[i].get(), CS_FLUSHED));
       // FlushDone requests Reset().
       ASSERT_NO_FATAL_FAILURE(
-          AssertWaitForStateOrDeleted(note, clients[i].get(), CS_RESETTING));
+          AssertWaitForStateOrDeleted(note, clients_[i].get(), CS_RESETTING));
     }
     ASSERT_NO_FATAL_FAILURE(
-        AssertWaitForStateOrDeleted(note, clients[i].get(), CS_RESET));
+        AssertWaitForStateOrDeleted(note, clients_[i].get(), CS_RESET));
     // ResetDone requests Destroy().
     ASSERT_NO_FATAL_FAILURE(
-        AssertWaitForStateOrDeleted(note, clients[i].get(), CS_DESTROYED));
+        AssertWaitForStateOrDeleted(note, clients_[i].get(), CS_DESTROYED));
   }
   // Finally assert that decoding went as expected.
   for (size_t i = 0;
        i < num_concurrent_decoders && !skip_performance_and_correctness_checks;
        ++i) {
     // We can only make performance/correctness assertions if the decoder was
     // allowed to finish.
     if (delete_decoder_state < CS_FLUSHED)
       continue;
-    GLRenderingVDAClient* client = clients[i].get();
+    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
         EXPECT_EQ(client->num_decoded_frames(), video_file->num_frames);
     }
@@ skipping 40 matching lines @@
                             std::vector<gfx::PNGCodec::Comment>(),
                             &png);
 
       LOG(ERROR) << "Unknown thumbnails MD5: " << md5_string;
 
       base::FilePath filepath(test_video_files_[0]->file_name);
       filepath = filepath.AddExtension(FILE_PATH_LITERAL(".bad_thumbnails"));
       filepath = filepath.AddExtension(FILE_PATH_LITERAL(".png"));
       int num_bytes = base::WriteFile(
           filepath, reinterpret_cast<char*>(&png[0]), png.size());
-      ASSERT_EQ(num_bytes, static_cast<int>(png.size()));
+      EXPECT_EQ(num_bytes, static_cast<int>(png.size()));
     }
-    ASSERT_NE(match, golden_md5s.end());
+    EXPECT_NE(match, golden_md5s.end());
     EXPECT_EQ(alpha_solid, true) << "RGBA frame had incorrect alpha";
   }
 
   // Output the frame delivery time to file
   // We can only make performance/correctness assertions if the decoder was
   // allowed to finish.
   if (g_output_log != NULL && delete_decoder_state >= CS_FLUSHED) {
     base::File output_file(
         base::FilePath(g_output_log),
         base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
     for (size_t i = 0; i < num_concurrent_decoders; ++i) {
-      clients[i]->OutputFrameDeliveryTimes(&output_file);
+      clients_[i]->OutputFrameDeliveryTimes(&output_file);
     }
   }
-
-  std::unique_ptr<ClientsVector> clients2(new ClientsVector);
-  clients2->swap(clients);
-  std::unique_ptr<NotesVector> notes2(new NotesVector);
-  notes2->swap(notes);
-
-  // |clients| must be deleted first because |clients| use |notes2|.
-  g_env->GetRenderingTaskRunner()->PostTask(
-      FROM_HERE, base::Bind(&Delete<ClientsVector>, base::Passed(&clients2)));
-
-  g_env->GetRenderingTaskRunner()->PostTask(
-      FROM_HERE, base::Bind(&Delete<NotesVector>, base::Passed(&notes2)));
-
-  WaitUntilIdle();
 };
 
 // Test that replay after EOS works fine.
 INSTANTIATE_TEST_CASE_P(
     ReplayAfterEOS,
     VideoDecodeAcceleratorParamTest,
     ::testing::Values(
         std::make_tuple(1, 1, 4, END_OF_STREAM_RESET, CS_RESET, false, false)));
 
 // Test that Reset() before the first Decode() works fine.
@@ skipping 296 matching lines @@
 
   media::g_env =
       reinterpret_cast<media::VideoDecodeAcceleratorTestEnvironment*>(
           testing::AddGlobalTestEnvironment(
               new media::VideoDecodeAcceleratorTestEnvironment()));
 
   return base::LaunchUnitTestsSerially(
       argc, argv,
       base::Bind(&media::VDATestSuite::Run, base::Unretained(&test_suite)));
 }