From e27ccf9a1681e0e4ff9281f9a18fea357d2bc890 Mon Sep 17 00:00:00 2001 From: Amit Hilbuch Date: Tue, 26 Mar 2019 17:36:53 +0000 Subject: [PATCH] Revert "in WebrtcVoiceEngine allow to set TaskQueueFactory" This reverts commit a39254da593bbdb0b1e072a44827229680afe3ee. Reason for revert: Tests are failing due to ThreadChecker's called on valid thread. Original change's description: > in WebrtcVoiceEngine allow to set TaskQueueFactory > > in production code keep using GlobalTaskQueueFactory() > in tests switch to use DefaultTaskQueueFactory directly. > > Bug: webrtc:10284 > Change-Id: I170274a98324796623089a965a39f0cbb7e281d9 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/128878 > Reviewed-by: Steve Anton > Commit-Queue: Danil Chapovalov > Cr-Commit-Position: refs/heads/master@{#27296} TBR=danilchap@webrtc.org,steveanton@webrtc.org Change-Id: I9742e5d0171a94f3840e197c40fdb44523e4963b No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:10284 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/129780 Reviewed-by: Amit Hilbuch Commit-Queue: Amit Hilbuch Cr-Commit-Position: refs/heads/master@{#27297} --- media/BUILD.gn | 4 -- .../null_webrtc_video_engine_unittest.cc | 37 ++++++++-------- media/engine/webrtc_media_engine.cc | 7 ++- media/engine/webrtc_voice_engine.cc | 18 ++++---- media/engine/webrtc_voice_engine.h | 4 +- media/engine/webrtc_voice_engine_unittest.cc | 43 ++++--------------- 6 files changed, 42 insertions(+), 71 deletions(-) diff --git a/media/BUILD.gn b/media/BUILD.gn index d02ef89d18..26a17a1bdf 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -260,8 +260,6 @@ rtc_static_library("rtc_audio_video") { libs = [] deps = [ "../api:scoped_refptr", - "../api/task_queue", - "../api/task_queue:global_task_queue_factory", "../api/video:video_bitrate_allocation", "../api/video:video_bitrate_allocator_factory", "../modules/audio_processing:api", @@ -495,8 +493,6 @@ if (rtc_include_tests) { "../:webrtc_common", "../api:fake_media_transport", "../api:scoped_refptr", - "../api/task_queue", - "../api/task_queue:default_task_queue_factory", "../api/test/video:function_video_factory", "../api/units:time_delta", "../api/video:video_frame_i420", diff --git a/media/engine/null_webrtc_video_engine_unittest.cc b/media/engine/null_webrtc_video_engine_unittest.cc index 343e167fcb..85eaea3a92 100644 --- a/media/engine/null_webrtc_video_engine_unittest.cc +++ b/media/engine/null_webrtc_video_engine_unittest.cc @@ -9,13 +9,7 @@ */ #include "media/engine/null_webrtc_video_engine.h" - -#include -#include - #include "absl/memory/memory.h" -#include "api/task_queue/default_task_queue_factory.h" -#include "api/task_queue/task_queue_factory.h" #include "media/engine/webrtc_voice_engine.h" #include "modules/audio_device/include/mock_audio_device.h" #include "modules/audio_processing/include/audio_processing.h" @@ -25,21 +19,30 @@ namespace cricket { +class WebRtcMediaEngineNullVideo : public CompositeMediaEngine { + public: + WebRtcMediaEngineNullVideo( + webrtc::AudioDeviceModule* adm, + const rtc::scoped_refptr& + audio_encoder_factory, + const rtc::scoped_refptr& + audio_decoder_factory) + : CompositeMediaEngine(absl::make_unique( + adm, + audio_encoder_factory, + audio_decoder_factory, + nullptr, + webrtc::AudioProcessingBuilder().Create()), + absl::make_unique()) {} +}; + // Simple test to check if NullWebRtcVideoEngine implements the methods // required by CompositeMediaEngine. TEST(NullWebRtcVideoEngineTest, CheckInterface) { - std::unique_ptr task_queue_factory = - webrtc::CreateDefaultTaskQueueFactory(); testing::NiceMock adm; - auto audio_engine = absl::make_unique( - task_queue_factory.get(), &adm, - webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), - webrtc::MockAudioDecoderFactory::CreateUnusedFactory(), nullptr, - webrtc::AudioProcessingBuilder().Create()); - - CompositeMediaEngine engine(std::move(audio_engine), - absl::make_unique()); - + WebRtcMediaEngineNullVideo engine( + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + webrtc::MockAudioDecoderFactory::CreateUnusedFactory()); EXPECT_TRUE(engine.Init()); } diff --git a/media/engine/webrtc_media_engine.cc b/media/engine/webrtc_media_engine.cc index 48269ee51b..5de0927a52 100644 --- a/media/engine/webrtc_media_engine.cc +++ b/media/engine/webrtc_media_engine.cc @@ -14,7 +14,6 @@ #include "absl/algorithm/container.h" #include "absl/memory/memory.h" -#include "api/task_queue/global_task_queue_factory.h" #include "api/video/builtin_video_bitrate_allocator_factory.h" #include "api/video_codecs/video_decoder_factory.h" #include "api/video_codecs/video_encoder_factory.h" @@ -62,9 +61,9 @@ std::unique_ptr WebRtcMediaEngineFactory::Create( auto video_engine = absl::make_unique(); #endif return std::unique_ptr(new CompositeMediaEngine( - absl::make_unique( - &webrtc::GlobalTaskQueueFactory(), adm, audio_encoder_factory, - audio_decoder_factory, audio_mixer, audio_processing), + absl::make_unique(adm, audio_encoder_factory, + audio_decoder_factory, audio_mixer, + audio_processing), std::move(video_engine))); } diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index a94fa7ab03..9fc9d08362 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -178,16 +178,12 @@ absl::optional ComputeSendBitrate(int max_send_bitrate_bps, } // namespace WebRtcVoiceEngine::WebRtcVoiceEngine( - webrtc::TaskQueueFactory* task_queue_factory, webrtc::AudioDeviceModule* adm, const rtc::scoped_refptr& encoder_factory, const rtc::scoped_refptr& decoder_factory, rtc::scoped_refptr audio_mixer, rtc::scoped_refptr audio_processing) - : low_priority_worker_queue_(task_queue_factory->CreateTaskQueue( - "rtc-low-prio", - webrtc::TaskQueueFactory::Priority::LOW)), - adm_(adm), + : adm_(adm), encoder_factory_(encoder_factory), decoder_factory_(decoder_factory), audio_mixer_(audio_mixer), @@ -220,6 +216,10 @@ void WebRtcVoiceEngine::Init() { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); RTC_LOG(LS_INFO) << "WebRtcVoiceEngine::Init"; + // TaskQueue expects to be created/destroyed on the same thread. + low_priority_worker_queue_.reset( + new rtc::TaskQueue("rtc-low-prio", rtc::TaskQueue::Priority::LOW)); + // Load our audio codec lists. RTC_LOG(LS_INFO) << "Supported send codecs in order of preference:"; send_codecs_ = CollectCodecs(encoder_factory_->GetSupportedEncoders()); @@ -580,8 +580,8 @@ void WebRtcVoiceEngine::UnregisterChannel(WebRtcVoiceMediaChannel* channel) { bool WebRtcVoiceEngine::StartAecDump(rtc::PlatformFile file, int64_t max_size_bytes) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - auto aec_dump = webrtc::AecDumpFactory::Create(file, max_size_bytes, - &low_priority_worker_queue_); + auto aec_dump = webrtc::AecDumpFactory::Create( + file, max_size_bytes, low_priority_worker_queue_.get()); if (!aec_dump) { return false; } @@ -592,8 +592,8 @@ bool WebRtcVoiceEngine::StartAecDump(rtc::PlatformFile file, void WebRtcVoiceEngine::StartAecDump(const std::string& filename) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - auto aec_dump = - webrtc::AecDumpFactory::Create(filename, -1, &low_priority_worker_queue_); + auto aec_dump = webrtc::AecDumpFactory::Create( + filename, -1, low_priority_worker_queue_.get()); if (aec_dump) { apm()->AttachAecDump(std::move(aec_dump)); } diff --git a/media/engine/webrtc_voice_engine.h b/media/engine/webrtc_voice_engine.h index 1dbba82d29..3bce78de0a 100644 --- a/media/engine/webrtc_voice_engine.h +++ b/media/engine/webrtc_voice_engine.h @@ -19,7 +19,6 @@ #include "api/audio_codecs/audio_encoder_factory.h" #include "api/rtp_receiver_interface.h" #include "api/scoped_refptr.h" -#include "api/task_queue/task_queue_factory.h" #include "call/audio_state.h" #include "call/call.h" #include "media/base/rtp_utils.h" @@ -47,7 +46,6 @@ class WebRtcVoiceEngine final : public VoiceEngineInterface { public: WebRtcVoiceEngine( - webrtc::TaskQueueFactory* task_queue_factory, webrtc::AudioDeviceModule* adm, const rtc::scoped_refptr& encoder_factory, const rtc::scoped_refptr& decoder_factory, @@ -97,7 +95,7 @@ class WebRtcVoiceEngine final : public VoiceEngineInterface { void StartAecDump(const std::string& filename); int CreateVoEChannel(); - rtc::TaskQueue low_priority_worker_queue_; + std::unique_ptr low_priority_worker_queue_; webrtc::AudioDeviceModule* adm(); webrtc::AudioProcessing* apm() const; diff --git a/media/engine/webrtc_voice_engine_unittest.cc b/media/engine/webrtc_voice_engine_unittest.cc index 2f6d74a3dc..a8935e1f8f 100644 --- a/media/engine/webrtc_voice_engine_unittest.cc +++ b/media/engine/webrtc_voice_engine_unittest.cc @@ -16,7 +16,6 @@ #include "api/audio_codecs/builtin_audio_encoder_factory.h" #include "api/rtp_parameters.h" #include "api/scoped_refptr.h" -#include "api/task_queue/default_task_queue_factory.h" #include "call/call.h" #include "logging/rtc_event_log/rtc_event_log.h" #include "media/base/fake_media_engine.h" @@ -136,8 +135,6 @@ void AdmSetupExpectations(webrtc::test::MockAudioDeviceModule* adm) { // Tests that our stub library "works". TEST(WebRtcVoiceEngineTestStubLibrary, StartupShutdown) { - std::unique_ptr task_queue_factory = - webrtc::CreateDefaultTaskQueueFactory(); StrictMock adm; AdmSetupExpectations(&adm); rtc::scoped_refptr> apm = @@ -150,8 +147,7 @@ TEST(WebRtcVoiceEngineTestStubLibrary, StartupShutdown) { EXPECT_CALL(*apm, DetachAecDump()); { cricket::WebRtcVoiceEngine engine( - task_queue_factory.get(), &adm, - webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), webrtc::MockAudioDecoderFactory::CreateUnusedFactory(), nullptr, apm); engine.Init(); } @@ -171,8 +167,7 @@ class WebRtcVoiceEngineTestFake : public testing::Test { WebRtcVoiceEngineTestFake() : WebRtcVoiceEngineTestFake("") {} explicit WebRtcVoiceEngineTestFake(const char* field_trials) - : task_queue_factory_(webrtc::CreateDefaultTaskQueueFactory()), - apm_(new rtc::RefCountedObject< + : apm_(new rtc::RefCountedObject< StrictMock>()), apm_gc_(*apm_->gain_control()), apm_ns_(*apm_->noise_suppression()), @@ -203,8 +198,7 @@ class WebRtcVoiceEngineTestFake : public testing::Test { auto encoder_factory = webrtc::CreateBuiltinAudioEncoderFactory(); auto decoder_factory = webrtc::CreateBuiltinAudioDecoderFactory(); engine_.reset(new cricket::WebRtcVoiceEngine( - task_queue_factory_.get(), &adm_, encoder_factory, decoder_factory, - nullptr, apm_)); + &adm_, encoder_factory, decoder_factory, nullptr, apm_)); engine_->Init(); send_parameters_.codecs.push_back(kPcmuCodec); recv_parameters_.codecs.push_back(kPcmuCodec); @@ -756,7 +750,6 @@ class WebRtcVoiceEngineTestFake : public testing::Test { } protected: - std::unique_ptr task_queue_factory_; StrictMock adm_; rtc::scoped_refptr> apm_; webrtc::test::MockGainControl& apm_gc_; @@ -3499,14 +3492,11 @@ TEST_F(WebRtcVoiceEngineTestFake, GetSourcesWithNonExistingSsrc) { TEST(WebRtcVoiceEngineTest, StartupShutdown) { // If the VoiceEngine wants to gather available codecs early, that's fine but // we never want it to create a decoder at this stage. - std::unique_ptr task_queue_factory = - webrtc::CreateDefaultTaskQueueFactory(); testing::NiceMock adm; rtc::scoped_refptr apm = webrtc::AudioProcessingBuilder().Create(); cricket::WebRtcVoiceEngine engine( - task_queue_factory.get(), &adm, - webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), webrtc::MockAudioDecoderFactory::CreateUnusedFactory(), nullptr, apm); engine.Init(); webrtc::RtcEventLogNullImpl event_log; @@ -3521,8 +3511,6 @@ TEST(WebRtcVoiceEngineTest, StartupShutdown) { // Tests that reference counting on the external ADM is correct. TEST(WebRtcVoiceEngineTest, StartupShutdownWithExternalADM) { - std::unique_ptr task_queue_factory = - webrtc::CreateDefaultTaskQueueFactory(); testing::NiceMock adm; EXPECT_CALL(adm, AddRef()).Times(3); EXPECT_CALL(adm, Release()) @@ -3532,8 +3520,7 @@ TEST(WebRtcVoiceEngineTest, StartupShutdownWithExternalADM) { rtc::scoped_refptr apm = webrtc::AudioProcessingBuilder().Create(); cricket::WebRtcVoiceEngine engine( - task_queue_factory.get(), &adm, - webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), webrtc::MockAudioDecoderFactory::CreateUnusedFactory(), nullptr, apm); engine.Init(); webrtc::RtcEventLogNullImpl event_log; @@ -3549,16 +3536,13 @@ TEST(WebRtcVoiceEngineTest, StartupShutdownWithExternalADM) { // Verify the payload id of common audio codecs, including CN, ISAC, and G722. TEST(WebRtcVoiceEngineTest, HasCorrectPayloadTypeMapping) { - std::unique_ptr task_queue_factory = - webrtc::CreateDefaultTaskQueueFactory(); // TODO(ossu): Why are the payload types of codecs with non-static payload // type assignments checked here? It shouldn't really matter. testing::NiceMock adm; rtc::scoped_refptr apm = webrtc::AudioProcessingBuilder().Create(); cricket::WebRtcVoiceEngine engine( - task_queue_factory.get(), &adm, - webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), webrtc::MockAudioDecoderFactory::CreateUnusedFactory(), nullptr, apm); engine.Init(); for (const cricket::AudioCodec& codec : engine.send_codecs()) { @@ -3599,14 +3583,11 @@ TEST(WebRtcVoiceEngineTest, HasCorrectPayloadTypeMapping) { // Tests that VoE supports at least 32 channels TEST(WebRtcVoiceEngineTest, Has32Channels) { - std::unique_ptr task_queue_factory = - webrtc::CreateDefaultTaskQueueFactory(); testing::NiceMock adm; rtc::scoped_refptr apm = webrtc::AudioProcessingBuilder().Create(); cricket::WebRtcVoiceEngine engine( - task_queue_factory.get(), &adm, - webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), webrtc::MockAudioDecoderFactory::CreateUnusedFactory(), nullptr, apm); engine.Init(); webrtc::RtcEventLogNullImpl event_log; @@ -3634,8 +3615,6 @@ TEST(WebRtcVoiceEngineTest, Has32Channels) { // Test that we set our preferred codecs properly. TEST(WebRtcVoiceEngineTest, SetRecvCodecs) { - std::unique_ptr task_queue_factory = - webrtc::CreateDefaultTaskQueueFactory(); // TODO(ossu): I'm not sure of the intent of this test. It's either: // - Check that our builtin codecs are usable by Channel. // - The codecs provided by the engine is usable by Channel. @@ -3647,8 +3626,7 @@ TEST(WebRtcVoiceEngineTest, SetRecvCodecs) { rtc::scoped_refptr apm = webrtc::AudioProcessingBuilder().Create(); cricket::WebRtcVoiceEngine engine( - task_queue_factory.get(), &adm, - webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), + &adm, webrtc::MockAudioEncoderFactory::CreateUnusedFactory(), webrtc::CreateBuiltinAudioDecoderFactory(), nullptr, apm); engine.Init(); webrtc::RtcEventLogNullImpl event_log; @@ -3680,8 +3658,6 @@ TEST(WebRtcVoiceEngineTest, CollectRecvCodecs) { specs.push_back( webrtc::AudioCodecSpec{{"codec5", 8000, 2}, {8000, 1, 64000}}); - std::unique_ptr task_queue_factory = - webrtc::CreateDefaultTaskQueueFactory(); rtc::scoped_refptr unused_encoder_factory = webrtc::MockAudioEncoderFactory::CreateUnusedFactory(); rtc::scoped_refptr mock_decoder_factory = @@ -3692,8 +3668,7 @@ TEST(WebRtcVoiceEngineTest, CollectRecvCodecs) { rtc::scoped_refptr apm = webrtc::AudioProcessingBuilder().Create(); - cricket::WebRtcVoiceEngine engine(task_queue_factory.get(), &adm, - unused_encoder_factory, + cricket::WebRtcVoiceEngine engine(&adm, unused_encoder_factory, mock_decoder_factory, nullptr, apm); engine.Init(); auto codecs = engine.recv_codecs();