Skip to content

Commit

Permalink
Reland "Refactor the PlatformThread API."
Browse files Browse the repository at this point in the history 
This reverts commit 793bac5.

Reason for revert: rare compilation error fixed

Original change's description:
> Revert "Refactor the PlatformThread API."
>
> This reverts commit c89fdd7.
>
> Reason for revert: Causes rare compilation error on win-libfuzzer-asan trybot.
> See https://ci.chromium.org/p/chromium/builders/try/win-libfuzzer-asan-rel/713745?
>
> Original change's description:
> > Refactor the PlatformThread API.
> >
> > PlatformThread's API is using old style function pointers, causes
> > casting, is unintuitive and forces artificial call sequences, and
> > is additionally possible to misuse in release mode.
> >
> > Fix this by an API face lift:
> > 1. The class is turned into a handle, which can be empty.
> > 2. The only way of getting a non-empty PlatformThread is by calling
> > SpawnJoinable or SpawnDetached, clearly conveying the semantics to the
> > code reader.
> > 3. Handles can be Finalized, which works differently for joinable and
> > detached threads:
> >   a) Handles for detached threads are simply closed where applicable.
> >   b) Joinable threads are joined before handles are closed.
> > 4. The destructor finalizes handles. No explicit call is needed.
> >
> > Fixed: webrtc:12727
> > Change-Id: Id00a0464edf4fc9e552b6a1fbb5d2e1280e88811
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215075
> > Commit-Queue: Markus Handell <[email protected]>
> > Reviewed-by: Harald Alvestrand <[email protected]>
> > Reviewed-by: Mirko Bonadei <[email protected]>
> > Reviewed-by: Tommi <[email protected]>
> > Cr-Commit-Position: refs/heads/master@{#33923}
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> [email protected]
>
> Bug: webrtc:12727
> Change-Id: Ic0146be8866f6dd3ad9c364fb8646650b8e07419
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217583
> Reviewed-by: Guido Urdaneta <[email protected]>
> Reviewed-by: Markus Handell <[email protected]>
> Commit-Queue: Guido Urdaneta <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#33936}

# Not skipping CQ checks because this is a reland.

Bug: webrtc:12727
Change-Id: Ifd6f44eac72fed84474277a1be03eb84d2f4376e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217881
Commit-Queue: Mirko Bonadei <[email protected]>
Reviewed-by: Mirko Bonadei <[email protected]>
Reviewed-by: Markus Handell <[email protected]>
Reviewed-by: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/master@{#33950}
  • Loading branch information
Markus Handell authored and WebRTC LUCI CQ committed May 7, 2021
1 parent f95536d commit ad5037b
Show file tree
Hide file tree
Showing 38 changed files with 589 additions and 866 deletions.
23 changes: 8 additions & 15 deletions api/sequence_checker_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,14 @@ class CompileTimeTestForGuardedBy {
};

void RunOnDifferentThread(rtc::FunctionView<void()> run) {
struct Object {
static void Run(void* obj) {
auto* me = static_cast<Object*>(obj);
me->run();
me->thread_has_run_event.Set();
}

rtc::FunctionView<void()> run;
rtc::Event thread_has_run_event;
} object{run};

rtc::PlatformThread thread(&Object::Run, &object, "thread");
thread.Start();
EXPECT_TRUE(object.thread_has_run_event.Wait(1000));
thread.Stop();
rtc::Event thread_has_run_event;
rtc::PlatformThread::SpawnJoinable(
[&] {
run();
thread_has_run_event.Set();
},
"thread");
EXPECT_TRUE(thread_has_run_event.Wait(1000));
}

} // namespace
Expand Down
118 changes: 44 additions & 74 deletions modules/audio_coding/acm2/audio_coding_module_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -429,21 +429,6 @@ class AudioCodingModuleMtTestOldApi : public AudioCodingModuleTestOldApi {

AudioCodingModuleMtTestOldApi()
: AudioCodingModuleTestOldApi(),
send_thread_(
CbSendThread,
this,
"send",
rtc::ThreadAttributes().SetPriority(rtc::kRealtimePriority)),
insert_packet_thread_(
CbInsertPacketThread,
this,
"insert_packet",
rtc::ThreadAttributes().SetPriority(rtc::kRealtimePriority)),
pull_audio_thread_(
CbPullAudioThread,
this,
"pull_audio",
rtc::ThreadAttributes().SetPriority(rtc::kRealtimePriority)),
send_count_(0),
insert_packet_count_(0),
pull_audio_count_(0),
Expand All @@ -460,17 +445,38 @@ class AudioCodingModuleMtTestOldApi : public AudioCodingModuleTestOldApi {

void StartThreads() {
quit_.store(false);
send_thread_.Start();
insert_packet_thread_.Start();
pull_audio_thread_.Start();

const auto attributes =
rtc::ThreadAttributes().SetPriority(rtc::ThreadPriority::kRealtime);
send_thread_ = rtc::PlatformThread::SpawnJoinable(
[this] {
while (!quit_.load()) {
CbSendImpl();
}
},
"send", attributes);
insert_packet_thread_ = rtc::PlatformThread::SpawnJoinable(
[this] {
while (!quit_.load()) {
CbInsertPacketImpl();
}
},
"insert_packet", attributes);
pull_audio_thread_ = rtc::PlatformThread::SpawnJoinable(
[this] {
while (!quit_.load()) {
CbPullAudioImpl();
}
},
"pull_audio", attributes);
}

void TearDown() {
AudioCodingModuleTestOldApi::TearDown();
quit_.store(true);
pull_audio_thread_.Stop();
send_thread_.Stop();
insert_packet_thread_.Stop();
pull_audio_thread_.Finalize();
send_thread_.Finalize();
insert_packet_thread_.Finalize();
}

bool RunTest() {
Expand All @@ -488,14 +494,6 @@ class AudioCodingModuleMtTestOldApi : public AudioCodingModuleTestOldApi {
return false;
}

static void CbSendThread(void* context) {
AudioCodingModuleMtTestOldApi* fixture =
reinterpret_cast<AudioCodingModuleMtTestOldApi*>(context);
while (!fixture->quit_.load()) {
fixture->CbSendImpl();
}
}

// The send thread doesn't have to care about the current simulated time,
// since only the AcmReceiver is using the clock.
void CbSendImpl() {
Expand All @@ -511,14 +509,6 @@ class AudioCodingModuleMtTestOldApi : public AudioCodingModuleTestOldApi {
}
}

static void CbInsertPacketThread(void* context) {
AudioCodingModuleMtTestOldApi* fixture =
reinterpret_cast<AudioCodingModuleMtTestOldApi*>(context);
while (!fixture->quit_.load()) {
fixture->CbInsertPacketImpl();
}
}

void CbInsertPacketImpl() {
SleepMs(1);
{
Expand All @@ -533,14 +523,6 @@ class AudioCodingModuleMtTestOldApi : public AudioCodingModuleTestOldApi {
InsertPacket();
}

static void CbPullAudioThread(void* context) {
AudioCodingModuleMtTestOldApi* fixture =
reinterpret_cast<AudioCodingModuleMtTestOldApi*>(context);
while (!fixture->quit_.load()) {
fixture->CbPullAudioImpl();
}
}

void CbPullAudioImpl() {
SleepMs(1);
{
Expand Down Expand Up @@ -699,16 +681,6 @@ class AcmReRegisterIsacMtTestOldApi : public AudioCodingModuleTestOldApi {

AcmReRegisterIsacMtTestOldApi()
: AudioCodingModuleTestOldApi(),
receive_thread_(
CbReceiveThread,
this,
"receive",
rtc::ThreadAttributes().SetPriority(rtc::kRealtimePriority)),
codec_registration_thread_(
CbCodecRegistrationThread,
this,
"codec_registration",
rtc::ThreadAttributes().SetPriority(rtc::kRealtimePriority)),
codec_registered_(false),
receive_packet_count_(0),
next_insert_packet_time_ms_(0),
Expand Down Expand Up @@ -740,28 +712,34 @@ class AcmReRegisterIsacMtTestOldApi : public AudioCodingModuleTestOldApi {

void StartThreads() {
quit_.store(false);
receive_thread_.Start();
codec_registration_thread_.Start();
const auto attributes =
rtc::ThreadAttributes().SetPriority(rtc::ThreadPriority::kRealtime);
receive_thread_ = rtc::PlatformThread::SpawnJoinable(
[this] {
while (!quit_.load() && CbReceiveImpl()) {
}
},
"receive", attributes);
codec_registration_thread_ = rtc::PlatformThread::SpawnJoinable(
[this] {
while (!quit_.load()) {
CbCodecRegistrationImpl();
}
},
"codec_registration", attributes);
}

void TearDown() override {
AudioCodingModuleTestOldApi::TearDown();
quit_.store(true);
receive_thread_.Stop();
codec_registration_thread_.Stop();
receive_thread_.Finalize();
codec_registration_thread_.Finalize();
}

bool RunTest() {
return test_complete_.Wait(10 * 60 * 1000); // 10 minutes' timeout.
}

static void CbReceiveThread(void* context) {
AcmReRegisterIsacMtTestOldApi* fixture =
reinterpret_cast<AcmReRegisterIsacMtTestOldApi*>(context);
while (!fixture->quit_.load() && fixture->CbReceiveImpl()) {
}
}

bool CbReceiveImpl() {
SleepMs(1);
rtc::Buffer encoded;
Expand Down Expand Up @@ -807,14 +785,6 @@ class AcmReRegisterIsacMtTestOldApi : public AudioCodingModuleTestOldApi {
return true;
}

static void CbCodecRegistrationThread(void* context) {
AcmReRegisterIsacMtTestOldApi* fixture =
reinterpret_cast<AcmReRegisterIsacMtTestOldApi*>(context);
while (!fixture->quit_.load()) {
fixture->CbCodecRegistrationImpl();
}
}

void CbCodecRegistrationImpl() {
SleepMs(1);
if (HasFatalFailure()) {
Expand Down
47 changes: 18 additions & 29 deletions modules/audio_device/dummy/file_audio_device.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,13 @@ int32_t FileAudioDevice::StartPlayout() {
}
}

_ptrThreadPlay.reset(new rtc::PlatformThread(
PlayThreadFunc, this, "webrtc_audio_module_play_thread",
rtc::ThreadAttributes().SetPriority(rtc::kRealtimePriority)));
_ptrThreadPlay->Start();
_ptrThreadPlay = rtc::PlatformThread::SpawnJoinable(
[this] {
while (PlayThreadProcess()) {
}
},
"webrtc_audio_module_play_thread",
rtc::ThreadAttributes().SetPriority(rtc::ThreadPriority::kRealtime));

RTC_LOG(LS_INFO) << "Started playout capture to output file: "
<< _outputFilename;
Expand All @@ -233,10 +236,8 @@ int32_t FileAudioDevice::StopPlayout() {
}

// stop playout thread first
if (_ptrThreadPlay) {
_ptrThreadPlay->Stop();
_ptrThreadPlay.reset();
}
if (!_ptrThreadPlay.empty())
_ptrThreadPlay.Finalize();

MutexLock lock(&mutex_);

Expand Down Expand Up @@ -276,11 +277,13 @@ int32_t FileAudioDevice::StartRecording() {
}
}

_ptrThreadRec.reset(new rtc::PlatformThread(
RecThreadFunc, this, "webrtc_audio_module_capture_thread",
rtc::ThreadAttributes().SetPriority(rtc::kRealtimePriority)));

_ptrThreadRec->Start();
_ptrThreadRec = rtc::PlatformThread::SpawnJoinable(
[this] {
while (RecThreadProcess()) {
}
},
"webrtc_audio_module_capture_thread",
rtc::ThreadAttributes().SetPriority(rtc::ThreadPriority::kRealtime));

RTC_LOG(LS_INFO) << "Started recording from input file: " << _inputFilename;

Expand All @@ -293,10 +296,8 @@ int32_t FileAudioDevice::StopRecording() {
_recording = false;
}

if (_ptrThreadRec) {
_ptrThreadRec->Stop();
_ptrThreadRec.reset();
}
if (!_ptrThreadRec.empty())
_ptrThreadRec.Finalize();

MutexLock lock(&mutex_);
_recordingFramesLeft = 0;
Expand Down Expand Up @@ -439,18 +440,6 @@ void FileAudioDevice::AttachAudioBuffer(AudioDeviceBuffer* audioBuffer) {
_ptrAudioBuffer->SetPlayoutChannels(0);
}

void FileAudioDevice::PlayThreadFunc(void* pThis) {
FileAudioDevice* device = static_cast<FileAudioDevice*>(pThis);
while (device->PlayThreadProcess()) {
}
}

void FileAudioDevice::RecThreadFunc(void* pThis) {
FileAudioDevice* device = static_cast<FileAudioDevice*>(pThis);
while (device->RecThreadProcess()) {
}
}

bool FileAudioDevice::PlayThreadProcess() {
if (!_playing) {
return false;
Expand Down
10 changes: 3 additions & 7 deletions modules/audio_device/dummy/file_audio_device.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,11 @@
#include <string>

#include "modules/audio_device/audio_device_generic.h"
#include "rtc_base/platform_thread.h"
#include "rtc_base/synchronization/mutex.h"
#include "rtc_base/system/file_wrapper.h"
#include "rtc_base/time_utils.h"

namespace rtc {
class PlatformThread;
} // namespace rtc

namespace webrtc {

// This is a fake audio device which plays audio from a file as its microphone
Expand Down Expand Up @@ -145,9 +142,8 @@ class FileAudioDevice : public AudioDeviceGeneric {
size_t _recordingFramesIn10MS;
size_t _playoutFramesIn10MS;

// TODO(pbos): Make plain members instead of pointers and stop resetting them.
std::unique_ptr<rtc::PlatformThread> _ptrThreadRec;
std::unique_ptr<rtc::PlatformThread> _ptrThreadPlay;
rtc::PlatformThread _ptrThreadRec;
rtc::PlatformThread _ptrThreadPlay;

bool _playing;
bool _recording;
Expand Down
Loading

0 comments on commit ad5037b

Please sign in to comment.