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

Issue 2931873002: Test and fix for huge bwe drop after alr state. (Closed)

Created:
3 years, 6 months ago by tschumi
Modified:
3 years, 6 months ago
CC:
webrtc-reviews_webrtc.org, zhuangzesen_agora.io, tlegrand-webrtc, tterriberry_mozilla.com, the sun, mflodman
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Test and fix for huge bwe drop after alr state. BUG=webrtc:7746 Review-Url: https://codereview.webrtc.org/2931873002 Cr-Commit-Position: refs/heads/master@{#18692} Committed: https://chromium.googlesource.com/external/webrtc/+/37aa8ba61641962119071646175bfe3bc2bda063

Patch Set 1 #

Total comments: 25

Patch Set 2 : Respond to commants #

Total comments: 6

Patch Set 3 : Removed BweTrendlineFilter field trial. #

Patch Set 4 : Renamed SentBeforeLeftAlr to SentInAlrState added a test. #

Total comments: 10

Patch Set 5 : Fixed field trial string. #

Patch Set 6 : Response to comments. #

Total comments: 15

Patch Set 7 : . #

Total comments: 4

Patch Set 8 : Respons to comments #

Total comments: 4

Patch Set 9 : . #

Patch Set 10 : Moved Dtx integraion test. #

Patch Set 11 : Rebased #

Patch Set 12 : Added missing dep. #

Patch Set 13 : fix_for_unittest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+585 lines, -207 lines) Patch
A resources/voice_engine/audio_dtx16.wav.sha1 View 1 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/audio/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +39 lines, -0 lines 0 comments Download
A webrtc/audio/test/audio_bwe_integration_test.h View 1 2 3 4 5 6 7 8 9 1 chunk +53 lines, -0 lines 0 comments Download
A webrtc/audio/test/audio_bwe_integration_test.cc View 1 2 3 4 5 6 7 8 9 1 chunk +146 lines, -0 lines 0 comments Download
M webrtc/modules/congestion_controller/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +6 lines, -2 lines 0 comments Download
M webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h View 1 1 chunk +0 lines, -48 lines 0 comments Download
D webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -115 lines 0 comments Download
A webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +55 lines, -0 lines 0 comments Download
A webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +81 lines, -0 lines 0 comments Download
A webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +177 lines, -0 lines 0 comments Download
A + webrtc/modules/congestion_controller/bitrate_estimator.h View 1 3 chunks +8 lines, -12 lines 0 comments Download
A + webrtc/modules/congestion_controller/bitrate_estimator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +8 lines, -22 lines 0 comments Download
M webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h View 1 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc View 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M webrtc/modules/congestion_controller/send_side_congestion_controller.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +5 lines, -3 lines 0 comments Download
M webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h View 1 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 78 (44 generated)
tschumi
@kwiberg, Used the std::function in acknowledge_bitrate_estimator.h for acknowledge_bitrate_estimator_unittest.cc.
3 years, 6 months ago (2017-06-08 15:15:43 UTC) #3
kwiberg-webrtc
Review of just the std::function bits. https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc#newcode32 webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:32: : &rtc::MakeUnique<BitrateEstimator>), Can ...
3 years, 6 months ago (2017-06-08 23:00:29 UTC) #6
philipel
lg, mostly stylistic comments. https://codereview.webrtc.org/2931873002/diff/1/webrtc/audio/test/low_bandwidth_audio_test.cc File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/audio/test/low_bandwidth_audio_test.cc#newcode234 webrtc/audio/test/low_bandwidth_audio_test.cc:234: TEST_F(LowBandwidthAudioTest, DTXTest) { Could you ...
3 years, 6 months ago (2017-06-09 14:49:53 UTC) #9
minyue-webrtc
On 2017/06/09 14:49:53, philipel wrote: > lg, mostly stylistic comments. > > https://codereview.webrtc.org/2931873002/diff/1/webrtc/audio/test/low_bandwidth_audio_test.cc > File ...
3 years, 6 months ago (2017-06-12 07:57:01 UTC) #11
minyue-webrtc
Good job! Take a look at my comments. I would like to understand why not ...
3 years, 6 months ago (2017-06-12 10:08:07 UTC) #12
tschumi
https://codereview.webrtc.org/2931873002/diff/1/webrtc/audio/test/low_bandwidth_audio_test.cc File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/audio/test/low_bandwidth_audio_test.cc#newcode234 webrtc/audio/test/low_bandwidth_audio_test.cc:234: TEST_F(LowBandwidthAudioTest, DTXTest) { On 2017/06/09 14:49:53, philipel wrote: > ...
3 years, 6 months ago (2017-06-12 11:33:28 UTC) #13
holmer
https://codereview.webrtc.org/2931873002/diff/20001/webrtc/audio/test/low_bandwidth_audio_test.cc File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/20001/webrtc/audio/test/low_bandwidth_audio_test.cc#newcode238 webrtc/audio/test/low_bandwidth_audio_test.cc:238: "Enabled/WebRTC-SendSideBwe-WithOverhead/Enabled/"); You can drop "WebRTC-BweTrendlineFilter/Enabled-20,0.9,4/" as it's already the ...
3 years, 6 months ago (2017-06-12 11:45:15 UTC) #15
tschumi
https://codereview.webrtc.org/2931873002/diff/20001/webrtc/audio/test/low_bandwidth_audio_test.cc File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/20001/webrtc/audio/test/low_bandwidth_audio_test.cc#newcode238 webrtc/audio/test/low_bandwidth_audio_test.cc:238: "Enabled/WebRTC-SendSideBwe-WithOverhead/Enabled/"); On 2017/06/12 11:45:14, holmer wrote: > You can ...
3 years, 6 months ago (2017-06-12 11:53:04 UTC) #16
stefan-webrtc
https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h#newcode41 webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h:41: bool SentBeforLeftAlr(const PacketFeedback& packet); On 2017/06/12 11:53:04, tschumi wrote: ...
3 years, 6 months ago (2017-06-12 12:17:23 UTC) #18
minyue-webrtc
https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_controller/bitrate_estimator.h File webrtc/modules/congestion_controller/bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_controller/bitrate_estimator.h#newcode25 webrtc/modules/congestion_controller/bitrate_estimator.h:25: class BitrateEstimator { On 2017/06/12 11:33:28, tschumi wrote: > ...
3 years, 6 months ago (2017-06-12 12:23:43 UTC) #19
tschumi
This is caused by a refactoring CL which I did to make this CL possible. ...
3 years, 6 months ago (2017-06-12 12:28:08 UTC) #20
tschumi
This is caused by a refactoring CL which I did to make this CL possible. ...
3 years, 6 months ago (2017-06-12 12:28:10 UTC) #21
tschumi
https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h#newcode41 webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h:41: bool SentBeforLeftAlr(const PacketFeedback& packet); I tried to impl. this ...
3 years, 6 months ago (2017-06-12 14:14:15 UTC) #22
terelius
https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc#newcode57 webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:57: bool AcknowledgedBitrateEstimator::SentInAlrState( I'd also prefer a more descriptive name ...
3 years, 6 months ago (2017-06-13 14:03:20 UTC) #23
tschumi
https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc#newcode57 webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:57: bool AcknowledgedBitrateEstimator::SentInAlrState( Renamed it to "SentBeforeAlrEnded" https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc#newcode63 webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:63: left_alr_state_ms_.reset(); ...
3 years, 6 months ago (2017-06-14 07:47:44 UTC) #24
philipel
https://codereview.webrtc.org/2931873002/diff/100001/webrtc/audio/test/low_bandwidth_audio_test.cc File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/audio/test/low_bandwidth_audio_test.cc#newcode209 webrtc/audio/test/low_bandwidth_audio_test.cc:209: void LogDelayBasedBweUpdate(int32_t bitrate_bps, Remove if not used. https://codereview.webrtc.org/2931873002/diff/100001/webrtc/audio/test/low_bandwidth_audio_test.cc#newcode218 webrtc/audio/test/low_bandwidth_audio_test.cc:218: ...
3 years, 6 months ago (2017-06-14 09:34:05 UTC) #25
tschumi
https://codereview.webrtc.org/2931873002/diff/100001/webrtc/audio/test/low_bandwidth_audio_test.cc File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/audio/test/low_bandwidth_audio_test.cc#newcode209 webrtc/audio/test/low_bandwidth_audio_test.cc:209: void LogDelayBasedBweUpdate(int32_t bitrate_bps, On 2017/06/14 09:34:05, philipel wrote: > ...
3 years, 6 months ago (2017-06-14 10:57:46 UTC) #26
kwiberg-webrtc
https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc#newcode36 webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc:36: struct AcknowledgedBitrateEstimatorTestStates { On 2017/06/14 10:57:46, tschumi wrote: > ...
3 years, 6 months ago (2017-06-14 11:49:21 UTC) #27
philipel
lgtm https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc#newcode36 webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc:36: struct AcknowledgedBitrateEstimatorTestStates { On 2017/06/14 11:49:21, kwiberg-webrtc wrote: ...
3 years, 6 months ago (2017-06-15 16:13:22 UTC) #28
stefan-webrtc
lg, but i'm a bit worried about the event log mocking. can we get around ...
3 years, 6 months ago (2017-06-16 13:53:28 UTC) #29
tschumi
https://codereview.webrtc.org/2931873002/diff/120001/webrtc/audio/test/low_bandwidth_audio_test.cc File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/120001/webrtc/audio/test/low_bandwidth_audio_test.cc#newcode181 webrtc/audio/test/low_bandwidth_audio_test.cc:181: class NoBandwidthDropAfterDTX : public AudioQualityTest { On 2017/06/16 13:53:27, ...
3 years, 6 months ago (2017-06-19 08:49:20 UTC) #30
holmer
lgtm % nit https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_bandwidth_audio_test.h File webrtc/audio/test/low_bandwidth_audio_test.h (right): https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_bandwidth_audio_test.h#newcode54 webrtc/audio/test/low_bandwidth_audio_test.h:54: Call* sender_call_; AFAICT this can still ...
3 years, 6 months ago (2017-06-19 08:53:49 UTC) #31
tschumi
https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_bandwidth_audio_test.h File webrtc/audio/test/low_bandwidth_audio_test.h (right): https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_bandwidth_audio_test.h#newcode54 webrtc/audio/test/low_bandwidth_audio_test.h:54: Call* sender_call_; I think i could move it in ...
3 years, 6 months ago (2017-06-19 10:36:03 UTC) #32
holmer
https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_bandwidth_audio_test.h File webrtc/audio/test/low_bandwidth_audio_test.h (right): https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_bandwidth_audio_test.h#newcode54 webrtc/audio/test/low_bandwidth_audio_test.h:54: Call* sender_call_; On 2017/06/19 10:36:03, tschumi wrote: > I ...
3 years, 6 months ago (2017-06-19 11:51:53 UTC) #33
tschumi
https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_bandwidth_audio_test.h File webrtc/audio/test/low_bandwidth_audio_test.h (right): https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_bandwidth_audio_test.h#newcode54 webrtc/audio/test/low_bandwidth_audio_test.h:54: Call* sender_call_; Ok moved sender_call_ to NoBandwidthDropAfterDtx.
3 years, 6 months ago (2017-06-19 12:08:48 UTC) #34
oprypin_webrtc
Regarding low_bandwidth_audio_test.cc: it's meant to be a performance test. It doesn't seem quite right to ...
3 years, 6 months ago (2017-06-19 12:20:07 UTC) #35
tschumi
I think you are right that the test don't relay belong to each other since ...
3 years, 6 months ago (2017-06-19 12:40:30 UTC) #36
tschumi
Moved Dtx test to webrtc pref test.
3 years, 6 months ago (2017-06-20 07:26:12 UTC) #37
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/2931873002/320001
3 years, 6 months ago (2017-06-21 06:22:37 UTC) #68
commit-bot: I haz the power
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18415)
3 years, 6 months ago (2017-06-21 06:26:57 UTC) #70
stefan-webrtc
On 2017/06/21 06:26:57, commit-bot: I haz the power wrote: > Try jobs failed on following ...
3 years, 6 months ago (2017-06-21 06:39:48 UTC) #71
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/2931873002/320001
3 years, 6 months ago (2017-06-21 06:40:17 UTC) #73
commit-bot: I haz the power
Committed patchset #13 (id:320001) as https://chromium.googlesource.com/external/webrtc/+/37aa8ba61641962119071646175bfe3bc2bda063
3 years, 6 months ago (2017-06-21 06:42:42 UTC) #76
terelius
3 years, 5 months ago (2017-06-30 14:08:51 UTC) #78
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:320001) has been created in
https://codereview.webrtc.org/2964213002/ by terelius@webrtc.org.

The reason for reverting is: Resetting the estimate means that we need to start
gathering data from scratch again. The combination of
1) DelayBasedEstimator not reacting to overuse unless there is a valid estimate
of the acknowledged bitrate, and 
2) AcknowledgedBitrateEstimator needing a significant amount of time/data to
obtain an provide an estimate
causes poor performance in simulations/tests. It is not clear whether this will
affect real networks negatively, but I suggest reverting this to be on the safe
side.
See also https://bugs.chromium.org/p/webrtc/issues/detail?id=7884.

Powered by Google App Engine
This is Rietveld 408576698