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

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

Created:
2 months, 1 week ago by ivoc
Modified:
1 month, 3 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(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 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. #

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
Trybot results:  android_compile_x64_dbg   android_more_configs   ios64_sim_ios9_dbg   win_baremetal   mac_asan   linux_memcheck   android_rel   linux_baremetal   linux_ubsan   linux32_arm_rel   android_compile_x86_rel   win_clang_dbg   ios_api_framework   ios_arm64_dbg   ios32_sim_ios9_dbg   linux_asan   mac_baremetal   mac_compile_dbg   presubmit   linux_arm64_rel   win_clang_rel   android_compile_x86_dbg   win_rel   win_dbg   linux32_arm_dbg   win_msvc_rel   linux_rel   android_compile_mips_dbg   linux_msan   ios_dbg   win_x64_clang_rel   android_arm64_rel   mac_rel   linux_arm64_dbg   linux_gcc_rel   linux_tsan2   ios64_sim_ios10_dbg   linux_compile_dbg   linux_ubsan_vptr   win_x64_clang_dbg   android_dbg   linux_more_configs   ios_rel   win_x64_dbg   win_x64_rel   linux_libfuzzer_rel   ios_arm64_rel   win_x64_rel   win_x64_clang_dbg   android_compile_x86_dbg   linux_gcc_rel   android_compile_x64_dbg   android_dbg   ios_api_framework   ios_dbg   linux_libfuzzer_rel   linux_asan   android_compile_mips_dbg   android_compile_x86_rel   android_arm64_rel   ios_arm64_dbg   win_x64_dbg   win_clang_rel   win_msvc_rel   linux_ubsan_vptr   linux32_arm_dbg   linux_arm64_dbg   ios_rel   linux_tsan2   linux_baremetal   linux32_arm_rel   win_dbg   ios64_sim_ios9_dbg   android_more_configs   mac_asan   linux_compile_dbg   win_baremetal   linux_more_configs   mac_rel   win_clang_dbg   linux_ubsan   linux_msan 

Messages

Total messages: 45 (20 generated)
ivoc
Hi guys, PTAL at this CL which adds two extra config parameters to the ANA ...
2 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
2 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 > ...
2 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 ...
2 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 ...
2 months, 1 week ago (2017-09-13 16:00:13 UTC) #9
alexnarest
lgtm
2 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 ...
2 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: > ...
2 months ago (2017-09-14 09:59:44 UTC) #12
alexnarest
lgtm
2 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 ...
2 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 ...
2 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 ...
2 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 ...
2 months ago (2017-09-14 14:16:04 UTC) #17
ossu
Alright. Then I'll discard my draft and lgtm instead!
2 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
2 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)
2 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
2 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)
2 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
2 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)
2 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
2 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; ...
2 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 ...
1 month, 3 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
1 month, 3 weeks ago (2017-09-28 07:43:04 UTC) #42
commit-bot: I haz the power
1 month, 3 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 efc10ee0f