Update audio code to use size_t more correctly,

Update audio code to use size_t more correctly,
webrtc/modules/audio_coding/neteq/ 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:37:04]

pkasting@chromium.org changed reviewers:
+ jan.skoglund@webrtc.org

[Peter Kasting, 2015-07-23 19:37:05]

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/background_noise.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/background_noise.cc:25: const size_t
BackgroundNoise::kMaxLpcOrder;
Changing the type of this from int to size_t means some compilers will complain
if it doesn't have an explicit definition in addition to the declaration. 
constexpr might be able to address this if only it worked in MSVC.

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

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

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

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

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

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

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

pkasting@chromium.org changed reviewers:
+ henrik.lundin@webrtc.org, tina.legrand@webrtc.org
- turaj@webrtc.org

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

Changing reviewers again; Tina or Henrik, could one of you review?

[hlundin-webrtc, 2015-08-10 11:30:02]

Thanks for fixing up the code!

I mainly have two comments on this CL, but they repeat several times (I have
certainly missed a few):

1. I don't think the variables names "fs_mult" should be size_t. They are a
factor derived from the sample rate, not a length, count of bytes, or index into
an array.

2. Please use rtc::checked_cast when casting down from size_t to (potentially)
smaller types.

Thanks!

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/background_noise.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/background_noise.cc:25: const size_t
BackgroundNoise::kMaxLpcOrder;
On 2015/07/23 19:37:05, Peter Kasting wrote:
> Changing the type of this from int to size_t means some compilers will
complain
> if it doesn't have an explicit definition in addition to the declaration. 
> constexpr might be able to address this if only it worked in MSVC.

Acknowledged.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/background_noise.cc:253: parameters.scale =
static_cast<int16_t>(WebRtcSpl_SqrtFloor(residual_energy));
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/decision_logic.h (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decision_logic.h:151: size_t fs_mult_;
I don't think this is should be represented as a size_t. The values are only
going to be 1, 2, 4, or 6, but it feels weird to represent a sample rate as a
size_t.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/dsp_helper.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/dsp_helper.cc:103: size_t num_peaks, size_t
fs_mult,
Again, fs_mult.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/dsp_helper.cc:150: void
DspHelper::ParabolicFit(int16_t* signal_points, size_t fs_mult,
fs_mult

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/dsp_helper.h (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/dsp_helper.h:82: size_t num_peaks, size_t
fs_mult,
Again, fs_mult should not be a size_t.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/dsp_helper.h:91: static void
ParabolicFit(int16_t* signal_points, size_t fs_mult,
And here.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:210: (31 -
WebRtcSpl_NormW32(static_cast<int32_t>(max_lag_))) - 5;
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:446: (31 -
WebRtcSpl_NormW32(static_cast<int32_t>(correlation_length))) - 31;
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:520: static_cast<int16_t>(energy2 >>
scaled_energy2));
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:523:
static_cast<int16_t>(WebRtcSpl_SqrtFloor(energy_ratio << 13));
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/merge.h (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/merge.h:91: const size_t fs_mult_;  // fs_hz_
/ 8000.
Not size_t

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:944: -(samples_left +
static_cast<int>(output_size_samples_)));
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:969: if (samples_left >=
static_cast<int>(output_size_samples_) &&
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1296: decode_length =
static_cast<int>(decoder_frame_length_);
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1841: packet_duration =
static_cast<int>(decoder_frame_length_);
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1861: packet_duration =
static_cast<int>(decoder_frame_length_);
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1880: } while (extracted_samples
< static_cast<int>(required_samples) &&
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.h (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.h:377: size_t fs_mult_
GUARDED_BY(crit_sect_);
Not this one.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:362: output_sample_rate_ =
static_cast<int>(*out_len / 10 * 1000);
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/normal.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/normal.cc:48: const size_t fs_mult = fs_hz_ /
8000;
not size_t

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/preemptive_expand.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/preemptive_expand.cc:67: size_t fs_mult_120 =
fs_mult_ * 120;
Keep the int.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/statistics_calculator.cc:126: const int
ms_per_packet = static_cast<int>(
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/test/RTPencode.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/test/RTPencode.cc:1: /*
Just FYI: This is very old and hairy code. Here be dragons...

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/test/neteq_opus_quality_test.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/test/neteq_opus_quality_test.cc:197:
static_cast<opus_int32>(max_bytes));
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/time_stretch.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/time_stretch.cc:28: size_t fs_mult_120 =
fs_mult_ * 120;  // Corresponds to 15 ms.
int

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/time_stretch.cc:80:
WebRtcSpl_NormW32(static_cast<int32_t>(peak_index));
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/time_stretch.cc:198: right_side =
static_cast<int32_t>(peak_index) * (right_side >> right_scale);
rtc::checked_cast

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/time_stretch.h (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/time_stretch.h:93: const size_t fs_mult_;  //
Sample rate multiplier = sample_rate_hz_ / 8000.
int

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:568:
static_cast<int>(1000 * samples_per_channel / kOutputBlockSizeMs);
rtc::checked_cast

[Peter Kasting, 2015-08-17 22:49:47]

Thanks for the detailed review!

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/background_noise.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/background_noise.cc:253: parameters.scale =
static_cast<int16_t>(WebRtcSpl_SqrtFloor(residual_energy));
On 2015/08/10 11:30:00, hlundin-webrtc wrote:
> rtc::checked_cast

This shouldn't be necessary, since by definition the square root of a 32-bit
quantity must fit in 16 bits.

If you would like, I could make this clearer by (separately) changing the
signature of WebRtcSpl_SqrtFloor() to return an int16_t.  It looks like we have
many places in the code which already unconditionally cast this return value to
int16_t, so this would enable removing all those casts.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/decision_logic.h (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decision_logic.h:151: size_t fs_mult_;
On 2015/08/10 11:30:00, hlundin-webrtc wrote:
> I don't think this is should be represented as a size_t. The values are only
> going to be 1, 2, 4, or 6, but it feels weird to represent a sample rate as a
> size_t.

|fs_mult| seems like a unitless quantity to me (not even a sample rate), which
is probably best represented as an int.  Changed back to int here and everywhere
else in the patch.

However, most of the places where we did something like:

  const size_t fs_mult_120 = 120 * fs_mult;

I left as size_t instead of making int.  In these cases, the usage of the
constants suggested that the units were things like sample or byte counts, which
do make sense to be size_t (and leaving as int would generally have meant
casting to size_t at every usage point).

This inconsistency between the meaning/type of |fs_mult| and |fs_mult_xxx| is
why I originally made |fs_mult| size_t as well, but I think probably having the
types differ, while a bit odd at first, is more semantically correct.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:210: (31 -
WebRtcSpl_NormW32(static_cast<int32_t>(max_lag_))) - 5;
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

Done.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:446: (31 -
WebRtcSpl_NormW32(static_cast<int32_t>(correlation_length))) - 31;
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

This shouldn't be necessary, as we're guaranteed that |correlation_length| is
between fs_mult * 60 and fs_mult * 120, inclusive, which should be well within
the range of int32_t.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:520: static_cast<int16_t>(energy2 >>
scaled_energy2));
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

This shouldn't be necessary, as the shift result here is guaranteed to fit in a
signed 16-bit value due to how |scaled_energy2| is calculated.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:523:
static_cast<int16_t>(WebRtcSpl_SqrtFloor(energy_ratio << 13));
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

See previous comment on WebRtcSpl_SqrtFloor().

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:944: -(samples_left +
static_cast<int>(output_size_samples_)));
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

Done.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:969: if (samples_left >=
static_cast<int>(output_size_samples_) &&
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

Done.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1296: decode_length =
static_cast<int>(decoder_frame_length_);
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

Done.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1841: packet_duration =
static_cast<int>(decoder_frame_length_);
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

Done.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1861: packet_duration =
static_cast<int>(decoder_frame_length_);
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

Done.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1880: } while (extracted_samples
< static_cast<int>(required_samples) &&
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

Done.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:362: output_sample_rate_ =
static_cast<int>(*out_len / 10 * 1000);
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

This shouldn't be necessary, as the ASSERT_TRUE just above guarantees that the
value of this expression is at most 32000.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/preemptive_expand.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/preemptive_expand.cc:67: size_t fs_mult_120 =
fs_mult_ * 120;
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> Keep the int.

This is an example of a place where I left |fs_mult_120| as size_t.  See how its
use below indicates that its units are the same as those for
|old_data_length_per_channel_| (which should definitely be a size_t).

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/statistics_calculator.cc:126: const int
ms_per_packet = static_cast<int>(
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

Done.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/test/neteq_opus_quality_test.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/test/neteq_opus_quality_test.cc:197:
static_cast<opus_int32>(max_bytes));
On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> rtc::checked_cast

This shouldn't be necessary, as if |max_bytes| is excessively long and the cast
truncates, the function will simply return OPUS_BUFFER_TOO_SMALL or fail to
properly convert, and the test will likely fail on the EXPECT_GE below (or else
should fail after returning).

In fact, it's probably better to not do a checked_cast here, as failing that
would crash the test instead; we always want tests to fail rather than crashing.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/time_stretch.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/time_stretch.cc:80:
WebRtcSpl_NormW32(static_cast<int32_t>(peak_index));
On 2015/08/10 11:30:02, hlundin-webrtc wrote:
> rtc::checked_cast

This shouldn't be necessary, as the asserts above guarantee that peak_index <=
119 * fs_mult_, which should fit in an int32_t.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/time_stretch.cc:198: right_side =
static_cast<int32_t>(peak_index) * (right_side >> right_scale);
On 2015/08/10 11:30:02, hlundin-webrtc wrote:
> rtc::checked_cast

While today this shouldn't be necessary as |peak_index| is guaranteed to fit in
an int32_t by the caller (see reply above), I suppose in the future someone
could change that.  I could change the function to take an int32_t instead, but
that seems more confusing, when the rest of the file uses a size_t for this
concept.  So, done.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:568:
static_cast<int>(1000 * samples_per_channel / kOutputBlockSizeMs);
On 2015/08/10 11:30:02, hlundin-webrtc wrote:
> rtc::checked_cast

Done.

[hlundin-webrtc, 2015-08-18 07:19:19]

Thanks, Peter. LGTM.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/background_noise.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/background_noise.cc:253: parameters.scale =
static_cast<int16_t>(WebRtcSpl_SqrtFloor(residual_energy));
On 2015/08/17 22:49:46, Peter Kasting wrote:
> On 2015/08/10 11:30:00, hlundin-webrtc wrote:
> > rtc::checked_cast
> 
> This shouldn't be necessary, since by definition the square root of a 32-bit
> quantity must fit in 16 bits.
> 
> If you would like, I could make this clearer by (separately) changing the
> signature of WebRtcSpl_SqrtFloor() to return an int16_t.  It looks like we
have
> many places in the code which already unconditionally cast this return value
to
> int16_t, so this would enable removing all those casts.

That seems like a good idea, if you don't mind.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/decision_logic.h (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/decision_logic.h:151: size_t fs_mult_;
On 2015/08/17 22:49:46, Peter Kasting wrote:
> On 2015/08/10 11:30:00, hlundin-webrtc wrote:
> > I don't think this is should be represented as a size_t. The values are only
> > going to be 1, 2, 4, or 6, but it feels weird to represent a sample rate as
a
> > size_t.
> 
> |fs_mult| seems like a unitless quantity to me (not even a sample rate), which
> is probably best represented as an int.  Changed back to int here and
everywhere
> else in the patch.
> 
> However, most of the places where we did something like:
> 
>   const size_t fs_mult_120 = 120 * fs_mult;
> 
> I left as size_t instead of making int.  In these cases, the usage of the
> constants suggested that the units were things like sample or byte counts,
which
> do make sense to be size_t (and leaving as int would generally have meant
> casting to size_t at every usage point).
> 
> This inconsistency between the meaning/type of |fs_mult| and |fs_mult_xxx| is
> why I originally made |fs_mult| size_t as well, but I think probably having
the
> types differ, while a bit odd at first, is more semantically correct.

Great. Thanks!

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:446: (31 -
WebRtcSpl_NormW32(static_cast<int32_t>(correlation_length))) - 31;
On 2015/08/17 22:49:46, Peter Kasting wrote:
> On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> > rtc::checked_cast
> 
> This shouldn't be necessary, as we're guaranteed that |correlation_length| is
> between fs_mult * 60 and fs_mult * 120, inclusive, which should be well within
> the range of int32_t.

Acknowledged.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:520: static_cast<int16_t>(energy2 >>
scaled_energy2));
On 2015/08/17 22:49:46, Peter Kasting wrote:
> On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> > rtc::checked_cast
> 
> This shouldn't be necessary, as the shift result here is guaranteed to fit in
a
> signed 16-bit value due to how |scaled_energy2| is calculated.

Acknowledged.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/expand.cc:523:
static_cast<int16_t>(WebRtcSpl_SqrtFloor(energy_ratio << 13));
On 2015/08/17 22:49:46, Peter Kasting wrote:
> On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> > rtc::checked_cast
> 
> See previous comment on WebRtcSpl_SqrtFloor().

Acknowledged.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:362: output_sample_rate_ =
static_cast<int>(*out_len / 10 * 1000);
On 2015/08/17 22:49:47, Peter Kasting wrote:
> On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> > rtc::checked_cast
> 
> This shouldn't be necessary, as the ASSERT_TRUE just above guarantees that the
> value of this expression is at most 32000.

Acknowledged.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/preemptive_expand.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/preemptive_expand.cc:67: size_t fs_mult_120 =
fs_mult_ * 120;
On 2015/08/17 22:49:47, Peter Kasting wrote:
> On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> > Keep the int.
> 
> This is an example of a place where I left |fs_mult_120| as size_t.  See how
its
> use below indicates that its units are the same as those for
> |old_data_length_per_channel_| (which should definitely be a size_t).

Acknowledged.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/test/neteq_opus_quality_test.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/test/neteq_opus_quality_test.cc:197:
static_cast<opus_int32>(max_bytes));
On 2015/08/17 22:49:47, Peter Kasting wrote:
> On 2015/08/10 11:30:01, hlundin-webrtc wrote:
> > rtc::checked_cast
> 
> This shouldn't be necessary, as if |max_bytes| is excessively long and the
cast
> truncates, the function will simply return OPUS_BUFFER_TOO_SMALL or fail to
> properly convert, and the test will likely fail on the EXPECT_GE below (or
else
> should fail after returning).
> 
> In fact, it's probably better to not do a checked_cast here, as failing that
> would crash the test instead; we always want tests to fail rather than
crashing.

Acknowledged.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/time_stretch.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/time_stretch.cc:80:
WebRtcSpl_NormW32(static_cast<int32_t>(peak_index));
On 2015/08/17 22:49:47, Peter Kasting wrote:
> On 2015/08/10 11:30:02, hlundin-webrtc wrote:
> > rtc::checked_cast
> 
> This shouldn't be necessary, as the asserts above guarantee that peak_index <=
> 119 * fs_mult_, which should fit in an int32_t.

Acknowledged.

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/time_stretch.cc:198: right_side =
static_cast<int32_t>(peak_index) * (right_side >> right_scale);
On 2015/08/17 22:49:47, Peter Kasting wrote:
> On 2015/08/10 11:30:02, hlundin-webrtc wrote:
> > rtc::checked_cast
> 
> While today this shouldn't be necessary as |peak_index| is guaranteed to fit
in
> an int32_t by the caller (see reply above), I suppose in the future someone
> could change that.  I could change the function to take an int32_t instead,
but
> that seems more confusing, when the rest of the file uses a size_t for this
> concept.  So, done.

Thanks. Future changes is what worries me sometimes.

[Peter Kasting, 2015-08-18 20:32:08]

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/background_noise.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/background_noise.cc:253: parameters.scale =
static_cast<int16_t>(WebRtcSpl_SqrtFloor(residual_energy));
On 2015/08/18 07:19:18, hlundin-webrtc wrote:
> On 2015/08/17 22:49:46, Peter Kasting wrote:
> > On 2015/08/10 11:30:00, hlundin-webrtc wrote:
> > > rtc::checked_cast
> > 
> > This shouldn't be necessary, since by definition the square root of a 32-bit
> > quantity must fit in 16 bits.
> > 
> > If you would like, I could make this clearer by (separately) changing the
> > signature of WebRtcSpl_SqrtFloor() to return an int16_t.  It looks like we
> have
> > many places in the code which already unconditionally cast this return value
> to
> > int16_t, so this would enable removing all those casts.
> 
> That seems like a good idea, if you don't mind.

Actually, I don't think this is safe after all.  The results fit in 16 bits, but
that's 16 value bits, not 15 bits + a sign bit.

The explicit cast I'm adding here doesn't actually change the function of the
code -- it just removes an "implicit cast" warning -- and we seem to get this
wrong in numerous other places.  Rather than just fix this location, I've filed
a bug about this general pattern:
https://code.google.com/p/webrtc/issues/detail?id=4924

I'd appreciate it if you could triage this bug.

[hlundin-webrtc, 2015-08-19 07:36:45]

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/background_noise.cc (right):

https://codereview.webrtc.org/1228843002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/background_noise.cc:253: parameters.scale =
static_cast<int16_t>(WebRtcSpl_SqrtFloor(residual_energy));
On 2015/08/18 20:32:07, Peter Kasting wrote:
> On 2015/08/18 07:19:18, hlundin-webrtc wrote:
> > On 2015/08/17 22:49:46, Peter Kasting wrote:
> > > On 2015/08/10 11:30:00, hlundin-webrtc wrote:
> > > > rtc::checked_cast
> > > 
> > > This shouldn't be necessary, since by definition the square root of a
32-bit
> > > quantity must fit in 16 bits.
> > > 
> > > If you would like, I could make this clearer by (separately) changing the
> > > signature of WebRtcSpl_SqrtFloor() to return an int16_t.  It looks like we
> > have
> > > many places in the code which already unconditionally cast this return
value
> > to
> > > int16_t, so this would enable removing all those casts.
> > 
> > That seems like a good idea, if you don't mind.
> 
> Actually, I don't think this is safe after all.  The results fit in 16 bits,
but
> that's 16 value bits, not 15 bits + a sign bit.
> 
> The explicit cast I'm adding here doesn't actually change the function of the
> code -- it just removes an "implicit cast" warning -- and we seem to get this
> wrong in numerous other places.  Rather than just fix this location, I've
filed
> a bug about this general pattern:
> https://code.google.com/p/webrtc/issues/detail?id=4924
> 
> I'd appreciate it if you could triage this bug.

I saw the bug and will triage it. Thanks for investigating!