Replaced the custom thread synchronization with rtc::Event

webrtc/modules/audio_processing/audio_processing_impl_locking_unittest.cc

 /*
  *  Copyright (c) 2015 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/audio_processing/audio_processing_impl.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "webrtc/base/array_view.h"
 #include "webrtc/base/criticalsection.h"
+#include "webrtc/base/event.h"
 #include "webrtc/base/platform_thread.h"
 #include "webrtc/config.h"
 #include "webrtc/modules/audio_processing/test/test_utils.h"
 #include "webrtc/modules/include/module_common_types.h"
-#include "webrtc/system_wrappers/include/event_wrapper.h"
 #include "webrtc/system_wrappers/include/sleep.h"
 #include "webrtc/test/random.h"
 
 namespace webrtc {
 
 namespace {
 
 class AudioProcessingImplLockTest;
 
 // Type of the render thread APM API call to use in the test.
@@ skipping 232 matching lines @@
   void IncreaseRenderCounter() {
     rtc::CritScope cs(&crit_);
     render_count++;
   }
 
   void IncreaseCaptureCounter() {
     rtc::CritScope cs(&crit_);
     capture_count++;
   }
 
-  int GetCaptureCounter() {
+  int GetCaptureCounter() const {
     rtc::CritScope cs(&crit_);
     return capture_count;
   }
 
-  int GetRenderCounter() {
+  int GetRenderCounter() const {
     rtc::CritScope cs(&crit_);
     return render_count;
   }
 
-  int CaptureMinusRenderCounters() {
+  int CaptureMinusRenderCounters() const {
     rtc::CritScope cs(&crit_);
     return capture_count - render_count;
   }
 
+  int RenderMinusCaptureCounters() const {
+    return -CaptureMinusRenderCounters();
+  }
+
   bool BothCountersExceedeThreshold(int threshold) {
     rtc::CritScope cs(&crit_);
     return (render_count > threshold && capture_count > threshold);
   }
 
  private:
-  rtc::CriticalSection crit_;
+  mutable rtc::CriticalSection crit_;
   int render_count GUARDED_BY(crit_) = 0;
   int capture_count GUARDED_BY(crit_) = 0;
 };
 
-// Checker for whether the capture side has been called.
-class CaptureSideCalledChecker {
- public:
-  bool CaptureSideCalled() {
-    rtc::CritScope cs(&crit_);
-    return capture_side_called_;
-  }
-
-  void FlagCaptureSideCalled() {
-    rtc::CritScope cs(&crit_);
-    capture_side_called_ = true;
-  }
-
- private:
-  rtc::CriticalSection crit_;
-  bool capture_side_called_ GUARDED_BY(crit_) = false;
-};
-
 // Class for handling the capture side processing.
 class CaptureProcessor {
  public:
   CaptureProcessor(int max_frame_size,
                    RandomGenerator* rand_gen,
+                   rtc::Event* render_call_event,
+                   rtc::Event* capture_call_event,
                    FrameCounters* shared_counters_state,
-                   CaptureSideCalledChecker* capture_call_checker,
                    AudioProcessingImplLockTest* test_framework,
                    TestConfig* test_config,
                    AudioProcessing* apm);
   bool Process();
 
  private:
   static const int kMaxCallDifference = 10;
   static const float kCaptureInputFloatLevel;
   static const int kCaptureInputFixLevel = 1024;
 
   void PrepareFrame();
   void CallApmCaptureSide();
   void ApplyRuntimeSettingScheme();
 
-  RandomGenerator* rand_gen_ = nullptr;
-  FrameCounters* frame_counters_ = nullptr;
-  CaptureSideCalledChecker* capture_call_checker_ = nullptr;
-  AudioProcessingImplLockTest* test_ = nullptr;
-  TestConfig* test_config_ = nullptr;
-  AudioProcessing* apm_ = nullptr;
+  RandomGenerator* const rand_gen_ = nullptr;
+  rtc::Event* const render_call_event_ = nullptr;
+  rtc::Event* const capture_call_event_ = nullptr;
+  FrameCounters* const frame_counters_ = nullptr;
+  AudioProcessingImplLockTest* const test_ = nullptr;
+  const TestConfig* const test_config_ = nullptr;
+  AudioProcessing* const apm_ = nullptr;
   AudioFrameData frame_data_;
 };
 
 // Class for handling the stats processing.
 class StatsProcessor {
  public:
   StatsProcessor(RandomGenerator* rand_gen,
                  TestConfig* test_config,
                  AudioProcessing* apm);
   bool Process();
 
  private:
   RandomGenerator* rand_gen_ = nullptr;
   TestConfig* test_config_ = nullptr;
   AudioProcessing* apm_ = nullptr;
 };
 
 // Class for handling the render side processing.
 class RenderProcessor {
  public:
   RenderProcessor(int max_frame_size,
                   RandomGenerator* rand_gen,
+                  rtc::Event* render_call_event,
+                  rtc::Event* capture_call_event,
                   FrameCounters* shared_counters_state,
-                  CaptureSideCalledChecker* capture_call_checker,
                   AudioProcessingImplLockTest* test_framework,
                   TestConfig* test_config,
                   AudioProcessing* apm);
   bool Process();
 
  private:
   static const int kMaxCallDifference = 10;
   static const int kRenderInputFixLevel = 16384;
   static const float kRenderInputFloatLevel;
 
   void PrepareFrame();
   void CallApmRenderSide();
   void ApplyRuntimeSettingScheme();
 
-  RandomGenerator* rand_gen_ = nullptr;
-  FrameCounters* frame_counters_ = nullptr;
-  CaptureSideCalledChecker* capture_call_checker_ = nullptr;
-  AudioProcessingImplLockTest* test_ = nullptr;
-  TestConfig* test_config_ = nullptr;
-  AudioProcessing* apm_ = nullptr;
-  bool first_render_side_call_ = true;
+  RandomGenerator* const rand_gen_ = nullptr;
+  rtc::Event* const render_call_event_ = nullptr;
+  rtc::Event* const capture_call_event_ = nullptr;
+  FrameCounters* const frame_counters_ = nullptr;
+  AudioProcessingImplLockTest* const test_ = nullptr;
+  const TestConfig* const test_config_ = nullptr;
+  AudioProcessing* const apm_ = nullptr;
   AudioFrameData frame_data_;
+  bool first_render_call_ = true;
 };
 
 class AudioProcessingImplLockTest
     : public ::testing::TestWithParam<TestConfig> {
  public:
   AudioProcessingImplLockTest();
-  EventTypeWrapper RunTest();
-  void CheckTestCompleteness();
+  bool RunTest();
+  bool MaybeEndTest();
 
  private:
   static const int kTestTimeOutLimit = 10 * 60 * 1000;
   static const int kMaxFrameSize = 480;
 
   // ::testing::TestWithParam<> implementation
   void SetUp() override;
   void TearDown() override;
 
   // Thread callback for the render thread
@@ skipping 24 matching lines @@
   // Start the threads used in the test.
   void StartThreads() {
     render_thread_.Start();
     render_thread_.SetPriority(rtc::kRealtimePriority);
     capture_thread_.Start();
     capture_thread_.SetPriority(rtc::kRealtimePriority);
     stats_thread_.Start();
     stats_thread_.SetPriority(rtc::kNormalPriority);
   }
 
-  // Event handler for the test.
-  const rtc::scoped_ptr<EventWrapper> test_complete_;
+  // Event handlers for the test.
+  rtc::Event test_complete_;
+  rtc::Event render_call_event_;
+  rtc::Event capture_call_event_;
 
   // Thread related variables.
   rtc::PlatformThread render_thread_;
   rtc::PlatformThread capture_thread_;
   rtc::PlatformThread stats_thread_;
   mutable RandomGenerator rand_gen_;

[pbos-webrtc, 2015/12/02 22:59:13]

This one looks shared between multiple instances, is it thread safe?

[peah-webrtc, 2015/12/03 08:17:17]

It is, there us a custom class RandomGenerator which adds a lock around a
test::Random random generator for this particular reason.

[terelius, 2015/12/09 17:10:38]

RandomGenerator comes from base/helpers.h which is not in rtc_base_approved,
though.

Basically it is a wrapper around an SSL (or Windows API) implementation of a
cryptographically secure RNG. Do you really need cryptographic security here?

[terelius, 2015/12/09 17:15:07]

Oops, sorry. You are right. Confused it with a similar generator in
base/helpers.h

[the sun, 2015/12/10 09:37:54]

Why share the PRNG? Why not just keep separate instances (with different seeds)
in the different helper classes?

 
   rtc::scoped_ptr<AudioProcessing> apm_;
   TestConfig test_config_;
   FrameCounters frame_counters_;
-  CaptureSideCalledChecker capture_call_checker_;
   RenderProcessor render_thread_state_;
   CaptureProcessor capture_thread_state_;
   StatsProcessor stats_thread_state_;
 };
 
 // Sleeps a random time between 0 and max_sleep milliseconds.
 void SleepRandomMs(int max_sleep, RandomGenerator* rand_gen) {
   int sleeptime = rand_gen->RandInt(0, max_sleep);
   SleepMs(sleeptime);
 }
@@ skipping 22 matching lines @@
     for (int k = 0; k < static_cast<int>(frame->samples_per_channel_); k++) {
       // Store random 16 bit number between -(amplitude+1) and
       // amplitude.
       frame->data_[k * ch] =
           rand_gen->RandInt(2 * amplitude + 1) - amplitude - 1;
     }
   }
 }
 
 AudioProcessingImplLockTest::AudioProcessingImplLockTest()
-    : test_complete_(EventWrapper::Create()),
+    : test_complete_(false, false),
+      render_call_event_(false, false),
+      capture_call_event_(false, false),
       render_thread_(RenderProcessorThreadFunc, this, "render"),
       capture_thread_(CaptureProcessorThreadFunc, this, "capture"),
       stats_thread_(StatsProcessorThreadFunc, this, "stats"),
       apm_(AudioProcessingImpl::Create()),
       render_thread_state_(kMaxFrameSize,
                            &rand_gen_,
+                           &render_call_event_,
+                           &capture_call_event_,
                            &frame_counters_,
-                           &capture_call_checker_,
                            this,
                            &test_config_,
                            apm_.get()),
       capture_thread_state_(kMaxFrameSize,
                             &rand_gen_,
+                            &render_call_event_,
+                            &capture_call_event_,
                             &frame_counters_,
-                            &capture_call_checker_,
                             this,
                             &test_config_,
                             apm_.get()),
       stats_thread_state_(&rand_gen_, &test_config_, apm_.get()) {}
 
 // Run the test with a timeout.
-EventTypeWrapper AudioProcessingImplLockTest::RunTest() {
+bool AudioProcessingImplLockTest::RunTest() {
   StartThreads();
-  return test_complete_->Wait(kTestTimeOutLimit);
+  return test_complete_.Wait(kTestTimeOutLimit);
 }
 
-void AudioProcessingImplLockTest::CheckTestCompleteness() {
+bool AudioProcessingImplLockTest::MaybeEndTest() {
   if (HasFatalFailure() || TestDone()) {
-    test_complete_->Set();
+    test_complete_.Set();
+    return true;
   }
+  return false;
 }
 
 // Setup of test and APM.
 void AudioProcessingImplLockTest::SetUp() {
   test_config_ = static_cast<TestConfig>(GetParam());
 
   ASSERT_EQ(apm_->kNoError, apm_->level_estimator()->Enable(true));
   ASSERT_EQ(apm_->kNoError, apm_->gain_control()->Enable(true));
 
   ASSERT_EQ(apm_->kNoError,
@@ skipping 24 matching lines @@
 
     config.Set<DelayAgnostic>(
         new DelayAgnostic(test_config_.aec_type ==
                           AecType::BasicWebRtcAecSettingsWithDelayAgnosticAec));
 
     apm_->SetExtraOptions(config);
   }
 }
 
 void AudioProcessingImplLockTest::TearDown() {
+  render_call_event_.Set();
+  capture_call_event_.Set();
   render_thread_.Stop();
   capture_thread_.Stop();
   stats_thread_.Stop();
 }
 
 StatsProcessor::StatsProcessor(RandomGenerator* rand_gen,
                                TestConfig* test_config,
                                AudioProcessing* apm)
     : rand_gen_(rand_gen), test_config_(test_config), apm_(apm) {}
 
@@ skipping 17 matching lines @@
 
   // The below return values are not testable.
   apm_->noise_suppression()->speech_probability();
   apm_->voice_detection()->is_enabled();
 
   return true;
 }
 
 const float CaptureProcessor::kCaptureInputFloatLevel = 0.03125f;
 
-CaptureProcessor::CaptureProcessor(
-    int max_frame_size,
-    RandomGenerator* rand_gen,
-    FrameCounters* shared_counters_state,
-    CaptureSideCalledChecker* capture_call_checker,
-    AudioProcessingImplLockTest* test_framework,
-    TestConfig* test_config,
-    AudioProcessing* apm)
+CaptureProcessor::CaptureProcessor(int max_frame_size,
+                                   RandomGenerator* rand_gen,
+                                   rtc::Event* render_call_event,
+                                   rtc::Event* capture_call_event,
+                                   FrameCounters* shared_counters_state,
+                                   AudioProcessingImplLockTest* test_framework,
+                                   TestConfig* test_config,
+                                   AudioProcessing* apm)
     : rand_gen_(rand_gen),
+      render_call_event_(render_call_event),
+      capture_call_event_(capture_call_event),
       frame_counters_(shared_counters_state),
-      capture_call_checker_(capture_call_checker),
       test_(test_framework),
       test_config_(test_config),
       apm_(apm),
       frame_data_(max_frame_size) {}
 
 // Implements the callback functionality for the capture thread.
 bool CaptureProcessor::Process() {
   // Sleep a random time to simulate thread jitter.
   SleepRandomMs(3, rand_gen_);
 
-  // End the test if complete.
-  test_->CheckTestCompleteness();
+  // Check whether the test is done.
+  if (test_->MaybeEndTest()) {
+    return false;
+  }
 
-  // Ensure that there are not more capture side calls than render side
-  // calls.
-  if (capture_call_checker_->CaptureSideCalled()) {
-    while (kMaxCallDifference < frame_counters_->CaptureMinusRenderCounters()) {
-      SleepMs(1);
-    }
+  // Ensure that the number of render and capture calls do not
+  // differ too much.
+  if (frame_counters_->CaptureMinusRenderCounters() > kMaxCallDifference) {
+    render_call_event_->Wait(rtc::Event::kForever);
   }
 
   // Apply any specified capture side APM non-processing runtime calls.
   ApplyRuntimeSettingScheme();
 
   // Apply the capture side processing call.
   CallApmCaptureSide();
 
   // Increase the number of capture-side calls.
   frame_counters_->IncreaseCaptureCounter();
 
-  // Flag that the capture side has been called at least once
-  // (needed to ensure that a capture call has been done
-  // before the first render call is performed (implicitly
-  // required by the APM API).
-  capture_call_checker_->FlagCaptureSideCalled();
+  // Flag to the render thread that another capture API call has occurred
+  // by triggering this threads call event.
+  capture_call_event_->Set();
 
   return true;
 }
 
 // Prepares a frame with relevant audio data and metadata.
 void CaptureProcessor::PrepareFrame() {
   // Restrict to a common fixed sample rate if the AudioFrame
   // interface is used.
   if (test_config_->capture_api_function ==
       CaptureApiImpl::ProcessStreamImpl1) {
@@ skipping 198 matching lines @@
   // the number of input channels.
   frame_data_.output_number_of_channels =
       std::min(frame_data_.output_number_of_channels,
                frame_data_.input_number_of_channels);
 }
 
 const float RenderProcessor::kRenderInputFloatLevel = 0.5f;
 
 RenderProcessor::RenderProcessor(int max_frame_size,
                                  RandomGenerator* rand_gen,
+                                 rtc::Event* render_call_event,
+                                 rtc::Event* capture_call_event,
                                  FrameCounters* shared_counters_state,
-                                 CaptureSideCalledChecker* capture_call_checker,
                                  AudioProcessingImplLockTest* test_framework,
                                  TestConfig* test_config,
                                  AudioProcessing* apm)
     : rand_gen_(rand_gen),
+      render_call_event_(render_call_event),
+      capture_call_event_(capture_call_event),
       frame_counters_(shared_counters_state),
-      capture_call_checker_(capture_call_checker),
       test_(test_framework),
       test_config_(test_config),
       apm_(apm),
       frame_data_(max_frame_size) {}
 
 // Implements the callback functionality for the render thread.
 bool RenderProcessor::Process() {
   // Conditional wait to ensure that a capture call has been done
   // before the first render call is performed (implicitly
   // required by the APM API).
-  if (first_render_side_call_) {
-    while (!capture_call_checker_->CaptureSideCalled()) {
-      SleepRandomMs(3, rand_gen_);
-    }
-
-    first_render_side_call_ = false;
+  if (first_render_call_) {
+    capture_call_event_->Wait(rtc::Event::kForever);
+    first_render_call_ = false;
   }
 
   // Sleep a random time to simulate thread jitter.
   SleepRandomMs(3, rand_gen_);
 
-  // End the test early if a fatal failure (ASSERT_*) has occurred.
-  test_->CheckTestCompleteness();
+  // Check whether the test is done.
+  if (test_->MaybeEndTest()) {

[pbos-webrtc, 2015/12/02 22:59:13]

Maybe rename MaybeEndTest to TestHasEnded?

[peah-webrtc, 2015/12/03 08:17:17]

That is a good name but I think it is a bit misleading. 
The problem is that the method has side-effects, and during a previous
discussion in another similar test CL we settled on the current name instead for
that particular reason. 

I'd be happy to go for another name, but then I think it should somehow reflect
both the end-query and the actions of ending the test that the method does. Last
time we thought the Maybe-name reflected that best.

I'll keep the name for now.

+    return false;
+  }
 
   // Ensure that the number of render and capture calls do not
   // differ too much.
-  while (kMaxCallDifference < -frame_counters_->CaptureMinusRenderCounters()) {
-    SleepMs(1);
+  if (frame_counters_->RenderMinusCaptureCounters() > kMaxCallDifference) {
+    capture_call_event_->Wait(rtc::Event::kForever);
   }
 
   // Apply any specified render side APM non-processing runtime calls.
   ApplyRuntimeSettingScheme();
 
   // Apply the render side processing call.
   CallApmRenderSide();
 
   // Increase the number of render-side calls.
   frame_counters_->IncreaseRenderCounter();
 
+  // Flag to the capture thread that another render API call has occurred
+  // by triggering this threads call event.
+  render_call_event_->Set();
   return true;
 }
 
 // Prepares the render side frame and the accompanying metadata
 // with the appropriate information.
 void RenderProcessor::PrepareFrame() {
   // Restrict to a common fixed sample rate if the AudioFrame interface is
   // used.
   if ((test_config_->render_api_function ==
        RenderApiImpl::AnalyzeReverseStreamImpl1) ||
@@ skipping 174 matching lines @@
   // the number of input channels.
   frame_data_.output_number_of_channels =
       std::min(frame_data_.output_number_of_channels,
                frame_data_.input_number_of_channels);
 }
 
 }  // anonymous namespace
 
 TEST_P(AudioProcessingImplLockTest, LockTest) {
   // Run test and verify that it did not time out.
-  ASSERT_EQ(kEventSignaled, RunTest());
+  ASSERT_TRUE(RunTest());
 }
 
 // Instantiate tests from the extreme test configuration set.
 INSTANTIATE_TEST_CASE_P(
     DISABLED_AudioProcessingImplLockExtensive,
     AudioProcessingImplLockTest,
     ::testing::ValuesIn(TestConfig::GenerateExtensiveTestConfigs()));
 
 INSTANTIATE_TEST_CASE_P(
     AudioProcessingImplLockBrief,
     AudioProcessingImplLockTest,
     ::testing::ValuesIn(TestConfig::GenerateBriefTestConfigs()));
 
 }  // namespace webrtc