Update audio code to use size_t more correctly, webrtc/modules/audio_processing/

Update audio code to use size_t more correctly, webrtc/modules/audio_processing/
portion.

This is a piece of https://codereview.webrtc.org/1230503003 , split out to a
separate change to make reviewing easier.

BUG=chromium:81439
TEST=none

[Peter Kasting, 2015-07-23 20:50:42]

pkasting@chromium.org changed reviewers:
+ andrew@webrtc.org

[Peter Kasting, 2015-07-23 20:50:43]

https://codereview.webrtc.org/1227213002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://codereview.webrtc.org/1227213002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:183: const
size_t NonlinearBeamformer::kNumFreqBins;
Changing the type of this from int to size_t means some compilers will complain
if it doesn't have an explicit definition in addition to the declaration. 
constexpr might be able to address this if only it worked in MSVC.

[Andrew MacDonald, 2015-07-24 04:01:42]

andrew@webrtc.org changed reviewers:
+ ekmeyerson@webrtc.org, turaj@webrtc.org

[Andrew MacDonald, 2015-07-24 04:01:44]

First, apologies, as a large-ish CL just landed in this code:
https://codereview.webrtc.org/1248393003/

which will cause you some merging fun.

My main concern is that you didn't convert number of channel quantities. What
was your rationale there? I pointed out a few of the cases, but there are many
more.

Elliot, Turaj: I got a bit tangentially angry at the intelligibility code; could
you have a quick look at my comments there?

https://codereview.webrtc.org/1227213002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://codereview.webrtc.org/1227213002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:183: const
size_t NonlinearBeamformer::kNumFreqBins;
On 2015/07/23 20:50:43, Peter Kasting wrote:
> Changing the type of this from int to size_t means some compilers will
complain
> if it doesn't have an explicit definition in addition to the declaration. 
> constexpr might be able to address this if only it worked in MSVC.

Acknowledged.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/agc/agc_manager_direct.h (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc/agc_manager_direct.h:59: int num_channels,
You've used size_t for num_channels elsewhere.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/matrix.h (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/matrix.h:100: int num_rows) {
num_rows should be size_t

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/matrix.h:109: void Resize(int
num_rows, int num_columns) {
And here, and everywhere else we have num_rows/num_columns.

Was your intent to try to convert everything?

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:351: int
num_input_channels,
and here

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h:62: int
num_input_channels,
num_input_channels and num_output_channels should be size_t

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:500: int
num_channels() const { return num_channels_; }
size_t

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:301:
size_t lf = 1, rf = 4;
Sorry to make you do this, but could you replace lf and rf with literals below?
These names provide absolutely no insight into what these quantities represent,
so it becomes less readable than literals.

Alternately, I've added ekmeyerson and turaj who might be able to suggest
meaningful names. (In which case they should be const and use kStyle).

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:305:
center_freqs_[i] = 676170.4f / (47.06538f - expf(0.08950404f * abs_temp));
Gah, what are these magic numbers? Elliot, please have a look later.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:318:
size_t lll, ll, rr, rrr;
This is horrific. Elliot, could you look into using more meaningful names here
later.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:321:
center_freqs_[max(one, i - lf) - 1] * freqs_ /
kOne, but why not just use a literal? "1u"

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h
(right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:48:
int channels,
size_t

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

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/splitting_filter_unittest.cc:55: is_present[j] =
i & (static_cast<size_t>(1) << j);
Literal?

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

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/audio_processing_unittest.cc:667:
std::min(static_cast<size_t>(16), frame_->samples_per_channel_ / 10);
Literal?

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/audio_processing_unittest.cc:991:
std::max(4, static_cast<int>(640 / frame_->samples_per_channel_));
4u instead of the cast?

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/transient/transient_suppressor.h (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/transient/transient_suppressor.h:89:
rtc::scoped_ptr<size_t[]> ip_;
I think the ultimate user of this wants an int, so I don't think this should be
changed.

[ekm, 2015-07-24 06:29:16]

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:301:
size_t lf = 1, rf = 4;
On 2015/07/24 04:01:43, andrew wrote:
> Sorry to make you do this, but could you replace lf and rf with literals
below?
> These names provide absolutely no insight into what these quantities
represent,
> so it becomes less readable than literals.
> 
> Alternately, I've added ekmeyerson and turaj who might be able to suggest
> meaningful names. (In which case they should be const and use kStyle).

Sorry about this. This function is a direct port of research code, variable
names and all. I'll rename and document once this is landed. 

lf and rf control the left and right slopes of the ERB filters.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:305:
center_freqs_[i] = 676170.4f / (47.06538f - expf(0.08950404f * abs_temp));
On 2015/07/24 04:01:44, andrew wrote:
> Gah, what are these magic numbers? Elliot, please have a look later.

This is the magic formula for inverse ERBS from psychoacoustics. I don't think
the literals have much meaning individually. I'll add a comment and pointer to a
document+paper describing their derivation, e.g.,
http://www.ee.ic.ac.uk/hp/staff/dmb/voicebox/doc/voicebox/erb2frq.html .

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:318:
size_t lll, ll, rr, rrr;
On 2015/07/24 04:01:44, andrew wrote:
> This is horrific. Elliot, could you look into using more meaningful names here
> later.

Agreed; will do. They correspond from left-to-right to the freq values of the 4
vertices of the trapezoidal filter.

[Peter Kasting, 2015-07-24 06:44:22]

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/agc/agc_manager_direct.h (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc/agc_manager_direct.h:59: int num_channels,
On 2015/07/24 at 04:01:43, andrew wrote:
> You've used size_t for num_channels elsewhere.

Not in this CL, at least as far as I'm aware.  I did a followon CL for channel
counts for you at https://codereview.webrtc.org/1238083005 , though it doesn't
convert these particular channel counts.

I'm definitely not going to add channel conversions to this CL.  I might try
adding it to that other CL, but remember that every time I touch anything I have
to touch the transitive closure, which is often hundreds of files.  It's just
infeasible to do too much at once -- this CL alone has taken some eight months
of my time.  I'm the only one doing this and I have to do it in pieces.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/matrix.h (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/matrix.h:100: int num_rows) {
On 2015/07/24 at 04:01:43, andrew wrote:
> num_rows should be size_t

That's also part of the followup: https://codereview.webrtc.org/1238083005

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/matrix.h:109: void Resize(int
num_rows, int num_columns) {
On 2015/07/24 at 04:01:43, andrew wrote:
> And here, and everywhere else we have num_rows/num_columns.
> 
> Was your intent to try to convert everything?

No!  Seriously, it would literally be impossible to do or to land.

Same response for the rest of the channel count places you point out.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:301:
size_t lf = 1, rf = 4;
On 2015/07/24 at 06:29:16, ekm wrote:
> On 2015/07/24 04:01:43, andrew wrote:
> > Sorry to make you do this, but could you replace lf and rf with literals
below?
> > These names provide absolutely no insight into what these quantities
represent,
> > so it becomes less readable than literals.
> > 
> > Alternately, I've added ekmeyerson and turaj who might be able to suggest
> > meaningful names. (In which case they should be const and use kStyle).
> 
> Sorry about this. This function is a direct port of research code, variable
names and all. I'll rename and document once this is landed.

Based on this, I won't touch these.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:321:
center_freqs_[max(one, i - lf) - 1] * freqs_ /
On 2015/07/24 at 04:01:44, andrew wrote:
> kOne, but why not just use a literal? "1u"

1u causes compile failures :(  (i tried that first)

I got the kOne thing correct in the real patch, apparently I screwed it up in
this carved-off copy.

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

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/splitting_filter_unittest.cc:55: is_present[j] =
i & (static_cast<size_t>(1) << j);
On 2015/07/24 at 04:01:44, andrew wrote:
> Literal?

That might work here, I can't remember.  I'll try it.

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

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/audio_processing_unittest.cc:667:
std::min(static_cast<size_t>(16), frame_->samples_per_channel_ / 10);
On 2015/07/24 at 04:01:44, andrew wrote:
> Literal?

Can't, compile errors.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/transient/transient_suppressor.h (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/transient/transient_suppressor.h:89:
rtc::scoped_ptr<size_t[]> ip_;
On 2015/07/24 at 04:01:44, andrew wrote:
> I think the ultimate user of this wants an int, so I don't think this should
be changed.

No, the FFT code is all changing to want size_ts.

[Andrew MacDonald, 2015-07-24 19:07:13]

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/agc/agc_manager_direct.h (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc/agc_manager_direct.h:59: int num_channels,
On 2015/07/24 06:44:22, Peter Kasting wrote:
> I'm definitely not going to add channel conversions to this CL.  I might try
> adding it to that other CL, but remember that every time I touch anything I
have
> to touch the transitive closure, which is often hundreds of files.  It's just
> infeasible to do too much at once -- this CL alone has taken some eight months
> of my time.  I'm the only one doing this and I have to do it in pieces.

Understood, thanks.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/beamformer/matrix.h (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/matrix.h:100: int num_rows) {
On 2015/07/24 06:44:22, Peter Kasting wrote:
> On 2015/07/24 at 04:01:43, andrew wrote:
> > num_rows should be size_t
> 
> That's also part of the followup: https://codereview.webrtc.org/1238083005

Acknowledged.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/beamformer/matrix.h:109: void Resize(int
num_rows, int num_columns) {
On 2015/07/24 06:44:22, Peter Kasting wrote:
> On 2015/07/24 at 04:01:43, andrew wrote:
> > And here, and everywhere else we have num_rows/num_columns.
> > 
> > Was your intent to try to convert everything?
> 
> No!  Seriously, it would literally be impossible to do or to land.
> 
> Same response for the rest of the channel count places you point out.

Acknowledged.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/transient/transient_suppressor.h (right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/transient/transient_suppressor.h:89:
rtc::scoped_ptr<size_t[]> ip_;
On 2015/07/24 06:44:22, Peter Kasting wrote:
> On 2015/07/24 at 04:01:44, andrew wrote:
> > I think the ultimate user of this wants an int, so I don't think this should
> be changed.
> 
> No, the FFT code is all changing to want size_ts.

Ah, I see. Question in the common_audio CL about that.

[Peter Kasting, 2015-07-27 23:09:55]

New snap up.

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:321:
center_freqs_[max(one, i - lf) - 1] * freqs_ /
On 2015/07/24 06:44:22, Peter Kasting wrote:
> On 2015/07/24 at 04:01:44, andrew wrote:
> > kOne, but why not just use a literal? "1u"
> 
> 1u causes compile failures :(  (i tried that first)
> 
> I got the kOne thing correct in the real patch, apparently I screwed it up in
> this carved-off copy.

Correctly copied over the kOne usage from the full CL now.

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

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/splitting_filter_unittest.cc:55: is_present[j] =
i & (static_cast<size_t>(1) << j);
On 2015/07/24 06:44:22, Peter Kasting wrote:
> On 2015/07/24 at 04:01:44, andrew wrote:
> > Literal?
> 
> That might work here, I can't remember.  I'll try it.

Just using "1" seems to work on Windows, not sure if I changed this for other
platforms or what.  I'll try "1", and if that won't compile on all the trybots,
I'll use "1u", and only use the cast version if neither works.

[Andrew MacDonald, 2015-07-28 07:05:44]

What you have lgtm, but you still need a rebase I believe.

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

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/splitting_filter_unittest.cc:55: is_present[j] =
i & (static_cast<size_t>(1) << j);
On 2015/07/27 23:09:54, Peter Kasting wrote:
> Just using "1" seems to work on Windows, not sure if I changed this for other
> platforms or what.  I'll try "1", and if that won't compile on all the
trybots,
> I'll use "1u", and only use the cast version if neither works.

Sounds good.

[Peter Kasting, 2015-07-28 22:34:32]

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

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/splitting_filter_unittest.cc:55: is_present[j] =
i & (static_cast<size_t>(1) << j);
On 2015/07/28 07:05:43, andrew wrote:
> On 2015/07/27 23:09:54, Peter Kasting wrote:
> > Just using "1" seems to work on Windows, not sure if I changed this for
other
> > platforms or what.  I'll try "1", and if that won't compile on all the
> trybots,
> > I'll use "1u", and only use the cast version if neither works.
> 
> Sounds good.

Update: the static_cast really is required for the Win x64 bot:

e:\b\build\slave\win\build\src\webrtc\modules\audio_processing\splitting_filter_unittest.cc(55)
: warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits
(was 64-bit shift intended?)

[Andrew MacDonald, 2015-07-29 01:10:56]

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

https://codereview.webrtc.org/1227213002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/splitting_filter_unittest.cc:55: is_present[j] =
i & (static_cast<size_t>(1) << j);
On 2015/07/28 22:34:32, Peter Kasting wrote:
> Update: the static_cast really is required for the Win x64 bot:
> 
>
e:\b\build\slave\win\build\src\webrtc\modules\audio_processing\splitting_filter_unittest.cc(55)
> : warning C4334: '<<' : result of 32-bit shift implicitly converted to 64 bits
> (was 64-bit shift intended?)

Thanks for checking.