NetEq: Fix a bug in expand_rate and speech_expand_rate calculation

NetEq: Fix a bug in expand_rate and speech_expand_rate calculation

After a Merge operation, the statistics for number of samples
generated using Expand must be corrected, and the correction can in
fact be negative. However, a bug was introduced in
https://codereview.webrtc.org/1230503003 which uses a size_t to
represent the correction, which leads to wrap-around of the negative
value. This is not a problem in itself, since this value is added to
another size_t, with the effect that the desired subtraction happens
anyway.

The actual problem arises if the statistics are polled/reset before a
subtraction happens -- that is, between an Expand and a Merge
operation. This will lead to an actual wrap-around of the stats value,
and large expand_rate (16384) is reported.

BUG=webrtc:7554
Review-Url: https://codereview.webrtc.org/2859483005
Cr-Commit-Position: refs/heads/master@{#18029}
Committed: https://chromium.googlesource.com/external/webrtc/+/2979f55f95ec71425a32fca31c50c9cbd71ad738

[hlundin-webrtc, 2017-05-04 13:01:52]

The CQ bit was checked by henrik.lundin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 13:01:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-04 13:31:25]

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

[commit-bot: I haz the power, 2017-05-04 13:31:26]

Dry run: This issue passed the CQ dry run.

[hlundin-webrtc, 2017-05-04 13:37:21]

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

[hlundin-webrtc, 2017-05-04 13:37:22]

Minyue, PTAL.

[kwiberg-webrtc, 2017-05-04 19:55:04]

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

https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1606:
rtc::dchecked_cast<int>(decoded_length / algorithm_buffer_->Channels());
Good---dchecked_cast seems like a good choice here.

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

https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/statistics_calculator.cc:27: if (a < 0 &&
static_cast<size_t>(-a) > b) {
if (safe_cmp::Gt(-a, b)) {

Both your and my version will misbehave if a == INT_MIN, though, so maybe

if (safe_cmp::Gt(-int64_t{a}, b)) {

https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/statistics_calculator.cc:32: return a + b;
I'm pretty sure a will be promoted to size_t before the addition so that
everything just works, but consider making it explicit:

  return dchecked_cast<size_t>(a) + b;

[minyue-webrtc, 2017-05-05 07:14:01]

good work. some small comments.

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

https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/merge.cc:157: RTC_DCHECK_EQ(output_length,
output->Size());
I don't quite see how this relates to the changes in stats.

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

https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/statistics_calculator.cc:27: if (a < 0 &&
static_cast<size_t>(-a) > b) {
On 2017/05/04 19:55:04, kwiberg-webrtc wrote:
> if (safe_cmp::Gt(-a, b)) {
> 
> Both your and my version will misbehave if a == INT_MIN, though, so maybe
> 
> if (safe_cmp::Gt(-int64_t{a}, b)) {

how about

size_t ret = b + a; // you may cast a if you want
return (a < 0 && ret > b) ? 0 : ret

[hlundin-webrtc, 2017-05-05 07:42:15]

Thanks, and PTAL again.

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

https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/merge.cc:157: RTC_DCHECK_EQ(output_length,
output->Size());
On 2017/05/05 07:14:00, minyue-webrtc wrote:
> I don't quite see how this relates to the changes in stats.

I basically just wanted to make sure that the return value from this function
would never go negative (which would cause a wrap). I'll reduce to a single
DCHECK.

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

https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/statistics_calculator.cc:27: if (a < 0 &&
static_cast<size_t>(-a) > b) {
On 2017/05/05 07:14:00, minyue-webrtc wrote:
> On 2017/05/04 19:55:04, kwiberg-webrtc wrote:
> > if (safe_cmp::Gt(-a, b)) {
> > 
> > Both your and my version will misbehave if a == INT_MIN, though, so maybe
> > 
> > if (safe_cmp::Gt(-int64_t{a}, b)) {
> 
> how about
> 
> size_t ret = b + a; // you may cast a if you want
> return (a < 0 && ret > b) ? 0 : ret
>   

Is it conceivable that abs(int_min) is larger than max(size_t) on any platform?
That would break Minyue's proposal. Otherwise, I like it.

I implemented Karl's suggestion for now. Let me know if you think I should
change.

https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/statistics_calculator.cc:32: return a + b;
On 2017/05/04 19:55:04, kwiberg-webrtc wrote:
> I'm pretty sure a will be promoted to size_t before the addition so that
> everything just works, but consider making it explicit:
> 
>   return dchecked_cast<size_t>(a) + b;

Done.

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

https://codereview.webrtc.org/2859483005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:449: const std::string
network_stats_checksum =
I belated git cl format...

[minyue-webrtc, 2017-05-05 07:50:39]

lgtm

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

https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/merge.cc:157: RTC_DCHECK_EQ(output_length,
output->Size());
On 2017/05/05 07:42:14, hlundin-webrtc wrote:
> On 2017/05/05 07:14:00, minyue-webrtc wrote:
> > I don't quite see how this relates to the changes in stats.
> 
> I basically just wanted to make sure that the return value from this function
> would never go negative (which would cause a wrap). I'll reduce to a single
> DCHECK.

Acknowledged.

[kwiberg-webrtc, 2017-05-05 08:34:29]

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

https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/statistics_calculator.cc:27: if (a < 0 &&
static_cast<size_t>(-a) > b) {
On 2017/05/05 07:42:14, hlundin-webrtc wrote:
> On 2017/05/05 07:14:00, minyue-webrtc wrote:
> > On 2017/05/04 19:55:04, kwiberg-webrtc wrote:
> > > if (safe_cmp::Gt(-a, b)) {
> > > 
> > > Both your and my version will misbehave if a == INT_MIN, though, so maybe
> > > 
> > > if (safe_cmp::Gt(-int64_t{a}, b)) {
> > 
> > how about
> > 
> > size_t ret = b + a; // you may cast a if you want
> > return (a < 0 && ret > b) ? 0 : ret
> >   
> 
> Is it conceivable that abs(int_min) is larger than max(size_t) on any
platform?

AFAICT, the standard just says that "The type size_t is an
implementation-defined unsigned integer type that is large enough to contain the
size in bytes of any object." But I doubt there's any system whose size_t is
smaller than int. You could static_assert it if you want.

> That would break Minyue's proposal. Otherwise, I like it.

Yes, I like it too---it makes neat use of the fact that signed-to-unsigned
conversion is well-defined (which is not the case for unsigned-to-signed
conversion).

> I implemented Karl's suggestion for now. Let me know if you think I should
> change.

Minyue's version is probably better, since it avoids using 64-bit types on
32-bit platforms.

[hlundin-webrtc, 2017-05-05 11:39:38]

Patchset #4 (id:60001) has been deleted

[hlundin-webrtc, 2017-05-05 11:40:09]

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

https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/statistics_calculator.cc:27: if (a < 0 &&
static_cast<size_t>(-a) > b) {
On 2017/05/05 08:34:29, kwiberg-webrtc wrote:
> On 2017/05/05 07:42:14, hlundin-webrtc wrote:
> > On 2017/05/05 07:14:00, minyue-webrtc wrote:
> > > On 2017/05/04 19:55:04, kwiberg-webrtc wrote:
> > > > if (safe_cmp::Gt(-a, b)) {
> > > > 
> > > > Both your and my version will misbehave if a == INT_MIN, though, so
maybe
> > > > 
> > > > if (safe_cmp::Gt(-int64_t{a}, b)) {
> > > 
> > > how about
> > > 
> > > size_t ret = b + a; // you may cast a if you want
> > > return (a < 0 && ret > b) ? 0 : ret
> > >   
> > 
> > Is it conceivable that abs(int_min) is larger than max(size_t) on any
> platform?
> 
> AFAICT, the standard just says that "The type size_t is an
> implementation-defined unsigned integer type that is large enough to contain
the
> size in bytes of any object." But I doubt there's any system whose size_t is
> smaller than int. You could static_assert it if you want.
> 
> > That would break Minyue's proposal. Otherwise, I like it.
> 
> Yes, I like it too---it makes neat use of the fact that signed-to-unsigned
> conversion is well-defined (which is not the case for unsigned-to-signed
> conversion).
> 
> > I implemented Karl's suggestion for now. Let me know if you think I should
> > change.
> 
> Minyue's version is probably better, since it avoids using 64-bit types on
> 32-bit platforms.

Changed to Minyue's version.

[hlundin-webrtc, 2017-05-05 11:40:18]

The CQ bit was checked by henrik.lundin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 11:40:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kwiberg-webrtc, 2017-05-05 11:43:52]

OK, looks good.

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

https://codereview.webrtc.org/2859483005/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/statistics_calculator.cc:30: "int must not be
wider than size_t for this to work");
It's up to you, but I'd skip the description. It's obvious from the context.

[commit-bot: I haz the power, 2017-05-05 11:59:16]

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

[commit-bot: I haz the power, 2017-05-05 11:59:17]

Dry run: This issue passed the CQ dry run.

[hlundin-webrtc, 2017-05-05 12:01:41]

The CQ bit was checked by henrik.lundin@webrtc.org

[hlundin-webrtc, 2017-05-05 12:01:41]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2859483005/#ps80001 (title:
"Changing to Minyue's suggestion")

[commit-bot: I haz the power, 2017-05-05 12:01:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-05 12:04:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493985701500240,
"parent_rev": "2a28035627ab93463005d9549b2f47db6600aa69", "commit_rev":
"2979f55f95ec71425a32fca31c50c9cbd71ad738"}

[commit-bot: I haz the power, 2017-05-05 12:04:20]

Description was changed from

==========
NetEq: Fix a bug in expand_rate and speech_expand_rate calculation

After a Merge operation, the statistics for number of samples
generated using Expand must be corrected, and the correction can in
fact be negative. However, a bug was introduced in
https://codereview.webrtc.org/1230503003 which uses a size_t to
represent the correction, which leads to wrap-around of the negative
value. This is not a problem in itself, since this value is added to
another size_t, with the effect that the desired subtraction happens
anyway.

The actual problem arises if the statistics are polled/reset before a
subtraction happens -- that is, between an Expand and a Merge
operation. This will lead to an actual wrap-around of the stats value,
and large expand_rate (16384) is reported.

BUG=webrtc:7554
==========

to

==========
NetEq: Fix a bug in expand_rate and speech_expand_rate calculation

After a Merge operation, the statistics for number of samples
generated using Expand must be corrected, and the correction can in
fact be negative. However, a bug was introduced in
https://codereview.webrtc.org/1230503003 which uses a size_t to
represent the correction, which leads to wrap-around of the negative
value. This is not a problem in itself, since this value is added to
another size_t, with the effect that the desired subtraction happens
anyway.

The actual problem arises if the statistics are polled/reset before a
subtraction happens -- that is, between an Expand and a Merge
operation. This will lead to an actual wrap-around of the stats value,
and large expand_rate (16384) is reported.

BUG=webrtc:7554

Review-Url: https://codereview.webrtc.org/2859483005
Cr-Commit-Position: refs/heads/master@{#18029}
Committed:
https://chromium.googlesource.com/external/webrtc/+/2979f55f95ec71425a32fca31...
==========

[commit-bot: I haz the power, 2017-05-05 12:04:21]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/2979f55f95ec71425a32fca31...