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..9ea874a5f016f07cf96018bb1fc34adb1efc1fe3 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(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
@@ -1280,20 +1280,53 @@ 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>>;
+
+  // 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.
@@ -1324,9 +1357,6 @@ 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);
 
@@ -1380,42 +1410,60 @@ TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
   // 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;
@@ -1485,9 +1533,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";
   }
 
@@ -1503,19 +1551,7 @@ TEST_P(VideoDecodeAcceleratorParamTest, TestSimpleDecode) {
     }
   }
 
-  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.