Convert channel counts to size_t.

Convert channel counts to size_t.

IIRC, this was originally requested by ajm during review of the other size_t conversions I did over the past year, and I agreed it made sense, but wanted to do it separately since those changes were already gargantuan.

BUG=chromium:81439
TEST=none
R=henrik.lundin@webrtc.org, henrika@webrtc.org, kjellander@webrtc.org, minyue@webrtc.org, perkj@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org, tina.legrand@webrtc.org

Committed: https://crrev.com/6955870806624479723addfae6dcf5d13968796c
Cr-Commit-Position: refs/heads/master@{#11229}

[Peter Kasting, 2015-12-28 18:25:55]

Description was changed from

==========
Convert channel counts to size_t.

BUG=chromium:81439
TEST=none
==========

to

==========
Convert channel counts to size_t.

IIRC, this was originally requested by ajm during review of the other size_t
conversions I did over the past year, and I agreed it made sense, but wanted to
do it separately since those changes were already gargantuan.

BUG=chromium:81439
TEST=none
==========

[Peter Kasting, 2015-12-28 18:25:55]

pkasting@chromium.org changed reviewers:
+ henrik.lundin@webrtc.org, henrika@webrtc.org, kjellander@webrtc.org,
minyue@webrtc.org, perkj@webrtc.org, stefan@webrtc.org, tina.legrand@webrtc.org

[Peter Kasting, 2015-12-28 18:26:47]

henrik.lundin: webrtc/modules/audio_processing/ OWNERS
henrika: webrtc/modules/audio_device/, webrtc/voice_engine/ OWNERS
kjellander: webrtc/tools/ OWNERS
minyue: webrtc/modules/audio_coding/, webrtc/modules/audio_conference_mixer/
OWNERS
perkj: talk/, webrtc/modules/media_file/, webrtc/system_wrappers/ OWNERS
stefan: webrtc/modules/rtp_rtcp/, webrtc/video/ OWNERS
tina.legrand: webrtc/audio/, webrtc/common_audio/, webrtc/common_types.h,
webrtc/modules/include/module_common_types.h OWNERS

[henrika_webrtc, 2015-12-29 09:00:44]

henrika: webrtc/modules/audio_device/, webrtc/voice_engine/ OWNERS

I am OK if you can guarantee that the interface changes in
webrtc/modules/audio_device/include/audio_device_defines.h does not break
Chrome.
If they do (which I think they do as is), perhaps you could skip making these
ADM changes unless you feel strongly about being consistent.

I will be OOO until Jan 4th and will check my parts again then. Hope I am not
delaying this change too much.

[henrika_webrtc, 2015-12-29 09:00:58]

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_dev...
File webrtc/modules/audio_device/include/audio_device_defines.h (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/include/audio_device_defines.h:47: class
AudioTransport {
Note that this interface is used in Chrome and in Libjingle. I guess this change
might break things, and if so, is it really worth doing it?

See e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

and

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

and

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/include/audio_device_defines.h:128: size_t
number_of_channels,
I think this change will break Chrome here:

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

[Peter Kasting, 2015-12-29 09:03:29]

Yes, I'm aware that I'll need to update the consumers of these APIs as well. 
That's the same story as the previous conversions to size_t I've done.

[Peter Kasting, 2015-12-29 09:04:47]

(And no, failing to update these APIs is not really an option.)

[henrika_webrtc, 2015-12-29 09:06:46]

Given that you seem to be in full control, LGTM from me then. Thanks for quick
reply.

[minyue-webrtc, 2015-12-29 10:42:47]

LG with some nits

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:26: size_t
samples_per_frame =
but why not casting |num_channels| to int and keep the old code. Literally,
|samples_per_frame| can go negative.

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc:70: channels_ =
static_cast<size_t>(get<0>(GetParam()));
Nit: can redefine this param if you like

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/tools/audio_codec_speed_test.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/tools/audio_codec_speed_test.cc:41: channels_
= static_cast<size_t>(get<0>(GetParam()));
can change the def of param 0 if you like

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/tools/neteq_quality_test.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/tools/neteq_quality_test.cc:213:
channels_(static_cast<size_t>(FLAGS_channels)),
can change the def of FLAGS_channels if you like

[Peter Kasting, 2015-12-30 00:55:45]

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:26: size_t
samples_per_frame =
On 2015/12/29 10:42:47, minyue-webrtc wrote:
> but why not casting |num_channels| to int and keep the old code. Literally,
> |samples_per_frame| can go negative.

What?  How can that be negative?  You'd have to have a negative frame size or a
negative sample rate, neither of which make sense.

And it's correct for |samples_per_frame| to be a size_t, as it's a count of
items stored in memory.

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc:70: channels_ =
static_cast<size_t>(get<0>(GetParam()));
On 2015/12/29 10:42:47, minyue-webrtc wrote:
> Nit: can redefine this param if you like

I don't understand what "redefine" means here.  What does it mean, concretely,
to redefine this param?

(Same comment other two places)

[minyue-webrtc, 2015-12-30 09:56:39]

On 2015/12/30 00:55:45, Peter Kasting wrote:
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):
> 
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:26: size_t
> samples_per_frame =
> On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > but why not casting |num_channels| to int and keep the old code. Literally,
> > |samples_per_frame| can go negative.
> 
> What?  How can that be negative?  You'd have to have a negative frame size or
a
> negative sample rate, neither of which make sense.
> 
> And it's correct for |samples_per_frame| to be a size_t, as it's a count of
> items stored in memory.
> 
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc (right):
> 
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc:70: channels_ =
> static_cast<size_t>(get<0>(GetParam()));
> On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > Nit: can redefine this param if you like
> 
> I don't understand what "redefine" means here.  What does it mean, concretely,
> to redefine this param?
> 
> (Same comment other two places)

Thanks. Do you receive my offline comment also?

[Peter Kasting, 2015-12-30 13:07:16]

On 2015/12/30 09:56:39, minyue-webrtc wrote:
> On 2015/12/30 00:55:45, Peter Kasting wrote:
> >
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> > File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> > webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:26: size_t
> > samples_per_frame =
> > On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > > but why not casting |num_channels| to int and keep the old code.
Literally,
> > > |samples_per_frame| can go negative.
> > 
> > What?  How can that be negative?  You'd have to have a negative frame size
or
> a
> > negative sample rate, neither of which make sense.
> > 
> > And it's correct for |samples_per_frame| to be a size_t, as it's a count of
> > items stored in memory.
> > 
> >
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> > File webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> > webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc:70: channels_ =
> > static_cast<size_t>(get<0>(GetParam()));
> > On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > > Nit: can redefine this param if you like
> > 
> > I don't understand what "redefine" means here.  What does it mean,
concretely,
> > to redefine this param?
> > 
> > (Same comment other two places)
> 
> Thanks. Do you receive my offline comment also?

I'm not sure what you're thanking me for, and I don't understand your question
about an offline comment.  Are you saying you sent me some sort of direct
comment separately?  If so, no, I've not received that.  If you're saying
something else, sorry, please restate?

[minyue-webrtc, 2016-01-04 09:06:03]

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:26: size_t
samples_per_frame =
On 2015/12/30 00:55:45, Peter Kasting wrote:
> On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > but why not casting |num_channels| to int and keep the old code. Literally,
> > |samples_per_frame| can go negative.
> 
> What?  How can that be negative?  You'd have to have a negative frame size or
a
> negative sample rate, neither of which make sense.
> 
> And it's correct for |samples_per_frame| to be a size_t, as it's a count of
> items stored in memory.

sorry for ambiguity. I meant that there is no check on |frame_size_ms| and
|sample_rate_hz| to be non-negative. But I agree with you that
|samples_per_frame| sounds a size_t. Keep it this way.

Do you plan to let NumSamplesPerFrame return a size_t also?

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc:70: channels_ =
static_cast<size_t>(get<0>(GetParam()));
On 2015/12/30 00:55:45, Peter Kasting wrote:
> On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > Nit: can redefine this param if you like
> 
> I don't understand what "redefine" means here.  What does it mean, concretely,
> to redefine this param?
> 
> (Same comment other two places)

See line 25.

[Peter Kasting, 2016-01-04 19:49:42]

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:26: size_t
samples_per_frame =
On 2016/01/04 09:06:02, minyue-webrtc wrote:
> On 2015/12/30 00:55:45, Peter Kasting wrote:
> > On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > > but why not casting |num_channels| to int and keep the old code.
Literally,
> > > |samples_per_frame| can go negative.
> > 
> > What?  How can that be negative?  You'd have to have a negative frame size
or
> a
> > negative sample rate, neither of which make sense.
> > 
> > And it's correct for |samples_per_frame| to be a size_t, as it's a count of
> > items stored in memory.
> 
> sorry for ambiguity. I meant that there is no check on |frame_size_ms| and
> |sample_rate_hz| to be non-negative. But I agree with you that
> |samples_per_frame| sounds a size_t. Keep it this way.
> 
> Do you plan to let NumSamplesPerFrame return a size_t also?

Looking at its use, it should be, yes.  I can make that change, since it doesn't
look like it will transitively affect much else.  It's not clear to me whether
it's really necessary to CHECK that it's less than int16_t::max at that point.

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc:70: channels_ =
static_cast<size_t>(get<0>(GetParam()));
On 2016/01/04 09:06:02, minyue-webrtc wrote:
> On 2015/12/30 00:55:45, Peter Kasting wrote:
> > On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > > Nit: can redefine this param if you like
> > 
> > I don't understand what "redefine" means here.  What does it mean,
concretely,
> > to redefine this param?
> > 
> > (Same comment other two places)
> 
> See line 25.

Oh.  Hmm, I'm not familiar with how TestWithParam<> works.  Can I safely just
change the type inside the tuple and it won't affect any other code anywhere
else (e.g. that invokes this test)?

[kjellander_webrtc, 2016-01-04 20:40:49]

webrtc/tools: lgtm

[minyue-webrtc, 2016-01-04 22:03:18]

lgtm to audio coding

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:26: size_t
samples_per_frame =
On 2016/01/04 19:49:42, Peter Kasting wrote:
> On 2016/01/04 09:06:02, minyue-webrtc wrote:
> > On 2015/12/30 00:55:45, Peter Kasting wrote:
> > > On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > > > but why not casting |num_channels| to int and keep the old code.
> Literally,
> > > > |samples_per_frame| can go negative.
> > > 
> > > What?  How can that be negative?  You'd have to have a negative frame size
> or
> > a
> > > negative sample rate, neither of which make sense.
> > > 
> > > And it's correct for |samples_per_frame| to be a size_t, as it's a count
of
> > > items stored in memory.
> > 
> > sorry for ambiguity. I meant that there is no check on |frame_size_ms| and
> > |sample_rate_hz| to be non-negative. But I agree with you that
> > |samples_per_frame| sounds a size_t. Keep it this way.
> > 
> > Do you plan to let NumSamplesPerFrame return a size_t also?
> 
> Looking at its use, it should be, yes.  I can make that change, since it
doesn't
> look like it will transitively affect much else.  It's not clear to me whether
> it's really necessary to CHECK that it's less than int16_t::max at that point.

I guess that check is to make sure that casting on line 31 won't wrap around.
Making NumSamplesPerFrame return a size_t may be out-reached for this CL. I just
wanted to know if there is a plan in the future.

It is up to you whether to add a sanity check on the input argument. I prefer
you do it, to the degree that the function gives the same result in all cases as
it did before this CL, except for error input, which fails at the check.

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc:70: channels_ =
static_cast<size_t>(get<0>(GetParam()));
On 2016/01/04 19:49:42, Peter Kasting wrote:
> On 2016/01/04 09:06:02, minyue-webrtc wrote:
> > On 2015/12/30 00:55:45, Peter Kasting wrote:
> > > On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > > > Nit: can redefine this param if you like
> > > 
> > > I don't understand what "redefine" means here.  What does it mean,
> concretely,
> > > to redefine this param?
> > > 
> > > (Same comment other two places)
> > 
> > See line 25.
> 
> Oh.  Hmm, I'm not familiar with how TestWithParam<> works.  Can I safely just
> change the type inside the tuple and it won't affect any other code anywhere
> else (e.g. that invokes this test)?

I think it is just safe to change the first param to size_t, and it looks nicer.
If you like, please try it.

[perkj_webrtc, 2016-01-07 09:32:30]

perkj@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[perkj_webrtc, 2016-01-07 09:32:32]

lgtm for my parts. But solenberg- please help comment on fakewebrtcvoiceengine.h

https://codereview.webrtc.org/1316523002/diff/320001/talk/media/webrtc/fakewe...
File talk/media/webrtc/fakewebrtcvoiceengine.h (right):

https://codereview.webrtc.org/1316523002/diff/320001/talk/media/webrtc/fakewe...
talk/media/webrtc/fakewebrtcvoiceengine.h:80: size_t num_input_channels() const
override { return 0; }
I am not sure I understand the purpose of WEBRTC_STUB_CONST.. Should there be a
macro for size_t as well to keep the same pattern?
solenberg - I guess that is your decision if you know own the voice engine
implementation.

[Peter Kasting, 2016-01-07 09:47:50]

https://codereview.webrtc.org/1316523002/diff/320001/talk/media/webrtc/fakewe...
File talk/media/webrtc/fakewebrtcvoiceengine.h (right):

https://codereview.webrtc.org/1316523002/diff/320001/talk/media/webrtc/fakewe...
talk/media/webrtc/fakewebrtcvoiceengine.h:80: size_t num_input_channels() const
override { return 0; }
On 2016/01/07 09:32:32, perkj_webrtc wrote:
> I am not sure I understand the purpose of WEBRTC_STUB_CONST.. Should there be
a
> macro for size_t as well to keep the same pattern?

AFAICT it's just to reduce verbosity.  I don't see much benefit to adding
another macro form for "const function which returns size_t" when that would
literally just be used here.

Frankly, given the style guide's direction to avoid macros when possible, I
think all the macros here should be removed and replaced with normal function
declarations like we use almost everywhere else, but I'm not going to try to
write that change and push it through.

[the sun, 2016-01-07 12:27:59]

lgtm for
talk/media/webrtc
webrtc/audio
webrtc/voice_engine

https://codereview.webrtc.org/1316523002/diff/320001/talk/media/webrtc/fakewe...
File talk/media/webrtc/fakewebrtcvoiceengine.h (right):

https://codereview.webrtc.org/1316523002/diff/320001/talk/media/webrtc/fakewe...
talk/media/webrtc/fakewebrtcvoiceengine.h:80: size_t num_input_channels() const
override { return 0; }
On 2016/01/07 09:47:50, Peter Kasting wrote:
> On 2016/01/07 09:32:32, perkj_webrtc wrote:
> > I am not sure I understand the purpose of WEBRTC_STUB_CONST.. Should there
be
> a
> > macro for size_t as well to keep the same pattern?
> 
> AFAICT it's just to reduce verbosity.  I don't see much benefit to adding
> another macro form for "const function which returns size_t" when that would
> literally just be used here.
> 
> Frankly, given the style guide's direction to avoid macros when possible, I
> think all the macros here should be removed and replaced with normal function
> declarations like we use almost everywhere else, but I'm not going to try to
> write that change and push it through.

Don't worry; this is going away. Don't create any new macros.

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:38: size_t      channels;
On 64-bit machines member alignment rules will cause this struct to be 30%
bigger. Probably not an issue, but it could be avoided by moving 'channels' to
be the first member.

Other structs touched by this CL would likely be affected in similar ways.

[Peter Kasting, 2016-01-07 13:14:41]

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:38: size_t      channels;
On 2016/01/07 12:27:59, the sun wrote:
> On 64-bit machines member alignment rules will cause this struct to be 30%
> bigger. Probably not an issue, but it could be avoided by moving 'channels' to
> be the first member.

It looks like other places (e.g. the param list of OnInitializeDecoder() below)
use the same param ordering.  If I changed one I'd kind of like to change the
others to be consistent.  That could be a lot of places.

Realistically, are there going to be tens of thousands or more of these objects,
such that the additional bytes would add up to a meaningful amount?

[the sun, 2016-01-07 13:21:06]

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:38: size_t      channels;
On 2016/01/07 13:14:41, Peter Kasting wrote:
> On 2016/01/07 12:27:59, the sun wrote:
> > On 64-bit machines member alignment rules will cause this struct to be 30%
> > bigger. Probably not an issue, but it could be avoided by moving 'channels'
to
> > be the first member.
> 
> It looks like other places (e.g. the param list of OnInitializeDecoder()
below)
> use the same param ordering.  If I changed one I'd kind of like to change the
> others to be consistent.  That could be a lot of places.

I don't see why the order of parameters necessarily need to be the same as the
order of fields in a data type. If all fields of the type are passed as params,
why not pass the struct instead?

> 
> Realistically, are there going to be tens of thousands or more of these
objects,
> such that the additional bytes would add up to a meaningful amount?

In this case I definitely doubt it would be noticeable. What other structs are
affected, and by how much? Also, there's always the rule about premature
pessimisation. :)

[tlegrand-webrtc, 2016-01-07 15:24:18]

LGTM for webrtc/common_audio/ and webrtc/common_types.h (webrtc/audio already
approved by Solenberg).

[Peter Kasting, 2016-01-07 22:28:39]

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:38: size_t      channels;
On 2016/01/07 13:21:06, the sun wrote:
> If all fields of the type are passed as params,
> why not pass the struct instead?

I think that makes sense, but it's a bit of refactoring that goes beyond the
scope of what I want to do in this patch.

> > Realistically, are there going to be tens of thousands or more of these
> objects,
> > such that the additional bytes would add up to a meaningful amount?
> 
> In this case I definitely doubt it would be noticeable. What other structs are
> affected, and by how much?

Don't know, but I can't imagine it's anything meaningful, since I'm changing the
type of channel counts, and there's shouldn't be any structs with a "number of
channels" member that are copied many times (e.g. actual audio packets wouldn't
have such a member).

[Peter Kasting, 2016-01-08 02:45:07]

tina.legrand: Make sure to look over
webrtc/modules/include/module_common_types.h as well.
henrik.lundin, stefan: Still waiting on your OKs.

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:26: size_t
samples_per_frame =
I simplified everything to just do this calculation inline at the callsite,
since the result is being stored in a size_t anyway.  This looks like how other
similar sites do this anyway.

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc:70: channels_ =
static_cast<size_t>(get<0>(GetParam()));
On 2016/01/04 22:03:18, minyue-webrtc wrote:
> On 2016/01/04 19:49:42, Peter Kasting wrote:
> > On 2016/01/04 09:06:02, minyue-webrtc wrote:
> > > On 2015/12/30 00:55:45, Peter Kasting wrote:
> > > > On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > > > > Nit: can redefine this param if you like
> > > > 
> > > > I don't understand what "redefine" means here.  What does it mean,
> > concretely,
> > > > to redefine this param?
> > > > 
> > > > (Same comment other two places)
> > > 
> > > See line 25.
> > 
> > Oh.  Hmm, I'm not familiar with how TestWithParam<> works.  Can I safely
just
> > change the type inside the tuple and it won't affect any other code anywhere
> > else (e.g. that invokes this test)?
> 
> I think it is just safe to change the first param to size_t, and it looks
nicer.
> If you like, please try it.

Done.

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/tools/audio_codec_speed_test.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/tools/audio_codec_speed_test.cc:41: channels_
= static_cast<size_t>(get<0>(GetParam()));
On 2015/12/29 10:42:47, minyue-webrtc wrote:
> can change the def of param 0 if you like

Done.

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/tools/neteq_quality_test.cc (right):

https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/tools/neteq_quality_test.cc:213:
channels_(static_cast<size_t>(FLAGS_channels)),
On 2015/12/29 10:42:47, minyue-webrtc wrote:
> can change the def of FLAGS_channels if you like

Unfortunately I can't; gflags doesn't provide macros to declare size_t FLAGS
variables.

[tlegrand-webrtc, 2016-01-08 08:58:32]

On 2016/01/08 02:45:07, Peter Kasting wrote:
> tina.legrand: Make sure to look over
> webrtc/modules/include/module_common_types.h as well.

Ah, right, I missed that I was asked to review that file too. Nice catch! LGTM
on webrtc/modules/include/module_common_types.h

> henrik.lundin, stefan: Still waiting on your OKs.
> 
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):
> 
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:26: size_t
> samples_per_frame =
> I simplified everything to just do this calculation inline at the callsite,
> since the result is being stored in a size_t anyway.  This looks like how
other
> similar sites do this anyway.
> 
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc (right):
> 
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/codecs/opus/opus_fec_test.cc:70: channels_ =
> static_cast<size_t>(get<0>(GetParam()));
> On 2016/01/04 22:03:18, minyue-webrtc wrote:
> > On 2016/01/04 19:49:42, Peter Kasting wrote:
> > > On 2016/01/04 09:06:02, minyue-webrtc wrote:
> > > > On 2015/12/30 00:55:45, Peter Kasting wrote:
> > > > > On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > > > > > Nit: can redefine this param if you like
> > > > > 
> > > > > I don't understand what "redefine" means here.  What does it mean,
> > > concretely,
> > > > > to redefine this param?
> > > > > 
> > > > > (Same comment other two places)
> > > > 
> > > > See line 25.
> > > 
> > > Oh.  Hmm, I'm not familiar with how TestWithParam<> works.  Can I safely
> just
> > > change the type inside the tuple and it won't affect any other code
anywhere
> > > else (e.g. that invokes this test)?
> > 
> > I think it is just safe to change the first param to size_t, and it looks
> nicer.
> > If you like, please try it.
> 
> Done.
> 
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/codecs/tools/audio_codec_speed_test.cc
(right):
> 
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/codecs/tools/audio_codec_speed_test.cc:41:
channels_
> = static_cast<size_t>(get<0>(GetParam()));
> On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > can change the def of param 0 if you like
> 
> Done.
> 
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/neteq/tools/neteq_quality_test.cc (right):
> 
>
https://codereview.webrtc.org/1316523002/diff/320001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/neteq/tools/neteq_quality_test.cc:213:
> channels_(static_cast<size_t>(FLAGS_channels)),
> On 2015/12/29 10:42:47, minyue-webrtc wrote:
> > can change the def of FLAGS_channels if you like
> 
> Unfortunately I can't; gflags doesn't provide macros to declare size_t FLAGS
> variables.

[stefan-webrtc, 2016-01-08 09:28:38]

lgtm for webrtc/modules/rtp_rtcp/, webrtc/video/

[hlundin-webrtc, 2016-01-08 12:06:18]

webrtc/modules/audio_processing/: lgtm

[Peter Kasting, 2016-01-12 21:59:05]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-01-12 21:59:06]

The patchset sent to the CQ was uploaded after l-g-t-m from henrika@webrtc.org,
minyue@webrtc.org, solenberg@webrtc.org, kjellander@webrtc.org,
perkj@webrtc.org, stefan@webrtc.org, tina.legrand@webrtc.org,
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1316523002/#ps380001 (title:
"Resync")

[commit-bot: I haz the power, 2016-01-12 21:59:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1316523002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1316523002/380001

[commit-bot: I haz the power, 2016-01-12 22:01:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-12 22:01:41]

Try jobs failed on following builders:
  linux_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/11791)
  linux_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/6352)

[Peter Kasting, 2016-01-12 22:57:23]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-01-12 22:57:24]

The patchset sent to the CQ was uploaded after l-g-t-m from henrika@webrtc.org,
stefan@webrtc.org, tina.legrand@webrtc.org, henrik.lundin@webrtc.org,
minyue@webrtc.org, solenberg@webrtc.org, kjellander@webrtc.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1316523002/#ps400001 (title:
"Fix compile")

[commit-bot: I haz the power, 2016-01-12 22:57:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1316523002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1316523002/400001

[Peter Kasting, 2016-01-13 00:27:03]

Description was changed from

==========
Convert channel counts to size_t.

IIRC, this was originally requested by ajm during review of the other size_t
conversions I did over the past year, and I agreed it made sense, but wanted to
do it separately since those changes were already gargantuan.

BUG=chromium:81439
TEST=none
==========

to

==========
Convert channel counts to size_t.

IIRC, this was originally requested by ajm during review of the other size_t
conversions I did over the past year, and I agreed it made sense, but wanted to
do it separately since those changes were already gargantuan.

BUG=chromium:81439
TEST=none
R=henrik.lundin@webrtc.org, henrika@webrtc.org, kjellander@webrtc.org,
minyue@webrtc.org, perkj@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org,
tina.legrand@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/6955870806624479723addfae...
==========

[commit-bot: I haz the power, 2016-01-13 00:27:03]

Description was changed from

==========
Convert channel counts to size_t.

IIRC, this was originally requested by ajm during review of the other size_t
conversions I did over the past year, and I agreed it made sense, but wanted to
do it separately since those changes were already gargantuan.

BUG=chromium:81439
TEST=none
==========

to

==========
Convert channel counts to size_t.

IIRC, this was originally requested by ajm during review of the other size_t
conversions I did over the past year, and I agreed it made sense, but wanted to
do it separately since those changes were already gargantuan.

BUG=chromium:81439
TEST=none
R=henrik.lundin@webrtc.org, henrika@webrtc.org, kjellander@webrtc.org,
minyue@webrtc.org, perkj@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org,
tina.legrand@webrtc.org

Committed: https://crrev.com/6955870806624479723addfae6dcf5d13968796c
Cr-Commit-Position: refs/heads/master@{#11229}
==========

[commit-bot: I haz the power, 2016-01-13 00:27:05]

Patchset 21 (id:??) landed as
https://crrev.com/6955870806624479723addfae6dcf5d13968796c
Cr-Commit-Position: refs/heads/master@{#11229}

[Peter Kasting, 2016-01-13 00:27:05]

Committed patchset #21 (id:400001) manually as
6955870806624479723addfae6dcf5d13968796c (presubmit successful).