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

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

Created:
8 months, 1 week ago by ivoc
Modified:
7 months, 4 weeks 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 ...
8 months, 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
8 months, 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 > ...
8 months, 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 ...
8 months, 1 week 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 ...
8 months, 1 week ago (2017-09-13 16:00:13 UTC) #9
alexnarest
lgtm
8 months, 1 week 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 ...
8 months, 1 week 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: > ...
8 months, 1 week ago (2017-09-14 09:59:44 UTC) #12
alexnarest
lgtm
8 months, 1 week 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 ...
8 months, 1 week 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 ...
8 months, 1 week 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 ...
8 months, 1 week 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 ...
8 months, 1 week ago (2017-09-14 14:16:04 UTC) #17
ossu
Alright. Then I'll discard my draft and lgtm instead!
8 months, 1 week 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
8 months, 1 week 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)
8 months, 1 week 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
8 months, 1 week 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)
8 months, 1 week 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
8 months, 1 week 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)
8 months, 1 week 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
8 months, 1 week 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; ...
8 months, 1 week 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 ...
7 months, 4 weeks 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
7 months, 4 weeks ago (2017-09-28 07:43:04 UTC) #42
commit-bot: I haz the power
7 months, 4 weeks 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