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

Issue 3013613002: Added configurable offsets to the per-packet overhead in ANA. (Closed)

Created:
3 years, 3 months ago by ivoc
Modified:
3 years, 2 months ago
Reviewers:
ossu, alexnarest
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

Added configurable offsets to the per-packet overhead in the ANA frame length and bitrate controllers. This adds four parameters to the protobuf that is used to configure the ANA controllers. These extra parameters allow for setting an offset to the per-packet overhead that is used when changing the frame length size and when changing bitrate. BUG=webrtc:8179 Review-Url: https://codereview.webrtc.org/3013613002 Cr-Commit-Position: refs/heads/master@{#20011} Committed: https://webrtc.googlesource.com/src/+/7e9c614648e933eb65d0408f016bc7617d19de73

Patch Set 1 #

Patch Set 2 : Fixed a silly mistake. #

Patch Set 3 : Added additional offset parameter for bitrate controller. #

Total comments: 1

Patch Set 4 : Split bitrate offset parameter into two, depending on the last change in frame length. #

Total comments: 2

Patch Set 5 : Set prev_direction_increase in the Frame length controller. #

Total comments: 7

Patch Set 6 : Comments by ossu #

Patch Set 7 : Added comment to explain the purpose of the bool. #

Patch Set 8 : Fixed DCHECKs to fix failing unittests. #

Patch Set 9 : Rebase. #

Patch Set 10 : Added conversion to size_t in DCHECK. #

Messages

Total messages: 45 (20 generated)
ivoc
Hi guys, PTAL at this CL which adds two extra config parameters to the ANA ...
3 years, 3 months ago (2017-09-12 15:53:11 UTC) #3
alexnarest
We need to add offsets at https://cs.corp.google.com/piper///depot/google3/third_party/webrtc/files/stable/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc?rcl=166753879&l=60 too
3 years, 3 months ago (2017-09-12 16:33:20 UTC) #4
ivoc
On 2017/09/12 16:33:20, alexnarest wrote: > We need to add offsets at > https://cs.corp.google.com/piper///depot/google3/third_party/webrtc/files/stable/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc?rcl=166753879&l=60 > ...
3 years, 3 months ago (2017-09-13 09:36:18 UTC) #7
alexnarest
https://codereview.webrtc.org/3013613002/diff/40001/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc (right): https://codereview.webrtc.org/3013613002/diff/40001/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc#newcode60 webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:60: (*overhead_bytes_per_packet_ + config_.overhead_offset) * 8 * 1000 / We ...
3 years, 3 months ago (2017-09-13 11:19:04 UTC) #8
ivoc
On 2017/09/13 11:19:04, alexnarest wrote: > https://codereview.webrtc.org/3013613002/diff/40001/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc > File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc > (right): > > https://codereview.webrtc.org/3013613002/diff/40001/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc#newcode60 ...
3 years, 3 months ago (2017-09-13 16:00:13 UTC) #9
alexnarest
lgtm
3 years, 3 months ago (2017-09-13 17:53:19 UTC) #10
alexnarest
We probably need to update https://cs.corp.google.com/piper///depot/google3/third_party/objective_c/Tachyon/Tachyon/Source/App/GTYCallManager.m?rcl=168615504&l=1460 and https://cs.corp.google.com/piper///depot/google3/third_party/tachyon/src/com/google/android/apps/tachyon/callmanager/internal/MediaConstraintFactory.java?rcl=168119299&l=112 https://codereview.webrtc.org/3013613002/diff/60001/webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/3013613002/diff/60001/webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc#newcode129 webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:129: config.last_fl_change_increase ...
3 years, 3 months ago (2017-09-14 07:53:43 UTC) #11
ivoc
https://codereview.webrtc.org/3013613002/diff/60001/webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/3013613002/diff/60001/webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc#newcode129 webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:129: config.last_fl_change_increase = prev_config_->last_fl_change_increase; On 2017/09/14 07:53:42, alexnarest wrote: > ...
3 years, 3 months ago (2017-09-14 09:59:44 UTC) #12
alexnarest
lgtm
3 years, 3 months ago (2017-09-14 11:09:59 UTC) #13
ossu
Coupl'a comments... https://codereview.webrtc.org/3013613002/diff/80001/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h (right): https://codereview.webrtc.org/3013613002/diff/80001/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h#newcode25 webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h:25: int fl_increase_overhead_offset, What does this mean? https://codereview.webrtc.org/3013613002/diff/80001/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller_unittest.cc ...
3 years, 3 months ago (2017-09-14 12:16:14 UTC) #14
ivoc
https://codereview.webrtc.org/3013613002/diff/80001/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h (right): https://codereview.webrtc.org/3013613002/diff/80001/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h#newcode25 webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h:25: int fl_increase_overhead_offset, On 2017/09/14 12:16:14, ossu wrote: > What ...
3 years, 3 months ago (2017-09-14 13:03:39 UTC) #15
alexnarest
https://codereview.webrtc.org/3013613002/diff/80001/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h (right): https://codereview.webrtc.org/3013613002/diff/80001/webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h#newcode25 webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h:25: int fl_increase_overhead_offset, On 2017/09/14 13:03:38, ivoc wrote: > On ...
3 years, 3 months ago (2017-09-14 13:26:11 UTC) #16
ivoc
Like discussed offline, I added a comment in audio_network_adaptor_config.h to explain a bit more about ...
3 years, 3 months ago (2017-09-14 14:16:04 UTC) #17
ossu
Alright. Then I'll discard my draft and lgtm instead!
3 years, 3 months ago (2017-09-14 15:15:59 UTC) #18
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/3013613002/120001
3 years, 3 months ago (2017-09-14 15:46:51 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/24786)
3 years, 3 months ago (2017-09-14 15:55:41 UTC) #24
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/3013613002/140001
3 years, 3 months ago (2017-09-15 15:14:54 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/25068)
3 years, 3 months ago (2017-09-15 15:19:04 UTC) #29
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/3013613002/160001
3 years, 3 months ago (2017-09-15 15:35:33 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/25075)
3 years, 3 months ago (2017-09-15 15:46:04 UTC) #34
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/3013613002/180001
3 years, 3 months ago (2017-09-15 15:54:55 UTC) #37
commit-bot: I haz the power
Try jobs failed on following builders: linux_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, build has not started yet; ...
3 years, 3 months ago (2017-09-15 17:55:13 UTC) #39
guptaromi2529
On 2017/09/15 17:55:13, commit-bot: I haz the power wrote: > Try jobs failed on following ...
3 years, 2 months ago (2017-09-28 04:48:30 UTC) #40
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/3013613002/180001
3 years, 2 months ago (2017-09-28 07:43:04 UTC) #42
commit-bot: I haz the power
3 years, 2 months ago (2017-09-28 08:11:23 UTC) #45
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://webrtc.googlesource.com/src/+/7e9c614648e933eb65d0408f016bc7617d19de73

Powered by Google App Engine
This is Rietveld 408576698