Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2778783002:
AecDump interface (Closed)
|
Created:
3 years, 8 months ago by aleloi Modified:
3 years, 7 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, Andrew MacDonald, aleloi, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, aluebs-webrtc, bjornv1 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAudioProcessingModule has a feature to make a recording of its
configuration, inputs and outputs over a period of time. It is
activated by AudioProcessing::StartRecording. The data is stored in
binary protobuf format in a specified file. The file IO is, as of
this CL, done from the real-time audio thread.
This CL contains an interface for AecDump, a new APM submodule that
will handle the recordings. Calls to the new interface from the
AudioProcessingModule are added. These calls have no effect, and for a
short while, audio_processing_impl.cc will contain two copies of
recording calls.
The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP
preprocessor define. They still have an effect, while the new ones do
not. In the following CLs, the old recording calls will be removed,
and an implementation of AecDump added.
The reasons for the refactoring is to move file IO operations from the
real-time audio thread, to add a top-level low-priority task queue for
logging tasks like this, to simplify and modularize audio_processing_impl.cc
and remove some of the preprocessor directives. These goals will be
archived by the upcoming CLs. The implementation is in
https://codereview.webrtc.org/2865113002.
BUG=webrtc:7404
Review-Url: https://codereview.webrtc.org/2778783002
Cr-Commit-Position: refs/heads/master@{#18233}
Committed: https://chromium.googlesource.com/external/webrtc/+/868f32f4230140c72a7b7abd6c3a5bb9d7feef67
Patch Set 1 : Implemented most of Karl's suggestions. #
Total comments: 63
Patch Set 2 : Next version; large changes to interface. #
Total comments: 30
Patch Set 3 : Removed factory and empty impl. #
Total comments: 11
Patch Set 4 : Small changes in response to reviewer comments. #
Total comments: 20
Patch Set 5 : vector -> array, split WriteConfigMessage, minor fixes. #
Total comments: 13
Patch Set 6 : Reorder, reduce code duplication, improve readability. #
Total comments: 12
Patch Set 7 : Put capture logging in methods. #
Total comments: 16
Patch Set 8 : New FloatAudioFrame class for passing float** audio. #
Total comments: 15
Patch Set 9 : Renamed new Start/Stop DebugRecording into Attach/Detach AecDump. #
Total comments: 9
Patch Set 10 : Update to changed CaptureStreamInfo interface. #
Total comments: 16
Patch Set 11 : Update comments and naming to reflect new usage. #
Total comments: 8
Patch Set 12 : Fix locks. #
Total comments: 1
Patch Set 13 : Default unique_ptr constructor. #
Total comments: 16
Patch Set 14 : Moved 'forced' config logic from AecDump to AudioProcessingImpl. Plus minor other issues. #
Total comments: 9
Patch Set 15 : Minor changes. #Patch Set 16 : Comment formatting. #Dependent Patchsets: Messages
Total messages: 168 (120 generated)
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_arm on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm/builds/7119) linux_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
Description was changed from ========== Interface CL almost finished BUG=7404 ========== to ========== [WORK IN PROGRESS] AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is done from the real-time audio thread. This CL contains an interface for AecDumper, a new APM submodule that will handle the recordings. It also contains an empty implementation of this interface. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc contains two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDumper added. An almost finished version of the complete implementation is here: https://codereview.webrtc.org/2747123007/ BUG=7404 ==========
Description was changed from ========== [WORK IN PROGRESS] AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is done from the real-time audio thread. This CL contains an interface for AecDumper, a new APM submodule that will handle the recordings. It also contains an empty implementation of this interface. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc contains two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDumper added. An almost finished version of the complete implementation is here: https://codereview.webrtc.org/2747123007/ BUG=7404 ========== to ========== [WORK IN PROGRESS] AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is done from the real-time audio thread. This CL contains an interface for AecDumper, a new APM submodule that will handle the recordings. It also contains an empty implementation of this interface. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc contains two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDumper added. An working and near-final version of the complete implementation is here: https://codereview.webrtc.org/2747123007/ BUG=7404 ==========
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== [WORK IN PROGRESS] AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is done from the real-time audio thread. This CL contains an interface for AecDumper, a new APM submodule that will handle the recordings. It also contains an empty implementation of this interface. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc contains two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDumper added. An working and near-final version of the complete implementation is here: https://codereview.webrtc.org/2747123007/ BUG=7404 ========== to ========== [WORK IN PROGRESS] AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is done from the real-time audio thread. This CL contains an interface for AecDumper, a new APM submodule that will handle the recordings. It also contains an empty implementation of this interface. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc contains two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDumper added. An working and near-final version of the complete implementation is here: https://codereview.webrtc.org/2747123007/ BUG=webrtc:7404 ==========
aleloi@webrtc.org changed reviewers: + peah@webrtc.org
Patchset #3 (id:80001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== [WORK IN PROGRESS] AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is done from the real-time audio thread. This CL contains an interface for AecDumper, a new APM submodule that will handle the recordings. It also contains an empty implementation of this interface. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc contains two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDumper added. An working and near-final version of the complete implementation is here: https://codereview.webrtc.org/2747123007/ BUG=webrtc:7404 ========== to ========== [WORK IN PROGRESS] AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs over a period of time. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is done from the real-time audio thread. This CL contains an interface for AecDumper, a new APM submodule that will handle the recordings. It also contains an empty implementation of this interface. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc contains two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDumper added. An working and near-final version of the complete implementation is here: https://codereview.webrtc.org/2747123007/ BUG=webrtc:7404 ==========
PTAL!
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/2778783002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1002: if (apm()->StartDebugRecording(aec_dump_file_stream, max_size_bytes, Could we get rid of these calls on the APM and just have one which sets the AecDumper implementation? Then we could just have one method on WebRtcVoiceEngine too - creating a dumper for either of rtc::PlatformFile or a file name would be handled higher up the stack and we'd not need to know about such details here...
Just checking - will this fix the "io on the audio thread" bug? Great if so. If we can also anticipate having all IO happening on a separate, low priority, task queue then that will make things easier down the line.
Great work! I have some comments though! https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/BUILD.gn (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/BUILD.gn:2: import("../../../webrtc.gni") Should there be a separate build target for this? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/aec_dumper.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:62: class AecDumper { Would it not make sense to at the same time change the name to ApmDumper? aecdump recordings are quite improperly named, as we actually use the to evaluate quite a lot more than just the AEC. I guess the name has stuck for legacy reasons but it would be good to take the opportunity to change that now, at least internally. WDYT? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:65: // the same protobuf message. To facilitate that, a Is really a line break needed here due to the length of the line? If CaptureStreamInfo can be fit into line 65, "Output" will fit into line 66. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:75: virtual void AddInput(std::vector<rtc::ArrayView<const float>> src) = 0; Won't this cause a copy of src? Would it not be better to pass it as a constant reference? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:76: virtual void AddOutput(std::vector<rtc::ArrayView<const float>> src) = 0; See above https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:92: // queue. The file is safely closed when the dtor is called. The Is this the case as it is now used in WebRTC? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:107: virtual void WriteReverseStreamMessage(const AudioFrame& frame) = 0; Would it make sense to change "Reverse" to "Render"? That would match the "Capture" term that is used in WriteCaptureStreamMessage. I think that "Reverse" is a bit ambiguous and not as descriptive as it could be. I personally prefer "Render". WDYT? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:110: std::vector<rtc::ArrayView<const float>> src) = 0; Won't this cause a copy of src? Would it not be better to pass it as a constant reference? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/aec_dumper_unittest.cc (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper_unittest.cc:48: capture_stream_info->AddInput(frame); It would be good to have calls for AddInput/AddOutput(std::vector<rtc::ArrayView<const float>> src) in the test as well. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/no_pb_aec_dumper.cc (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/no_pb_aec_dumper.cc:11: #include "webrtc/modules/audio_processing/aec_dumper/aec_dumper.h" As I see it, there are two alternatives to include "empty" aecdump functionality: 1) Use #if/#endif-s in the code to separate the two cases. 2) Select what code to include from the build-file. I personally prefer 1) as that does not require the reader of the code to read the buildfiles to see what is happening. Furthermore, I have a feeling that it is easier to test for that. Have you considered doing this using 1) instead? What drawbacks/advantages do you see between 1) and 2)? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/null_aec_dumper.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/null_aec_dumper.h:35: class NullAecDumper final : public AecDumper { What is the purpose of the NullAecDumper class? Could you please write a description? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:309: aec_dumper_(AecDumper::CreateNullDumper()), Why cannot we use an empty aecdumper? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:827: std::vector<rtc::ArrayView<const float>> src_view; Afaics, the emplace_back call will cause arrayview objects to dynamically be created in the vector, right? Furthermore, the vector may dynamically change it's size during the loop. On top of that, line 1418 will dynamically allocate the vector data. Is this really necessary? That is a lot of dynamic memory allocation for something that we basically already know the maximum size of. Is there no way that we can use a stack array of a maximum size for this? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:836: // This earlier happened in ProcessCaptureStreamLocked(). Please remove this comment. This is more of a comment to the reviewers, and I don't think it will make any sense to people later reading the code. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:838: public_submodules_->echo_control_mobile->is_enabled())); Why were there DCHECKs moved to here? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:868: dest_view.emplace_back(dest[i], channel_size); Afaics, the emplace_back call will cause arrayview objects to dynamically be created in the vector, right? Furthermore, the vector may dynamically change it's size during the loop. On top of that, line 1418 will dynamically allocate the vector data. Is this really necessary? That is a lot of dynamic memory allocation for something that we basically already know the maximum size of. Is there no way that we can use a stack array of a maximum size for this? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1127: RTC_DCHECK(!(public_submodules_->echo_cancellation->is_enabled() && Why did you move these DCHECKs here? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1424: src_view.emplace_back(src[i], channel_size); Afaics, the emplace_back call will cause arrayview objects to dynamically be created in the vector, right? Furthermore, the vector may dynamically change it's size during the loop. On top of that, line 1418 will dynamically allocate the vector data. Is this really necessary? That is a lot of dynamic memory allocation for something that we basically already know the maximum size of. Is there no way that we can use a stack array of a maximum size for this? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1480: aec_dumper_->WriteReverseStreamMessage(*frame); Does this work? This accesses aec_dumper_ holding the crit_render_lock. But on 1646 aec_dumper is reassigned without holding any lock. I guess it is similar for the crit_capture_lock https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1646: aec_dumper_ = AecDumper::CreateNullDumper(); Why do we need to have a null dumper? Is it not sufficient to have an null (empty) unique pointer? https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:307: // TODO(aleloi) doc. Please change the comment according to the style guidelines https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:308: std::unique_ptr<AecDumper> aec_dumper_; Won't this be accessed concurrently? In that case, it would be good to annotate it with the corresponding lock. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:466: rtc::TaskQueue* worker_queue) = 0; These API changes will probably break quite a number of downstream dependencies.
Some unsolicited comments which you'll love! https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/aec_dumper.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:36: struct InternalAPMConfig { Is it possible to extend the APM::Config to include the information missing here, instead of adding another type? APM::Config could be extended with the information here which is missing, *without* using that data to actually configure the APM. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:62: class AecDumper { On 2017/03/31 07:24:42, peah-webrtc wrote: > Would it not make sense to at the same time change the name to ApmDumper? > aecdump recordings are quite improperly named, as we actually use the to > evaluate quite a lot more than just the AEC. I guess the name has stuck for > legacy reasons but it would be good to take the opportunity to change that now, > at least internally. > > WDYT? Let's keep AecDump for now. 1) The concept is already established when discussing these recordings. 2) In Chrome we have other types of recordings as well (input/output capture). 3) Hopefully, there is a future without a monolithic APM, in which case talking about APM recordings makes as much (or as little) sense as talking about AEC dumps. I'd suggest you call the interface AecDump and drop the "er". https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:94: static std::unique_ptr<AecDumper> Create(std::string file_name, For the reasons we've discussed offline (proto library support) it is better to separate the interface from the implementations/factory methods in this case. The interface should live in audio_processing/include, but the implementations (one build target for each type) lives in a separate folder. The null implementation should probably live with the interface, though I'm not sure in this case there should be one, or if it is better to make the calls to the AECDump implementation conditional in AudioProcessingImpl... hmm. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:107: virtual void WriteReverseStreamMessage(const AudioFrame& frame) = 0; On 2017/03/31 07:24:42, peah-webrtc wrote: > Would it make sense to change "Reverse" to "Render"? That would match the > "Capture" term that is used in WriteCaptureStreamMessage. > > I think that "Reverse" is a bit ambiguous and not as descriptive as it could be. > I personally prefer "Render". > > WDYT? +1! "Reverse" stream makes my head explode. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:70: int StartDebugRecording(const char filename[kMaxFilenameSize], I think we only need one method StartAecDump(std::unique_ptr<AecDump> aec_dump); Send in a valid impl to start dumping, send in a nullptr to stop. Whether the dump is created from a FILE* or a char filename[], whether it uses a TaskQueue or a separate thread, that should be up to the AecDump implementation and disconnected from the APM. This class shouldn't include file_wrapper.h and not debug.bp.h.
https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/aec_dumper.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:62: class AecDumper { On 2017/04/03 14:33:02, the sun wrote: > On 2017/03/31 07:24:42, peah-webrtc wrote: > > Would it not make sense to at the same time change the name to ApmDumper? > > aecdump recordings are quite improperly named, as we actually use the to > > evaluate quite a lot more than just the AEC. I guess the name has stuck for > > legacy reasons but it would be good to take the opportunity to change that > now, > > at least internally. > > > > WDYT? > > Let's keep AecDump for now. > 1) The concept is already established when discussing these recordings. > 2) In Chrome we have other types of recordings as well (input/output capture). > 3) Hopefully, there is a future without a monolithic APM, in which case talking > about APM recordings makes as much (or as little) sense as talking about AEC > dumps. > > I'd suggest you call the interface AecDump and drop the "er". Makes sense. In particular I think 1) is a very important point. Agree to this I do.
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #4 (id:160001) has been deleted
Patchset #5 (id:200001) has been deleted
Patchset #4 (id:180001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19291) linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/21539)
Patchset #4 (id:220001) has been deleted
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
I've responded to the comments and uploaded a new version. PTAL! https://codereview.webrtc.org/2778783002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1002: if (apm()->StartDebugRecording(aec_dump_file_stream, max_size_bytes, On 2017/03/28 22:35:29, the sun wrote: > Could we get rid of these calls on the APM and just have one which sets the > AecDumper implementation? Then we could just have one method on > WebRtcVoiceEngine too - creating a dumper for either of rtc::PlatformFile or a > file name would be handled higher up the stack and we'd not need to know about > such details here... Sure! I've added a new APM::StartDebugRecording method that takes an AecDump. However, I havn't changed anything here and not deprecated the old ones. This CL doesn't contain the implementation of AecDump yet, and StartDebugRecording(AecDump) does nothing. Therefore the old calls are still here. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/BUILD.gn (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/BUILD.gn:2: import("../../../webrtc.gni") On 2017/03/31 07:24:42, peah-webrtc wrote: > Should there be a separate build target for this? I think it's best: it disconnects the APM from the logging, allows different implementations of AecDump, reduces compilation time and allows better use of GN (e.g. more header checks, fine-grained dependencies, check that protobuf must be enabled to use the aec_dumper_impl and that two different AecDump factory impls may not be included at once). https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/aec_dumper.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:36: struct InternalAPMConfig { On 2017/04/03 14:33:02, the sun wrote: > Is it possible to extend the APM::Config to include the information missing > here, instead of adding another type? APM::Config could be extended with the > information here which is missing, *without* using that data to actually > configure the APM. Probably. For some reason, I thought we were moving away from the template based webrtc::Config. We would have to add twice as many ConfigOptionsIDs. I don't know if that's a good idea: setting the new configuration variables in webrtc::Config would not have any effect when configuring/creating APM. That would complicate a public interface. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:62: class AecDumper { On 2017/04/05 04:41:50, peah-webrtc wrote: > On 2017/04/03 14:33:02, the sun wrote: > > On 2017/03/31 07:24:42, peah-webrtc wrote: > > > Would it not make sense to at the same time change the name to ApmDumper? > > > aecdump recordings are quite improperly named, as we actually use the to > > > evaluate quite a lot more than just the AEC. I guess the name has stuck for > > > legacy reasons but it would be good to take the opportunity to change that > > now, > > > at least internally. > > > > > > WDYT? > > > > Let's keep AecDump for now. > > 1) The concept is already established when discussing these recordings. > > 2) In Chrome we have other types of recordings as well (input/output capture). > > 3) Hopefully, there is a future without a monolithic APM, in which case > talking > > about APM recordings makes as much (or as little) sense as talking about AEC > > dumps. > > > > I'd suggest you call the interface AecDump and drop the "er". > > Makes sense. In particular I think 1) is a very important point. > > Agree to this I do. Changed to AecDump. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:65: // the same protobuf message. To facilitate that, a On 2017/03/31 07:24:42, peah-webrtc wrote: > Is really a line break needed here due to the length of the line? If > CaptureStreamInfo can be fit into line 65, "Output" will fit into line 66. Done. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:75: virtual void AddInput(std::vector<rtc::ArrayView<const float>> src) = 0; On 2017/03/31 07:24:42, peah-webrtc wrote: > Won't this cause a copy of src? Would it not be better to pass it as a constant > reference? Done. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:76: virtual void AddOutput(std::vector<rtc::ArrayView<const float>> src) = 0; On 2017/03/31 07:24:42, peah-webrtc wrote: > See above Done. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:92: // queue. The file is safely closed when the dtor is called. The On 2017/03/31 07:24:42, peah-webrtc wrote: > Is this the case as it is now used in WebRTC? I think using a single file / AecDump instance makes the interface simpler. That a task queue must outlive objects that post on it is assumed in other places in WebRTC. I've removed mentions of files, queues and handles from this interface. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:94: static std::unique_ptr<AecDumper> Create(std::string file_name, On 2017/04/03 14:33:02, the sun wrote: > For the reasons we've discussed offline (proto library support) it is better to > separate the interface from the implementations/factory methods in this case. > The interface should live in audio_processing/include, but the implementations > (one build target for each type) lives in a separate folder. > > The null implementation should probably live with the interface, though I'm not > sure in this case there should be one, or if it is better to make the calls to > the AECDump implementation conditional in AudioProcessingImpl... hmm. Done, I think. I prefer having a null implementation to conditionally making calls on a pointer: less logic in audio_processing_impl, less branches and less potential for pointer errors. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:107: virtual void WriteReverseStreamMessage(const AudioFrame& frame) = 0; On 2017/04/03 14:33:02, the sun wrote: > On 2017/03/31 07:24:42, peah-webrtc wrote: > > Would it make sense to change "Reverse" to "Render"? That would match the > > "Capture" term that is used in WriteCaptureStreamMessage. > > > > I think that "Reverse" is a bit ambiguous and not as descriptive as it could > be. > > I personally prefer "Render". > > > > WDYT? > > +1! "Reverse" stream makes my head explode. Done. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:110: std::vector<rtc::ArrayView<const float>> src) = 0; On 2017/03/31 07:24:42, peah-webrtc wrote: > Won't this cause a copy of src? Would it not be better to pass it as a constant > reference? Done. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/aec_dumper_unittest.cc (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper_unittest.cc:48: capture_stream_info->AddInput(frame); On 2017/03/31 07:24:42, peah-webrtc wrote: > It would be good to have calls for > AddInput/AddOutput(std::vector<rtc::ArrayView<const float>> src) in the test as > well. Done. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/no_pb_aec_dumper.cc (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/no_pb_aec_dumper.cc:11: #include "webrtc/modules/audio_processing/aec_dumper/aec_dumper.h" On 2017/03/31 07:24:43, peah-webrtc wrote: > As I see it, there are two alternatives to include "empty" aecdump > functionality: > 1) Use #if/#endif-s in the code to separate the two cases. > 2) Select what code to include from the build-file. > > I personally prefer 1) as that does not require the reader of the code to read > the buildfiles to see what is happening. Furthermore, I have a feeling that it > is easier to test for that. > > Have you considered doing this using 1) instead? What drawbacks/advantages do > you see between 1) and 2)? I would almost always prefer virtual functions to the pre-processor. Advantages are (specific to this case): less logic in one place (audio_processing_impl), it forces to think about abstractions and division of responsibility. IMHO, it is actually *easier* to read and to test, because the interface can be mocked. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/null_aec_dumper.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/null_aec_dumper.h:35: class NullAecDumper final : public AecDumper { On 2017/03/31 07:24:43, peah-webrtc wrote: > What is the purpose of the NullAecDumper class? Could you please write a > description? Done. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:309: aec_dumper_(AecDumper::CreateNullDumper()), On 2017/03/31 07:24:43, peah-webrtc wrote: > Why cannot we use an empty aecdumper? It's possible. Then we would have to add 'if (aec_dumper_)' checks everywhere. This way there is an invariant: aec_dumper_ is never empty. There are no special cases and the code is slightly simpler in this file. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:827: std::vector<rtc::ArrayView<const float>> src_view; On 2017/03/31 07:24:43, peah-webrtc wrote: > Afaics, the emplace_back call will cause arrayview objects to dynamically be > created in the vector, right? Furthermore, the vector may dynamically change > it's size during the loop. On top of that, line 1418 will dynamically allocate > the vector data. > > Is this really necessary? That is a lot of dynamic memory allocation for > something that we basically already know the maximum size of. Is there no way > that we can use a stack array of a maximum size for this? We could use a static array of large enough size. Or reserve vector size in declaration of src_view. I profiled (link below) and found that this part doesn't have a noticeable impact on performance. Perhaps leave it as simple and readable as possible Profiling report: no aec processing but resampling and aec-dump recording https://drive.google.com/open?id=0B0ohpnzd-DTsVHBta0t6UGJObTQ https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:836: // This earlier happened in ProcessCaptureStreamLocked(). On 2017/03/31 07:24:43, peah-webrtc wrote: > Please remove this comment. This is more of a comment to the reviewers, and I > don't think it will make any sense to people later reading the code. Done. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:838: public_submodules_->echo_control_mobile->is_enabled())); On 2017/03/31 07:24:43, peah-webrtc wrote: > Why were there DCHECKs moved to here? The conditions were DCHECKed before the protobuf stream object was filled with delay, drift, etc. That was done in ProcessCaptureStreamLocked. To avoid sending the CaptureStreamInfo object to ProcessCaptureStreamLocked, it was moved here. The DCHECKS were copied here to ensure everything is executed in the same order as before this change. I think it should be safe to leave them in ProcessCaptureStreamLocked(). https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:868: dest_view.emplace_back(dest[i], channel_size); On 2017/03/31 07:24:43, peah-webrtc wrote: > Afaics, the emplace_back call will cause arrayview objects to dynamically be > created in the vector, right? Furthermore, the vector may dynamically change > it's size during the loop. On top of that, line 1418 will dynamically allocate > the vector data. > > Is this really necessary? That is a lot of dynamic memory allocation for > something that we basically already know the maximum size of. Is there no way > that we can use a stack array of a maximum size for this? Acknowledged. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1127: RTC_DCHECK(!(public_submodules_->echo_cancellation->is_enabled() && On 2017/03/31 07:24:43, peah-webrtc wrote: > Why did you move these DCHECKs here? Same as the DCHECKs above. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1424: src_view.emplace_back(src[i], channel_size); On 2017/03/31 07:24:43, peah-webrtc wrote: > Afaics, the emplace_back call will cause arrayview objects to dynamically be > created in the vector, right? Furthermore, the vector may dynamically change > it's size during the loop. On top of that, line 1418 will dynamically allocate > the vector data. > > Is this really necessary? That is a lot of dynamic memory allocation for > something that we basically already know the maximum size of. Is there no way > that we can use a stack array of a maximum size for this? Acknowledged. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1480: aec_dumper_->WriteReverseStreamMessage(*frame); On 2017/03/31 07:24:43, peah-webrtc wrote: > Does this work? This accesses aec_dumper_ holding the crit_render_lock. But on > 1646 aec_dumper is reassigned without holding any lock. I guess it is similar > for the crit_capture_lock Good point. Accesses to AecDump are thread safe, but accesses to the unique_ptr are not. Holding a lock when reassigning to aec_dump_ is enough. I've changed that. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1646: aec_dumper_ = AecDumper::CreateNullDumper(); On 2017/03/31 07:24:43, peah-webrtc wrote: > Why do we need to have a null dumper? Is it not sufficient to have an null > (empty) unique pointer? It is. I can remove the null dumper if there are sufficiently many voices for that. There is one reason for the null dumper (not relevant at this line): it allows to define a null factory, which can be included by the build system when protobuf support is off. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:70: int StartDebugRecording(const char filename[kMaxFilenameSize], On 2017/04/03 14:33:02, the sun wrote: > I think we only need one method > > StartAecDump(std::unique_ptr<AecDump> aec_dump); > > Send in a valid impl to start dumping, send in a nullptr to stop. Whether the > dump is created from a FILE* or a char filename[], whether it uses a TaskQueue > or a separate thread, that should be up to the AecDump implementation and > disconnected from the APM. This class shouldn't include file_wrapper.h and not > debug.bp.h. Good idea! I can't get rid of the other StartDebugRecordings right away, because there are downstream dependencies and because this CL doesn't include the AecDump implementation. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:307: // TODO(aleloi) doc. On 2017/03/31 07:24:43, peah-webrtc wrote: > Please change the comment according to the style guidelines Done. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:308: std::unique_ptr<AecDumper> aec_dumper_; On 2017/03/31 07:24:43, peah-webrtc wrote: > Won't this be accessed concurrently? In that case, it would be good to annotate > it with the corresponding lock. The implementation is actually thread-safe. The methods construct pb-objects without changing state. Then pb-objects are then sent to the task queue, which is safe to do from different threads. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:466: rtc::TaskQueue* worker_queue) = 0; On 2017/03/31 07:24:43, peah-webrtc wrote: > These API changes will probably break quite a number of downstream dependencies. I've added a new one as suggested by solenberg@ instead of changing the old ones.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Quick reply to clear up a misunderstanding. I'll take a closer look... https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dumper/aec_dumper.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dumper/aec_dumper.h:36: struct InternalAPMConfig { On 2017/04/06 15:46:11, aleloi wrote: > On 2017/04/03 14:33:02, the sun wrote: > > Is it possible to extend the APM::Config to include the information missing > > here, instead of adding another type? APM::Config could be extended with the > > information here which is missing, *without* using that data to actually > > configure the APM. > Probably. For some reason, I thought we were moving away from the template based > webrtc::Config. We would have to add twice as many ConfigOptionsIDs. I don't > know if that's a good idea: setting the new configuration variables in > webrtc::Config would not have any effect when configuring/creating APM. That > would complicate a public interface. Oh, misunderstanding. Not the template based global config. Please help us obliterate that one. There's a simple struct in APM which is currently used to set up HPF and newer components: https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud... Ideally we'd get rid of all the component-specific interfaces and only set up the APM with this config.
Thanks for the new patch. I have some further comments. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:309: aec_dumper_(AecDumper::CreateNullDumper()), On 2017/04/06 15:46:11, aleloi wrote: > On 2017/03/31 07:24:43, peah-webrtc wrote: > > Why cannot we use an empty aecdumper? > > It's possible. Then we would have to add 'if (aec_dumper_)' checks everywhere. > This way there is an invariant: aec_dumper_ is never empty. There are no special > cases and the code is slightly simpler in this file. Absolutely, I definitely like the simpler code. But is comes at quite a large cost. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:827: std::vector<rtc::ArrayView<const float>> src_view; On 2017/04/06 15:46:11, aleloi wrote: > On 2017/03/31 07:24:43, peah-webrtc wrote: > > Afaics, the emplace_back call will cause arrayview objects to dynamically be > > created in the vector, right? Furthermore, the vector may dynamically change > > it's size during the loop. On top of that, line 1418 will dynamically allocate > > the vector data. > > > > Is this really necessary? That is a lot of dynamic memory allocation for > > something that we basically already know the maximum size of. Is there no way > > that we can use a stack array of a maximum size for this? > > We could use a static array of large enough size. Or reserve vector size in > declaration of src_view. > > I profiled (link below) and found that this part doesn't have a noticeable > impact on performance. Perhaps leave it as simple and readable as possible > > Profiling report: no aec processing but resampling and aec-dump recording > https://drive.google.com/open?id=0B0ohpnzd-DTsVHBta0t6UGJObTQ Yes, the impact of this is definitely not noticeable. I still think it makes sense to as a rule not add dynamic allocations if it is not needed. Some sources state that a heap allocation costs about 200-500 cycles, compared to 0 cycles for an array. I'm not sure about the cost. However, in this case we actually know the number of channels, as well as the maximum possible number of channels so any cost required for this is actually not necessary. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:838: public_submodules_->echo_control_mobile->is_enabled())); On 2017/04/06 15:46:11, aleloi wrote: > On 2017/03/31 07:24:43, peah-webrtc wrote: > > Why were there DCHECKs moved to here? > > The conditions were DCHECKed before the protobuf stream object was filled with > delay, drift, etc. That was done in ProcessCaptureStreamLocked. To avoid sending > the CaptureStreamInfo object to ProcessCaptureStreamLocked, it was moved here. > The DCHECKS were copied here to ensure everything is executed in the same order > as before this change. I think it should be safe to leave them in > ProcessCaptureStreamLocked(). I agree, it should be safe to leave them there. In case there is a DCHECK, the recording will regardless be halted and incomplete. Having one or more objects sent for recording does not matter in that case. Please move these back to where they were. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h (right): https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h:29: static std::unique_ptr<AecDump> Create(std::string file_name, Why is a class needed for this? Or is more functionality to be added? https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:820: std::unique_ptr<AecDump::CaptureStreamInfo> stream_info = I definitely like the aspect of the NullImplementation that it does not require conditionals. The extra cost is, however, quite significant. Between lines 820 and 870, for the case where no aecdumping is active this variant actually adds: -9 virtual function calls -6 ordinary function calls -2 copies of all the data in the capture buffer. Only the function calls are about 700 cycles. Some of these can maybe be inlined by the compiler but as there still are the extra copies of data I don't think this overhead warrants the not having to use conditionals. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1109: aec_dump_->GetCaptureStreamInfo(); Same as above. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1417: const size_t channel_size = Same as above. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1589: aec_dump_->WriteInitMessage(formats_.api_format); Is this needed? Nothing is anyway written until the new StartDebugRecording is called. And at that point, the init message is written as well. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1617: aec_dump_->WriteInitMessage(formats_.api_format); Same as above. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:94: // the flow of messages, some will be silently dropped. I'm not that happy that messages will be dropped. In particular, it is not at all good that that happens silently. If the aecdump recordings are to be used as a tool for debugging issues on platforms, they must be reliable. Otherwise this will just cause another reason of doubt for what caused an issue. What is the reason that they need to be dropped? https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:95: virtual void WriteInitMessage(const ProcessingConfig& api_format) = 0; Even though the ProcessingConfig class contains all the information needed, it is fairly heavily based on StreamConfig. I'm not sure that we will be wanting to maintain either ProcessingConfig nor StreamConfig in the long run. Would it be possible to implement this without adding further usage of ProcessingConfig? Or do you think that it will be easy to refactor this if we want to remove/change ProcessingConfig in the near future? https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:100: std::vector<rtc::ArrayView<const float>> src) = 0; This call will copy a vector. Is that desired? Would it be possible to pass it as a const-ref? https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:111: class NullCaptureStreamInfo final : public AecDump::CaptureStreamInfo { Where is this classed used? https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:131: std::unique_ptr<CaptureStreamInfo> GetCaptureStreamInfo() override; This should also need an implementation, right? https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:133: void WriteInitMessage(const ProcessingConfig& api_format) override{}; I assume that this method, as well as the ones below, would be inlined by the compiler to a no-op? E.g., there would be no method call overhead in practice? How does this work when the call is virtual? https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:138: std::vector<rtc::ArrayView<const float>> src) override{}; This call will copy a vector. Is that desired? Would it be possible to pass it as a const-ref? https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:456: // called. The d-tor call may block until all pending logging tasks Is the blocking needed because the taskqueue must be destroyed as well? https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/mock_audio_processing.h (right): https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/mock_audio_processing.h:212: virtual void StartDebugRecording(std::unique_ptr<AecDump> aec_dump) {} Why don't we want to mock this method similarly to the other StartDebugRecording methods?
Patchset #5 (id:260001) has been deleted
Patchset #5 (id:280001) has been deleted
New patch set and responses to comments. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:309: aec_dumper_(AecDumper::CreateNullDumper()), On 2017/04/07 12:57:15, peah-webrtc wrote: > On 2017/04/06 15:46:11, aleloi wrote: > > On 2017/03/31 07:24:43, peah-webrtc wrote: > > > Why cannot we use an empty aecdumper? > > > > It's possible. Then we would have to add 'if (aec_dumper_)' checks everywhere. > > This way there is an invariant: aec_dumper_ is never empty. There are no > special > > cases and the code is slightly simpler in this file. > > Absolutely, I definitely like the simpler code. But is comes at quite a large > cost. I checked: virtual calls are always generated in release-mode, even when rtc_enable_protobuf is false. Now null dump is replaced with nullptr. https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2778783002/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:308: std::unique_ptr<AecDumper> aec_dumper_; I've added locks for writing to aec_dump_, and an AecDump instance by itself is thread safe. I don't think the code is safe as is. Suppose StopRecording is called. Locks are aquired and aec_dump_ is destroyed. Suppose another thread is inside an unlocked part of ProcessStream(). It has unguarded 'if (aec_dump_)' blocks, which is a data race. I will fix that somehow in the next patch set. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h (right): https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h:29: static std::unique_ptr<AecDump> Create(std::string file_name, On 2017/04/07 12:57:15, peah-webrtc wrote: > Why is a class needed for this? Or is more functionality to be added? It's gone now, because this CL doesn't create real AecDump instances. I added it originally to not add more ::Create methods to the webrtc namespace. The choice was between new namespace or static methods in a new class. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:820: std::unique_ptr<AecDump::CaptureStreamInfo> stream_info = On 2017/04/07 12:57:15, peah-webrtc wrote: > I definitely like the aspect of the NullImplementation that it does not require > conditionals. The extra cost is, however, quite significant. > > Between lines 820 and 870, for the case where no aecdumping is active > this variant actually adds: > -9 virtual function calls > -6 ordinary function calls > -2 copies of all the data in the capture buffer. > Only the function calls are about 700 cycles. Some of these can maybe be inlined > by the compiler but as there still are the extra copies of data I don't think > this overhead warrants the not having to use conditionals. I agree. Now everything is wrapped with if (aec_dump_) https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1109: aec_dump_->GetCaptureStreamInfo(); On 2017/04/07 12:57:15, peah-webrtc wrote: > Same as above. Acknowledged. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1417: const size_t channel_size = On 2017/04/07 12:57:15, peah-webrtc wrote: > Same as above. Acknowledged. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1589: aec_dump_->WriteInitMessage(formats_.api_format); On 2017/04/07 12:57:15, peah-webrtc wrote: > Is this needed? Nothing is anyway written until the new StartDebugRecording is > called. And at that point, the init message is written as well. Good point. Removed. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1617: aec_dump_->WriteInitMessage(formats_.api_format); On 2017/04/07 12:57:15, peah-webrtc wrote: > Same as above. Acknowledged. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:94: // the flow of messages, some will be silently dropped. On 2017/04/07 12:57:16, peah-webrtc wrote: > I'm not that happy that messages will be dropped. In particular, it is not at > all good that that happens silently. If the aecdump recordings are to be used as > a tool for debugging issues on platforms, they must be reliable. Otherwise this > will just cause another reason of doubt for what caused an issue. > > What is the reason that they need to be dropped? Sorry, missed to update the comment after I removed the EventPool (a pool of protobuf objects). Now log requests will be put on a task queue, but not be dropped. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:95: virtual void WriteInitMessage(const ProcessingConfig& api_format) = 0; On 2017/04/07 12:57:16, peah-webrtc wrote: > Even though the ProcessingConfig class contains all the information needed, it > is fairly heavily based on StreamConfig. I'm not sure that we will be wanting to > maintain either ProcessingConfig nor StreamConfig in the long run. > > Would it be possible to implement this without adding further usage of > ProcessingConfig? Or do you think that it will be easy to refactor this if we > want to remove/change ProcessingConfig in the near future? I have added a new type similar to InternalAPMConfig. Apart from not having to maintain ProcessConfig/StreamConfig, there are other benefits: * consistent naming (no more reverse_input) * no dependency on audio_processing.h in aec_dump.h, which makes the cyclic include check happy https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:100: std::vector<rtc::ArrayView<const float>> src) = 0; On 2017/04/07 12:57:16, peah-webrtc wrote: > This call will copy a vector. Is that desired? Would it be possible to pass it > as a const-ref? I have changed this to take the value by const& exactly as AddInput in CaptureStreamInfo. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:111: class NullCaptureStreamInfo final : public AecDump::CaptureStreamInfo { On 2017/04/07 12:57:16, peah-webrtc wrote: > Where is this classed used? It was previously used for AecDumps that did nothing and in the unit test. It has now been deleted. The unit test will use a real and possibly a mock AecDump after the impl has been uploaded. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:131: std::unique_ptr<CaptureStreamInfo> GetCaptureStreamInfo() override; On 2017/04/07 12:57:16, peah-webrtc wrote: > This should also need an implementation, right? It was previously in aec_dump.cc. Now it's removed. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:133: void WriteInitMessage(const ProcessingConfig& api_format) override{}; On 2017/04/07 12:57:15, peah-webrtc wrote: > I assume that this method, as well as the ones below, would be inlined by the > compiler to a no-op? E.g., there would be no method call overhead in practice? > How does this work when the call is virtual? I checked by compiling in release-mode and disassembling and stepping with gdb. The methods were not inlined. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:138: std::vector<rtc::ArrayView<const float>> src) override{}; On 2017/04/07 12:57:16, peah-webrtc wrote: > This call will copy a vector. Is that desired? Would it be possible to pass it > as a const-ref? Changed. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:456: // called. The d-tor call may block until all pending logging tasks On 2017/04/07 12:57:16, peah-webrtc wrote: > Is the blocking needed because the taskqueue must be destroyed as well? The destructor of the AecDump impl uses this pattern: rtc::Event thread_sync_event(false /* manual_reset */, false); worker_queue_->PostTask([&thread_sync_event] { thread_sync_event.Set(); }); thread_sync_event.Wait(rtc::Event::kForever); The 3rd line blocks the task in the 2nd line has been run. The queue is FIFO, so all pending tasks must be done first. This pattern is used in other places, e.g. AudioSendStream::Stop(), VideoSendStream(). It simplifies resource handling with task queues: the member variables of AecDump may only be deallocated when there are no queued tasks with pointers to them. The task queue itself is not destroyed. https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/mock_audio_processing.h (right): https://codereview.webrtc.org/2778783002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/mock_audio_processing.h:212: virtual void StartDebugRecording(std::unique_ptr<AecDump> aec_dump) {} On 2017/04/07 12:57:16, peah-webrtc wrote: > Why don't we want to mock this method similarly to the other StartDebugRecording > methods? Mocked methods can't have move-only arguments. If we want to check that StartDebugRecording is called in a unit test, it's possible to add a mocked method like this: MOCK_METHOD1(StartDebugRecordingMock, void(AecDump* dump)); and call it in the non-mocked StartDebugRecording.
Looking pretty good. Can we get rid of the Internal... structs? https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:863: if (aec_dump_) { Looks like you can fold this conditional section with the 2 above. CopyFrom() on the audio buffer should not affect any of the below flags. https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:887: if (aec_dump_) { if (stream_info) { ...otherwise the contract is that GetCaptureStreamInfo() may never fail. https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1155: if (aec_dump_) { Fold with the conditional on line 1136 https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:30: struct InternalAPMConfig { If this is really internal, why does it live in the top namespace? Is there no way we can get rid of this, and the associated conversions? https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/mock_audio_processing.h (right): https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/mock_audio_processing.h:212: virtual void StartDebugRecording(std::unique_ptr<AecDump> aec_dump) {} nit: maintain order of class declaration
Patchset #6 (id:320001) has been deleted
Thanks for the comments! I've made some small changes. I think it will be difficult to remove the Internal... structs without putting back the #if PROTOBUF guards in audio_processing_impl. The data in the Internal... structs is collected from several sub-modules of APM. https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:863: if (aec_dump_) { On 2017/04/18 08:46:12, the sun wrote: > Looks like you can fold this conditional section with the 2 above. CopyFrom() on > the audio buffer should not affect any of the below flags. Done. Thanks for spotting this! https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:887: if (aec_dump_) { On 2017/04/18 08:46:12, the sun wrote: > if (stream_info) { > > ...otherwise the contract is that GetCaptureStreamInfo() may never fail. That should be the case now that there is no Null impl. I've added a DCHECK. https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1155: if (aec_dump_) { On 2017/04/18 08:46:12, the sun wrote: > Fold with the conditional on line 1136 Done. https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:30: struct InternalAPMConfig { On 2017/04/18 08:46:12, the sun wrote: > If this is really internal, why does it live in the top namespace? > > Is there no way we can get rid of this, and the associated conversions? I don't think so, because the data is spread out over different APM sub-modules. With the current solution, this header doesn't have to depend on audio_processing.h. An alternative would be to move the struct declaration there (to audio_processing.h) and add back the dependency. It can be called something else, though. Perhaps ApmConfigForAecDump? https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:58: struct InternalAPMStreamsConfig { Rename into ApmStreamsConfigForAecDump? https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/mock_audio_processing.h (right): https://codereview.webrtc.org/2778783002/diff/300001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/mock_audio_processing.h:212: virtual void StartDebugRecording(std::unique_ptr<AecDump> aec_dump) {} On 2017/04/18 08:46:12, the sun wrote: > nit: maintain order of class declaration Done.
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've looked carefully at the locking scheme. It's safe as it is now: * The aec_dump pointer is only changed in StartDebugRecording after holding both locks. * aec_dump is only accessed when holding either crit_render or crit_capture. Since AecDump itself is thread safe, method calls need not be guarded. There seems to be now way to express this invariant with locking annotations. I will see if it's possible to add a test that changes aec_dump_ concurrently by calling StartDebugRecording while accessing ProcessStream/ProcessReverseStream from another thread. The race condition detection tools should detect possible issues then.
Great! Thanks for the new version. I have added some comments. PTAL. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:848: if (aec_dump_) { Why not merge the if-statements on 848 and 852? https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:868: It would probably make sense to have the empty line before 867 instead of after, as 867 is related to 869 and 870. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:891: dest_view.emplace_back(dest[i], channel_size); This code construct using a vector causes heap allocation, and possible vector reallocation. That happens for each frame. I'd rather prefer using an array std::array<rtc::ArrayView<const float>, kMaxNumChannels> dest_view; for (...) { dest_view[i] = rtc::ArrayView<const float>(dest[i], channel_size); } WDYT? https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1159: Remove empty line. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1165: stream_info->AddOutput(*frame); DCHECK on stream_info? https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1455: i < formats_.api_format.reverse_input_stream().num_channels(); ++i) { See above comment about heap allocation https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1607: const int error = WriteConfigMessage(true); It would be nice to avoid having the #ifdef-endif around the new aec-dump functionality. Would it be possible to split WriteConfigMessage allready now so that the new aecdump code can be written without #ifdef-endif's ? https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1688: aec_dump_ = nullptr; aec_dump_.reset()? https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:33: InternalAPMConfig(const InternalAPMConfig&) = delete; Is it not possible to use the functionality in constructor_magic.h for this? https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:98: static std::unique_ptr<AecDump> CreateNullDump(); Is this still needed?
Patchset #7 (id:360001) has been deleted
Patchset #7 (id:380001) has been deleted
Patchset #7 (id:400001) has been deleted
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Thanks for the comments, here is a new version! https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:848: if (aec_dump_) { On 2017/04/19 12:30:29, peah-webrtc wrote: > Why not merge the if-statements on 848 and 852? Oh, I thought I did that... Done! Although a bit on the verbose side. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:868: On 2017/04/19 12:30:29, peah-webrtc wrote: > It would probably make sense to have the empty line before 867 instead of after, > as 867 is related to 869 and 870. Done. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:891: dest_view.emplace_back(dest[i], channel_size); On 2017/04/19 12:30:29, peah-webrtc wrote: > This code construct using a vector causes heap allocation, and possible vector > reallocation. That happens for each frame. > > I'd rather prefer using an array > std::array<rtc::ArrayView<const float>, kMaxNumChannels> dest_view; > for (...) { > dest_view[i] = rtc::ArrayView<const float>(dest[i], channel_size); > } > > WDYT? Done. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1159: On 2017/04/19 12:30:29, peah-webrtc wrote: > Remove empty line. Done. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1165: stream_info->AddOutput(*frame); On 2017/04/19 12:30:29, peah-webrtc wrote: > DCHECK on stream_info? It's done just after stream_info is assigned to on 1136. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1455: i < formats_.api_format.reverse_input_stream().num_channels(); ++i) { On 2017/04/19 12:30:29, peah-webrtc wrote: > See above comment about heap allocation Done. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1607: const int error = WriteConfigMessage(true); On 2017/04/19 12:30:29, peah-webrtc wrote: > It would be nice to avoid having the #ifdef-endif around the new aec-dump > functionality. > > Would it be possible to split WriteConfigMessage allready now so that the new > aecdump code can be written without #ifdef-endif's ? Yes, I've made an aec-dump-only vesrion of WriteConfigMessage. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1688: aec_dump_ = nullptr; On 2017/04/19 12:30:29, peah-webrtc wrote: > aec_dump_.reset()? Done. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:33: InternalAPMConfig(const InternalAPMConfig&) = delete; On 2017/04/19 12:30:29, peah-webrtc wrote: > Is it not possible to use the functionality in constructor_magic.h for this? RTC_DISABLE only works for the non-move versions. I got the impression from our C++ gurus that we are discouraged from using the macro on new code, because * it's a macro * it's non-standard (not immediately recognizable for non-webrtc devs) * it's a pre-c++11 trick that's not needed now we can use C++11. https://codereview.webrtc.org/2778783002/diff/340001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:98: static std::unique_ptr<AecDump> CreateNullDump(); On 2017/04/19 12:30:29, peah-webrtc wrote: > Is this still needed? No. Now removed, thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/24867)
Patchset #7 (id:420001) has been deleted
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi, Thanks for the patch! Looks great! I have some more comments though. https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:133: constexpr size_t kMaxNumChannels = 2; To be on the safe side, could we set this to 4 (I've seen that in one recording). I'm not sure whether the rest of the code can cope with that though. Also, as this still applies only to the recording, maybe the naming could reflect that? e.g,, kMaxNumChannelsToRecord WDYT? https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:852: aec_dump_->WriteConfig(CollectApmConfig(), false); Would it be ok to move this code block, and the other similar code blocks, to separate member methods. It is quite nice not to have the recording functionality clutter the rest of the code. The ability to avoid this is a huge gain with this CL, and it would be nice to go as far as possible with that. WDYT? That is definitely something that could be done in another CL as well though. https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:861: RTC_DCHECK_LE(num_channels, kMaxNumChannels); An alternative here is also to do the for-loop to min(num_channels, kMaxNumChannels); In that way, the code won't crash in release mode. https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:893: sizeof(float) * formats_.api_format.output_stream().num_frames(); This code is quite similar to the code in 1458. Have you looked into whether this code repetition could be avoided by moving the code to a method? https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1465: for (size_t i = 0; i < num_channels; ++i) { Same thing here, what about looping against min(num_channels, kMaxNumChannels) https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:82: virtual void AddInput( Are the method implementations of these classes to be added in an upcoming path? One thing I don't follow is why this needs to be an interface? There will be only one AecDump implementation, right? Which in my mind should mean that no interface is needed. Or did I get this wrong?
Patchset #8 (id:460001) has been deleted
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Thanks for the comments! Now there is less code duplication and the verbosity is hidden. PTAL on the latest PS. https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:133: constexpr size_t kMaxNumChannels = 2; On 2017/04/21 05:15:05, peah-webrtc wrote: > To be on the safe side, could we set this to 4 (I've seen that in one > recording). I'm not sure whether the rest of the code can cope with that though. > > Also, as this still applies only to the recording, maybe the naming could > reflect that? e.g,, kMaxNumChannelsToRecord > > WDYT? Sure, done. https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:852: aec_dump_->WriteConfig(CollectApmConfig(), false); On 2017/04/21 05:15:05, peah-webrtc wrote: > Would it be ok to move this code block, and the other similar code blocks, to > separate member methods. > > It is quite nice not to have the recording functionality clutter the rest of the > code. The ability to avoid this is a huge gain with this CL, and it would be > nice to go as far as possible with that. > > WDYT? That is definitely something that could be done in another CL as well > though. Good idea, I didn't think of that. I've tried, see the next PS. The pattern of creating |stream_info|, filling it with input, then doing processing, then filling it with output complicates matters. Nonetheless, I like the latest version more. https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:861: RTC_DCHECK_LE(num_channels, kMaxNumChannels); On 2017/04/21 05:15:05, peah-webrtc wrote: > An alternative here is also to do the for-loop to min(num_channels, > kMaxNumChannels); > In that way, the code won't crash in release mode. Done. https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:893: sizeof(float) * formats_.api_format.output_stream().num_frames(); On 2017/04/21 05:15:05, peah-webrtc wrote: > This code is quite similar to the code in 1458. Have you looked into whether > this code repetition could be avoided by moving the code to a method? Done. https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1465: for (size_t i = 0; i < num_channels; ++i) { On 2017/04/21 05:15:05, peah-webrtc wrote: > Same thing here, what about looping against min(num_channels, kMaxNumChannels) Done. https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:82: virtual void AddInput( On 2017/04/21 05:15:05, peah-webrtc wrote: > Are the method implementations of these classes to be added in an upcoming path? > > One thing I don't follow is why this needs to be an interface? There will be > only one AecDump implementation, right? Which in my mind should mean that no > interface is needed. Or did I get this wrong? Yes, the implementation is the next step. Reasons for using an interface: * without an interface, it is known at compile time how the implementation looks. The implementation uses protobuf objects, meaning that we need #ifdef directives to hide them. * an interface allows injecting a mock (which I'm planning to do for testing this) * it gets possible to have different independent targets for audio_processing and the implementation(s) and the build system ensures that audio_processing is independent from the implementation details (single thread-blocking, task queue, any other model). This comes at the cost of virtual calls though. I compared running with/without virtual calls: the added cost is unnoticeable behind the locks and threading.
Thanks for the new patch! Here are some more comments. https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/440001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:82: virtual void AddInput( On 2017/04/21 13:47:55, aleloi wrote: > On 2017/04/21 05:15:05, peah-webrtc wrote: > > Are the method implementations of these classes to be added in an upcoming > path? > > > > One thing I don't follow is why this needs to be an interface? There will be > > only one AecDump implementation, right? Which in my mind should mean that no > > interface is needed. Or did I get this wrong? > > Yes, the implementation is the next step. > > Reasons for using an interface: > * without an interface, it is known at compile time how the implementation > looks. The implementation uses protobuf objects, meaning that we need #ifdef > directives to hide them. > * an interface allows injecting a mock (which I'm planning to do for testing > this) > * it gets possible to have different independent targets for audio_processing > and the implementation(s) and the build system ensures that audio_processing is > independent from the implementation details (single thread-blocking, task queue, > any other model). > > This comes at the cost of virtual calls though. I compared running with/without > virtual calls: the added cost is unnoticeable behind the locks and threading. Good points all of them. Sounds great! https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:133: constexpr size_t kMaxNumChannels = 4; I think you commented that this would be changed. https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:176: rtc::ArrayView<rtc::ArrayView<const float>> stream_view( Does this name follow the style guideline? What about CreateStreamView? https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:865: aec_dump_->WriteConfig(CollectApmConfig(), false); Same thing here, would it be possible to move the whole recording functionality into a method? https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:896: sizeof(float) * formats_.api_format.output_stream().num_frames(); Same thing here, would it be possible to move the whole recording functionality into a method? https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1141: stream_info = aec_dump_->GetCaptureStreamInfo(); Same thing here, would it be possible to move the whole recording functionality into a method? https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1453: const size_t num_channels = Same thing here, would it be possible to move the whole recording functionality into a method?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #9 (id:500001) has been deleted
All done, PTAL! https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:133: constexpr size_t kMaxNumChannels = 4; On 2017/04/21 14:05:15, peah-webrtc wrote: > I think you commented that this would be changed. The name or the number? I've changed the name now (somehow the change didn't end up in this P-set). I think we said that 4 should be enough for now. https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:176: rtc::ArrayView<rtc::ArrayView<const float>> stream_view( On 2017/04/21 14:05:15, peah-webrtc wrote: > Does this name follow the style guideline? What about CreateStreamView? I thought the style allowed names_with_underscores for cheap functions. Re-reading it showed I was wrong: Regular functions have mixed case; accessors and mutators may be named like variables. In any case, CreateStreamView is more descriptive. https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:865: aec_dump_->WriteConfig(CollectApmConfig(), false); On 2017/04/21 14:05:15, peah-webrtc wrote: > Same thing here, would it be possible to move the whole recording functionality > into a method? WDYT about the latest change? I don't think it's perfect, because the RecordUnprocessedCaptureStream() method does lots of things at once, but at least ProcessStream looks simpler now. https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:896: sizeof(float) * formats_.api_format.output_stream().num_frames(); On 2017/04/21 14:05:15, peah-webrtc wrote: > Same thing here, would it be possible to move the whole recording functionality > into a method? Acknowledged. https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1141: stream_info = aec_dump_->GetCaptureStreamInfo(); On 2017/04/21 14:05:15, peah-webrtc wrote: > Same thing here, would it be possible to move the whole recording functionality > into a method? Acknowledged. https://codereview.webrtc.org/2778783002/diff/480001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1453: const size_t num_channels = On 2017/04/21 14:05:15, peah-webrtc wrote: > Same thing here, would it be possible to move the whole recording functionality > into a method? Acknowledged.
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great! The partitioning of the aecdump functionality into separate methods looks great! lgtm, but please have a look at the comments in audio_processing_impl.cc. To me symmetry increases the readability of the code. https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:865: aec_dump_ ? RecordUnprocessedCaptureStream(src) : nullptr; Is the else statement really needed (the nullptr assignment)? An alternative is: std::unique_ptr<AecDump::CaptureStreamInfo> stream_info; if (aec_dump_) { RecordUnProcessedCaptureStream(src); } Even though that is not a one-liner (or two-liner), it is symmetric with the code in 884-886. https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1123: std::unique_ptr<AecDump::CaptureStreamInfo> stream_info = Same comment as above. https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:70: class AecDump { You probably should have a comment describing the purpose of the class.
Description was changed from ========== [WORK IN PROGRESS] AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs over a period of time. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is done from the real-time audio thread. This CL contains an interface for AecDumper, a new APM submodule that will handle the recordings. It also contains an empty implementation of this interface. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc contains two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDumper added. An working and near-final version of the complete implementation is here: https://codereview.webrtc.org/2747123007/ BUG=webrtc:7404 ========== to ========== AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs over a period of time. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is, as of this CL, done from the real-time audio thread. This CL contains an interface for AecDump, a new APM submodule that will handle the recordings. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc will contain two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDump added. The reasons for the refactoring is to move file IO operations from the real-time audio thread, to add a top-level low-priority task queue for logging tasks like this, to simplify and modularize audio_processing_impl.cc and remove some of the preprocessor directives. These goals will be archived by the upcoming CLs. BUG=webrtc:7404 ==========
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:865: aec_dump_ ? RecordUnprocessedCaptureStream(src) : nullptr; On 2017/04/25 21:04:46, peah-webrtc wrote: > Is the else statement really needed (the nullptr assignment)? > An alternative is: > std::unique_ptr<AecDump::CaptureStreamInfo> stream_info; > if (aec_dump_) { > RecordUnProcessedCaptureStream(src); > } > > Even though that is not a one-liner (or two-liner), it is symmetric with the > code in 884-886. Done. https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1123: std::unique_ptr<AecDump::CaptureStreamInfo> stream_info = On 2017/04/25 21:04:46, peah-webrtc wrote: > Same comment as above. Done. https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:70: class AecDump { On 2017/04/25 21:04:46, peah-webrtc wrote: > You probably should have a comment describing the purpose of the class. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #11 (id:560001) has been deleted
Incomplete feedback, but there's one thing you probably want to fix so I'm sending it anyway. Don't expect more today as I'm OOO. https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:133: constexpr size_t kMaxNumChannelsToRecord = 4; Why 4? We only support stereo anyway. And why arbitrarily limit it to 4? If we were to support multi-channel scenarios, 8 channels (to support up to 7.1 or 6.2 configs) seems more useful. https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:187: return rtc::ArrayView<rtc::ArrayView<const float>>(&array_stream_view[0], ArrayView is just a pointer and length, so in this case you're returning a pointer to stack allocated data, which will be freed. In other words a crash bug. https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:70: void StartDebugRecording(std::unique_ptr<AecDump> aec_dump) override; nit: perhaps "Start" isn't the right verb here. "Set" seems more appropriate. Perhaps even SetAecDump()? https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:338: std::unique_ptr<AecDump> aec_dump_; EXCLUSIVE_LOCKS_REQUIRED(crit_render_, crit_capture_)
https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:133: constexpr size_t kMaxNumChannelsToRecord = 4; On 2017/04/28 11:06:00, the sun wrote: > Why 4? We only support stereo anyway. And why arbitrarily limit it to 4? If we > were to support multi-channel scenarios, 8 channels (to support up to 7.1 or 6.2 > configs) seems more useful. Good point :-) The reason for 4 is that that is the maximum I've seen in the APM input. The first solution stored dynamically all channels each frame but as that caused a vector allocation each frame, and since we in practice already know the maximum vector size, I preferred a static allocation. If we want to be more general and support even more channels, we could allocate that vector on the state each time the number of channels is changed. It does, however, make sense to increase it to 8 as well, but I doubt our code can handle that.
https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:187: return rtc::ArrayView<rtc::ArrayView<const float>>(&array_stream_view[0], On 2017/04/28 11:06:00, the sun wrote: > ArrayView is just a pointer and length, so in this case you're returning a > pointer to stack allocated data, which will be freed. In other words a crash > bug. +1. I haven't read the code closely, but I suspect what you need is std::vector<ArrayView<const float>> or rtc::Buffer<ArrayView<const float>>. ArrayView<ArrayView<>> is almost never right.
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
PTAL at the latest patch set! There are also (not yet completely finalized) dependent CLs with implementation and tests added. https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:133: constexpr size_t kMaxNumChannelsToRecord = 4; On 2017/04/28 11:06:00, the sun wrote: > Why 4? We only support stereo anyway. And why arbitrarily limit it to 4? If we > were to support multi-channel scenarios, 8 channels (to support up to 7.1 or 6.2 > configs) seems more useful. Removed in latest patch. https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:187: return rtc::ArrayView<rtc::ArrayView<const float>>(&array_stream_view[0], On 2017/04/28 11:06:00, the sun wrote: > ArrayView is just a pointer and length, so in this case you're returning a > pointer to stack allocated data, which will be freed. In other words a crash > bug. Now fixed. https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:70: void StartDebugRecording(std::unique_ptr<AecDump> aec_dump) override; On 2017/04/28 11:06:00, the sun wrote: > nit: perhaps "Start" isn't the right verb here. "Set" seems more appropriate. > Perhaps even SetAecDump()? Ok, changing to SetAecDump(). https://codereview.webrtc.org/2778783002/diff/520001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.h:338: std::unique_ptr<AecDump> aec_dump_; On 2017/04/28 11:06:00, the sun wrote: > EXCLUSIVE_LOCKS_REQUIRED(crit_render_, crit_capture_) Calling methods concurrently on a AecDump instance is supposed to be safe. Modifying the aec_dump_ pointer concurrently is not. Currently, calls to the AecDump are guarded by *some* of the render/capture locks. Changes to the pointer are guarded by both locks. The locking scheme used currently is safe, but it cannot be expressed with compiler annotations. I don't think it's worth adding extra locks where they are not needed for correctness to be able to have compiler-checked annotations.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just a quick drive-by comment. https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:73: size_t channel_size) These are nontrivial arguments. Document them? https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:90: }; This is exactly what I had in mind when we spoke: a simple value class that hides its members and provides safe accessors.
Hi, This looks really nice! Awesome! I have some more comments. https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:2004: void AudioProcessingImpl::PopulateStreamInfoWithConfig( This naming is a big ambiguous as we have WriteConfig that stores the apm_config, and the config referred to in this call is something different. Furthermore, I don't think it is a configuration that is stored, rather than a state. Could this be renamed to make it more clear? What about PopulateStreamInfoWithState PopulateStreamInfoMetadata PopulateStreamInfo https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:2008: stream_info->set_delay(capture_nonlocked_.stream_delay_ms); Would it be possible to instead bundle these 4 setters into a single set_ call in stream_info that takes 4 arguments? That would be cheaper, would avoid the need for this method and would make the code in audio_processing_impl.cc shorter. https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:108: virtual void AddInput(FloatAudioFrame src) = 0; These calls are made by copying the FloatAudioFrame objects. Even though those are small, I guess it would be faster to const-ref them? Or is it better for the compiler to have them copied? If not for complexity, it may be good for the reader of the code as at a casual look at the code it looks as if these objects are large (A FloatAudioFrame looks like a container for a frame of samples). https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:115: virtual void set_drift(int drift) = 0; I think it would make sense to combine these 4 setters into a single set method as they are alway called at the same time, that will avoid missing calling one of them by mistake, and cause the code to be smaller. WDYT? https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:122: virtual std::unique_ptr<CaptureStreamInfo> GetCaptureStreamInfo() const = 0; Does the GetCaptureStreamInfo() method create CaptureStream Objects?
https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:2004: void AudioProcessingImpl::PopulateStreamInfoWithConfig( On 2017/05/04 06:19:08, peah-webrtc wrote: > This naming is a big ambiguous as we have WriteConfig that stores the > apm_config, and the config referred to in this call is something different. > Furthermore, I don't think it is a configuration that is stored, rather than a > state. > > Could this be renamed to make it more clear? > What about > PopulateStreamInfoWithState > PopulateStreamInfoMetadata > PopulateStreamInfo I like PopulateStreamInfoWithState the most. Done. https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:2008: stream_info->set_delay(capture_nonlocked_.stream_delay_ms); On 2017/05/04 06:19:08, peah-webrtc wrote: > Would it be possible to instead bundle these 4 setters into a single set_ call > in stream_info that takes 4 arguments? That would be cheaper, would avoid the > need for this method and would make the code in audio_processing_impl.cc > shorter. I'd rather not. The setter names serve as documentation for what is logged. The style guide has an example in the 'Function Argument Comments' section https://google.github.io/styleguide/cppguide.html#Implementation_Comments that argues against several parameters. https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:73: size_t channel_size) On 2017/05/03 18:57:57, kwiberg-webrtc wrote: > These are nontrivial arguments. Document them? Done. https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:108: virtual void AddInput(FloatAudioFrame src) = 0; On 2017/05/04 06:19:08, peah-webrtc wrote: > These calls are made by copying the FloatAudioFrame objects. Even though those > are small, I guess it would be faster to const-ref them? Or is it better for the > compiler to have them copied? If not for complexity, it may be good for the > reader of the code as at a casual look at the code it looks as if these objects > are large (A FloatAudioFrame looks like a container for a frame of samples). I think no copies are made here because of return value optimization rules. I've changed to const-ref for it to read better. https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:115: virtual void set_drift(int drift) = 0; On 2017/05/04 06:19:08, peah-webrtc wrote: > I think it would make sense to combine these 4 setters into a single set method > as they are alway called at the same time, that will avoid missing calling one > of them by mistake, and cause the code to be smaller. WDYT? I think a set_state(int, int, int, bool) method would harm readability. WDYT re adding a AECState{delay, drift, level, keypress} struct (which can have setters, but the setters would not be virtual and be inlined) and a set_state(const AECState &) method? That would follow the recommendations in https://google.github.io/styleguide/cppguide.html#Implementation_Comments https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:122: virtual std::unique_ptr<CaptureStreamInfo> GetCaptureStreamInfo() const = 0; On 2017/05/04 06:19:08, peah-webrtc wrote: > Does the GetCaptureStreamInfo() method create CaptureStream Objects? Yes, it should be called 'CreateCaptureStreamInfo'. Now renamed.
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #11 (id:580001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #9 (id:600001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the update! I have some more comments/questions. PTAL. https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:2008: stream_info->set_delay(capture_nonlocked_.stream_delay_ms); On 2017/05/04 08:56:40, aleloi wrote: > On 2017/05/04 06:19:08, peah-webrtc wrote: > > Would it be possible to instead bundle these 4 setters into a single set_ call > > in stream_info that takes 4 arguments? That would be cheaper, would avoid the > > need for this method and would make the code in audio_processing_impl.cc > > shorter. > > I'd rather not. The setter names serve as documentation for what is logged. The > style guide has an example in the 'Function Argument Comments' section > https://google.github.io/styleguide/cppguide.html#Implementation_Comments that > argues against several parameters. I definitely like the documentation part. But this comes at an additional cost of up to 180 cycles as these are virtual methods. Please see the further comments in aec_dump.h about this. https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/640001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:115: virtual void set_drift(int drift) = 0; On 2017/05/04 08:56:40, aleloi wrote: > On 2017/05/04 06:19:08, peah-webrtc wrote: > > I think it would make sense to combine these 4 setters into a single set > method > > as they are alway called at the same time, that will avoid missing calling one > > of them by mistake, and cause the code to be smaller. WDYT? > > I think a set_state(int, int, int, bool) method would harm readability. WDYT re > adding a AECState{delay, drift, level, keypress} struct (which can have setters, > but the setters would not be virtual and be inlined) and a set_state(const > AECState &) method? That would follow the recommendations in > https://google.github.io/styleguide/cppguide.html#Implementation_Comments I don't think the style guide comment really applies to reducing the number of parameters in general but rather to commenting code, but I may be wrong. For readability I prefer shorter code, and replacing multi-parameter methods with several single parameters produces more code, both for making the calls as well as for implementing the setters (more boilerplate code). In this case I think it is ok to have it as it is as I think your point about the explicit documentation about what is logged is a really strong one. Regarding the AECState, I'd prefer instead avoiding making AecDump::CaptureStreamInfo an interface. With that change, the setters won't be virtual, which means that they can be inlined and the cpu-overhead will be nonexistent. https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:106: // AecDump::GetCaptureStreamInfo. Add the input and output to GetCaptureStreamInfo -> CreateCaptureStreamInfo https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:108: class CaptureStreamInfo { Does CaptureStreamInfo really need to be an interface? Is it not sufficient that AecDump is an interface? https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:125: virtual std::unique_ptr<CaptureStreamInfo> CreateCaptureStreamInfo() Regarding the implementation of this. Does it create a new CaptureStreamInfo object, or is a previously existing object selected from an object pool and have its ownership passed via the unique_ptr? The reason that I'm asking is for concerns about the cost of this, as it sounds as if this does dynamic object creation which in turn is used in a realtime thread.
Thanks for the comments! https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:106: // AecDump::GetCaptureStreamInfo. Add the input and output to On 2017/05/04 10:57:51, peah-webrtc wrote: > GetCaptureStreamInfo -> CreateCaptureStreamInfo Done in upcoming patch set. https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:108: class CaptureStreamInfo { On 2017/05/04 10:57:51, peah-webrtc wrote: > Does CaptureStreamInfo really need to be an interface? Is it not sufficient that > AecDump is an interface? If it's not an interface, an extra copy of the data is needed. Which might be worth it. Here is the reasoning: audio_processing is isolated from the protobuf headers and targets. In the implementation of the dependent CL https://codereview.webrtc.org/2838133003/, CaptureStreamInfoImpl holds a protobuf object, to which the Add* methods copy data. A possible change would be to make CaptureStreamInfo non-virtual and store the data in a struct similar to Internal* in this file. Then AecDumpImpl could be responsible for copying data (a 2:nd time) into pb objects. I can to another profiling recording to find out if the version with inlined methods but an extra copy is better than no extra copy but virtual methods. https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:125: virtual std::unique_ptr<CaptureStreamInfo> CreateCaptureStreamInfo() On 2017/05/04 10:57:51, peah-webrtc wrote: > Regarding the implementation of this. Does it create a new CaptureStreamInfo > object, or is a previously existing object selected from an object pool and have > its ownership passed via the unique_ptr? > The reason that I'm asking is for concerns about the cost of this, as it sounds > as if this does dynamic object creation which in turn is used in a realtime > thread. Yes, a new CaptureStreamInfo is created. I tried reusing objects with a pool based on SwapQueue, but profiling showed that the required locks and relatively small object size didn't make it worth it. CaptureStreamInfoImpl is smaller than the protobuf objects.
Some more comments. https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:108: class CaptureStreamInfo { On 2017/05/04 11:34:59, aleloi wrote: > On 2017/05/04 10:57:51, peah-webrtc wrote: > > Does CaptureStreamInfo really need to be an interface? Is it not sufficient > that > > AecDump is an interface? > > If it's not an interface, an extra copy of the data is needed. Which might be > worth it. Here is the reasoning: > > audio_processing is isolated from the protobuf headers and targets. In the > implementation of the dependent CL https://codereview.webrtc.org/2838133003/, > CaptureStreamInfoImpl holds a protobuf object, to which the Add* methods copy > data. > > A possible change would be to make CaptureStreamInfo non-virtual and store the > data in a struct similar to Internal* in this file. Then AecDumpImpl could be > responsible for copying data (a 2:nd time) into pb objects. > > I can to another profiling recording to find out if the version with inlined > methods but an extra copy is better than no extra copy but virtual methods. I'm probably getting this fully wrong, but I was under the impression that the reason why AecDump needed to be an interface was that the TaskQueue needed that. But looking at the other CL, I don't really see any need for either AecDump or CaptureStreamInfo to be interfaces. What is posted to the taskqueue is the WriteToFileTask object. What is the reason that AecDump and CaptureStreamInfo needs to be interfaces? Is it because there needs to be two implementations, depending on whether the protobuf headers are included or not? https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:117: virtual void set_delay(int delay) = 0; I'm kind of changing my mind for these. Each of these are in the other CL implemented as event_->mutable_stream()->set_nnnn(..); Where I guess that both event_-> and mutable_stream()-> involves a virtual pointer checkup? That means that each of these costs 180 cycles, totalling 720 cycles, in comparison to the 180 cycles it would cost to bundle them in one call, right? https://codereview.webrtc.org/2778783002/diff/680001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:125: virtual std::unique_ptr<CaptureStreamInfo> CreateCaptureStreamInfo() On 2017/05/04 11:35:00, aleloi wrote: > On 2017/05/04 10:57:51, peah-webrtc wrote: > > Regarding the implementation of this. Does it create a new CaptureStreamInfo > > object, or is a previously existing object selected from an object pool and > have > > its ownership passed via the unique_ptr? > > The reason that I'm asking is for concerns about the cost of this, as it > sounds > > as if this does dynamic object creation which in turn is used in a realtime > > thread. > > Yes, a new CaptureStreamInfo is created. I tried reusing objects with a pool > based on SwapQueue, but profiling showed that the required locks and relatively > small object size didn't make it worth it. CaptureStreamInfoImpl is smaller than > the protobuf objects. I thought the object size did not really matter when that much when doing a new operation. And comparing to aquiring a lock I would have thought that the new operation was much more expensive as long as there is no contention. Does that mean that the swapqueue approach is more expensive compared to allocating new objects?
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs over a period of time. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is, as of this CL, done from the real-time audio thread. This CL contains an interface for AecDump, a new APM submodule that will handle the recordings. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc will contain two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDump added. The reasons for the refactoring is to move file IO operations from the real-time audio thread, to add a top-level low-priority task queue for logging tasks like this, to simplify and modularize audio_processing_impl.cc and remove some of the preprocessor directives. These goals will be archived by the upcoming CLs. BUG=webrtc:7404 ========== to ========== AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs over a period of time. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is, as of this CL, done from the real-time audio thread. This CL contains an interface for AecDump, a new APM submodule that will handle the recordings. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc will contain two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDump added. The reasons for the refactoring is to move file IO operations from the real-time audio thread, to add a top-level low-priority task queue for logging tasks like this, to simplify and modularize audio_processing_impl.cc and remove some of the preprocessor directives. These goals will be archived by the upcoming CLs. The implementation is in https://codereview.webrtc.org/2865113002. BUG=webrtc:7404 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/13408)
PTAL on the changes of AecDump to make it non-polymorphic. See also the implementation CL https://codereview.webrtc.org/2865113002/ and the (very short) CL https://codereview.webrtc.org/2864373002 that replaces the aec dump recording with this submodule in the APM tests.
On 2017/05/08 15:00:29, aleloi wrote: > PTAL on the changes of AecDump to make it non-polymorphic. See also the > implementation CL https://codereview.webrtc.org/2865113002/ and the (very short) > CL https://codereview.webrtc.org/2864373002 that replaces the aec dump recording > with this submodule in the APM tests. One of the reasons for having an interface for the AecDump was to be able to supply alternate implementations. So that: a) We wouldn't need a dump object with N different ctors to be used in different situations. b) The clients wouldn't need to depend on the protobuf library. c) The dump functionality could be tested in isolation. d) Build config flags wouldn't magically decide which version was being used. Build-time polymorphism? :) Explain to me what made you decide against these, or how the current proposal solves the problems.
On 2017/05/08 20:41:57, the sun wrote: > On 2017/05/08 15:00:29, aleloi wrote: > > PTAL on the changes of AecDump to make it non-polymorphic. See also the > > implementation CL https://codereview.webrtc.org/2865113002/ and the (very > short) > > CL https://codereview.webrtc.org/2864373002 that replaces the aec dump > recording > > with this submodule in the APM tests. > > One of the reasons for having an interface for the AecDump was to be able to > supply alternate implementations. So that: > > a) We wouldn't need a dump object with N different ctors to be used in different > situations. > b) The clients wouldn't need to depend on the protobuf library. > c) The dump functionality could be tested in isolation. > d) Build config flags wouldn't magically decide which version was being used. > Build-time polymorphism? :) > > Explain to me what made you decide against these, or how the current proposal > solves the problems. It was me who started questioning the interface approach :-) . My main concerns were that there were 3/4 layers of interfaces that needed to be traversed multiple of times for each writing of an aecdump packet and in the spirit of reducing the amount of virtual method calls and the amount of code I suggested to avoid the first 2 layers being interfaces. Regarding the advantages a) The ctors is a drawback, but using an interface to avoid that still requires the same number of creation methods so I don't see that drawback as a anything big. b) I believe that it was found that this can be achieved without aecdump being an interface. c) Testing the aecdump functionality integration in APM would require this interface. But I don't think that testing the actual aecdump recording functionality should, right? That class is fairly independent of APM. d) You mean that we should use the build system instead of using #defines in the methods? Relying on the build system to achieve that should still be possible. e) I don't see a need for alternative implementations, but if we see that need, that is indeed a strong argument for an interface. Let's discuss this further. My concern was that the gain payed overhead both in CPU cycles and amount of code was not anything that we really would have use of, but I'm happy to be convinced otherwise!
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/8966)
Hi, Thanks for the new patch! I have some more comments. PTAL https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1109: stream_info = RecordUnprocessedCaptureStream(*frame); I think the usage of having stream_info as an output of RecordUnpr.. and as an input of RecordPro... makes this code, and the writing of aecdumps, more nonlinear than it needs to. There is actually nothing that requires the code in this method to hold a copy of CaptureStreamInfo. Since you anyway interact with aec_dump_ in both methods, and aec_dump stores the CaptureStreamInfo object, I think you should not use that as inputs and outputs here. That would make the storing more self-contained and linear, and make this code smaller. WDYT? https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1650: DetachAecDump(); Please move this to before the locks (as DetachAecDump() acquires the locks too) and move the locks within the #ifdef to 1653. https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1960: aec_dump_->WriteConfig(CollectApmConfig(), false); How can this method be const? It does writing to aec_dump_ so it changes the state. https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1966: stream_info->AddInput(FloatAudioFrame(src, num_channels, channel_size)); I'm not strongly against it, but I'd suggest dropping the FloatAudioFrame class as I think does not simplify neither the use of AecDump nor the implementation of the class, but regardless of that adds more code. Why not instead just pass the pointer to the data, the number of channels and the channel size to aec_dump.h? https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1985: AecDump::CaptureStreamInfo* stream_info) const { +1, how can it be const? (and elsewhere) https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:66: // Class to pass audio data in float** format. This is to avoid Since FloatAudioFrame is defined and used only within AecDump, (or calls into that). I would suggest removing it. It adds an abstraction to AudioBuffer which neither make the calling of the AecDump methods simpler (the same number of parameters are used) nor the AecDump methods implementations. WDYT? https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:456: // be called. The d-tor call may block until all pending logging The blocking part is not good at all. If a new aecdump is created when there is another existing aecdump, this may render the aecdump unusable (as it will cause glitches). Will the d-tor really block? What is causing the need of the d-tor blocking?
https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1966: stream_info->AddInput(FloatAudioFrame(src, num_channels, channel_size)); On 2017/05/15 05:32:51, peah-webrtc wrote: > I'm not strongly against it, but I'd suggest dropping the FloatAudioFrame class > as I think does not simplify neither the use of AecDump nor the implementation > of the class, but regardless of that adds more code. > > Why not instead just pass the pointer to the data, the number of channels and > the channel size to aec_dump.h? Thinking a bit more about FloatAudioFrame, I guess it would make more sense if it was a generic class that was used elsewhere as well in APM. But that would not work, I guess, as we'd then be hitting the same issue with it being non-public as is the case with AudioBuffer?
https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1966: stream_info->AddInput(FloatAudioFrame(src, num_channels, channel_size)); On 2017/05/15 05:57:07, peah-webrtc wrote: > On 2017/05/15 05:32:51, peah-webrtc wrote: > > I'm not strongly against it, but I'd suggest dropping the FloatAudioFrame > class > > as I think does not simplify neither the use of AecDump nor the implementation > > of the class, but regardless of that adds more code. > > > > Why not instead just pass the pointer to the data, the number of channels and > > the channel size to aec_dump.h? > > Thinking a bit more about FloatAudioFrame, I guess it would make more sense if > it was a generic class that was used elsewhere as well in APM. But that would > not work, I guess, as we'd then be hitting the same issue with it being > non-public as is the case with AudioBuffer? I now saw your comment about this in the upcoming CL. That makes sense. Let's stick with FloatAudioFrame. https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:66: // Class to pass audio data in float** format. This is to avoid On 2017/05/15 05:32:51, peah-webrtc wrote: > Since FloatAudioFrame is defined and used only within AecDump, (or calls into > that). I would suggest removing it. > > It adds an abstraction to AudioBuffer which neither make the calling of the > AecDump methods simpler (the same number of parameters are used) nor the AecDump > methods implementations. > > WDYT? I now saw your comment about this in the upcoming CL. That makes sense. Let's stick with FloatAudioFrame.
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/13687) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/26066)
Patchset #13 (id:700001) has been deleted
Patchset #8 (id:540001) has been deleted
Patchset #8 (id:620001) has been deleted
Patchset #9 (id:660001) has been deleted
Patchset #10 (id:720001) has been deleted
Patchset #10 (id:740001) has been deleted
Patchset #11 (id:780001) has been deleted
PTAL on the latest changes to interface & implementation! https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1109: stream_info = RecordUnprocessedCaptureStream(*frame); On 2017/05/15 05:32:51, peah-webrtc wrote: > I think the usage of having stream_info as an output of RecordUnpr.. and as an > input of RecordPro... makes this code, and the writing of aecdumps, more > nonlinear than it needs to. > > There is actually nothing that requires the code in this method to hold a copy > of CaptureStreamInfo. Since you anyway interact with aec_dump_ in both methods, > and aec_dump stores the CaptureStreamInfo object, I think you should not use > that as inputs and outputs here. > > That would make the storing more self-contained and linear, and make this code > smaller. > > WDYT? I've changed it now. It looks a little better now. https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1650: DetachAecDump(); On 2017/05/15 05:32:51, peah-webrtc wrote: > Please move this to before the locks (as DetachAecDump() acquires the locks too) > and move the locks within the #ifdef to 1653. Done. https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1960: aec_dump_->WriteConfig(CollectApmConfig(), false); On 2017/05/15 05:32:51, peah-webrtc wrote: > How can this method be const? It does writing to aec_dump_ so it changes the > state. Done. https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1985: AecDump::CaptureStreamInfo* stream_info) const { On 2017/05/15 05:32:51, peah-webrtc wrote: > +1, how can it be const? (and elsewhere) Done. https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:66: // Class to pass audio data in float** format. This is to avoid On 2017/05/15 07:25:18, peah-webrtc wrote: > On 2017/05/15 05:32:51, peah-webrtc wrote: > > Since FloatAudioFrame is defined and used only within AecDump, (or calls into > > that). I would suggest removing it. > > > > It adds an abstraction to AudioBuffer which neither make the calling of the > > AecDump methods simpler (the same number of parameters are used) nor the > AecDump > > methods implementations. > > > > WDYT? > > I now saw your comment about this in the upcoming CL. That makes sense. Let's > stick with FloatAudioFrame. Acknowledged. https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2778783002/diff/760001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/audio_processing.h:456: // be called. The d-tor call may block until all pending logging On 2017/05/15 05:32:51, peah-webrtc wrote: > The blocking part is not good at all. If a new aecdump is created when there is > another existing aecdump, this may render the aecdump unusable (as it will cause > glitches). > > Will the d-tor really block? What is causing the need of the d-tor blocking? After offline discussions d-tor still blocks, but not while holding both the render and capture locks. If StopDebugDump or DetachAecDump is called from something which is not the real-time audio thread, audio processing should continue without glitches. Short version of why the d-tor blocks: When an AecDumpImpl object is destoryed, worker threads must not try to access pointers to it. The WriteToFile tasks have access to 'int64_t num_bytes_left_for_log_' and, 'FileWrapper* debug_file_', which are owned by AecDumpImpl. A task queue has no 'flushing' functionality. We could possibly add a way to notify pending tasks that their AecDumpImpl pointers are stale and that the task should not be executed. That would imply * rewriting a number of AecDump tests, because the current ones rely on every task being executed * adding extra locks and complicating the memory pattern. Another comment: a blocking d-tor is the pattern used in most other places with a task queue/working thread in WebRTC: AudioSendStream, RTC event log, VideoSendStream, voice engine channel.
Nice! Awesome! lgtm, but conditioned on that you address the issue in AttachAecDump and also have a look at the other comments. https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1562: aec_dump_ = std::move(aec_dump); What happens if someone has forgot to Detach the previous aecdump before attaching the new aecdump? I think we should force a detach before doing the attachment: void AudioProcessingImpl::AttachAecDump(std::unique_ptr<AecDump> aec_dump) { std::unique_ptr<AecDump> old_aec_dump = nullptr; { rtc::CritScope cs_render(&crit_render_); rtc::CritScope cs_capture(&crit_capture_); if (aec_dump_) { old_aec_dump = std::move(aec_dump_); } RTC_DCHECK(aec_dump); aec_dump_ = std::move(aec_dump); aec_dump_->WriteConfig(CollectApmConfig(), true); aec_dump_->WriteInitMessage(ToStreamsConfig(formats_.api_format)); } } https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:95: struct AudioProcessingState { Put this part of AecDump? The name is so generic, so I think it makes sense to put it as part of AecDump to give context. WDYT? https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:108: // A capture stream frame is logged before and after processing in This comment needs updating. https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:126: // virtual CaptureStreamInfo* GetCaptureStreamInfo() = 0; Remove?
Thanks for the comments. I'll see if the interface can be landed tomorrow. https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1562: aec_dump_ = std::move(aec_dump); On 2017/05/16 04:55:17, peah-webrtc wrote: > What happens if someone has forgot to Detach the previous aecdump before > attaching the new aecdump? > > I think we should force a detach before doing the attachment: > > void AudioProcessingImpl::AttachAecDump(std::unique_ptr<AecDump> aec_dump) { > std::unique_ptr<AecDump> old_aec_dump = nullptr; > { > rtc::CritScope cs_render(&crit_render_); > rtc::CritScope cs_capture(&crit_capture_); > if (aec_dump_) { > old_aec_dump = std::move(aec_dump_); > } > RTC_DCHECK(aec_dump); > aec_dump_ = std::move(aec_dump); > aec_dump_->WriteConfig(CollectApmConfig(), true); > aec_dump_->WriteInitMessage(ToStreamsConfig(formats_.api_format)); > } > } Thanks, great that you spotted it! https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:95: struct AudioProcessingState { On 2017/05/16 04:55:17, peah-webrtc wrote: > Put this part of AecDump? The name is so generic, so I think it makes sense to > put it as part of AecDump to give context. > > WDYT? Done. https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:108: // A capture stream frame is logged before and after processing in On 2017/05/16 04:55:17, peah-webrtc wrote: > This comment needs updating. Done. https://codereview.webrtc.org/2778783002/diff/840001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:126: // virtual CaptureStreamInfo* GetCaptureStreamInfo() = 0; On 2017/05/16 04:55:17, peah-webrtc wrote: > Remove? Done.
Patchset #14 (id:860001) has been deleted
Patchset #12 (id:820001) has been deleted
Patchset #11 (id:800001) has been deleted
Great! Awesome work! Still lgtm, I have one small comment though. https://codereview.webrtc.org/2778783002/diff/880001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/880001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1570: std::unique_ptr<AecDump> old_aec_dump = nullptr; You should skip the assignment here. It is not needed, and I guess it adds cost?
https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1574: old_aec_dump = std::move(aec_dump_); We don't have to use std::move for everything. You could instead do: RTC_DCHECK(aec_dump); [claim locks] aec_dump_.swap(aec_dump); aec_dump_->WriteConfig(... and avoid the extra scope and moves. https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1590: aec_dump = std::move(aec_dump_); If you use aec_dump_.reset(nullptr); you don't need the extra scope and the local unique_ptr. https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1978: aec_dump_->WriteConfig(CollectApmConfig(), false); Why do we always write the config? https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1993: aec_dump_->WriteConfig(CollectApmConfig(), false); Why is the config written after stream input in this variant, but not in the one above? https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:108: // To log an input/output pair, call the AddCapture* methods Place this comment with the AddCapture* methods, and move WriteCaptureStreamMessage() to the same place, so the semantics are more obvious. https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:113: virtual void AddCaptureStreamInput(const FloatAudioFrame& src) = 0; Do we really need to support both the fp and non-fp interfaces? https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:124: virtual void WriteInitMessage( This should go first, since it happens first. https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:133: // If not |forced|, only writes the current config if it is This sounds like an implementation detail which should reside in APM - leave the AecDump implementation dumb.
https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1978: aec_dump_->WriteConfig(CollectApmConfig(), false); On 2017/05/22 22:32:55, the sun wrote: > Why do we always write the config? That is because of the APM API which allows access to submodules. In order to be able to store changes of settings, we need to poll the config for changes, and store them if there are any. The "false" means that the storing is only done if there are changes. https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:113: virtual void AddCaptureStreamInput(const FloatAudioFrame& src) = 0; On 2017/05/22 22:32:56, the sun wrote: > Do we really need to support both the fp and non-fp interfaces? Yes, we do. On mobiles the non-fp interface is used and on desktops the fp interface is used.
PTAL on latest (minor) changes and responses to comments. https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1574: old_aec_dump = std::move(aec_dump_); On 2017/05/22 22:32:55, the sun wrote: > We don't have to use std::move for everything. > > You could instead do: > > RTC_DCHECK(aec_dump); > [claim locks] > aec_dump_.swap(aec_dump); > aec_dump_->WriteConfig(... > > and avoid the extra scope and moves. Thanks! I was worried the destructor of the previously attached AecDump would be called while holding the locks. Hence the extra scope. With the new version it's called when the 'aec_dump' parameter is destroyed. That happens after the function exits and after the locks are released. Standard quote (quoted from SO thread https://stackoverflow.com/questions/2506793): Temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created. The 'aec_dump' parameter is a temporary object. In a call apm->AttachAecDump(aec_dump); 'aec_dump' with the previously attached dump is destroyed at the last ')', which is after locks are released. Sorry for wall of text. I wanted to convince myself that the code still does the same thing. https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1590: aec_dump = std::move(aec_dump_); On 2017/05/22 22:32:56, the sun wrote: > If you use > aec_dump_.reset(nullptr); > you don't need the extra scope and the local unique_ptr. Then the AecDump d-tor would be called during the 'reset' call, which is while holding the locks. We could make by without the extra scope by relying on destroying variables in reverse order of initialization. This way: Detach() { aec_dump = nullptr; lock1 = ...; lock2 = ...; std::swap(aec_dump_, aec_dump); // or move, which I think is more explicit // automatically call local var d-tors in reverse order of initialization: // 1. lock2 released // 2. lock1 released // 3. previously attached aec_dump destroyed } I think that either construction is fine, but that the one in the current PS (with creating an extra scope) is more explicit. https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1993: aec_dump_->WriteConfig(CollectApmConfig(), false); On 2017/05/22 22:32:56, the sun wrote: > Why is the config written after stream input in this variant, but not in the one > above? Fixed, thanks! This doesn't change the order in which the pb objects are written to file. It's still CONFIG then STREAM, because the STREAM is written in RecordProcessedCaptureStream(), which happens after this method is called. https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:108: // To log an input/output pair, call the AddCapture* methods On 2017/05/22 22:32:56, the sun wrote: > Place this comment with the AddCapture* methods, and move > WriteCaptureStreamMessage() to the same place, so the semantics are more > obvious. Done. https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:124: virtual void WriteInitMessage( On 2017/05/22 22:32:56, the sun wrote: > This should go first, since it happens first. Done. https://codereview.webrtc.org/2778783002/diff/900001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:133: // If not |forced|, only writes the current config if it is On 2017/05/22 22:32:56, the sun wrote: > This sounds like an implementation detail which should reside in APM - leave > the AecDump implementation dumb. I like that! The change would also improve efficiency: this feature is implemented by calling SerializeAsString() and comparing the result with the previous string. That call shows up pretty high in debug recordings: https://drive.google.com/open?id=0B0ohpnzd-DTsd0FRTmh5VnU1S2M (see RecThreadProcess -> (...) -> RecordedDataIsAvailable -> ProcessStream -> RecordUnprocessedCaptureStream() Changing it co compare a 'InternalAPMStreamsConfig would * be faster because nothing is serialized to string * avoid a virtual call and grabbing a lock for posting something on the task queue
lgtm % comments https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1611: RTC_DCHECK(aec_dump); super nit: DCHECK before the locks https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1629: apm_config_for_aec_dump_ = InternalAPMConfig(); Why? This variable is force-updated in AttachAecDump? https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:34: bool Equals(const InternalAPMConfig& other); operator== ? https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:94: size_t channel_size_; Default init or = delete on default ctor https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:128: virtual void WriteRenderStreamMessage(const AudioFrame& frame) = 0; nit: clean up order and everything in this interface so that it is nice and tidy.
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1611: RTC_DCHECK(aec_dump); On 2017/05/23 12:00:22, the sun wrote: > super nit: DCHECK before the locks Done. https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/audio_processing_impl.cc:1629: apm_config_for_aec_dump_ = InternalAPMConfig(); On 2017/05/23 12:00:22, the sun wrote: > Why? This variable is force-updated in AttachAecDump? Done. https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/include/aec_dump.h (right): https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:34: bool Equals(const InternalAPMConfig& other); On 2017/05/23 12:00:22, the sun wrote: > operator== ? Done. I thought the style guide discouraged that, but I was wrong. Quote: Define overloaded operators only if their meaning is obvious, unsurprising, and consistent with the corresponding built-in operators. Don't go out of your way to avoid defining operator overloads. For example, prefer to define ==, =, and <<, rather than Equals(), CopyFrom(), and PrintTo(). https://codereview.webrtc.org/2778783002/diff/920001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/include/aec_dump.h:94: size_t channel_size_; On 2017/05/23 12:00:22, the sun wrote: > Default init or = delete on default ctor Done.
Patchset #16 (id:960001) has been deleted
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2778783002/#ps980001 (title: "Comment formatting.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 980001, "attempt_start_ts": 1495544105431020,
"parent_rev": "dceb42da3e7d24ffeee93e20c4ce1ce90953bf88", "commit_rev":
"868f32f4230140c72a7b7abd6c3a5bb9d7feef67"}
Message was sent while issue was closed.
Description was changed from ========== AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs over a period of time. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is, as of this CL, done from the real-time audio thread. This CL contains an interface for AecDump, a new APM submodule that will handle the recordings. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc will contain two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDump added. The reasons for the refactoring is to move file IO operations from the real-time audio thread, to add a top-level low-priority task queue for logging tasks like this, to simplify and modularize audio_processing_impl.cc and remove some of the preprocessor directives. These goals will be archived by the upcoming CLs. The implementation is in https://codereview.webrtc.org/2865113002. BUG=webrtc:7404 ========== to ========== AudioProcessingModule has a feature to make a recording of its configuration, inputs and outputs over a period of time. It is activated by AudioProcessing::StartRecording. The data is stored in binary protobuf format in a specified file. The file IO is, as of this CL, done from the real-time audio thread. This CL contains an interface for AecDump, a new APM submodule that will handle the recordings. Calls to the new interface from the AudioProcessingModule are added. These calls have no effect, and for a short while, audio_processing_impl.cc will contain two copies of recording calls. The original calls are guarded by the WEBRTC_AUDIOPROC_DEBUG_DUMP preprocessor define. They still have an effect, while the new ones do not. In the following CLs, the old recording calls will be removed, and an implementation of AecDump added. The reasons for the refactoring is to move file IO operations from the real-time audio thread, to add a top-level low-priority task queue for logging tasks like this, to simplify and modularize audio_processing_impl.cc and remove some of the preprocessor directives. These goals will be archived by the upcoming CLs. The implementation is in https://codereview.webrtc.org/2865113002. BUG=webrtc:7404 Review-Url: https://codereview.webrtc.org/2778783002 Cr-Commit-Position: refs/heads/master@{#18233} Committed: https://chromium.googlesource.com/external/webrtc/+/868f32f4230140c72a7b7abd6... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:980001) as https://chromium.googlesource.com/external/webrtc/+/868f32f4230140c72a7b7abd6... |
