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

Issue 1535963002: Wire-up BWE feedback for audio receive streams. (Closed)

Created:
5 years ago by stefan-webrtc
Modified:
4 years, 11 months ago
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), zhuangzesen_agora.io, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, mflodman
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Wire-up BWE feedback for audio receive streams. Also wires up receiving transport sequence numbers. BUG=webrtc:5263 R=mflodman@webrtc.org, pbos@webrtc.org, solenberg@webrtc.org Committed: https://crrev.com/3842c5c7f73639527e653f41c65334245d2317a1 Cr-Commit-Position: refs/heads/master@{#11220}

Patch Set 1 #

Patch Set 2 : Cleanup and improve test coverage. #

Patch Set 3 : Move mocks. #

Patch Set 4 : Send-stream tests fixed. #

Patch Set 5 : . #

Patch Set 6 : Fix audio receive stream test expectation. #

Total comments: 14

Patch Set 7 : Comments addressed. #

Total comments: 25

Patch Set 8 : Comments addressed #

Total comments: 19

Patch Set 9 : Comments addressed #

Patch Set 10 : Fix a test I forgot to run. #

Total comments: 14

Patch Set 11 : Comments addressed. #

Total comments: 5

Patch Set 12 : Comment addressed. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+295 lines, -89 lines) Patch
M webrtc/audio/audio_receive_stream.h View 1 2 3 4 5 6 7 8 3 chunks +3 lines, -2 lines 0 comments Download
M webrtc/audio/audio_receive_stream.cc View 1 2 3 4 5 6 7 8 9 6 chunks +42 lines, -10 lines 0 comments Download
M webrtc/audio/audio_receive_stream_unittest.cc View 1 2 3 4 5 6 7 8 9 10 9 chunks +119 lines, -36 lines 0 comments Download
M webrtc/audio/audio_send_stream_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -9 lines 0 comments Download
M webrtc/audio_receive_stream.h View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download
M webrtc/call/call.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -2 lines 0 comments Download
M webrtc/call/congestion_controller.h View 1 2 3 4 5 6 7 8 2 chunks +23 lines, -17 lines 0 comments Download
A webrtc/call/mock/mock_congestion_controller.h View 1 2 3 4 5 6 7 8 1 chunk +52 lines, -0 lines 0 comments Download
A webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h View 1 2 3 4 5 6 1 chunk +30 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_utility.cc View 1 2 3 4 5 6 7 3 chunks +5 lines, -6 lines 0 comments Download
M webrtc/voice_engine/channel.cc View 1 2 3 4 5 6 7 8 1 chunk +12 lines, -7 lines 0 comments Download

Messages

Total messages: 54 (16 generated)
stefan-webrtc
Cleanup and improve test coverage.
5 years ago (2015-12-18 12:57:56 UTC) #1
stefan-webrtc
Send-stream tests fixed.
5 years ago (2015-12-18 13:44:56 UTC) #2
stefan-webrtc
.
5 years ago (2015-12-18 13:58:55 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1535963002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535963002/80001
5 years ago (2015-12-18 14:00:46 UTC) #5
stefan-webrtc
Fix audio receive stream test expectation.
5 years ago (2015-12-18 14:02:40 UTC) #7
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1535963002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535963002/100001
5 years ago (2015-12-18 14:03:06 UTC) #9
pbos-webrtc
webrtc/call lgtm (but I don't like having these virtuals :()
5 years ago (2015-12-18 14:22:33 UTC) #11
stefan-webrtc
5 years ago (2015-12-18 14:27:33 UTC) #13
stefan-webrtc
solenberg, please take a look
5 years ago (2015-12-18 14:37:38 UTC) #16
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) ...
5 years ago (2015-12-18 16:03:23 UTC) #18
the sun
A few first comments https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_receive_stream_unittest.cc File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_receive_stream_unittest.cc#newcode64 webrtc/audio/audio_receive_stream_unittest.cc:64: : call_stats_(Clock::GetRealTimeClock()), RealTimeClock? Can we ...
5 years ago (2015-12-20 23:16:15 UTC) #19
stefan-webrtc
Comments addressed.
5 years ago (2015-12-21 08:01:14 UTC) #20
stefan-webrtc
https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_receive_stream_unittest.cc File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_receive_stream_unittest.cc#newcode64 webrtc/audio/audio_receive_stream_unittest.cc:64: : call_stats_(Clock::GetRealTimeClock()), On 2015/12/20 23:16:15, the sun OOO until ...
5 years ago (2015-12-21 08:01:29 UTC) #21
the sun
https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_receive_stream_unittest.cc File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_receive_stream_unittest.cc#newcode64 webrtc/audio/audio_receive_stream_unittest.cc:64: : call_stats_(Clock::GetRealTimeClock()), On 2015/12/21 08:01:29, stefan-webrtc (holmer) wrote: > ...
5 years ago (2015-12-22 00:14:14 UTC) #22
stefan-webrtc
Comments addressed
4 years, 11 months ago (2016-01-07 13:42:58 UTC) #23
stefan-webrtc
https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_receive_stream.cc File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_receive_stream.cc#newcode36 webrtc/audio/audio_receive_stream.cc:36: static bool UseSendSideBwe(const webrtc::AudioReceiveStream::Config& config) { On 2015/12/22 00:14:14, ...
4 years, 11 months ago (2016-01-07 13:43:41 UTC) #24
the sun
https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_receive_stream.cc File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_receive_stream.cc#newcode87 webrtc/audio/audio_receive_stream.cc:87: ? congestion_controller->GetRemoteBitrateEstimator( On 2016/01/07 13:43:40, stefan-webrtc (holmer) wrote: > ...
4 years, 11 months ago (2016-01-08 10:29:36 UTC) #25
stefan-webrtc
Comments addressed
4 years, 11 months ago (2016-01-08 15:35:59 UTC) #26
stefan-webrtc
https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_receive_stream.cc File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_receive_stream.cc#newcode87 webrtc/audio/audio_receive_stream.cc:87: : remote_bitrate_estimator_( On 2016/01/08 10:29:35, the sun wrote: > ...
4 years, 11 months ago (2016-01-08 15:36:02 UTC) #27
stefan-webrtc
Fix a test I forgot to run.
4 years, 11 months ago (2016-01-08 16:00:23 UTC) #28
the sun
just a few nits in the ARS test. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/mock/mock_congestion_controller.h File webrtc/call/mock/mock_congestion_controller.h (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/mock/mock_congestion_controller.h#newcode25 webrtc/call/mock/mock_congestion_controller.h:25: : ...
4 years, 11 months ago (2016-01-11 11:47:20 UTC) #29
stefan-webrtc
Comments addressed.
4 years, 11 months ago (2016-01-11 17:15:41 UTC) #30
stefan-webrtc
https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_receive_stream_unittest.cc File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_receive_stream_unittest.cc#newcode81 webrtc/audio/audio_receive_stream_unittest.cc:81: channel_proxy_ = new testing::StrictMock<MockVoEChannelProxy>(); On 2016/01/11 11:47:19, the sun ...
4 years, 11 months ago (2016-01-11 17:15:48 UTC) #31
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1535963002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535963002/200001
4 years, 11 months ago (2016-01-11 18:24:07 UTC) #33
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2782)
4 years, 11 months ago (2016-01-11 18:46:15 UTC) #35
the sun
one more thought, promise! https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_controller.cc File webrtc/call/congestion_controller.cc (right): https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_controller.cc#newcode150 webrtc/call/congestion_controller.cc:150: : remb_(new VieRemb(Clock::GetRealTimeClock())), On 2016/01/11 ...
4 years, 11 months ago (2016-01-12 10:24:14 UTC) #36
stefan-webrtc
https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_controller.cc File webrtc/call/congestion_controller.cc (right): https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_controller.cc#newcode150 webrtc/call/congestion_controller.cc:150: : remb_(new VieRemb(Clock::GetRealTimeClock())), On 2016/01/12 10:24:13, the sun wrote: ...
4 years, 11 months ago (2016-01-12 11:15:16 UTC) #37
the sun
On 2016/01/12 11:15:16, stefan-webrtc (holmer) wrote: > https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_controller.cc > File webrtc/call/congestion_controller.cc (right): > > https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_controller.cc#newcode150 ...
4 years, 11 months ago (2016-01-12 11:57:13 UTC) #38
the sun
lgtm
4 years, 11 months ago (2016-01-12 11:57:24 UTC) #39
stefan-webrtc
https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/mock/mock_congestion_controller.h File webrtc/call/mock/mock_congestion_controller.h (right): https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/mock/mock_congestion_controller.h#newcode25 webrtc/call/mock/mock_congestion_controller.h:25: : CongestionController(process_thread, call_stats, bitrate_observer) {} On 2016/01/12 11:15:15, stefan-webrtc ...
4 years, 11 months ago (2016-01-12 11:58:47 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1535963002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1535963002/200001
4 years, 11 months ago (2016-01-12 11:59:01 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2797)
4 years, 11 months ago (2016-01-12 12:10:50 UTC) #45
stefan-webrtc
Magnus, can you take a look, specifically at webrtc/?
4 years, 11 months ago (2016-01-12 12:29:08 UTC) #47
mflodman
One comment, then LGTM. https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/congestion_controller.cc File webrtc/call/congestion_controller.cc (right): https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/congestion_controller.cc#newcode172 webrtc/call/congestion_controller.cc:172: RTC_DCHECK(bitrate_observer); I don't this check ...
4 years, 11 months ago (2016-01-12 12:42:53 UTC) #48
stefan-webrtc
https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/congestion_controller.cc File webrtc/call/congestion_controller.cc (right): https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/congestion_controller.cc#newcode172 webrtc/call/congestion_controller.cc:172: RTC_DCHECK(bitrate_observer); On 2016/01/12 12:42:53, mflodman wrote: > I don't ...
4 years, 11 months ago (2016-01-12 12:53:18 UTC) #49
stefan-webrtc
Comment addressed.
4 years, 11 months ago (2016-01-12 12:54:43 UTC) #50
commit-bot: I haz the power
Patchset 12 (id:??) landed as https://crrev.com/3842c5c7f73639527e653f41c65334245d2317a1 Cr-Commit-Position: refs/heads/master@{#11220}
4 years, 11 months ago (2016-01-12 12:55:18 UTC) #53
stefan-webrtc
4 years, 11 months ago (2016-01-12 12:55:18 UTC) #54
Message was sent while issue was closed.
Committed patchset #12 (id:220001) manually as
3842c5c7f73639527e653f41c65334245d2317a1 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698