Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2254973003:
Added functionality for specifying the initial signal level to use for the gain estimation in the l… (Closed)
|
Created:
4 years, 4 months ago by peah-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, henrika_webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionThis CL adds functionality in the level controller to
receive a signal level to use initially, instead of the
default initial signal level.
BUG=
Committed: https://crrev.com/57fec1d828113241186e78710ec5e851cc1a0e81
Cr-Commit-Position: refs/heads/master@{#13931}
Patch Set 1 #Patch Set 2 : Merge from upstream CL #
Total comments: 8
Patch Set 3 : Changes in response to reviewer comments #
Total comments: 2
Patch Set 4 : Removing the usage of the API for setting the levelcontroller initial level until clarity has been … #
Total comments: 2
Patch Set 5 : Added range check of the supplied level #
Total comments: 12
Patch Set 6 : Extended the use of the minimum level constant #Patch Set 7 : Changed the level to be specified in dBFS #
Total comments: 2
Patch Set 8 : Rebase #Messages
Total messages: 41 (14 generated)
Description was changed from ========== Added functionality for specifying the initial signal level to use for the gain estimation in the level controller BUG= ========== to ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. BUG= ==========
peah@webrtc.org changed reviewers: + aleloi@webrtc.org, henrik.lundin@webrtc.org
LGTM! https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/gain_selector.cc (right): https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/gain_selector.cc:50: jump_start_gain) { Is this to avoid having to wait for 100 iterations until a gain is applied? https://codereview.webrtc.org/2254973003/diff/20001/webrtc/voice_engine/voe_a... File webrtc/voice_engine/voe_audio_processing_impl.h (right): https://codereview.webrtc.org/2254973003/diff/20001/webrtc/voice_engine/voe_a... webrtc/voice_engine/voe_audio_processing_impl.h:23: Later, I think we could take another look at how the initial level enters the APM. I was wondering whether the VoE redesign has any consequences for voe_audio_processing_impl.
https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/gain_selector.cc (right): https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/gain_selector.cc:50: jump_start_gain) { On 2016/08/18 11:43:39, aleloi wrote: > Is this to avoid having to wait for 100 iterations until a gain is applied? Yes, that is correct. https://codereview.webrtc.org/2254973003/diff/20001/webrtc/voice_engine/voe_a... File webrtc/voice_engine/voe_audio_processing_impl.h (right): https://codereview.webrtc.org/2254973003/diff/20001/webrtc/voice_engine/voe_a... webrtc/voice_engine/voe_audio_processing_impl.h:23: On 2016/08/18 11:43:39, aleloi wrote: > Later, I think we could take another look at how the initial level enters the > APM. I was wondering whether the VoE redesign has any consequences for > voe_audio_processing_impl. That definitely makes sense!
lgtm with nits. https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller.h (right): https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller.h:75: bool gain_jumpstart_ = false; Naming: the method parameter is called jump_start_gain, while this is not. Consider aligning the naming. https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:32: void PeakLevelEstimator::SetInitialLevel(float level) { The method should be called SetInitialPeakLevel, reflecting the name of the member variable.
https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/level_controller.h (right): https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/level_controller.h:75: bool gain_jumpstart_ = false; On 2016/08/18 12:58:12, hlundin-webrtc wrote: > Naming: the method parameter is called jump_start_gain, while this is not. > Consider aligning the naming. Good point! Done. https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2254973003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:32: void PeakLevelEstimator::SetInitialLevel(float level) { On 2016/08/18 12:58:13, hlundin-webrtc wrote: > The method should be called SetInitialPeakLevel, reflecting the name of the > member variable. Good find! Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, aleloi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2254973003/#ps40001 (title: "Changes in response to reviewer comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7537)
peah@webrtc.org changed reviewers: + henrika@webrtc.org, mflodman@webrtc.org
+mflodman@ as an OWNER of webrtc/media/ +henrika@ as an OWNER of webrtc/voice_engine/
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/2254973003/diff/40001/webrtc/voice_engine/voe_a... File webrtc/voice_engine/voe_audio_processing_impl.h (right): https://codereview.webrtc.org/2254973003/diff/40001/webrtc/voice_engine/voe_a... webrtc/voice_engine/voe_audio_processing_impl.h:22: void SetLevelControllerInitialLevel(float level); Can we avoid adding more to the legacy API please? If you need to set the level in e.g. WebRtcVoiceEngine/MediaChannel, it is possible to get at the APM* directly there.
https://codereview.webrtc.org/2254973003/diff/40001/webrtc/voice_engine/voe_a... File webrtc/voice_engine/voe_audio_processing_impl.h (right): https://codereview.webrtc.org/2254973003/diff/40001/webrtc/voice_engine/voe_a... webrtc/voice_engine/voe_audio_processing_impl.h:22: void SetLevelControllerInitialLevel(float level); On 2016/08/22 08:00:17, the sun wrote: > Can we avoid adding more to the legacy API please? > > If you need to set the level in e.g. WebRtcVoiceEngine/MediaChannel, it is > possible to get at the APM* directly there. Great! Is that preferred over going via voi_audio_processing_impl.cc? To me this looks like a wrapper layer over audio_processing_impl and I thought it would be confusing if I bypassed it.
On 2016/08/22 08:04:23, peah-webrtc wrote: > https://codereview.webrtc.org/2254973003/diff/40001/webrtc/voice_engine/voe_a... > File webrtc/voice_engine/voe_audio_processing_impl.h (right): > > https://codereview.webrtc.org/2254973003/diff/40001/webrtc/voice_engine/voe_a... > webrtc/voice_engine/voe_audio_processing_impl.h:22: void > SetLevelControllerInitialLevel(float level); > On 2016/08/22 08:00:17, the sun wrote: > > Can we avoid adding more to the legacy API please? > > > > If you need to set the level in e.g. WebRtcVoiceEngine/MediaChannel, it is > > possible to get at the APM* directly there. > > Great! Is that preferred over going via voi_audio_processing_impl.cc? > > To me this looks like a wrapper layer over audio_processing_impl and I thought > it would be confusing if I bypassed it. We're working to get rid of the voe_* APIs. They're legacy. So yes, the other method is preferred.
Since there are (potentially) several possible places where the API call for setting the initial level could be used, this CL is now restricted to only opening up the APM API for this.
-henrik@ and -mflodman@ as OWNERship is now covered by solenberg@, as as no reviews have not yet been received from you. (Please re-add yourselves if you still want to review this CL)
peah@webrtc.org changed reviewers: - henrika@webrtc.org, mflodman@webrtc.org
henrika@webrtc.org changed reviewers: + henrika@webrtc.org
LGTM https://codereview.webrtc.org/2254973003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2254973003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:423: // to compute the signal gain. Any specific range supported?
On 2016/08/22 11:36:14, henrika_webrtc wrote: > LGTM > > https://codereview.webrtc.org/2254973003/diff/60001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/include/audio_processing.h (right): > > https://codereview.webrtc.org/2254973003/diff/60001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/include/audio_processing.h:423: // to compute > the signal gain. > Any specific range supported? Excellent point! I created a new patch where I added a comment about that. I also added DCHECKS and limitation to ensure that a level that is outside of the range is caught in debug builds and does neither cause bad level controller behavior.
https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:40: RTC_DCHECK_GE(32768.f, level); Why not use the constants here? https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:43: initial_peak_level_ = std::min(std::max(level, kMinLevel), kMaxLevel); Do you really need to DCHECK as well as clamp? https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:50: RTC_DCHECK_LE(30.f, peak_level_); Should this be kMinLevel?
I added a new patch where the hardcoded minimum level was replaced by the new constant for this. https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:40: RTC_DCHECK_GE(32768.f, level); On 2016/08/22 11:58:22, the sun wrote: > Why not use the constants here? That is a good question: The thinking was like this: -I don't want to complicate that APM API by imposing a complicated level requirement, so therefore I basically chose the full range. -If the internal range, e.g., currently 30.0f-32768.0f would change (for reasons of adjusting the performance, it is nice not to have to change the API because of that for that. -If I DCHECK on the internal range, that would cause valid input data (according to the API description) to cause DCHECKS, which does not make sense. https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:43: initial_peak_level_ = std::min(std::max(level, kMinLevel), kMaxLevel); On 2016/08/22 11:58:22, the sun wrote: > Do you really need to DCHECK as well as clamp? That is a good question. Please see above response for the answer.
I added a new patch where the hardcoded minimum level was replaced by the new constant for this.
https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:40: RTC_DCHECK_GE(32768.f, level); On 2016/08/22 12:09:04, peah-webrtc wrote: > On 2016/08/22 11:58:22, the sun wrote: > > Why not use the constants here? > > That is a good question: > > The thinking was like this: > -I don't want to complicate that APM API by imposing a complicated level > requirement, so therefore I basically chose the full range. Ok that makes sense, but when I think about it, 0 dBFS in is usually 1.0 in the floating point world, so from that perspective, the limits here appear a bit random. > -If the internal range, e.g., currently 30.0f-32768.0f would change (for reasons > of adjusting the performance, it is nice not to have to change the API because > of that for that. I think you need to explain that to me in person. :) > -If I DCHECK on the internal range, that would cause valid input data (according > to the API description) to cause DCHECKS, which does not make sense. Agree. https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:43: initial_peak_level_ = std::min(std::max(level, kMinLevel), kMaxLevel); On 2016/08/22 12:09:04, peah-webrtc wrote: > On 2016/08/22 11:58:22, the sun wrote: > > Do you really need to DCHECK as well as clamp? > > That is a good question. Please see above response for the answer. Right. I think generally APIs should aggressively assert the specified contract, as you say. In this case you should only need the std::max() to limit the range though.
Patchset #7 (id:120001) has been deleted
https://codereview.webrtc.org/2254973003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2254973003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:423: // to compute the signal gain. On 2016/08/22 11:36:14, henrika_webrtc wrote: > Any specific range supported? Yes, excellent point! I added that now. https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:40: RTC_DCHECK_GE(32768.f, level); On 2016/08/22 14:18:39, the sun wrote: > On 2016/08/22 12:09:04, peah-webrtc wrote: > > On 2016/08/22 11:58:22, the sun wrote: > > > Why not use the constants here? > > > > That is a good question: > > > > The thinking was like this: > > -I don't want to complicate that APM API by imposing a complicated level > > requirement, so therefore I basically chose the full range. > > Ok that makes sense, but when I think about it, 0 dBFS in is usually 1.0 in the > floating point world, so from that perspective, the limits here appear a bit > random. > > > -If the internal range, e.g., currently 30.0f-32768.0f would change (for > reasons > > of adjusting the performance, it is nice not to have to change the API because > > of that for that. > > I think you need to explain that to me in person. :) > > > -If I DCHECK on the internal range, that would cause valid input data > (according > > to the API description) to cause DCHECKS, which does not make sense. > > Agree. > You are right! Excellent point. I changed the API to instead receive the level in dBFS with a specified range of [-100, 0]. What do you think of that solution? https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:43: initial_peak_level_ = std::min(std::max(level, kMinLevel), kMaxLevel); On 2016/08/22 14:18:39, the sun wrote: > On 2016/08/22 12:09:04, peah-webrtc wrote: > > On 2016/08/22 11:58:22, the sun wrote: > > > Do you really need to DCHECK as well as clamp? > > > > That is a good question. Please see above response for the answer. > > Right. I think generally APIs should aggressively assert the specified contract, > as you say. In this case you should only need the std::max() to limit the range > though. True, I removed the std::min. Done. https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:50: RTC_DCHECK_LE(30.f, peak_level_); On 2016/08/22 11:58:22, the sun wrote: > Should this be kMinLevel? Done.
lgtm https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:40: RTC_DCHECK_GE(32768.f, level); On 2016/08/23 05:33:44, peah-webrtc wrote: > On 2016/08/22 14:18:39, the sun wrote: > > On 2016/08/22 12:09:04, peah-webrtc wrote: > > > On 2016/08/22 11:58:22, the sun wrote: > > > > Why not use the constants here? > > > > > > That is a good question: > > > > > > The thinking was like this: > > > -I don't want to complicate that APM API by imposing a complicated level > > > requirement, so therefore I basically chose the full range. > > > > Ok that makes sense, but when I think about it, 0 dBFS in is usually 1.0 in > the > > floating point world, so from that perspective, the limits here appear a bit > > random. > > > > > -If the internal range, e.g., currently 30.0f-32768.0f would change (for > > reasons > > > of adjusting the performance, it is nice not to have to change the API > because > > > of that for that. > > > > I think you need to explain that to me in person. :) > > > > > -If I DCHECK on the internal range, that would cause valid input data > > (according > > > to the API description) to cause DCHECKS, which does not make sense. > > > > Agree. > > > > You are right! Excellent point. > > I changed the API to instead receive the level in dBFS with a specified range of > [-100, 0]. What do you think of that solution? Nice, I would have gone for range 0-1.0 myself, but this is even better. Unless, of course, if this method is called often, which I guess it wouldn't be? https://codereview.webrtc.org/2254973003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2254973003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:58: peak_level_ += 0.1f * (frame_peak_level - peak_level_); Sanity check: this calculation means that peak_level_ never contains the actual peak level. The filtered average will lag behind the true dynamics of the signal. But I guess that is the intention?
https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2254973003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:40: RTC_DCHECK_GE(32768.f, level); On 2016/08/23 06:23:38, the sun wrote: > On 2016/08/23 05:33:44, peah-webrtc wrote: > > On 2016/08/22 14:18:39, the sun wrote: > > > On 2016/08/22 12:09:04, peah-webrtc wrote: > > > > On 2016/08/22 11:58:22, the sun wrote: > > > > > Why not use the constants here? > > > > > > > > That is a good question: > > > > > > > > The thinking was like this: > > > > -I don't want to complicate that APM API by imposing a complicated level > > > > requirement, so therefore I basically chose the full range. > > > > > > Ok that makes sense, but when I think about it, 0 dBFS in is usually 1.0 in > > the > > > floating point world, so from that perspective, the limits here appear a bit > > > random. > > > > > > > -If the internal range, e.g., currently 30.0f-32768.0f would change (for > > > reasons > > > > of adjusting the performance, it is nice not to have to change the API > > because > > > > of that for that. > > > > > > I think you need to explain that to me in person. :) > > > > > > > -If I DCHECK on the internal range, that would cause valid input data > > > (according > > > > to the API description) to cause DCHECKS, which does not make sense. > > > > > > Agree. > > > > > > > You are right! Excellent point. > > > > I changed the API to instead receive the level in dBFS with a specified range > of > > [-100, 0]. What do you think of that solution? > > Nice, I would have gone for range 0-1.0 myself, but this is even better. Unless, > of course, if this method is called often, which I guess it wouldn't be? That is correct, this is not something that will happen often. https://codereview.webrtc.org/2254973003/diff/140001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc (right): https://codereview.webrtc.org/2254973003/diff/140001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc:58: peak_level_ += 0.1f * (frame_peak_level - peak_level_); On 2016/08/23 06:23:38, the sun wrote: > Sanity check: this calculation means that peak_level_ never contains the actual > peak level. The filtered average will lag behind the true dynamics of the > signal. But I guess that is the intention? Yes, that is the intention. It should be more of a long-term value than the instantaneous value.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, aleloi@webrtc.org, henrika@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2254973003/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. BUG= ========== to ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. BUG= ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. BUG= ========== to ========== This CL adds functionality in the level controller to receive a signal level to use initially, instead of the default initial signal level. BUG= Committed: https://crrev.com/57fec1d828113241186e78710ec5e851cc1a0e81 Cr-Commit-Position: refs/heads/master@{#13931} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/57fec1d828113241186e78710ec5e851cc1a0e81 Cr-Commit-Position: refs/heads/master@{#13931}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:160001) has been created in https://codereview.webrtc.org/2283793002/ by peah@webrtc.org. The reason for reverting is: This caused build breakage due to upstream dependencies. These dependencies need to be resolved before landing the CL.. |
