Synchronize task queue operations in QualityScaler tests.

webrtc/modules/video_coding/utility/quality_scaler_unittest.cc

 /*
  *  Copyright (c) 2014 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/video_coding/utility/quality_scaler.h"
 
 #include <memory>
 
 #include "webrtc/base/event.h"
 #include "webrtc/base/task_queue.h"
 #include "webrtc/test/gmock.h"
 #include "webrtc/test/gtest.h"
 
 namespace webrtc {
 namespace {
 static const int kFramerate = 30;
 static const int kLowQp = 15;
 static const int kLowQpThreshold = 18;
 static const int kHighQp = 40;
 static const size_t kDefaultTimeoutMs = 150;
 }  // namespace
 
+#define DO_SYNC(q, block) do {    \
+  rtc::Event event(false, false); \
+  q->PostTask([this, &event] {    \
+    block;                        \
+    event.Set();                  \
+  });                             \
+  event.Wait(1000);               \

[sprang_webrtc, 2017/03/29 08:53:05]

RTC_CHECK this wait

+  } while (0)
+

[sprang_webrtc, 2017/03/26 12:24:47]

I don't think this needs to be macro. Add a helper function or struct to do this
instead if you feel there's too much code duplication.
Also this feels a little like an antipattern, now we really have no use for the
queue at all?

Also, why do we suspect this will help? I don't like sprinkling synchronizations
around for blind luck.

[kthelgason, 2017/03/27 06:55:04]

This can't be a function, at least with the current design. To make it a regular
function would mean wrapping the code currently passed as  `block` in a lambda.
It's less code and IMO less distracting from the point of the test to do this
with a macro.
I agree, doing this in production code would be ridiculous, but the issue with
these tests is that they have to run on Task Queue but that causes a data race.
My hypothesis for why these tests were flaky is that ScaleDown does not actually
execute on the task queue until the timeout expires. Waiting for it to execute
would eliminate that condition. These tests are only flaky on the trybots, where
resources are limited, and I suspect this is why. 

I may absolutely be wrong though, and I'd love to hear competing theories!

[sprang_webrtc, 2017/03/29 08:53:05]

I don't like it, but fair enough :)
Let's see if this helps. I don't have time to dig further into it right now.

+
 class MockAdaptationObserver : public AdaptationObserverInterface {
  public:
   MockAdaptationObserver() : event(false, false) {}
   virtual ~MockAdaptationObserver() {}
 
   void AdaptUp(AdaptReason r) override {
     adapt_up_events_++;
     event.Set();
   }
   void AdaptDown(AdaptReason r) override {
@@ skipping 19 matching lines @@
   enum ScaleDirection {
     kKeepScaleAtHighQp,
     kScaleDown,
     kScaleDownAboveHighQp,
     kScaleUp
   };
 
   QualityScalerTest()
       : q_(new rtc::TaskQueue("QualityScalerTestQueue")),
         observer_(new MockAdaptationObserver()) {
-    rtc::Event event(false, false);
-    q_->PostTask([this, &event] {
+    DO_SYNC(q_, {
       qs_ = std::unique_ptr<QualityScaler>(new QualityScalerUnderTest(
           observer_.get(),
-          VideoEncoder::QpThresholds(kLowQpThreshold, kHighQp)));
-      event.Set();
-    });
-    EXPECT_TRUE(event.Wait(kDefaultTimeoutMs));
+          VideoEncoder::QpThresholds(kLowQpThreshold, kHighQp)));});
   }
 
   ~QualityScalerTest() {
-    rtc::Event event(false, false);
-    q_->PostTask([this, &event] {
-      qs_.reset(nullptr);
-      event.Set();
-    });
-    EXPECT_TRUE(event.Wait(kDefaultTimeoutMs));
+    DO_SYNC(q_, {qs_.reset(nullptr);});
   }
 
   void TriggerScale(ScaleDirection scale_direction) {
     for (int i = 0; i < kFramerate * 5; ++i) {
       switch (scale_direction) {
         case kScaleUp:
           qs_->ReportQP(kLowQp);
           break;
         case kScaleDown:
           qs_->ReportDroppedFrame();
           break;
         case kKeepScaleAtHighQp:
           qs_->ReportQP(kHighQp);
           break;
         case kScaleDownAboveHighQp:
           qs_->ReportQP(kHighQp + 1);
           break;
       }
     }
   }
 
   std::unique_ptr<rtc::TaskQueue> q_;
   std::unique_ptr<QualityScaler> qs_;
   std::unique_ptr<MockAdaptationObserver> observer_;
 };
 
-#define DISABLED_TEST(basename, test) TEST_F(basename, DISABLED_##test)
-DISABLED_TEST(QualityScalerTest, DownscalesAfterContinuousFramedrop) {
-  q_->PostTask([this] { TriggerScale(kScaleDown); });
+TEST_F(QualityScalerTest, DownscalesAfterContinuousFramedrop) {
+  DO_SYNC(q_, { TriggerScale(kScaleDown); });
   EXPECT_TRUE(observer_->event.Wait(kDefaultTimeoutMs));
   EXPECT_EQ(1, observer_->adapt_down_events_);
 }
 
-DISABLED_TEST(QualityScalerTest, KeepsScaleAtHighQp) {
-  q_->PostTask([this] { TriggerScale(kKeepScaleAtHighQp); });
+TEST_F(QualityScalerTest, KeepsScaleAtHighQp) {
+  DO_SYNC(q_, { TriggerScale(kKeepScaleAtHighQp); });
   EXPECT_FALSE(observer_->event.Wait(kDefaultTimeoutMs));
   EXPECT_EQ(0, observer_->adapt_down_events_);
   EXPECT_EQ(0, observer_->adapt_up_events_);
 }
 
-DISABLED_TEST(QualityScalerTest, DownscalesAboveHighQp) {
-  q_->PostTask([this] { TriggerScale(kScaleDownAboveHighQp); });
+TEST_F(QualityScalerTest, DownscalesAboveHighQp) {
+  DO_SYNC(q_, { TriggerScale(kScaleDownAboveHighQp); });
   EXPECT_TRUE(observer_->event.Wait(kDefaultTimeoutMs));
   EXPECT_EQ(1, observer_->adapt_down_events_);
   EXPECT_EQ(0, observer_->adapt_up_events_);
 }
 
-DISABLED_TEST(QualityScalerTest, DownscalesAfterTwoThirdsFramedrop) {
-  q_->PostTask([this] {
+TEST_F(QualityScalerTest, DownscalesAfterTwoThirdsFramedrop) {
+  DO_SYNC(q_, {
     qs_->ReportDroppedFrame();
     qs_->ReportDroppedFrame();
     qs_->ReportQP(kHighQp);
   });
   EXPECT_TRUE(observer_->event.Wait(kDefaultTimeoutMs));
   EXPECT_EQ(1, observer_->adapt_down_events_);
   EXPECT_EQ(0, observer_->adapt_up_events_);
 }
 
-DISABLED_TEST(QualityScalerTest, DoesNotDownscaleOnNormalQp) {
-  q_->PostTask([this] { TriggerScale(kScaleDownAboveHighQp); });
+TEST_F(QualityScalerTest, DoesNotDownscaleOnNormalQp) {
+  DO_SYNC(q_, { TriggerScale(kScaleDownAboveHighQp); });
   EXPECT_TRUE(observer_->event.Wait(kDefaultTimeoutMs));
   EXPECT_EQ(1, observer_->adapt_down_events_);
   EXPECT_EQ(0, observer_->adapt_up_events_);
 }
 
-DISABLED_TEST(QualityScalerTest, DoesNotDownscaleAfterHalfFramedrop) {
-  q_->PostTask([this] {
+TEST_F(QualityScalerTest, DoesNotDownscaleAfterHalfFramedrop) {
+  DO_SYNC(q_, {
     qs_->ReportDroppedFrame();
     qs_->ReportQP(kHighQp);
   });
   EXPECT_FALSE(observer_->event.Wait(kDefaultTimeoutMs));
   EXPECT_EQ(0, observer_->adapt_down_events_);
   EXPECT_EQ(0, observer_->adapt_up_events_);
 }
 
-DISABLED_TEST(QualityScalerTest, UpscalesAfterLowQp) {
-  q_->PostTask([this] { TriggerScale(kScaleUp); });
+TEST_F(QualityScalerTest, UpscalesAfterLowQp) {
+  DO_SYNC(q_, { TriggerScale(kScaleUp); });
   EXPECT_TRUE(observer_->event.Wait(kDefaultTimeoutMs));
   EXPECT_EQ(0, observer_->adapt_down_events_);
   EXPECT_EQ(1, observer_->adapt_up_events_);
 }
 
-DISABLED_TEST(QualityScalerTest, ScalesDownAndBackUp) {
-  q_->PostTask([this] { TriggerScale(kScaleDown); });
+TEST_F(QualityScalerTest, ScalesDownAndBackUp) {
+  DO_SYNC(q_, { TriggerScale(kScaleDown); });
   EXPECT_TRUE(observer_->event.Wait(kDefaultTimeoutMs));
   EXPECT_EQ(1, observer_->adapt_down_events_);
   EXPECT_EQ(0, observer_->adapt_up_events_);
-  q_->PostTask([this] { TriggerScale(kScaleUp); });
+  DO_SYNC(q_, { TriggerScale(kScaleUp); });
   EXPECT_TRUE(observer_->event.Wait(kDefaultTimeoutMs));
   EXPECT_EQ(1, observer_->adapt_down_events_);
   EXPECT_EQ(1, observer_->adapt_up_events_);
 }
-#undef DISABLED_TEST
 }  // namespace webrtc
+#undef DO_SYNC