media: Fix a flaky AVDACodecAllocator test

media/gpu/avda_codec_allocator_unittest.cc

 // Copyright 2016 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.
 
 #include "media/gpu/avda_codec_allocator.h"
 
 #include <stdint.h>
 
 #include <memory>
 
 #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"
 
 using testing::Invoke;
 using testing::NiceMock;
 using testing::_;
 
 namespace media {
 namespace {
@@ skipping 11 matching lines @@
   if (about_to_wait_event)
     about_to_wait_event->Signal();
   wait_event->Wait();
 }
 
 void SignalImmediately(base::WaitableEvent* event) {
   event->Signal();
 }
 }
 
-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));
   MOCK_METHOD0(OnSurfaceDestroyed, void());
 
   // Gmock doesn't let us mock methods taking move-only types.
   MOCK_METHOD1(OnCodecConfiguredMock, void(VideoCodecBridge* media_codec));
   void OnCodecConfigured(
       std::unique_ptr<VideoCodecBridge> media_codec) override {
     OnCodecConfiguredMock(media_codec.get());
   }
 };
 
 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:
   void SetUp() override {
     // Start the main thread for the allocator.  This would normally be the GPU
     // 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 {
     // Don't leave any threads hung, or this will hang too.
     // It would be nice if we could let a unique ptr handle this, but the
     // destructor is private.  We also have to destroy it on the right thread.
     PostAndWait(FROM_HERE, base::Bind(
                                [](AVDACodecAllocator* allocator) {
                                  delete allocator;
                                  return true;
@@ skipping 21 matching lines @@
     // stop has completed.  It's async with respect to the allocator thread.
     PostAndWait(FROM_HERE, base::Bind(
                                [](AVDACodecAllocator* allocator,
                                   AVDACodecAllocatorClient* avda) {
                                  allocator->StopThread(avda);
                                  return true;
                                },
                                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.
   bool IsThreadRunning(TaskType task_type) {
     return PostAndWait(
         FROM_HERE,
         base::Bind(
             [](AVDACodecAllocator* allocator, TaskType task_type) {
               return allocator->GetThreadForTesting(task_type).IsRunning();
             },
@@ skipping 24 matching lines @@
     ReturnType return_value = ReturnType();
     allocator_thread_.task_runner()->PostTask(
         from_here,
         base::Bind(&RunAndSignalTask<ReturnType>, &event, &return_value, cb));
     event.Wait();
     return return_value;
   }
 
   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
   // should only be using one of the two. They are not unique_ptrs because the
   // destructor is private and they need to be destructed on the right thread.
   AVDACodecAllocator* allocator_ = nullptr;
   AVDACodecAllocator* allocator2_ = nullptr;
 
   NiceMock<MockClient> client1_, client2_, client3_;
   NiceMock<MockClient>* avda1_ = &client1_;
   NiceMock<MockClient>* avda2_ = &client2_;
   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.
   base::WaitableEvent about_to_wait_event(
       base::WaitableEvent::ResetPolicy::MANUAL,
       base::WaitableEvent::InitialState::NOT_SIGNALED);
   base::WaitableEvent wait_event(
       base::WaitableEvent::ResetPolicy::MANUAL,
       base::WaitableEvent::InitialState::NOT_SIGNALED);
   TaskRunnerFor(TaskType::AUTO_CODEC)
       ->PostTask(FROM_HERE, base::Bind(&WaitUntilRestarted,
                                        &about_to_wait_event, &wait_event));
   // Wait until the task starts, so that |allocator_| starts the hang timer.
   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.
   ASSERT_EQ(TaskType::SW_CODEC, TaskTypeForAllocation());
 
   // Un-hang the thread and wait for it to let another task run.  This will
   // notify |allocator_| that the thread is no longer hung.
   base::WaitableEvent done_waiting_event(
       base::WaitableEvent::ResetPolicy::MANUAL,
       base::WaitableEvent::InitialState::NOT_SIGNALED);
@@ skipping 68 matching lines @@
 
 TEST_F(AVDACodecAllocatorTest, DeallocatingIsSafeDuringSurfaceDestroyed) {
   allocator2_->AllocateSurface(avda1_, 1);
   EXPECT_CALL(*avda1_, OnSurfaceDestroyed()).WillOnce(Invoke([=]() {
     allocator2_->DeallocateSurface(avda1_, 1);
   }));
   allocator2_->OnSurfaceDestroyed(1);
 }
 
 }  // namespace media