Fix for left-shift of potentially negative values in NetEq.

Fix for left-shift of potentially negative values in NetEq.

Left shifting negative integers is undefined behavior, and should be prevented. This CL fixes one such instance in the NetEq Expand function.

BUG=chromium:677106
Review-Url: https://codereview.webrtc.org/2616363003
Cr-Commit-Position: refs/heads/master@{#15966}
Committed: https://chromium.googlesource.com/external/webrtc/+/4843dd13aa3426d9a3caee768fc015d8731c5d18

[ivoc, 2017-01-09 14:02:50]

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

[ivoc, 2017-01-09 14:02:50]

Hi Henrik, PTAL. The values in this shift operation (peak values of a cross
correlation) can be negative numbers, so we should use a multiplication instead
of a shift.

[ivoc, 2017-01-09 14:03:15]

Description was changed from

==========
Fix for left-shift of potentially negative values.

Left shifting negative integers is undefined behavior, and should be prevented.
This CL fixes one such instance in the NetEq Expand function.

BUG=chromium:677106
==========

to

==========
Fix for left-shift of potentially negative values in NetEq.

Left shifting negative integers is undefined behavior, and should be prevented.
This CL fixes one such instance in the NetEq Expand function.

BUG=chromium:677106
==========

[kwiberg-webrtc, 2017-01-09 14:07:13]

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

[kwiberg-webrtc, 2017-01-09 14:07:14]

lgtm

https://codereview.webrtc.org/2616363003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/2616363003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/expand.cc:444: ratio = (best_correlation[i] *
65536) / best_distortion[i];
Since larger powers of two have lots of digits that one can get wrong (and that
not everyone has memorized), I've found it useful to write this sort of thing
like this instead:

  ratio = (best_correlation[i] * (1 << 16)) / best_distortion[i];

[hlundin-webrtc, 2017-01-09 14:12:08]

lgtm with Karl's suggestion.

https://codereview.webrtc.org/2616363003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/2616363003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/expand.cc:444: ratio = (best_correlation[i] *
65536) / best_distortion[i];
On 2017/01/09 14:07:13, kwiberg-webrtc wrote:
> Since larger powers of two have lots of digits that one can get wrong (and
that
> not everyone has memorized), I've found it useful to write this sort of thing
> like this instead:
> 
>   ratio = (best_correlation[i] * (1 << 16)) / best_distortion[i];

Acknowledged.

[ivoc, 2017-01-09 14:39:53]

https://codereview.webrtc.org/2616363003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/expand.cc (right):

https://codereview.webrtc.org/2616363003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/expand.cc:444: ratio = (best_correlation[i] *
65536) / best_distortion[i];
On 2017/01/09 14:12:07, hlundin-webrtc wrote:
> On 2017/01/09 14:07:13, kwiberg-webrtc wrote:
> > Since larger powers of two have lots of digits that one can get wrong (and
> that
> > not everyone has memorized), I've found it useful to write this sort of
thing
> > like this instead:
> > 
> >   ratio = (best_correlation[i] * (1 << 16)) / best_distortion[i];
> 
> Acknowledged.

Good idea, done.

[ivoc, 2017-01-09 14:40:25]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2017-01-09 14:40:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2616363003/#ps20001 (title:
"Update with Karl's suggestion.")

[commit-bot: I haz the power, 2017-01-09 14:40:28]

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-01-09 16:31:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483972825254690,
"parent_rev": "39ced8cf35249e9e034c57edf08029c4cef33c14", "commit_rev":
"4843dd13aa3426d9a3caee768fc015d8731c5d18"}

[commit-bot: I haz the power, 2017-01-09 16:31:44]

Description was changed from

==========
Fix for left-shift of potentially negative values in NetEq.

Left shifting negative integers is undefined behavior, and should be prevented.
This CL fixes one such instance in the NetEq Expand function.

BUG=chromium:677106
==========

to

==========
Fix for left-shift of potentially negative values in NetEq.

Left shifting negative integers is undefined behavior, and should be prevented.
This CL fixes one such instance in the NetEq Expand function.

BUG=chromium:677106

Review-Url: https://codereview.webrtc.org/2616363003
Cr-Commit-Position: refs/heads/master@{#15966}
Committed:
https://chromium.googlesource.com/external/webrtc/+/4843dd13aa3426d9a3caee768...
==========

[commit-bot: I haz the power, 2017-01-09 16:31:46]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/4843dd13aa3426d9a3caee768...