From 09e9a83d91a701f9b1aeec5b47cff43e0ff946ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Per=20=C3=85hgren?= Date: Mon, 11 May 2020 11:03:47 +0200 Subject: [PATCH] Change the way that AecDumps are created in APM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL changes the way that AecDumps are created in APM. Instead of being injected, they are now created via the API. This removes the AecDumpFactory from the API surface of APM and makes the API more explicit. The CL will be followed by one more CL that deprecates the usage of the AttachAecDump API also within the audio_processing and the fuzzer folders. The CL also moves the aec_dump.* files from the include folder to the aec_dump folder and changes the build files. The reasons for this are that 1) The content of aec_dump.h is not really part of the API surface of APM. 2) Those files anyway needed to be moved to a separate build- target to avoid a circular build-file dependency caused by the other changes in this CL Bug: webrtc:5298 Change-Id: I7dd6b49de76eb44158472874e1d4ae17dca9be54 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/174750 Commit-Queue: Per Åhgren Reviewed-by: Sam Zackrisson Reviewed-by: Artem Titov Reviewed-by: Mirko Bonadei Cr-Commit-Position: refs/heads/master@{#31207} --- media/engine/webrtc_voice_engine.cc | 10 ++----- modules/audio_processing/BUILD.gn | 26 ++++++++++++++++-- modules/audio_processing/aec_dump/BUILD.gn | 7 ++--- .../audio_processing/audio_processing_impl.cc | 27 +++++++++++++++++++ .../audio_processing/audio_processing_impl.h | 10 +++++++ .../include/audio_processing.h | 22 +++++++++++++++ .../include/mock_audio_processing.h | 12 ++++++--- test/pc/e2e/test_peer_factory.cc | 4 +-- 8 files changed, 100 insertions(+), 18 deletions(-) diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index 47bfa7d812..85c72804c1 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -587,14 +587,8 @@ bool WebRtcVoiceEngine::StartAecDump(webrtc::FileWrapper file, return false; } - auto aec_dump = webrtc::AecDumpFactory::Create( - std::move(file), max_size_bytes, low_priority_worker_queue_.get()); - if (!aec_dump) { - return false; - } - - ap->AttachAecDump(std::move(aec_dump)); - return true; + return ap->CreateAndAttachAecDump(file.Release(), max_size_bytes, + low_priority_worker_queue_.get()); } void WebRtcVoiceEngine::StopAecDump() { diff --git a/modules/audio_processing/BUILD.gn b/modules/audio_processing/BUILD.gn index 86ecbffd6c..7ca78e20b4 100644 --- a/modules/audio_processing/BUILD.gn +++ b/modules/audio_processing/BUILD.gn @@ -50,6 +50,7 @@ rtc_library("api") { "../../rtc_base:macromagic", "../../rtc_base:rtc_base_approved", "../../rtc_base/system:arch", + "../../rtc_base/system:file_wrapper", "../../rtc_base/system:rtc_export", "agc:gain_control_interface", "//third_party/abseil-cpp/absl/types:optional", @@ -112,6 +113,20 @@ rtc_library("high_pass_filter") { ] } +rtc_source_set("aec_dump_interface") { + visibility = [ "*" ] + sources = [ + "include/aec_dump.cc", + "include/aec_dump.h", + ] + + deps = [ + ":api", + ":audio_frame_view", + "../../rtc_base:deprecation", + ] +} + rtc_library("audio_processing") { visibility = [ "*" ] configs += [ ":apm_debug_dump" ] @@ -134,8 +149,6 @@ rtc_library("audio_processing") { "gain_control_impl.h", "gain_controller2.cc", "gain_controller2.h", - "include/aec_dump.cc", - "include/aec_dump.h", "level_estimator.cc", "level_estimator.h", "render_queue_item_verifier.h", @@ -147,6 +160,7 @@ rtc_library("audio_processing") { defines = [] deps = [ + ":aec_dump_interface", ":api", ":apm_logging", ":audio_buffer", @@ -178,6 +192,7 @@ rtc_library("audio_processing") { "../../system_wrappers:field_trial", "../../system_wrappers:metrics", "aec3", + "aec_dump:aec_dump", "aecm:aecm_core", "agc", "agc:gain_control_interface", @@ -198,6 +213,12 @@ rtc_library("audio_processing") { "../../rtc_base:rtc_base_approved", "../../system_wrappers", ] + + if (rtc_enable_protobuf) { + deps += [ "aec_dump:aec_dump_impl" ] + } else { + deps += [ "aec_dump:null_aec_dump_factory" ] + } } rtc_library("voice_detection") { @@ -283,6 +304,7 @@ if (rtc_include_tests) { testonly = true sources = [ "include/mock_audio_processing.h" ] deps = [ + ":aec_dump_interface", ":api", ":audio_buffer", ":audio_processing", diff --git a/modules/audio_processing/aec_dump/BUILD.gn b/modules/audio_processing/aec_dump/BUILD.gn index 7ba3bc08e0..9887f7dcf0 100644 --- a/modules/audio_processing/aec_dump/BUILD.gn +++ b/modules/audio_processing/aec_dump/BUILD.gn @@ -13,7 +13,7 @@ rtc_source_set("aec_dump") { sources = [ "aec_dump_factory.h" ] deps = [ - "../", + "..:aec_dump_interface", "../../../rtc_base:rtc_base_approved", "../../../rtc_base/system:file_wrapper", "../../../rtc_base/system:rtc_export", @@ -29,6 +29,7 @@ if (rtc_include_tests) { ] deps = [ + "..:aec_dump_interface", "..:audioproc_test_utils", "../", "../../../test:test_support", @@ -64,7 +65,7 @@ if (rtc_enable_protobuf) { deps = [ ":aec_dump", - "../", + "..:aec_dump_interface", "../../../api/audio:audio_frame_api", "../../../api/task_queue", "../../../rtc_base:checks", @@ -104,6 +105,6 @@ rtc_library("null_aec_dump_factory") { deps = [ ":aec_dump", - "../", + "..:aec_dump_interface", ] } diff --git a/modules/audio_processing/audio_processing_impl.cc b/modules/audio_processing/audio_processing_impl.cc index 7751bacd91..6abebd2612 100644 --- a/modules/audio_processing/audio_processing_impl.cc +++ b/modules/audio_processing/audio_processing_impl.cc @@ -22,6 +22,7 @@ #include "api/audio/audio_frame.h" #include "common_audio/audio_converter.h" #include "common_audio/include/audio_util.h" +#include "modules/audio_processing/aec_dump/aec_dump_factory.h" #include "modules/audio_processing/agc2/gain_applier.h" #include "modules/audio_processing/audio_buffer.h" #include "modules/audio_processing/common.h" @@ -1532,6 +1533,32 @@ int AudioProcessingImpl::recommended_stream_analog_level() const { } } +bool AudioProcessingImpl::CreateAndAttachAecDump(const std::string& file_name, + int64_t max_log_size_bytes, + rtc::TaskQueue* worker_queue) { + std::unique_ptr aec_dump = + AecDumpFactory::Create(file_name, max_log_size_bytes, worker_queue); + if (!aec_dump) { + return false; + } + + AttachAecDump(std::move(aec_dump)); + return true; +} + +bool AudioProcessingImpl::CreateAndAttachAecDump(FILE* handle, + int64_t max_log_size_bytes, + rtc::TaskQueue* worker_queue) { + std::unique_ptr aec_dump = + AecDumpFactory::Create(handle, max_log_size_bytes, worker_queue); + if (!aec_dump) { + return false; + } + + AttachAecDump(std::move(aec_dump)); + return true; +} + void AudioProcessingImpl::AttachAecDump(std::unique_ptr aec_dump) { RTC_DCHECK(aec_dump); rtc::CritScope cs_render(&crit_render_); diff --git a/modules/audio_processing/audio_processing_impl.h b/modules/audio_processing/audio_processing_impl.h index 188777eb51..3aa86ac5a1 100644 --- a/modules/audio_processing/audio_processing_impl.h +++ b/modules/audio_processing/audio_processing_impl.h @@ -11,8 +11,11 @@ #ifndef MODULES_AUDIO_PROCESSING_AUDIO_PROCESSING_IMPL_H_ #define MODULES_AUDIO_PROCESSING_AUDIO_PROCESSING_IMPL_H_ +#include + #include #include +#include #include #include "api/function_view.h" @@ -70,6 +73,13 @@ class AudioProcessingImpl : public AudioProcessing { int Initialize(const ProcessingConfig& processing_config) override; void ApplyConfig(const AudioProcessing::Config& config) override; void SetExtraOptions(const webrtc::Config& config) override; + bool CreateAndAttachAecDump(const std::string& file_name, + int64_t max_log_size_bytes, + rtc::TaskQueue* worker_queue) override; + bool CreateAndAttachAecDump(FILE* handle, + int64_t max_log_size_bytes, + rtc::TaskQueue* worker_queue) override; + // TODO(webrtc:5298) Deprecated variant. void AttachAecDump(std::unique_ptr aec_dump) override; void DetachAecDump() override; void SetRuntimeSetting(RuntimeSetting setting) override; diff --git a/modules/audio_processing/include/audio_processing.h b/modules/audio_processing/include/audio_processing.h index 25b08c61f5..d84318f2a0 100644 --- a/modules/audio_processing/include/audio_processing.h +++ b/modules/audio_processing/include/audio_processing.h @@ -33,8 +33,13 @@ #include "rtc_base/arraysize.h" #include "rtc_base/deprecation.h" #include "rtc_base/ref_count.h" +#include "rtc_base/system/file_wrapper.h" #include "rtc_base/system/rtc_export.h" +namespace rtc { +class TaskQueue; +} // namespace rtc + namespace webrtc { class AecDump; @@ -600,6 +605,23 @@ class RTC_EXPORT AudioProcessing : public rtc::RefCountInterface { // with this chunk of audio. virtual void set_stream_key_pressed(bool key_pressed) = 0; + // Creates and attaches an webrtc::AecDump for recording debugging + // information. + // The |worker_queue| may not be null and must outlive the created + // AecDump instance. |max_log_size_bytes == -1| means the log size + // will be unlimited. |handle| may not be null. The AecDump takes + // responsibility for |handle| and closes it in the destructor. A + // return value of true indicates that the file has been + // sucessfully opened, while a value of false indicates that + // opening the file failed. + virtual bool CreateAndAttachAecDump(const std::string& file_name, + int64_t max_log_size_bytes, + rtc::TaskQueue* worker_queue) = 0; + virtual bool CreateAndAttachAecDump(FILE* handle, + int64_t max_log_size_bytes, + rtc::TaskQueue* worker_queue) = 0; + + // TODO(webrtc:5298) Deprecated variant. // Attaches provided webrtc::AecDump for recording debugging // information. Log file and maximum file size logic is supposed to // be handled by implementing instance of AecDump. Calling this diff --git a/modules/audio_processing/include/mock_audio_processing.h b/modules/audio_processing/include/mock_audio_processing.h index 9492a38cd2..bdae99a91a 100644 --- a/modules/audio_processing/include/mock_audio_processing.h +++ b/modules/audio_processing/include/mock_audio_processing.h @@ -13,7 +13,6 @@ #include -#include "modules/audio_processing/audio_buffer.h" #include "modules/audio_processing/include/aec_dump.h" #include "modules/audio_processing/include/audio_processing.h" #include "modules/audio_processing/include/audio_processing_statistics.h" @@ -128,8 +127,15 @@ class MockAudioProcessing : public ::testing::NiceMock { MOCK_CONST_METHOD0(delay_offset_ms, int()); MOCK_METHOD1(set_stream_analog_level, void(int)); MOCK_CONST_METHOD0(recommended_stream_analog_level, int()); - - virtual void AttachAecDump(std::unique_ptr aec_dump) {} + MOCK_METHOD3(CreateAndAttachAecDump, + bool(const std::string& file_name, + int64_t max_log_size_bytes, + rtc::TaskQueue* worker_queue)); + MOCK_METHOD3(CreateAndAttachAecDump, + bool(FILE* handle, + int64_t max_log_size_bytes, + rtc::TaskQueue* worker_queue)); + MOCK_METHOD1(AttachAecDump, void(std::unique_ptr)); MOCK_METHOD0(DetachAecDump, void()); MOCK_METHOD0(GetStatistics, AudioProcessingStats()); diff --git a/test/pc/e2e/test_peer_factory.cc b/test/pc/e2e/test_peer_factory.cc index 2b01d8dbb0..0d08f8e2d0 100644 --- a/test/pc/e2e/test_peer_factory.cc +++ b/test/pc/e2e/test_peer_factory.cc @@ -292,8 +292,8 @@ std::unique_ptr TestPeerFactory::CreateTestPeer( rtc::scoped_refptr audio_processing = webrtc::AudioProcessingBuilder().Create(); if (params->aec_dump_path && audio_processing) { - audio_processing->AttachAecDump( - AecDumpFactory::Create(*params->aec_dump_path, -1, task_queue)); + audio_processing->CreateAndAttachAecDump(*params->aec_dump_path, -1, + task_queue); } rtc::scoped_refptr audio_device_module = CreateAudioDeviceModule(