Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1739993003:
Adding fraction of filter divergence in AEC metrics. (Closed)
|
Created:
4 years, 10 months ago by minyue-webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding fraction of filter divergence in AEC metrics.
With the current AEC algorithm, the divergence of the echo cancelling linear filter is a strong signal of non-transparency. During double talk, it can result in a ducking artifacts.
In this CL, a metric that tells the fraction of filter divergence is added. This can measure the severity of non-transparency.
BUG=
Committed: https://crrev.com/e10fc3fb2db623c65bb047000d6e6346f728e7dd
Cr-Commit-Position: refs/heads/master@{#12276}
Patch Set 1 #
Total comments: 5
Patch Set 2 : rebase #Patch Set 3 : adding MeanCalculator #Patch Set 4 : renaming #
Total comments: 59
Patch Set 5 : addressing Per's comments #
Total comments: 9
Patch Set 6 : rebasing #
Total comments: 1
Patch Set 7 : adding audibility and more #
Total comments: 17
Patch Set 8 : adding block_fraction_calculator and more #
Total comments: 7
Patch Set 9 : moving DivergentFilterFraction to aec_core #Patch Set 10 : adding a header #
Messages
Total messages: 46 (20 generated)
Description was changed from ========== removing probe adding diverge metric Merge branch 'master' of https://chromium.googlesource.com/external/webrtc into ducking_analysis probe + linoutlevel BUG= ========== to ========== Adding fraction of filter divergence in AEC metrics. With the current AEC algorithm, the divergence of the echo cancelling linear filter is a strong signal of non-transparency. During double talk, it can result in a ducking artifacts. In this CL, a metric that tells the fraction of filter divergence is added. This can measure the severity of non-transparency. BUG= ==========
minyue@webrtc.org changed reviewers: + peah@webrtc.org, tina.legrand@webrtc.org
Hi Tina and Per, I'd like you to review this CL. Thanks!
https://codereview.webrtc.org/1739993003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:625: // is larger than that of near-end signal by -30dB or more. I don't think the usage of "-30dB" is correct. I think that it should be 0.0043 dB. I.e. (including some rephrasing): // We count it as a filter divergence when the audio level of the linear filter output // is larger than that of the near-end signal by 0.0043 dB or more. https://codereview.webrtc.org/1739993003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core_internal.h (right): https://codereview.webrtc.org/1739993003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core_internal.h:122: int num_filter_diverge; I think it would make sense to bundle these into a struct instead as they belong, and are only usable, together. That would simplify code readability and refactorability. You could then also more easily name the variables. Currently I think it could be more clear from the variable names what they store.
Great that you are adding this metric! I have to leave it to Per to assess whether the metric is good or not (but I think it is). So, you only get nit-picky comments from me. :) https://codereview.webrtc.org/1739993003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:50: I'm wondering if "Diverge" is the right term, or if we should call it "Divergence". What do you guys think makes most sense? https://codereview.webrtc.org/1739993003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:51: // Diverge metric is based on audio level, which gets updated which every remove "which" in "which every"
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/1739993003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.cc:50: On 2016/03/02 15:30:29, tlegrand-webrtc wrote: > I'm wondering if "Diverge" is the right term, or if we should call it > "Divergence". What do you guys think makes most sense? I believe "Divergent" is the trendy form.
kwiberg@webrtc.org changed reviewers: - kwiberg@webrtc.org
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #3 (id:140001) has been deleted
Hi, Thanks for the review! I did more study on the metric. My findings are 1. It tells the degree of divergence of AEC linear filter, thus can indicates when ducking happens. 2. It also makes an alarm when the near-end microphone pick is small, and far-end signal is loud. This is the case when near-end wears a headphone. This is because the linear filter diverges in such case. Linear filter should be improved in such situation. So generally, the metric can be used as an evaluation of AEC (although in 2nd case, it may not be associated with ducking) ~~~~ New patch sets: Patch set 2, rebase Patch set 3, to bound the quantities for calculation the filter divergent fraction, a mean calculator is introduced. It can be used for other mean related calculation. Patch set 4, renaming. I am never good at naming, I just took Karl's suggestion and made it a separate patch set. Please help review the new changes. Thanks!
https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:625: const float level_increase = What about renaming level_increase to level_difference? https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:630: aec->fraction_filter_divergent->AddSample(level_increase > Is a sliding window average measure really needed for this? It is quite expensive compared to the alternatives, both in terms of computations, amount of code and amount of memory. Have you considered using a counter that counts occurrences when this happens? Since you are logging binary 1 or 0 that should be an option. What about a sliding average? Have you looked into that? https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1451: aec->fraction_filter_divergent.reset( Does this need to be dynamically allocated? Since it is initialized using a constant, it should be possible to not have it dynamically allocated. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/mean_calculator.cc (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:2: * Copyright 2015 The WebRTC Project Authors. All rights reserved. The year should be 2016 https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:23: MeanCalculator::~MeanCalculator() = default; Is the destructor needed? https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:29: sum_ -= buffer_[head_]; This is not guaranteed to give the correct sum. Actually, a sum of only positive values may yield a negative value when updated in this way. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:42: if (IsWindowFull()) { I think it is better to use full_ directly instead of call the IsWindowFull function as the function call requires acquiring the lock twice. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/mean_calculator.h (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:2: * Copyright 2015 The WebRTC Project Authors. All rights reserved. The year should be 2016. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:13: Until there are more uses of this class, I think it should not be placed in utilities and instead be put into the aec folder and be tailormade to exactly what it is needed there. As it is now, the only usage is in aec_core.cc where it is used to store float values of 1.0 or 0.0 which I think makes it motivated to write the code to exactly look at that. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:24: class MeanCalculator { In the code, this class is currently only used to store the values 1.0f and 0.0f. I think the class then should be replaced by an int/bool array wrapper. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:26: explicit MeanCalculator(size_t window_length); The name of this class is a bit too vague I think. It provides a sliding window mean computation over an array of values and it would be nice if it reflects that. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:38: void Flush(); What about Clear() ? https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:42: bool IsWindowFull(); The return of an optional and this function provides redundant functionality. I think one of them should be removed. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:42: bool IsWindowFull(); Should be declared const https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:45: rtc::CriticalSection crit_; Why is a lock required in this class? It is not currently accessed in a concurrent manner, right? https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:46: size_t window_length_ GUARDED_BY(crit_); This variable can be removed if you use a vector (be replaced by .size()). https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:47: std::unique_ptr<float[]> buffer_ GUARDED_BY(crit_); Please use a vector instead of a dynamically allocated float array. Then you get the niceness of the vector class while still having the ability to dynamically allocate the data. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/mean_calculator_unittest.cc (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator_unittest.cc:17: TEST(MeanCalculatorTest, Fullness) { There should also be a test that checks that the correct values of the mean are computed.
Hi Per, Thanks for the comments! I tried to answer your concerns + uploaded a new patch. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:625: const float level_increase = On 2016/03/14 09:12:53, peah-webrtc wrote: > What about renaming level_increase to level_difference? I thought about it, "increase" gives a direction: we don't want level to increase, and therefore there should be a "<" logic on it later. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:630: aec->fraction_filter_divergent->AddSample(level_increase > On 2016/03/14 09:12:53, peah-webrtc wrote: > Is a sliding window average measure really needed for this? It is quite > expensive compared to the alternatives, both in terms of computations, amount of > code and amount of memory. > > Have you considered using a counter that counts occurrences when this happens? > Since you are logging binary 1 or 0 that should be an option. > > What about a sliding average? Have you looked into that? I wrote a new MeanCalculator, with the hope that it can be used for other mean calculations, e.g., the near/far/linout/nlpout audio levels. I think, yes, it increases memory usage, but benefits are that 1) the update frequency (time when we want to obtain the mean) is determined by the averaging window length, 2) the mean can become smoothed. Let me know if these benefits are worthy for introducing MeanCalculator. Computationally, it is really cheap: sum = sum - sample[t - window] + sample[t] https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1451: aec->fraction_filter_divergent.reset( On 2016/03/14 09:12:53, peah-webrtc wrote: > Does this need to be dynamically allocated? Since it is initialized using a > constant, it should be possible to not have it dynamically allocated. Problem is |aec| does not have a constructor (unless we want to add one in this CL) https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/mean_calculator.cc (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:2: * Copyright 2015 The WebRTC Project Authors. All rights reserved. On 2016/03/14 09:12:53, peah-webrtc wrote: > The year should be 2016 Yes https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:23: MeanCalculator::~MeanCalculator() = default; On 2016/03/14 09:12:53, peah-webrtc wrote: > Is the destructor needed? deleted https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:29: sum_ -= buffer_[head_]; On 2016/03/14 09:12:53, peah-webrtc wrote: > This is not guaranteed to give the correct sum. Actually, a sum of only positive > values may yield a negative value when updated in this way. I was also worried about the numerical accuracy. I think, as a mean calculator, some numerical error should be tolerable. But I think we can use Kahan Algorithm: https://en.wikipedia.org/wiki/Kahan_summation_algorithm https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:42: if (IsWindowFull()) { On 2016/03/14 09:12:53, peah-webrtc wrote: > I think it is better to use full_ directly instead of call the IsWindowFull > function as the function call requires acquiring the lock twice. I agree https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/mean_calculator.h (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:2: * Copyright 2015 The WebRTC Project Authors. All rights reserved. On 2016/03/14 09:12:53, peah-webrtc wrote: > The year should be 2016. yes, thanks https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:13: On 2016/03/14 09:12:53, peah-webrtc wrote: > Until there are more uses of this class, I think it should not be placed in > utilities and instead be put into the aec folder and be tailormade to exactly > what it is needed there. As it is now, the only usage is in aec_core.cc where it > is used to store float values of 1.0 or 0.0 which I think makes it motivated to > write the code to exactly look at that. As mentioned in an earlier reply, the reason of writing it this way is indeed to make more use of it. not in this CL though WDYT? https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:24: class MeanCalculator { On 2016/03/14 09:12:53, peah-webrtc wrote: > In the code, this class is currently only used to store the values 1.0f and > 0.0f. I think the class then should be replaced by an int/bool array wrapper. replied in earlier comments https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:26: explicit MeanCalculator(size_t window_length); On 2016/03/14 09:12:53, peah-webrtc wrote: > The name of this class is a bit too vague I think. It provides a sliding window > mean computation over an array of values and it would be nice if it reflects > that. I am very bad at naming, do you have any suggestion? https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:38: void Flush(); On 2016/03/14 09:12:53, peah-webrtc wrote: > What about Clear() ? I'd buy it. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:42: bool IsWindowFull(); On 2016/03/14 09:12:53, peah-webrtc wrote: > Should be declared const Yes, thanks. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:42: bool IsWindowFull(); On 2016/03/14 09:12:53, peah-webrtc wrote: > The return of an optional and this function provides redundant functionality. I > think one of them should be removed. I think that using GetMean() to determine IsWindowFull() is a bit too much. Not only because it creates an Option, but also because the name does not reflect this functionality. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:45: rtc::CriticalSection crit_; On 2016/03/14 09:12:53, peah-webrtc wrote: > Why is a lock required in this class? It is not currently accessed in a > concurrent manner, right? There is no trouble on the current use case of it. But it needs to protect itself on other future use cases. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:46: size_t window_length_ GUARDED_BY(crit_); On 2016/03/14 09:12:53, peah-webrtc wrote: > This variable can be removed if you use a vector (be replaced by .size()). Doesn't size() give the actual number of values added, rather than capacity. And there seems no way to define and return capacity for vector<>, right? https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:47: std::unique_ptr<float[]> buffer_ GUARDED_BY(crit_); On 2016/03/14 09:12:53, peah-webrtc wrote: > Please use a vector instead of a dynamically allocated float array. Then you get > the niceness of the vector class while still having the ability to dynamically > allocate the data. ok. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/mean_calculator_unittest.cc (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator_unittest.cc:17: TEST(MeanCalculatorTest, Fullness) { On 2016/03/14 09:12:53, peah-webrtc wrote: > There should also be a test that checks that the correct values of the mean are > computed. renamed the test
https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:625: const float level_increase = On 2016/03/14 14:51:41, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > What about renaming level_increase to level_difference? > > I thought about it, "increase" gives a direction: we don't want level to > increase, and therefore there should be a "<" logic on it later. Ok. (I think it sounds weird with a negative increase, but I think your point is definitely valid). https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:630: aec->fraction_filter_divergent->AddSample(level_increase > On 2016/03/14 14:51:41, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > Is a sliding window average measure really needed for this? It is quite > > expensive compared to the alternatives, both in terms of computations, amount > of > > code and amount of memory. > > > > Have you considered using a counter that counts occurrences when this happens? > > Since you are logging binary 1 or 0 that should be an option. > > > > What about a sliding average? Have you looked into that? > > I wrote a new MeanCalculator, with the hope that it can be used for other mean > calculations, e.g., the near/far/linout/nlpout audio levels. I think, yes, it > increases memory usage, but benefits are that 1) the update frequency (time when > we want to obtain the mean) is determined by the averaging window length, 2) the > mean can become smoothed. > > Let me know if these benefits are worthy for introducing MeanCalculator. > > Computationally, it is really cheap: sum = sum - sample[t - window] + sample[t] I think that one thing that is missing is that the metric is currently not used anywhere, so it is really hard to see that this kind of update frequency/order of accuracy is needed. Could you please add the reporting of the metric as well in this CL? https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:630: aec->fraction_filter_divergent->AddSample(level_increase > On 2016/03/14 14:51:41, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > Is a sliding window average measure really needed for this? It is quite > > expensive compared to the alternatives, both in terms of computations, amount > of > > code and amount of memory. > > > > Have you considered using a counter that counts occurrences when this happens? > > Since you are logging binary 1 or 0 that should be an option. > > > > What about a sliding average? Have you looked into that? > > I wrote a new MeanCalculator, with the hope that it can be used for other mean > calculations, e.g., the near/far/linout/nlpout audio levels. I think, yes, it > increases memory usage, but benefits are that 1) the update frequency (time when > we want to obtain the mean) is determined by the averaging window length, 2) the > mean can become smoothed. > > Let me know if these benefits are worthy for introducing MeanCalculator. > > Computationally, it is really cheap: sum = sum - sample[t - window] + sample[t] If you foresee future uses of the class I think you should add the class only when you introduce that use of it. As it is now, I think the class is not really fitting what is done as it is much too general. Regardless of the class being already available, I think the way it is used in this place motivates a different implementation of it (one option could be to use a template class that allow computing the means for a certain type but at this stage, with only one place using it, I think adding such a class is too early). https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:630: aec->fraction_filter_divergent->AddSample(level_increase > On 2016/03/14 14:51:41, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > Is a sliding window average measure really needed for this? It is quite > > expensive compared to the alternatives, both in terms of computations, amount > of > > code and amount of memory. > > > > Have you considered using a counter that counts occurrences when this happens? > > Since you are logging binary 1 or 0 that should be an option. > > > > What about a sliding average? Have you looked into that? > > I wrote a new MeanCalculator, with the hope that it can be used for other mean > calculations, e.g., the near/far/linout/nlpout audio levels. I think, yes, it > increases memory usage, but benefits are that 1) the update frequency (time when > we want to obtain the mean) is determined by the averaging window length, 2) the > mean can become smoothed. > > Let me know if these benefits are worthy for introducing MeanCalculator. > > Computationally, it is really cheap: sum = sum - sample[t - window] + sample[t] What do you mean by that the update frequency is determined by the window length? I don't really follow that. What do you mean by the smoothing of the mean? If smoothing is needed for the mean is needed, I think it makes sense to add that in the scope of this CL as such a smoothing would alter the MeanCalculator class. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1451: aec->fraction_filter_divergent.reset( On 2016/03/14 14:51:41, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > Does this need to be dynamically allocated? Since it is initialized using a > > constant, it should be possible to not have it dynamically allocated. > > Problem is |aec| does not have a constructor (unless we want to add one in this > CL) Acknowledged. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/mean_calculator.cc (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:23: MeanCalculator::~MeanCalculator() = default; On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > Is the destructor needed? > > deleted No, it is just commented out. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:29: sum_ -= buffer_[head_]; On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > This is not guaranteed to give the correct sum. Actually, a sum of only > positive > > values may yield a negative value when updated in this way. > > I was also worried about the numerical accuracy. I think, as a mean calculator, > some numerical error should be tolerable. But I think we can use Kahan > Algorithm: https://en.wikipedia.org/wiki/Kahan_summation_algorithm I don't agree that as a mean calculator an error is tolerable. It depends on the intended usage of the mean. For instance, for the AEC that numerical error would be fatal. Since this is a general class, I think the numerical error must be clearly specified. As it is now, it is fully possible to have a mean that is negative although all the values used when computing the mean are positive. And it is not at all clear from the class that that might happen. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:29: sum_ -= buffer_[head_]; On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > This is not guaranteed to give the correct sum. Actually, a sum of only > positive > > values may yield a negative value when updated in this way. > > I was also worried about the numerical accuracy. I think, as a mean calculator, > some numerical error should be tolerable. But I think we can use Kahan > Algorithm: https://en.wikipedia.org/wiki/Kahan_summation_algorithm The computation of a sliding window arithmetic mean for floating point values is not straightforward, and since the current usage of this class actually does not require a mean of floating point values (as it currently uses integers), I think you should postpone the implementation of this class. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.cc:42: if (IsWindowFull()) { On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > I think it is better to use full_ directly instead of call the IsWindowFull > > function as the function call requires acquiring the lock twice. > > I agree Acknowledged. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/mean_calculator.h (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:2: * Copyright 2015 The WebRTC Project Authors. All rights reserved. On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > The year should be 2016. > > yes, thanks Acknowledged. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:13: On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > Until there are more uses of this class, I think it should not be placed in > > utilities and instead be put into the aec folder and be tailormade to exactly > > what it is needed there. As it is now, the only usage is in aec_core.cc where > it > > is used to store float values of 1.0 or 0.0 which I think makes it motivated > to > > write the code to exactly look at that. > > As mentioned in an earlier reply, the reason of writing it this way is indeed to > make more use of it. not in this CL though WDYT? I think that it then is better to delay the introduction of the class until next time it is needed , as the functionality in the class is not that well suited for how it is used (to compute the mean of integers). https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:24: class MeanCalculator { On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > In the code, this class is currently only used to store the values 1.0f and > > 0.0f. I think the class then should be replaced by an int/bool array wrapper. > > replied in earlier comments Acknowledged. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:26: explicit MeanCalculator(size_t window_length); On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > The name of this class is a bit too vague I think. It provides a sliding > window > > mean computation over an array of values and it would be nice if it reflects > > that. > > I am very bad at naming, do you have any suggestion? What about SlidingWindowAverage? https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:38: void Flush(); On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > What about Clear() ? > > I'd buy it. Acknowledged. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:42: bool IsWindowFull(); On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > The return of an optional and this function provides redundant functionality. > I > > think one of them should be removed. > > I think that using GetMean() to determine IsWindowFull() is a bit too much. Not > only because it creates an Option, but also because the name does not reflect > this functionality. It totally depends on how it is to be used. Since neither of the methods is actually yet used in the code (apart from in the unit test) I can only guess on the intended usage so probably it would be more clear if you add usage of these in the code. In my mind, what I think you intend to do is something like: A) rtc::Optional<float> mean = calc.GetMean(); if (mean) { reportNewMean(*mean); } As I see it, with IsWindowFull() that could instead be used as B) if (calc.IsWindowFull()) { rtc::Optional<float> mean = calc.GetMean(); RTC_DCHECK(mean); reportNewMean(*mean); } That is the usage that I foresee and then I think both A) and B) makes sense, but in either of these only one of the functionalities are used. Therefore, as I don't think it makes sense to add functionality to the class before it is really needed, I think the IsWindowFull() method should be removed. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:45: rtc::CriticalSection crit_; On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > Why is a lock required in this class? It is not currently accessed in a > > concurrent manner, right? > > There is no trouble on the current use case of it. But it needs to protect > itself on other future use cases. The problem with a lock is that it adds a cost and I don't think that we should add locks if there is no strong need for that. Furthermore, even though I agree with you that there is no risk with the current code, locks are always risky in terms of deadlocks. Another argument against a lock is that it encourages using this in a multi-threaded manner and that is something that we definitely should avoid. Until there is a clear need for a lock I see no reason to add that. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:46: size_t window_length_ GUARDED_BY(crit_); On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > This variable can be removed if you use a vector (be replaced by .size()). > > Doesn't size() give the actual number of values added, rather than capacity. And > there seems no way to define and return capacity for vector<>, right? The vector should be preallocated to the size of the vector, i.e., it should not grow for each of the values that are added. Then the size reflects exactly what window_length_ does. (The capacity of the vector can be assessed by the capacity() method) https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator.h:47: std::unique_ptr<float[]> buffer_ GUARDED_BY(crit_); On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > Please use a vector instead of a dynamically allocated float array. Then you > get > > the niceness of the vector class while still having the ability to dynamically > > allocate the data. > > ok. Acknowledged. https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/mean_calculator_unittest.cc (right): https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator_unittest.cc:17: TEST(MeanCalculatorTest, Fullness) { On 2016/03/14 14:51:42, minyue-webrtc wrote: > On 2016/03/14 09:12:53, peah-webrtc wrote: > > There should also be a test that checks that the correct values of the mean > are > > computed. > > renamed the test I think you should have a separate test that tests the correctness of the mean and one that only tests the IsWindowFull functionality. https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/mean_calculator_unittest.cc (right): https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/mean_calculator_unittest.cc:29: EXPECT_EQ(i - 0.5 * (1 + kWindowLength), *mean_calculator.GetMean()); The test tests the accuracy of the mean calculator only when integer values (albeit cast to float) are used. I think you should add tests where 1) Values with nonzero fractions are stored. 2) Value of largely differing magnitudes are stored. 3) A large number of values are first processed stored in the calculator, and then the correctness of the mean is verified after that (the mean should then be equal to the mean of the kWindowLength values processed at the end.
Comments from me for parts of Patch Set 5. I haven't looked at the newly added files yet. https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:53: // Diverge metric is based on audio level, which gets updated which every You haven't addressed my comments from Patch Set 1: Diverge -> Divergence which every -> every. https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1452: new webrtc::MeanCalculator(kDivergeMetricAggregationWindow)); Do you need webrtc:: here? Looks like we are in the webrtc namespace. https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:1453: if (aec->fraction_filter_divergent.get() == nullptr) { Can you explain what happens here? https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core_internal.h (right): https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core_internal.h:126: std::unique_ptr<webrtc::MeanCalculator> fraction_filter_divergent; Is webrtc:: needed? Maybe a better name is just filter_divergence? Or do we need to have "fraction" in the name? Divergent isn't really the same as Divergence, but maybe you meant divergent?
Patchset #6 (id:220001) has been deleted
Patchset #6 (id:240001) has been deleted
Hi reviewers,
Thanks for the review! Two new patch sets are added now.
Patch set 6: rebasing
Since BlockMeanCalculator is added by another commit, we dropped the concept
of MeanCalculator in this CL. And now it is much cleaner.
Patch set 7: adding audibility
Tina's comments are addressed. And more importantly, non-audible divergence
is excluded in the measure.
https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec/aec_core.cc (right):
https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec/aec_core.cc:630:
aec->fraction_filter_divergent->AddSample(level_increase >
On 2016/03/15 09:18:40, peah-webrtc wrote:
> On 2016/03/14 14:51:41, minyue-webrtc wrote:
> > On 2016/03/14 09:12:53, peah-webrtc wrote:
> > > Is a sliding window average measure really needed for this? It is quite
> > > expensive compared to the alternatives, both in terms of computations,
> amount
> > of
> > > code and amount of memory.
> > >
> > > Have you considered using a counter that counts occurrences when this
> happens?
> > > Since you are logging binary 1 or 0 that should be an option.
> > >
> > > What about a sliding average? Have you looked into that?
> >
> > I wrote a new MeanCalculator, with the hope that it can be used for other
mean
> > calculations, e.g., the near/far/linout/nlpout audio levels. I think, yes,
it
> > increases memory usage, but benefits are that 1) the update frequency (time
> when
> > we want to obtain the mean) is determined by the averaging window length, 2)
> the
> > mean can become smoothed.
> >
> > Let me know if these benefits are worthy for introducing MeanCalculator.
> >
> > Computationally, it is really cheap: sum = sum - sample[t - window] +
> sample[t]
>
> What do you mean by that the update frequency is determined by the window
> length? I don't really follow that.
>
> What do you mean by the smoothing of the mean? If smoothing is needed for the
> mean is needed, I think it makes sense to add that in the scope of this CL as
> such a smoothing would alter the MeanCalculator class.
as we offline discussed
https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec/aec_core.cc:630:
aec->fraction_filter_divergent->AddSample(level_increase >
On 2016/03/15 09:18:40, peah-webrtc wrote:
> On 2016/03/14 14:51:41, minyue-webrtc wrote:
> > On 2016/03/14 09:12:53, peah-webrtc wrote:
> > > Is a sliding window average measure really needed for this? It is quite
> > > expensive compared to the alternatives, both in terms of computations,
> amount
> > of
> > > code and amount of memory.
> > >
> > > Have you considered using a counter that counts occurrences when this
> happens?
> > > Since you are logging binary 1 or 0 that should be an option.
> > >
> > > What about a sliding average? Have you looked into that?
> >
> > I wrote a new MeanCalculator, with the hope that it can be used for other
mean
> > calculations, e.g., the near/far/linout/nlpout audio levels. I think, yes,
it
> > increases memory usage, but benefits are that 1) the update frequency (time
> when
> > we want to obtain the mean) is determined by the averaging window length, 2)
> the
> > mean can become smoothed.
> >
> > Let me know if these benefits are worthy for introducing MeanCalculator.
> >
> > Computationally, it is really cheap: sum = sum - sample[t - window] +
> sample[t]
>
> I think that one thing that is missing is that the metric is currently not
used
> anywhere, so it is really hard to see that this kind of update frequency/order
> of accuracy is needed. Could you please add the reporting of the metric as
well
> in this CL?
Better to add in a follow up CL IMO (and it is drafted already)
https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec/aec_core.cc:630:
aec->fraction_filter_divergent->AddSample(level_increase >
On 2016/03/15 09:18:40, peah-webrtc wrote:
> On 2016/03/14 14:51:41, minyue-webrtc wrote:
> > On 2016/03/14 09:12:53, peah-webrtc wrote:
> > > Is a sliding window average measure really needed for this? It is quite
> > > expensive compared to the alternatives, both in terms of computations,
> amount
> > of
> > > code and amount of memory.
> > >
> > > Have you considered using a counter that counts occurrences when this
> happens?
> > > Since you are logging binary 1 or 0 that should be an option.
> > >
> > > What about a sliding average? Have you looked into that?
> >
> > I wrote a new MeanCalculator, with the hope that it can be used for other
mean
> > calculations, e.g., the near/far/linout/nlpout audio levels. I think, yes,
it
> > increases memory usage, but benefits are that 1) the update frequency (time
> when
> > we want to obtain the mean) is determined by the averaging window length, 2)
> the
> > mean can become smoothed.
> >
> > Let me know if these benefits are worthy for introducing MeanCalculator.
> >
> > Computationally, it is really cheap: sum = sum - sample[t - window] +
> sample[t]
>
> If you foresee future uses of the class I think you should add the class only
> when you introduce that use of it.
>
> As it is now, I think the class is not really fitting what is done as it is
much
> too general. Regardless of the class being already available, I think the way
it
> is used in this place motivates a different implementation of it (one option
> could be to use a template class that allow computing the means for a certain
> type but at this stage, with only one place using it, I think adding such a
> class is too early).
Ok. I'd remove MeanCalculator
https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/utility/mean_calculator.h (right):
https://codereview.webrtc.org/1739993003/diff/180001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/utility/mean_calculator.h:13:
On 2016/03/15 09:18:41, peah-webrtc wrote:
> On 2016/03/14 14:51:42, minyue-webrtc wrote:
> > On 2016/03/14 09:12:53, peah-webrtc wrote:
> > > Until there are more uses of this class, I think it should not be placed
in
> > > utilities and instead be put into the aec folder and be tailormade to
> exactly
> > > what it is needed there. As it is now, the only usage is in aec_core.cc
> where
> > it
> > > is used to store float values of 1.0 or 0.0 which I think makes it
motivated
> > to
> > > write the code to exactly look at that.
> >
> > As mentioned in an earlier reply, the reason of writing it this way is
indeed
> to
> > make more use of it. not in this CL though WDYT?
>
> I think that it then is better to delay the introduction of the class until
next
> time it is needed , as the functionality in the class is not that well suited
> for how it is used (to compute the mean of integers).
Ok, sure. I will remove it
https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec/aec_core.cc (right):
https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec/aec_core.cc:53: // Diverge metric is based
on audio level, which gets updated which every
On 2016/03/17 12:28:39, tlegrand-webrtc wrote:
> You haven't addressed my comments from Patch Set 1:
> Diverge -> Divergence
> which every -> every.
Sorry, I might have missed it. Now it is addressed.
https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec/aec_core.cc:1452: new
webrtc::MeanCalculator(kDivergeMetricAggregationWindow));
On 2016/03/17 12:28:39, tlegrand-webrtc wrote:
> Do you need webrtc:: here? Looks like we are in the webrtc namespace.
True. the namespace was added recently. now I can remove webrtc::
https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec/aec_core.cc:1453: if
(aec->fraction_filter_divergent.get() == nullptr) {
On 2016/03/17 12:28:39, tlegrand-webrtc wrote:
> Can you explain what happens here?
This is to check if the memory is allocated correctly. But now I avoid dynamic
memory allocation.
https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec/aec_core_internal.h (right):
https://codereview.webrtc.org/1739993003/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec/aec_core_internal.h:126:
std::unique_ptr<webrtc::MeanCalculator> fraction_filter_divergent;
On 2016/03/17 12:28:39, tlegrand-webrtc wrote:
> Is webrtc:: needed?
>
> Maybe a better name is just filter_divergence? Or do we need to have
"fraction"
> in the name? Divergent isn't really the same as Divergence, but maybe you
meant
> divergent?
"divergent" was Karl's suggestion. I think it works. I think it is better to put
"fraction" there, since the value will be between 0 and 1, and "fraction" tells
what it means.
https://codereview.webrtc.org/1739993003/diff/260001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/aec/aec_core.cc (right):
https://codereview.webrtc.org/1739993003/diff/260001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/aec/aec_core.cc:574:
self->fraction_filter_diverge.Reset();
this should be "divergent". It is fixed in the next patch set.
I added a comment also. I think the CL is close to finishing. Would you help reviewing the final changes? https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:623: 100.0 * aec->nlpoutlevel.minlevel; I found that 100.0 might be a bit large. I prefer using 40.0, which is consistent with actThresholdClean (line 606). Experiments showed a bit better result.
https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:160: // TODO(minyue): Due to a legacy bug, |framelevel| and |averagelevel| use a I'd prefer to have the comments outside of the constructor definition. https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:622: const bool audible = aec->nlpoutlevel.framelevel.GetLatestMean() > The audible flag basically checks wether the NLP mean is more than 100 times (20 dB) larger than the NLP min, right? To me the name does not really make sense as I cannot make the connection to what it is that would be audible; the echo or the nearend? Please either clarify this in a comment, or rename the variable. https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:623: 100.0 * aec->nlpoutlevel.minlevel; On 2016/04/04 15:07:44, minyue-webrtc wrote: > I found that 100.0 might be a bit large. I prefer using 40.0, which is > consistent with actThresholdClean (line 606). Experiments showed a bit better > result. What do you mean here? Do you want to keep it 100 or do you want to change it to 40? https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:627: aec->fraction_filter_divergent.AddValue(audible && Here the fraction_filter_divergent object is only used as an integer counter. I don't think the MeanCalculator should be used for this as that is a class for computing the blocked mean of floating point variables. In this case, the all the variables are integers and as the block length is known and hardcoded the mean does not really need to be computed. https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:627: aec->fraction_filter_divergent.AddValue(audible && What about renaming fraction_filter_divergent to filter_divergence?
Thanks for the comments! Before adding a new patch set, let me discuss with you on possible changes inline. https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:160: // TODO(minyue): Due to a legacy bug, |framelevel| and |averagelevel| use a On 2016/04/05 09:15:52, peah-webrtc wrote: > I'd prefer to have the comments outside of the constructor definition. Done. https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:622: const bool audible = aec->nlpoutlevel.framelevel.GetLatestMean() > On 2016/04/05 09:15:52, peah-webrtc wrote: > The audible flag basically checks wether the NLP mean is more than 100 times (20 > dB) larger than the NLP min, right? To me the name does not really make sense as > I cannot make the connection to what it is that would be audible; the echo or > the nearend? > Please either clarify this in a comment, or rename the variable. how about nlpout_active? https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:623: 100.0 * aec->nlpoutlevel.minlevel; On 2016/04/05 09:15:52, peah-webrtc wrote: > On 2016/04/04 15:07:44, minyue-webrtc wrote: > > I found that 100.0 might be a bit large. I prefer using 40.0, which is > > consistent with actThresholdClean (line 606). Experiments showed a bit better > > result. > > What do you mean here? Do you want to keep it 100 or do you want to change it to > 40? I want to change to 40, which gave a bit better result in my offline tests. I did not add a patch set just for this, therefore, I put a comment instead. https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:627: aec->fraction_filter_divergent.AddValue(audible && On 2016/04/05 09:15:52, peah-webrtc wrote: > What about renaming fraction_filter_divergent to filter_divergence? Tina also wanted to drop "fraction". My idea is that the value is between 0 and 1, and "fraction" directly tells what it means. Regarding "divergent" or "divergence", I have no preference. "divergent" was suggested by Karl. How about "filter_divergence_fraction"? https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:627: aec->fraction_filter_divergent.AddValue(audible && On 2016/04/05 09:15:52, peah-webrtc wrote: > Here the fraction_filter_divergent object is only used as an integer counter. I > don't think the MeanCalculator should be used for this as that is a class for > computing the blocked mean of floating point variables. > > In this case, the all the variables are integers and as the block length is > known and hardcoded the mean does not really need to be computed. I agree that BlockMeanCalculator is for floating point, while fraction is to count the occurrences. If we want to calculate fraction, we basically need to introduce three variables: size_t count_ // counts total samples size_t occurrences_ // counts occurrences float latest_fraction_ and do the following if (event occurred) { occurrences_++; } count_++ if (count_ == block_length) { count_ = 0; latest_fraction_ = (float)occurrences_ / block_length; } This is essentially what BlockMeanCalculator does. I think using BlockMeanCalculator eliminates much code duplication. My suggestion is adding a comment on why we can use BlockMeanCalculator to calculate fraction. WDYT?
https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:622: const bool audible = aec->nlpoutlevel.framelevel.GetLatestMean() > On 2016/04/05 09:50:31, minyue-webrtc wrote: > On 2016/04/05 09:15:52, peah-webrtc wrote: > > The audible flag basically checks wether the NLP mean is more than 100 times > (20 > > dB) larger than the NLP min, right? To me the name does not really make sense > as > > I cannot make the connection to what it is that would be audible; the echo or > > the nearend? > > Please either clarify this in a comment, or rename the variable. > > how about nlpout_active? That is also vague to me. Do you mean that the NLP is transparent? audible will be high if -the nlp gain varies between low and high, but low it either -the nlp gain is constantly high -the nlp gain is constantly low. Therefore I think audible is neither a measure of nlp transparency, nlp activeness or whether something is audible in the output. https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:623: 100.0 * aec->nlpoutlevel.minlevel; On 2016/04/05 09:50:31, minyue-webrtc wrote: > On 2016/04/05 09:15:52, peah-webrtc wrote: > > On 2016/04/04 15:07:44, minyue-webrtc wrote: > > > I found that 100.0 might be a bit large. I prefer using 40.0, which is > > > consistent with actThresholdClean (line 606). Experiments showed a bit > better > > > result. > > > > What do you mean here? Do you want to keep it 100 or do you want to change it > to > > 40? > > I want to change to 40, which gave a bit better result in my offline tests. I > did not add a patch set just for this, therefore, I put a comment instead. Acknowledged. https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:627: aec->fraction_filter_divergent.AddValue(audible && On 2016/04/05 09:50:31, minyue-webrtc wrote: > On 2016/04/05 09:15:52, peah-webrtc wrote: > > Here the fraction_filter_divergent object is only used as an integer counter. > I > > don't think the MeanCalculator should be used for this as that is a class for > > computing the blocked mean of floating point variables. > > > > In this case, the all the variables are integers and as the block length is > > known and hardcoded the mean does not really need to be computed. > > I agree that BlockMeanCalculator is for floating point, while fraction is to > count the occurrences. If we want to calculate fraction, we basically need to > introduce three variables: > > size_t count_ // counts total samples > size_t occurrences_ // counts occurrences > float latest_fraction_ > > and do the following > > if (event occurred) { > occurrences_++; > } > count_++ > if (count_ == block_length) { > count_ = 0; > latest_fraction_ = (float)occurrences_ / block_length; > } > > This is essentially what BlockMeanCalculator does. I think using > BlockMeanCalculator eliminates much code duplication. > > My suggestion is adding a comment on why we can use BlockMeanCalculator to > calculate fraction. WDYT? I think it would be better to use a proper counter or that you create a separate class for that. The current usage of the AddValue method involves an implicit cast from bool to int and relies on this to be a summable integer which is added to the mean. I think that even though it is fully possible to reuse the BlockMeanCalculator this complicates the code. (And even though the code is shorter, it is slightly more complex as the cast is needed and as the AddValue calls is needed even when the filter is not diverging). https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:627: aec->fraction_filter_divergent.AddValue(audible && On 2016/04/05 09:50:31, minyue-webrtc wrote: > On 2016/04/05 09:15:52, peah-webrtc wrote: > > What about renaming fraction_filter_divergent to filter_divergence? > > Tina also wanted to drop "fraction". My idea is that the value is between 0 and > 1, and "fraction" directly tells what it means. > > Regarding "divergent" or "divergence", I have no preference. "divergent" was > suggested by Karl. > > How about "filter_divergence_fraction"? Lets go with that, or with divergent_filter_fraction. It is nice to have a unit attached to the variable; I think there should be a better unit for that but I cannot come up with a better suggestion so lets go for that.
Patchset #8 (id:300001) has been deleted
Per the discussion on the use of BlockMeanCalculator to calculate fraction, I added BlockFractionCalculator, it is similar to BlockMeanCalculator but more dedicated to fraction calculation. Other comments are addressed in the new patch set as well. https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:622: const bool audible = aec->nlpoutlevel.framelevel.GetLatestMean() > On 2016/04/05 10:42:03, peah-webrtc wrote: > On 2016/04/05 09:50:31, minyue-webrtc wrote: > > On 2016/04/05 09:15:52, peah-webrtc wrote: > > > The audible flag basically checks wether the NLP mean is more than 100 times > > (20 > > > dB) larger than the NLP min, right? To me the name does not really make > sense > > as > > > I cannot make the connection to what it is that would be audible; the echo > or > > > the nearend? > > > Please either clarify this in a comment, or rename the variable. > > > > how about nlpout_active? > > That is also vague to me. Do you mean that the NLP is transparent? > > audible will be high if > -the nlp gain varies between low and high, > but low it either > -the nlp gain is constantly high > -the nlp gain is constantly low. > > Therefore I think audible is neither a measure of nlp transparency, nlp > activeness or whether something is audible in the output. I used nlpout_active to tell if the final output signal is active or silent. It is similar to line 644 that measures whether far-end signal is active. But maybe nlpout_active as a name has too much emphasis on nlp, how about out_signal_active? https://codereview.webrtc.org/1739993003/diff/280001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:627: aec->fraction_filter_divergent.AddValue(audible && On 2016/04/05 10:42:03, peah-webrtc wrote: > On 2016/04/05 09:50:31, minyue-webrtc wrote: > > On 2016/04/05 09:15:52, peah-webrtc wrote: > > > Here the fraction_filter_divergent object is only used as an integer > counter. > > I > > > don't think the MeanCalculator should be used for this as that is a class > for > > > computing the blocked mean of floating point variables. > > > > > > In this case, the all the variables are integers and as the block length is > > > known and hardcoded the mean does not really need to be computed. > > > > I agree that BlockMeanCalculator is for floating point, while fraction is to > > count the occurrences. If we want to calculate fraction, we basically need to > > introduce three variables: > > > > size_t count_ // counts total samples > > size_t occurrences_ // counts occurrences > > float latest_fraction_ > > > > and do the following > > > > if (event occurred) { > > occurrences_++; > > } > > count_++ > > if (count_ == block_length) { > > count_ = 0; > > latest_fraction_ = (float)occurrences_ / block_length; > > } > > > > This is essentially what BlockMeanCalculator does. I think using > > BlockMeanCalculator eliminates much code duplication. > > > > My suggestion is adding a comment on why we can use BlockMeanCalculator to > > calculate fraction. WDYT? > > I think it would be better to use a proper counter or that you create a separate > class for that. The current usage of the AddValue method involves an implicit > cast from bool to int and relies on this to be a summable integer which is added > to the mean. > > I think that even though it is fully possible to reuse the BlockMeanCalculator > this complicates the code. (And even though the code is shorter, it is slightly > more complex as the cast is needed and as the AddValue calls is needed even when > the filter is not diverging). > > > Then I prefer to add a new class: BlockFractionCalculator. It can also be used for fraction_poor_delays in the future.
https://codereview.webrtc.org/1739993003/diff/320001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:57: static const int kDivergeMetricAggregationWindow = 25; This should probably be called kDivergeMetricAggregationWindowSize. https://codereview.webrtc.org/1739993003/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:630: level_increase > std::max(0.01 * near_level, 1.0)); If this would be a general and common task maybe a separate class would be merited but at this stage this is only used here, and what is done is so simple, so I don't think that a separate utility class is motivated for this. I think the class instead should be locally implemented inside this file. https://codereview.webrtc.org/1739993003/diff/320001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/block_fraction_calculator.h (right): https://codereview.webrtc.org/1739993003/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/block_fraction_calculator.h:22: class BlockFractionCalculator { Please make this local toaec_core.cc instead. https://codereview.webrtc.org/1739993003/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/block_fraction_calculator.h:22: class BlockFractionCalculator { I think it is better if you name the class in a more specific and less general manner as it is only used in one place right now. Please change the name to what is used for, i.e. something along the line of DivergentFilterFraction.
Hi Per, I followed your request. How does it look now? https://codereview.webrtc.org/1739993003/diff/320001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1739993003/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:57: static const int kDivergeMetricAggregationWindow = 25; On 2016/04/06 11:01:10, peah-webrtc wrote: > This should probably be called kDivergeMetricAggregationWindowSize. Done. https://codereview.webrtc.org/1739993003/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:630: level_increase > std::max(0.01 * near_level, 1.0)); On 2016/04/06 11:01:10, peah-webrtc wrote: > If this would be a general and common task maybe a separate class would be > merited but at this stage this is only used here, and what is done is so simple, > so I don't think that a separate utility class is motivated for this. > > I think the class instead should be locally implemented inside this file. > Done. https://codereview.webrtc.org/1739993003/diff/320001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/utility/block_fraction_calculator.h (right): https://codereview.webrtc.org/1739993003/diff/320001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/utility/block_fraction_calculator.h:22: class BlockFractionCalculator { On 2016/04/06 11:01:10, peah-webrtc wrote: > I think it is better if you name the class in a more specific and less general > manner as it is only used in one place right now. Please change the name to what > is used for, i.e. something along the line of DivergentFilterFraction. ok
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739993003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739993003/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_dbg/builds/8370)
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739993003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739993003/360001
On 2016/04/07 08:08:00, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1739993003/360001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1739993003/360001 Nice! lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1739993003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1739993003/360001
Message was sent while issue was closed.
Description was changed from ========== Adding fraction of filter divergence in AEC metrics. With the current AEC algorithm, the divergence of the echo cancelling linear filter is a strong signal of non-transparency. During double talk, it can result in a ducking artifacts. In this CL, a metric that tells the fraction of filter divergence is added. This can measure the severity of non-transparency. BUG= ========== to ========== Adding fraction of filter divergence in AEC metrics. With the current AEC algorithm, the divergence of the echo cancelling linear filter is a strong signal of non-transparency. During double talk, it can result in a ducking artifacts. In this CL, a metric that tells the fraction of filter divergence is added. This can measure the severity of non-transparency. BUG= ==========
Message was sent while issue was closed.
Committed patchset #10 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Adding fraction of filter divergence in AEC metrics. With the current AEC algorithm, the divergence of the echo cancelling linear filter is a strong signal of non-transparency. During double talk, it can result in a ducking artifacts. In this CL, a metric that tells the fraction of filter divergence is added. This can measure the severity of non-transparency. BUG= ========== to ========== Adding fraction of filter divergence in AEC metrics. With the current AEC algorithm, the divergence of the echo cancelling linear filter is a strong signal of non-transparency. During double talk, it can result in a ducking artifacts. In this CL, a metric that tells the fraction of filter divergence is added. This can measure the severity of non-transparency. BUG= Committed: https://crrev.com/e10fc3fb2db623c65bb047000d6e6346f728e7dd Cr-Commit-Position: refs/heads/master@{#12276} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e10fc3fb2db623c65bb047000d6e6346f728e7dd Cr-Commit-Position: refs/heads/master@{#12276}
Message was sent while issue was closed.
Sorry for the late review. LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
