Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(583)

Issue 2859483005: NetEq: Fix a bug in expand_rate and speech_expand_rate calculation (Closed)

Created:
3 years, 7 months ago by hlundin-webrtc
Modified:
3 years, 7 months ago
Reviewers:
minyue-webrtc
CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Fix android checksum #

Total comments: 11

Patch Set 3 : After first round of reviews #

Total comments: 1

Patch Set 4 : Changing to Minyue's suggestion #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -14 lines) Patch
M webrtc/modules/audio_coding/neteq/merge.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/neteq_impl.cc View 1 chunk +6 lines, -4 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/neteq_unittest.cc View 1 2 2 chunks +10 lines, -10 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/statistics_calculator.h View 1 chunk +8 lines, -0 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/statistics_calculator.cc View 1 2 3 2 chunks +20 lines, -0 lines 1 comment Download

Messages

Total messages: 24 (14 generated)
hlundin-webrtc
Minyue, PTAL.
3 years, 7 months ago (2017-05-04 13:37:22 UTC) #6
kwiberg-webrtc
https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/neteq_impl.cc File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/neteq_impl.cc#newcode1606 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 ...
3 years, 7 months ago (2017-05-04 19:55:04 UTC) #7
minyue-webrtc
good work. some small comments. https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/merge.cc File webrtc/modules/audio_coding/neteq/merge.cc (right): https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/merge.cc#newcode157 webrtc/modules/audio_coding/neteq/merge.cc:157: RTC_DCHECK_EQ(output_length, output->Size()); I don't ...
3 years, 7 months ago (2017-05-05 07:14:01 UTC) #8
hlundin-webrtc
Thanks, and PTAL again. https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/merge.cc File webrtc/modules/audio_coding/neteq/merge.cc (right): https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/merge.cc#newcode157 webrtc/modules/audio_coding/neteq/merge.cc:157: RTC_DCHECK_EQ(output_length, output->Size()); On 2017/05/05 07:14:00, ...
3 years, 7 months ago (2017-05-05 07:42:15 UTC) #9
minyue-webrtc
lgtm https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/merge.cc File webrtc/modules/audio_coding/neteq/merge.cc (right): https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/merge.cc#newcode157 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: > ...
3 years, 7 months ago (2017-05-05 07:50:39 UTC) #10
kwiberg-webrtc
https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/statistics_calculator.cc File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right): https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/statistics_calculator.cc#newcode27 webrtc/modules/audio_coding/neteq/statistics_calculator.cc:27: if (a < 0 && static_cast<size_t>(-a) > b) { ...
3 years, 7 months ago (2017-05-05 08:34:29 UTC) #11
hlundin-webrtc
https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/statistics_calculator.cc File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right): https://codereview.webrtc.org/2859483005/diff/20001/webrtc/modules/audio_coding/neteq/statistics_calculator.cc#newcode27 webrtc/modules/audio_coding/neteq/statistics_calculator.cc:27: if (a < 0 && static_cast<size_t>(-a) > b) { ...
3 years, 7 months ago (2017-05-05 11:40:09 UTC) #13
kwiberg-webrtc
OK, looks good. https://codereview.webrtc.org/2859483005/diff/80001/webrtc/modules/audio_coding/neteq/statistics_calculator.cc File webrtc/modules/audio_coding/neteq/statistics_calculator.cc (right): https://codereview.webrtc.org/2859483005/diff/80001/webrtc/modules/audio_coding/neteq/statistics_calculator.cc#newcode30 webrtc/modules/audio_coding/neteq/statistics_calculator.cc:30: "int must not be wider than ...
3 years, 7 months ago (2017-05-05 11:43:52 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2859483005/80001
3 years, 7 months ago (2017-05-05 12:01:45 UTC) #21
commit-bot: I haz the power
3 years, 7 months ago (2017-05-05 12:04:21 UTC) #24
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/2979f55f95ec71425a32fca31...

Powered by Google App Engine
This is Rietveld 408576698