Commit 53c571c9 authored by sergeyu's avatar sergeyu Committed by Commit bot

Fix threading issues in the audio pipeline for WebRTC remoting protocol.

1. Updated unittests to use a separate thread for audio encoding and
   decoding.
2. WebrtcAudioSinkAdapter was calling AudioStub on a wrong thread.
3. WebrtcAudioModule was destroying base::Timer on a thread different
   from the one on which it is created.

Review-Url: https://codereview.chromium.org/2650633003
Cr-Original-Commit-Position: refs/heads/master@{#445566}
Committed: https://chromium.googlesource.com/chromium/src/+/25680c62320767f590d037d301edfe15e9c55650
Review-Url: https://codereview.chromium.org/2650633003
Cr-Commit-Position: refs/heads/master@{#445672}
parent 0a70d0b1
......@@ -11,6 +11,8 @@
#include "base/memory/ptr_util.h"
#include "base/message_loop/message_loop.h"
#include "base/run_loop.h"
#include "base/threading/thread.h"
#include "base/threading/thread_checker.h"
#include "remoting/base/constants.h"
#include "remoting/proto/audio.pb.h"
#include "remoting/protocol/audio_source.h"
......@@ -185,6 +187,7 @@ class FakeAudioPlayer : public AudioStub {
// AudioStub interface.
void ProcessAudioPacket(std::unique_ptr<AudioPacket> packet,
const base::Closure& done) override {
EXPECT_TRUE(thread_checker_.CalledOnValidThread());
EXPECT_EQ(AudioPacket::ENCODING_RAW, packet->encoding());
EXPECT_EQ(AudioPacket::SAMPLING_RATE_48000, packet->sampling_rate());
EXPECT_EQ(AudioPacket::BYTES_PER_SAMPLE_2, packet->bytes_per_sample());
......@@ -241,6 +244,7 @@ class FakeAudioPlayer : public AudioStub {
base::WeakPtr<AudioStub> GetWeakPtr() { return weak_factory_.GetWeakPtr(); }
private:
base::ThreadChecker thread_checker_;
std::vector<char> data_;
base::RunLoop* run_loop_ = nullptr;
size_t samples_expected_ = 0;
......@@ -253,7 +257,14 @@ class FakeAudioPlayer : public AudioStub {
class ConnectionTest : public testing::Test,
public testing::WithParamInterface<bool> {
public:
ConnectionTest() {}
ConnectionTest()
: video_encode_thread_("VideoEncode"),
audio_encode_thread_("AudioEncode"),
audio_decode_thread_("AudioDecode") {
video_encode_thread_.Start();
audio_encode_thread_.Start();
audio_decode_thread_.Start();
}
void DestroyHost() {
host_connection_.reset();
......@@ -296,7 +307,7 @@ class ConnectionTest : public testing::Test,
client_connection_->set_clipboard_stub(&client_clipboard_stub_);
client_connection_->set_video_renderer(&client_video_renderer_);
client_connection_->InitializeAudio(message_loop_.task_runner(),
client_connection_->InitializeAudio(audio_decode_thread_.task_runner(),
client_audio_player_.GetWeakPtr());
}
......@@ -438,6 +449,10 @@ class ConnectionTest : public testing::Test,
std::unique_ptr<FakeSession> owned_client_session_;
bool client_connected_ = false;
base::Thread video_encode_thread_;
base::Thread audio_encode_thread_;
base::Thread audio_decode_thread_;
private:
DISALLOW_COPY_AND_ASSIGN(ConnectionTest);
};
......
......@@ -5,8 +5,10 @@
#include "remoting/protocol/webrtc_audio_module.h"
#include "base/bind.h"
#include "base/memory/ptr_util.h"
#include "base/stl_util.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/timer/timer.h"
namespace remoting {
namespace protocol {
......@@ -502,14 +504,15 @@ int WebrtcAudioModule::GetRecordAudioParameters(
void WebrtcAudioModule::StartPlayoutOnAudioThread() {
DCHECK(audio_task_runner_->BelongsToCurrentThread());
poll_timer_.Start(
poll_timer_ = base::MakeUnique<base::RepeatingTimer>();
poll_timer_->Start(
FROM_HERE, kPollInterval,
base::Bind(&WebrtcAudioModule::PollFromSource, base::Unretained(this)));
}
void WebrtcAudioModule::StopPlayoutOnAudioThread() {
DCHECK(audio_task_runner_->BelongsToCurrentThread());
poll_timer_.Stop();
poll_timer_.reset();
}
void WebrtcAudioModule::PollFromSource() {
......
......@@ -7,10 +7,10 @@
#include "base/memory/ref_counted.h"
#include "base/synchronization/lock.h"
#include "base/timer/timer.h"
#include "third_party/webrtc/modules/audio_device/include/audio_device.h"
namespace base {
class RepeatingTimer;
class SingleThreadTaskRunner;
} // namespace base
......@@ -155,7 +155,7 @@ class WebrtcAudioModule : public webrtc::AudioDeviceModule {
// Timer running on the |audio_task_runner_| that polls audio from
// |audio_transport_|.
base::RepeatingTimer poll_timer_;
std::unique_ptr<base::RepeatingTimer> poll_timer_;
};
} // namespace protocol
......
......@@ -4,6 +4,7 @@
#include "remoting/protocol/webrtc_audio_sink_adapter.h"
#include "base/bind.h"
#include "base/callback.h"
#include "remoting/proto/audio.pb.h"
#include "remoting/protocol/audio_stub.h"
......@@ -13,20 +14,17 @@ namespace protocol {
WebrtcAudioSinkAdapter::WebrtcAudioSinkAdapter(
scoped_refptr<webrtc::MediaStreamInterface> stream,
base::WeakPtr<AudioStub> audio_stub) {
audio_stub_ = audio_stub;
media_stream_ = std::move(stream);
base::WeakPtr<AudioStub> audio_stub)
: task_runner_(base::ThreadTaskRunnerHandle::Get()),
audio_stub_(audio_stub),
media_stream_(std::move(stream)) {
webrtc::AudioTrackVector audio_tracks = media_stream_->GetAudioTracks();
// Caller must verify that the media stream contains audio tracks.
DCHECK(!audio_tracks.empty());
if (audio_tracks.size() > 1U) {
LOG(WARNING) << "Received media stream with multiple audio tracks.";
}
audio_track_ = audio_tracks[0];
audio_track_->GetSource()->AddSink(this);
}
......@@ -40,9 +38,6 @@ void WebrtcAudioSinkAdapter::OnData(const void* audio_data,
int sample_rate,
size_t number_of_channels,
size_t number_of_frames) {
if (!audio_stub_)
return;
std::unique_ptr<AudioPacket> audio_packet(new AudioPacket());
audio_packet->set_encoding(AudioPacket::ENCODING_RAW);
......@@ -73,7 +68,10 @@ void WebrtcAudioSinkAdapter::OnData(const void* audio_data,
size_t data_size =
number_of_frames * number_of_channels * (bits_per_sample / 8);
audio_packet->add_data(reinterpret_cast<const char*>(audio_data), data_size);
audio_stub_->ProcessAudioPacket(std::move(audio_packet), base::Closure());
task_runner_->PostTask(
FROM_HERE, base::Bind(&AudioStub::ProcessAudioPacket, audio_stub_,
base::Passed(&audio_packet), base::Closure()));
}
} // namespace protocol
......
......@@ -7,8 +7,13 @@
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "base/threading/thread_task_runner_handle.h"
#include "third_party/webrtc/api/mediastreaminterface.h"
namespace base {
class SingleThreadTaskRunner;
} // namespace base
namespace remoting {
namespace protocol {
......@@ -27,10 +32,10 @@ class WebrtcAudioSinkAdapter : public webrtc::AudioTrackSinkInterface {
size_t number_of_frames) override;
private:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
base::WeakPtr<AudioStub> audio_stub_;
scoped_refptr<webrtc::MediaStreamInterface> media_stream_;
scoped_refptr<webrtc::AudioTrackInterface> audio_track_;
base::WeakPtr<AudioStub> audio_stub_;
};
} // namespace protocol
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment