Fixing an overflow issue in NetEq::BackgroundNoise

Fixing an overflow issue in NetEq::BackgroundNoise

BUG=chromium:676941
Review-Url: https://codereview.webrtc.org/2722533002
Cr-Commit-Position: refs/heads/master@{#16938}
Committed: https://chromium.googlesource.com/external/webrtc/+/a67e5f5de97366587246ff6cc9a92248064e258c

[hlundin-webrtc, 2017-02-27 12:03:32]

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

[hlundin-webrtc, 2017-02-27 12:03:32]

kwiberg, ptal.

[kwiberg-webrtc, 2017-02-28 10:31:35]

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

https://codereview.webrtc.org/2722533002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/background_noise.cc:114: (sample_energy > 0))
{
Hmm. Wouldn't it make more sense to have

  (residual_energy >= kMaxEnergyToCompare) || ...

since the original code enters the if block if residual_energy is *high* enough?

Alternatively, consider doing

  if (sample_energy > 0 && int64_t{5} * residual_energy >= 16 * sample_energy)

(Or use int64_t{16} if the rhs could overflow as well.)

Godbolt experiment here: https://godbolt.org/g/0DvNYi
Note that testing against kMaxEnergyToCompare seems to introduce a branch on all
of x86-64 clang, x86-64 gcc, arm64 gcc, and arm32 gcc. Doing 64-bit arithmetic
seems to result in a bunch more instructions on arm32 (all of them cheap
arithmetic things, though), but looks very efficient on the 64-bit platforms.

[hlundin-webrtc, 2017-02-28 12:15:32]

PTAL again.

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

https://codereview.webrtc.org/2722533002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/background_noise.cc:114: (sample_energy > 0))
{
On 2017/02/28 10:31:35, kwiberg-webrtc wrote:
> Hmm. Wouldn't it make more sense to have
> 
>   (residual_energy >= kMaxEnergyToCompare) || ...
> 
> since the original code enters the if block if residual_energy is *high*
enough?
> 
> Alternatively, consider doing
> 
>   if (sample_energy > 0 && int64_t{5} * residual_energy >= 16 * sample_energy)
> 
> (Or use int64_t{16} if the rhs could overflow as well.)
> 
> Godbolt experiment here: https://godbolt.org/g/0DvNYi
> Note that testing against kMaxEnergyToCompare seems to introduce a branch on
all
> of x86-64 clang, x86-64 gcc, arm64 gcc, and arm32 gcc. Doing 64-bit arithmetic
> seems to result in a bunch more instructions on arm32 (all of them cheap
> arithmetic things, though), but looks very efficient on the 64-bit platforms.

I went for the int64_t{x} suggestion. Thanks!

[kwiberg-webrtc, 2017-02-28 12:19:59]

lgtm

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

https://codereview.webrtc.org/2722533002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/background_noise.cc:107: (int64_t{5} *
residual_energy >= int64_t{16} * sample_energy)) {
Optionally, remove the redundant parentheses.

[hlundin-webrtc, 2017-03-01 10:18:49]

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

[commit-bot: I haz the power, 2017-03-01 10:18:58]

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-03-01 11:06:50]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488363529092700,
"parent_rev": "f31969a584bcafe9406c214a9d4c3afb49d19650", "commit_rev":
"a67e5f5de97366587246ff6cc9a92248064e258c"}

[commit-bot: I haz the power, 2017-03-01 11:06:52]

Description was changed from

==========
Fixing an overflow issue in NetEq::BackgroundNoise

BUG=chromium:676941
==========

to

==========
Fixing an overflow issue in NetEq::BackgroundNoise

BUG=chromium:676941

Review-Url: https://codereview.webrtc.org/2722533002
Cr-Commit-Position: refs/heads/master@{#16938}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a67e5f5de97366587246ff6cc...
==========

[commit-bot: I haz the power, 2017-03-01 11:06:54]

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