From 0b69826ffbf74d58fc26834c56aeb57a8ab7ce1c Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Thu, 7 Mar 2019 09:17:19 +0100 Subject: [PATCH] Don't inject worker queue into send streams. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This prepares for making AudioSendStream use its own task queue. In the future more of the functionality that depends on running on the task queue is planned to be moved directly into RtpTransportControllerSend. They should instead get it from the transport controller. This affects the media transport tests which previously assumed that the transport controller could be missing. However, this is not something that is used in production, so this is an improvement of the tests as they will behave more like production code. Bug: webrtc:9883 Change-Id: Ie32f4c2f6433ec37ac16a08d531ceb690ea9c0b5 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/126000 Reviewed-by: Oskar Sundbom Reviewed-by: Niels Moller Reviewed-by: Erik Språng Commit-Queue: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#27010} --- audio/BUILD.gn | 3 +++ audio/audio_send_stream.cc | 10 +++++----- audio/audio_send_stream.h | 2 -- audio/audio_send_stream_unittest.cc | 7 +++++-- audio/test/media_transport_test.cc | 23 ++++++++++++++++------- call/call.cc | 11 ++--------- video/video_send_stream.cc | 3 +-- video/video_send_stream.h | 1 - 8 files changed, 32 insertions(+), 28 deletions(-) diff --git a/audio/BUILD.gn b/audio/BUILD.gn index 3a1d12316d..9e0bb9f1a9 100644 --- a/audio/BUILD.gn +++ b/audio/BUILD.gn @@ -47,6 +47,7 @@ rtc_static_library("audio") { "../api/audio:audio_frame_api", "../api/audio:audio_mixer_api", "../api/audio_codecs:audio_codecs_api", + "../api/task_queue", "../call:bitrate_allocator", "../call:call_interfaces", "../call:rtp_interfaces", @@ -128,12 +129,14 @@ if (rtc_include_tests) { "../api/audio_codecs:audio_codecs_api", "../api/audio_codecs/opus:audio_decoder_opus", "../api/audio_codecs/opus:audio_encoder_opus", + "../api/task_queue:global_task_queue_factory", "../api/units:time_delta", "../call:mock_bitrate_allocator", "../call:mock_call_interfaces", "../call:mock_rtp_interfaces", "../call:rtp_interfaces", "../call:rtp_receiver", + "../call:rtp_sender", "../common_audio", "../logging:mocks", "../logging:rtc_event_log_api", diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc index 280c915f96..68819a7f18 100644 --- a/audio/audio_send_stream.cc +++ b/audio/audio_send_stream.cc @@ -85,7 +85,6 @@ AudioSendStream::AudioSendStream( Clock* clock, const webrtc::AudioSendStream::Config& config, const rtc::scoped_refptr& audio_state, - rtc::TaskQueue* worker_queue, ProcessThread* module_process_thread, RtpTransportControllerSendInterface* rtp_transport, BitrateAllocatorInterface* bitrate_allocator, @@ -95,14 +94,13 @@ AudioSendStream::AudioSendStream( : AudioSendStream(clock, config, audio_state, - worker_queue, rtp_transport, bitrate_allocator, event_log, rtcp_rtt_stats, suspended_rtp_state, voe::CreateChannelSend(clock, - worker_queue, + rtp_transport->GetWorkerQueue(), module_process_thread, config.media_transport, /*overhead_observer=*/this, @@ -118,7 +116,6 @@ AudioSendStream::AudioSendStream( Clock* clock, const webrtc::AudioSendStream::Config& config, const rtc::scoped_refptr& audio_state, - rtc::TaskQueue* worker_queue, RtpTransportControllerSendInterface* rtp_transport, BitrateAllocatorInterface* bitrate_allocator, RtcEventLog* event_log, @@ -126,7 +123,7 @@ AudioSendStream::AudioSendStream( const absl::optional& suspended_rtp_state, std::unique_ptr channel_send) : clock_(clock), - worker_queue_(worker_queue), + worker_queue_(rtp_transport->GetWorkerQueue()), config_(Config(/*send_transport=*/nullptr, /*media_transport=*/nullptr)), audio_state_(audio_state), @@ -144,6 +141,9 @@ AudioSendStream::AudioSendStream( RTC_DCHECK(audio_state_); RTC_DCHECK(channel_send_); RTC_DCHECK(bitrate_allocator_); + // Currently we require the rtp transport even when media transport is used. + RTC_DCHECK(rtp_transport); + // TODO(nisse): Eventually, we should have only media_transport. But for the // time being, we can have either. When media transport is injected, there // should be no rtp_transport, and below check should be strengthened to XOR diff --git a/audio/audio_send_stream.h b/audio/audio_send_stream.h index afe932127b..0d35562d90 100644 --- a/audio/audio_send_stream.h +++ b/audio/audio_send_stream.h @@ -42,7 +42,6 @@ class AudioSendStream final : public webrtc::AudioSendStream, AudioSendStream(Clock* clock, const webrtc::AudioSendStream::Config& config, const rtc::scoped_refptr& audio_state, - rtc::TaskQueue* worker_queue, ProcessThread* module_process_thread, RtpTransportControllerSendInterface* rtp_transport, BitrateAllocatorInterface* bitrate_allocator, @@ -53,7 +52,6 @@ class AudioSendStream final : public webrtc::AudioSendStream, AudioSendStream(Clock* clock, const webrtc::AudioSendStream::Config& config, const rtc::scoped_refptr& audio_state, - rtc::TaskQueue* worker_queue, RtpTransportControllerSendInterface* rtp_transport, BitrateAllocatorInterface* bitrate_allocator, RtcEventLog* event_log, diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc index df5ee30c40..80d5ad4d2e 100644 --- a/audio/audio_send_stream_unittest.cc +++ b/audio/audio_send_stream_unittest.cc @@ -13,6 +13,7 @@ #include #include "absl/memory/memory.h" +#include "api/task_queue/global_task_queue_factory.h" #include "api/test/mock_frame_encryptor.h" #include "audio/audio_send_stream.h" #include "audio/audio_state.h" @@ -161,11 +162,13 @@ struct ConfigHelper { } std::unique_ptr CreateAudioSendStream() { + EXPECT_CALL(rtp_transport_, GetWorkerQueue()) + .WillRepeatedly(Return(&worker_queue_)); return std::unique_ptr( new internal::AudioSendStream( Clock::GetRealTimeClock(), stream_config_, audio_state_, - &worker_queue_, &rtp_transport_, &bitrate_allocator_, &event_log_, - &rtcp_rtt_stats_, absl::nullopt, + &rtp_transport_, &bitrate_allocator_, &event_log_, &rtcp_rtt_stats_, + absl::nullopt, std::unique_ptr(channel_send_))); } diff --git a/audio/test/media_transport_test.cc b/audio/test/media_transport_test.cc index 02e3f753cb..71a3fb8fc3 100644 --- a/audio/test/media_transport_test.cc +++ b/audio/test/media_transport_test.cc @@ -13,10 +13,12 @@ #include "api/audio_codecs/audio_encoder_factory_template.h" #include "api/audio_codecs/opus/audio_decoder_opus.h" #include "api/audio_codecs/opus/audio_encoder_opus.h" +#include "api/task_queue/global_task_queue_factory.h" #include "api/test/loopback_media_transport.h" #include "api/test/mock_audio_mixer.h" #include "audio/audio_receive_stream.h" #include "audio/audio_send_stream.h" +#include "call/rtp_transport_controller_send.h" #include "call/test/mock_bitrate_allocator.h" #include "logging/rtc_event_log/rtc_event_log.h" #include "modules/audio_device/include/test_audio_device.h" @@ -32,6 +34,10 @@ namespace webrtc { namespace test { namespace { +using testing::_; +using testing::NiceMock; +using testing::Return; + constexpr int kPayloadTypeOpus = 17; constexpr int kSamplingFrequency = 48000; constexpr int kNumChannels = 2; @@ -69,10 +75,10 @@ TEST(AudioWithMediaTransport, DeliversAudio) { std::unique_ptr transport_thread = rtc::Thread::Create(); transport_thread->Start(); MediaTransportPair transport_pair(transport_thread.get()); - MockTransport rtcp_send_transport; - MockTransport send_transport; + NiceMock rtcp_send_transport; + NiceMock send_transport; std::unique_ptr null_event_log = RtcEventLog::CreateNull(); - MockBitrateAllocator bitrate_allocator; + NiceMock bitrate_allocator; rtc::scoped_refptr audio_device = TestAudioDeviceModule::CreateTestAudioDeviceModule( @@ -117,13 +123,16 @@ TEST(AudioWithMediaTransport, DeliversAudio) { send_config.send_codec_spec = AudioSendStream::Config::SendCodecSpec(kPayloadTypeOpus, audio_format); send_config.encoder_factory = CreateAudioEncoderFactory(); - rtc::TaskQueue send_tq("audio send queue"); std::unique_ptr send_process_thread = ProcessThread::Create("audio send thread"); + RtpTransportControllerSend rtp_transport( + Clock::GetRealTimeClock(), null_event_log.get(), nullptr, + BitrateConstraints(), ProcessThread::Create("Pacer"), + &GlobalTaskQueueFactory()); webrtc::internal::AudioSendStream send_stream( - Clock::GetRealTimeClock(), send_config, audio_state, &send_tq, - send_process_thread.get(), - /*transport=*/nullptr, &bitrate_allocator, null_event_log.get(), + Clock::GetRealTimeClock(), send_config, audio_state, + send_process_thread.get(), &rtp_transport, &bitrate_allocator, + null_event_log.get(), /*rtcp_rtt_stats=*/nullptr, absl::optional()); audio_device->Init(); // Starts thread. diff --git a/call/call.cc b/call/call.cc index 08fdcd2153..3000244807 100644 --- a/call/call.cc +++ b/call/call.cc @@ -666,12 +666,8 @@ webrtc::AudioSendStream* Call::CreateAudioSendStream( } } - // TODO(srte): AudioSendStream should call GetWorkerQueue directly rather than - // having it injected. - AudioSendStream* send_stream = new AudioSendStream( - clock_, config, config_.audio_state, - transport_send_ptr_->GetWorkerQueue(), module_process_thread_.get(), + clock_, config, config_.audio_state, module_process_thread_.get(), transport_send_ptr_, bitrate_allocator_.get(), event_log_, call_stats_.get(), suspended_rtp_state); { @@ -802,11 +798,8 @@ webrtc::VideoSendStream* Call::CreateVideoSendStream( // Copy ssrcs from |config| since |config| is moved. std::vector ssrcs = config.rtp.ssrcs; - // TODO(srte): VideoSendStream should call GetWorkerQueue directly rather than - // having it injected. VideoSendStream* send_stream = new VideoSendStream( - clock_, num_cpu_cores_, module_process_thread_.get(), - transport_send_ptr_->GetWorkerQueue(), task_queue_factory_, + clock_, num_cpu_cores_, module_process_thread_.get(), task_queue_factory_, call_stats_.get(), transport_send_ptr_, bitrate_allocator_.get(), video_send_delay_stats_.get(), event_log_, std::move(config), std::move(encoder_config), suspended_video_send_ssrcs_, diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc index 3b6c4f5068..edf39223a7 100644 --- a/video/video_send_stream.cc +++ b/video/video_send_stream.cc @@ -69,7 +69,6 @@ VideoSendStream::VideoSendStream( Clock* clock, int num_cpu_cores, ProcessThread* module_process_thread, - rtc::TaskQueue* worker_queue, TaskQueueFactory* task_queue_factory, CallStats* call_stats, RtpTransportControllerSendInterface* transport, @@ -81,7 +80,7 @@ VideoSendStream::VideoSendStream( const std::map& suspended_ssrcs, const std::map& suspended_payload_states, std::unique_ptr fec_controller) - : worker_queue_(worker_queue), + : worker_queue_(transport->GetWorkerQueue()), stats_proxy_(clock, config, encoder_config.content_type), config_(std::move(config)), content_type_(encoder_config.content_type) { diff --git a/video/video_send_stream.h b/video/video_send_stream.h index f1ac4773b8..827f991d62 100644 --- a/video/video_send_stream.h +++ b/video/video_send_stream.h @@ -56,7 +56,6 @@ class VideoSendStream : public webrtc::VideoSendStream { Clock* clock, int num_cpu_cores, ProcessThread* module_process_thread, - rtc::TaskQueue* worker_queue, TaskQueueFactory* task_queue_factory, CallStats* call_stats, RtpTransportControllerSendInterface* transport,