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

Issue 1410073006: ACM: Move NACK functionality inside NetEq (Closed)

Created:
5 years, 1 month ago by hlundin-webrtc
Modified:
5 years, 1 month ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, hlundin-webrtc, peah-webrtc, kwiberg-webrtc, tlegrand-webrtc
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

ACM: Move NACK functionality inside NetEq Negative acknowledgement (NACK) has up to now been implemented in ACM. But, since NetEq is in charge of the actual packet buffer, it makes more sense to have the NACK functionlaity in there. This CL does the following: - Move nack.{h,cc} and the unit tests from main/acm2 to neteq. - Move the NACK related code in ACM into NetEq. - NACK related functions in AcmReceiver are changed to simple forwarding APIs. - Remove unused members in AcmReceiver. - Remove unused API functions in NetEq. BUG=webrtc:3520 Committed: https://crrev.com/48ed930975ef7e84023044ed584c4eff914e6c9a Cr-Commit-Position: refs/heads/master@{#10448}

Patch Set 1 #

Patch Set 2 : Add function descriptions #

Total comments: 15

Patch Set 3 : Updates after review #

Patch Set 4 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+140 lines, -1101 lines) Patch
M webrtc/modules/audio_coding/BUILD.gn View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M webrtc/modules/audio_coding/main/acm2/acm_receiver.h View 1 2 3 2 chunks +0 lines, -4 lines 0 comments Download
M webrtc/modules/audio_coding/main/acm2/acm_receiver.cc View 1 2 3 6 chunks +4 lines, -62 lines 0 comments Download
D webrtc/modules/audio_coding/main/acm2/nack.h View 1 2 3 1 chunk +0 lines, -213 lines 0 comments Download
D webrtc/modules/audio_coding/main/acm2/nack.cc View 1 2 3 1 chunk +0 lines, -231 lines 0 comments Download
D webrtc/modules/audio_coding/main/acm2/nack_unittest.cc View 1 2 3 1 chunk +0 lines, -486 lines 0 comments Download
M webrtc/modules/audio_coding/main/audio_coding_module.gypi View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/include/neteq.h View 1 2 3 1 chunk +11 lines, -4 lines 0 comments Download
A + webrtc/modules/audio_coding/neteq/nack.h View 1 2 3 5 chunks +6 lines, -10 lines 0 comments Download
A + webrtc/modules/audio_coding/neteq/nack.cc View 1 2 3 9 chunks +28 lines, -28 lines 0 comments Download
A + webrtc/modules/audio_coding/neteq/nack_unittest.cc View 1 2 3 14 chunks +28 lines, -32 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/neteq.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/neteq_impl.h View 1 2 3 3 chunks +8 lines, -13 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/neteq_impl.cc View 1 2 3 7 chunks +50 lines, -13 lines 0 comments Download
M webrtc/modules/modules.gyp View 2 chunks +1 line, -1 line 0 comments Download

Dependent Patchsets:

Messages

Total messages: 18 (6 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410073006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410073006/1
5 years, 1 month ago (2015-10-26 14:36:57 UTC) #2
hlundin-webrtc
Minyue, Please, take a look at this CL. Thanks!
5 years, 1 month ago (2015-10-26 15:17:49 UTC) #5
minyue-webrtc
good and some comments https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_coding/neteq/nack.cc File webrtc/modules/audio_coding/neteq/nack.cc (right): https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_coding/neteq/nack.cc#newcode142 webrtc/modules/audio_coding/neteq/nack.cc:142: nack_list_.begin()->second.time_to_play_ms <= 10) I know ...
5 years, 1 month ago (2015-10-27 14:35:08 UTC) #6
hlundin-webrtc
Updates after review
5 years, 1 month ago (2015-10-28 15:02:16 UTC) #7
hlundin-webrtc
Thanks. PTAL again. https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_coding/neteq/nack.cc File webrtc/modules/audio_coding/neteq/nack.cc (right): https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_coding/neteq/nack.cc#newcode142 webrtc/modules/audio_coding/neteq/nack.cc:142: nack_list_.begin()->second.time_to_play_ms <= 10) On 2015/10/27 14:35:08, ...
5 years, 1 month ago (2015-10-28 15:03:36 UTC) #8
minyue-webrtc
nice, lgtm if you considered a last comment. https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_coding/neteq/nack.cc File webrtc/modules/audio_coding/neteq/nack.cc (right): https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_coding/neteq/nack.cc#newcode142 webrtc/modules/audio_coding/neteq/nack.cc:142: nack_list_.begin()->second.time_to_play_ms ...
5 years, 1 month ago (2015-10-29 10:36:10 UTC) #9
hlundin-webrtc
https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_coding/neteq/neteq_impl.cc File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_coding/neteq/neteq_impl.cc#newcode638 webrtc/modules/audio_coding/neteq/neteq_impl.cc:638: RTC_DCHECK(nack_); On 2015/10/29 10:36:10, minyue-webrtc wrote: > On 2015/10/28 ...
5 years, 1 month ago (2015-10-29 10:38:18 UTC) #10
hlundin-webrtc
rebase
5 years, 1 month ago (2015-10-29 10:44:01 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410073006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410073006/60001
5 years, 1 month ago (2015-10-29 10:45:35 UTC) #14
kwiberg-webrtc
https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_coding/neteq/neteq_impl.cc File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1410073006/diff/20001/webrtc/modules/audio_coding/neteq/neteq_impl.cc#newcode638 webrtc/modules/audio_coding/neteq/neteq_impl.cc:638: RTC_DCHECK(nack_); On 2015/10/29 10:38:18, hlundin-webrtc wrote: > On 2015/10/29 ...
5 years, 1 month ago (2015-10-29 10:46:01 UTC) #16
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 1 month ago (2015-10-29 12:36:28 UTC) #17
commit-bot: I haz the power
5 years, 1 month ago (2015-10-29 12:36:36 UTC) #18
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/48ed930975ef7e84023044ed584c4eff914e6c9a
Cr-Commit-Position: refs/heads/master@{#10448}

Powered by Google App Engine
This is Rietveld 408576698