Update audio code to use size_t more correctly,

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

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

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

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

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

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

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

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

[Peter Kasting, 2015-08-07 22:01:42]

Ping

[minyue-webrtc, 2015-08-12 16:08:08]

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/interface/isacfix.h (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/interface/isacfix.h:194: *      -
packet_size       : size of the packet.
could you update the comments for |packet_size| with its unit, like "size of the
packet in bytes" or "bytes of the packet",

also similar places.

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/codec.h (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/codec.h:144: void
WebRtcIsacfix_NormLatticeFilterMa(size_t orderCoef,
not sure if orderCoef is a count of objects

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/isacfix.c (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/isacfix.c:871: const uint16_t
  *encoded,
It seems that the spacing here was not good. Would you arrange them a bit? also
other functions that the CL touches.

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/lattice_mips.c (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/lattice_mips.c:24: int count
= (int)(order_coef - 1);
I do not know what if order_coef = 0 here (but that is not related to this CL)

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/pitch_filter.c (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/pitch_filter.c:37: static
__inline size_t CalcLrIntQ(int16_t fixVal,
I do not quite get the point of changes in this file.

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h:37: size_t
stream_size;  /* The size of stream. */
could you add a unit in the comment

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h:182: size_t
stretchLag;
I am not sure if Lag is a count. What unit does it have? Can it be negative
(non-causal)?

[Peter Kasting, 2015-08-14 22:32:26]

Please try to refrain from asking me to fix unrelated style and comment issues
while I'm touching files -- remember that the overall patch here is hundreds of
files and thousands of lines, and every such change both increases the
likelihood I'll screw something up and break things, and means future spelunkers
will see more lines changed than necessary when going through git blame.

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/interface/isacfix.h (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/interface/isacfix.h:194: *      -
packet_size       : size of the packet.
On 2015/08/12 16:08:08, minyue-webrtc wrote:
> could you update the comments for |packet_size| with its unit, like "size of
the
> packet in bytes" or "bytes of the packet",
> 
> also similar places.

Done (in this file)

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/codec.h (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/codec.h:144: void
WebRtcIsacfix_NormLatticeFilterMa(size_t orderCoef,
On 2015/08/12 16:08:08, minyue-webrtc wrote:
> not sure if orderCoef is a count of objects

Based on how it's used in these functions, it is; specifically, it looks like
the size of |filt_coefQ15|.

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/isacfix.c (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/isacfix.c:871: const uint16_t
  *encoded,
On 2015/08/12 16:08:08, minyue-webrtc wrote:
> It seems that the spacing here was not good. Would you arrange them a bit?
also
> other functions that the CL touches.

Done (in this file)

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/lattice_mips.c (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/lattice_mips.c:24: int count
= (int)(order_coef - 1);
On 2015/08/12 16:08:08, minyue-webrtc wrote:
> I do not know what if order_coef = 0 here (but that is not related to this CL)

In practice this never happens; we always pass in ORDERLO or ORDERHI (12 and 6,
respectively).

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/pitch_filter.c (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/pitch_filter.c:37: static
__inline size_t CalcLrIntQ(int16_t fixVal,
On 2015/08/12 16:08:08, minyue-webrtc wrote:
> I do not quite get the point of changes in this file.

Returning a size_t here matches caller usage in general but especially is
necessary to match the change in signature to WebRtcIsacfix_PitchFilterCore()
(since WebRtcIsacfix_PitchFilter() passes the return value here directly to that
function).  Most of the rest of the changes are to accommodate that.  Changing
|fixVal| to int16_t here is simply to match what all the callers are actually
doing.

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h:37: size_t
stream_size;  /* The size of stream. */
On 2015/08/12 16:08:08, minyue-webrtc wrote:
> could you add a unit in the comment

Done.

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h:182: size_t
stretchLag;
On 2015/08/12 16:08:08, minyue-webrtc wrote:
> I am not sure if Lag is a count. What unit does it have?

From reading caller code, I believe it's a sample count.

> Can it be negative
> (non-causal)? 

It better not, or the existing code is broken, since in many places it indexes
arrays by this value.

[hlundin-webrtc, 2015-08-17 08:13:21]

henrik.lundin@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[hlundin-webrtc, 2015-08-17 08:13:22]

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h:182: size_t
stretchLag;
On 2015/08/14 22:32:26, Peter Kasting wrote:
> On 2015/08/12 16:08:08, minyue-webrtc wrote:
> > I am not sure if Lag is a count. What unit does it have?
> 
> From reading caller code, I believe it's a sample count.
> 
> > Can it be negative
> > (non-causal)? 
> 
> It better not, or the existing code is broken, since in many places it indexes
> arrays by this value.

I agree with Minyue, Even though I believe it is always a positive value, you
will be introducing new inconsistencies if you change this. strechLag is often
given its value from another int16_t variable (lastPitchLag_Q7), which may
ripple even further.

Also, I recall that some of the signal processing code in WebRTC was written
with negative indexing. So just because it is used to index arrays doesn't
necessarily mean it is non-negative.

[hlundin-webrtc, 2015-08-17 08:13:35]

henrik.lundin@webrtc.org changed reviewers:
- henrik.lundin@webrtc.org

[Peter Kasting, 2015-08-17 08:23:53]

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h (right):

https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h:182: size_t
stretchLag;
On 2015/08/17 08:13:21, hlundin-webrtc wrote:
> On 2015/08/14 22:32:26, Peter Kasting wrote:
> > On 2015/08/12 16:08:08, minyue-webrtc wrote:
> > > I am not sure if Lag is a count. What unit does it have?
> > 
> > From reading caller code, I believe it's a sample count.
> > 
> > > Can it be negative
> > > (non-causal)? 
> > 
> > It better not, or the existing code is broken, since in many places it
indexes
> > arrays by this value.
> 
> I agree with Minyue, Even though I believe it is always a positive value, you
> will be introducing new inconsistencies if you change this. strechLag is often
> given its value from another int16_t variable (lastPitchLag_Q7), which may
> ripple even further.
> 
> Also, I recall that some of the signal processing code in WebRTC was written
> with negative indexing. So just because it is used to index arrays doesn't
> necessarily mean it is non-negative.

There are a number of lag-related variables I've changed to be size_t.  I don't
think changing this back in isolation makes sense, nor does trying to change all
these (as they interact with a variety of other variables that are also now
size_t).  And at the core I don't think such a change makes sense semantically;
in the cases I've changed these, I believe these _should_ be size_t, as they are
e.g. sample counts.

I intentionally didn't change lastPitchLag_Q7 as its units are not precisely
"sample counts" (due to shifting), and changing it turned out to introduce more
problems.  However, I did check its uses.  We only read its value in one place,
and I cast back to size_t appropriately at that spot.

Regarding negative indexing, there is at least one place where I caught that and
accommodated it, but its use should be very rare, and it isn't happening here as
some code does things like indexing stack-allocated buffers (so a negative index
is clearly wrong by inspection).

I tried multiple solutions and thought through this quite a bit while
constructing this part of the change set, and I stand by it.

[minyue-webrtc, 2015-08-17 15:46:57]

On 2015/08/17 08:23:53, Peter Kasting wrote:
>
https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
> File webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h (right):
> 
>
https://codereview.chromium.org/1227163003/diff/20001/webrtc/modules/audio_co...
> webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h:182: size_t
> stretchLag;
> On 2015/08/17 08:13:21, hlundin-webrtc wrote:
> > On 2015/08/14 22:32:26, Peter Kasting wrote:
> > > On 2015/08/12 16:08:08, minyue-webrtc wrote:
> > > > I am not sure if Lag is a count. What unit does it have?
> > > 
> > > From reading caller code, I believe it's a sample count.
> > > 
> > > > Can it be negative
> > > > (non-causal)? 
> > > 
> > > It better not, or the existing code is broken, since in many places it
> indexes
> > > arrays by this value.
> > 
> > I agree with Minyue, Even though I believe it is always a positive value,
you
> > will be introducing new inconsistencies if you change this. strechLag is
often
> > given its value from another int16_t variable (lastPitchLag_Q7), which may
> > ripple even further.
> > 
> > Also, I recall that some of the signal processing code in WebRTC was written
> > with negative indexing. So just because it is used to index arrays doesn't
> > necessarily mean it is non-negative.
> 
> There are a number of lag-related variables I've changed to be size_t.  I
don't
> think changing this back in isolation makes sense, nor does trying to change
all
> these (as they interact with a variety of other variables that are also now
> size_t).  And at the core I don't think such a change makes sense
semantically;
> in the cases I've changed these, I believe these _should_ be size_t, as they
are
> e.g. sample counts.
> 
> I intentionally didn't change lastPitchLag_Q7 as its units are not precisely
> "sample counts" (due to shifting), and changing it turned out to introduce
more
> problems.  However, I did check its uses.  We only read its value in one
place,
> and I cast back to size_t appropriately at that spot.
> 
> Regarding negative indexing, there is at least one place where I caught that
and
> accommodated it, but its use should be very rare, and it isn't happening here
as
> some code does things like indexing stack-allocated buffers (so a negative
index
> is clearly wrong by inspection).
> 
> I tried multiple solutions and thought through this quite a bit while
> constructing this part of the change set, and I stand by it.

I think can live with this solution. I can give LGTM. But please wait for
Henrik's feedback on this.

[hlundin-webrtc, 2015-08-18 09:03:16]

henrik.lundin@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[hlundin-webrtc, 2015-08-18 09:03:16]

I'm fine with this change, too.
LGTM.

Thanks for your efforts, Peter!

https://codereview.webrtc.org/1227163003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h (right):

https://codereview.webrtc.org/1227163003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/fix/source/structs.h:182: size_t
stretchLag;
On 2015/08/17 08:23:53, Peter Kasting wrote:
> On 2015/08/17 08:13:21, hlundin-webrtc wrote:
> > On 2015/08/14 22:32:26, Peter Kasting wrote:
> > > On 2015/08/12 16:08:08, minyue-webrtc wrote:
> > > > I am not sure if Lag is a count. What unit does it have?
> > > 
> > > From reading caller code, I believe it's a sample count.
> > > 
> > > > Can it be negative
> > > > (non-causal)? 
> > > 
> > > It better not, or the existing code is broken, since in many places it
> indexes
> > > arrays by this value.
> > 
> > I agree with Minyue, Even though I believe it is always a positive value,
you
> > will be introducing new inconsistencies if you change this. strechLag is
often
> > given its value from another int16_t variable (lastPitchLag_Q7), which may
> > ripple even further.
> > 
> > Also, I recall that some of the signal processing code in WebRTC was written
> > with negative indexing. So just because it is used to index arrays doesn't
> > necessarily mean it is non-negative.
> 
> There are a number of lag-related variables I've changed to be size_t.  I
don't
> think changing this back in isolation makes sense, nor does trying to change
all
> these (as they interact with a variety of other variables that are also now
> size_t).  And at the core I don't think such a change makes sense
semantically;
> in the cases I've changed these, I believe these _should_ be size_t, as they
are
> e.g. sample counts.
> 
> I intentionally didn't change lastPitchLag_Q7 as its units are not precisely
> "sample counts" (due to shifting), and changing it turned out to introduce
more
> problems.  However, I did check its uses.  We only read its value in one
place,
> and I cast back to size_t appropriately at that spot.
> 
> Regarding negative indexing, there is at least one place where I caught that
and
> accommodated it, but its use should be very rare, and it isn't happening here
as
> some code does things like indexing stack-allocated buffers (so a negative
index
> is clearly wrong by inspection).
> 
> I tried multiple solutions and thought through this quite a bit while
> constructing this part of the change set, and I stand by it.

Acknowledged.