Synchronize task queue operations in QualityScaler tests.

webrtc/modules/video_coding/utility/quality_scaler_unittest.cc

Index: webrtc/modules/video_coding/utility/quality_scaler_unittest.cc
diff --git a/webrtc/modules/video_coding/utility/quality_scaler_unittest.cc b/webrtc/modules/video_coding/utility/quality_scaler_unittest.cc
index ff21e3dc2bffc261b87abfcfc745bab34edb42e5..c966570390f06b354bf49c4e611e274cd2f89c9b 100644
--- a/webrtc/modules/video_coding/utility/quality_scaler_unittest.cc
+++ b/webrtc/modules/video_coding/utility/quality_scaler_unittest.cc
@@ -26,6 +26,16 @@ 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) {}
@@ -65,23 +75,14 @@ class QualityScalerTest : public ::testing::Test {
   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) {
@@ -108,29 +109,28 @@ class QualityScalerTest : public ::testing::Test {
   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);
@@ -140,15 +140,15 @@ DISABLED_TEST(QualityScalerTest, DownscalesAfterTwoThirdsFramedrop) {
   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);
   });
@@ -157,22 +157,22 @@ DISABLED_TEST(QualityScalerTest, DoesNotDownscaleAfterHalfFramedrop) {
   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