Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1925933002:
Limit the maximum and minimum gain the IntelligibilityEnhancer can apply (Closed)
|
Created:
4 years, 7 months ago by aluebs-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, minyue-webrtc, the sun, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionLimit the maximum and minimum gain the IntelligibilityEnhancer can apply
Although the algorithm should generate the optimal gains, these limits are in place so that it doesn't go bananas in extreme noise situations or if there are some errors on the noise estimate. The limits were chosen as the extreme values from recordings with -3dB SNR, which is the minimum where the POLQA metric increases when processed.
Committed: https://crrev.com/4896aaa9e4e7d4e1322b301406e64777268965b9
Cr-Commit-Position: refs/heads/master@{#12550}
Patch Set 1 #
Total comments: 6
Messages
Total messages: 21 (7 generated)
aluebs@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, peah@webrtc.org, turaj@webrtc.org
LGTM with a nit/question. https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:17: #include <limits> Is this still needed?
LGTM https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:17: #include <limits> On 2016/04/28 06:57:14, hlundin-webrtc wrote: > Is this still needed? for std::numeric_limits<float>::epsilon(), I guess.
peah, could you PTAL? https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:17: #include <limits> On 2016/04/28 17:00:21, turaj wrote: > On 2016/04/28 06:57:14, hlundin-webrtc wrote: > > Is this still needed? > > for std::numeric_limits<float>::epsilon(), I guess. Exactly!
On 2016/04/28 18:10:45, aluebs-webrtc wrote: > peah, could you PTAL? > > https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc > (right): > > https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:17: > #include <limits> > On 2016/04/28 17:00:21, turaj wrote: > > On 2016/04/28 06:57:14, hlundin-webrtc wrote: > > > Is this still needed? > > > > for std::numeric_limits<float>::epsilon(), I guess. > > Exactly! lgtm for the code change. (For the algorithm, I cannot tell without more description about the change).
https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:26: const float kMaxFactor = 1000.f; It is a bit weird to have a limit on the max and min which is asymmetrical, i.e. for a maximum of 0.01 I would have expected a minimum of 0.001.
https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:17: #include <limits> On 2016/04/28 18:10:45, aluebs-webrtc wrote: > On 2016/04/28 17:00:21, turaj wrote: > > On 2016/04/28 06:57:14, hlundin-webrtc wrote: > > > Is this still needed? > > > > for std::numeric_limits<float>::epsilon(), I guess. > > Exactly! Acknowledged. My bad.
Description was changed from ========== Limit the maximum and minimum gain the IntelligibilityEnhancer can apply ========== to ========== Limit the maximum and minimum gain the IntelligibilityEnhancer can apply Although the algorithm should generate the optimal gains, these limits are in place so that it doesn't go bananas in extreme noise situations or if there are some errors on the noise estimate. The limits were chosen as the extreme values from recordings with -3dB SNR, which is the minimum where the POLQA metric increases when processed. ==========
On 2016/04/28 18:42:58, peah-webrtc wrote: > On 2016/04/28 18:10:45, aluebs-webrtc wrote: > > peah, could you PTAL? > > > > > https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... > > File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc > > (right): > > > > > https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... > > webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:17: > > #include <limits> > > On 2016/04/28 17:00:21, turaj wrote: > > > On 2016/04/28 06:57:14, hlundin-webrtc wrote: > > > > Is this still needed? > > > > > > for std::numeric_limits<float>::epsilon(), I guess. > > > > Exactly! > > > lgtm for the code change. > (For the algorithm, I cannot tell without more description about the change). Expanded the description.
https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc (right): https://codereview.webrtc.org/1925933002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:26: const float kMaxFactor = 1000.f; On 2016/04/28 18:43:24, peah-webrtc wrote: > It is a bit weird to have a limit on the max and min which is asymmetrical, i.e. > for a maximum of 0.01 I would have expected a minimum of 0.001. I don't see why they should be symmetric.
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/1925933002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925933002/1
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
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/1925933002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925933002/1
Message was sent while issue was closed.
Description was changed from ========== Limit the maximum and minimum gain the IntelligibilityEnhancer can apply Although the algorithm should generate the optimal gains, these limits are in place so that it doesn't go bananas in extreme noise situations or if there are some errors on the noise estimate. The limits were chosen as the extreme values from recordings with -3dB SNR, which is the minimum where the POLQA metric increases when processed. ========== to ========== Limit the maximum and minimum gain the IntelligibilityEnhancer can apply Although the algorithm should generate the optimal gains, these limits are in place so that it doesn't go bananas in extreme noise situations or if there are some errors on the noise estimate. The limits were chosen as the extreme values from recordings with -3dB SNR, which is the minimum where the POLQA metric increases when processed. ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Limit the maximum and minimum gain the IntelligibilityEnhancer can apply Although the algorithm should generate the optimal gains, these limits are in place so that it doesn't go bananas in extreme noise situations or if there are some errors on the noise estimate. The limits were chosen as the extreme values from recordings with -3dB SNR, which is the minimum where the POLQA metric increases when processed. ========== to ========== Limit the maximum and minimum gain the IntelligibilityEnhancer can apply Although the algorithm should generate the optimal gains, these limits are in place so that it doesn't go bananas in extreme noise situations or if there are some errors on the noise estimate. The limits were chosen as the extreme values from recordings with -3dB SNR, which is the minimum where the POLQA metric increases when processed. Committed: https://crrev.com/4896aaa9e4e7d4e1322b301406e64777268965b9 Cr-Commit-Position: refs/heads/master@{#12550} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4896aaa9e4e7d4e1322b301406e64777268965b9 Cr-Commit-Position: refs/heads/master@{#12550} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
