Issue 2090583002: New module for the adaptive level controlling functionality in the audio processing module

Issue 2090583002: New module for the adaptive level controlling functionality in the audio processing module (Closed)

Created:
4 years, 6 months ago by peah-webrtc

Modified:
4 years, 4 months ago

Reviewers:
hlundin-webrtc, kjellander_webrtc, aleloi, aluebs-webrtc, tlegrand-webrtc

CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), kwiberg-webrtc, Andrew MacDonald, hlundin-webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, minyue-webrtc, the sun, aluebs-webrtc, bjornv1, ehmaldonado_webrtc

Base URL:
https://chromium.googlesource.com/external/webrtc.git@master

Target Ref:
refs/pending/heads/master

Project:
webrtc

Visibility:
Public.

More Reviews

Description

New module for the adaptive level controlling functionality in the audio processing module NOTRY=true TBR=aluebs@webrtc.org BUG=webrtc:5920 Committed: https://crrev.com/ca4cac7e7494e7b5d60bda4b21b6a00592f8e1df Cr-Commit-Position: refs/heads/master@{#13333}

Patch Set 1 #

Patch Set 2 : Adding missing include #

Patch Set 3 : Changed downsampler coefficients #

Patch Set 4 : Corrected the level controller integration #

Patch Set 5 : Added complexity benchmarking performance unittest #

Patch Set 6 : Corrected the initial behavior for the peak level estimate, and ensured a nonzero minimum peak leve… #

Total comments: 184

Patch Set 7 : Functionality updates to improve robustness in noise-only scenarios #

Patch Set 8 : Partially applied the changes in response to reviewer comments #

Patch Set 9 : Changes in response to reviewer comments #

Patch Set 10 : Added missing checks for APM API call return values #

Patch Set 11 : Corrected an assignment error #

Total comments: 41

Patch Set 12 : Changes in response to reviewer comments #

Total comments: 4

Patch Set 13 : Added API method to obtain the latest applied gain #

Patch Set 14 : Added reporting of metrics #

Total comments: 8

Patch Set 15 : Added log-line that reports whether the level controller is explicitly activated/deactivated #

Patch Set 16 : Changes in response to reviewer comments #

Patch Set 17 : Corrected a wrong placement of the addition of the level controller experiment description string i… #

Patch Set 18 : Activated bitexactness tests with preliminary bitexactness test vectors #

Patch Set 19 : Corrected type for the average peak level metric #

Patch Set 20 : Adding missing initialization of the noise spectrum #

Patch Set 21 : Added proper initialization of the dc_level estimator #

Patch Set 22 : Corrected test vector for Android #

Patch Set 23 : Removed comma #

Patch Set 24 : Temporarily deactivated the level controller until the CL with the proper tuning has been landed #

Total comments: 4

Created: 4 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+2113 lines, -12 lines)			Patch
M	webrtc/common.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22	1 chunk	+2 lines, -1 line	0 comments	Download
M	webrtc/modules/audio_processing/BUILD.gn	View		1 chunk	+21 lines, -0 lines	0 comments	Download
M	webrtc/modules/audio_processing/audio_processing.gypi	View		1 chunk	+21 lines, -0 lines	0 comments	Download
M	webrtc/modules/audio_processing/audio_processing_impl.h	View		3 chunks	+6 lines, -2 lines	0 comments	Download
M	webrtc/modules/audio_processing/audio_processing_impl.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23	10 chunks	+34 lines, -3 lines	0 comments	Download
M	webrtc/modules/audio_processing/audio_processing_tests.gypi	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	webrtc/modules/audio_processing/include/audio_processing.h	View		1 chunk	+8 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/biquad_filter.h	View	1 2 3 4 5 6 7 8	1 chunk	+58 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/biquad_filter.cc	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+35 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/down_sampler.h	View	1 2 3 4 5 6 7 8	1 chunk	+40 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/down_sampler.cc	View	1 2 3 4 5 6 7 8	1 chunk	+101 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/gain_applier.h	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+40 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/gain_applier.cc	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+143 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/gain_selector.h	View	1 2 3 4 5 6 7	1 chunk	+39 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/gain_selector.cc	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+85 lines, -0 lines	0 comments	Download
A +	webrtc/modules/audio_processing/level_controller/lc_constants.h	View		1 chunk	+7 lines, -4 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/level_controller.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18	1 chunk	+80 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/level_controller.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+230 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+345 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21	1 chunk	+122 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/noise_level_estimator.h	View	1 2 3 4 5 6 7 8	1 chunk	+37 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/noise_level_estimator.cc	View	1 2 3 4 5 6 7 8	1 chunk	+72 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/noise_spectrum_estimator.h	View	1 2 3 4 5 6 7 8	1 chunk	+40 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/noise_spectrum_estimator.cc	View	1 2 3 4 5 6 7 8	1 chunk	+68 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/peak_level_estimator.h	View	1 2 3 4 5 6 7 8	1 chunk	+36 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/peak_level_estimator.cc	View	1 2 3 4 5 6 7 8	1 chunk	+58 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/saturating_gain_estimator.h	View	1 2 3 4 5 6 7 8	1 chunk	+37 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/saturating_gain_estimator.cc	View		1 chunk	+48 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/signal_classifier.h	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+65 lines, -0 lines	0 comments	Download
A	webrtc/modules/audio_processing/level_controller/signal_classifier.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	1 chunk	+166 lines, -0 lines	0 comments	Download
M	webrtc/modules/audio_processing/logging/apm_data_dumper.h	View		2 chunks	+25 lines, -0 lines	0 comments	Download
M	webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	webrtc/modules/audio_processing/test/audio_processing_simulator.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	webrtc/modules/audio_processing/test/audio_processing_simulator.cc	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	webrtc/modules/audio_processing/test/audioproc_float.cc	View		2 chunks	+4 lines, -0 lines	0 comments	Download
M	webrtc/modules/audio_processing/test/debug_dump_test.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13	1 chunk	+24 lines, -0 lines	0 comments	Download
M	webrtc/modules/audio_processing/test/process_test.cc	View	1 2 3 4 5 6 7 8	2 chunks	+4 lines, -0 lines	0 comments	Download
M	webrtc/modules/modules.gyp	View	1 2 3 4	1 chunk	+1 line, -2 lines	0 comments	Download
M	webrtc/webrtc_tests.gypi	View	1 2 3 4	1 chunk	+1 line, -0 lines	4 comments	Download

Dependent Patchsets:

Issue 2111553002 Patch 60001

Issue 2111553002 Patch 80001

Issue 2111553002 Patch 100001

Issue 2111553002 Patch 120001

Issue 2111553002 Patch 140001

Messages

Total messages: 64 (24 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

peah-webrtc

Hi, The level controller functionality is now reviewable. Please have a look! Best, Per

4 years, 6 months ago (2016-06-22 13:20:53 UTC) #2

peah-webrtc

On 2016/06/22 13:20:53, peah-webrtc wrote: > Hi, > The level controller functionality is now reviewable. ...

4 years, 6 months ago (2016-06-23 07:18:27 UTC) #3

tlegrand-webrtc

On 2016/06/23 11:46:42, peah-webrtc wrote: > +tina.legrand@ as an owner of webrtc/common.h LGTM for webrtc/common.h.

4 years, 6 months ago (2016-06-23 13:32:20 UTC) #6

hlundin-webrtc

Very nice work! I have a few (I lied...) comments. https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_processing/level_controller/biquad_filter.cc File webrtc/modules/audio_processing/level_controller/biquad_filter.cc (right): https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_processing/level_controller/biquad_filter.cc#newcode20 ...

4 years, 5 months ago (2016-06-27 11:21:17 UTC) #7

peah-webrtc

Thanks for the careful review and all the great comments! I have addressed them in ...

4 years, 5 months ago (2016-06-27 22:51:50 UTC) #8

aleloi

This looks very nice! I have learned a lot while reading. Here are some comments ...

4 years, 5 months ago (2016-06-28 09:35:52 UTC) #9

aleloi

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_processing/level_controller/down_sampler.cc File webrtc/modules/audio_processing/level_controller/down_sampler.cc (right): https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_processing/level_controller/down_sampler.cc#newcode71 webrtc/modules/audio_processing/level_controller/down_sampler.cc:71: data_dumper_->DumpWav("lc_down_sampler_input", in, sample_rate_hz_, 1); On 2016/06/28 09:35:51, aleloi wrote: ...

4 years, 5 months ago (2016-06-28 09:40:01 UTC) #10

hlundin-webrtc

Wow. Much improve. Very better. https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_processing/level_controller/down_sampler.h File webrtc/modules/audio_processing/level_controller/down_sampler.h (right): https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_processing/level_controller/down_sampler.h#newcode27 webrtc/modules/audio_processing/level_controller/down_sampler.h:27: ~DownSampler(); On 2016/06/27 22:51:47, ...

4 years, 5 months ago (2016-06-28 11:29:01 UTC) #11

Wow. Much improve. Very better.

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/down_sampler.h (right):

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/down_sampler.h:27:
~DownSampler();
On 2016/06/27 22:51:47, peah-webrtc wrote:
> On 2016/06/27 11:21:14, hlundin-webrtc wrote:
> > Do you need this? And if so, consider using = defualt inline.
> 
> I think it is needed as the ApmDataDumper and BiQuadFilter types are only
> specified using forwards. If the default destructor is used, the compilation
> fails as the compiler needs to know the sizes of the object when building
> down_sampler.h and therefore cannot use forward declarations for the member
> types.
> 
> At least this is what I think happens, and I cannot get the compile to work
when
> using default destructor implementations with only forward declarations of the
> above types.
> 
> Does that make sense, or am I have I missed something in that?

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/gain_selector.cc (right):

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_selector.cc:44:
RTC_DCHECK_LT(0.f, peak_level);
On 2016/06/27 22:51:47, peah-webrtc wrote:
> On 2016/06/27 11:21:14, hlundin-webrtc wrote:
> > This will fail if peak_level == 0.f. Is this intended?
> 
> Yes, the peak level detector should ensure a minimum peak level that is
greated
> than 0.
> 
> By relying on that, it is not needed to check so that the division by
peak_level
> does not involve a division by 0.

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc
(right):

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc:293:
AudioProcessing::kSampleRate32kHz, AudioProcessing::kSampleRate48kHz};
On 2016/06/27 22:51:49, peah-webrtc wrote:
> On 2016/06/27 11:21:15, hlundin-webrtc wrote:
> > No 44100 Hz here?
> 
> No. The submodule only supports native rates (8, 16, 32 and 48 kHz). When the
> submodule is run inside APM, APM resamples the signal to a native rate before
> the submodule is called so therefore it is possible to use 44.1 kHz as an
input
> to the test where the level controller is run via APM.

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/signal_classifier.cc
(right):

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/signal_classifier.cc:95: class
FrameExtender {
On 2016/06/27 22:51:50, peah-webrtc wrote:
> On 2016/06/27 11:21:17, hlundin-webrtc wrote:
> > Why is this outside of the unnamed namespace?
> 
> I think it needs to be outside of that for the reason that it is a member of
the
> state.

Oh, I see. Then I think I would prefer to have it as a private subclass inside
the SignalClassifier.

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/signal_classifier.cc:132: //
Compute the signal power spectrum.
On 2016/06/27 22:51:50, peah-webrtc wrote:
> On 2016/06/27 11:21:17, hlundin-webrtc wrote:
> > You may want to DCHECK some properties of the AudioBuffer, such as sample
rate
> > and/or number of samples.
> 
> That would make sense, but I think AudioBuffer does not provide any
> functionality to access the sample rate. And apart from that there is nothing
in
> this method (apart from the downsampler) which relies on the frame length so I
> cannot find any good candidates to DCHECK on. WDYT?

Can't you just check that the number of samples in the audio frame adds up as
expected with the assumed sample rate?

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/gain_selector.cc (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_selector.cc:56: if
(highly_nonstationary_signal_hold_counter_ > 0) {
Please, explain this logic in a comment.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/level_controller.h
(right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller.h:24: #include
"webrtc/modules/audio_processing/level_controller/gain_selector.h"
Order is wrong; gain_selector.h should go higher up.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc
(right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc:115:
timestamps_us_.resize(kNumFramesToProcess);
You must use reserve, not resize. reserve allocates memory, but the vector is
still empty, while resize also populates it with default-initialized elements.
This will not work at all now that you are using push_back below.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc
(right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:72:
TEST(LevelControlBitExactnessTest, Mono8kHz_DISABLED) {
Should be DISABLED_Mono8kHz, otherwise the test will still run.

peah-webrtc

Thanks for the great comments! I have uploaded a patch with the changes done in ...

4 years, 5 months ago (2016-06-28 22:19:38 UTC) #12

Thanks for the great comments!
I have uploaded a patch with the changes done in response to the comments.

Best,
Per

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/down_sampler.h (right):

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/down_sampler.h:27:
~DownSampler();
On 2016/06/28 11:29:00, hlundin-webrtc wrote:
> On 2016/06/27 22:51:47, peah-webrtc wrote:
> > On 2016/06/27 11:21:14, hlundin-webrtc wrote:
> > > Do you need this? And if so, consider using = defualt inline.
> > 
> > I think it is needed as the ApmDataDumper and BiQuadFilter types are only
> > specified using forwards. If the default destructor is used, the compilation
> > fails as the compiler needs to know the sizes of the object when building
> > down_sampler.h and therefore cannot use forward declarations for the member
> > types.
> > 
> > At least this is what I think happens, and I cannot get the compile to work
> when
> > using default destructor implementations with only forward declarations of
the
> > above types.
> > 
> > Does that make sense, or am I have I missed something in that?
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/gain_selector.cc (right):

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_selector.cc:44:
RTC_DCHECK_LT(0.f, peak_level);
On 2016/06/28 11:29:00, hlundin-webrtc wrote:
> On 2016/06/27 22:51:47, peah-webrtc wrote:
> > On 2016/06/27 11:21:14, hlundin-webrtc wrote:
> > > This will fail if peak_level == 0.f. Is this intended?
> > 
> > Yes, the peak level detector should ensure a minimum peak level that is
> greated
> > than 0.
> > 
> > By relying on that, it is not needed to check so that the division by
> peak_level
> > does not involve a division by 0.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc
(right):

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc:293:
AudioProcessing::kSampleRate32kHz, AudioProcessing::kSampleRate48kHz};
On 2016/06/28 11:29:00, hlundin-webrtc wrote:
> On 2016/06/27 22:51:49, peah-webrtc wrote:
> > On 2016/06/27 11:21:15, hlundin-webrtc wrote:
> > > No 44100 Hz here?
> > 
> > No. The submodule only supports native rates (8, 16, 32 and 48 kHz). When
the
> > submodule is run inside APM, APM resamples the signal to a native rate
before
> > the submodule is called so therefore it is possible to use 44.1 kHz as an
> input
> > to the test where the level controller is run via APM.
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/signal_classifier.cc
(right):

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/signal_classifier.cc:95: class
FrameExtender {
On 2016/06/28 11:29:01, hlundin-webrtc wrote:
> On 2016/06/27 22:51:50, peah-webrtc wrote:
> > On 2016/06/27 11:21:17, hlundin-webrtc wrote:
> > > Why is this outside of the unnamed namespace?
> > 
> > I think it needs to be outside of that for the reason that it is a member of
> the
> > state.
> 
> Oh, I see. Then I think I would prefer to have it as a private subclass inside
> the SignalClassifier.

Done.

https://codereview.webrtc.org/2090583002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/signal_classifier.cc:132: //
Compute the signal power spectrum.
On 2016/06/28 11:29:01, hlundin-webrtc wrote:
> On 2016/06/27 22:51:50, peah-webrtc wrote:
> > On 2016/06/27 11:21:17, hlundin-webrtc wrote:
> > > You may want to DCHECK some properties of the AudioBuffer, such as sample
> rate
> > > and/or number of samples.
> > 
> > That would make sense, but I think AudioBuffer does not provide any
> > functionality to access the sample rate. And apart from that there is
nothing
> in
> > this method (apart from the downsampler) which relies on the frame length so
I
> > cannot find any good candidates to DCHECK on. WDYT?
> 
> Can't you just check that the number of samples in the audio frame adds up as
> expected with the assumed sample rate?

True!
Good point!
Done.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.cc:35: #include
"webrtc/modules/audio_processing/level_controller/level_controller.h"
On 2016/06/28 09:35:51, aleloi wrote:
> level_controller.h is included twice.

Nice find! Thanks!!!

Done.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/biquad_filter.cc (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/biquad_filter.cc:15: // This
method applies an biquad filter to an input signal x to produce an
On 2016/06/28 09:35:51, aleloi wrote:
> an -> a

Done.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/down_sampler.cc (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/down_sampler.cc:55:
sample_rate_hz_ = sample_rate_hz;
On 2016/06/28 09:35:51, aleloi wrote:
> This line looks fishy. It's an assignment of the parameter sample_rate_hz to
> itself because of scoping rules, right?

No, I think you probably missed the underscore in the variable name to the left
of the equal sign. This statement sets the member variable sample_rate_hz_ to
the value of sample_rate_hz.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/down_sampler.cc:71:
data_dumper_->DumpWav("lc_down_sampler_input", in, sample_rate_hz_, 1);
On 2016/06/28 09:35:51, aleloi wrote:
> Is saving the input/output to file always the intended functionality?

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/down_sampler.cc:71:
data_dumper_->DumpWav("lc_down_sampler_input", in, sample_rate_hz_, 1);
On 2016/06/28 09:40:01, aleloi wrote:
> On 2016/06/28 09:35:51, aleloi wrote:
> > Is saving the input/output to file always the intended functionality?
> 
> Just noticed that dumping only happens when WEBRTC_AEC_DEBUG_DUMP == 1.
> Disregard previous comment!

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/down_sampler.cc:84:
low_pass_filter_.Process(in, rtc::ArrayView<float>(x, in.size()));
On 2016/06/28 09:35:51, aleloi wrote:
> Just to check that I understood correctly: we have a custom simple filter and
> resampling algorithm for performance reasons? Instead of using one from webrtc
> that is already implemented.

You definitely have a good point! It would perhaps make sense to reuse one of
the other resamplers. 
I still think it may make sense to use this resampler regardless of that as the
current resampler is tailormade to use a cutoff filter with a quite low cutoff
frequency which allows the bandlimiting filter to be likely much cheaper than
what is the case for the other more general resamplers. Furthermore, having now
looked at the existing resampling code, I think that looks more expensive. Still
it would make sense to look into that.

WDYT, is it ok to keep it as it is with a custom made downsampler for now?

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/down_sampler.h (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/down_sampler.h:32: int
down_sampling_factor_;
On 2016/06/28 09:35:52, aleloi wrote:
> Could the variables (including data_dumper_) be made const? It should be
> possible to both check that 8000 divides sample_rate_hz and make it const at
the
> same time.

The other values cannot be const as they are changed in the Initialize method.
It would be nice to have them const, but with the current design it is not
possible to do that.

data_dumper_ could be made a const pointer, but not pointer to a const object.
I'll change that to be a const pointer.

WDYT? Does that sound ok?

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/gain_applier.cc (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_applier.cc:28: return
std::count_if(in.begin(), in.end(), [](float v) {
On 2016/06/28 09:35:52, aleloi wrote:
> Probably the compiler is smart and this is not affecting performance in any
way,
> but the array has const floats while the lambda takes floats. If it were an
> array of complex objects, I think copying would happen. What is the difference
> to '[](const float & v)' ?

I'm not an expert on this, but I think it should be fine. As you say, for
complex objects there would be copies made, but a float should fit into the
registers and should be very cheap to handle.

But I guess it cannot increase the complexity to make it a const reference, and
as you point out, that solution is consistent in that the array consists of
const floats, so I'll change it to that.

Done.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_applier.cc:63: gain =
std::min(new_gain, gain + step_size);
On 2016/06/28 09:35:52, aleloi wrote:
> Maybe move the gain calculation outside of the loop for improved readability?
Or
> call ApplyConstantGain after calculating it. Same for the function below.

I'm not fully sure of what you mean, but the intention is to  adjust the gain
towards the new gain in a smooth manner to avoid discontinuities resulting in
audible distortions. In order to do that, the updating of the gain needs to be
inside the loop. Setting a new gain before the loop and applying the gain to the
whole frame could potentially cause an audible distortion.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_applier.cc:114:
gain_change_step_size_ = 0.001f * (48000.f / sample_rate_hz);
On 2016/06/28 09:35:52, aleloi wrote:
> Please define a constant for 0.001f.

That definitely makes sense.

Done.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_applier.cc:115: }
On 2016/06/28 09:35:52, aleloi wrote:
> Do we need both a constructor and an Initialize() method? With just a
> constructor, the object is never in a half-initialized state, and it's
possible
> to document that gain_change_step_size_ doesn't change. Are there any benefits
> with two methods?

I definitely agree that it would be nicer to do the initialization only at
construction time. However, as the audio processing module (APM) is constructed,
the sample rate and number of channels is specified by the incoming audio stream
and therefore not known when APM is constructed. Furthermore, the audio stream
properties sample rate and number of channels can, and do, change over time.
Each time these change, a reinitialization of the APM is needed and (I guess) to
let that reinitialization have a minimal impact on the performance most of the
submodules are reinitialized without being reconstructed.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/gain_applier.h (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_applier.h:25: int
Process(float new_gain, AudioBuffer* audio);
On 2016/06/28 09:40:01, aleloi wrote:
> Please document that Process counts saturations.

Good point!
Done.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_applier.h:28:
ApmDataDumper* data_dumper_;
On 2016/06/28 09:35:52, aleloi wrote:
> Since data_dumper_ is set only once in the constructor, it could be a const
> pointer. Am I right?

Good point! That is correct! 
Done.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/gain_selector.cc (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_selector.cc:56: if
(highly_nonstationary_signal_hold_counter_ > 0) {
On 2016/06/28 11:29:01, hlundin-webrtc wrote:
> Please, explain this logic in a comment.

Done.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/level_controller.h
(right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller.h:24: #include
"webrtc/modules/audio_processing/level_controller/gain_selector.h"
On 2016/06/28 11:29:01, hlundin-webrtc wrote:
> Order is wrong; gain_selector.h should go higher up.

Done.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc
(right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller_complexity_unittest.cc:115:
timestamps_us_.resize(kNumFramesToProcess);
On 2016/06/28 11:29:01, hlundin-webrtc wrote:
> You must use reserve, not resize. reserve allocates memory, but the vector is
> still empty, while resize also populates it with default-initialized elements.
> This will not work at all now that you are using push_back below.

Fully true! Thanks!

Done.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc
(right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller_unittest.cc:72:
TEST(LevelControlBitExactnessTest, Mono8kHz_DISABLED) {
On 2016/06/28 11:29:01, hlundin-webrtc wrote:
> Should be DISABLED_Mono8kHz, otherwise the test will still run.

Done.

peah-webrtc

Thanks for the great comments!!! I have uploaded a patch with the changes done in ...

4 years, 5 months ago (2016-06-28 22:21:17 UTC) #13

aluebs-webrtc

drive-by https://codereview.webrtc.org/2090583002/diff/220001/webrtc/modules/audio_processing/audio_processing_impl.cc File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2090583002/diff/220001/webrtc/modules/audio_processing/audio_processing_impl.cc#newcode775 webrtc/modules/audio_processing/audio_processing_impl.cc:775: private_submodules_->level_controller->Process(ca); I am not sure how this component ...

4 years, 5 months ago (2016-06-28 22:45:02 UTC) #15

peah-webrtc

https://codereview.webrtc.org/2090583002/diff/220001/webrtc/modules/audio_processing/audio_processing_impl.cc File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2090583002/diff/220001/webrtc/modules/audio_processing/audio_processing_impl.cc#newcode775 webrtc/modules/audio_processing/audio_processing_impl.cc:775: private_submodules_->level_controller->Process(ca); On 2016/06/28 22:45:02, aluebs-webrtc wrote: > I am ...

4 years, 5 months ago (2016-06-29 06:22:09 UTC) #16

hlundin-webrtc

A few nits, then I'm happy. https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_processing/level_controller/down_sampler.cc File webrtc/modules/audio_processing/level_controller/down_sampler.cc (right): https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_processing/level_controller/down_sampler.cc#newcode84 webrtc/modules/audio_processing/level_controller/down_sampler.cc:84: low_pass_filter_.Process(in, rtc::ArrayView<float>(x, in.size())); ...

4 years, 5 months ago (2016-06-29 08:56:28 UTC) #18

A few nits, then I'm happy.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/down_sampler.cc (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/down_sampler.cc:84:
low_pass_filter_.Process(in, rtc::ArrayView<float>(x, in.size()));
On 2016/06/28 22:19:37, peah-webrtc wrote:
> On 2016/06/28 09:35:51, aleloi wrote:
> > Just to check that I understood correctly: we have a custom simple filter
and
> > resampling algorithm for performance reasons? Instead of using one from
webrtc
> > that is already implemented.
> 
> You definitely have a good point! It would perhaps make sense to reuse one of
> the other resamplers. 
> I still think it may make sense to use this resampler regardless of that as
the
> current resampler is tailormade to use a cutoff filter with a quite low cutoff
> frequency which allows the bandlimiting filter to be likely much cheaper than
> what is the case for the other more general resamplers. Furthermore, having
now
> looked at the existing resampling code, I think that looks more expensive.
Still
> it would make sense to look into that.
> 
> WDYT, is it ok to keep it as it is with a custom made downsampler for now?

I think we should keep this for now, since time is scarce and you've done
testing with the current implementation. But in general I do agree that we
should try to consolidate things like general filter implementations.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/gain_applier.cc (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_applier.cc:28: return
std::count_if(in.begin(), in.end(), [](float v) {
On 2016/06/28 22:19:37, peah-webrtc wrote:
> On 2016/06/28 09:35:52, aleloi wrote:
> > Probably the compiler is smart and this is not affecting performance in any
> way,
> > but the array has const floats while the lambda takes floats. If it were an
> > array of complex objects, I think copying would happen. What is the
difference
> > to '[](const float & v)' ?
> 
> I'm not an expert on this, but I think it should be fine. As you say, for
> complex objects there would be copies made, but a float should fit into the
> registers and should be very cheap to handle.
> 
> But I guess it cannot increase the complexity to make it a const reference,
and
> as you point out, that solution is consistent in that the array consists of
> const floats, so I'll change it to that.
> 
> Done.

I think that in this case it makes no difference. Passing a float or a reference
should be just as efficient. For the record, the declaration of count_if does
indeed allow const ref as well as "by value".

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/level_controller.cc
(right):

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller.cc:85: void
LevelController::Metrics::ResetEstimation() {
Just call this method Reset.

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller.cc:109:
"WebRTC.Audio.LevelControlMaxNoisePower",
I think you should increase the readability of the histogram names by adding a
'.' after LeveLControl. In this case "WebRTC.Audio.LevelControl.MaxNoisePower"
and similar below.

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller.cc:110:
static_cast<int>(10 *
This was an awkward line wrap. Did clang format produce this?

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/level_controller.h
(right):

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller.h:51: int
metrics_frame_counter_;
A counter should probably be size_t.

peah-webrtc

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_processing/level_controller/down_sampler.cc File webrtc/modules/audio_processing/level_controller/down_sampler.cc (right): https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_processing/level_controller/down_sampler.cc#newcode84 webrtc/modules/audio_processing/level_controller/down_sampler.cc:84: low_pass_filter_.Process(in, rtc::ArrayView<float>(x, in.size())); On 2016/06/29 08:56:28, hlundin-webrtc wrote: > ...

4 years, 5 months ago (2016-06-29 09:13:53 UTC) #19

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/down_sampler.cc (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/down_sampler.cc:84:
low_pass_filter_.Process(in, rtc::ArrayView<float>(x, in.size()));
On 2016/06/29 08:56:28, hlundin-webrtc wrote:
> On 2016/06/28 22:19:37, peah-webrtc wrote:
> > On 2016/06/28 09:35:51, aleloi wrote:
> > > Just to check that I understood correctly: we have a custom simple filter
> and
> > > resampling algorithm for performance reasons? Instead of using one from
> webrtc
> > > that is already implemented.
> > 
> > You definitely have a good point! It would perhaps make sense to reuse one
of
> > the other resamplers. 
> > I still think it may make sense to use this resampler regardless of that as
> the
> > current resampler is tailormade to use a cutoff filter with a quite low
cutoff
> > frequency which allows the bandlimiting filter to be likely much cheaper
than
> > what is the case for the other more general resamplers. Furthermore, having
> now
> > looked at the existing resampling code, I think that looks more expensive.
> Still
> > it would make sense to look into that.
> > 
> > WDYT, is it ok to keep it as it is with a custom made downsampler for now?
> 
> I think we should keep this for now, since time is scarce and you've done
> testing with the current implementation. But in general I do agree that we
> should try to consolidate things like general filter implementations.

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/gain_applier.cc (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_applier.cc:28: return
std::count_if(in.begin(), in.end(), [](float v) {
On 2016/06/29 08:56:28, hlundin-webrtc wrote:
> On 2016/06/28 22:19:37, peah-webrtc wrote:
> > On 2016/06/28 09:35:52, aleloi wrote:
> > > Probably the compiler is smart and this is not affecting performance in
any
> > way,
> > > but the array has const floats while the lambda takes floats. If it were
an
> > > array of complex objects, I think copying would happen. What is the
> difference
> > > to '[](const float & v)' ?
> > 
> > I'm not an expert on this, but I think it should be fine. As you say, for
> > complex objects there would be copies made, but a float should fit into the
> > registers and should be very cheap to handle.
> > 
> > But I guess it cannot increase the complexity to make it a const reference,
> and
> > as you point out, that solution is consistent in that the array consists of
> > const floats, so I'll change it to that.
> > 
> > Done.
> 
> I think that in this case it makes no difference. Passing a float or a
reference
> should be just as efficient. For the record, the declaration of count_if does
> indeed allow const ref as well as "by value".

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/level_controller.cc
(right):

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller.cc:85: void
LevelController::Metrics::ResetEstimation() {
On 2016/06/29 08:56:28, hlundin-webrtc wrote:
> Just call this method Reset.

Done.

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller.cc:109:
"WebRTC.Audio.LevelControlMaxNoisePower",
On 2016/06/29 08:56:28, hlundin-webrtc wrote:
> I think you should increase the readability of the histogram names by adding a
> '.' after LeveLControl. In this case "WebRTC.Audio.LevelControl.MaxNoisePower"
> and similar below.

Done.

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller.cc:110:
static_cast<int>(10 *
On 2016/06/29 08:56:28, hlundin-webrtc wrote:
> This was an awkward line wrap. Did clang format produce this?

Yes, that was clang format. Sorry, I did not check the result.

I now instead manually reformatted this part of the code. Could you please check
that it looks ok?

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/level_controller.h
(right):

https://codereview.webrtc.org/2090583002/diff/270001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/level_controller.h:51: int
metrics_frame_counter_;
On 2016/06/29 08:56:28, hlundin-webrtc wrote:
> A counter should probably be size_t.

Done.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2090583002/370001

4 years, 5 months ago (2016-06-29 10:33:22 UTC) #22

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14593)

4 years, 5 months ago (2016-06-29 10:48:35 UTC) #24

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2090583002/410001

4 years, 5 months ago (2016-06-29 12:21:49 UTC) #26

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14604)

4 years, 5 months ago (2016-06-29 12:30:00 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2090583002/430001

4 years, 5 months ago (2016-06-29 13:56:08 UTC) #30

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14611)

4 years, 5 months ago (2016-06-29 14:03:25 UTC) #32

aleloi

LGTM! It looks very nice https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_processing/level_controller/down_sampler.h File webrtc/modules/audio_processing/level_controller/down_sampler.h (right): https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_processing/level_controller/down_sampler.h#newcode32 webrtc/modules/audio_processing/level_controller/down_sampler.h:32: int down_sampling_factor_; On 2016/06/28 ...

4 years, 5 months ago (2016-06-29 14:11:18 UTC) #33

LGTM! It looks very nice

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/down_sampler.h (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/down_sampler.h:32: int
down_sampling_factor_;
On 2016/06/28 22:19:37, peah-webrtc wrote:
> On 2016/06/28 09:35:52, aleloi wrote:
> > Could the variables (including data_dumper_) be made const? It should be
> > possible to both check that 8000 divides sample_rate_hz and make it const at
> the
> > same time.
> 
> The other values cannot be const as they are changed in the Initialize method.
> It would be nice to have them const, but with the current design it is not
> possible to do that.
> 
> data_dumper_ could be made a const pointer, but not pointer to a const object.
> I'll change that to be a const pointer.
> 
> WDYT? Does that sound ok?

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/level_controller/gain_applier.cc (right):

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_applier.cc:63: gain =
std::min(new_gain, gain + step_size);
On 2016/06/28 22:19:37, peah-webrtc wrote:
> On 2016/06/28 09:35:52, aleloi wrote:
> > Maybe move the gain calculation outside of the loop for improved
readability?
> Or
> > call ApplyConstantGain after calculating it. Same for the function below.
> 
> I'm not fully sure of what you mean, but the intention is to  adjust the gain
> towards the new gain in a smooth manner to avoid discontinuities resulting in
> audible distortions. In order to do that, the updating of the gain needs to be
> inside the loop. Setting a new gain before the loop and applying the gain to
the
> whole frame could potentially cause an audible distortion.
> 

Acknowledged.

https://codereview.webrtc.org/2090583002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/level_controller/gain_applier.cc:115: }
On 2016/06/28 22:19:37, peah-webrtc wrote:
> On 2016/06/28 09:35:52, aleloi wrote:
> > Do we need both a constructor and an Initialize() method? With just a
> > constructor, the object is never in a half-initialized state, and it's
> possible
> > to document that gain_change_step_size_ doesn't change. Are there any
benefits
> > with two methods?
> 
> I definitely agree that it would be nicer to do the initialization only at
> construction time. However, as the audio processing module (APM) is
constructed,
> the sample rate and number of channels is specified by the incoming audio
stream
> and therefore not known when APM is constructed. Furthermore, the audio stream
> properties sample rate and number of channels can, and do, change over time.
> Each time these change, a reinitialization of the APM is needed and (I guess)
to
> let that reinitialization have a minimal impact on the performance most of the
> submodules are reinitialized without being reconstructed.
> 
>   

Acknowledged.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2090583002/450001

4 years, 5 months ago (2016-06-29 15:30:37 UTC) #37

commit-bot: I haz the power

Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14621)

4 years, 5 months ago (2016-06-29 15:43:17 UTC) #39

aluebs-webrtc

4 years, 5 months ago (2016-06-29 16:40:28 UTC) #40

peah-webrtc

4 years, 5 months ago (2016-06-29 20:40:00 UTC) #41

https://codereview.webrtc.org/2090583002/diff/220001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2090583002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/audio_processing_impl.cc:775:
private_submodules_->level_controller->Process(ca);
On 2016/06/29 16:40:28, aluebs-webrtc wrote:
> On 2016/06/29 06:22:09, peah-webrtc wrote:
> > On 2016/06/28 22:45:02, aluebs-webrtc wrote:
> > > I am not sure how this component works (it is not in the CL description or
> > > documented in the header file), but if it applies an additional gain to
the
> > > signal, we probably want to compensate for that in the gain_db parameter
> when
> > > calling intelligibility_enhancer->SetCaptureNoiseEstimate() in line 728.
> > 
> > The component indeed applies an additional gain. 
> > 
> > I'll add an API call for obtaining the latest applied gain, but I'll let you
> use
> > that gain in the intelligibility enhancer in another CL since
> > 1) I don't really understand how that gain is to be used in the
> intelligibility
> > enhancer.
> > 2) It is nice to make this CL only concern the level control and not also
> affect
> > the intelligibility enhancer.
> > 3) The intelligibility enhancer is currently not active by default so the
> > addition of the level control will not cause any downstream breakages.
> > 
> > Lets discuss it further offline if you want.
> 
> 2) This CL concerns the IE, because it applies a gain without compensating for
> it.
> 3) It can be manually enabled and some of us are using it already.
> But I can do a separate CL if you want.

I definitely see that it affects the IE, but I think it is better if you do that
change as you know how that is designed. Furthermore, I'd expect that that
change needs to be verified carefully, as the level controller may change the
power of the signal by as much as 20 dB.

An alternative is to ensure that the IE and the level control are never active
at the same time.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2090583002/470001

4 years, 5 months ago (2016-06-29 21:06:22 UTC) #44

commit-bot: I haz the power

Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14637)

4 years, 5 months ago (2016-06-29 21:21:17 UTC) #46

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2090583002/470001

4 years, 5 months ago (2016-06-29 21:54:19 UTC) #48

commit-bot: I haz the power

Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14640)

4 years, 5 months ago (2016-06-29 22:02:03 UTC) #50

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2090583002/470001

4 years, 5 months ago (2016-06-29 22:24:01 UTC) #53

commit-bot: I haz the power

Patchset 24 (id:??) landed as https://crrev.com/ca4cac7e7494e7b5d60bda4b21b6a00592f8e1df Cr-Commit-Position: refs/heads/master@{#13333}

4 years, 5 months ago (2016-06-29 22:26:31 UTC) #58

kjellander_webrtc

Edward found an error in this CL. Can you fix? https://codereview.webrtc.org/2090583002/diff/470001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/2090583002/diff/470001/webrtc/webrtc_tests.gypi#newcode437 ...

4 years, 4 months ago (2016-08-17 15:21:19 UTC) #60

peah-webrtc

https://codereview.webrtc.org/2090583002/diff/470001/webrtc/webrtc_tests.gypi File webrtc/webrtc_tests.gypi (right): https://codereview.webrtc.org/2090583002/diff/470001/webrtc/webrtc_tests.gypi#newcode437 webrtc/webrtc_tests.gypi:437: 'modules/audio_processing/level_controller/level_controller_complexity_unittest.cc', On 2016/08/17 15:21:19, kjellander_webrtc wrote: > I failed ...

4 years, 4 months ago (2016-08-18 05:48:05 UTC) #61

peah-webrtc

4 years, 4 months ago (2016-08-18 06:40:08 UTC) #62

kjellander_webrtc

I posted my comment in the wrong file, but you figured it out anyway :) ...

4 years, 4 months ago (2016-08-18 07:25:29 UTC) #63

peah-webrtc

4 years, 4 months ago (2016-08-18 07:26:34 UTC) #64

Message was sent while issue was closed.

On 2016/08/18 07:25:29, kjellander_webrtc wrote:
> I posted my comment in the wrong file, but you figured it out anyway :)
> 
> https://codereview.webrtc.org/2090583002/diff/470001/webrtc/webrtc_tests.gypi
> File webrtc/webrtc_tests.gypi (right):
> 
>
https://codereview.webrtc.org/2090583002/diff/470001/webrtc/webrtc_tests.gypi...
> webrtc/webrtc_tests.gypi:437:
>
'modules/audio_processing/level_controller/level_controller_complexity_unittest.cc',
> On 2016/08/18 06:40:08, peah-webrtc wrote:
> > On 2016/08/18 05:48:05, peah-webrtc wrote:
> > > On 2016/08/17 15:21:19, kjellander_webrtc wrote:
> > > > I failed to spot this error. This line should have been added into
> > > > webrtc/modules/BUILD.gn in order to be in the same target as GYP
> > > > (modules_unittests).
> > > > Can you fix in a follow-up CL?
> > > 
> > > I can definitely do that.
> > > I'm not really sure what is incorrect though. Is there a special reason
why
> > this
> > > test should go into modules_unittests?
> > > 
> > > The CL https://codereview.webrtc.org/2178963002 added this test to
> > > webrtc/BUILD.gn where the other performance tests are defined and to me
that
> > > looks ok. But please correct me if I'm wrong!
> > > 
> > > 
> > > 
> > 
> > I think you meant the level_controller_unittest.cc file, right? That is
indeed
> > missing from modules/BUILD.gn. I'll add that in a new CL.
> 
> Oh, right. Sorry for posting the comment on the wrong line :P Confusing to say
> the least.

No worries! Thanks for finding this out!

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages