Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 10 months ago by aluebs-webrtc Modified:
4 years, 10 months ago Reviewers:
hlundin-webrtc, turaj 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@ns Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUsing the NS noise estimate for the IE
Committed: https://crrev.com/c466badd86369e515b7d35ee1fac6d4b39bb5eb7
Cr-Commit-Position: refs/heads/master@{#11559}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Demote functions to local unnamed namespace #Patch Set 3 : Rebasing #
Total comments: 6
Patch Set 4 : Rebasing #Patch Set 5 : Move FilterVariance to its original position #Patch Set 6 : CHECK instead of if-guarding #
Total comments: 2
Created: 4 years, 10 months ago
Dependent Patchsets: Messages
Total messages: 16 (3 generated)
aluebs@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, turaj@webrtc.org
Nice improvements in general. Please, see comments. https://codereview.webrtc.org/1672343002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1672343002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:127: if (capture_filter_bank_.size() != bank_size_ || What kind of events lead up to capture_filter_bank_.size() != bank_size_? https://codereview.webrtc.org/1672343002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:347: void IntelligibilityEnhancer::FilterVariance( This method no longer uses any class members, right? You can demote it to a local function in an unnamed namespace. https://codereview.webrtc.org/1672343002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1672343002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:63: // Sets the capture noise estimate. "Noise estimate" is a bit generic. It is the noise spectrum, right?
https://codereview.webrtc.org/1672343002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1672343002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:127: if (capture_filter_bank_.size() != bank_size_ || On 2016/02/08 10:29:28, hlundin-webrtc wrote: > What kind of events lead up to capture_filter_bank_.size() != bank_size_? Because the input length of the noise vector is not known in construction time, the capture_filter_bank_ is not created in the constructor. So the first time this method is called, this condition guards against the break of evaluating the second condition thanks to the lazy evaluation. https://codereview.webrtc.org/1672343002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:347: void IntelligibilityEnhancer::FilterVariance( On 2016/02/08 10:29:28, hlundin-webrtc wrote: > This method no longer uses any class members, right? You can demote it to a > local function in an unnamed namespace. Done. https://codereview.webrtc.org/1672343002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1672343002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:63: // Sets the capture noise estimate. On 2016/02/08 10:29:28, hlundin-webrtc wrote: > "Noise estimate" is a bit generic. It is the noise spectrum, right? Right. Improved the comment.
https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:791: public_submodules_->noise_suppression->is_enabled()) { Can we have noise estimation part of NS to be auto-enabled when IE is enabled? Otherwise one has to know about details that how sub-modules depend on one another. https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:154: FilterVariance(&noise[0], Bastiaan's idea is one does not need to update IE gains very often, perhaps something like every 3-4 sec. If we still want to stick to that, does it make sense that IE has access to Noise Estimator and refresh its noise estimation on its own pace?
https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:791: public_submodules_->noise_suppression->is_enabled()) { On 2016/02/09 16:40:33, turaj wrote: > Can we have noise estimation part of NS to be auto-enabled when IE is enabled? > Otherwise one has to know about details that how sub-modules depend on one > another. I thought about pulling out the noise-estimation from the NS as a stand-alone component, but the truth is that this is a ton of work, mainly because it is old legacy C-code and is implemented radically different on the float and fixed versions. To give you an idea, the float version calculates the noise-estimate on the analysis stage, while the fixed version does it in the process stage. But I don't think it makes sense to have yet another separate noise-estimation (AEC has its own already), since the one in the NS gives a good estimate and runs in most cases anyway. I agree that these dependencies between components are sub-optimal, but we already have some, for example the TS depends on the AGC. I think it is safe to assume that a noisy environment that would benefit from IE would also do so from NS. The only solution that I see, without going into a huge refactoring project of the NS, is to enable the NS when the IE is enabled. WDYT? If you have another idea of how we could deal with this, please let me know. https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:154: FilterVariance(&noise[0], On 2016/02/09 16:40:33, turaj wrote: > Bastiaan's idea is one does not need to update IE gains very often, perhaps > something like every 3-4 sec. If we still want to stick to that, does it make > sense that IE has access to Noise Estimator and refresh its noise estimation on > its own pace? Good point! I moved the FilterVariance to where it was before, so this is now only a setter which squares the input for consistency. We might want to add some additional smoothing to this setter in some follow up CL. I will also look into what makes sense for updating the gains in the future.
https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:791: public_submodules_->noise_suppression->is_enabled()) { On 2016/02/09 19:13:35, aluebs-webrtc wrote: > On 2016/02/09 16:40:33, turaj wrote: > > Can we have noise estimation part of NS to be auto-enabled when IE is enabled? > > Otherwise one has to know about details that how sub-modules depend on one > > another. > > I thought about pulling out the noise-estimation from the NS as a stand-alone > component, but the truth is that this is a ton of work, mainly because it is old > legacy C-code and is implemented radically different on the float and fixed > versions. To give you an idea, the float version calculates the noise-estimate > on the analysis stage, while the fixed version does it in the process stage. But > I don't think it makes sense to have yet another separate noise-estimation (AEC > has its own already), since the one in the NS gives a good estimate and runs in > most cases anyway. > I agree that these dependencies between components are sub-optimal, but we > already have some, for example the TS depends on the AGC. I think it is safe to > assume that a noisy environment that would benefit from IE would also do so from > NS. > The only solution that I see, without going into a huge refactoring project of > the NS, is to enable the NS when the IE is enabled. WDYT? > If you have another idea of how we could deal with this, please let me know. You could add a (D)CHECK in a strategic position to verify that the relevant sub-components are enabled.
https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1672343002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:791: public_submodules_->noise_suppression->is_enabled()) { On 2016/02/09 19:25:03, hlundin-webrtc wrote: > On 2016/02/09 19:13:35, aluebs-webrtc wrote: > > On 2016/02/09 16:40:33, turaj wrote: > > > Can we have noise estimation part of NS to be auto-enabled when IE is > enabled? > > > Otherwise one has to know about details that how sub-modules depend on one > > > another. > > > > I thought about pulling out the noise-estimation from the NS as a stand-alone > > component, but the truth is that this is a ton of work, mainly because it is > old > > legacy C-code and is implemented radically different on the float and fixed > > versions. To give you an idea, the float version calculates the noise-estimate > > on the analysis stage, while the fixed version does it in the process stage. > But > > I don't think it makes sense to have yet another separate noise-estimation > (AEC > > has its own already), since the one in the NS gives a good estimate and runs > in > > most cases anyway. > > I agree that these dependencies between components are sub-optimal, but we > > already have some, for example the TS depends on the AGC. I think it is safe > to > > assume that a noisy environment that would benefit from IE would also do so > from > > NS. > > The only solution that I see, without going into a huge refactoring project of > > the NS, is to enable the NS when the IE is enabled. WDYT? > > If you have another idea of how we could deal with this, please let me know. > > You could add a (D)CHECK in a strategic position to verify that the relevant > sub-components are enabled. Done.
lgtm
lgtm https://codereview.webrtc.org/1672343002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1672343002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:53: void FilterVariance(const float* var, I wish this was called something else (I know you just copied it here). This, in fact, is a mapping rather than a filter, or maybe I'm missing something. Especially that we also have filtering of the norm of DFT coeffs over time (frames) to compute average power.
https://codereview.webrtc.org/1672343002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1672343002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:53: void FilterVariance(const float* var, On 2016/02/10 15:30:40, turaj wrote: > I wish this was called something else (I know you just copied it here). This, in > fact, is a mapping rather than a filter, or maybe I'm missing something. > Especially that we also have filtering of the norm of DFT coeffs over time > (frames) to compute average power. This method is going to be renamed in my next CL anyways, so I will do that there, if you agree.
The CQ bit was checked by aluebs@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1672343002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1672343002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Using the NS noise estimate for the IE ========== to ========== Using the NS noise estimate for the IE Committed: https://crrev.com/c466badd86369e515b7d35ee1fac6d4b39bb5eb7 Cr-Commit-Position: refs/heads/master@{#11559} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c466badd86369e515b7d35ee1fac6d4b39bb5eb7 Cr-Commit-Position: refs/heads/master@{#11559} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
