From ee8c6d327357ecd2e17edede8d15f6e3893409a8 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Thu, 13 Aug 2015 14:27:18 -0700 Subject: [PATCH] In PeerConnectionTestWrapper, put audio input on a separate thread. This will prevent it from blocking network input when it falls behind, which is happening when running with ThreadSanitizer. BUG=webrtc:4663 Review URL: https://codereview.webrtc.org/1236023010 Cr-Commit-Position: refs/heads/master@{#9707} --- talk/app/webrtc/peerconnection_unittest.cc | 3 +- .../webrtc/peerconnectionendtoend_unittest.cc | 13 +--- .../app/webrtc/test/fakeaudiocapturemodule.cc | 66 ++++++++----------- talk/app/webrtc/test/fakeaudiocapturemodule.h | 15 ++--- .../test/fakeaudiocapturemodule_unittest.cc | 18 +++-- .../webrtc/test/peerconnectiontestwrapper.cc | 3 +- .../webrtc/test/peerconnectiontestwrapper.h | 1 - 7 files changed, 49 insertions(+), 70 deletions(-) diff --git a/talk/app/webrtc/peerconnection_unittest.cc b/talk/app/webrtc/peerconnection_unittest.cc index 08e6cef1c6..c077fe003f 100644 --- a/talk/app/webrtc/peerconnection_unittest.cc +++ b/talk/app/webrtc/peerconnection_unittest.cc @@ -518,8 +518,7 @@ class PeerConnectionTestClientBase if (!allocator_factory_) { return false; } - fake_audio_capture_module_ = FakeAudioCaptureModule::Create( - rtc::Thread::Current()); + fake_audio_capture_module_ = FakeAudioCaptureModule::Create(); if (fake_audio_capture_module_ == NULL) { return false; diff --git a/talk/app/webrtc/peerconnectionendtoend_unittest.cc b/talk/app/webrtc/peerconnectionendtoend_unittest.cc index 7da7cc8139..7800a6724a 100644 --- a/talk/app/webrtc/peerconnectionendtoend_unittest.cc +++ b/talk/app/webrtc/peerconnectionendtoend_unittest.cc @@ -268,17 +268,7 @@ class PeerConnectionEndToEndTest DataChannelList callee_signaled_data_channels_; }; -// Disable for TSan v2, see -// https://code.google.com/p/webrtc/issues/detail?id=1205 for details. -#if !defined(THREAD_SANITIZER) - -// Flaky on Windows. Disabled per issue 4464. -#ifdef WEBRTC_WIN -#define MAYBE_Call DISABLED_Call -#else -#define MAYBE_Call Call -#endif -TEST_F(PeerConnectionEndToEndTest, MAYBE_Call) { +TEST_F(PeerConnectionEndToEndTest, Call) { CreatePcs(); GetAndAddUserMedia(); Negotiate(); @@ -427,4 +417,3 @@ TEST_F(PeerConnectionEndToEndTest, EXPECT_EQ(1U, dc_1_observer->received_message_count()); EXPECT_EQ(1U, dc_2_observer->received_message_count()); } -#endif // if !defined(THREAD_SANITIZER) diff --git a/talk/app/webrtc/test/fakeaudiocapturemodule.cc b/talk/app/webrtc/test/fakeaudiocapturemodule.cc index 47f17a1739..321e76ba09 100644 --- a/talk/app/webrtc/test/fakeaudiocapturemodule.cc +++ b/talk/app/webrtc/test/fakeaudiocapturemodule.cc @@ -54,13 +54,11 @@ static const uint32_t kMaxVolume = 14392; enum { MSG_START_PROCESS, MSG_RUN_PROCESS, - MSG_STOP_PROCESS, }; -FakeAudioCaptureModule::FakeAudioCaptureModule( - rtc::Thread* process_thread) +FakeAudioCaptureModule::FakeAudioCaptureModule() : last_process_time_ms_(0), - audio_callback_(NULL), + audio_callback_(nullptr), recording_(false), playing_(false), play_is_initialized_(false), @@ -68,23 +66,20 @@ FakeAudioCaptureModule::FakeAudioCaptureModule( current_mic_level_(kMaxVolume), started_(false), next_frame_time_(0), - process_thread_(process_thread), frames_received_(0) { } FakeAudioCaptureModule::~FakeAudioCaptureModule() { - // Ensure that thread stops calling ProcessFrame(). - process_thread_->Send(this, MSG_STOP_PROCESS); + if (process_thread_) { + process_thread_->Stop(); + } } -rtc::scoped_refptr FakeAudioCaptureModule::Create( - rtc::Thread* process_thread) { - if (process_thread == NULL) return NULL; - +rtc::scoped_refptr FakeAudioCaptureModule::Create() { rtc::scoped_refptr capture_module( - new rtc::RefCountedObject(process_thread)); + new rtc::RefCountedObject()); if (!capture_module->Initialize()) { - return NULL; + return nullptr; } return capture_module; } @@ -601,9 +596,6 @@ void FakeAudioCaptureModule::OnMessage(rtc::Message* msg) { case MSG_RUN_PROCESS: ProcessFrameP(); break; - case MSG_STOP_PROCESS: - StopProcessP(); - break; default: // All existing messages should be caught. Getting here should never // happen. @@ -650,14 +642,22 @@ bool FakeAudioCaptureModule::ShouldStartProcessing() { void FakeAudioCaptureModule::UpdateProcessing(bool start) { if (start) { + if (!process_thread_) { + process_thread_.reset(new rtc::Thread()); + process_thread_->Start(); + } process_thread_->Post(this, MSG_START_PROCESS); } else { - process_thread_->Send(this, MSG_STOP_PROCESS); + if (process_thread_) { + process_thread_->Stop(); + process_thread_.reset(nullptr); + } + started_ = false; } } void FakeAudioCaptureModule::StartProcessP() { - ASSERT(rtc::Thread::Current() == process_thread_); + ASSERT(process_thread_->IsCurrent()); if (started_) { // Already started. return; @@ -666,26 +666,21 @@ void FakeAudioCaptureModule::StartProcessP() { } void FakeAudioCaptureModule::ProcessFrameP() { - ASSERT(rtc::Thread::Current() == process_thread_); + ASSERT(process_thread_->IsCurrent()); if (!started_) { next_frame_time_ = rtc::Time(); started_ = true; } - bool playing; - bool recording; { rtc::CritScope cs(&crit_); - playing = playing_; - recording = recording_; - } - - // Receive and send frames every kTimePerFrameMs. - if (playing) { - ReceiveFrameP(); - } - if (recording) { - SendFrameP(); + // Receive and send frames every kTimePerFrameMs. + if (playing_) { + ReceiveFrameP(); + } + if (recording_) { + SendFrameP(); + } } next_frame_time_ += kTimePerFrameMs; @@ -696,7 +691,7 @@ void FakeAudioCaptureModule::ProcessFrameP() { } void FakeAudioCaptureModule::ReceiveFrameP() { - ASSERT(rtc::Thread::Current() == process_thread_); + ASSERT(process_thread_->IsCurrent()); { rtc::CritScope cs(&crit_callback_); if (!audio_callback_) { @@ -727,7 +722,7 @@ void FakeAudioCaptureModule::ReceiveFrameP() { } void FakeAudioCaptureModule::SendFrameP() { - ASSERT(rtc::Thread::Current() == process_thread_); + ASSERT(process_thread_->IsCurrent()); rtc::CritScope cs(&crit_callback_); if (!audio_callback_) { return; @@ -747,8 +742,3 @@ void FakeAudioCaptureModule::SendFrameP() { SetMicrophoneVolume(current_mic_level); } -void FakeAudioCaptureModule::StopProcessP() { - ASSERT(rtc::Thread::Current() == process_thread_); - started_ = false; - process_thread_->Clear(this); -} diff --git a/talk/app/webrtc/test/fakeaudiocapturemodule.h b/talk/app/webrtc/test/fakeaudiocapturemodule.h index 8ff4aa19e7..be1df9a7bd 100644 --- a/talk/app/webrtc/test/fakeaudiocapturemodule.h +++ b/talk/app/webrtc/test/fakeaudiocapturemodule.h @@ -40,14 +40,13 @@ #include "webrtc/base/basictypes.h" #include "webrtc/base/criticalsection.h" #include "webrtc/base/messagehandler.h" +#include "webrtc/base/scoped_ptr.h" #include "webrtc/base/scoped_ref_ptr.h" #include "webrtc/common_types.h" #include "webrtc/modules/audio_device/include/audio_device.h" namespace rtc { - class Thread; - } // namespace rtc class FakeAudioCaptureModule @@ -62,10 +61,7 @@ class FakeAudioCaptureModule static const int kNumberBytesPerSample = sizeof(Sample); // Creates a FakeAudioCaptureModule or returns NULL on failure. - // |process_thread| is used to push and pull audio frames to and from the - // returned instance. Note: ownership of |process_thread| is not handed over. - static rtc::scoped_refptr Create( - rtc::Thread* process_thread); + static rtc::scoped_refptr Create(); // Returns the number of frames that have been successfully pulled by the // instance. Note that correctly detecting success can only be done if the @@ -206,7 +202,7 @@ class FakeAudioCaptureModule // exposed in which case the burden of proper instantiation would be put on // the creator of a FakeAudioCaptureModule instance. To create an instance of // this class use the Create(..) API. - explicit FakeAudioCaptureModule(rtc::Thread* process_thread); + explicit FakeAudioCaptureModule(); // The destructor is protected because it is reference counted and should not // be deleted directly. virtual ~FakeAudioCaptureModule(); @@ -239,8 +235,6 @@ class FakeAudioCaptureModule void ReceiveFrameP(); // Pushes frames to the registered webrtc::AudioTransport. void SendFrameP(); - // Stops the periodic calling of ProcessFrame() in a thread safe way. - void StopProcessP(); // The time in milliseconds when Process() was last called or 0 if no call // has been made. @@ -266,8 +260,7 @@ class FakeAudioCaptureModule bool started_; uint32 next_frame_time_; - // User provided thread context. - rtc::Thread* process_thread_; + rtc::scoped_ptr process_thread_; // Buffer for storing samples received from the webrtc::AudioTransport. char rec_buffer_[kNumberSamples * kNumberBytesPerSample]; diff --git a/talk/app/webrtc/test/fakeaudiocapturemodule_unittest.cc b/talk/app/webrtc/test/fakeaudiocapturemodule_unittest.cc index 56f1d070ed..fcfdf7e754 100644 --- a/talk/app/webrtc/test/fakeaudiocapturemodule_unittest.cc +++ b/talk/app/webrtc/test/fakeaudiocapturemodule_unittest.cc @@ -29,6 +29,7 @@ #include +#include "webrtc/base/criticalsection.h" #include "webrtc/base/gunit.h" #include "webrtc/base/scoped_ref_ptr.h" #include "webrtc/base/thread.h" @@ -48,8 +49,7 @@ class FakeAdmTest : public testing::Test, } virtual void SetUp() { - fake_audio_capture_module_ = FakeAudioCaptureModule::Create( - rtc::Thread::Current()); + fake_audio_capture_module_ = FakeAudioCaptureModule::Create(); EXPECT_TRUE(fake_audio_capture_module_.get() != NULL); } @@ -65,6 +65,7 @@ class FakeAdmTest : public testing::Test, const uint32_t currentMicLevel, const bool keyPressed, uint32_t& newMicLevel) override { + rtc::CritScope cs(&crit_); rec_buffer_bytes_ = nSamples * nBytesPerSample; if ((rec_buffer_bytes_ == 0) || (rec_buffer_bytes_ > FakeAudioCaptureModule::kNumberSamples * @@ -87,6 +88,7 @@ class FakeAdmTest : public testing::Test, uint32_t& nSamplesOut, int64_t* elapsed_time_ms, int64_t* ntp_time_ms) override { + rtc::CritScope cs(&crit_); ++pull_iterations_; const uint32_t audio_buffer_size = nSamples * nBytesPerSample; const uint32_t bytes_out = RecordedDataReceived() ? @@ -98,8 +100,14 @@ class FakeAdmTest : public testing::Test, return 0; } - int push_iterations() const { return push_iterations_; } - int pull_iterations() const { return pull_iterations_; } + int push_iterations() const { + rtc::CritScope cs(&crit_); + return push_iterations_; + } + int pull_iterations() const { + rtc::CritScope cs(&crit_); + return pull_iterations_; + } rtc::scoped_refptr fake_audio_capture_module_; @@ -118,6 +126,8 @@ class FakeAdmTest : public testing::Test, return min_buffer_size; } + mutable rtc::CriticalSection crit_; + int push_iterations_; int pull_iterations_; diff --git a/talk/app/webrtc/test/peerconnectiontestwrapper.cc b/talk/app/webrtc/test/peerconnectiontestwrapper.cc index aecdcbb52f..2eb24d9700 100644 --- a/talk/app/webrtc/test/peerconnectiontestwrapper.cc +++ b/talk/app/webrtc/test/peerconnectiontestwrapper.cc @@ -75,8 +75,7 @@ bool PeerConnectionTestWrapper::CreatePc( return false; } - fake_audio_capture_module_ = FakeAudioCaptureModule::Create( - rtc::Thread::Current()); + fake_audio_capture_module_ = FakeAudioCaptureModule::Create(); if (fake_audio_capture_module_ == NULL) { return false; } diff --git a/talk/app/webrtc/test/peerconnectiontestwrapper.h b/talk/app/webrtc/test/peerconnectiontestwrapper.h index 21ee7dc16f..b65426326f 100644 --- a/talk/app/webrtc/test/peerconnectiontestwrapper.h +++ b/talk/app/webrtc/test/peerconnectiontestwrapper.h @@ -33,7 +33,6 @@ #include "talk/app/webrtc/test/fakeconstraints.h" #include "talk/app/webrtc/test/fakevideotrackrenderer.h" #include "webrtc/base/sigslot.h" -#include "webrtc/base/thread.h" namespace webrtc { class DtlsIdentityStoreInterface;