Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2584493002:
Added first layer of the echo canceller 3 functionality (Closed)
|
Created:
4 years ago by peah-webrtc Modified:
3 years, 11 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded first layer of the echo canceller 3 functionality.
This CL adds the first layer of the echo canceller 3.
All of the code is as it should, apart from
block_processor.* which only contains placeholder
functionality. (Upcoming CLs will add proper
functionality into those files.)
BUG=webrtc:6018
Review-Url: https://codereview.webrtc.org/2584493002
Cr-Commit-Position: refs/heads/master@{#15861}
Committed: https://chromium.googlesource.com/external/webrtc/+/38fd1758e90bcdc7690a552e7ef0ec0d143d2f30
Patch Set 1 #Patch Set 2 : Restricted the AnalyzeRender access, added ability to add external reporting of echo failure, and o… #
Total comments: 166
Patch Set 3 : Changes in response to reviewer comments #
Total comments: 146
Patch Set 4 : Changes in response to reviewer comments #Patch Set 5 : Minor changes #
Total comments: 66
Patch Set 6 : Changes in response to reviewer comments #Patch Set 7 : Added more crash tests #
Total comments: 34
Patch Set 8 : Changes in response to reviewer comments #Patch Set 9 : Added checks and unittests for null pointer parameters #Patch Set 10 : Rebase #Patch Set 11 : Changed biquad filter coefficients to be float #Patch Set 12 : Corrected array declaration that caused a build error on Windows #Patch Set 13 : Added missing comment #Patch Set 14 : Disabled the BlockProcessor DEATH tests due to false alarms on the trybots #Messages
Total messages: 36 (6 generated)
Description was changed from ========== First patch First patch First patch BUG= ========== to ========== Added first layer of the echo canceller 3 functionality. This CL adds the first layer of the echo canceller 3. All of the code is as it should, apart from block_processor.* which only contains placeholder functionality. (Upcoming CLs will add proper functionality into those files.) BUG=webrtc:6018 ==========
peah@webrtc.org changed reviewers: + aleloi@webrtc.org, henrik.lundin@webrtc.org, ivoc@webrtc.org
Next CL in the AEC3 CL series: PTAL Thanks!
Hi, I came up with a new patch. Sorry about that! (it improves the code significantly though.
On 2016/12/16 06:59:46, peah-webrtc wrote: > Hi, > I came up with a new patch. Sorry about that! (it improves the code > significantly though. Where are all the unit tests for your new classes?
Nice! But I have a few comments... https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3_constants.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants.h:16: // TODO(peah): Remove when the code is landed. When which code is landed? This CL or all of AEC3? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:19: BlockFramer::BlockFramer(size_t num_bands, size_t frame_length) I'm having some issues with the implementation of this class. It seems to me that you are storing all bands in one and the same vector, but never utilize the fact that they reside in some kind of contiguous memory. So the single vector approach just makes the indexing harder. I would suggest that you have num_bands vectors instead. Then you could also consider using circular arrays/buffers instead of plain vectors, and avoid the shifting. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:50: if (buffer_size_ > 0) { Add a comment that the code below shifts the remaining part of the buffer to the start. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:51: RTC_DCHECK_LE(buffer_size_, kMaxSize); This should always be true; move up to beginning of method. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:22: /* Class for producing 10 ms second frames from 64 sample blocks */ Use // style comments. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:22: /* Class for producing 10 ms second frames from 64 sample blocks */ I'd like to see a little bit more of an explanation. In particular, what is the relationship between "block" and "frame". You insert blocks and extract frames. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:28: void ExtractFrame(size_t sub_frame_index, float* const* frame); Comment, please. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:31: const size_t kMaxSize = 2 * kBlockSize; constexpr https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:22: /* Class for performing echo cancellation on 64 sample blocks of audio data */ // style https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:30: /* Processes a block of capture data */ // style https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:35: /* Buffers a block of render data supplied by a FrameBlocker object */ // style https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:30: CascadedBiQuadFilter::CascadedBiQuadFilter( Order of methods is not the same as in h-file. Move the ctor and dtor to the top. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:37: biquad_state.a[0] = biquad_state.a[1] = 0; Probably doesn't matter, but 0.f. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:38: biquad_state.b[0] = biquad_state.b[1] = 0; 0.f https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:48: const auto coefficients_b = coefficients_.b; Since you are introducing these local variables to simplify notation, I'd go for much shorter names. Consider c_a and c_b. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:50: auto memory_b = biquad_state->b; Following the notation of https://en.wikipedia.org/wiki/Digital_biquad_filter, I would argue that mem_x and mem_y are better. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:21: class CascadedBiQuadFilter { Short comment for the class. Cascades a number of identical (?) biquads. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:24: float b[2]; Slightly confusing that both the state and the coefficients have components 'a' and 'b'. I think the state should have x and y instead. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:37: void Process(rtc::ArrayView<const float> x, rtc::ArrayView<float> y); Comment on the Process methods, and how they differ. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:49: if (capture_blocker->IsBlockAvailable()) { Nit: I prefer early return. if (!capture_blocker->IsBlockAvailable()) { return; } ... https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:51: capture_blocker->ExtractBlockIfAvailable(capture_block); ...IfAvailable? I thought we established that it was available. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:81: void CopyAudioBufferIntoFrame(AudioBuffer* buffer, I assume this cannot be made a const argument because of magic happening inside the buffer. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:117: : data_dumper_(data_dumper), Is nullptr valid? Otherwise, add a DCHECK. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:127: bool EchoCanceller3::RenderWriterState::Insert(AudioBuffer* render) { "render" is a dubious name here. I think "input" would me more descriptive. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:128: RTC_DCHECK_EQ(1u, render->num_channels()); Nit: I think you no longer need the 'u' suffix, as a result of https://codereview.webrtc.org/2535593002. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:150: num_bands_(sample_rate_hz_ == 8000 ? 1 : sample_rate_hz / 16000), You have this expression defined as NumBandsForRate(sample_rate_hz) in aec3_constants.h. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:174: std::move(render_highpass_filter), sample_rate_hz_, What happens when you move from an (explicitly) uninitialized unique_ptr? Are you sure you get a nullptr as expected? From http://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr, it seems that the default ctor value-initializes the pointer, which I assume sets it to nullptr. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:188: bool EchoCanceller3::EmptyRenderQueue() { Check the order of the methods in this file, and make sure it is the same as in the h-file. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:214: saturated_microphone_signal_ = false; You reset saturated_microphone_signal_ both here and at the end of ProcessCapture. Is this intentional? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:216: saturated_microphone_signal_ |= I assume you will expand the work of this for loop later one. Otherwise, you can do early return on finding the first saturation. for () { if (DetectSaturation(blah) { saturated_microphone_signal_ = true; return; } } https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:34: // The class is supposed to be used in a single-threaded manner What does single-threaded mean here? Should all methods except AnalyzeRender be called from the same thread, or just not at the same time? Is there anything that enforces this design? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:65: class RenderWriterState { "State" suggests that this is a simple data container, but it is more than that. Consider renaming to RenderWriter. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:104: bool EmptyRenderQueue(); Declare methods before data members. https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:1: /* Order of methods is wrong. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:31: rtc::ArrayView<const float> frame_view[num_bands_]; Please, comment that this is an array of ArrayViews. The sloppy reader might interpret this as a single array view of size num_bands_. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:37: InsertFrameAndExtractBlock(rtc::ArrayView<const rtc::ArrayView<const float>>( An ArrayView of ArrayViews... I think a vector<ArrayView<const float>> would be better. Pass it as a const ref. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:46: rtc::ArrayView<const float> frame_view[num_bands_]; Again, consider using a vector. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:56: void FrameBlocker::InsertFrameAndExtractBlock( I need a better explanation of what is supposed to happen here. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:57: rtc::ArrayView<const rtc::ArrayView<const float>> frame, Aha! Here is the root of all evil. :) I would recommend const std::vector<rtc::ArrayView<const float>>& instead. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:59: RTC_DCHECK_LE(buffer_size_, kBlockSize); DCHECK that block is long enough. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:23: /* Class for producing 64 sample multiband blocks from 10 ms frames */ // style https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:26: explicit FrameBlocker(size_t num_bands, size_t frame_length); Drop 'explicit'. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:43: void InsertFrameAndExtractBlock( Methods before data members. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_transfer_buffer.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.cc:17: RenderTransferBufferWriter::~RenderTransferBufferWriter() {} = default https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.cc:25: queue_.Clear(); What does Clear() do on top of the ctor? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.cc:28: RenderTransferBuffer::~RenderTransferBuffer() {} = default https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.cc:35: bool result = queue_.Remove(frame); Why the local storage in result? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_transfer_buffer.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:34: bool Insert(std::vector<float>* frame) override; Comment on the fact that frame will be swapped. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:35: bool Remove(std::vector<float>* frame); And here. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:38: static const size_t kQueueSize = 30; constexpr
Some comments. I've not checked the biquad_filter.* and echo_canceller3.* yet. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3_constants.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants.h:33: return sample_rate_hz == 8000 ? 1 : sample_rate_hz / 16000; I think some of our compilers will complain here. It's a signed->unsigned conversion. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:19: BlockFramer::BlockFramer(size_t num_bands, size_t frame_length) On 2016/12/16 10:04:46, hlundin-webrtc wrote: > I'm having some issues with the implementation of this class. > > It seems to me that you are storing all bands in one and the same vector, but > never utilize the fact that they reside in some kind of contiguous memory. So > the single vector approach just makes the indexing harder. I would suggest that > you have num_bands vectors instead. Then you could also consider using circular > arrays/buffers instead of plain vectors, and avoid the shifting. +1 Does anybody know if we have a stock circular buffer somewhere? I found base/bufferqueue.h, but it seems to have some draw-backs. Only bytes can be read/written and it's thread-safe. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:28: RTC_DCHECK_LE(buffer_size_ + kBlockSize, kMaxSize); There is an assumption here that the block is (at least) num_bands_*kBlockSize large, right? I think we could add a check for the size of |block|, since ArrayView::subview doesn't have any bound checking. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:38: void BlockFramer::ExtractFrame(size_t sub_frame_index, float* const* frame) { Is there some way to avoid the naked pointers in the output argument? Is it possible to make |frame| be a vector of ArrayViews instead that is filled by this method? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:22: /* Class for producing 10 ms second frames from 64 sample blocks */ On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Use // style comments. Remove 'second' https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:28: void ExtractFrame(size_t sub_frame_index, float* const* frame); Maybe document the threading model and use base/thread_annotations.h? Document that Extract and Insert must accessed sequentially. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:37: bool BufferRender(F&& blocker) { Perhaps change type to std::function<void(ArrayView)> &&, to improve readability? I don't think that the r-value ref is necessary here to avoid copies. If a temporary is passed by value, I think copy-elision will apply (http://en.cppreference.com/w/cpp/language/copy_elision, in the call g(f()) no copy of the f() result is made). Also, the style guide discourages using r-value refs for other things than move-assignment, construction and forwarding: https://google.github.io/styleguide/cppguide.html#Rvalue_references https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:30: RTC_DCHECK_LE(sub_frame_index * kSubFrameLength, frame_length_); Please DCHECK that the frame is large enough. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:35: kSubFrameLength); I think this does the same thing: ... = frame.subview(i * frame_length_ + kSubFrameLength * sub_frame_index, kSubFrameLength) https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:61: I think this is complex enough to warrant a few unit tests to check that data is written to the right place of |block|, that there are no reads outside of the buffer, that band_start_index is updated correctly. If there is one buffer for each band, it will be a little simpler, though. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:40: std::vector<float> buffer_; I think the copying and indexing in *ExtractBlock* will be simpler if there is one buffer for every band. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_transfer_buffer.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:26: virtual ~RenderTransferBufferWriter() = 0; For some reason, the style guide says that dtors should not be pure virtual: https://google.github.io/styleguide/cppguide.html#Interfaces https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:40: std::vector<float> frame_; Will frame_ be used later? It's not used at the moment except during initialization.
I will have a closer look at echo_canceller3.cc later. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:22: (void)num_bands; I'm assuming this is to stop the compiler from complaining about unused variables? Another way to handle that is to comment out the name of the variable in the function, i.e.: BlockProcessor::BlockProcessor(ApmDataDumper* /*data_dumper*/, https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:37: biquad_state.a[0] = biquad_state.a[1] = 0; On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Probably doesn't matter, but 0.f. Is it possible to do this initialization in the header? i.e. float a[2] = {0.f, 0.f}; https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:21: class CascadedBiQuadFilter { On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Short comment for the class. Cascades a number of identical (?) biquads. It would also be nice to make a note of which form this class implements, i.e. direct form 1, 2 or something else. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:23: struct BiQuadState { I think you can consider restructuring this a little bit. I think it would be nice to have one class called BiQuadFilter that implements a single filter. It could have it's own state and coefficients as members and it should have a process function. WDYT? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:45: const size_t num_biquads_; I don't think this is used anywhere. Also, the size is available from the vector if you need it later. So, I think it should be removed. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:21: if (y_k > 32767.0f || y_k < -32768.0f) { How about if the signals are equal to the limits? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:67: }); Can you explain what this is supposed to do? The way the code looks now doesn't make much sense to me. For example, this lambda is passed to the block processor (why?), and is never executed there. I guess some steps will be added later? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:65: class RenderWriterState { On 2016/12/16 10:04:47, hlundin-webrtc wrote: > "State" suggests that this is a simple data container, but it is more than that. > Consider renaming to RenderWriter. Since this class is private and EchoCanceller3 only has a unique_ptr to it, I think you can move the implementation to the .cc file and put a forward declaration in the header. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:40: std::vector<float> buffer_; On 2016/12/16 15:04:31, aleloi wrote: > I think the copying and indexing in *ExtractBlock* will be simpler if there is > one buffer for every band. Perhaps there could even be a subclass to handle one of these buffers, with methods for inserting and extracting. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_transfer_buffer.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:38: static const size_t kQueueSize = 30; On 2016/12/16 10:04:48, hlundin-webrtc wrote: > constexpr Move to .cc file?
Thanks for the great feedback!!! I have made some quite major changes in the code in response to the comments. The main things changed are that 1) Unittests are added. 2) BlockProcessor was made an interface in order to allow testing of the EchoCanceller3 layer. 3) The blocks are now stored as vectors of vectors. PTAL https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3_constants.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants.h:16: // TODO(peah): Remove when the code is landed. On 2016/12/16 10:04:46, hlundin-webrtc wrote: > When which code is landed? This CL or all of AEC3? Sorry, the comment is incorrect. It is currently used for result analysis during development. I altered the comment to be more correct. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants.h:33: return sample_rate_hz == 8000 ? 1 : sample_rate_hz / 16000; On 2016/12/16 15:04:31, aleloi wrote: > I think some of our compilers will complain here. It's a signed->unsigned > conversion. Good point! Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:19: BlockFramer::BlockFramer(size_t num_bands, size_t frame_length) On 2016/12/16 10:04:46, hlundin-webrtc wrote: > I'm having some issues with the implementation of this class. > > It seems to me that you are storing all bands in one and the same vector, but > never utilize the fact that they reside in some kind of contiguous memory. So > the single vector approach just makes the indexing harder. I would suggest that > you have num_bands vectors instead. Then you could also consider using circular > arrays/buffers instead of plain vectors, and avoid the shifting. Good point! I altered to vectors of vectors. Regarding the circular buffers I don't think that anything is gained by that. Basically by shifting you refer to the copy on line 53, right? My concern with circular buffers is that the wraparound behavior and length of the input and outputs create a fairly complicated wraparound behavior. That means that you either get really complicated code with a number of for-loops or need to use a circularly updated buffer index variable (of the form k = (k+1)%size). My feeling is that loops with circularly updated buffer indices are quite hard for the compiler to do efficiently as the memory that is copied is not continuous. Therefore I think that it is better to do the copying as several memcopies instead, which should be very efficient. WDYT? I don't think we gain anyth https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:19: BlockFramer::BlockFramer(size_t num_bands, size_t frame_length) On 2016/12/16 15:04:31, aleloi wrote: > On 2016/12/16 10:04:46, hlundin-webrtc wrote: > > I'm having some issues with the implementation of this class. > > > > It seems to me that you are storing all bands in one and the same vector, but > > never utilize the fact that they reside in some kind of contiguous memory. So > > the single vector approach just makes the indexing harder. I would suggest > that > > you have num_bands vectors instead. Then you could also consider using > circular > > arrays/buffers instead of plain vectors, and avoid the shifting. > > +1 Does anybody know if we have a stock circular buffer somewhere? I found > base/bufferqueue.h, but it seems to have some draw-backs. Only bytes can be > read/written and it's thread-safe. We have a stock circular buffer available, but that is C-based and does not have a nice implementation. I'll change the code to not have the extra shifting of the data. I'm not sure whether it would be faster to have a circular buffer though. PTAL https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:28: RTC_DCHECK_LE(buffer_size_ + kBlockSize, kMaxSize); On 2016/12/16 15:04:31, aleloi wrote: > There is an assumption here that the block is (at least) num_bands_*kBlockSize > large, right? I think we could add a check for the size of |block|, since > ArrayView::subview doesn't have any bound checking. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:38: void BlockFramer::ExtractFrame(size_t sub_frame_index, float* const* frame) { On 2016/12/16 15:04:31, aleloi wrote: > Is there some way to avoid the naked pointers in the output argument? Is it > possible to make |frame| be a vector of ArrayViews instead that is filled by > this method? That is a great point! I did that instead by wrapping the frames into arrayviews before the call to BlockFramer and FrameBlocker. PTAL https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:50: if (buffer_size_ > 0) { On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Add a comment that the code below shifts the remaining part of the buffer to the > start. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:51: RTC_DCHECK_LE(buffer_size_, kMaxSize); On 2016/12/16 10:04:46, hlundin-webrtc wrote: > This should always be true; move up to beginning of method. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:22: /* Class for producing 10 ms second frames from 64 sample blocks */ On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Use // style comments. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:22: /* Class for producing 10 ms second frames from 64 sample blocks */ On 2016/12/16 10:04:46, hlundin-webrtc wrote: > I'd like to see a little bit more of an explanation. In particular, what is the > relationship between "block" and "frame". You insert blocks and extract frames. I improved it a bit. PTAL https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:22: /* Class for producing 10 ms second frames from 64 sample blocks */ On 2016/12/16 15:04:31, aleloi wrote: > On 2016/12/16 10:04:46, hlundin-webrtc wrote: > > Use // style comments. > > Remove 'second' Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:28: void ExtractFrame(size_t sub_frame_index, float* const* frame); On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Comment, please. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:28: void ExtractFrame(size_t sub_frame_index, float* const* frame); On 2016/12/16 15:04:31, aleloi wrote: > Maybe document the threading model and use base/thread_annotations.h? Document > that Extract and Insert must accessed sequentially. I don't think we should add thread annotations as this class is supposed not to be used in a concurrent manner and afaics it is not used in that way either. But if there is any annotations available that ensure the proper call order, that would be awesome. Agree on the documentation. Added that. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:31: const size_t kMaxSize = 2 * kBlockSize; On 2016/12/16 10:04:46, hlundin-webrtc wrote: > constexpr Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.cc:22: (void)num_bands; On 2016/12/19 11:30:21, ivoc wrote: > I'm assuming this is to stop the compiler from complaining about unused > variables? Another way to handle that is to comment out the name of the variable > in the function, i.e.: > BlockProcessor::BlockProcessor(ApmDataDumper* /*data_dumper*/, Yes, that was the intention. But I now see that it was not needed for these, so I removed them. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:22: /* Class for performing echo cancellation on 64 sample blocks of audio data */ On 2016/12/16 10:04:46, hlundin-webrtc wrote: > // style Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:30: /* Processes a block of capture data */ On 2016/12/16 10:04:46, hlundin-webrtc wrote: > // style Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:35: /* Buffers a block of render data supplied by a FrameBlocker object */ On 2016/12/16 10:04:46, hlundin-webrtc wrote: > // style Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:37: bool BufferRender(F&& blocker) { On 2016/12/16 15:04:31, aleloi wrote: > Perhaps change type to std::function<void(ArrayView)> &&, to improve > readability? > > I don't think that the r-value ref is necessary here to avoid copies. If a > temporary is passed by value, I think copy-elision will apply > (http://en.cppreference.com/w/cpp/language/copy_elision, in the call g(f()) no > copy of the f() result is made). > > Also, the style guide discourages using r-value refs for other things than > move-assignment, construction and forwarding: > https://google.github.io/styleguide/cppguide.html#Rvalue_references That are definitely valid points! I addressed these, and some of the other comments, by replacing this approach to instead rely on the swap capabilities of std::vector. PTAL https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:30: CascadedBiQuadFilter::CascadedBiQuadFilter( On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Order of methods is not the same as in h-file. Move the ctor and dtor to the > top. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:37: biquad_state.a[0] = biquad_state.a[1] = 0; On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Probably doesn't matter, but 0.f. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:37: biquad_state.a[0] = biquad_state.a[1] = 0; On 2016/12/19 11:30:21, ivoc wrote: > On 2016/12/16 10:04:46, hlundin-webrtc wrote: > > Probably doesn't matter, but 0.f. > > Is it possible to do this initialization in the header? i.e. float a[2] = {0.f, > 0.f}; Not as far as I can see. Afaics brace initialization does not seem to be allowed in initializer lists. But I moved this to the constructor of the state which made better code that what it was before (I think). Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:38: biquad_state.b[0] = biquad_state.b[1] = 0; On 2016/12/16 10:04:46, hlundin-webrtc wrote: > 0.f Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:48: const auto coefficients_b = coefficients_.b; On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Since you are introducing these local variables to simplify notation, I'd go for > much shorter names. Consider c_a and c_b. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:50: auto memory_b = biquad_state->b; On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Following the notation of https://en.wikipedia.org/wiki/Digital_biquad_filter, I > would argue that mem_x and mem_y are better. I'll change to _x and _y. I think that since we anyway abbreviate, I'll use m_ as the abbreviation. WDYT? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:21: class CascadedBiQuadFilter { On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Short comment for the class. Cascades a number of identical (?) biquads. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:21: class CascadedBiQuadFilter { On 2016/12/19 11:30:22, ivoc wrote: > On 2016/12/16 10:04:46, hlundin-webrtc wrote: > > Short comment for the class. Cascades a number of identical (?) biquads. > > It would also be nice to make a note of which form this class implements, i.e. > direct form 1, 2 or something else. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:23: struct BiQuadState { On 2016/12/19 11:30:22, ivoc wrote: > I think you can consider restructuring this a little bit. I think it would be > nice to have one class called BiQuadFilter that implements a single filter. It > could have it's own state and coefficients as members and it should have a > process function. WDYT? It would be nicer code-wise but I prefer to have it as one class as it will be faster (due to the same coefficients being used for all states) and have a lower memory footprint, as long as not biquads of different coefficients are used (I think the far most common thing is to have biquads of the same coefficients). Even though only one biquad is used in cascade in the current code, another part of the aec3 code that is yet to be landed is using this functionality with more than one biquad in cascade. WDYT? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:24: float b[2]; On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Slightly confusing that both the state and the coefficients have components 'a' > and 'b'. I think the state should have x and y instead. Agree. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:37: void Process(rtc::ArrayView<const float> x, rtc::ArrayView<float> y); On 2016/12/16 10:04:46, hlundin-webrtc wrote: > Comment on the Process methods, and how they differ. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:45: const size_t num_biquads_; On 2016/12/19 11:30:22, ivoc wrote: > I don't think this is used anywhere. Also, the size is available from the vector > if you need it later. So, I think it should be removed. Great find! Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:21: if (y_k > 32767.0f || y_k < -32768.0f) { On 2016/12/19 11:30:22, ivoc wrote: > How about if the signals are equal to the limits? Good find! Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:49: if (capture_blocker->IsBlockAvailable()) { On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Nit: I prefer early return. > if (!capture_blocker->IsBlockAvailable()) { > return; > } > ... Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:51: capture_blocker->ExtractBlockIfAvailable(capture_block); On 2016/12/16 10:04:47, hlundin-webrtc wrote: > ...IfAvailable? I thought we established that it was available. True, I changed the implementation of FrameBlocker accordingly. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:67: }); On 2016/12/19 11:30:22, ivoc wrote: > Can you explain what this is supposed to do? The way the code looks now doesn't > make much sense to me. For example, this lambda is passed to the block processor > (why?), and is never executed there. I guess some steps will be added later? Yes, the reason for this construct is to provide a cheap way of producing a render block from a frame and passing it to the BufferRender method. The reason for why it is not used in the block_processor is that that functionality is to be added in the next CL. However, with the code changes in the new patch, this lambda is no longer used and I instead use the vector-swap functionality for efficiently passing the blocks. PTAL https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:81: void CopyAudioBufferIntoFrame(AudioBuffer* buffer, On 2016/12/16 10:04:47, hlundin-webrtc wrote: > I assume this cannot be made a const argument because of magic happening inside > the buffer. Afaics, that is the case. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:117: : data_dumper_(data_dumper), On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Is nullptr valid? Otherwise, add a DCHECK. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:127: bool EchoCanceller3::RenderWriterState::Insert(AudioBuffer* render) { On 2016/12/16 10:04:46, hlundin-webrtc wrote: > "render" is a dubious name here. I think "input" would me more descriptive. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:128: RTC_DCHECK_EQ(1u, render->num_channels()); On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Nit: I think you no longer need the 'u' suffix, as a result of > https://codereview.webrtc.org/2535593002. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:150: num_bands_(sample_rate_hz_ == 8000 ? 1 : sample_rate_hz / 16000), On 2016/12/16 10:04:47, hlundin-webrtc wrote: > You have this expression defined as NumBandsForRate(sample_rate_hz) in > aec3_constants.h. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:174: std::move(render_highpass_filter), sample_rate_hz_, On 2016/12/16 10:04:47, hlundin-webrtc wrote: > What happens when you move from an (explicitly) uninitialized unique_ptr? Are > you sure you get a nullptr as expected? From > http://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr, it seems that the > default ctor value-initializes the pointer, which I assume sets it to nullptr. Good point! I cannot find whether it actually initializes it to null though. I added that specifically. PTAL https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:188: bool EchoCanceller3::EmptyRenderQueue() { On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Check the order of the methods in this file, and make sure it is the same as in > the h-file. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:214: saturated_microphone_signal_ = false; On 2016/12/16 10:04:47, hlundin-webrtc wrote: > You reset saturated_microphone_signal_ both here and at the end of > ProcessCapture. Is this intentional? Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:216: saturated_microphone_signal_ |= On 2016/12/16 10:04:47, hlundin-webrtc wrote: > I assume you will expand the work of this for loop later one. Otherwise, you can > do early return on finding the first saturation. > for () { > if (DetectSaturation(blah) { > saturated_microphone_signal_ = true; > return; > } > } There will be no more work on this loop. I added early return. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:34: // The class is supposed to be used in a single-threaded manner On 2016/12/16 10:04:47, hlundin-webrtc wrote: > What does single-threaded mean here? Should all methods except AnalyzeRender be > called from the same thread, or just not at the same time? Is there anything > that enforces this design? Good point. I changed the comment to be more clear (I think). What I mean is not single-threadedness but rather concurrent calling. There is nothing that enforces this, and I'm not sure that we should do that. I don't want to add a lock just to ensure that the class in not used in the correct manner. The thing is, the echo canceller is not intended for cuncurrent usage, apart from the AnalyzeRender call. That call is safe as it does not use any common resources with the rest of the code, apart from the SwapQueue which is thread safe. WDYT? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:65: class RenderWriterState { On 2016/12/16 10:04:47, hlundin-webrtc wrote: > "State" suggests that this is a simple data container, but it is more than that. > Consider renaming to RenderWriter. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:65: class RenderWriterState { On 2016/12/19 11:30:22, ivoc wrote: > On 2016/12/16 10:04:47, hlundin-webrtc wrote: > > "State" suggests that this is a simple data container, but it is more than > that. > > Consider renaming to RenderWriter. > > Since this class is private and EchoCanceller3 only has a unique_ptr to it, I > think you can move the implementation to the .cc file and put a forward > declaration in the header. Good point! Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:104: bool EmptyRenderQueue(); On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Declare methods before data members. > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:1: /* On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Order of methods is wrong. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:30: RTC_DCHECK_LE(sub_frame_index * kSubFrameLength, frame_length_); On 2016/12/16 15:04:31, aleloi wrote: > Please DCHECK that the frame is large enough. I'm not sure what you mean here. The frame is the input, and this method does not store anything but instead passes the data back on the next InsertFrameAndExtractBlock method. I'd rather put more DCHECKS there if needed. WDYT? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:31: rtc::ArrayView<const float> frame_view[num_bands_]; On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Please, comment that this is an array of ArrayViews. The sloppy reader might > interpret this as a single array view of size num_bands_. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:35: kSubFrameLength); On 2016/12/16 15:04:31, aleloi wrote: > I think this does the same thing: > ... = frame.subview(i * frame_length_ + kSubFrameLength * sub_frame_index, > kSubFrameLength) Great! That is definitely better. I changed to that. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:37: InsertFrameAndExtractBlock(rtc::ArrayView<const rtc::ArrayView<const float>>( On 2016/12/16 10:04:47, hlundin-webrtc wrote: > An ArrayView of ArrayViews... I think a vector<ArrayView<const float>> would be > better. Pass it as a const ref. What would be the benefit of that be? I changed the array size to a compile time-constant and then that should be faster than a vector allocation. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:46: rtc::ArrayView<const float> frame_view[num_bands_]; On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Again, consider using a vector. Please see question above. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:56: void FrameBlocker::InsertFrameAndExtractBlock( On 2016/12/16 10:04:47, hlundin-webrtc wrote: > I need a better explanation of what is supposed to happen here. True. I added that into the header file. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:57: rtc::ArrayView<const rtc::ArrayView<const float>> frame, On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Aha! Here is the root of all evil. :) > I would recommend const std::vector<rtc::ArrayView<const float>>& instead. Please see question above. I don't see why we want to allocate a vector for that. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:59: RTC_DCHECK_LE(buffer_size_, kBlockSize); On 2016/12/16 10:04:47, hlundin-webrtc wrote: > DCHECK that block is long enough. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:61: On 2016/12/16 15:04:31, aleloi wrote: > I think this is complex enough to warrant a few unit tests to check that data is > written to the right place of |block|, that there are no reads outside of the > buffer, that band_start_index is updated correctly. If there is one buffer for > each band, it will be a little simpler, though. I fully agree. I have now changed to one band per buffer, and added unit tests. PTAL https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:23: /* Class for producing 64 sample multiband blocks from 10 ms frames */ On 2016/12/16 10:04:47, hlundin-webrtc wrote: > // style Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:26: explicit FrameBlocker(size_t num_bands, size_t frame_length); On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Drop 'explicit'. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:40: std::vector<float> buffer_; On 2016/12/16 15:04:31, aleloi wrote: > I think the copying and indexing in *ExtractBlock* will be simpler if there is > one buffer for every band. Agree. I did it now as hundin@ suggested with a vector of vectors. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:40: std::vector<float> buffer_; On 2016/12/19 11:30:22, ivoc wrote: > On 2016/12/16 15:04:31, aleloi wrote: > > I think the copying and indexing in *ExtractBlock* will be simpler if there is > > one buffer for every band. > > Perhaps there could even be a subclass to handle one of these buffers, with > methods for inserting and extracting. That would definitely make sense and the initial version (before this CL) had a specific class for that. However, the benefits of vectors outweigh that, I think, and with the new patch I think the code is in a better state. WDYT? https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:43: void InsertFrameAndExtractBlock( On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Methods before data members. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_transfer_buffer.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.cc:17: RenderTransferBufferWriter::~RenderTransferBufferWriter() {} On 2016/12/16 10:04:48, hlundin-webrtc wrote: > = default I have removed this class in the new patch. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.cc:25: queue_.Clear(); On 2016/12/16 10:04:47, hlundin-webrtc wrote: > What does Clear() do on top of the ctor? I have removed this class in the new patch. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.cc:28: RenderTransferBuffer::~RenderTransferBuffer() {} On 2016/12/16 10:04:48, hlundin-webrtc wrote: > = default I have removed this class in the new patch. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.cc:35: bool result = queue_.Remove(frame); On 2016/12/16 10:04:47, hlundin-webrtc wrote: > Why the local storage in result? I have removed this class in the new patch. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_transfer_buffer.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:26: virtual ~RenderTransferBufferWriter() = 0; On 2016/12/16 15:04:32, aleloi wrote: > For some reason, the style guide says that dtors should not be pure virtual: > https://google.github.io/styleguide/cppguide.html#Interfaces Good find! I have removed this class in the new patch. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:34: bool Insert(std::vector<float>* frame) override; On 2016/12/16 10:04:48, hlundin-webrtc wrote: > Comment on the fact that frame will be swapped. have removed this class in the new patch. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:35: bool Remove(std::vector<float>* frame); On 2016/12/16 10:04:48, hlundin-webrtc wrote: > And here. I have removed this class in the new patch. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:38: static const size_t kQueueSize = 30; On 2016/12/16 10:04:48, hlundin-webrtc wrote: > constexpr I have removed this class in the new patch. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:38: static const size_t kQueueSize = 30; On 2016/12/19 11:30:22, ivoc wrote: > On 2016/12/16 10:04:48, hlundin-webrtc wrote: > > constexpr > > Move to .cc file? I have removed this class in the new patch. Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:40: std::vector<float> frame_; On 2016/12/16 15:04:31, aleloi wrote: > Will frame_ be used later? It's not used at the moment except during > initialization. Fully true, that should be removed. I have removed this class in the new patch though. Done.
Much improved. I'm still missing unit tests for BlockFramer, BlockProcessor, CascadedBiquadFilter, and FrameBlocker. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:50: auto memory_b = biquad_state->b; On 2016/12/20 10:10:24, peah-webrtc wrote: > On 2016/12/16 10:04:46, hlundin-webrtc wrote: > > Following the notation of https://en.wikipedia.org/wiki/Digital_biquad_filter, > I > > would argue that mem_x and mem_y are better. > > I'll change to _x and _y. I think that since we anyway abbreviate, I'll use m_ > as the abbreviation. > > WDYT? Acknowledged. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:34: // The class is supposed to be used in a single-threaded manner On 2016/12/20 10:10:25, peah-webrtc wrote: > On 2016/12/16 10:04:47, hlundin-webrtc wrote: > > What does single-threaded mean here? Should all methods except AnalyzeRender > be > > called from the same thread, or just not at the same time? Is there anything > > that enforces this design? > > Good point. I changed the comment to be more clear (I think). What I mean is not > single-threadedness but rather concurrent calling. > > There is nothing that enforces this, and I'm not sure that we should do that. I > don't want to add a lock just to ensure that the class in not used in the > correct manner. > > The thing is, the echo canceller is not intended for cuncurrent usage, apart > from the AnalyzeRender call. That call is safe as it does not use any common > resources with the rest of the code, apart from the SwapQueue which is thread > safe. > > > WDYT? I propose you use a RaceChecker, similarly to how it is used in audio_mixer_impl.{cc,h}. You will then need two RaceChecker members in the state; one for capture and one for render. Then you should annotate all state variables except the SwapQueue with the correct GUARDED_BY. Finally, use RTC_DCHECK_RUNS_SERIALIZED with the correct RaceChecker member in each method. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:37: InsertFrameAndExtractBlock(rtc::ArrayView<const rtc::ArrayView<const float>>( On 2016/12/20 10:10:26, peah-webrtc wrote: > On 2016/12/16 10:04:47, hlundin-webrtc wrote: > > An ArrayView of ArrayViews... I think a vector<ArrayView<const float>> would > be > > better. Pass it as a const ref. > > What would be the benefit of that be? I changed the array size to a compile > time-constant and then that should be faster than a vector allocation. > Acknowledged. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3_constants.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants.h:16: // TODO(peah): Remove when the all code is landed and activated by defaule. defaule -> default https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:30: buffer_[i].resize(block[i].size()); I think you can simplify these two lines to a single buffer_[i].insert(buffer_[i].begin(), block[i].begin(), block[i].end()); It will do the resizing for you. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:46: std::copy(buffer_[i].begin(), buffer_[i].end(), sub_frame[i].begin()); You can do sub_frame[i].insert(...) here, and skip the DCHECK on the size of sub_frame[i], if you want to. But that is optional. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:47: std::copy(block[i].begin(), block[i].begin() + samples_to_frame, Same here; again, optional. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:49: buffer_[i].resize(kBlockSize - samples_to_frame); I don't think you have to resize; simply insert(). https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:23: // from from 64 sample blocks. from from https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:44: class BlockProcessorImpl : public BlockProcessor { final https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:44: class BlockProcessorImpl : public BlockProcessor { Since you have a Create method in the interface class, and the method is implemented in the .cc file, you can actually move also the declaration of BlockProcessorImpl to the .cc file. This is how it is done in for audio_coding_module.cc. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:49: // Processes a block of capture data. You can skip all the duplicated method comments. They will only diverge over time. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:68: rtc::ArrayView<float> sub_frame_view_data[block->size()]; This is not pretty. Sorry... Is there any way we can improve this code? https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:64: BlockProcessor* block_processor); It is not obvious to the caller that the ownership of block_processor is transferred. Use an std::unique_ptr. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:24: void PopulateInputFrame(size_t frame_length, Please, comment on how the frame is filled. What values? https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:40: float* const* frame, More const. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:80: // Class for testing that echo leakage is properly reported to the This is not used for the next 400 lines. Define this class closer to where it is used. But, as I comment down below, use a mock instead, and define it in a separate file under aec3/mock/. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:100: BlockProcessorEchoLeakageReport::BlockProcessorEchoLeakageReport( Merge definitions into the class declaration above. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:123: // Class for testing that echo leakage is properly reported to the Copy-paste failure with the comment. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:209: class BlockProcessorRenderRedirect : public BlockProcessor { Move closer to where it is used. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:227: BlockProcessorRenderRedirect::BlockProcessorRenderRedirect(size_t num_bands) {} Merge definitions into class declaration. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:268: BlockProcessorTransparentCapture::BlockProcessorTransparentCapture( Merge definitions into class declaration. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:286: void RunCapturePipelineVerificationTest(int sample_rate_hz) { Write a comment about what the test actually tests. "Verifies blah bleh delay of 64 samples because of block-blah..." https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:287: const size_t num_bands = NumBandsForRate(sample_rate_hz); You are duplicating these 10-20 lines an awful lot of times throughout the file. Consolidate somehow. You can for instance put them all into a helper struct/class that does this initialization in the ctor. I could even consider accepting a test fixture in this case. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:389: new BlockProcessorKnownEchoPathChangeReport(num_bands)); Use a mock instead to verify that BlockProcessor::ProcessCapture is called with the correct parameters. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:404: default: You can omit the default case. Since you are switching on an enum (class), the compiler will tell you if you forgot to handle any of the enum values. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:436: default: Remove default. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:470: new BlockProcessorEchoLeakageReport(num_bands)); Rewrite this test to use a mock instead of BlockProcessorEchoLeakageReport. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:494: default: Remove default. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:533: default: Remove default. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:567: new BlockProcessorCaptureSaturationReport(num_bands)); Use a mock instead to verify that BlockProcessor::ProcessCapture is called with the correct parameters. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:587: default: Remove default. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:624: default: Remove default. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:630: void RunRenderPipelineSwapQueueTest(int sample_rate_hz) { Comment on what this test does. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:679: void RunRenderPipelineSwapQueueOverrunReturnValueTest(int sample_rate_hz) { Comment... https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:718: void RunBlockerAndFramerTest(int sample_rate_hz) { This looks like unit tests for FrameBlocker and BlockFramer. Should be in separate file(s). https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:769: for (auto rate : {8000, 16000, 32000, 48000}) { This makes it tricky to know which test case is actually failing. Consider one of these options: 1. Create a separate TEST for each sample rate (this explodes a bit with your two-dimensional test matrices below). 2. Use a SCOPED_TRACE to get additional printouts. 3. (Less nice) Use TEST_P. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:22: buffer_(num_bands_, std::vector<float>(kBlockSize, 0.f)) { Instead of setting up vectors of length kBlockSize and then resizing them to zero, I propose the following: buffer_(num_bands_) { for (auto& b : buffer_) { b.reserve(kBlockSize); RTC_DCHECK(b.empty()); // Not needed, but maybe nice? } } https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:40: std::copy(buffer_[i].begin(), buffer_[i].end(), (*block)[i].begin()); You can use vector::insert here too. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:41: std::copy(sub_frame[i].begin(), sub_frame[i].begin() + samples_to_block, And here. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:44: std::copy(sub_frame[i].begin() + samples_to_block, sub_frame[i].end(), And here, and skip the resize. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:59: std::copy(buffer_[i].begin(), buffer_[i].end(), (*block)[i].begin()); vector::insert https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:60: buffer_[i].resize(0); clear()
New batch of comments... It looks good! I've only got the unit tests left now. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:19: BlockFramer::BlockFramer(size_t num_bands, size_t frame_length) Acknowledged. The new version looks very good! https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:38: void BlockFramer::ExtractFrame(size_t sub_frame_index, float* const* frame) { On 2016/12/20 10:10:24, peah-webrtc wrote: > On 2016/12/16 15:04:31, aleloi wrote: > > Is there some way to avoid the naked pointers in the output argument? Is it > > possible to make |frame| be a vector of ArrayViews instead that is filled by > > this method? > > That is a great point! I did that instead by wrapping the frames into arrayviews > before the call to BlockFramer and FrameBlocker. > > PTAL Acknowledged. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:61: On 2016/12/20 10:10:26, peah-webrtc wrote: > On 2016/12/16 15:04:31, aleloi wrote: > > I think this is complex enough to warrant a few unit tests to check that data > is > > written to the right place of |block|, that there are no reads outside of the > > buffer, that band_start_index is updated correctly. If there is one buffer for > > each band, it will be a little simpler, though. > > I fully agree. I have now changed to one band per buffer, and added unit tests. > > PTAL I like it now! https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_transfer_buffer.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:26: virtual ~RenderTransferBufferWriter() = 0; On 2016/12/16 15:04:32, aleloi wrote: > For some reason, the style guide says that dtors should not be pure virtual: > https://google.github.io/styleguide/cppguide.html#Interfaces If someone knows why, please enlighten me! Here's my hypothesis: a pure virtual dtor means that all derived classes must implement it. But that usually means writing '{}' or '=default' in lots of places where the compiler would implicitly generate the same thing. The style guide has no rule for always writing a dtor, but a pure virtual dtor implies a mandatory requirement for exactly that. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3_constants.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants.h:16: // TODO(peah): Remove when the all code is landed and activated by defaule. misspell https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:29: RTC_DCHECK_EQ(0, buffer_[i].size()); It seems that the BlockFramer methods can only be called in this sequence, otherwise the buffer is not empty when InsertBlock is called, or there is not enough samples for a whole subframe in InsertBlockAndExtractFrame. Did I get it right? InsertBlock # 64 samples after this step InsertBlockAndExtractSubFrame # 2*64 - 80 = 48 samples left InsertBlockAndExtractSubFrame # 48+64- 80 = 32 InsertBlockAndExtractSubFrame # 32+64- 80 = 16 InsertBlockAndExtractSubFrame # 16+64- 80 = 0 InsertBlock # 64 samples again That this works depends on the exact values of kSubFrameLength and kBlockSize (if the buffer had 8 samples at some point, it wouldn't work). The mathematical requirement is that (kSubFrameLength - kBlockSize) divides kBlockSize (here 16 divides 64). I think there should be a comment saying 'constants chosen so that the buffer is either empty or has enough samples for InsertBlockAndExtractSubFrame' and that methods have to be called in the right order. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:22: // BlockProcessor interface. I think this comment can be removed or changed to describe what a BlockProcessor is. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:26: virtual ~BlockProcessor() {} = default. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:40: CascadedBiQuadFilter::BiQuadState* biquad_state) { Please check that y is large enough. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:26: BiQuadState() { x[0] = x[1] = y[0] = y[1] = 0.f; } Can be done in member initialization list by '..State() : x(), y() {}' https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:30: rtc::ArrayView<rtc::ArrayView<float>> sub_frame_view) { Can this FillSubFrameView be implemented in terms of the one below with <const float>?. Also, maybe check that sub_frame_index is in range either here or at call site. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:36: for (auto v_k : v) { const auto &. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:64: BlockProcessor* block_processor); I think block_processor should be a unique_ptr<BlockProcessor>, since this class assumes ownership over it by placing it in a unique_ptr. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:24: b.resize(0); I think this should be done in the member initialization list above. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:49: bool FrameBlocker::IsBlockAvailable() { const https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:36: bool IsBlockAvailable(); const
A few more comments... LG! https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:46: std::copy(buffer_[i].begin(), buffer_[i].end(), sub_frame[i].begin()); On 2016/12/20 15:10:35, hlundin-webrtc wrote: > You can do sub_frame[i].insert(...) here, and skip the DCHECK on the size of > sub_frame[i], if you want to. But that is optional. sub_frame is ArrayView, not vector. It can only be copied to. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:68: rtc::ArrayView<float> sub_frame_view_data[block->size()]; On 2016/12/20 15:10:35, hlundin-webrtc wrote: > This is not pretty. Sorry... > Is there any way we can improve this code? Parts of the complexity is here because ArrayViews are created. I think that the main use case of ArrayView is to hide unsafe pointers to blocks of memory. When new memory is created, like the array of array views below, ArrayView need not be the best choice, because we do not need to have any raw pointers to begin with. Instead maybe use std::vector or std::array (since it's fixed size)? https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:75: FillSubFrameView(capture, sub_frame_index, sub_frame_view); Better to move creation and filling of sub_frame_view below to where it is used, I think. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:78: capture_blocker->InsertSubFrameAndExtractBlock(sub_frame_const_view, block); Is there a type error if a non-const view is passed? If not, maybe we could get by with less FillSubFrameViews? https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:153: c = saturated_microphone_signal ? 1 : 0; Nit: 1.f, 0.f for consistency. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:195: c = known_echo_path_change ? 1 : 0; nit: 1.f, 0.f
https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:46: std::copy(buffer_[i].begin(), buffer_[i].end(), sub_frame[i].begin()); On 2016/12/21 10:07:45, aleloi wrote: > On 2016/12/20 15:10:35, hlundin-webrtc wrote: > > You can do sub_frame[i].insert(...) here, and skip the DCHECK on the size of > > sub_frame[i], if you want to. But that is optional. > > sub_frame is ArrayView, not vector. It can only be copied to. Acknowledged. My bad. Ignore those comments when I've slapped them on to ArrayView objects rather than vectors.
I will look at the unittests later. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:37: biquad_state.a[0] = biquad_state.a[1] = 0; On 2016/12/20 10:10:24, peah-webrtc wrote: > On 2016/12/19 11:30:21, ivoc wrote: > > On 2016/12/16 10:04:46, hlundin-webrtc wrote: > > > Probably doesn't matter, but 0.f. > > > > Is it possible to do this initialization in the header? i.e. float a[2] = > {0.f, > > 0.f}; > > Not as far as I can see. Afaics brace initialization does not seem to be allowed > in initializer lists. But I moved this to the constructor of the state which > made better code that what it was before (I think). > > Done. Acknowledged. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:23: struct BiQuadState { On 2016/12/20 10:10:24, peah-webrtc wrote: > On 2016/12/19 11:30:22, ivoc wrote: > > I think you can consider restructuring this a little bit. I think it would be > > nice to have one class called BiQuadFilter that implements a single filter. It > > could have it's own state and coefficients as members and it should have a > > process function. WDYT? > > It would be nicer code-wise but I prefer to have it as one class as it will be > faster (due to the same coefficients being used for all states) and have a lower > memory footprint, as long as not biquads of different coefficients are used (I > think the far most common thing is to have biquads of the same coefficients). > > Even though only one biquad is used in cascade in the current code, another part > of the aec3 code that is yet to be landed is using this functionality with more > than one biquad in cascade. > > WDYT? Ok, that makes sense, let's leave it like this. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:67: }); On 2016/12/20 10:10:25, peah-webrtc wrote: > On 2016/12/19 11:30:22, ivoc wrote: > > Can you explain what this is supposed to do? The way the code looks now > doesn't > > make much sense to me. For example, this lambda is passed to the block > processor > > (why?), and is never executed there. I guess some steps will be added later? > > Yes, the reason for this construct is to provide a cheap way of producing a > render block from a frame and passing it to the BufferRender method. The reason > for why it is not used in the block_processor is that that functionality is to > be added in the next CL. > > However, with the code changes in the new patch, this lambda is no longer used > and I instead use the vector-swap functionality for efficiently passing the > blocks. > > PTAL It looks easier to understand now. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3_constants.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants.h:16: // TODO(peah): Remove when the all code is landed and activated by defaule. Typo: default https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:29: RTC_DCHECK_EQ(0, buffer_[i].size()); I don't really understand why the buffer size should be zero here, will this function only be called in the beginning? https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:40: CascadedBiQuadFilter::BiQuadState* biquad_state) { On 2016/12/20 15:55:35, aleloi wrote: > Please check that y is large enough. x as well, and a dcheck that biquad_state is not a nullptr could be useful. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:260: render_highpass_filter.reset(nullptr); Is this really necessary? Isn't it set to nullptr by default? https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:35: result &= v.size() == num_bands_; This is the bitwise-AND operator (like a = a & b), I don't think that is the right thing to do here. You can just return if the check doesn't hold: if (v.size() != num_bands_) { return false; } https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:37: result &= v_k.size() == frame_length_; I suggest using an early return here: if (v_k.size() != frame_length) { return false; } This way, the result variable can be eliminated.
https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:251: class BlockProcessorTransparentCapture : public BlockProcessor { Is it me or does this class do nothing? Why is it needed? https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:366: enum class EchoPathTestVariant { kNone, kOneSticky, kOneNonSticky }; Please add a comment to explain what each variant is testing. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:442: enum class EchoLeakageTestVariant { Please add a comment here as well to explain the variants. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:539: enum class SaturationTestVariant { Comment about variants, please.
Thanks for the reviews and the awesome comments!!! I have created another patch with changes according to those. PTAL https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:19: BlockFramer::BlockFramer(size_t num_bands, size_t frame_length) On 2016/12/20 15:55:35, aleloi wrote: > Acknowledged. The new version looks very good! Great! https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:38: void BlockFramer::ExtractFrame(size_t sub_frame_index, float* const* frame) { On 2016/12/20 15:55:35, aleloi wrote: > On 2016/12/20 10:10:24, peah-webrtc wrote: > > On 2016/12/16 15:04:31, aleloi wrote: > > > Is there some way to avoid the naked pointers in the output argument? Is it > > > possible to make |frame| be a vector of ArrayViews instead that is filled by > > > this method? > > > > That is a great point! I did that instead by wrapping the frames into > arrayviews > > before the call to BlockFramer and FrameBlocker. > > > > PTAL > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:37: biquad_state.a[0] = biquad_state.a[1] = 0; On 2016/12/21 13:04:04, ivoc wrote: > On 2016/12/20 10:10:24, peah-webrtc wrote: > > On 2016/12/19 11:30:21, ivoc wrote: > > > On 2016/12/16 10:04:46, hlundin-webrtc wrote: > > > > Probably doesn't matter, but 0.f. > > > > > > Is it possible to do this initialization in the header? i.e. float a[2] = > > {0.f, > > > 0.f}; > > > > Not as far as I can see. Afaics brace initialization does not seem to be > allowed > > in initializer lists. But I moved this to the constructor of the state which > > made better code that what it was before (I think). > > > > Done. > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:50: auto memory_b = biquad_state->b; On 2016/12/20 15:10:34, hlundin-webrtc wrote: > On 2016/12/20 10:10:24, peah-webrtc wrote: > > On 2016/12/16 10:04:46, hlundin-webrtc wrote: > > > Following the notation of > https://en.wikipedia.org/wiki/Digital_biquad_filter, > > I > > > would argue that mem_x and mem_y are better. > > > > I'll change to _x and _y. I think that since we anyway abbreviate, I'll use m_ > > as the abbreviation. > > > > WDYT? > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:23: struct BiQuadState { On 2016/12/21 13:04:04, ivoc wrote: > On 2016/12/20 10:10:24, peah-webrtc wrote: > > On 2016/12/19 11:30:22, ivoc wrote: > > > I think you can consider restructuring this a little bit. I think it would > be > > > nice to have one class called BiQuadFilter that implements a single filter. > It > > > could have it's own state and coefficients as members and it should have a > > > process function. WDYT? > > > > It would be nicer code-wise but I prefer to have it as one class as it will be > > faster (due to the same coefficients being used for all states) and have a > lower > > memory footprint, as long as not biquads of different coefficients are used (I > > think the far most common thing is to have biquads of the same coefficients). > > > > Even though only one biquad is used in cascade in the current code, another > part > > of the aec3 code that is yet to be landed is using this functionality with > more > > than one biquad in cascade. > > > > WDYT? > > Ok, that makes sense, let's leave it like this. Acknowledged. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:67: }); On 2016/12/21 13:04:04, ivoc wrote: > On 2016/12/20 10:10:25, peah-webrtc wrote: > > On 2016/12/19 11:30:22, ivoc wrote: > > > Can you explain what this is supposed to do? The way the code looks now > > doesn't > > > make much sense to me. For example, this lambda is passed to the block > > processor > > > (why?), and is never executed there. I guess some steps will be added later? > > > > Yes, the reason for this construct is to provide a cheap way of producing a > > render block from a frame and passing it to the BufferRender method. The > reason > > for why it is not used in the block_processor is that that functionality is to > > be added in the next CL. > > > > However, with the code changes in the new patch, this lambda is no longer used > > and I instead use the vector-swap functionality for efficiently passing the > > blocks. > > > > PTAL > > It looks easier to understand now. Acknowledged. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:34: // The class is supposed to be used in a single-threaded manner On 2016/12/20 15:10:34, hlundin-webrtc wrote: > On 2016/12/20 10:10:25, peah-webrtc wrote: > > On 2016/12/16 10:04:47, hlundin-webrtc wrote: > > > What does single-threaded mean here? Should all methods except AnalyzeRender > > be > > > called from the same thread, or just not at the same time? Is there anything > > > that enforces this design? > > > > Good point. I changed the comment to be more clear (I think). What I mean is > not > > single-threadedness but rather concurrent calling. > > > > There is nothing that enforces this, and I'm not sure that we should do that. > I > > don't want to add a lock just to ensure that the class in not used in the > > correct manner. > > > > The thing is, the echo canceller is not intended for cuncurrent usage, apart > > from the AnalyzeRender call. That call is safe as it does not use any common > > resources with the rest of the code, apart from the SwapQueue which is thread > > safe. > > > > > > WDYT? > > I propose you use a RaceChecker, similarly to how it is used in > audio_mixer_impl.{cc,h}. You will then need two RaceChecker members in the > state; one for capture and one for render. Then you should annotate all state > variables except the SwapQueue with the correct GUARDED_BY. Finally, use > RTC_DCHECK_RUNS_SERIALIZED with the correct RaceChecker member in each method. I agree! Done. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:37: InsertFrameAndExtractBlock(rtc::ArrayView<const rtc::ArrayView<const float>>( On 2016/12/20 15:10:34, hlundin-webrtc wrote: > On 2016/12/20 10:10:26, peah-webrtc wrote: > > On 2016/12/16 10:04:47, hlundin-webrtc wrote: > > > An ArrayView of ArrayViews... I think a vector<ArrayView<const float>> would > > be > > > better. Pass it as a const ref. > > > > What would be the benefit of that be? I changed the array size to a compile > > time-constant and then that should be faster than a vector allocation. > > > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:61: On 2016/12/20 15:55:35, aleloi wrote: > On 2016/12/20 10:10:26, peah-webrtc wrote: > > On 2016/12/16 15:04:31, aleloi wrote: > > > I think this is complex enough to warrant a few unit tests to check that > data > > is > > > written to the right place of |block|, that there are no reads outside of > the > > > buffer, that band_start_index is updated correctly. If there is one buffer > for > > > each band, it will be a little simpler, though. > > > > I fully agree. I have now changed to one band per buffer, and added unit > tests. > > > > PTAL > > I like it now! Great! https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/render_transfer_buffer.h (right): https://codereview.webrtc.org/2584493002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/render_transfer_buffer.h:26: virtual ~RenderTransferBufferWriter() = 0; On 2016/12/20 15:55:35, aleloi wrote: > On 2016/12/16 15:04:32, aleloi wrote: > > For some reason, the style guide says that dtors should not be pure virtual: > > https://google.github.io/styleguide/cppguide.html#Interfaces > > If someone knows why, please enlighten me! Here's my hypothesis: a pure virtual > dtor means that all derived classes must implement it. But that usually means > writing '{}' or '=default' in lots of places where the compiler would implicitly > generate the same thing. > > The style guide has no rule for always writing a dtor, but a pure virtual dtor > implies a mandatory requirement for exactly that. That makes sense, but I don't know if that is the reason. It would be really interesting to know, though. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3_constants.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants.h:16: // TODO(peah): Remove when the all code is landed and activated by defaule. On 2016/12/20 15:10:34, hlundin-webrtc wrote: > defaule -> default Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants.h:16: // TODO(peah): Remove when the all code is landed and activated by defaule. On 2016/12/20 15:55:35, aleloi wrote: > misspell Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants.h:16: // TODO(peah): Remove when the all code is landed and activated by defaule. On 2016/12/21 13:04:05, ivoc wrote: > Typo: default Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:29: RTC_DCHECK_EQ(0, buffer_[i].size()); On 2016/12/20 15:55:35, aleloi wrote: > It seems that the BlockFramer methods can only be called in this sequence, > otherwise the buffer is not empty when InsertBlock is called, or there is not > enough samples for a whole subframe in InsertBlockAndExtractFrame. Did I get it > right? > > InsertBlock # 64 samples after this step > InsertBlockAndExtractSubFrame # 2*64 - 80 = 48 samples left > InsertBlockAndExtractSubFrame # 48+64- 80 = 32 > InsertBlockAndExtractSubFrame # 32+64- 80 = 16 > InsertBlockAndExtractSubFrame # 16+64- 80 = 0 > InsertBlock # 64 samples again > > That this works depends on the exact values of kSubFrameLength and kBlockSize > (if the buffer had 8 samples at some point, it wouldn't work). The mathematical > requirement is that (kSubFrameLength - kBlockSize) divides kBlockSize (here 16 > divides 64). > > I think there should be a comment saying 'constants chosen so that the buffer is > either empty or has enough samples for InsertBlockAndExtractSubFrame' and that > methods have to be called in the right order. That is fully correct! Great analysis! Added the comment. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:29: RTC_DCHECK_EQ(0, buffer_[i].size()); On 2016/12/21 13:04:05, ivoc wrote: > I don't really understand why the buffer size should be zero here, will this > function only be called in the beginning? It needs to be zero because of the cooperation between FrameBlocker and BlockFramer, and in order for EchoCancellation3 to be able to produce one output frame for each input frame. The above analysis by aleloi@ shows this. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:30: buffer_[i].resize(block[i].size()); On 2016/12/20 15:10:34, hlundin-webrtc wrote: > I think you can simplify these two lines to a single > buffer_[i].insert(buffer_[i].begin(), block[i].begin(), block[i].end()); > It will do the resizing for you. Great suggestion! I did not know that! Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:46: std::copy(buffer_[i].begin(), buffer_[i].end(), sub_frame[i].begin()); On 2016/12/20 15:10:35, hlundin-webrtc wrote: > You can do sub_frame[i].insert(...) here, and skip the DCHECK on the size of > sub_frame[i], if you want to. But that is optional. It seems like arrayview does not allow that. Acknowledged. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:46: std::copy(buffer_[i].begin(), buffer_[i].end(), sub_frame[i].begin()); On 2016/12/21 10:07:45, aleloi wrote: > On 2016/12/20 15:10:35, hlundin-webrtc wrote: > > You can do sub_frame[i].insert(...) here, and skip the DCHECK on the size of > > sub_frame[i], if you want to. But that is optional. > > sub_frame is ArrayView, not vector. It can only be copied to. Acknowledged. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:46: std::copy(buffer_[i].begin(), buffer_[i].end(), sub_frame[i].begin()); On 2016/12/21 10:14:19, hlundin-webrtc wrote: > On 2016/12/21 10:07:45, aleloi wrote: > > On 2016/12/20 15:10:35, hlundin-webrtc wrote: > > > You can do sub_frame[i].insert(...) here, and skip the DCHECK on the size of > > > sub_frame[i], if you want to. But that is optional. > > > > sub_frame is ArrayView, not vector. It can only be copied to. > > Acknowledged. My bad. Ignore those comments when I've slapped them on to > ArrayView objects rather than vectors. Acknowledged. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:47: std::copy(block[i].begin(), block[i].begin() + samples_to_frame, On 2016/12/20 15:10:34, hlundin-webrtc wrote: > Same here; again, optional. Not allowed by arrayview. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:49: buffer_[i].resize(kBlockSize - samples_to_frame); On 2016/12/20 15:10:35, hlundin-webrtc wrote: > I don't think you have to resize; simply insert(). A resize to 0 was needed before the insert. But I added the insert. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.h:23: // from from 64 sample blocks. On 2016/12/20 15:10:35, hlundin-webrtc wrote: > from from Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:22: // BlockProcessor interface. On 2016/12/20 15:55:35, aleloi wrote: > I think this comment can be removed or changed to describe what a BlockProcessor > is. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:26: virtual ~BlockProcessor() {} On 2016/12/20 15:55:35, aleloi wrote: > = default. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:44: class BlockProcessorImpl : public BlockProcessor { On 2016/12/20 15:10:35, hlundin-webrtc wrote: > final Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:44: class BlockProcessorImpl : public BlockProcessor { On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Since you have a Create method in the interface class, and the method is > implemented in the .cc file, you can actually move also the declaration of > BlockProcessorImpl to the .cc file. This is how it is done in for > audio_coding_module.cc. Awesome suggestion! Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor.h:49: // Processes a block of capture data. On 2016/12/20 15:10:35, hlundin-webrtc wrote: > You can skip all the duplicated method comments. They will only diverge over > time. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:40: CascadedBiQuadFilter::BiQuadState* biquad_state) { On 2016/12/20 15:55:35, aleloi wrote: > Please check that y is large enough. Good point! Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.cc:40: CascadedBiQuadFilter::BiQuadState* biquad_state) { On 2016/12/21 13:04:05, ivoc wrote: > On 2016/12/20 15:55:35, aleloi wrote: > > Please check that y is large enough. > x as well, and a dcheck that biquad_state is not a nullptr could be useful. Good find! Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h:26: BiQuadState() { x[0] = x[1] = y[0] = y[1] = 0.f; } On 2016/12/20 15:55:35, aleloi wrote: > Can be done in member initialization list by '..State() : x(), y() {}' Supernice! Much more neat! Thanks! Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:30: rtc::ArrayView<rtc::ArrayView<float>> sub_frame_view) { On 2016/12/20 15:55:35, aleloi wrote: > Can this FillSubFrameView be implemented in terms of the one below with <const > float>?. Also, maybe check that sub_frame_index is in range either here or at > call site. I changed this to instead used a vector stored on the state. That became better. Added the checks on sub_frame_index. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:68: rtc::ArrayView<float> sub_frame_view_data[block->size()]; On 2016/12/20 15:10:35, hlundin-webrtc wrote: > This is not pretty. Sorry... > Is there any way we can improve this code? I changed to using a vector that is cached on the state. That looks better, not great but at least better. I think that is as far as we come with the current implementation of AudioBuffer. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:68: rtc::ArrayView<float> sub_frame_view_data[block->size()]; On 2016/12/21 10:07:45, aleloi wrote: > On 2016/12/20 15:10:35, hlundin-webrtc wrote: > > This is not pretty. Sorry... > > Is there any way we can improve this code? > > Parts of the complexity is here because ArrayViews are created. I think that the > main use case of ArrayView is to hide unsafe pointers to blocks of memory. When > new memory is created, like the array of array views below, ArrayView need not > be the best choice, because we do not need to have any raw pointers to begin > with. Instead maybe use std::vector or std::array (since it's fixed size)? Agree. I instead solved this by using a vector stored on the state. I don't want to create the vector for each frame as that includes dynamic allocation, but storing it on the state avoids that. PTAL https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:75: FillSubFrameView(capture, sub_frame_index, sub_frame_view); On 2016/12/21 10:07:45, aleloi wrote: > Better to move creation and filling of sub_frame_view below to where it is used, > I think. Agree. With the new change, it looks better. PTAL https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:78: capture_blocker->InsertSubFrameAndExtractBlock(sub_frame_const_view, block); On 2016/12/21 10:07:45, aleloi wrote: > Is there a type error if a non-const view is passed? If not, maybe we could get > by with less FillSubFrameViews? Yes, there was a type error. I don't think the automatic const type conversion works for rtc::ArrayView<rtc::ArrayView<>> types. I did the above mentioned workaround instead. PTAL https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:260: render_highpass_filter.reset(nullptr); On 2016/12/21 13:04:05, ivoc wrote: > Is this really necessary? Isn't it set to nullptr by default? I'm not sure. The issue was that there was an explicit move of one of these and there was concern whether one could rely on that then would move a nullptr or something else. I added this to make sure. Do you know? I removed one of these, at least, since that one is not needed. WDYT? https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:35: result &= v.size() == num_bands_; On 2016/12/21 13:04:05, ivoc wrote: > This is the bitwise-AND operator (like a = a & b), I don't think that is the > right thing to do here. You can just return if the check doesn't hold: > if (v.size() != num_bands_) { > return false; > } Good one! Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:36: for (auto v_k : v) { On 2016/12/20 15:55:35, aleloi wrote: > const auto &. Good find!!! Thanks! Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:37: result &= v_k.size() == frame_length_; On 2016/12/21 13:04:05, ivoc wrote: > I suggest using an early return here: > if (v_k.size() != frame_length) { > return false; > } > This way, the result variable can be eliminated. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:64: BlockProcessor* block_processor); On 2016/12/20 15:10:35, hlundin-webrtc wrote: > It is not obvious to the caller that the ownership of block_processor is > transferred. Use an std::unique_ptr. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.h:64: BlockProcessor* block_processor); On 2016/12/20 15:55:35, aleloi wrote: > I think block_processor should be a unique_ptr<BlockProcessor>, since this class > assumes ownership over it by placing it in a unique_ptr. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:24: void PopulateInputFrame(size_t frame_length, On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Please, comment on how the frame is filled. What values? Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:40: float* const* frame, On 2016/12/20 15:10:35, hlundin-webrtc wrote: > More const. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:80: // Class for testing that echo leakage is properly reported to the On 2016/12/20 15:10:35, hlundin-webrtc wrote: > This is not used for the next 400 lines. Define this class closer to where it is > used. > > But, as I comment down below, use a mock instead, and define it in a separate > file under aec3/mock/. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:100: BlockProcessorEchoLeakageReport::BlockProcessorEchoLeakageReport( On 2016/12/20 15:10:36, hlundin-webrtc wrote: > Merge definitions into the class declaration above. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:123: // Class for testing that echo leakage is properly reported to the On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Copy-paste failure with the comment. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:153: c = saturated_microphone_signal ? 1 : 0; On 2016/12/21 10:07:45, aleloi wrote: > Nit: 1.f, 0.f for consistency. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:195: c = known_echo_path_change ? 1 : 0; On 2016/12/21 10:07:45, aleloi wrote: > nit: 1.f, 0.f Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:209: class BlockProcessorRenderRedirect : public BlockProcessor { On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Move closer to where it is used. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:227: BlockProcessorRenderRedirect::BlockProcessorRenderRedirect(size_t num_bands) {} On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Merge definitions into class declaration. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:251: class BlockProcessorTransparentCapture : public BlockProcessor { On 2016/12/21 14:52:13, ivoc wrote: > Is it me or does this class do nothing? Why is it needed? It is needed to ensure that the capture signal is not processed and that when that happens the capture signal is kept intact throughout the method call. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:268: BlockProcessorTransparentCapture::BlockProcessorTransparentCapture( On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Merge definitions into class declaration. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:286: void RunCapturePipelineVerificationTest(int sample_rate_hz) { On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Write a comment about what the test actually tests. "Verifies blah bleh delay of > 64 samples because of block-blah..." Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:287: const size_t num_bands = NumBandsForRate(sample_rate_hz); On 2016/12/20 15:10:35, hlundin-webrtc wrote: > You are duplicating these 10-20 lines an awful lot of times throughout the file. > Consolidate somehow. You can for instance put them all into a helper > struct/class that does this initialization in the ctor. I could even consider > accepting a test fixture in this case. I made a tester class that unifies the common parts of these test methods and that contains the test methods. PTAL https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:366: enum class EchoPathTestVariant { kNone, kOneSticky, kOneNonSticky }; On 2016/12/21 14:52:13, ivoc wrote: > Please add a comment to explain what each variant is testing. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:389: new BlockProcessorKnownEchoPathChangeReport(num_bands)); On 2016/12/20 15:10:36, hlundin-webrtc wrote: > Use a mock instead to verify that BlockProcessor::ProcessCapture is called with > the correct parameters. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:404: default: On 2016/12/20 15:10:35, hlundin-webrtc wrote: > You can omit the default case. Since you are switching on an enum (class), the > compiler will tell you if you forgot to handle any of the enum values. Great! Thanks! Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:436: default: On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Remove default. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:442: enum class EchoLeakageTestVariant { On 2016/12/21 14:52:13, ivoc wrote: > Please add a comment here as well to explain the variants. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:470: new BlockProcessorEchoLeakageReport(num_bands)); On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Rewrite this test to use a mock instead of BlockProcessorEchoLeakageReport. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:494: default: On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Remove default. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:533: default: On 2016/12/20 15:10:36, hlundin-webrtc wrote: > Remove default. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:539: enum class SaturationTestVariant { On 2016/12/21 14:52:13, ivoc wrote: > Comment about variants, please. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:567: new BlockProcessorCaptureSaturationReport(num_bands)); On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Use a mock instead to verify that BlockProcessor::ProcessCapture is called with > the correct parameters. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:587: default: On 2016/12/20 15:10:36, hlundin-webrtc wrote: > Remove default. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:624: default: On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Remove default. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:630: void RunRenderPipelineSwapQueueTest(int sample_rate_hz) { On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Comment on what this test does. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:679: void RunRenderPipelineSwapQueueOverrunReturnValueTest(int sample_rate_hz) { On 2016/12/20 15:10:35, hlundin-webrtc wrote: > Comment... Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:718: void RunBlockerAndFramerTest(int sample_rate_hz) { On 2016/12/20 15:10:35, hlundin-webrtc wrote: > This looks like unit tests for FrameBlocker and BlockFramer. Should be in > separate file(s). Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:769: for (auto rate : {8000, 16000, 32000, 48000}) { On 2016/12/20 15:10:36, hlundin-webrtc wrote: > This makes it tricky to know which test case is actually failing. Consider one > of these options: > 1. Create a separate TEST for each sample rate (this explodes a bit with your > two-dimensional test matrices below). > 2. Use a SCOPED_TRACE to get additional printouts. > 3. (Less nice) Use TEST_P. Good point! I went for SCOPED_TRACE. PTAL https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:22: buffer_(num_bands_, std::vector<float>(kBlockSize, 0.f)) { On 2016/12/20 15:10:36, hlundin-webrtc wrote: > Instead of setting up vectors of length kBlockSize and then resizing them to > zero, I propose the following: > > buffer_(num_bands_) { > for (auto& b : buffer_) { > b.reserve(kBlockSize); > RTC_DCHECK(b.empty()); // Not needed, but maybe nice? > } > } Nice one! Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:24: b.resize(0); On 2016/12/20 15:55:35, aleloi wrote: > I think this should be done in the member initialization list above I want to ensure that the reserved space is proper. I think hlundin@s suggestion achieves that in a good way so I added that. PTAL https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:40: std::copy(buffer_[i].begin(), buffer_[i].end(), (*block)[i].begin()); On 2016/12/20 15:10:36, hlundin-webrtc wrote: > You can use vector::insert here too. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:41: std::copy(sub_frame[i].begin(), sub_frame[i].begin() + samples_to_block, On 2016/12/20 15:10:36, hlundin-webrtc wrote: > And here. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:44: std::copy(sub_frame[i].begin() + samples_to_block, sub_frame[i].end(), On 2016/12/20 15:10:36, hlundin-webrtc wrote: > And here, and skip the resize. Nice! There is still a resize needed to zero though. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:49: bool FrameBlocker::IsBlockAvailable() { On 2016/12/20 15:55:35, aleloi wrote: > const Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:59: std::copy(buffer_[i].begin(), buffer_[i].end(), (*block)[i].begin()); On 2016/12/20 15:10:36, hlundin-webrtc wrote: > vector::insert Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:60: buffer_[i].resize(0); On 2016/12/20 15:10:36, hlundin-webrtc wrote: > clear() This sounds a bit dangerous. Are we sure that there is no reallocation. The documentation says there might be. I'll keep resize(0) for now. WDYT? https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.h (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.h:36: bool IsBlockAvailable(); On 2016/12/20 15:55:35, aleloi wrote: > const Done.
I found some further things that merited changes, so I added another patch with those.
Getting close now. Mostly minor comments. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:287: const size_t num_bands = NumBandsForRate(sample_rate_hz); On 2016/12/21 23:13:52, peah-webrtc wrote: > On 2016/12/20 15:10:35, hlundin-webrtc wrote: > > You are duplicating these 10-20 lines an awful lot of times throughout the > file. > > Consolidate somehow. You can for instance put them all into a helper > > struct/class that does this initialization in the ctor. I could even consider > > accepting a test fixture in this case. > > I made a tester class that unifies the common parts of these test methods and > that contains the test methods. > > PTAL OK. That's _almost_ a test fixture then... :) https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:60: buffer_[i].resize(0); On 2016/12/21 23:13:53, peah-webrtc wrote: > On 2016/12/20 15:10:36, hlundin-webrtc wrote: > > clear() > > This sounds a bit dangerous. Are we sure that there is no reallocation. The > documentation says there might be. > > I'll keep resize(0) for now. > > WDYT? http://en.cppreference.com/w/cpp/container/vector/clear: "Leaves the capacity() of the vector unchanged" See also http://stackoverflow.com/questions/18467624/what-does-the-standard-say-about-.... https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3_constants_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants_unittest.cc:13: #include "webrtc/modules/audio_processing/aec3/aec3_constants.h" In unit tests, the .h file associated with the code under test should be included at the very top. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants_unittest.cc:36: { Explicitly write all the test cases, like you do above. That will cost you 4 lines, while the current construct clocks in at the double. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:52: buffer_[i].resize(0); clear() https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer_unittest.cc:15: #include "webrtc/modules/audio_processing/aec3/block_framer.h" This one should go first. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer_unittest.cc:98: } You are testing the "good use case", but you are not testing the bad one. The class under test depends on the right calling sequence of the methods; what if that is violated. I think a death test would be good. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:16: #include "webrtc/modules/audio_processing/aec3/block_processor.h" This goes at the top. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:13: #include "webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h" This goes first. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:19: Can you comment on how you arrived at these coefficients? Matlab command? https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:50: for (float v : values) { You can do EXPECT_EQ(std::vector<float>(1000, 0.f), values); GTest works pretty well with STL containers. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:82: for (size_t k = 0; k < input.size(); ++k) { You can just do EXPECT_EQ(input, output). https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:18: #include "webrtc/modules/audio_processing/aec3/echo_canceller3.h" This goes to the top. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:78: explicit CaptureTransportVerificationProcessor(size_t num_bands) {} = defualt? https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:215: EXPECT_CALL(*block_processor_mock, BufferRender(testing::_)) Add a using testing::_ and simplify all of these to only _ https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:242: block_processor_mock = nullptr; Looks like you want to make block_processor_mock a unique_ptr and move it in the aec3 ctor. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:315: EXPECT_CALL(*block_processor_mock, ReportEchoLeakage(true)).Times(1); I think you will want to enforce the order of the calls. See https://github.com/google/googlemock/blob/master/googlemock/docs/CookBook.md#.... https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:323: block_processor_mock = nullptr; Same here. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:405: EXPECT_CALL(*block_processor_mock, Enforce order here too. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:416: block_processor_mock = nullptr; unique_ptr https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:532: std::ostringstream ss; Won't you have to #include something? https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:40: (*block)[i].resize(0); clear https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:46: buffer_[i].resize(0); clear https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:63: (*block)[i].resize(0); clear https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker_unittest.cc:16: #include "webrtc/modules/audio_processing/aec3/frame_blocker.h" To the top. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker_unittest.cc:85: // Verifies that the FrameBlocker properly forms blocks out ouf the frames. ouf? https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker_unittest.cc:87: const size_t kNumSubFramesToProcess = 20; constexpr https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker_unittest.cc:179: Add some (death) tests to verify that things blow up when the calling sequence to the blocker is wrong.
https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:260: render_highpass_filter.reset(nullptr); On 2016/12/21 23:13:51, peah-webrtc wrote: > On 2016/12/21 13:04:05, ivoc wrote: > > Is this really necessary? Isn't it set to nullptr by default? > > I'm not sure. The issue was that there was an explicit move of one of these and > there was concern whether one could rely on that then would move a nullptr or > something else. I added this to make sure. Do you know? > > I removed one of these, at least, since that one is not needed. > > WDYT? I'm pretty sure that it's not needed, the default value of a unique_ptr is nullptr, so this would have no effect. Calling .reset(nullptr) is only useful to delete an object that was previously owned by the unique_ptr. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:251: class BlockProcessorTransparentCapture : public BlockProcessor { On 2016/12/21 23:13:52, peah-webrtc wrote: > On 2016/12/21 14:52:13, ivoc wrote: > > Is it me or does this class do nothing? Why is it needed? > > It is needed to ensure that the capture signal is not processed and that when > that happens the capture signal is kept intact throughout the method call. > Acknowledged. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3_constants_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants_unittest.cc:29: EXPECT_EQ(1u, NumBandsForRate(8000)); I think this test is fine, but a nice alternative would be a compile-time test, since the code you're testing is constexpr. You could write something like: static_assert(NumBandsForRate(8000) == 1); If it somehow fails, the code will stop compiling, which I think is good, because the earlier you catch errors the less effort they are to fix. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants_unittest.cc:36: { Why the extra braces? https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:50: for (float v : values) { On 2016/12/22 10:23:56, hlundin-webrtc wrote: > You can do > EXPECT_EQ(std::vector<float>(1000, 0.f), values); > > GTest works pretty well with STL containers. Out of curiosity, what does it show when one of the 1000 numbers doesn't match? Just the wrong one? https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:45: RTC_DCHECK_LE(0, sub_frame_index); Doesn't this hold for any size_t? (since values are >= 0 by definition)
https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:50: for (float v : values) { On 2016/12/22 13:38:13, ivoc wrote: > On 2016/12/22 10:23:56, hlundin-webrtc wrote: > > You can do > > EXPECT_EQ(std::vector<float>(1000, 0.f), values); > > > > GTest works pretty well with STL containers. > > Out of curiosity, what does it show when one of the 1000 numbers doesn't match? > Just the wrong one? Turns out it prints out the first few (approx. 25) elements, even if the error is elsewhere. So, not ideal, but the current code isn't either, imo. You'll probably have to dig deeper than what the default printout gives anyway, so then I prefer the shorter code.
Thanks again for a great review! I've addressed all the comments. I think, however, that we should add crash tests as well for the other classes so I'll follow this patch with another patch that does that. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/aec3_constants_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants_unittest.cc:13: #include "webrtc/modules/audio_processing/aec3/aec3_constants.h" On 2016/12/22 10:23:56, hlundin-webrtc wrote: > In unit tests, the .h file associated with the code under test should be > included at the very top. > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Thanks! Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants_unittest.cc:29: EXPECT_EQ(1u, NumBandsForRate(8000)); On 2016/12/22 13:38:13, ivoc wrote: > I think this test is fine, but a nice alternative would be a compile-time test, > since the code you're testing is constexpr. You could write something like: > static_assert(NumBandsForRate(8000) == 1); > If it somehow fails, the code will stop compiling, which I think is good, > because the earlier you catch errors the less effort they are to fix. Great point! Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants_unittest.cc:36: { On 2016/12/22 10:23:56, hlundin-webrtc wrote: > Explicitly write all the test cases, like you do above. That will cost you 4 > lines, while the current construct clocks in at the double. Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/aec3_constants_unittest.cc:36: { On 2016/12/22 13:38:13, ivoc wrote: > Why the extra braces? Afaics those are needed for the scoped trace to ensure the correct trace output. I have, however, removed all of these now. Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:52: buffer_[i].resize(0); On 2016/12/22 10:23:56, hlundin-webrtc wrote: > clear() Afaics, the standards does not prevent clear() from changing the capacity of the vector. Since we don't want that to happen (as that will affect the performance) I think it is better to just to a resize(). WDYT? https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer_unittest.cc:15: #include "webrtc/modules/audio_processing/aec3/block_framer.h" On 2016/12/22 10:23:56, hlundin-webrtc wrote: > This one should go first. > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer_unittest.cc:98: } On 2016/12/22 10:23:56, hlundin-webrtc wrote: > You are testing the "good use case", but you are not testing the bad one. The > class under test depends on the right calling sequence of the methods; what if > that is violated. I think a death test would be good. Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:16: #include "webrtc/modules/audio_processing/aec3/block_processor.h" On 2016/12/22 10:23:56, hlundin-webrtc wrote: > This goes at the top. Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:13: #include "webrtc/modules/audio_processing/aec3/cascaded_biquad_filter.h" On 2016/12/22 10:23:56, hlundin-webrtc wrote: > This goes first. Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:19: On 2016/12/22 10:23:56, hlundin-webrtc wrote: > Can you comment on how you arrived at these coefficients? Matlab command? Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:50: for (float v : values) { On 2016/12/22 10:23:56, hlundin-webrtc wrote: > You can do > EXPECT_EQ(std::vector<float>(1000, 0.f), values); > > GTest works pretty well with STL containers. Wow! That's supernice!!! Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:50: for (float v : values) { On 2016/12/22 13:38:13, ivoc wrote: > On 2016/12/22 10:23:56, hlundin-webrtc wrote: > > You can do > > EXPECT_EQ(std::vector<float>(1000, 0.f), values); > > > > GTest works pretty well with STL containers. > > Out of curiosity, what does it show when one of the 1000 numbers doesn't match? > Just the wrong one? I got the following output: [ RUN ] CascadedBiquadFilter.BlockingConfiguration ../../webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:53: Failure Value of: values Actual: { 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ... } Expected: std::vector<float>(1000, 0.f) Which is: { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ... } [ FAILED ] CascadedBiquadFilter.BlockingConfiguration (0 ms) https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:50: for (float v : values) { On 2016/12/22 13:51:30, hlundin-webrtc wrote: > On 2016/12/22 13:38:13, ivoc wrote: > > On 2016/12/22 10:23:56, hlundin-webrtc wrote: > > > You can do > > > EXPECT_EQ(std::vector<float>(1000, 0.f), values); > > > > > > GTest works pretty well with STL containers. > > > > Out of curiosity, what does it show when one of the 1000 numbers doesn't > match? > > Just the wrong one? > > Turns out it prints out the first few (approx. 25) elements, even if the error > is elsewhere. So, not ideal, but the current code isn't either, imo. You'll > probably have to dig deeper than what the default printout gives anyway, so then > I prefer the shorter code. Acknowledged. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/cascaded_biquad_filter_unittest.cc:82: for (size_t k = 0; k < input.size(); ++k) { On 2016/12/22 10:23:56, hlundin-webrtc wrote: > You can just do EXPECT_EQ(input, output). Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:45: RTC_DCHECK_LE(0, sub_frame_index); On 2016/12/22 13:38:13, ivoc wrote: > Doesn't this hold for any size_t? (since values are >= 0 by definition) Good find! Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:18: #include "webrtc/modules/audio_processing/aec3/echo_canceller3.h" On 2016/12/22 10:23:57, hlundin-webrtc wrote: > This goes to the top. Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:78: explicit CaptureTransportVerificationProcessor(size_t num_bands) {} On 2016/12/22 10:23:57, hlundin-webrtc wrote: > = defualt? That does not seem to work (the compiler says "only special member functions may be defaulted") https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:215: EXPECT_CALL(*block_processor_mock, BufferRender(testing::_)) On 2016/12/22 10:23:57, hlundin-webrtc wrote: > Add a using testing::_ and simplify all of these to only _ Great suggestion! Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:242: block_processor_mock = nullptr; On 2016/12/22 10:23:56, hlundin-webrtc wrote: > Looks like you want to make block_processor_mock a unique_ptr and move it in the > aec3 ctor. Thanks! That's much better! Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:315: EXPECT_CALL(*block_processor_mock, ReportEchoLeakage(true)).Times(1); On 2016/12/22 10:23:56, hlundin-webrtc wrote: > I think you will want to enforce the order of the calls. See > https://github.com/google/googlemock/blob/master/googlemock/docs/CookBook.md#.... Great point! Thanks! Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:323: block_processor_mock = nullptr; On 2016/12/22 10:23:57, hlundin-webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:405: EXPECT_CALL(*block_processor_mock, On 2016/12/22 10:23:56, hlundin-webrtc wrote: > Enforce order here too. Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:416: block_processor_mock = nullptr; On 2016/12/22 10:23:57, hlundin-webrtc wrote: > unique_ptr Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:532: std::ostringstream ss; On 2016/12/22 10:23:57, hlundin-webrtc wrote: > Won't you have to #include something? Good find! It must have been indirectly included. I added the include file in all files where it is used. Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:40: (*block)[i].resize(0); On 2016/12/22 10:23:57, hlundin-webrtc wrote: > clear Afaics, there is no guarantee from the standard that clear() maintains the capacity of the vector, so I keep this as it is. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:46: buffer_[i].resize(0); On 2016/12/22 10:23:57, hlundin-webrtc wrote: > clear Afaics, there is no guarantee from the standard that clear() maintains the capacity of the vector, so I keep this as it is. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:63: (*block)[i].resize(0); On 2016/12/22 10:23:57, hlundin-webrtc wrote: > clear Afaics, there is no guarantee from the standard that clear() maintains the capacity of the vector, so I keep this as it is. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker_unittest.cc:16: #include "webrtc/modules/audio_processing/aec3/frame_blocker.h" On 2016/12/22 10:23:57, hlundin-webrtc wrote: > To the top. Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker_unittest.cc:85: // Verifies that the FrameBlocker properly forms blocks out ouf the frames. On 2016/12/22 10:23:57, hlundin-webrtc wrote: > ouf? Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker_unittest.cc:87: const size_t kNumSubFramesToProcess = 20; On 2016/12/22 10:23:57, hlundin-webrtc wrote: > constexpr Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker_unittest.cc:179: On 2016/12/22 10:23:57, hlundin-webrtc wrote: > Add some (death) tests to verify that things blow up when the calling sequence > to the blocker is wrong. Done.
https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/block_processor.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_processor.cc:44: instance_count_ = rtc::AtomicOps::Increment(&instance_count_); If I understand correctly this is actually wrong. The Increment function will already increment the variable, so there's no need for the assignment. In fact, rtc::AtomicOps::Increment returns the old value (before the increment), so the assignment undoes the increment. I think the best place to put the Increment operation is on the line above this one: data_dumper_(new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_)) Doing it that way means it can never happen that different ApmDataDumpers are constructed with the same instance_count_, which can still happen with the current code depending on the ordering of operation between threads. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:244: render_highpass_filter.reset(nullptr); I think it's safe to remove this one as well. From what I could find the default state of a unique_ptr is exactly the same as initializing it with nullptr (see http://www.cplusplus.com/reference/memory/unique_ptr/unique_ptr/, constructors 1 and 2 are described as being the same). https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:254: instance_count_ = rtc::AtomicOps::Increment(&instance_count_); See other comment about rtc::AtomicOps::Increment. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:18: #include <utility> This should be above vector. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:510: const int aec3_sample_rate_hz = sample_rate_hz_ == 48000 ? 32000 : 48000; Is this correct? I think the other way around would make more sense (48000 ? 48000 : 32000) https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:522: const int aec3_sample_rate_hz = sample_rate_hz_ == 48000 ? 32000 : 48000; Same here https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:533: const int aec3_sample_rate_hz = sample_rate_hz_ == 8000 ? 16000 : 8000; And here https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:547: const int aec3_sample_rate_hz = sample_rate_hz_ == 8000 ? 16000 : 8000; And here
I didn't read the new death tests very carefully, but it seems good. I don't agree with you about vector::clear, as I noted inline. Except that, and a few minor comments, you have my LGTM (because it is soon Christmas (and at Christmas, you give LGTMs)). :) https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:52: buffer_[i].resize(0); On 2016/12/22 16:40:03, peah-webrtc wrote: > On 2016/12/22 10:23:56, hlundin-webrtc wrote: > > clear() > > Afaics, the standards does not prevent clear() from changing the capacity of the > vector. Since we don't want that to happen (as that will affect the performance) > I think it is better to just to a resize(). > > WDYT? I think http://en.cppreference.com/w/cpp/container/vector/clear is pretty clear (pun intended): "Leaves the capacity() of the vector unchanged". Also, in that document, there is also a reference to http://stackoverflow.com/questions/18467624/what-does-the-standard-say-about-... which further elaborates on how clear does not change the capacity. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/block_framer_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_framer_unittest.cc:166: std::ostringstream ss; Break out the generation of the string into a separate function, and save yourself some 20% of TEST code. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/block_processor.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_processor.cc:44: instance_count_ = rtc::AtomicOps::Increment(&instance_count_); On 2016/12/23 10:32:33, ivoc wrote: > If I understand correctly this is actually wrong. The Increment function will > already increment the variable, so there's no need for the assignment. In fact, > rtc::AtomicOps::Increment returns the old value (before the increment), so the > assignment undoes the increment. > I think the best place to put the Increment operation is on the line above this > one: > data_dumper_(new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_)) > Doing it that way means it can never happen that different ApmDataDumpers are > constructed with the same instance_count_, which can still happen with the > current code depending on the ordering of operation between threads. Oh! Good find! I didn't look into the declaration of Increment, but your reasoning makes sense to me. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:95: std::ostringstream ss; Breaks out this too.
lgtm with suggestion. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:52: buffer_[i].resize(0); On 2016/12/22 16:40:03, peah-webrtc wrote: > On 2016/12/22 10:23:56, hlundin-webrtc wrote: > > clear() > > Afaics, the standards does not prevent clear() from changing the capacity of the > vector. Since we don't want that to happen (as that will affect the performance) > I think it is better to just to a resize(). > > WDYT? I didn't know it. Then resize() sounds fine. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_framer.cc:21: buffer_(num_bands_, std::vector<float>(kBlockSize, 0.f)) {} Suggestion: initialize the buffer with 16 zeroes and possibly reserve space for 64. That would introduce less delay, and the calling scheme will still work. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/block_framer_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_framer_unittest.cc:27: rtc::ArrayView<float>(&(*sub_frame)[k][0], (*sub_frame)[k].size()); I suggest to replace address of first element with (*sub_frame)[k].data: IMO it's slightly more readable. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_framer_unittest.cc:82: EXPECT_TRUE(VerifySubFrame(sub_frame_index, -64, output_sub_frame_view)); Perhaps the BlockFramer should start with 16 zeroes in the buffer instead of 64? That would introduce less delay.
Thanks again for the reviews and comments. I have uploaded a new patch with the changes done in response to the comments. I also created another patch that adds checks and corresponding unittests for null inputs whenever pointers are passed to the methods. Finally, I also added a patch with a rebase to latest master. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:260: render_highpass_filter.reset(nullptr); On 2016/12/22 13:38:13, ivoc wrote: > On 2016/12/21 23:13:51, peah-webrtc wrote: > > On 2016/12/21 13:04:05, ivoc wrote: > > > Is this really necessary? Isn't it set to nullptr by default? > > > > I'm not sure. The issue was that there was an explicit move of one of these > and > > there was concern whether one could rely on that then would move a nullptr or > > something else. I added this to make sure. Do you know? > > > > I removed one of these, at least, since that one is not needed. > > > > WDYT? > > I'm pretty sure that it's not needed, the default value of a unique_ptr is > nullptr, so this would have no effect. Calling .reset(nullptr) is only useful to > delete an object that was previously owned by the unique_ptr. Thanks! I agree. Done. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:251: class BlockProcessorTransparentCapture : public BlockProcessor { On 2016/12/22 13:38:13, ivoc wrote: > On 2016/12/21 23:13:52, peah-webrtc wrote: > > On 2016/12/21 14:52:13, ivoc wrote: > > > Is it me or does this class do nothing? Why is it needed? > > > > It is needed to ensure that the capture signal is not processed and that when > > that happens the capture signal is kept intact throughout the method call. > > > > Acknowledged. Acknowledged. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:287: const size_t num_bands = NumBandsForRate(sample_rate_hz); On 2016/12/22 10:23:56, hlundin-OOOtoJan9 wrote: > On 2016/12/21 23:13:52, peah-webrtc wrote: > > On 2016/12/20 15:10:35, hlundin-webrtc wrote: > > > You are duplicating these 10-20 lines an awful lot of times throughout the > > file. > > > Consolidate somehow. You can for instance put them all into a helper > > > struct/class that does this initialization in the ctor. I could even > consider > > > accepting a test fixture in this case. > > > > I made a tester class that unifies the common parts of these test methods and > > that contains the test methods. > > > > PTAL > > OK. That's _almost_ a test fixture then... :) :-) Yes, but more explicit. https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/frame_blocker.cc (right): https://codereview.webrtc.org/2584493002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/frame_blocker.cc:60: buffer_[i].resize(0); On 2016/12/22 10:23:56, hlundin-OOOtoJan9 wrote: > On 2016/12/21 23:13:53, peah-webrtc wrote: > > On 2016/12/20 15:10:36, hlundin-webrtc wrote: > > > clear() > > > > This sounds a bit dangerous. Are we sure that there is no reallocation. The > > documentation says there might be. > > > > I'll keep resize(0) for now. > > > > WDYT? > > http://en.cppreference.com/w/cpp/container/vector/clear: > "Leaves the capacity() of the vector unchanged" > > See also > http://stackoverflow.com/questions/18467624/what-does-the-standard-say-about-.... Thanks! I agree. I'll change to clear. Done. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:52: buffer_[i].resize(0); On 2016/12/23 14:28:38, aleloi wrote: > On 2016/12/22 16:40:03, peah-webrtc wrote: > > On 2016/12/22 10:23:56, hlundin-webrtc wrote: > > > clear() > > > > Afaics, the standards does not prevent clear() from changing the capacity of > the > > vector. Since we don't want that to happen (as that will affect the > performance) > > I think it is better to just to a resize(). > > > > WDYT? > > I didn't know it. Then resize() sounds fine. Acknowledged. https://codereview.webrtc.org/2584493002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec3/block_framer.cc:52: buffer_[i].resize(0); On 2016/12/23 13:38:12, hlundin-OOOtoJan9 wrote: > On 2016/12/22 16:40:03, peah-webrtc wrote: > > On 2016/12/22 10:23:56, hlundin-webrtc wrote: > > > clear() > > > > Afaics, the standards does not prevent clear() from changing the capacity of > the > > vector. Since we don't want that to happen (as that will affect the > performance) > > I think it is better to just to a resize(). > > > > WDYT? > > I think http://en.cppreference.com/w/cpp/container/vector/clear is pretty clear > (pun intended): "Leaves the capacity() of the vector unchanged". > > Also, in that document, there is also a reference to > http://stackoverflow.com/questions/18467624/what-does-the-standard-say-about-... > which further elaborates on how clear does not change the capacity. Thanks for the links! They definitely indicate what you state about clear(). I before found some other references that were more cautious about that but now I cannot find anything else than a stackoverflow comment on that the C++ standard doesn't guarantee a capacity change. Therefore I'll go with your suggestion (which definitely looks much better :-) ). Done. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_framer.cc:21: buffer_(num_bands_, std::vector<float>(kBlockSize, 0.f)) {} On 2016/12/23 14:28:38, aleloi wrote: > Suggestion: initialize the buffer with 16 zeroes and possibly reserve space for > 64. That would introduce less delay, and the calling scheme will still work. That does not seem to work. I think the reason is the way that BlockFramer is used together with FrameBlocker. For each frame that FrameBlocker receives, BlockFramer should output exactly one frame (otherwise there is a gap in the stream). This means for insteance that for a framelength of 80 samples (the same thing happens for 160 sample frame lengths) Frame #1 FrameBlocker input size: 80 FrameBlocker output num blocks: 1 FrameBlocker buffer size (after frame being received): 16 BlockFramer input num blocks: 1 FrameBlocker output size: 80 FrameBlocker buffer size (before frame is produced): x FrameBlocker buffer size (after frame is produced): x-16 Frame #2 FrameBlocker input size: 80 FrameBlocker output num blocks: 1 FrameBlocker buffer size (after frame being received): 32 BlockFramer input num blocks: 1 FrameBlocker output size: 80 FrameBlocker buffer size (before frame is produced): x-16 FrameBlocker buffer size (after frame is produced): x-32 Frame #3 FrameBlocker input size: 80 FrameBlocker output num blocks: 1 FrameBlocker buffer size (after frame being received): 48 BlockFramer input num blocks: 1 FrameBlocker output size: 80 FrameBlocker buffer size (before frame is produced): x-32 FrameBlocker buffer size (after frame is produced): x-48 Frame #4 FrameBlocker input size: 80 FrameBlocker output num blocks: 2 FrameBlocker buffer size (after frame being received): 0 BlockFramer input num blocks: 2 FrameBlocker output size: 80 FrameBlocker buffer size (before frame is produced): x-48 FrameBlocker buffer size (after frame is produced but before block 2 is received): x-64 FrameBlocker buffer size (after frame is produced and after block 2 is received): x In order to ensure that the BlockFramer buffer size does not go below 0, the initial buffer size x must be chosen sufficiently large. The example above I think shows that x at least needs to be 64 for that to be the case. Currently, the case when more 2 blocks are received by BlockFramer when one single frame is produced is handled in such a way that the extra block is received via a separate method call (InsertBlock). If we would bundle that call with the extraction of the frame (in InsertBlockAndExtractSubFrame) that would allow for the buffer size to be reduced to 48. This would, however, have the consequence that an extra block would (for performance reasons) need to be cached on the EchoCanceller3 state so in total that would result in an increase of 48 samples in state memory footprint. Furthermore, that reduction to a buffer size of 48 would not correspond to a lower signal delay. Therefore, I think it is better to use an initial (and maximum) buffer size of 64 inside BlockFramer. Sorry about the long explanation, but I needed to convince myself as well that my reasoning was correct. To summarize, in order to maintain a continuous output stream I think an initial buffer size of 64 is needed. It is possible to jump-start it by filling it with true output samples, but at some time during the first frames, depending on how this is done, there will anyway be a lack of samples when the output frame is to be produced. Those samples will need to be filled by zeros as it is not possible to wait with producing the frame. And that insertion of zeros will again revert the delay reduction achieved by the jump-start. WDYT? Does this make sense? https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/block_framer_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_framer_unittest.cc:27: rtc::ArrayView<float>(&(*sub_frame)[k][0], (*sub_frame)[k].size()); On 2016/12/23 14:28:38, aleloi wrote: > I suggest to replace address of first element with (*sub_frame)[k].data: IMO > it's slightly more readable. I agree! Done. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_framer_unittest.cc:82: EXPECT_TRUE(VerifySubFrame(sub_frame_index, -64, output_sub_frame_view)); On 2016/12/23 14:28:38, aleloi wrote: > Perhaps the BlockFramer should start with 16 zeroes in the buffer instead of 64? > That would introduce less delay. Afaics, that does not work. 16 zeros would produce a full output subframe for the first block, but that then requires that two blocks are inserted for the second subframe that is produced. The only way to achieve that together with FrameBlocker is to insert zeros which in turn would increase the delay. Alternatively, one could wait until sufficient data has been received from FrameBlocker in order to avoid having this initial buffer size, but that would correspond to the same delay as is caused by the buffer. Afaics, there is no way of reducing this delay, but please correct me if I'm wrong. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_framer_unittest.cc:166: std::ostringstream ss; On 2016/12/23 13:38:12, hlundin-OOOtoJan9 wrote: > Break out the generation of the string into a separate function, and save > yourself some 20% of TEST code. Done. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/block_processor.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_processor.cc:44: instance_count_ = rtc::AtomicOps::Increment(&instance_count_); On 2016/12/23 10:32:33, ivoc wrote: > If I understand correctly this is actually wrong. The Increment function will > already increment the variable, so there's no need for the assignment. In fact, > rtc::AtomicOps::Increment returns the old value (before the increment), so the > assignment undoes the increment. > I think the best place to put the Increment operation is on the line above this > one: > data_dumper_(new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_)) > Doing it that way means it can never happen that different ApmDataDumpers are > constructed with the same instance_count_, which can still happen with the > current code depending on the ordering of operation between threads. Great find and suggestion! Thanks!!! Done. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_processor.cc:44: instance_count_ = rtc::AtomicOps::Increment(&instance_count_); On 2016/12/23 13:38:12, hlundin-OOOtoJan9 wrote: > On 2016/12/23 10:32:33, ivoc wrote: > > If I understand correctly this is actually wrong. The Increment function will > > already increment the variable, so there's no need for the assignment. In > fact, > > rtc::AtomicOps::Increment returns the old value (before the increment), so the > > assignment undoes the increment. > > I think the best place to put the Increment operation is on the line above > this > > one: > > data_dumper_(new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_)) > > Doing it that way means it can never happen that different ApmDataDumpers are > > constructed with the same instance_count_, which can still happen with the > > current code depending on the ordering of operation between threads. > > Oh! Good find! I didn't look into the declaration of Increment, but your > reasoning makes sense to me. Acknowledged. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/block_processor_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_processor_unittest.cc:95: std::ostringstream ss; On 2016/12/23 13:38:12, hlundin-OOOtoJan9 wrote: > Breaks out this too. Done. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/echo_canceller3.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:244: render_highpass_filter.reset(nullptr); On 2016/12/23 10:32:33, ivoc wrote: > I think it's safe to remove this one as well. From what I could find the default > state of a unique_ptr is exactly the same as initializing it with nullptr (see > http://www.cplusplus.com/reference/memory/unique_ptr/unique_ptr/, constructors 1 > and 2 are described as being the same). Great! I think you are correct! It really looks like that is the case. Done. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3.cc:254: instance_count_ = rtc::AtomicOps::Increment(&instance_count_); On 2016/12/23 10:32:33, ivoc wrote: > See other comment about rtc::AtomicOps::Increment. Done. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:18: #include <utility> On 2016/12/23 10:32:33, ivoc wrote: > This should be above vector. Done. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:510: const int aec3_sample_rate_hz = sample_rate_hz_ == 48000 ? 32000 : 48000; On 2016/12/23 10:32:33, ivoc wrote: > Is this correct? I think the other way around would make more sense (48000 ? > 48000 : 32000) It is correct, but since this statement is very misleading I now added a comment. The purpose of that assignment is to ensure that aec3_sample_rate_hz is such that the corresponding number of bands is different from what is the case for sample_rate_hz_. That is accomplished by forcing aec3_sample_rate_hz to 48000 in all cases except for when sample_rate_hz_ is 48000, in which case aec3_sample_rate_hz is forced to 32000. Done. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:522: const int aec3_sample_rate_hz = sample_rate_hz_ == 48000 ? 32000 : 48000; On 2016/12/23 10:32:33, ivoc wrote: > Same here Added comment, see above for details. Done. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:533: const int aec3_sample_rate_hz = sample_rate_hz_ == 8000 ? 16000 : 8000; On 2016/12/23 10:32:33, ivoc wrote: > And here The idea here is to choose aec3_sample_rate_hz such that the frame length (in each band) becomes different than what it is for sample_rate_hz_. As the frame length is 80 samples for 8 kHz, and 160 samples otherwise, this can only be accomplished by forcing aec3_sample_rate_hz to be 8 kHz in all cases apart from when sample_rate_hz_ is 8 kHz. I added a comment since this is not at all obvious. Done. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:547: const int aec3_sample_rate_hz = sample_rate_hz_ == 8000 ? 16000 : 8000; On 2016/12/23 10:32:33, ivoc wrote: > And here Added comment, see above for details. Done.
Hi again, I added two new patch-sets to fix build issues on Windows: a) Change the specfied biquad filter coefficients to be float. b) Change a 2D-array to be of a fixed size in the EchoCanceller3 unittest code.
Nice work! LGTM, see one small nit below. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:510: const int aec3_sample_rate_hz = sample_rate_hz_ == 48000 ? 32000 : 48000; On 2017/01/02 08:45:10, peah-webrtc wrote: > On 2016/12/23 10:32:33, ivoc wrote: > > Is this correct? I think the other way around would make more sense (48000 ? > > 48000 : 32000) > > It is correct, but since this statement is very misleading I now added a > comment. > The purpose of that assignment is to ensure that aec3_sample_rate_hz is such > that the corresponding number of bands is different from what is the case for > sample_rate_hz_. That is accomplished by forcing aec3_sample_rate_hz to 48000 in > all cases except for when sample_rate_hz_ is 48000, in which case > aec3_sample_rate_hz is forced to 32000. > > Done. Great, it's much easier to understand what's going on now. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:522: const int aec3_sample_rate_hz = sample_rate_hz_ == 48000 ? 32000 : 48000; On 2017/01/02 08:45:10, peah-webrtc wrote: > On 2016/12/23 10:32:33, ivoc wrote: > > Same here > > Added comment, see above for details. > > Done. Acknowledged. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:533: const int aec3_sample_rate_hz = sample_rate_hz_ == 8000 ? 16000 : 8000; On 2017/01/02 08:45:10, peah-webrtc wrote: > On 2016/12/23 10:32:33, ivoc wrote: > > And here > The idea here is to choose aec3_sample_rate_hz such that the frame length (in > each band) becomes different than what it is for sample_rate_hz_. As the frame > length is 80 samples for 8 kHz, and 160 samples otherwise, this can only be > accomplished by forcing aec3_sample_rate_hz to be 8 kHz in all cases apart from > when sample_rate_hz_ is 8 kHz. > > I added a comment since this is not at all obvious. > > Done. Acknowledged. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:547: const int aec3_sample_rate_hz = sample_rate_hz_ == 8000 ? 16000 : 8000; On 2017/01/02 08:45:10, peah-webrtc wrote: > On 2016/12/23 10:32:33, ivoc wrote: > > And here > > Added comment, see above for details. > > Done. I don't see a comment here, it would be nice to have one as well.
Thanks for a great review effort (and for the Christmas LGTMs) ! The codes is now in much better shape thanks to your feedback. https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/echo_canceller3_unittest.cc:547: const int aec3_sample_rate_hz = sample_rate_hz_ == 8000 ? 16000 : 8000; On 2017/01/02 10:17:11, ivoc wrote: > On 2017/01/02 08:45:10, peah-webrtc wrote: > > On 2016/12/23 10:32:33, ivoc wrote: > > > And here > > > > Added comment, see above for details. > > > > Done. > > I don't see a comment here, it would be nice to have one as well. Ah! Sorry, missed to add that one. Thanks! Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org, henrik.lundin@webrtc.org, ivoc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2584493002/#ps240001 (title: "Added missing comment")
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": 240001, "attempt_start_ts": 1483352766050840,
"parent_rev": "f709e56b4adc2931b656b30f2b42bac7e1d3d347", "commit_rev":
"38fd1758e90bcdc7690a552e7ef0ec0d143d2f30"}
Message was sent while issue was closed.
Description was changed from ========== Added first layer of the echo canceller 3 functionality. This CL adds the first layer of the echo canceller 3. All of the code is as it should, apart from block_processor.* which only contains placeholder functionality. (Upcoming CLs will add proper functionality into those files.) BUG=webrtc:6018 ========== to ========== Added first layer of the echo canceller 3 functionality. This CL adds the first layer of the echo canceller 3. All of the code is as it should, apart from block_processor.* which only contains placeholder functionality. (Upcoming CLs will add proper functionality into those files.) BUG=webrtc:6018 Review-Url: https://codereview.webrtc.org/2584493002 Cr-Commit-Position: refs/heads/master@{#15861} Committed: https://chromium.googlesource.com/external/webrtc/+/38fd1758e90bcdc7690a552e7... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/external/webrtc/+/38fd1758e90bcdc7690a552e7...
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.webrtc.org/2603293002/ by terelius@webrtc.org. The reason for reverting is: Memcheck buildbot detected memory leaks in the death tests. The code looks fine to me, but please investigate what causes the error and reland..
Message was sent while issue was closed.
https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec3/block_framer.cc (right): https://codereview.webrtc.org/2584493002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec3/block_framer.cc:21: buffer_(num_bands_, std::vector<float>(kBlockSize, 0.f)) {} On 2017/01/02 08:45:10, peah-webrtc wrote: > On 2016/12/23 14:28:38, aleloi wrote: > > Suggestion: initialize the buffer with 16 zeroes and possibly reserve space > for > > 64. That would introduce less delay, and the calling scheme will still work. > That does not seem to work. > I think the reason is the way that BlockFramer is used together with > FrameBlocker. > > For each frame that FrameBlocker receives, BlockFramer should output exactly one > frame (otherwise there is a gap in the stream). This means for insteance that > for a framelength of 80 samples (the same thing happens for 160 sample frame > lengths) > Frame #1 > FrameBlocker input size: 80 > FrameBlocker output num blocks: 1 > FrameBlocker buffer size (after frame being received): 16 > BlockFramer input num blocks: 1 > FrameBlocker output size: 80 > FrameBlocker buffer size (before frame is produced): x > FrameBlocker buffer size (after frame is produced): x-16 > > Frame #2 > FrameBlocker input size: 80 > FrameBlocker output num blocks: 1 > FrameBlocker buffer size (after frame being received): 32 > BlockFramer input num blocks: 1 > FrameBlocker output size: 80 > FrameBlocker buffer size (before frame is produced): x-16 > FrameBlocker buffer size (after frame is produced): x-32 > > Frame #3 > FrameBlocker input size: 80 > FrameBlocker output num blocks: 1 > FrameBlocker buffer size (after frame being received): 48 > BlockFramer input num blocks: 1 > FrameBlocker output size: 80 > FrameBlocker buffer size (before frame is produced): x-32 > FrameBlocker buffer size (after frame is produced): x-48 > > Frame #4 > FrameBlocker input size: 80 > FrameBlocker output num blocks: 2 > FrameBlocker buffer size (after frame being received): 0 > BlockFramer input num blocks: 2 > FrameBlocker output size: 80 > FrameBlocker buffer size (before frame is produced): x-48 > FrameBlocker buffer size (after frame is produced but before block 2 is > received): x-64 > FrameBlocker buffer size (after frame is produced and after block 2 is > received): x > > > In order to ensure that the BlockFramer buffer size does not go below 0, the > initial buffer size x must be chosen sufficiently large. The example above I > think shows that x at least needs to be 64 for that to be the case. > > Currently, the case when more 2 blocks are received by BlockFramer when one > single frame is produced is handled in such a way that the extra block is > received via a separate method call (InsertBlock). If we would bundle that call > with the extraction of the frame (in InsertBlockAndExtractSubFrame) that would > allow for the buffer size to be reduced to 48. This would, however, have the > consequence that an extra block would (for performance reasons) need to be > cached on the EchoCanceller3 state so in total that would result in an increase > of 48 samples in state memory footprint. Furthermore, that reduction to a buffer > size of 48 would not correspond to a lower signal delay. Therefore, I think it > is better to use an initial (and maximum) buffer size of 64 inside BlockFramer. > > Sorry about the long explanation, but I needed to convince myself as well that > my reasoning was correct. > > To summarize, in order to maintain a continuous output stream I think an initial > buffer size of 64 is needed. It is possible to jump-start it by filling it with > true output samples, but at some time during the first frames, depending on how > this is done, there will anyway be a lack of samples when the output frame is to > be produced. Those samples will need to be filled by zeros as it is not possible > to wait with producing the frame. And that insertion of zeros will again revert > the delay reduction achieved by the jump-start. > > WDYT? Does this make sense? > > > > Ok, I see. Sorry for the delay. |
