vda unittest: delete GLRenderingVDAClient in correct thread

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 ed295a15010ef587332d2e6d5a0879537fab00d7..8b658b0ce6cee2fc56f233e18ceca082527e598a 100644
--- a/media/gpu/video_decode_accelerator_unittest.cc
+++ b/media/gpu/video_decode_accelerator_unittest.cc
@@ -206,15 +206,15 @@ void ReadGoldenThumbnailMD5s(const TestVideoFile* video_file,
     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
@@ -1280,7 +1280,36 @@ void VideoDecodeAcceleratorTest::OutputLogFile(
 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() {
+  std::unique_ptr<ClientsVector> clients2(new ClientsVector);

[Owen Lin, 2017/01/05 05:56:46]

This part of code looks redundant to me.

How about changing Delete to 

template <typename T>
static void Delete<T>(T item) {
  // item will be dropped in the end of the function scope.
}

So that we can pass it directly?

base::Bind(&Delete<ClientVector>, base::Passed(&client_);

I think this also applied to the only other usage where delete a
TestFilesVector.

[johnylin1, 2017/01/05 06:52:55]

I'm not sure if it is intended to do this way before. Delete template was added
in this CL: 
https://codereview.chromium.org/2371783002/patch/140001/150019

And before this CL, it used base::STLDeleteElements() which is already
deprecated, but I guess it was also implemented by unique_ptr

[johnylin1, 2017/01/09 07:11:05]

I think Owen is right. base::Passed() will do std::move so for std::vector we
don't need to wrap an additional unique_ptr then.

+  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();
+
+  // 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.
@@ -1324,11 +1353,8 @@ TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
   // 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;
@@ -1347,7 +1373,7 @@ TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
         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 &&
@@ -1357,14 +1383,14 @@ TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
 
     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
@@ -1374,14 +1400,14 @@ TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
   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;
@@ -1392,29 +1418,28 @@ TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
                 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;
@@ -1424,7 +1449,7 @@ TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
     // 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) {
@@ -1485,9 +1510,9 @@ TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
       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";
   }
 
@@ -1499,23 +1524,9 @@ TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
         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.