Update audio code to use size_t more correctly,

Update audio code to use size_t more correctly,
webrtc/modules/audio_coding/codecs/isac/ 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 19:20:16]

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

[Peter Kasting, 2015-07-27 23:41:51]

pkasting@chromium.org changed reviewers:
+ minyue@webrtc.org, turaj@webrtc.org
- kwiberg@webrtc.org

[Peter Kasting, 2015-07-27 23:42:54]

pkasting@chromium.org changed reviewers:
- turaj@webrtc.org

[Peter Kasting, 2015-07-27 23:43:49]

Changing reviewer to minyue in hopes of finding someone who is not on vacation
to review.

[minyue-webrtc, 2015-07-29 08:34:41]

On 2015/07/27 23:43:49, Peter Kasting wrote:
> Changing reviewer to minyue in hopes of finding someone who is not on vacation
> to review.

Yes, I can do the review.

[minyue-webrtc, 2015-07-31 14:10:14]

Hi Peter,

It is a good effort to unify the variable types. Looking at the CL, I would like
to see clear criteria of what should be changed/what not? I prefer
   a. apply changes to arguments, not on return
   b. apply changes on APIs, not on internal variables (unless necessary)
   c. try to make a whole coverage, this may be hard, we can limit the scope,
but let's define a clear filter.

Look at my comments inline, most of them are a repeat of this general question.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h:61: size_t
Max10MsFramesInAPacket() const override;
I would like to know what determines the choice between int and size_t：
unsigned/signed. 32bit/64bit, or other.

I see that some ints, e.g., the return of SampleRateHz and NumChannels remains
int, although they are supposedly also non-negative.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h:66: size_t
DecodePlc(size_t num_frames, int16_t* decoded) override;
I know that DecodePlc may give non-negative return in current implementation.
But it may potentially return error codes as well, like DecodeInternal, (where
can people know an error otherwise).

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/interface/isac.h (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/interface/isac.h:243: size_t
WebRtcIsac_DecodePlc(
why removing possibility of returning -1?

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/bandwidth_estimator.h
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/bandwidth_estimator.h:98:
const size_t pksize);
why not on frame_length as well? But anyways, back to the general question, what
factors decide whether to size_t or not?

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:219: int
N,                   /* number of input samples */
We have an ‘N’ here too. I think it is hard to cover everything in one CL. But I
would like to know what the CL should cover and how the current changes are
chosen.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/isac.c (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/isac.c:1094: /* return error
code if the packet length is null. */
I am not sure if lenEncodedBytes < 0 has been a valid case. Take some caution.
But I do like the idea of use size_t for lenEncodedBytes

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/isac.c:1113: not fill it in
if it fails and returns -1). */
See that WebRtcIsac_DecodeLb returns -1 was a designed case.

In general, I think it is good and safe to change arguments to unsigned, it
makes them more readable and restrictive.

It might not be best to modify return type.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/isac_unittest.cc
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/isac_unittest.cc:99:
encoded_bytes =  WebRtcIsac_Encode(isac_codec_, speech_data_, bitstream_);
Add EXPECT_GT(encoded_bytes, 0)

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc:46:
size_t i;
Another general point: I think this CL might better focus on the API changes. We
may not need to touch internal codes, which can be too many to do otherwise.

[Peter Kasting, 2015-08-04 02:02:53]

PTAL

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h:61: size_t
Max10MsFramesInAPacket() const override;
On 2015/07/31 14:10:14, minyuel wrote:
> I would like to know what determines the choice between int and size_t：
> unsigned/signed. 32bit/64bit, or other.
> 
> I see that some ints, e.g., the return of SampleRateHz and NumChannels remains
> int, although they are supposedly also non-negative.

size_t should be used for byte or object counts, especially for things in
memory.  Counts of bytes, frames, and channels are all good candidates;
converting NumChannels to size_t is being done in a subsequent patch (so as to
avoid boiling the ocean).  SampleRateHz, by contrast, isn't an object count,
it's simply a numeric quantity, and per the Google style guide, things like that
should remain as ints (or int64_ts or whatever if necessary).  Basically, the
criteria is something to the effect of: what would the STL do?

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h:66: size_t
DecodePlc(size_t num_frames, int16_t* decoded) override;
On 2015/07/31 14:10:14, minyuel wrote:
> I know that DecodePlc may give non-negative return in current implementation.
> But it may potentially return error codes as well, like DecodeInternal, (where
> can people know an error otherwise).

If I'm converting this interface to size_t, it never returns a negative value,
even in error cases, in the current implementation.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/interface/isac.h (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/interface/isac.h:243: size_t
WebRtcIsac_DecodePlc(
On 2015/07/31 14:10:14, minyuel wrote:
> why removing possibility of returning -1?

Because the function cannot actually fail, so it doesn't need the ability to
return -1 (and, lacking that need, the quantity the function is returning is
something best measured with a size_t).

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/bandwidth_estimator.h
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/bandwidth_estimator.h:98:
const size_t pksize);
On 2015/07/31 14:10:14, minyuel wrote:
> why not on frame_length as well? But anyways, back to the general question,
what
> factors decide whether to size_t or not?

According to the comments here, frame_length is in milliseconds, which unless
other factors intruded I would normally measure using an int, as opposed to
being, say, bytes, which should use a size_t.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:219: int
N,                   /* number of input samples */
On 2015/07/31 14:10:14, minyuel wrote:
> We have an ‘N’ here too. I think it is hard to cover everything in one CL. But
I
> would like to know what the CL should cover and how the current changes are
> chosen.

Yes, |N| here should be changed.  Done.  Also fixed the other functions in this
file.

In general the CL covers the transitive closure of changes necessary to make
things compile after changing some of the core decoding functions.  Because even
that set of changes is so extensive (hundreds of files, thousands of lines), no
effort was made to e.g. do a systematic search through every part of the
codebase to look for change candidates, which is why I missed some cases like
this.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/isac.c (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/isac.c:1094: /* return error
code if the packet length is null. */
On 2015/07/31 14:10:14, minyuel wrote:
> I am not sure if lenEncodedBytes < 0 has been a valid case. Take some caution.

If I changed the argument type, then I also changed all the places callers pass
in a value, and so on.  In general, the only way we get negatives for these
sorts of things is in test code which takes arguments from the command line, and
in those cases I added validation.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/isac.c:1113: not fill it in
if it fails and returns -1). */
On 2015/07/31 14:10:14, minyuel wrote:
> See that WebRtcIsac_DecodeLb returns -1 was a designed case.

Yes, and I didn't change its return type.  In fact you'll notice I continue to
explicitly check for a negative return value below, and I handle that correctly.

> In general, I think it is good and safe to change arguments to unsigned, it
> makes them more readable and restrictive.
> 
> It might not be best to modify return type. 

I have only modified return types where doing so is safe.  That's why I didn't
modify them in this case.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/isac_unittest.cc
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/isac_unittest.cc:99:
encoded_bytes =  WebRtcIsac_Encode(isac_codec_, speech_data_, bitstream_);
On 2015/07/31 14:10:14, minyuel wrote:
> Add EXPECT_GT(encoded_bytes, 0)

OK, just try to avoid suggesting I make too many functional changes, as in
theory I'd like this patch to _just_ change types and nothing else.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc:46:
size_t i;
On 2015/07/31 14:10:14, minyuel wrote:
> Another general point: I think this CL might better focus on the API changes.
We
> may not need to touch internal codes, which can be too many to do otherwise.

You can't disentangle the two.  If I change a function prototype, I have to
change the types of all the code that calls that function, or add casts (which
is generally worse).  And then the types of everything that touched those
objects, and so forth transitively.

Here |i| changed because |stream_len| changed, which changed because
WebRtcIsac_UpdateBwEstimate() changed.

[minyue-webrtc, 2015-08-04 07:31:50]

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h:61: size_t
Max10MsFramesInAPacket() const override;
On 2015/08/04 02:02:52, Peter Kasting wrote:
> On 2015/07/31 14:10:14, minyuel wrote:
> > I would like to know what determines the choice between int and size_t：
> > unsigned/signed. 32bit/64bit, or other.
> > 
> > I see that some ints, e.g., the return of SampleRateHz and NumChannels
remains
> > int, although they are supposedly also non-negative.
> 
> size_t should be used for byte or object counts, especially for things in
> memory.  Counts of bytes, frames, and channels are all good candidates;
> converting NumChannels to size_t is being done in a subsequent patch (so as to
> avoid boiling the ocean).  SampleRateHz, by contrast, isn't an object count,
> it's simply a numeric quantity, and per the Google style guide, things like
that
> should remain as ints (or int64_ts or whatever if necessary).  Basically, the
> criteria is something to the effect of: what would the STL do?

Fair enough

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/bandwidth_estimator.h
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/bandwidth_estimator.h:98:
const size_t pksize);
On 2015/08/04 02:02:52, Peter Kasting wrote:
> On 2015/07/31 14:10:14, minyuel wrote:
> > why not on frame_length as well? But anyways, back to the general question,
> what
> > factors decide whether to size_t or not?
> 
> According to the comments here, frame_length is in milliseconds, which unless
> other factors intruded I would normally measure using an int, as opposed to
> being, say, bytes, which should use a size_t.

Fair enough

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:219: int
N,                   /* number of input samples */
On 2015/08/04 02:02:53, Peter Kasting wrote:
> On 2015/07/31 14:10:14, minyuel wrote:
> > We have an ‘N’ here too. I think it is hard to cover everything in one CL.
But
> I
> > would like to know what the CL should cover and how the current changes are
> > chosen.
> 
> Yes, |N| here should be changed.  Done.  Also fixed the other functions in
this
> file.
> 
> In general the CL covers the transitive closure of changes necessary to make
> things compile after changing some of the core decoding functions.  Because
even
> that set of changes is so extensive (hundreds of files, thousands of lines),
no
> effort was made to e.g. do a systematic search through every part of the
> codebase to look for change candidates, which is why I missed some cases like
> this.

"after changing some of the core decoding functions" is fair enough.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/isac.c (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/isac.c:1113: not fill it in
if it fails and returns -1). */
On 2015/08/04 02:02:53, Peter Kasting wrote:
> On 2015/07/31 14:10:14, minyuel wrote:
> > See that WebRtcIsac_DecodeLb returns -1 was a designed case.
> 
> Yes, and I didn't change its return type.  In fact you'll notice I continue to
> explicitly check for a negative return value below, and I handle that
correctly.
> 
> > In general, I think it is good and safe to change arguments to unsigned, it
> > makes them more readable and restrictive.
> > 
> > It might not be best to modify return type. 
> 
> I have only modified return types where doing so is safe.  That's why I didn't
> modify them in this case.

May I suggest not to change return types at all? Current implementation of an
interface that does not return negative values does not mean that it never will.
For example: DecodePlc(), all other DecodeXxx() returns -1 to indicate any
failure, DecodePlc() should also have the capability, although not implemented
that way in current implementation. I am open to discussion.

I am very pleased with changing all bytes-related arguments to size_t.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc:46:
size_t i;
On 2015/08/04 02:02:53, Peter Kasting wrote:
> On 2015/07/31 14:10:14, minyuel wrote:
> > Another general point: I think this CL might better focus on the API
changes.
> We
> > may not need to touch internal codes, which can be too many to do otherwise.
> 
> You can't disentangle the two.  If I change a function prototype, I have to
> change the types of all the code that calls that function, or add casts (which
> is generally worse).  And then the types of everything that touched those
> objects, and so forth transitively.
> 
> Here |i| changed because |stream_len| changed, which changed because
> WebRtcIsac_UpdateBwEstimate() changed.

True. I think this is mainly because of the changes in the return type, which I
am not so sure.

[Peter Kasting, 2015-08-04 18:57:15]

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/isac.c (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/isac.c:1113: not fill it in
if it fails and returns -1). */
On 2015/08/04 07:31:50, minyuel wrote:
> On 2015/08/04 02:02:53, Peter Kasting wrote:
> > On 2015/07/31 14:10:14, minyuel wrote:
> > > See that WebRtcIsac_DecodeLb returns -1 was a designed case.
> > 
> > Yes, and I didn't change its return type.  In fact you'll notice I continue
to
> > explicitly check for a negative return value below, and I handle that
> correctly.
> > 
> > > In general, I think it is good and safe to change arguments to unsigned,
it
> > > makes them more readable and restrictive.
> > > 
> > > It might not be best to modify return type. 
> > 
> > I have only modified return types where doing so is safe.  That's why I
didn't
> > modify them in this case.
> 
> May I suggest not to change return types at all? Current implementation of an
> interface that does not return negative values does not mean that it never
will.
> For example: DecodePlc(), all other DecodeXxx() returns -1 to indicate any
> failure, DecodePlc() should also have the capability, although not implemented
> that way in current implementation. I am open to discussion.

If we need to allow failure in the future we can always make the function do
that at that time.

However, if the function can't fail for now, then returning a size_t is better. 
This makes it clearer that the function will not fail and matches the type
returned to the semantic meaning of the return value.  (If, for example, a
function is returning a byte count, it should ideally return size_t.)  This in
turn reduces the number of casts at callsites and makes calling code less
complex and error-prone (because if the function returns an int, then callers
have to add handler code for negative returns, even if in practice this can't
happen).

Really, the ideal coding pattern would probably be for all these functions to
separate "there was an error" from the return value, e.g. by returning bool and
then having a size_t outparam, or returning a tuple, or (in a world where we
used exceptions) throwing an exception.  Overloading the return value to mean
"an object count, or an error code" results in casting and inclarity, and the
possibility of callers goofing things up by forgetting to handle negative values
(which I encountered several times while writing this patch).  But I think
changing that is a bigger patch and one I don't really want to handle.  For now,
just fixing return types to be size_t where feasible is good enough.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc:46:
size_t i;
On 2015/08/04 07:31:50, minyuel wrote:
> On 2015/08/04 02:02:53, Peter Kasting wrote:
> > On 2015/07/31 14:10:14, minyuel wrote:
> > > Another general point: I think this CL might better focus on the API
> changes.
> > We
> > > may not need to touch internal codes, which can be too many to do
otherwise.
> > 
> > You can't disentangle the two.  If I change a function prototype, I have to
> > change the types of all the code that calls that function, or add casts
(which
> > is generally worse).  And then the types of everything that touched those
> > objects, and so forth transitively.
> > 
> > Here |i| changed because |stream_len| changed, which changed because
> > WebRtcIsac_UpdateBwEstimate() changed.
> 
> True. I think this is mainly because of the changes in the return type, which
I
> am not so sure.

The problem is that values returned by one function are then passed to another. 
Let's say you have code like:

int GetStreamByteLen();
int DoStuff(int stream_byte_len);

int stream_byte_len = GetStreamByteLen();
DoStuff(stream_byte_len);

Now you want to change DoStuff() to take a size_t.  If you are able to also
change GetStreamByteLen, life is good, because the caller looks like this:

size_t stream_byte_len = GetStreamByteLen();
DoStuff(stream_byte_len);

But if not, you have much more complex code:

int stream_byte_len_int = GetStreamByteLen();
if (stream_byte_len_int < 0) {
  HandleError();
  return;
}
size_t stream_byte_len = static_cast<size_t>(stream_byte_len_int);
DoStuff(stream_byte_len);

So not changing return types will in many cases actually result in more problems
and complexity on the caller side, not fewer.

[minyue-webrtc, 2015-08-06 14:07:28]

LG. Just a few comments.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:263: void
WebRtcIsac_Highpass_float(const float *in, double *out, double *state, int N)
why removed? rebase?

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/isac.c (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/isac.c:1113: not fill it in
if it fails and returns -1). */
On 2015/08/04 18:57:15, Peter Kasting wrote:
> On 2015/08/04 07:31:50, minyuel wrote:
> > On 2015/08/04 02:02:53, Peter Kasting wrote:
> > > On 2015/07/31 14:10:14, minyuel wrote:
> > > > See that WebRtcIsac_DecodeLb returns -1 was a designed case.
> > > 
> > > Yes, and I didn't change its return type.  In fact you'll notice I
continue
> to
> > > explicitly check for a negative return value below, and I handle that
> > correctly.
> > > 
> > > > In general, I think it is good and safe to change arguments to unsigned,
> it
> > > > makes them more readable and restrictive.
> > > > 
> > > > It might not be best to modify return type. 
> > > 
> > > I have only modified return types where doing so is safe.  That's why I
> didn't
> > > modify them in this case.
> > 
> > May I suggest not to change return types at all? Current implementation of
an
> > interface that does not return negative values does not mean that it never
> will.
> > For example: DecodePlc(), all other DecodeXxx() returns -1 to indicate any
> > failure, DecodePlc() should also have the capability, although not
implemented
> > that way in current implementation. I am open to discussion.
> 
> If we need to allow failure in the future we can always make the function do
> that at that time.
> 
> However, if the function can't fail for now, then returning a size_t is
better. 
> This makes it clearer that the function will not fail and matches the type
> returned to the semantic meaning of the return value.  (If, for example, a
> function is returning a byte count, it should ideally return size_t.)  This in
> turn reduces the number of casts at callsites and makes calling code less
> complex and error-prone (because if the function returns an int, then callers
> have to add handler code for negative returns, even if in practice this can't
> happen).
If we target at the ideal style in a long run, the logic here is sound.

> 
> Really, the ideal coding pattern would probably be for all these functions to
> separate "there was an error" from the return value, e.g. by returning bool
and
> then having a size_t outparam, or returning a tuple, or (in a world where we
> used exceptions) throwing an exception. 

I agree.

> Overloading the return value to mean
> "an object count, or an error code" results in casting and inclarity, and the
> possibility of callers goofing things up by forgetting to handle negative
values
> (which I encountered several times while writing this patch).  But I think
> changing that is a bigger patch and one I don't really want to handle.  For
now,
> just fixing return types to be size_t where feasible is good enough.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/isac_unittest.cc
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/isac_unittest.cc:99:
encoded_bytes =  WebRtcIsac_Encode(isac_codec_, speech_data_, bitstream_);
On 2015/08/04 02:02:53, Peter Kasting wrote:
> On 2015/07/31 14:10:14, minyuel wrote:
> > Add EXPECT_GT(encoded_bytes, 0)
> 
> OK, just try to avoid suggesting I make too many functional changes, as in
> theory I'd like this patch to _just_ change types and nothing else.

Ok, I think it was missing. But I also want to make sure that encoded_bytes did
not go negative before it is casted to size_t. Thanks for adding the line.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc:46:
size_t i;
On 2015/08/04 18:57:15, Peter Kasting wrote:
> On 2015/08/04 07:31:50, minyuel wrote:
> > On 2015/08/04 02:02:53, Peter Kasting wrote:
> > > On 2015/07/31 14:10:14, minyuel wrote:
> > > > Another general point: I think this CL might better focus on the API
> > changes.
> > > We
> > > > may not need to touch internal codes, which can be too many to do
> otherwise.
> > > 
> > > You can't disentangle the two.  If I change a function prototype, I have
to
> > > change the types of all the code that calls that function, or add casts
> (which
> > > is generally worse).  And then the types of everything that touched those
> > > objects, and so forth transitively.
> > > 
> > > Here |i| changed because |stream_len| changed, which changed because
> > > WebRtcIsac_UpdateBwEstimate() changed.
> > 
> > True. I think this is mainly because of the changes in the return type,
which
> I
> > am not so sure.
> 
> The problem is that values returned by one function are then passed to
another. 
> Let's say you have code like:
> 
> int GetStreamByteLen();
> int DoStuff(int stream_byte_len);
> 
> int stream_byte_len = GetStreamByteLen();
> DoStuff(stream_byte_len);
> 
> Now you want to change DoStuff() to take a size_t.  If you are able to also
> change GetStreamByteLen, life is good, because the caller looks like this:
> 
> size_t stream_byte_len = GetStreamByteLen();
> DoStuff(stream_byte_len);
> 
> But if not, you have much more complex code:
> 
> int stream_byte_len_int = GetStreamByteLen();
> if (stream_byte_len_int < 0) {
>   HandleError();
>   return;
> }
> size_t stream_byte_len = static_cast<size_t>(stream_byte_len_int);
> DoStuff(stream_byte_len);
> 
> So not changing return types will in many cases actually result in more
problems
> and complexity on the caller side, not fewer.

I was actually thinking

1. int stream_byte_len = GetStreamByteLen();
2. if (stream_byte_len < 0) {
3.   HandleError();
4.   return;
5. }
6. DoStuff(static_cast<size_t>(stream_byte_len));

Since 1~5 should have been there, what needed is a static_cast.

Anyways, since you convinced me that changing some return types is beneficial,
this also goes away.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc:714:
stream_len = static_cast<size_t>(WebRtcIsac_GetRedPayload(
Can WebRtcIsac_GetRedPayload return -1? Wan't it handled in our code? Can you
write a CHECK() here?

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c:409:
(size_t)rcuStreamLen, decoded,
no static_cast?

https://codereview.webrtc.org/1228793004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:25: int
orderCoef){
a space before {

https://codereview.webrtc.org/1228793004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:64:
double *Out){
same

https://codereview.webrtc.org/1228793004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:92:
double *Out){
same

https://codereview.webrtc.org/1228793004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:267: {
place { on 266 if you can

[Peter Kasting, 2015-08-06 18:36:49]

New snap not yet up, will fix the below issues later (am WFH today)

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:263: void
WebRtcIsac_Highpass_float(const float *in, double *out, double *state, int N)
On 2015/08/06 14:07:27, minyue-webrtc wrote:
> why removed? rebase?

Because this function is never called anywhere.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc:714:
stream_len = static_cast<size_t>(WebRtcIsac_GetRedPayload(
On 2015/08/06 14:07:28, minyue-webrtc wrote:
> Can WebRtcIsac_GetRedPayload return -1? Wan't it handled in our code? Can you
> write a CHECK() here?

Right, the old code didn't handle it, and since this was just test code, I
didn't bother to add new checks.  I can add one.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c:409:
(size_t)rcuStreamLen, decoded,
On 2015/08/06 14:07:28, minyue-webrtc wrote:
> no static_cast?

This is a C file, not C++.

https://codereview.webrtc.org/1228793004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:25: int
orderCoef){
On 2015/08/06 14:07:28, minyue-webrtc wrote:
> a space before {

Sure, I was just leaving style as much the way I found it as possible.  (I could
also fix the *s to be on the typename and not the variables, for example.)

[Peter Kasting, 2015-08-07 21:52:12]

(No response needed)

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc:714:
stream_len = static_cast<size_t>(WebRtcIsac_GetRedPayload(
On 2015/08/06 18:36:49, Peter Kasting wrote:
> On 2015/08/06 14:07:28, minyue-webrtc wrote:
> > Can WebRtcIsac_GetRedPayload return -1? Wan't it handled in our code? Can
you
> > write a CHECK() here?
> 
> Right, the old code didn't handle it, and since this was just test code, I
> didn't bother to add new checks.  I can add one.

Added a check that handles errors similarly to how the code just below handles
GetNewBitStream errors.

https://codereview.webrtc.org/1228793004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:25: int
orderCoef){
On 2015/08/06 18:36:49, Peter Kasting wrote:
> On 2015/08/06 14:07:28, minyue-webrtc wrote:
> > a space before {
> 
> Sure, I was just leaving style as much the way I found it as possible.  (I
could
> also fix the *s to be on the typename and not the variables, for example.)

Fixed a variety of issues with the function declarations in this file (* on type
name, space before {, { on same line as last arg, no more than two blank lines
between functions)

[Peter Kasting, 2015-08-07 21:57:19]

Actually, a response is needed, since you said "LG" instead of "LGTM".

[minyue-webrtc, 2015-08-10 07:34:15]

only a couple of nits

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:263: void
WebRtcIsac_Highpass_float(const float *in, double *out, double *state, int N)
On 2015/08/06 18:36:49, Peter Kasting wrote:
> On 2015/08/06 14:07:27, minyue-webrtc wrote:
> > why removed? rebase?
> 
> Because this function is never called anywhere.

Better not to delete this in this CL.

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c (right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/simpleKenny.c:409:
(size_t)rcuStreamLen, decoded,
On 2015/08/06 18:36:49, Peter Kasting wrote:
> On 2015/08/06 14:07:28, minyue-webrtc wrote:
> > no static_cast?
> 
> This is a C file, not C++.

Oh, right!

https://codereview.webrtc.org/1228793004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:25: int
orderCoef){
On 2015/08/07 21:52:12, Peter Kasting wrote:
> On 2015/08/06 18:36:49, Peter Kasting wrote:
> > On 2015/08/06 14:07:28, minyue-webrtc wrote:
> > > a space before {
> > 
> > Sure, I was just leaving style as much the way I found it as possible.  (I
> could
> > also fix the *s to be on the typename and not the variables, for example.)
> 
> Fixed a variety of issues with the function declarations in this file (* on
type
> name, space before {, { on same line as last arg, no more than two blank lines
> between functions)

Nice! Thanks.

https://codereview.webrtc.org/1228793004/diff/60001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc
(right):

https://codereview.webrtc.org/1228793004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc:719:
};
remove ;

[Peter Kasting, 2015-08-10 08:19:17]

Can you please LGTM me if you trust me to make changes you're asking for, so I
don't have to keep ping-ponging you from a different time zone?

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:263: void
WebRtcIsac_Highpass_float(const float *in, double *out, double *state, int N)
On 2015/08/10 07:34:14, minyue-webrtc wrote:
> On 2015/08/06 18:36:49, Peter Kasting wrote:
> > On 2015/08/06 14:07:27, minyue-webrtc wrote:
> > > why removed? rebase?
> > 
> > Because this function is never called anywhere.
> 
> Better not to delete this in this CL.

Why?  I don't want to update the API and code to match the other changes, I
don't want to leave it as is, and there's no downside to deleting it; it's dead
code.

https://codereview.webrtc.org/1228793004/diff/60001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc
(right):

https://codereview.webrtc.org/1228793004/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/test/ReleaseTest-API/ReleaseTest-API.cc:719:
};
On 2015/08/10 07:34:14, minyue-webrtc wrote:
> remove ;

Oops

[minyue-webrtc, 2015-08-10 08:23:27]

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c
(right):

https://codereview.webrtc.org/1228793004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/main/source/filter_functions.c:263: void
WebRtcIsac_Highpass_float(const float *in, double *out, double *state, int N)
On 2015/08/10 08:19:16, Peter Kasting wrote:
> On 2015/08/10 07:34:14, minyue-webrtc wrote:
> > On 2015/08/06 18:36:49, Peter Kasting wrote:
> > > On 2015/08/06 14:07:27, minyue-webrtc wrote:
> > > > why removed? rebase?
> > > 
> > > Because this function is never called anywhere.
> > 
> > Better not to delete this in this CL.
> 
> Why?  I don't want to update the API and code to match the other changes, I
> don't want to leave it as is, and there's no downside to deleting it; it's
dead
> code.

I feel that this does not live up to the main aim of this CL, as the title says
"use size_t more correctly"

[minyue-webrtc, 2015-08-10 08:30:12]

Yes, I can leave the remaining question for you to decide. It is ready of LGTM.

[Peter Kasting, 2015-08-14 01:02:24]

FYI, I updated this per your comments.  (No response needed)

[minyue-webrtc, 2015-08-14 08:09:00]

On 2015/08/14 01:02:24, Peter Kasting wrote:
> FYI, I updated this per your comments.  (No response needed)

Thanks!