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(WARNING, 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(WARNING, !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->size() < 1U) << "  MD5 checksum file ("

[Owen Lin, 2016/12/27 07:32:44]

== 0
Why this one is error and others are just warning? Incorrect or missing file
doesn't sound so different to me.

[johnylin1, 2016/12/27 14:02:06]

My opinion is "missing file" will definitely make test fail because there is
nothing to compare (failure will be raised in line 1538). So it deserves an
ERROR.

"Incorrect format" could be only found in one of md5_string in md5 file (that's
say if there is a mis-typed string in md5 file), but it may not affect the test
if we still get other md5_string matched. So I set them to WARNING and the user
may also get noticed there is incorrect entry in md5 file.

[wuchengli, 2017/01/03 09:10:11]

+1. Use LOG_IF(ERROR, for consistency because it's indeed a bug.

[johnylin1, 2017/01/04 15:34:36]

Done.

+                                          << 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>>;
+
+  // Delete all GLRenderingClient objects in their own thread.
+  void DeleteClientsAndNotes(ClientsVector clients, NotesVector notes);
+};
+
+void VideoDecodeAcceleratorParamTest::DeleteClientsAndNotes(

[wuchengli, 2017/01/03 09:10:12]

This can be renamed to VideoDecodeAcceleratorParamTest::TearDown and override
VideoDecodeAcceleratorTest::TearDown, which does similar clean-ups. Most code in
TestSimpleDecode and AssertWaitForStateOrDeleted don't need to change.

[johnylin1, 2017/01/04 15:34:36]

Sounds great. I override TearDown() in VideoDecodeAcceleratorParamTest ,and in
the end call VideoDecodeAcceleratorTest::TearDown() so everything will be
cleaned in the correct order.

+    ClientsVector clients,
+    NotesVector notes) {
+  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();
+}
 
 // Wait for |note| to report a state and if it's not |expected_state| then
-// assert |client| has deleted its decoder.
+// expect |client| has deleted its decoder and set terminate_ealy.
 static void AssertWaitForStateOrDeleted(
     ClientStateNotification<ClientState>* note,
     GLRenderingVDAClient* client,
+    bool* terminate_early,
     ClientState expected_state) {
+  // If terminate_early is true it means something is already wrong of decoder,
+  // we should bypass all latter state checks.
+  if (*terminate_early)
+    return;
   ClientState state = note->Wait();
   if (state == expected_state)
     return;
-  ASSERT_TRUE(client->decoder_deleted())
-      << "Decoder not deleted but Wait() returned " << state
-      << ", instead of " << expected_state;
+  if (!client->decoder_deleted()) {
+    LOG(ERROR) << "Decoder not deleted but Wait() returned " << state
+               << ", instead of " << expected_state;
+    *terminate_early = true;
+  }
 }
 
 // We assert a minimal number of concurrent decoders we expect to succeed.
 // Different platforms can support more concurrent decoders, so we don't assert
 // failure above this.
 enum { kMinSupportedNumConcurrentDecoders = 3 };
 
 // Test the most straightforward case possible: data is decoded from a single
 // chunk and rendered to the screen.
 TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
@@ skipping 10 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);
 
   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);
@@ skipping 33 matching lines @@
 
   InitializeRenderingHelper(helper_params);
 
   for (size_t index = 0; index < num_concurrent_decoders; ++index) {
     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;
+  bool terminate_early = false;

[Owen Lin, 2016/12/27 07:32:44]

Instead, can we have a helper function which returns true or false ?

bool VerifyClientState(
    num_concurrent_decoders, num_play_throughts, notes, clients) {

  for (size_t i = 0; i < num_.... ) {
     if (state_check_failed) return false;
 
  }


  return true;
}

[johnylin1, 2017/01/04 15:34:36]

This is no longer needed since we gonna delete clients in TearDown(). When the
assertion happens, TearDown() will be called.

   for (size_t i = 0; i < num_concurrent_decoders; ++i) {
     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));
+      // number of decoders is instantiated. Break the loop and then terminate
+      // test gracefully that something else didn't trigger failure. Do not use
+      // assertion otherwise clients won't be deleted in the correct thread.
+      if (num_concurrent_decoders <=
+          static_cast<size_t>(kMinSupportedNumConcurrentDecoders)) {
+        LOG(ERROR) << "Initialization failed while the number of decoders "
+                   << "is still supportable. num_concurrent_decoders = "
+                   << num_concurrent_decoders;
+        terminate_early = true;
+        break;
+      }
       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(), &terminate_early, 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));
-      ASSERT_NO_FATAL_FAILURE(
-          AssertWaitForStateOrDeleted(note, clients[i].get(), CS_FLUSHED));
+      ASSERT_NO_FATAL_FAILURE(AssertWaitForStateOrDeleted(
+          note, clients[i].get(), &terminate_early, CS_FLUSHING));
+      ASSERT_NO_FATAL_FAILURE(AssertWaitForStateOrDeleted(
+          note, clients[i].get(), &terminate_early, CS_FLUSHED));
       // FlushDone requests Reset().
-      ASSERT_NO_FATAL_FAILURE(
-          AssertWaitForStateOrDeleted(note, clients[i].get(), CS_RESETTING));
+      ASSERT_NO_FATAL_FAILURE(AssertWaitForStateOrDeleted(
+          note, clients[i].get(), &terminate_early, CS_RESETTING));
     }
-    ASSERT_NO_FATAL_FAILURE(
-        AssertWaitForStateOrDeleted(note, clients[i].get(), CS_RESET));
+    ASSERT_NO_FATAL_FAILURE(AssertWaitForStateOrDeleted(
+        note, clients[i].get(), &terminate_early, CS_RESET));
     // ResetDone requests Destroy().
-    ASSERT_NO_FATAL_FAILURE(
-        AssertWaitForStateOrDeleted(note, clients[i].get(), CS_DESTROYED));
+    ASSERT_NO_FATAL_FAILURE(AssertWaitForStateOrDeleted(
+        note, clients[i].get(), &terminate_early, CS_DESTROYED));
+    // Break the loop and terminate test gracefully if there is already
+    // something wrong.
+    if (terminate_early)
+      break;
   }
+  // There is already something wrong and should delete clients and then stop
+  // testing.
+  if (terminate_early) {
+    DeleteClientsAndNotes(std::move(clients), std::move(notes));

[Owen Lin, 2016/12/27 07:32:44]

To me, it looks like the main issue here is to call DeleteClientsAndNotes when
we exist the function.

In that case, can we have a helper object to do this?

class ClientAndNotesDeleter {
public:
  ClientAndNotesDeleter(clients, notes);
  ~ClientAndNotesDeleter() {
    DeleteClientsAndNotes();
  }
}

[johnylin1, 2016/12/30 12:38:37]

I got another idea is to delete clients and notes in the destructor of
VideoDecodeAcceleratorParamTest. Exiting the test also means
VideoDecodeAcceleratorParamTest gets released.

class VideoDecodeAcceleratorParamTest {
 public:
  ~VideoDecodeAcceleratorParamTest();
 protected:
  NotesVector notes;
  ClientsVector clients;
};

VideoDecodeAcceleratorParamTest::~VideoDecodeAcceleratorParamTest() {
  DeleteClientsAndNotes();
}

[johnylin1, 2017/01/04 15:34:36]

Override TearDown() is even better and also makes everything cleaned in the
correct order.

+  }
+  ASSERT_FALSE(terminate_early) << "Test is terminated due to failure above.";
+
   // 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();
     TestVideoFile* video_file =
@@ skipping 49 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);
     }
   }
 
-  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();
+  DeleteClientsAndNotes(std::move(clients), std::move(notes));
 };
 
 // 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)));
 }