Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1718793002:
Fix the gain calculation in IntelligibilityEnhancer (Closed)
|
Created:
4 years, 10 months ago by aluebs-webrtc Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, 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@vad Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix the gain calculation in IntelligibilityEnhancer
Committed: https://crrev.com/f99af6b885747d407d20c6788bc9dabdd88a4079
Cr-Commit-Position: refs/heads/master@{#11755}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Calculate zero distance only once #Patch Set 3 : Simplify quadratic logic using max #Patch Set 4 : Rebasing and Formatting #Patch Set 5 : Make windows happy #
Messages
Total messages: 20 (7 generated)
aluebs@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, turaj@webrtc.org
https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:303: float gamma0 = 0.5f * kRho * pow_x0[n] * pow_n0[n] + I like local consts... https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:307: if (beta0 * beta0 < 4.f * alpha0 * gamma0) { You are essentially calculating beta0 * beta0 - 4.f * alpha0 * gamma0 twice. Why not store it? const float some_good_name = beta0 * beta0 - 4.f * alpha0 * gamma0; if (some_good_name < 0) { ... } else { sols[n] = ... sqrt(some_good_name) ... https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:308: sols[n] = -beta0 / (2.f * alpha0); This is not the same as the old code, right? https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:310: sols[n] = (-beta0 - sqrtf(beta0 * beta0 - 4.f * alpha0 * gamma0)) / No need for regularization any longer?
https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:308: sols[n] = -beta0 / (2.f * alpha0); My interpretation of the paper Eq 18 is that the quadratic equation always have real roots. How -beta/(2 * alpha) is derived? The old code considers the non-quadratic case for rho=1 which results in \alpha to be zero. Perhpas I'm missing some point of the paper.
https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:303: float gamma0 = 0.5f * kRho * pow_x0[n] * pow_n0[n] + On 2016/02/22 12:59:22, hlundin-webrtc wrote: > I like local consts... I like them as well, but apparently I have a hard time remembering them :) Added the const. https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:307: if (beta0 * beta0 < 4.f * alpha0 * gamma0) { On 2016/02/22 12:59:22, hlundin-webrtc wrote: > You are essentially calculating beta0 * beta0 - 4.f * alpha0 * gamma0 twice. Why > not store it? > const float some_good_name = beta0 * beta0 - 4.f * alpha0 * gamma0; > if (some_good_name < 0) { > ... > } else { > sols[n] = ... sqrt(some_good_name) ... Good point, done. Although I am not creative enough to find a good name. I called it zero distance, although the actual distance is after taking the square root and dividing by 2a. Any better ideas? https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:308: sols[n] = -beta0 / (2.f * alpha0); On 2016/02/22 16:05:20, turaj wrote: > My interpretation of the paper Eq 18 is that the quadratic equation always have > real roots. How -beta/(2 * alpha) is derived? The old code considers the > non-quadratic case for rho=1 which results in \alpha to be zero. Perhpas I'm > missing some point of the paper. I agree that the quadratic equation always has real roots, but here I try to guard against numerical errors. So when this value is slightly negative I just assume it is zero, therefore -b/2a. Does that make sense? https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:310: sols[n] = (-beta0 - sqrtf(beta0 * beta0 - 4.f * alpha0 * gamma0)) / On 2016/02/22 12:59:22, hlundin-webrtc wrote: > No need for regularization any longer? No, because now I check for a minimum power in line 300, which (knowing 0<kRho<1 and lambda<0) ensures alpha0>0. I only added this regularization for the zero-division error, but I found afterwards that epsilon was probably too-large for this, since it changed the results.
LGTM with a suggestion. https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:307: if (beta0 * beta0 < 4.f * alpha0 * gamma0) { On 2016/02/22 23:56:11, aluebs-webrtc wrote: > On 2016/02/22 12:59:22, hlundin-webrtc wrote: > > You are essentially calculating beta0 * beta0 - 4.f * alpha0 * gamma0 twice. > Why > > not store it? > > const float some_good_name = beta0 * beta0 - 4.f * alpha0 * gamma0; > > if (some_good_name < 0) { > > ... > > } else { > > sols[n] = ... sqrt(some_good_name) ... > > Good point, done. Although I am not creative enough to find a good name. I > called it zero distance, although the actual distance is after taking the square > root and dividing by 2a. Any better ideas? I don't know the algorithm good enough to suggest anything better. But if the distance is obtained after sqrt, then maybe squared_distance is an option? I'll leave it up to you; I'm fine with zero_distance, too. https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:310: sols[n] = (-beta0 - sqrtf(beta0 * beta0 - 4.f * alpha0 * gamma0)) / On 2016/02/22 23:56:10, aluebs-webrtc wrote: > On 2016/02/22 12:59:22, hlundin-webrtc wrote: > > No need for regularization any longer? > > No, because now I check for a minimum power in line 300, which (knowing 0<kRho<1 > and lambda<0) ensures alpha0>0. I only added this regularization for the > zero-division error, but I found afterwards that epsilon was probably too-large > for this, since it changed the results. You may want to add a DCHECK to document/verify your assumptions.
https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:307: if (beta0 * beta0 < 4.f * alpha0 * gamma0) { On 2016/02/24 09:51:00, hlundin-webrtc wrote: > On 2016/02/22 23:56:11, aluebs-webrtc wrote: > > On 2016/02/22 12:59:22, hlundin-webrtc wrote: > > > You are essentially calculating beta0 * beta0 - 4.f * alpha0 * gamma0 twice. > > Why > > > not store it? > > > const float some_good_name = beta0 * beta0 - 4.f * alpha0 * gamma0; > > > if (some_good_name < 0) { > > > ... > > > } else { > > > sols[n] = ... sqrt(some_good_name) ... > > > > Good point, done. Although I am not creative enough to find a good name. I > > called it zero distance, although the actual distance is after taking the > square > > root and dividing by 2a. Any better ideas? > > I don't know the algorithm good enough to suggest anything better. But if the > distance is obtained after sqrt, then maybe squared_distance is an option? I'll > leave it up to you; I'm fine with zero_distance, too. If you consider my suggestion of using max(0, b^2 - 4*a*c) there will be no need for extra variable. https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:308: sols[n] = -beta0 / (2.f * alpha0); On 2016/02/22 23:56:10, aluebs-webrtc wrote: > On 2016/02/22 16:05:20, turaj wrote: > > My interpretation of the paper Eq 18 is that the quadratic equation always > have > > real roots. How -beta/(2 * alpha) is derived? The old code considers the > > non-quadratic case for rho=1 which results in \alpha to be zero. Perhpas I'm > > missing some point of the paper. > > I agree that the quadratic equation always has real roots, but here I try to > guard against numerical errors. So when this value is slightly negative I just > assume it is zero, therefore -b/2a. Does that make sense? Thanks for the explanation, it makes total sense, if I may suggest, it might be easier to see this if you write sols[n] = (-beta0 - sqrt(std::max(0, beta0^2 - 4 * alpha0 * gamma0)) with a comment as you explained.
I forgot, LGTM
https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:307: if (beta0 * beta0 < 4.f * alpha0 * gamma0) { On 2016/02/24 15:26:42, turaj wrote: > On 2016/02/24 09:51:00, hlundin-webrtc wrote: > > On 2016/02/22 23:56:11, aluebs-webrtc wrote: > > > On 2016/02/22 12:59:22, hlundin-webrtc wrote: > > > > You are essentially calculating beta0 * beta0 - 4.f * alpha0 * gamma0 > twice. > > > Why > > > > not store it? > > > > const float some_good_name = beta0 * beta0 - 4.f * alpha0 * gamma0; > > > > if (some_good_name < 0) { > > > > ... > > > > } else { > > > > sols[n] = ... sqrt(some_good_name) ... > > > > > > Good point, done. Although I am not creative enough to find a good name. I > > > called it zero distance, although the actual distance is after taking the > > square > > > root and dividing by 2a. Any better ideas? > > > > I don't know the algorithm good enough to suggest anything better. But if the > > distance is obtained after sqrt, then maybe squared_distance is an option? > I'll > > leave it up to you; I'm fine with zero_distance, too. > > If you consider my suggestion of using max(0, b^2 - 4*a*c) there will be no need > for extra variable. No name is best name :) https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:308: sols[n] = -beta0 / (2.f * alpha0); On 2016/02/24 15:26:42, turaj wrote: > On 2016/02/22 23:56:10, aluebs-webrtc wrote: > > On 2016/02/22 16:05:20, turaj wrote: > > > My interpretation of the paper Eq 18 is that the quadratic equation always > > have > > > real roots. How -beta/(2 * alpha) is derived? The old code considers the > > > non-quadratic case for rho=1 which results in \alpha to be zero. Perhpas I'm > > > missing some point of the paper. > > > > I agree that the quadratic equation always has real roots, but here I try to > > guard against numerical errors. So when this value is slightly negative I just > > assume it is zero, therefore -b/2a. Does that make sense? > > Thanks for the explanation, it makes total sense, if I may suggest, it might be > easier to see this if you write > > sols[n] = (-beta0 - sqrt(std::max(0, beta0^2 - 4 * alpha0 * gamma0)) > > with a comment as you explained. That is a great point, done. https://codereview.webrtc.org/1718793002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:310: sols[n] = (-beta0 - sqrtf(beta0 * beta0 - 4.f * alpha0 * gamma0)) / On 2016/02/24 09:51:00, hlundin-webrtc wrote: > On 2016/02/22 23:56:10, aluebs-webrtc wrote: > > On 2016/02/22 12:59:22, hlundin-webrtc wrote: > > > No need for regularization any longer? > > > > No, because now I check for a minimum power in line 300, which (knowing > 0<kRho<1 > > and lambda<0) ensures alpha0>0. I only added this regularization for the > > zero-division error, but I found afterwards that epsilon was probably > too-large > > for this, since it changed the results. > > You may want to add a DCHECK to document/verify your assumptions. Done.
The CQ bit was checked by aluebs@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, turaj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1718793002/#ps60001 (title: "Rebasing and Formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1718793002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1718793002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_drmemory_light on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
The CQ bit was checked by aluebs@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, turaj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1718793002/#ps80001 (title: "Make windows happy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1718793002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1718793002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix the gain calculation in IntelligibilityEnhancer ========== to ========== Fix the gain calculation in IntelligibilityEnhancer Committed: https://crrev.com/f99af6b885747d407d20c6788bc9dabdd88a4079 Cr-Commit-Position: refs/heads/master@{#11755} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f99af6b885747d407d20c6788bc9dabdd88a4079 Cr-Commit-Position: refs/heads/master@{#11755} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
