Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(150)

Issue 2995043002: AGC2 dummy module: fixed gain param, APM integration, audioproc_f adaptation (Closed)

Created:
8 months, 1 week ago by AleBzk
Modified:
6 months, 3 weeks ago
Reviewers:
aleloi, peah-webrtc, peah
CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, Andrew MacDonald, aleloi, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

AGC2 dummy module: fixed gain param, APM integration, audioproc_f adaptation In preparation of coming CLs that will add an AGC interface to make the gain controller injectable. This CL simplifies AGC2 (dummy sub-module of audioproc_f) since it only implements the fixed digital mode with hard-clipping - i.e., no limiter is used. The AGC2 config now includes the fixed gain to apply and audioproc_f has been adapted accordingly. Finally, this CL slightly simplifies the AGC2 integration into APM. BUG=webrtc:7494

Patch Set 1 #

Total comments: 7

Patch Set 2 : UT fix #

Total comments: 27

Patch Set 3 : comments addressed #

Total comments: 25
Unified diffs Side-by-side diffs Delta from patch set Stats (+139 lines, -179 lines) Patch
M webrtc/modules/audio_processing/BUILD.gn View 1 2 1 chunk +0 lines, -2 lines 0 comments Download
D webrtc/modules/audio_processing/agc2/digital_gain_applier.h View 1 2 1 chunk +0 lines, -32 lines 0 comments Download
D webrtc/modules/audio_processing/agc2/digital_gain_applier.cc View 1 chunk +0 lines, -38 lines 0 comments Download
M webrtc/modules/audio_processing/agc2/gain_controller2.h View 1 2 2 chunks +8 lines, -8 lines 4 comments Download
M webrtc/modules/audio_processing/agc2/gain_controller2.cc View 1 2 1 chunk +45 lines, -23 lines 12 comments Download
M webrtc/modules/audio_processing/agc2/gain_controller2_unittest.cc View 1 2 3 chunks +50 lines, -54 lines 0 comments Download
M webrtc/modules/audio_processing/audio_processing_impl.h View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M webrtc/modules/audio_processing/audio_processing_impl.cc View 1 2 8 chunks +19 lines, -21 lines 7 comments Download
M webrtc/modules/audio_processing/include/audio_processing.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/modules/audio_processing/test/audio_processing_simulator.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/audio_processing/test/audio_processing_simulator.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/audio_processing/test/audioproc_float.cc View 1 2 5 chunks +13 lines, -0 lines 2 comments Download

Messages

Total messages: 12 (4 generated)
AleBzk
Hi, I made some changes in preparation for the CL that will add the AGC ...
8 months, 1 week ago (2017-08-15 12:44:59 UTC) #3
aleloi
Could you please add a test to check that the settings are applied? E.g. start ...
8 months, 1 week ago (2017-08-15 14:34:03 UTC) #4
peah-webrtc
Hi, Thanks for the CL. I've added some comments https://codereview.webrtc.org/2995043002/diff/1/webrtc/modules/audio_processing/agc2/gain_controller2.cc File webrtc/modules/audio_processing/agc2/gain_controller2.cc (right): https://codereview.webrtc.org/2995043002/diff/1/webrtc/modules/audio_processing/agc2/gain_controller2.cc#newcode27 webrtc/modules/audio_processing/agc2/gain_controller2.cc:27: ...
8 months ago (2017-08-18 04:51:06 UTC) #6
aleloi
https://codereview.webrtc.org/2995043002/diff/1/webrtc/modules/audio_processing/audio_processing_impl.cc File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2995043002/diff/1/webrtc/modules/audio_processing/audio_processing_impl.cc#newcode1886 webrtc/modules/audio_processing/audio_processing_impl.cc:1886: // TODO(alessiob): If GainController2 was already enabled and the ...
8 months ago (2017-08-18 08:28:32 UTC) #7
peah-webrtc
https://codereview.webrtc.org/2995043002/diff/1/webrtc/modules/audio_processing/include/audio_processing.h File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2995043002/diff/1/webrtc/modules/audio_processing/include/audio_processing.h#newcode280 webrtc/modules/audio_processing/include/audio_processing.h:280: float fixed_gain_db = 5.f; On 2017/08/18 08:28:31, aleloi wrote: ...
8 months ago (2017-08-18 09:12:46 UTC) #8
AleBzk
Hi, Thanks for your comments. I addressed them and thoroughly tested the AGC2 integration in ...
7 months, 1 week ago (2017-09-14 09:21:56 UTC) #10
peah-webrtc
Thanks for the patch! I have some comments. PTAL https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_processing/agc2/gain_controller2.cc File webrtc/modules/audio_processing/agc2/gain_controller2.cc (right): https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_processing/agc2/gain_controller2.cc#newcode26 webrtc/modules/audio_processing/agc2/gain_controller2.cc:26: ...
7 months ago (2017-09-15 07:44:26 UTC) #11
AleBzk
6 months, 3 weeks ago (2017-09-29 09:39:06 UTC) #12
Thanks for your comments Per.

I moved this CL to Gerrit, the new link is
https://webrtc-review.googlesource.com/c/src/+/4661

I merged and addressed all your comments.

Alessio

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/agc2/gain_controller2.cc (right):

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc2/gain_controller2.cc:26: : data_dumper_(new
ApmDataDumper(instance_count_)),
On 2017/09/15 07:44:25, peah-webrtc wrote:
> You need to increase instance_count_ for each new instance, otherwise the
> datafiles won't be unique for each instance.
> 
> Note that that increment needs to be done in an atomic manner to avoid race
> conditions:
> new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))), 

Done, but note that I took LC as reference, in which no atomic increment is used
(see
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_process...).
I think it's the same case for AGC2.

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc2/gain_controller2.cc:27: sample_rate_hz_(0),
On 2017/09/15 07:44:25, peah-webrtc wrote:
> Please initialize the sample rate to a valid rate.

Done.

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc2/gain_controller2.cc:29: ++instance_count_;
On 2017/09/15 07:44:25, peah-webrtc wrote:
> Please remove this increase and instead do it as above.

Done.

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc2/gain_controller2.cc:40:
data_dumper_->DumpRaw("fixed gain (linear)", fixed_gain_);
On 2017/09/15 07:44:25, peah-webrtc wrote:
> Does filenames with spaces and parenthesis always work?
> My suggestion would be to anyway avoid that, as you'll need to escape those in
> terminals to get them to work. 

Done.

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc2/gain_controller2.cc:53: if
(audio->channels_f()[k][j] == -32768.f ||
On 2017/09/15 07:44:25, peah-webrtc wrote:
> This is only used for the purpose of the DumpRaw command below, right?
> 
> I don't think it makes sense to do this just for the purpose of debugging.
This
> will incur a nonnegligible cost even during runtime, which I don't think makes
> sense for this.
> 
> The alternative would be to put that code under build flags, but if possible
> that should be avoided I think as it clutters the code.

Done.

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc2/gain_controller2.cc:68: fixed_gain_ =
(config.fixed_gain_db == 0.f)
On 2017/09/15 07:44:25, peah-webrtc wrote:
> The parentheses are not needed here.

Done.

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/agc2/gain_controller2.h (right):

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc2/gain_controller2.h:28: // It temporarily
implements a fixed gain mode with hard-clipping.
On 2017/09/15 07:44:25, peah-webrtc wrote:
> It temporarily -> Temporarily

Done.

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc2/gain_controller2.h:49: static int
instance_count_;
On 2017/09/15 07:44:25, peah-webrtc wrote:
> Please move this declaration to before data_dumper as that one will use
> instance_count_.

Done.

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:376: // controller is
enabled.
On 2017/09/15 07:44:25, peah-webrtc wrote:
> On 2017/09/14 09:21:56, AleBzk wrote:
> > @Per: I replicated the TODO from LC, but the code below will change when we
> will
> > have AGC injection.
> 
> Acknowledged.

Done.

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/audio_processing_impl.cc:683: if
(!GainController2::Validate(config_.gain_controller2)) {
On 2017/09/15 07:44:25, peah-webrtc wrote:
> Even though this works, I think it makes sense to follow the same pattern as
> above, and not change this from how it was before.
> 
> I'm not sure of the background to why it is like that, but I think it is to
> simplify debugging using a debugger.

Done.

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/test/audioproc_float.cc (right):

https://codereview.webrtc.org/2995043002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/audioproc_float.cc:193: if (value !=
kParameterNotSpecifiedValue) {
On 2017/09/15 07:44:25, peah-webrtc wrote:
> I'm not 100 % sure this comparison can be done for doubles. Have you checked
> that it works?

Right. I removed this function, since it was only used for agc2_fixed_gain_db,
which is 0 by default. Plus, I made agc2_fixed_gain_db non optional and set to 0
by default.

Powered by Google App Engine
This is Rietveld 408576698