Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1581183005:
Cleaning up AEC metrics. (Closed)
|
Created:
4 years, 11 months ago by minyue-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-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. |
DescriptionCleaning up AEC metrics.
Current implementation of AEC metrics does not read nicely. It messes up between a noise-removed calculation and a raw calculation.
I tried to clean it up, in which, I stick to the raw calculation since the noise-removed version can show some problem when the noise is overestimated.
BUG=
Committed: https://crrev.com/2b6707826e4c73662c4196f5c999fee3f13538ba
Cr-Commit-Position: refs/heads/master@{#12455}
Patch Set 1 #
Total comments: 11
Patch Set 2 : rebasing #Patch Set 3 : adding protect over division #
Total comments: 28
Patch Set 4 : improve the calculation #
Total comments: 8
Patch Set 5 : rebasing #Patch Set 6 : renaming a function #
Total comments: 12
Patch Set 7 : revising comment #
Total comments: 15
Messages
Total messages: 35 (7 generated)
Description was changed from ========== clean up BUG= ========== to ========== Cleaning up AEC metrics. Current implementation of AEC metrics does not read nicely. It messes up between a noise-removed calculation and a raw calculation. I tried to clean it up, in which, I stick to the raw calculation since the noise-removed version can show some problem when the noise is overestimated. BUG= ==========
minyue@webrtc.org changed reviewers: + peah@webrtc.org, tina.legrand@webrtc.org
Hi Per and Tina, I tried to clean up the AEC metrics calculation. There is no hurry to check this in. But it is good to know there is something to improve in this scope. The actual way to improve it can still be discussed. The CL is my suggestion. The change will fail some unit tests. Updating the reference stats file is needed, when we decide the way to go for.
On 2016/01/14 12:30:21, minyue-webrtc wrote: > Hi Per and Tina, > > I tried to clean up the AEC metrics calculation. There is no hurry to check this > in. But it is good to know there is something to improve in this scope. The > actual way to improve it can still be discussed. The CL is my suggestion. > > The change will fail some unit tests. Updating the reference stats file is > needed, when we decide the way to go for. By the way, from the try result, we can see that the changes in metrics are reasonably small (5db max)
https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:332: metric->counter++; This increase should handle wraparounds. I know this is very unlikely to happen, but regardless of that I think we should still have it. (for 32 bit ints, this will wraparound after 248 days, but for 16 bit ints it will wraparound after 5 hours). https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:338: metric->hicounter++; As above, please add handling of wraparound. https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:680: UpdateMetric(&aec->erl, 10 * (float)log10(1e-10f + While we are anyway changing the metrics: is there any reason for logging the metric in dB? It is anyway stored as a float, and the log function may be costly on some platforms. Storing the metric as the actual ratio would 1) Remove 3 log10 operations from the code. 2) Allow us to remove the 1e-10f constant which bias the values for levels close to zero. If the people monitoring these metrics know how to interpret these dB values, we probably should keep them in dB (provided that there is no way to do that conversion in the stats monitoring system). Otherwise I think we should instead store the actual ratio. https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:681: aec->farlevel.averagelevel / aec->nearlevel.averagelevel)); Is there any guarantee that averagelevel cannot be 0? https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:685: aec->nearlevel.averagelevel / (2 * aec->linoutlevel.averagelevel))); Is there any guarantee that averagelevel cannot be 0? https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:689: aec->nearlevel.averagelevel / (2 * aec->nlpoutlevel.averagelevel))); Is there any guarantee that averagelevel cannot be 0?
On 2016/01/14 13:37:18, minyue-webrtc wrote: > On 2016/01/14 12:30:21, minyue-webrtc wrote: > > Hi Per and Tina, > > > > I tried to clean up the AEC metrics calculation. There is no hurry to check > this > > in. But it is good to know there is something to improve in this scope. The > > actual way to improve it can still be discussed. The CL is my suggestion. > > > > The change will fail some unit tests. Updating the reference stats file is > > needed, when we decide the way to go for. > > By the way, from the try result, we can see that the changes in metrics are > reasonably small (5db max) Great CL! One comment regarding your comment is that 5 dB is actually quite big as the practical ranges for these ratios are 0-40 dB. But I guess that you mean that it does not happen often, and for levels where the differences don't make sense?
https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:680: UpdateMetric(&aec->erl, 10 * (float)log10(1e-10f + On 2016/01/15 06:52:20, peah-webrtc wrote: > While we are anyway changing the metrics: is there any reason for logging the > metric in dB? It is anyway stored as a float, and the log function may be costly > on some platforms. > > Storing the metric as the actual ratio would > 1) Remove 3 log10 operations from the code. > 2) Allow us to remove the 1e-10f constant which bias the values for levels close > to zero. > > If the people monitoring these metrics know how to interpret these dB values, we > probably should keep them in dB (provided that there is no way to do that > conversion in the stats monitoring system). > Otherwise I think we should instead store the actual ratio. Good suggestion, but be minded that to an upper level, we need to report integer (unless we want to change that also). Then I am not sure if dynamic range of integer is bit enough. https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:685: aec->nearlevel.averagelevel / (2 * aec->linoutlevel.averagelevel))); On 2016/01/15 06:52:20, peah-webrtc wrote: > Is there any guarantee that averagelevel cannot be 0? I don't see, and I am not sure how it did not run into any problem before. Good to add epsilon to the denominator any way, IMO.
Hi, After some rebasing, this CL is ready for review again. To recapitulate what this CL is about: 1) It is to improve the readability of the metric calculation in AEC. 2) Also the calculation of ERLE and A_NLP, but not ERL, were based on a noise-removed audio level. By offline discussion, we think it is good to use the raw audio level.
First set of review comments from me. I think the new way of calculating the stats make sense, and I'm thinking we might add tests or sanity checks in the code. https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:332: metric->counter++; On 2016/01/15 06:52:20, peah-webrtc wrote: > This increase should handle wraparounds. I know this is very unlikely to happen, > but regardless of that I think we should still have it. > (for 32 bit ints, this will wraparound after 248 days, but for 16 bit ints it > will wraparound after 5 hours). To Per: Are there any platforms today where int is only 16-bits? https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:373: static void UpdateMetric(Stats* metric, float nomin, float denom) { How do we test this function, to make sure it handles different input in the right way, like metric == NULL and denom = 0? https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:374: const float temp = nomin / denom; Is division with 0 allowed for float? https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:391: metric->counter++; Do we ever reset these counters? I realize you didn't change this functionality, but might be worth checking. What happens if you keep a call running over night?
Patchset #4 (id:60001) has been deleted
https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:332: metric->counter++; On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > On 2016/01/15 06:52:20, peah-webrtc wrote: > > This increase should handle wraparounds. I know this is very unlikely to > happen, > > but regardless of that I think we should still have it. > > (for 32 bit ints, this will wraparound after 248 days, but for 16 bit ints it > > will wraparound after 5 hours). > > To Per: Are there any platforms today where int is only 16-bits? I would guess there are are no important such platforms. As far as I know, int is in practice only 16 bit on 16 bit platforms. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:373: static void UpdateMetric(Stats* metric, float nomin, float denom) { On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > How do we test this function, to make sure it handles different input in the > right way, like metric == NULL and denom = 0? Since it is local within aec_core.cc, I think a DCHECK on metric not being null should be sufficient for that. But the check for demom==0 must be added. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:373: static void UpdateMetric(Stats* metric, float nomin, float denom) { Breaking out the computation of the metric in a separate function is great as it removes a lot of duplicated code. I think, however, that the function name UpdateMetric is misleading, as it is only a certain type of metric that is updated by this. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:374: const float temp = nomin / denom; On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > Is division with 0 allowed for float? No, we should never do that. (If happening on the tests, that will also throw an exception that the trybots won't accept.) https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:374: const float temp = nomin / denom; Please rename this variable to what is is, as it is only being used for this purpose. For instance, the name quotient would be more indicative to what it stores. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:375: if (!std::isfinite(temp)) I think it is better to check for denom being zero, and then return. That is more explicit, avoids the division by zero, and has a lower complexity. Furthermore, I think a special care should be taken to what happens with the metric when denom is zero. For instance, an SNR should probably be set high if the denom is zero, rather than just keep the previous SNR. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:391: metric->counter++; On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > Do we ever reset these counters? I realize you didn't change this functionality, > but might be worth checking. What happens if you keep a call running over night? I think it is nice to have a controlled wraparound, even though they will take very long to wraparound on 32 bit systems. For 32 bit integers, if they are updated 250 times a second (currently they are updated less frequenctly) wraparound will happen after 99 days, but since that wraparound will eventually cause a division by zero I think it is better to handle that case explicitly in a controlled manner. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:708: // ERL Comment should be terminated with a period. Furthermore, I think it would be good with a more verbose comment, since the ERL abbreviation does not add anything more than what can be read from the UpdateMetric method call. For instance, a description describing in words what the metric measures. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:711: To be consistent with how it is done below, I think the empty line here should be removed. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:714: // A_NLP See comment above. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:719: // ERLE See comment above.
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/aec/aec_core.c (right): https://codereview.webrtc.org/1581183005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/aec/aec_core.c:332: metric->counter++; On 2016/04/14 13:46:10, peah-webrtc wrote: > On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > > On 2016/01/15 06:52:20, peah-webrtc wrote: > > > This increase should handle wraparounds. I know this is very unlikely to > > happen, > > > but regardless of that I think we should still have it. > > > (for 32 bit ints, this will wraparound after 248 days, but for 16 bit ints > it > > > will wraparound after 5 hours). > > > > To Per: Are there any platforms today where int is only 16-bits? > > I would guess there are are no important such platforms. As far as I know, int > is in practice only 16 bit on 16 bit platforms. "You should assume that an int is at least 32 bits, but don't assume that it has more than 32 bits." https://google.github.io/styleguide/cppguide.html#Integer_Types
https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:373: static void UpdateMetric(Stats* metric, float nomin, float denom) { On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > How do we test this function, to make sure it handles different input in the > right way, like metric == NULL and denom = 0? Yes, it is good to sanity check on |metric|, added a DCHECK. For denom = 0, it will be caught by std::isfinite(temp), which will catch other error, like ((a very large number) / (a very small number)) https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:374: const float temp = nomin / denom; On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > Is division with 0 allowed for float? For float, denom = 0 will not crash. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:391: metric->counter++; On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > Do we ever reset these counters? I realize you didn't change this functionality, > but might be worth checking. What happens if you keep a call running over night? Very good point. I am not fully sure what we would like the average value to represent. The average of a full call? If so, we only need to try to avoid overflow or improve the calculation. I did two things: 1) change the counter to type to size_t, which this, overflow should in principle never happen. 2) add a CHECK to protect overflow. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:391: metric->counter++; On 2016/04/14 13:46:10, peah-webrtc wrote: > On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > > Do we ever reset these counters? I realize you didn't change this > functionality, > > but might be worth checking. What happens if you keep a call running over > night? > > I think it is nice to have a controlled wraparound, even though they will take > very long to wraparound on 32 bit systems. For 32 bit integers, if they are > updated 250 times a second (currently they are updated less frequenctly) > wraparound will happen after 99 days, but since that wraparound will eventually > cause a division by zero I think it is better to handle that case explicitly in > a controlled manner. After 391, I added a RTC_CHECK_NE(0u, metric->counter). It is gone (weird). Do you think it is good? or is it better to add something like if (metric->counter == 0) { metric->sum = 0; } else { metric->sum += metric->instant; metric->average = metric->sum / metric->counter; } To me, |metric->counter == 0| is more or less a design error, for e.g., too frequent update, or wrong type on counter. A check would be good. But I agree it is safer to go for the second solution.
https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:373: static void UpdateMetric(Stats* metric, float nomin, float denom) { On 2016/04/14 14:11:12, minyue-webrtc wrote: > On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > > How do we test this function, to make sure it handles different input in the > > right way, like metric == NULL and denom = 0? > > Yes, it is good to sanity check on |metric|, added a DCHECK. > > For denom = 0, it will be caught by std::isfinite(temp), which will catch other > error, like ((a very large number) / (a very small number)) Maybe it is ok and a good solution, but to me it feels weird to do the division by zero. However, what I don't think is correct in that case is to just keep the last computed value for the metric. If the metric is infinite, that has a meaning in itself and should not be hidden by not updating the metric. I'd rather have a maximum value set for the metric if it is detected to be infinite. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:373: static void UpdateMetric(Stats* metric, float nomin, float denom) { Would it be possible to change the names of nomin and denom to something more meaningful that indicates what kind of values that are passed to the function? https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:374: const float temp = nomin / denom; On 2016/04/14 14:11:12, minyue-webrtc wrote: > On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > > Is division with 0 allowed for float? > > For float, denom = 0 will not crash. Ah, you are right, I did not know that. But I still don't see the advantage of first doing the division and then test for whether it becomes infinite instead of first checking if it will become infinite and only do the division if it will not become infinite. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:391: metric->counter++; On 2016/04/14 14:11:12, minyue-webrtc wrote: > On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > > Do we ever reset these counters? I realize you didn't change this > functionality, > > but might be worth checking. What happens if you keep a call running over > night? > > Very good point. I am not fully sure what we would like the average value to > represent. The average of a full call? If so, we only need to try to avoid > overflow or improve the calculation. > > I did two things: 1) change the counter to type to size_t, which this, overflow > should in principle never happen. 2) add a CHECK to protect overflow. Acknowledged.
Hi, To make the handling of divisor being zero, or a/b resulting in infinity etc, I modified the calculation, by change log(a/b) into log(a) - log(b). Please take a look at Patch set 4. Patchset 5 is a rebasing, which does not require big attention. Thanks! https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:373: static void UpdateMetric(Stats* metric, float nomin, float denom) { On 2016/04/15 10:46:37, peah-webrtc wrote: > Would it be possible to change the names of nomin and denom to something more > meaningful that indicates what kind of values that are passed to the function? Done. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:373: static void UpdateMetric(Stats* metric, float nomin, float denom) { On 2016/04/15 10:46:37, peah-webrtc wrote: > On 2016/04/14 14:11:12, minyue-webrtc wrote: > > On 2016/04/13 14:33:31, tlegrand-webrtc wrote: > > > How do we test this function, to make sure it handles different input in > the > > > right way, like metric == NULL and denom = 0? > > > > Yes, it is good to sanity check on |metric|, added a DCHECK. > > > > For denom = 0, it will be caught by std::isfinite(temp), which will catch > other > > error, like ((a very large number) / (a very small number)) > > Maybe it is ok and a good solution, but to me it feels weird to do the division > by zero. > > However, what I don't think is correct in that case is to just keep the last > computed value for the metric. If the metric is infinite, that has a meaning in > itself and should not be hidden by not updating the metric. > I'd rather have a maximum value set for the metric if it is detected to be > infinite. I think one reason for inf is that we calculate division then log, and the division is easy to overflow. How about use (log a - log b) instead of log(a/b), see new patch. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:374: const float temp = nomin / denom; On 2016/04/14 13:46:10, peah-webrtc wrote: > Please rename this variable to what is is, as it is only being used for this > purpose. For instance, the name quotient would be more indicative to what it > stores. Done. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:708: // ERL On 2016/04/14 13:46:10, peah-webrtc wrote: > Comment should be terminated with a period. Furthermore, I think it would be > good with a more verbose comment, since the ERL abbreviation does not add > anything more than what can be read from the UpdateMetric method call. For > instance, a description describing in words what the metric measures. Done. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:711: On 2016/04/14 13:46:10, peah-webrtc wrote: > To be consistent with how it is done below, I think the empty line here should > be removed. Done. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:714: // A_NLP On 2016/04/14 13:46:10, peah-webrtc wrote: > See comment above. done, although I am not sure if I name it properly. I do not know what A_NLP stands for exactly, so I tried to described it from its calculation. https://codereview.webrtc.org/1581183005/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:719: // ERLE On 2016/04/14 13:46:10, peah-webrtc wrote: > See comment above. Done.
https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:374: static void UpdateMetric(Stats* metric, float numerator, float denominator) { Did you consider changing the name of the method to reflect that it only relates to updating stats metric? What about using the name UpdateStatsMetric? Or maybe better, UpdateRatioBasedMetric, as that would partly explain the meanings of numerator/demoninator. https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:374: static void UpdateMetric(Stats* metric, float numerator, float denominator) { The roles of the numerator/denominator input variables are very unclear and the names are quite anonymous. If you change the name of the function to UpdateRatioBasedMetric their meaning will be more clear, but if you could come up with some other way to more properly name the variables (I could not) it would be even better. https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:381: metric->instant = 10.0 * (log_numerator - log_denominator); I don't see why we cannot treat the case of the denominator == 0 as a special case. It is cheaper, the calculation is more correct and it is explicitly stating that the case of denominator == 0 is a special one that should be treated separately. Adding the epsilon term is fine mathematically, but apart from fixing the numerical issue of division by zero I think it hides away information that could otherwise be passed to the metric and adds an unnecessary bias. https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:713: // ERL The terminating period in the comment is still missing. Also, I still think it would be better if you instead of the abbreviations wrote the description of the abbreviation (e.g., instead of ERL wrote echo return loss). https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:719: // A_NLP The terminating period in the comment is still missing. Also, I still think it would be better if you instead of the abbreviations wrote the description of the abbreviation (e.g., instead of ERL wrote echo return loss). https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:724: // ERLE The terminating period in the comment is still missing. Also, I still think it would be better if you instead of the abbreviations wrote the description of the abbreviation (e.g., instead of ERL wrote echo return loss).
https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:381: metric->instant = 10.0 * (log_numerator - log_denominator); On 2016/04/19 07:03:37, peah-webrtc wrote: > I don't see why we cannot treat the case of the denominator == 0 as a special > case. It is cheaper, the calculation is more correct and it is explicitly > stating that the case of denominator == 0 is a special one that should be > treated separately. > > Adding the epsilon term is fine mathematically, but apart from fixing the > numerical issue of division by zero I think it hides away information that could > otherwise be passed to the metric and adds an unnecessary bias. > > > log(a) - log(b) does not only solve denominator being zero. Another problem with a/b is that if a/b goes out of range, it will become inf, even though log can still represent it.
https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec/aec_core.cc:381: metric->instant = 10.0 * (log_numerator - log_denominator); On 2016/04/19 07:21:06, minyue-webrtc wrote: > On 2016/04/19 07:03:37, peah-webrtc wrote: > > I don't see why we cannot treat the case of the denominator == 0 as a special > > case. It is cheaper, the calculation is more correct and it is explicitly > > stating that the case of denominator == 0 is a special one that should be > > treated separately. > > > > Adding the epsilon term is fine mathematically, but apart from fixing the > > numerical issue of division by zero I think it hides away information that > could > > otherwise be passed to the metric and adds an unnecessary bias. > > > > > > > > log(a) - log(b) does not only solve denominator being zero. Another problem with > a/b is that if a/b goes out of range, it will become inf, even though log can > still represent it. That is definitely true. Let's keep it like this. But please look at th e other comments as well.
On 2016/04/19 10:48:34, peah-webrtc wrote: > https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/aec/aec_core.cc (right): > > https://codereview.webrtc.org/1581183005/diff/80001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/aec/aec_core.cc:381: metric->instant = 10.0 * > (log_numerator - log_denominator); > On 2016/04/19 07:21:06, minyue-webrtc wrote: > > On 2016/04/19 07:03:37, peah-webrtc wrote: > > > I don't see why we cannot treat the case of the denominator == 0 as a > special > > > case. It is cheaper, the calculation is more correct and it is explicitly > > > stating that the case of denominator == 0 is a special one that should be > > > treated separately. > > > > > > Adding the epsilon term is fine mathematically, but apart from fixing the > > > numerical issue of division by zero I think it hides away information that > > could > > > otherwise be passed to the metric and adds an unnecessary bias. > > > > > > > > > > > > > log(a) - log(b) does not only solve denominator being zero. Another problem > with > > a/b is that if a/b goes out of range, it will become inf, even though log can > > still represent it. > > That is definitely true. Let's keep it like this. But please look at th e other > comments as well. Yes, now comes a patch set. Your concerns on the readability are addressed by renaming the function there.
https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:369: // Update metric with 10 * log10(numerator / denominator), This comment should end with a period. https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:376: const float log_numerator = log10(numerator + 1e-10); Maybe use 1e-10f? Or does that not matter? https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:377: const float log_denominator = log10(denominator + 1e-10); Maybe use 1e-10f? Or does that not matter? https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:378: metric->instant = 10.0 * (log_numerator - log_denominator); Maybe use 10.0f? Or does that not matter? https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:709: The comment should end with a period. Furthermore, did you consider my comment on expanding the abbreviation in order to make the comment more descriptive? https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:716: // A_NLP The comment should end with a period. Furthermore, did you consider my comment on expanding the abbreviation in order to make the comment more descriptive? https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:722: // ERLE The comment should end with a period. Furthermore, did you consider my comment on expanding the abbreviation in order to make the comment more descriptive?
Thanks for the feedback! I revised the commenting. https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:369: // Update metric with 10 * log10(numerator / denominator), On 2016/04/20 13:57:41, peah-webrtc wrote: > This comment should end with a period. oh, thanks. https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:376: const float log_numerator = log10(numerator + 1e-10); On 2016/04/20 13:57:41, peah-webrtc wrote: > Maybe use 1e-10f? Or does that not matter? I think it is better to use f-suffix, it helps the compiler to take it as float as opposed to double. https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:709: On 2016/04/20 13:57:40, peah-webrtc wrote: > The comment should end with a period. > > Furthermore, did you consider my comment on expanding the abbreviation in order > to make the comment more descriptive? oh, thanks. I did it locally. I even asked you about the what A_NLP may mean. It might not have be committed properly. Sorry. https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:716: // A_NLP On 2016/04/20 13:57:40, peah-webrtc wrote: > The comment should end with a period. > > Furthermore, did you consider my comment on expanding the abbreviation in order > to make the comment more descriptive? done, although I am not sure if I name it properly. I do not know what A_NLP stands for exactly, so I tried to described it from its calculation. https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:722: // ERLE On 2016/04/20 13:57:40, peah-webrtc wrote: > The comment should end with a period. > > Furthermore, did you consider my comment on expanding the abbreviation in order > to make the comment more descriptive? Done.
On 2016/04/20 14:30:04, minyue-webrtc wrote: > Thanks for the feedback! I revised the commenting. > > https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... > File webrtc/modules/audio_processing/aec/aec_core.cc (right): > > https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/aec/aec_core.cc:369: // Update metric with 10 * > log10(numerator / denominator), > On 2016/04/20 13:57:41, peah-webrtc wrote: > > This comment should end with a period. > > oh, thanks. > > https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/aec/aec_core.cc:376: const float log_numerator = > log10(numerator + 1e-10); > On 2016/04/20 13:57:41, peah-webrtc wrote: > > Maybe use 1e-10f? Or does that not matter? > > I think it is better to use f-suffix, it helps the compiler to take it as float > as opposed to double. > > https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/aec/aec_core.cc:709: > On 2016/04/20 13:57:40, peah-webrtc wrote: > > The comment should end with a period. > > > > Furthermore, did you consider my comment on expanding the abbreviation in > order > > to make the comment more descriptive? > > oh, thanks. I did it locally. I even asked you about the what A_NLP may mean. It > might not have be committed properly. Sorry. > > https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/aec/aec_core.cc:716: // A_NLP > On 2016/04/20 13:57:40, peah-webrtc wrote: > > The comment should end with a period. > > > > Furthermore, did you consider my comment on expanding the abbreviation in > order > > to make the comment more descriptive? > > done, although I am not sure if I name it properly. I do not know what A_NLP > stands for exactly, so I tried to described it from its calculation. > > https://codereview.webrtc.org/1581183005/diff/120001/webrtc/modules/audio_pro... > webrtc/modules/audio_processing/aec/aec_core.cc:722: // ERLE > On 2016/04/20 13:57:40, peah-webrtc wrote: > > The comment should end with a period. > > > > Furthermore, did you consider my comment on expanding the abbreviation in > order > > to make the comment more descriptive? > > Done. Awesome! lgtm
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/1581183005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1581183005/140001
Message was sent while issue was closed.
Description was changed from ========== Cleaning up AEC metrics. Current implementation of AEC metrics does not read nicely. It messes up between a noise-removed calculation and a raw calculation. I tried to clean it up, in which, I stick to the raw calculation since the noise-removed version can show some problem when the noise is overestimated. BUG= ========== to ========== Cleaning up AEC metrics. Current implementation of AEC metrics does not read nicely. It messes up between a noise-removed calculation and a raw calculation. I tried to clean it up, in which, I stick to the raw calculation since the noise-removed version can show some problem when the noise is overestimated. BUG= ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Cleaning up AEC metrics. Current implementation of AEC metrics does not read nicely. It messes up between a noise-removed calculation and a raw calculation. I tried to clean it up, in which, I stick to the raw calculation since the noise-removed version can show some problem when the noise is overestimated. BUG= ========== to ========== Cleaning up AEC metrics. Current implementation of AEC metrics does not read nicely. It messes up between a noise-removed calculation and a raw calculation. I tried to clean it up, in which, I stick to the raw calculation since the noise-removed version can show some problem when the noise is overestimated. BUG= Committed: https://crrev.com/2b6707826e4c73662c4196f5c999fee3f13538ba Cr-Commit-Position: refs/heads/master@{#12455} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2b6707826e4c73662c4196f5c999fee3f13538ba Cr-Commit-Position: refs/heads/master@{#12455}
Message was sent while issue was closed.
On 2016/04/21 09:07:24, commit-bot: I haz the power wrote: > Patchset 7 (id:??) landed as > https://crrev.com/2b6707826e4c73662c4196f5c999fee3f13538ba > Cr-Commit-Position: refs/heads/master@{#12455} This CL was committed before I had reviewed the changes. I have some comments on both the log10(a/b) and on naming of variables.
Message was sent while issue was closed.
Comments after the fact. https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:373: RTC_CHECK(numerator >= 0); Why isn't numerator = 0 allowed? https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:377: const float log_denominator = log10(denominator + 1e-10f); Which is more computational complex, log10 or division? I'm thinking log10 is more expensive, but I'm not sure. If it is, we are wasting cycles here, just to get aroung the division with 0, right? https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:380: // Max. This comment doesn't add any value. Same with "Min." below. https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:395: // Upper mean. hicounter, hisum and himean doesn't follow the naming convention. Should be on the format hi_counter, but I'd like to see something more descriptive than "hi". https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.h (right): https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.h:51: size_t hicounter; None of the names are good, since "counter" is too general, and "hicounter" doesn't follow the naming format.
Message was sent while issue was closed.
On 2016/04/25 13:04:37, tlegrand-webrtc wrote: > On 2016/04/21 09:07:24, commit-bot: I haz the power wrote: > > Patchset 7 (id:??) landed as > > https://crrev.com/2b6707826e4c73662c4196f5c999fee3f13538ba > > Cr-Commit-Position: refs/heads/master@{#12455} > > This CL was committed before I had reviewed the changes. I have some comments on > both the log10(a/b) and on naming of variables. Oh, sorry, my bad. Per had similar comments and those were addressed. But I'd like to revert this CL or make another one if you have more comments.
Message was sent while issue was closed.
https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:373: RTC_CHECK(numerator >= 0); On 2016/04/25 13:09:56, tlegrand-webrtc wrote: > Why isn't numerator = 0 allowed? It is. But what I'm wondering is why we allow denominator == 0... (Also, for the future, please use RTC_DCHECK_GE, _GT, etc.---they print better error messages.) https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:377: const float log_denominator = log10(denominator + 1e-10f); On 2016/04/25 13:09:56, tlegrand-webrtc wrote: > Which is more computational complex, log10 or division? I'm thinking log10 is > more expensive, but I'm not sure. If it is, we are wasting cycles here, just to > get aroung the division with 0, right? Yes, I'd be surprised if division wasn't significantly faster than log10. See e.g. http://stackoverflow.com/questions/2858483/how-can-i-compare-the-performance-...
Message was sent while issue was closed.
Thanks for the comments. I wrote some answers. I think I can make a separate refactoring CL. https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:373: RTC_CHECK(numerator >= 0); On 2016/04/25 13:09:56, tlegrand-webrtc wrote: > Why isn't numerator = 0 allowed? It is allowed, isn't it? https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:377: const float log_denominator = log10(denominator + 1e-10f); On 2016/04/25 13:09:56, tlegrand-webrtc wrote: > Which is more computational complex, log10 or division? I'm thinking log10 is > more expensive, but I'm not sure. If it is, we are wasting cycles here, just to > get aroung the division with 0, right? Per also asked about this. My reasoning was that it is not only to avoid dividing by zero, but also to avoid overflow. Say if a/b results in infinity (b does not need to be zero), it does not mean that log(a/b) is infinity. So log a - log b has a better representability. https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:380: // Max. On 2016/04/25 13:09:56, tlegrand-webrtc wrote: > This comment doesn't add any value. Same with "Min." below. I agree. I'd like to make a separate CL, which may refactor hicounter etc. as well. https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:395: // Upper mean. On 2016/04/25 13:09:56, tlegrand-webrtc wrote: > hicounter, hisum and himean doesn't follow the naming convention. Should be on > the format hi_counter, but I'd like to see something more descriptive than "hi". True, this will need to refactor other part of AEC. We may better do it in a separate CL.
Message was sent while issue was closed.
Thanks Minyue! Fixing naming on variables can definitiely be done in a follow-up CL. If you think the log10(a)-log10(b) brings more value than log10(a/b), I'm fine with that too. https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec/aec_core.cc (right): https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:373: RTC_CHECK(numerator >= 0); On 2016/04/25 13:26:56, minyue-webrtc wrote: > On 2016/04/25 13:09:56, tlegrand-webrtc wrote: > > Why isn't numerator = 0 allowed? > > It is allowed, isn't it? Right, my mistake. I was thinking 0 wouldn't be allowed for the denominator and confused myself. But I realize now that 0 should be (and is) allowed for both. https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:377: const float log_denominator = log10(denominator + 1e-10f); On 2016/04/25 13:26:56, minyue-webrtc wrote: > On 2016/04/25 13:09:56, tlegrand-webrtc wrote: > > Which is more computational complex, log10 or division? I'm thinking log10 is > > more expensive, but I'm not sure. If it is, we are wasting cycles here, just > to > > get aroung the division with 0, right? > > Per also asked about this. My reasoning was that it is not only to avoid > dividing by zero, but also to avoid overflow. Say if a/b results in infinity (b > does not need to be zero), it does not mean that log(a/b) is infinity. So log a > - log b has a better representability. Acknowledged. https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:380: // Max. On 2016/04/25 13:26:56, minyue-webrtc wrote: > On 2016/04/25 13:09:56, tlegrand-webrtc wrote: > > This comment doesn't add any value. Same with "Min." below. > > I agree. I'd like to make a separate CL, which may refactor hicounter etc. as > well. Acknowledged. https://codereview.webrtc.org/1581183005/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec/aec_core.cc:395: // Upper mean. On 2016/04/25 13:26:56, minyue-webrtc wrote: > On 2016/04/25 13:09:56, tlegrand-webrtc wrote: > > hicounter, hisum and himean doesn't follow the naming convention. Should be on > > the format hi_counter, but I'd like to see something more descriptive than > "hi". > > True, this will need to refactor other part of AEC. We may better do it in a > separate CL. Acknowledged. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
