From 048cbdda0d4525f67dad3696fb37e2d5d167bafb Mon Sep 17 00:00:00 2001 From: aleloi Date: Mon, 29 May 2017 02:56:27 -0700 Subject: [PATCH] Reland of Activate 'offload debug dump recordings from audio thread to TaskQueue'. (patchset #1 id:1 of https://codereview.webrtc.org/2910633002/ ) Reason for revert: Revert of revert of revert of revert of 'Activating..'. Or "reland of reland of 'Activate..'". *Now* the internal projects are fixed and the fix is verified. Original issue's description: > Revert of Activate 'offload debug dump recordings from audio thread to TaskQueue'. (patchset #1 id:1 of https://codereview.webrtc.org/2903153005/ ) > > Reason for revert: > Reverting again: internal project issues were apparently not completely fixed. > > Original issue's description: > > Reland of Activate 'offload debug dump recordings from audio thread to TaskQueue'. (patchset #1 id:1 of https://codereview.webrtc.org/2904893002/ ) > > > > Reason for revert: > > Revert the revert now that internal projects are updated. > > > > Original issue's description: > > > Revert of Activate 'offload debug dump recordings from audio thread to TaskQueue'. (patchset #4 id:160001 of https://codereview.webrtc.org/2896813002/ ) > > > > > > Reason for revert: > > > Breaks internal project. > > > > > > Original issue's description: > > > > Activate 'offload debug dump recordings from audio thread to TaskQueue'. > > > > > > > > A low priority task queue is added to WebRTCVoiceEngine. The > > > > start/stop debug calls make file logging happen on the task queue. > > > > > > > > In a dependent CL (https://codereview.webrtc.org/2888303003), the task queue is moved to PeerConnectionFactory, > > > > so that it can be shared for low priority tasks between different > > > > subcomponents. It will require some changes to MediaEngine, > > > > CompositeMediaEngine, WebRTCVoiceEngine, and changes in internal > > > > projects. > > > > > > > > A task queue must be created and destroyed from the same thread. With > > > > this CL that will be the worker thread, which creates and destroys > > > > WebRTCVoiceEngine. With the dependent CL, it will probably change to > > > > the signaling thread. > > > > > > > > NOTRY=True # tests just passed > > > > > > > > BUG=webrtc:7404 > > > > > > > > Review-Url: https://codereview.webrtc.org/2896813002 > > > > Cr-Commit-Position: refs/heads/master@{#18252} > > > > Committed: https://chromium.googlesource.com/external/webrtc/+/c61bf947b4ac31f3500858ffcae6fee39d799930 > > > > > > TBR=solenberg@webrtc.org,tommi@webrtc.org,perkj@webrtc.org,danilchap@webrtc.org,tommi@chromium.org > > > # Skipping CQ checks because original CL landed less than 1 days ago. > > > NOPRESUBMIT=true > > > NOTREECHECKS=true > > > NOTRY=true > > > BUG=webrtc:7404 > > > > > > Review-Url: https://codereview.webrtc.org/2904893002 > > > Cr-Commit-Position: refs/heads/master@{#18255} > > > Committed: https://chromium.googlesource.com/external/webrtc/+/be68b72cfad0686dcd892bba1368b199a7ee16ca > > > > TBR=solenberg@webrtc.org,tommi@webrtc.org,perkj@webrtc.org,danilchap@webrtc.org,tommi@chromium.org > > # Not skipping CQ checks because original CL landed more than 1 days ago. > > BUG=webrtc:7404 > > > > Review-Url: https://codereview.webrtc.org/2903153005 > > Cr-Commit-Position: refs/heads/master@{#18270} > > Committed: https://chromium.googlesource.com/external/webrtc/+/d2303a2338106feab684860f1c133877b46bdd4f > > TBR=solenberg@webrtc.org,tommi@webrtc.org,perkj@webrtc.org,danilchap@webrtc.org,tommi@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=webrtc:7404 > > Review-Url: https://codereview.webrtc.org/2910633002 > Cr-Commit-Position: refs/heads/master@{#18272} > Committed: https://chromium.googlesource.com/external/webrtc/+/fe9ecb07ea8254d8a09605f25203a4d045b3ffee TBR=solenberg@webrtc.org,tommi@webrtc.org,perkj@webrtc.org,danilchap@webrtc.org,tommi@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=webrtc:7404 Review-Url: https://codereview.webrtc.org/2904423002 Cr-Commit-Position: refs/heads/master@{#18300} --- webrtc/media/BUILD.gn | 6 +++ webrtc/media/engine/webrtcvoiceengine.cc | 42 ++++++------------- webrtc/media/engine/webrtcvoiceengine.h | 4 ++ .../engine/webrtcvoiceengine_unittest.cc | 2 + 4 files changed, 25 insertions(+), 29 deletions(-) diff --git a/webrtc/media/BUILD.gn b/webrtc/media/BUILD.gn index cc77598439..d33b399d7c 100644 --- a/webrtc/media/BUILD.gn +++ b/webrtc/media/BUILD.gn @@ -220,6 +220,11 @@ rtc_static_library("rtc_media") { public_configs += [ ":rtc_media_defines_config" ] deps += [ "../modules/video_capture:video_capture_internal_impl" ] } + if (rtc_enable_protobuf) { + deps += [ "../modules/audio_processing/aec_dump:aec_dump_impl" ] + } else { + deps += [ "../modules/audio_processing/aec_dump:null_aec_dump_factory" ] + } deps += [ ":rtc_media_base", "..:webrtc_common", @@ -237,6 +242,7 @@ rtc_static_library("rtc_media") { "../modules/audio_device:audio_device", "../modules/audio_mixer:audio_mixer_impl", "../modules/audio_processing:audio_processing", + "../modules/audio_processing/aec_dump", "../modules/video_capture:video_capture_module", "../modules/video_coding", "../modules/video_coding:webrtc_h264", diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc index 2103138960..020d74d826 100644 --- a/webrtc/media/engine/webrtcvoiceengine.cc +++ b/webrtc/media/engine/webrtcvoiceengine.cc @@ -38,6 +38,7 @@ #include "webrtc/media/engine/webrtcmediaengine.h" #include "webrtc/media/engine/webrtcvoe.h" #include "webrtc/modules/audio_mixer/audio_mixer_impl.h" +#include "webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h" #include "webrtc/modules/audio_processing/include/audio_processing.h" #include "webrtc/system_wrappers/include/field_trial.h" #include "webrtc/system_wrappers/include/metrics.h" @@ -228,7 +229,8 @@ WebRtcVoiceEngine::WebRtcVoiceEngine( const rtc::scoped_refptr& decoder_factory, rtc::scoped_refptr audio_mixer, VoEWrapper* voe_wrapper) - : adm_(adm), + : low_priority_worker_queue_("rtc-low-prio", rtc::TaskQueue::Priority::LOW), + adm_(adm), encoder_factory_(encoder_factory), decoder_factory_(decoder_factory), voe_wrapper_(voe_wrapper) { @@ -687,46 +689,28 @@ void WebRtcVoiceEngine::UnregisterChannel(WebRtcVoiceMediaChannel* channel) { bool WebRtcVoiceEngine::StartAecDump(rtc::PlatformFile file, int64_t max_size_bytes) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - FILE* aec_dump_file_stream = rtc::FdopenPlatformFileForWriting(file); - if (!aec_dump_file_stream) { - LOG(LS_ERROR) << "Could not open AEC dump file stream."; - if (!rtc::ClosePlatformFile(file)) - LOG(LS_WARNING) << "Could not close file."; + auto aec_dump = webrtc::AecDumpFactory::Create(file, max_size_bytes, + &low_priority_worker_queue_); + if (!aec_dump) { return false; } - StopAecDump(); - if (apm()->StartDebugRecording(aec_dump_file_stream, max_size_bytes) != - webrtc::AudioProcessing::kNoError) { - LOG_RTCERR0(StartDebugRecording); - fclose(aec_dump_file_stream); - return false; - } - is_dumping_aec_ = true; + apm()->AttachAecDump(std::move(aec_dump)); return true; } void WebRtcVoiceEngine::StartAecDump(const std::string& filename) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - if (!is_dumping_aec_) { - // Start dumping AEC when we are not dumping. - if (apm()->StartDebugRecording(filename.c_str(), -1) != - webrtc::AudioProcessing::kNoError) { - LOG_RTCERR1(StartDebugRecording, filename.c_str()); - } else { - is_dumping_aec_ = true; - } + + auto aec_dump = + webrtc::AecDumpFactory::Create(filename, -1, &low_priority_worker_queue_); + if (aec_dump) { + apm()->AttachAecDump(std::move(aec_dump)); } } void WebRtcVoiceEngine::StopAecDump() { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - if (is_dumping_aec_) { - // Stop dumping AEC when we are dumping. - if (apm()->StopDebugRecording() != webrtc::AudioProcessing::kNoError) { - LOG_RTCERR0(StopDebugRecording); - } - is_dumping_aec_ = false; - } + apm()->DetachAecDump(); } int WebRtcVoiceEngine::CreateVoEChannel() { diff --git a/webrtc/media/engine/webrtcvoiceengine.h b/webrtc/media/engine/webrtcvoiceengine.h index d8466a0b41..bbfec880f8 100644 --- a/webrtc/media/engine/webrtcvoiceengine.h +++ b/webrtc/media/engine/webrtcvoiceengine.h @@ -22,6 +22,7 @@ #include "webrtc/base/constructormagic.h" #include "webrtc/base/networkroute.h" #include "webrtc/base/scoped_ref_ptr.h" +#include "webrtc/base/task_queue.h" #include "webrtc/base/thread_checker.h" #include "webrtc/call/audio_state.h" #include "webrtc/call/call.h" @@ -110,6 +111,9 @@ class WebRtcVoiceEngine final : public webrtc::TraceCallback { void StartAecDump(const std::string& filename); int CreateVoEChannel(); + + rtc::TaskQueue low_priority_worker_queue_; + webrtc::AudioDeviceModule* adm(); webrtc::AudioProcessing* apm(); webrtc::voe::TransmitMixer* transmit_mixer(); diff --git a/webrtc/media/engine/webrtcvoiceengine_unittest.cc b/webrtc/media/engine/webrtcvoiceengine_unittest.cc index 544a4e42b7..98a9101811 100644 --- a/webrtc/media/engine/webrtcvoiceengine_unittest.cc +++ b/webrtc/media/engine/webrtcvoiceengine_unittest.cc @@ -125,6 +125,7 @@ TEST(WebRtcVoiceEngineTestStubLibrary, StartupShutdown) { EXPECT_CALL(apm, ApplyConfig(testing::_)); EXPECT_CALL(apm, SetExtraOptions(testing::_)); EXPECT_CALL(apm, Initialize()).WillOnce(Return(0)); + EXPECT_CALL(apm, DetachAecDump()); StrictMock transmit_mixer; EXPECT_CALL(transmit_mixer, EnableStereoChannelSwapping(false)); cricket::FakeWebRtcVoiceEngine voe(&apm, &transmit_mixer); @@ -163,6 +164,7 @@ class WebRtcVoiceEngineTestFake : public testing::Test { EXPECT_CALL(apm_, ApplyConfig(testing::_)); EXPECT_CALL(apm_, SetExtraOptions(testing::_)); EXPECT_CALL(apm_, Initialize()).WillOnce(Return(0)); + EXPECT_CALL(apm_, DetachAecDump()); // Default Options. EXPECT_CALL(apm_ec_, Enable(true)).WillOnce(Return(0)); EXPECT_CALL(apm_ec_, enable_metrics(true)).WillOnce(Return(0));