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 1045 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() {
+  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.
 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)));
 }