Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(46)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week ago by ivoc
Modified:
4 days, 17 hours 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(BackIn2018March)
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

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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+120 lines, -30 lines) Patch
M modules/audio_coding/audio_network_adaptor/bitrate_controller.h View 1 2 3 4 5 6 7 8 1 chunk +6 lines, -1 line 0 comments Download
M modules/audio_coding/audio_network_adaptor/bitrate_controller.cc View 1 2 3 4 5 6 7 8 2 chunks +15 lines, -5 lines 0 comments Download
M modules/audio_coding/audio_network_adaptor/bitrate_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 chunks +11 lines, -11 lines 0 comments Download
M modules/audio_coding/audio_network_adaptor/config.proto View 1 2 3 4 5 6 7 8 2 chunks +12 lines, -1 line 0 comments Download
M modules/audio_coding/audio_network_adaptor/controller_manager.cc View 1 2 3 4 5 6 7 8 3 chunks +27 lines, -6 lines 0 comments Download
M modules/audio_coding/audio_network_adaptor/frame_length_controller.h View 1 2 3 4 5 6 7 8 3 chunks +10 lines, -0 lines 0 comments Download
M modules/audio_coding/audio_network_adaptor/frame_length_controller.cc View 1 2 3 4 5 6 7 8 9 5 chunks +26 lines, -5 lines 0 comments Download
M modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -1 line 0 comments Download
M modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor_config.h View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 39 (17 generated)
ivoc
Hi guys, PTAL at this CL which adds two extra config parameters to the ANA ...
1 week 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
1 week 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 > ...
1 week 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 ...
6 days, 23 hours 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 ...
6 days, 19 hours ago (2017-09-13 16:00:13 UTC) #9
alexnarest
lgtm
6 days, 17 hours 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 ...
6 days, 3 hours 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: > ...
6 days, 1 hour ago (2017-09-14 09:59:44 UTC) #12
alexnarest
lgtm
5 days, 23 hours 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 ...
5 days, 22 hours 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 ...
5 days, 22 hours 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 ...
5 days, 21 hours 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 ...
5 days, 20 hours ago (2017-09-14 14:16:04 UTC) #17
ossu
Alright. Then I'll discard my draft and lgtm instead!
5 days, 19 hours 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
5 days, 19 hours 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)
5 days, 19 hours 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
4 days, 19 hours 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)
4 days, 19 hours 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
4 days, 19 hours 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)
4 days, 19 hours 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
4 days, 19 hours ago (2017-09-15 15:54:55 UTC) #37
commit-bot: I haz the power
4 days, 17 hours ago (2017-09-15 17:55:13 UTC) #39
Try jobs failed on following builders:
  linux_baremetal on master.tryserver.webrtc (JOB_TIMED_OUT, build has not
started yet; builder either lacks capacity or does not exist (misspelled?))
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b