Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1242943008:
Remove C++11 calls from intelligibility_utils (Closed)
|
Created:
5 years, 5 months ago by ekm Modified:
5 years, 5 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove C++11 calls from intelligibility_utils
The C++11 here was overkill. This replaces it with simpler logic that
covers all cases encountered so far in practice.
The problem was previously brought up here: https://codereview.webrtc.org/1250663007/
BUG=427718, 487341, webrtc:4866
R=andrew@webrtc.org
Committed: https://crrev.com/3ab2f14d56db91665002f36f899c0a97f926d91f
Cr-Commit-Position: refs/heads/master@{#9627}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Got rid of static in zerofudge #
Total comments: 6
Patch Set 3 : Uncommented debug comment #Patch Set 4 : Fixed dither #
Total comments: 3
Patch Set 5 : Removed unnecesary abs #Patch Set 6 : Fixed merge conflict #
Messages
Total messages: 23 (7 generated)
ekmeyerson@webrtc.org changed reviewers: + jdduke@chromium.org
On 2015/07/22 22:04:29, ekm wrote: Much appreciated with the quick turnaround.
https://codereview.webrtc.org/1242943008/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1242943008/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:42: static int fudge_index = 0; Ahh, non-const static! Do we really need this zerofudge thing at all? There's a comment here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... that says "Can probably be removed." https://codereview.webrtc.org/1242943008/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:132: if (skip_fudge && false) { Can you remove these three lines?
https://codereview.webrtc.org/1242943008/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1242943008/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:42: static int fudge_index = 0; On 2015/07/22 22:17:07, andrew wrote: > Ahh, non-const static! > > Do we really need this zerofudge thing at all? There's a comment here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > that says "Can probably be removed." I think that's saying the |skip_fudge| flag can be removed. zerofudge does catch some div by 0 errors in other places (e.g., IntelligibilityEnhancer::SolverForGainsGivenLambda). There's surely a better way to deal with this, but we need to do something. I've changed it to use rand(). https://codereview.webrtc.org/1242943008/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:132: if (skip_fudge && false) { On 2015/07/22 22:17:07, andrew wrote: > Can you remove these three lines? Done.
https://codereview.webrtc.org/1242943008/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1242943008/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:42: static int fudge_index = 0; On 2015/07/22 22:45:25, ekm wrote: > On 2015/07/22 22:17:07, andrew wrote: > > Ahh, non-const static! > > > > Do we really need this zerofudge thing at all? There's a comment here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > that says "Can probably be removed." > > I think that's saying the |skip_fudge| flag can be removed. zerofudge does catch > some div by 0 errors in other places (e.g., > IntelligibilityEnhancer::SolverForGainsGivenLambda). There's surely a better way > to deal with this, but we need to do something. I've changed it to use rand(). Got it. Is there any benefit to adding this dither, or can we just ensure the components never fall below some small value? Something like this: float ClampToMinAbs(float value, float min_abs_value) { return std::abs(value) < min_abs_value ? min_abs_value : value; } complex<float> ZeroFudge(complex<float> c) { const float kMinValue = 0.001f; return complex<float>(ClampToMinAbs(c.real(), kMinValue), ClampToMinAbs(c.imag(), kMinValue)); } https://codereview.webrtc.org/1242943008/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1242943008/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:38: return c + std::complex<float>(float_rand(),float_rand()); This would overwrite a valid component in the case that only one of them is zero. https://codereview.webrtc.org/1242943008/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:137: //sample = zerofudge(sample); You didn't mean to leave this commented, right? Or can it be removed? https://codereview.webrtc.org/1242943008/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:149: // variance_[i] = decay_ * variance_[i] + (1.0f - decay_) * ( Can this be removed? Or did you intend to do something with it?
https://codereview.webrtc.org/1242943008/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1242943008/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:42: static int fudge_index = 0; On 2015/07/22 23:34:59, andrew wrote: > On 2015/07/22 22:45:25, ekm wrote: > > On 2015/07/22 22:17:07, andrew wrote: > > > Ahh, non-const static! > > > > > > Do we really need this zerofudge thing at all? There's a comment here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > > > that says "Can probably be removed." > > > > I think that's saying the |skip_fudge| flag can be removed. zerofudge does > catch > > some div by 0 errors in other places (e.g., > > IntelligibilityEnhancer::SolverForGainsGivenLambda). There's surely a better > way > > to deal with this, but we need to do something. I've changed it to use rand(). > > Got it. Is there any benefit to adding this dither, or can we just ensure the > components never fall below some small value? Something like this: > > float ClampToMinAbs(float value, float min_abs_value) { > return std::abs(value) < min_abs_value ? min_abs_value : value; > } > > complex<float> ZeroFudge(complex<float> c) { > const float kMinValue = 0.001f; > return complex<float>(ClampToMinAbs(c.real(), kMinValue), > ClampToMinAbs(c.imag(), kMinValue)); > } That's a good question. I think we want the dither, otherwise we could still end up with 0 variance if all samples are equal. https://codereview.webrtc.org/1242943008/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1242943008/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:38: return c + std::complex<float>(float_rand(),float_rand()); On 2015/07/22 23:35:00, andrew wrote: > This would overwrite a valid component in the case that only one of them is > zero. Fixed. Used your suggested template. https://codereview.webrtc.org/1242943008/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:137: //sample = zerofudge(sample); On 2015/07/22 23:35:00, andrew wrote: > You didn't mean to leave this commented, right? Or can it be removed? Done. https://codereview.webrtc.org/1242943008/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:149: // variance_[i] = decay_ * variance_[i] + (1.0f - decay_) * ( On 2015/07/22 23:34:59, andrew wrote: > Can this be removed? Or did you intend to do something with it? It's garbage. Removed.
https://codereview.webrtc.org/1242943008/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1242943008/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:36: return std::abs(value) == 0.f ? std::rand() * 0.01f / RAND_MAX : value; Drive-by nit: I believe -0.f is equivalent to 0.f in C++, so you can probably drop the std::abs here.
https://codereview.webrtc.org/1242943008/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1242943008/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:36: return std::abs(value) == 0.f ? std::rand() * 0.01f / RAND_MAX : value; On 2015/07/23 00:24:07, jdduke wrote: > Drive-by nit: I believe -0.f is equivalent to 0.f in C++, so you can probably > drop the std::abs here. Yep. That was left over from a cut&paste. Removed.
lgtm with jdduke's comment. https://codereview.webrtc.org/1242943008/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1242943008/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:36: return std::abs(value) == 0.f ? std::rand() * 0.01f / RAND_MAX : value; On 2015/07/23 00:24:07, jdduke wrote: > Drive-by nit: I believe -0.f is equivalent to 0.f in C++, so you can probably > drop the std::abs here. Agreed.
The CQ bit was checked by ekmeyerson@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1242943008/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1242943008/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/8705)
The CQ bit was checked by ekmeyerson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from andrew@webrtc.org Link to the patchset: https://codereview.webrtc.org/1242943008/#ps100001 (title: "Fixed merge conflict")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1242943008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1242943008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/6763)
The CQ bit was checked by ekmeyerson@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1242943008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1242943008/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3ab2f14d56db91665002f36f899c0a97f926d91f Cr-Commit-Position: refs/heads/master@{#9627} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
