media: Fix a flaky AVDACodecAllocator test

media/gpu/avda_codec_allocator_unittest.cc

Index: media/gpu/avda_codec_allocator_unittest.cc
diff --git a/media/gpu/avda_codec_allocator_unittest.cc b/media/gpu/avda_codec_allocator_unittest.cc
index 03c8284f736cbcc222a0bcaef9249644b0e45ecb..2044c263bee42583a71ef6841d58909808f75e92 100644
--- a/media/gpu/avda_codec_allocator_unittest.cc
+++ b/media/gpu/avda_codec_allocator_unittest.cc
@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/test/simple_test_tick_clock.h"
 #include "base/time/tick_clock.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -41,29 +42,6 @@ void SignalImmediately(base::WaitableEvent* event) {
 }
 }
 
-class MockTickClock : public base::TickClock {
- public:
-  MockTickClock() {
-    // Don't start with the null time.
-    Advance(1000);
-  }
-  ~MockTickClock() override{};
-  base::TimeTicks NowTicks() override {
-    base::AutoLock auto_lock(lock_);
-    return now_;
-  }
-
-  // Handy utility.
-  void Advance(int msec) {
-    base::AutoLock auto_lock(lock_);
-    now_ += base::TimeDelta::FromMilliseconds(msec);
-  }
-
- private:
-  base::Lock lock_;
-  base::TimeTicks now_;
-};
-
 class MockClient : public AVDACodecAllocatorClient {
  public:
   MOCK_METHOD1(OnSurfaceAvailable, void(bool success));
@@ -79,7 +57,14 @@ class MockClient : public AVDACodecAllocatorClient {
 
 class AVDACodecAllocatorTest : public testing::Test {
  public:
-  AVDACodecAllocatorTest() : allocator_thread_("AllocatorThread") {}
+  AVDACodecAllocatorTest()
+      : allocator_thread_("AllocatorThread"),
+        stop_event_(base::WaitableEvent::ResetPolicy::AUTOMATIC,
+                    base::WaitableEvent::InitialState::NOT_SIGNALED) {
+    // Don't start the clock at null.
+    tick_clock_.Advance(base::TimeDelta::FromSeconds(1));
+  }
+
   ~AVDACodecAllocatorTest() override {}
 
  protected:
@@ -88,34 +73,14 @@ class AVDACodecAllocatorTest : public testing::Test {
     // main thread.
     ASSERT_TRUE(allocator_thread_.Start());
 
-    // AVDACodecAllocator likes to post tasks to the current thread.
-
-    test_information_.reset(new AVDACodecAllocator::TestInformation());
-    test_information_->tick_clock_.reset(new MockTickClock());
-    test_information_->stop_event_.reset(new base::WaitableEvent(
-        base::WaitableEvent::ResetPolicy::AUTOMATIC,
-        base::WaitableEvent::InitialState::NOT_SIGNALED));
-
-    // Allocate the allocator on the appropriate thread.
+    // Create the first allocator on the allocator thread.
     allocator_ = PostAndWait(
         FROM_HERE, base::Bind(
-                       [](AVDACodecAllocator::TestInformation* test_info) {
-                         return new AVDACodecAllocator(test_info);
+                       [](base::TickClock* clock, base::WaitableEvent* event) {
+                         return new AVDACodecAllocator(clock, event);
                        },
-                       test_information_.get()));
-    allocator2_ = new AVDACodecAllocator(test_information_.get());
-
-    // All threads should be stopped
-    ASSERT_FALSE(IsThreadRunning(TaskType::AUTO_CODEC));
-    ASSERT_FALSE(IsThreadRunning(TaskType::SW_CODEC));
-
-    // Register an AVDA instance to start the allocator's threads.
-    ASSERT_TRUE(StartThread(avda1_));
-
-    // Assert that at least the AUTO_CODEC thread is started.  The other might
-    // not be.
-    ASSERT_TRUE(IsThreadRunning(TaskType::AUTO_CODEC));
-    ASSERT_EQ(TaskType::AUTO_CODEC, TaskTypeForAllocation());
+                       &tick_clock_, &stop_event_));
+    allocator2_ = new AVDACodecAllocator();
   }
 
   void TearDown() override {
@@ -157,7 +122,7 @@ class AVDACodecAllocatorTest : public testing::Test {
                                allocator_, avda));
     // Note that we don't do this on the allocator thread, since that's the
     // thread that will signal it.
-    test_information_->stop_event_->Wait();
+    stop_event_.Wait();
   }
 
   // Return the running state of |task_type|, doing the necessary thread hops.
@@ -202,8 +167,9 @@ class AVDACodecAllocatorTest : public testing::Test {
 
   base::Thread allocator_thread_;
 
-  // Test info that we provide to the codec allocator.
-  std::unique_ptr<AVDACodecAllocator::TestInformation> test_information_;
+  // The test params for |allocator_|.
+  base::SimpleTestTickClock tick_clock_;
+  base::WaitableEvent stop_event_;
 
   // Allocators that we own. The first is intialized to be used on the allocator
   // thread and the second one is initialized on the test thread. Each test
@@ -218,29 +184,27 @@ class AVDACodecAllocatorTest : public testing::Test {
   NiceMock<MockClient>* avda3_ = &client3_;
 };
 
-TEST_F(AVDACodecAllocatorTest, TestMultiInstance) {
-  // Add an avda instance.  This one must succeed immediately, since the last
-  // one is still running.
-  ASSERT_TRUE(StartThread(avda2_));
+TEST_F(AVDACodecAllocatorTest, ThreadsStartWhenClientsStart) {
+  ASSERT_FALSE(IsThreadRunning(TaskType::AUTO_CODEC));
+  ASSERT_FALSE(IsThreadRunning(TaskType::SW_CODEC));
+  ASSERT_TRUE(StartThread(avda1_));
+  // Assert that at least the AUTO_CODEC thread is started. The other might
+  // not be.
+  ASSERT_TRUE(IsThreadRunning(TaskType::AUTO_CODEC));
+}
 
-  // Stop the original avda instance.
+TEST_F(AVDACodecAllocatorTest, ThreadsStopAfterAllClientsStop) {
+  StartThread(avda1_);
+  StartThread(avda2_);
   StopThread(avda1_);
-
-  // Verify that the AUTO_CODEC thread is still running.
   ASSERT_TRUE(IsThreadRunning(TaskType::AUTO_CODEC));
-  ASSERT_EQ(TaskType::AUTO_CODEC, TaskTypeForAllocation());
-
-  // Remove the second instance and wait for it to stop.  Remember that it
-  // stops after messages have been posted to the thread, so we don't know
-  // how long it will take.
   StopThread(avda2_);
-
-  // Verify that the threads have stopped.
   ASSERT_FALSE(IsThreadRunning(TaskType::AUTO_CODEC));
   ASSERT_FALSE(IsThreadRunning(TaskType::SW_CODEC));

[liberato (no reviews please), 2016/11/30 06:47:30]

i'm not sure why this thread must be stopped by now.  only auto_codec signals
the event; the sw codec stop task might still be pending on the stop thread, i
think.  might be the cause of the flakiness.

the caveat is it's late.  i just happened to notice this CL when i logged in to
write down some thoughts i had this evening on the avda refactor (i didn't want
to forget them by morning; turns out that i broke threading for sync surface
destruction, oops!)  so, if this comment seems totally wrong, then please just
ignore it and i'll do an actual review tomorrow in which i, you know, actually
read your code before commenting. :)

[watk, 2016/11/30 20:48:42]

Oh genius! I think you've got it.

 }
 
 TEST_F(AVDACodecAllocatorTest, TestHangThread) {
+  StartThread(avda1_);
   ASSERT_EQ(TaskType::AUTO_CODEC, TaskTypeForAllocation());
 
   // Hang the AUTO_CODEC thread.
@@ -257,8 +221,7 @@ TEST_F(AVDACodecAllocatorTest, TestHangThread) {
   about_to_wait_event.Wait();
 
   // Verify that we've failed over after a long time has passed.
-  static_cast<MockTickClock*>(test_information_->tick_clock_.get())
-      ->Advance(1000);
+  tick_clock_.Advance(base::TimeDelta::FromSeconds(1));
   // Note that this should return the SW codec task type even if that thread
   // failed to start.  TaskRunnerFor() will return the current thread in that
   // case too.