Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionWire-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. #
Messages
Total messages: 54 (16 generated)
Cleanup and improve test coverage.
Send-stream tests fixed.
.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by stefan@webrtc.org
Fix audio receive stream test expectation.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
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
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
webrtc/call lgtm (but I don't like having these virtuals :()
stefan@webrtc.org changed reviewers: + solenberg@webrtc.org
Description was changed from ========== Wire-up BWE feedback for audio receive streams. BUG=webrtc:5263 ========== to ========== Wire-up BWE feedback for audio receive streams. Also wires up receiving transport sequence numbers. BUG=webrtc:5263 ==========
Description was changed from ========== Wire-up BWE feedback for audio receive streams. BUG=webrtc:5263 ========== to ========== Wire-up BWE feedback for audio receive streams. Also wires up receiving transport sequence numbers. BUG=webrtc:5263 ==========
solenberg, please take a look
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
A few first comments https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_rec... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream_unittest.cc:64: : call_stats_(Clock::GetRealTimeClock()), RealTimeClock? Can we run on simulated time instead? https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_sen... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_sen... webrtc/audio/audio_send_stream_unittest.cc:70: channel_proxy_ = new testing::StrictMock<MockVoEChannelProxy>(); Why did this move? https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio_receive_s... File webrtc/audio_receive_stream.h (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio_receive_s... webrtc/audio_receive_stream.h:76: // See draft-holmer-rmcat-transport-wide-cc-extensions for details. // Enable send side bandwidth estimation. // See .... for details. ? https://codereview.chromium.org/1535963002/diff/100001/webrtc/call/mock/mock_... File webrtc/call/mock/mock_congestion_controller.h (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/call/mock/mock_... webrtc/call/mock/mock_congestion_controller.h:17: namespace webrtc { I'd put this under namespace webrtc { namespace test { https://codereview.chromium.org/1535963002/diff/100001/webrtc/modules/bitrate... File webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/modules/bitrate... webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h:17: namespace webrtc { I add test code under namespace webrtc { namespace test { because it feels good to keep the test code separate. https://codereview.chromium.org/1535963002/diff/100001/webrtc/modules/rtp_rtc... File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/modules/rtp_rtc... webrtc/modules/rtp_rtcp/source/rtp_utility.cc:399: << static_cast<int>(len); Would it be easier to store the length in an int? :)
Comments addressed.
https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_rec... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream_unittest.cc:64: : call_stats_(Clock::GetRealTimeClock()), On 2015/12/20 23:16:15, the sun OOO until jan 7th wrote: > RealTimeClock? Can we run on simulated time instead? There is nothing stopping us, but I don't really see the point until we actually need the functionality provided by a simulated clock. If you really would prefer it I could change it. https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_sen... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_sen... webrtc/audio/audio_send_stream_unittest.cc:70: channel_proxy_ = new testing::StrictMock<MockVoEChannelProxy>(); On 2015/12/20 23:16:15, the sun OOO until jan 7th wrote: > Why did this move? Sorry, I can move this back. I had to move it initially, but fixed the underlying problem has been fixed. https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio_receive_s... File webrtc/audio_receive_stream.h (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio_receive_s... webrtc/audio_receive_stream.h:76: // See draft-holmer-rmcat-transport-wide-cc-extensions for details. On 2015/12/20 23:16:15, the sun OOO until jan 7th wrote: > // Enable send side bandwidth estimation. > // See .... for details. > ? Done. https://codereview.chromium.org/1535963002/diff/100001/webrtc/call/mock/mock_... File webrtc/call/mock/mock_congestion_controller.h (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/call/mock/mock_... webrtc/call/mock/mock_congestion_controller.h:17: namespace webrtc { On 2015/12/20 23:16:15, the sun OOO until jan 7th wrote: > I'd put this under > > namespace webrtc { > namespace test { Sure, sounds good. https://codereview.chromium.org/1535963002/diff/100001/webrtc/modules/bitrate... File webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/modules/bitrate... webrtc/modules/bitrate_controller/include/mock/mock_bitrate_controller.h:17: namespace webrtc { On 2015/12/20 23:16:15, the sun OOO until jan 7th wrote: > I add test code under > > namespace webrtc { > namespace test { > > because it feels good to keep the test code separate. Done. https://codereview.chromium.org/1535963002/diff/100001/webrtc/modules/rtp_rtc... File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/modules/rtp_rtc... webrtc/modules/rtp_rtcp/source/rtp_utility.cc:399: << static_cast<int>(len); On 2015/12/20 23:16:15, the sun OOO until jan 7th wrote: > Would it be easier to store the length in an int? :) Haha, agreed.
https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_rec... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream_unittest.cc:64: : call_stats_(Clock::GetRealTimeClock()), On 2015/12/21 08:01:29, stefan-webrtc (holmer) wrote: > On 2015/12/20 23:16:15, the sun OOO until jan 7th wrote: > > RealTimeClock? Can we run on simulated time instead? > > There is nothing stopping us, but I don't really see the point until we actually > need the functionality provided by a simulated clock. If you really would prefer > it I could change it. So we don't actually need a clock at all? Then we should have a mock clock. My concern is that if we add a RT clock here we *will* depend on it somewhere down the line, then adding test flakiness and inefficiency. https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_sen... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.chromium.org/1535963002/diff/100001/webrtc/audio/audio_sen... webrtc/audio/audio_send_stream_unittest.cc:70: channel_proxy_ = new testing::StrictMock<MockVoEChannelProxy>(); On 2015/12/21 08:01:29, stefan-webrtc (holmer) wrote: > On 2015/12/20 23:16:15, the sun OOO until jan 7th wrote: > > Why did this move? > > Sorry, I can move this back. I had to move it initially, but fixed the > underlying problem has been fixed. Please do. https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream.cc:36: static bool UseSendSideBwe(const webrtc::AudioReceiveStream::Config& config) { no need to declare static inside an anonymous namespace? https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream.cc:37: if (!config.rtp.transport_cc) nit: This file consistently uses {} for one-line control statements. https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream.cc:87: ? congestion_controller->GetRemoteBitrateEstimator( Why not have two accessor methods on the CC? Sending in a flag to tell the function which member to return is not a super nice interface. I think you should also consider making the returned RBE ref counted so we can keep it in a scoped_refptr here. I'm ok with you addressing both in a separate CL though. https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream.cc:121: channel_proxy_->SetCongestionControlObjects( We are relying on the underlying channel *only* being used for either send or receive? https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream.cc:131: channel_proxy_->SetCongestionControlObjects(nullptr, nullptr, nullptr); Can we always set the CC obj nullptrs? Make the code a little simpler. https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream.cc:173: header.extension.hasTransportSequenceNumber)) { No need to logic-and with confg_.rtp.transport_cc for transport seq num? The RBE will handle it? https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream_unittest.cc:65: process_thread_(ProcessThread::Create("AudioTestThread")), Are we relying on this thread to run somehow? Or is it just to satisfy the CC c-tor? Can we use a mock instead? https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream_unittest.cc:130: ASSERT_TRUE(channel_proxy_); thanks https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream_unittest.cc:159: void BuildOneByteExtension(uint8_t* buffer, vector<uint8_t>::iterator? https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream_unittest.cc:163: const size_t kRtpOneByteHeaderLength = 4; Can this be removed? https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio/audio_rec... webrtc/audio/audio_receive_stream_unittest.cc:178: size_t CreateRtpHeaderWithOneByteExtension(uint8_t* header, How about just returning a vector<uint8_t> by value. Safer. https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio_receive_s... File webrtc/audio_receive_stream.h (right): https://codereview.chromium.org/1535963002/diff/120001/webrtc/audio_receive_s... webrtc/audio_receive_stream.h:77: // See draft-holmer-rmcat-transport-wide-cc-extensions for details. nit: can you make it a url even? https://codereview.chromium.org/1535963002/diff/120001/webrtc/voice_engine/ch... File webrtc/voice_engine/channel.cc (right): https://codereview.chromium.org/1535963002/diff/120001/webrtc/voice_engine/ch... webrtc/voice_engine/channel.cc:2930: RTC_DCHECK(packet_router != nullptr || packet_router_ != nullptr); Since we are relying on the channel only being used for either of send and receive, and there being nothing to prevent the same channel id from being specified in the Config for AudioReceive/SendStreams, it would be good to have DCHECKs here to cover the expected cases: 1. internal objects null && all arguments non-null 2. internal objects null && packet router non-null 3. internal objects all non-null && all arguments null 4. packet_router_ non null && all arguments null ...if it is possible to write that in a way which isn't atrocious...
Comments addressed
https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:36: static bool UseSendSideBwe(const webrtc::AudioReceiveStream::Config& config) { On 2015/12/22 00:14:14, the sun wrote: > no need to declare static inside an anonymous namespace? Done. https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:37: if (!config.rtp.transport_cc) On 2015/12/22 00:14:13, the sun wrote: > nit: This file consistently uses {} for one-line control statements. Done. https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:87: ? congestion_controller->GetRemoteBitrateEstimator( On 2015/12/22 00:14:14, the sun wrote: > Why not have two accessor methods on the CC? Sending in a flag to tell the > function which member to return is not a super nice interface. > > I think you should also consider making the returned RBE ref counted so we can > keep it in a scoped_refptr here. > > I'm ok with you addressing both in a separate CL though. Yes, I agree that it'd be nicer with two accessors. I'm not sure if it should be refcounted though, I think it's simpler to have a single owner (CongestionController). Anyway, I'm making that change in a different CL. https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:121: channel_proxy_->SetCongestionControlObjects( On 2015/12/22 00:14:14, the sun wrote: > We are relying on the underlying channel *only* being used for either send or > receive? Yes, but the packet router is needed both when sending and receiving. In the receiving case it's needed for sending RTCP. https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:131: channel_proxy_->SetCongestionControlObjects(nullptr, nullptr, nullptr); On 2015/12/22 00:14:13, the sun wrote: > Can we always set the CC obj nullptrs? Make the code a little simpler. Yes, good suggestion. https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:173: header.extension.hasTransportSequenceNumber)) { On 2015/12/22 00:14:13, the sun wrote: > No need to logic-and with confg_.rtp.transport_cc for transport seq num? The RBE > will handle it? I think it's a matter of interpretation. Should audio packets be included in the feedback message even though the audio receive stream itself doesn't send feedback messages? I think that would be ok. https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:65: process_thread_(ProcessThread::Create("AudioTestThread")), On 2015/12/22 00:14:14, the sun wrote: > Are we relying on this thread to run somehow? Or is it just to satisfy the CC > c-tor? Can we use a mock instead? Done. https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:163: const size_t kRtpOneByteHeaderLength = 4; On 2015/12/22 00:14:14, the sun wrote: > Can this be removed? Done. https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:178: size_t CreateRtpHeaderWithOneByteExtension(uint8_t* header, On 2015/12/22 00:14:14, the sun wrote: > How about just returning a vector<uint8_t> by value. Safer. Done. https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio_receive_str... File webrtc/audio_receive_stream.h (right): https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio_receive_str... webrtc/audio_receive_stream.h:77: // See draft-holmer-rmcat-transport-wide-cc-extensions for details. On 2015/12/22 00:14:14, the sun wrote: > nit: can you make it a url even? Done. https://codereview.webrtc.org/1535963002/diff/120001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1535963002/diff/120001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2930: RTC_DCHECK(packet_router != nullptr || packet_router_ != nullptr); On 2015/12/22 00:14:14, the sun wrote: > Since we are relying on the channel only being used for either of send and > receive, and there being nothing to prevent the same channel id from being > specified in the Config for AudioReceive/SendStreams, it would be good to have > DCHECKs here to cover the expected cases: > > 1. internal objects null && all arguments non-null > 2. internal objects null && packet router non-null > 3. internal objects all non-null && all arguments null > 4. packet_router_ non null && all arguments null > > ...if it is possible to write that in a way which isn't atrocious... I think it's non-trivial to test those without knowing if the channel is used for sending or receiving. That is currently unknown and I'd prefer not wiring up something like that for this cl. What I can check is that if all members are null, then at least the packet_router parameter should not be null. Since we allow calling with null on all parameters even if SetCongestionControlObjects() weren't called in the constructor of AudioReceiveStream, we can't check that some of the members are non-null when all parameters are null. Not sure if it's worth adding getters to all proxies just to be able to do this one DCHECK though?
https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/1535963002/diff/120001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:87: ? congestion_controller->GetRemoteBitrateEstimator( On 2016/01/07 13:43:40, stefan-webrtc (holmer) wrote: > On 2015/12/22 00:14:14, the sun wrote: > > Why not have two accessor methods on the CC? Sending in a flag to tell the > > function which member to return is not a super nice interface. > > > > I think you should also consider making the returned RBE ref counted so we can > > keep it in a scoped_refptr here. > > > > I'm ok with you addressing both in a separate CL though. > > Yes, I agree that it'd be nicer with two accessors. I'm not sure if it should be > refcounted though, I think it's simpler to have a single owner > (CongestionController). In that case, making the CC ref counted would make the object life times clearer+safer. > Anyway, I'm making that change in a different CL. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:87: : remote_bitrate_estimator_( I'd prefer if you set remote_bitrate_estimator_ = nullptr in the .h and move this initialization into the block starting at line 122 below. Then all CC/RBE setup is in one place. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:96: RTC_DCHECK_NE(config_.voe_channel_id, -1); RTC_DCHECK(congestion_controller); https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:123: channel_proxy_->SetCongestionControlObjects( // Configure bandwidth estimation. if (config.combined_audio_video_bwe) { if (UseSendSideBwe(config)) { remote_bitrate_estimator_ = congestion_controller->GetRemoteBitrateEstimator(true); channel_proxy_->SetCongestionControlObjects( nullptr, nullptr, congestion_controller->packet_router()); } else { remote_bitrate_estimator_ = congestion_controller->GetRemoteBitrateEstimator(false); } RTC_DCHECK(remote_bitrate_estimator_); } https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:133: remote_bitrate_estimator_->RemoveStream(config_.rtp.remote_ssrc); It would be simpler to make the call conditional on remote_bitrate_estimator_. Then the actual logic doesn't need to match/be repeated. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:172: if (config_.combined_audio_video_bwe && Ditto: if (remote_bitrate_estimator_ && ... https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:66: : call_stats_(Clock::GetRealTimeClock()), Please mock the clock. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/congestion_c... File webrtc/call/congestion_controller.h (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/congestion_c... webrtc/call/congestion_controller.h:88: int min_bitrate_bps_; RTC_DISALLOW_IMPLICIT_CONSTRUCTORS https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/mock/mock_co... File webrtc/call/mock/mock_congestion_controller.h (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/mock/mock_co... webrtc/call/mock/mock_congestion_controller.h:25: : CongestionController(process_thread, call_stats, bitrate_observer) {} I wouldn't mind moving the process thread mock, call stats and bitrate observer mock objects into this class to avoid setup boilerplate elsewhere. Understand that use cases may pop up where the configurable version would be needed, but since we don't have those right now it could be better. WDYT? https://codereview.webrtc.org/1535963002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2940: if (seq_num_allocator_proxy_.get()) nit: {}
Comments addressed
https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:87: : remote_bitrate_estimator_( On 2016/01/08 10:29:35, the sun wrote: > I'd prefer if you set remote_bitrate_estimator_ = nullptr in the .h and move > this initialization into the block starting at line 122 below. Then all CC/RBE > setup is in one place. I'll let you decide since it's in your code, but this means I'll have to drop the const on remote_bitrate_estimator_. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:96: RTC_DCHECK_NE(config_.voe_channel_id, -1); On 2016/01/08 10:29:35, the sun wrote: > RTC_DCHECK(congestion_controller); Done. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:123: channel_proxy_->SetCongestionControlObjects( On 2016/01/08 10:29:36, the sun wrote: > // Configure bandwidth estimation. > if (config.combined_audio_video_bwe) { > if (UseSendSideBwe(config)) { > remote_bitrate_estimator_ = > congestion_controller->GetRemoteBitrateEstimator(true); > channel_proxy_->SetCongestionControlObjects( > nullptr, nullptr, congestion_controller->packet_router()); > } else { > remote_bitrate_estimator_ = > congestion_controller->GetRemoteBitrateEstimator(false); > } > RTC_DCHECK(remote_bitrate_estimator_); > } Done. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:133: remote_bitrate_estimator_->RemoveStream(config_.rtp.remote_ssrc); On 2016/01/08 10:29:35, the sun wrote: > It would be simpler to make the call conditional on remote_bitrate_estimator_. > Then the actual logic doesn't need to match/be repeated. Done. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:172: if (config_.combined_audio_video_bwe && On 2016/01/08 10:29:35, the sun wrote: > Ditto: > if (remote_bitrate_estimator_ && > ... Done. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:66: : call_stats_(Clock::GetRealTimeClock()), On 2016/01/08 10:29:36, the sun wrote: > Please mock the clock. I'll pass in simulated time. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/congestion_c... File webrtc/call/congestion_controller.h (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/congestion_c... webrtc/call/congestion_controller.h:88: int min_bitrate_bps_; On 2016/01/08 10:29:36, the sun wrote: > RTC_DISALLOW_IMPLICIT_CONSTRUCTORS Done. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/mock/mock_co... File webrtc/call/mock/mock_congestion_controller.h (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/mock/mock_co... webrtc/call/mock/mock_congestion_controller.h:25: : CongestionController(process_thread, call_stats, bitrate_observer) {} On 2016/01/08 10:29:36, the sun wrote: > I wouldn't mind moving the process thread mock, call stats and bitrate observer > mock objects into this class to avoid setup boilerplate elsewhere. Understand > that use cases may pop up where the configurable version would be needed, but > since we don't have those right now it could be better. WDYT? I don't think that is possible. CongestionController() must be called before any members in MockCongestionController are initialized, and CongestionController will call bitrate_observer on construction. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2940: if (seq_num_allocator_proxy_.get()) On 2016/01/08 10:29:36, the sun wrote: > nit: {} Done.
Fix a test I forgot to run.
just a few nits in the ARS test. https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/mock/mock_co... File webrtc/call/mock/mock_congestion_controller.h (right): https://codereview.webrtc.org/1535963002/diff/140001/webrtc/call/mock/mock_co... webrtc/call/mock/mock_congestion_controller.h:25: : CongestionController(process_thread, call_stats, bitrate_observer) {} On 2016/01/08 15:36:02, stefan-webrtc (holmer) wrote: > On 2016/01/08 10:29:36, the sun wrote: > > I wouldn't mind moving the process thread mock, call stats and bitrate > observer > > mock objects into this class to avoid setup boilerplate elsewhere. Understand > > that use cases may pop up where the configurable version would be needed, but > > since we don't have those right now it could be better. WDYT? > > I don't think that is possible. CongestionController() must be called before any > members in MockCongestionController are initialized, and CongestionController > will call bitrate_observer on construction. Acknowledged. https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:81: channel_proxy_ = new testing::StrictMock<MockVoEChannelProxy>(); Move back into ChannelProxyFactory() so this test works the same as that for audio_send_stream https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:105: stream_config_.rtp.extensions.push_back( Should AST be mutually exclusive with transport seq num? https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:154: MockCongestionController congestion_controller_; Do you need to NiceMock or StrictMock the CC? Check test output for warnings about unexpected calls. https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:238: EXPECT_CALL(*helper.congestion_controller(), GetRemoteBitrateEstimator(false)) Except for the argument to GetRemoteBitrateEstimator, this is the same as what SetupMockForBweFeedback does? Could you make that function take the bool as argument to align the code in these two tests a bit more? https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... File webrtc/call/congestion_controller.cc (right): https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... webrtc/call/congestion_controller.cc:150: : remb_(new VieRemb(Clock::GetRealTimeClock())), separate CL? We now have the MockCC depending on real time clock. https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... webrtc/call/congestion_controller.cc:172: RTC_DCHECK(bitrate_observer != nullptr); nit: != nullptr not necessary
Comments addressed.
https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream_unittest.cc (right): https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:81: channel_proxy_ = new testing::StrictMock<MockVoEChannelProxy>(); On 2016/01/11 11:47:19, the sun wrote: > Move back into ChannelProxyFactory() so this test works the same as that for > audio_send_stream Done. https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:105: stream_config_.rtp.extensions.push_back( On 2016/01/11 11:47:19, the sun wrote: > Should AST be mutually exclusive with transport seq num? I don't think there's any reason to enforce that at the receiver, or do you suggest I should? https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:154: MockCongestionController congestion_controller_; On 2016/01/11 11:47:19, the sun wrote: > Do you need to NiceMock or StrictMock the CC? Check test output for warnings > about unexpected calls. Nope, not that I can see. https://codereview.webrtc.org/1535963002/diff/180001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream_unittest.cc:238: EXPECT_CALL(*helper.congestion_controller(), GetRemoteBitrateEstimator(false)) On 2016/01/11 11:47:19, the sun wrote: > Except for the argument to GetRemoteBitrateEstimator, this is the same as what > SetupMockForBweFeedback does? Could you make that function take the bool as > argument to align the code in these two tests a bit more? Good point. https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... File webrtc/call/congestion_controller.cc (right): https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... webrtc/call/congestion_controller.cc:150: : remb_(new VieRemb(Clock::GetRealTimeClock())), On 2016/01/11 11:47:19, the sun wrote: > separate CL? > We now have the MockCC depending on real time clock. Maybe, on the other hand it will never be affected by it since it's not possible to access remb_ in any way. If you are using the MockCC you will not be able to call any methods of the base CongestionController? Not sure it makes sense to expose the clock in CongestionController if we are not using it in a test? https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... webrtc/call/congestion_controller.cc:172: RTC_DCHECK(bitrate_observer != nullptr); On 2016/01/11 11:47:19, the sun wrote: > nit: != nullptr not necessary Done.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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)
one more thought, promise! https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... File webrtc/call/congestion_controller.cc (right): https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... webrtc/call/congestion_controller.cc:150: : remb_(new VieRemb(Clock::GetRealTimeClock())), On 2016/01/11 17:15:48, stefan-webrtc (holmer) wrote: > On 2016/01/11 11:47:19, the sun wrote: > > separate CL? > > We now have the MockCC depending on real time clock. > > Maybe, on the other hand it will never be affected by it since it's not possible > to access remb_ in any way. If you are using the MockCC you will not be able to > call any methods of the base CongestionController? > > Not sure it makes sense to expose the clock in CongestionController if we are > not using it in a test? However confusingly phrased, my concern was really more about testability of the CC class itself... :) https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/mock/mock_co... File webrtc/call/mock/mock_congestion_controller.h (right): https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/mock/mock_co... webrtc/call/mock/mock_congestion_controller.h:25: : CongestionController(process_thread, call_stats, bitrate_observer) {} Sorry for not thinking of this before, but you *could* add a default ctor to CC and avoid some of the messy setting up.
https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... File webrtc/call/congestion_controller.cc (right): https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... webrtc/call/congestion_controller.cc:150: : remb_(new VieRemb(Clock::GetRealTimeClock())), On 2016/01/12 10:24:13, the sun wrote: > On 2016/01/11 17:15:48, stefan-webrtc (holmer) wrote: > > On 2016/01/11 11:47:19, the sun wrote: > > > separate CL? > > > We now have the MockCC depending on real time clock. > > > > Maybe, on the other hand it will never be affected by it since it's not > possible > > to access remb_ in any way. If you are using the MockCC you will not be able > to > > call any methods of the base CongestionController? > > > > Not sure it makes sense to expose the clock in CongestionController if we are > > not using it in a test? > > However confusingly phrased, my concern was really more about testability of the > CC class itself... :) I see, then let's leave that to when we actually make changes to the CC class https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/mock/mock_co... File webrtc/call/mock/mock_congestion_controller.h (right): https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/mock/mock_co... webrtc/call/mock/mock_congestion_controller.h:25: : CongestionController(process_thread, call_stats, bitrate_observer) {} On 2016/01/12 10:24:13, the sun wrote: > Sorry for not thinking of this before, but you *could* add a default ctor to CC > and avoid some of the messy setting up. Yeah, I tried that too, but adding a default ctor to CC involves a lot of code changes since the dtor assumes that the pointers are not null. I would prefer to not have to introduce a lot of if statements in CC just to be able to simplify the mock constructor.
On 2016/01/12 11:15:16, stefan-webrtc (holmer) wrote: > https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... > File webrtc/call/congestion_controller.cc (right): > > https://codereview.webrtc.org/1535963002/diff/180001/webrtc/call/congestion_c... > webrtc/call/congestion_controller.cc:150: : remb_(new > VieRemb(Clock::GetRealTimeClock())), > On 2016/01/12 10:24:13, the sun wrote: > > On 2016/01/11 17:15:48, stefan-webrtc (holmer) wrote: > > > On 2016/01/11 11:47:19, the sun wrote: > > > > separate CL? > > > > We now have the MockCC depending on real time clock. > > > > > > Maybe, on the other hand it will never be affected by it since it's not > > possible > > > to access remb_ in any way. If you are using the MockCC you will not be able > > to > > > call any methods of the base CongestionController? > > > > > > Not sure it makes sense to expose the clock in CongestionController if we > are > > > not using it in a test? > > > > However confusingly phrased, my concern was really more about testability of > the > > CC class itself... :) > > I see, then let's leave that to when we actually make changes to the CC class > > https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/mock/mock_co... > File webrtc/call/mock/mock_congestion_controller.h (right): > > https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/mock/mock_co... > webrtc/call/mock/mock_congestion_controller.h:25: : > CongestionController(process_thread, call_stats, bitrate_observer) {} > On 2016/01/12 10:24:13, the sun wrote: > > Sorry for not thinking of this before, but you *could* add a default ctor to > CC > > and avoid some of the messy setting up. > > Yeah, I tried that too, but adding a default ctor to CC involves a lot of code > changes since the dtor assumes that the pointers are not null. I would prefer to > not have to introduce a lot of if statements in CC just to be able to simplify > the mock constructor. Ok, well the way to solve that is of course to make CC an interface and separate the mock an real implementations...
lgtm
https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/mock/mock_co... File webrtc/call/mock/mock_congestion_controller.h (right): https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/mock/mock_co... webrtc/call/mock/mock_congestion_controller.h:25: : CongestionController(process_thread, call_stats, bitrate_observer) {} On 2016/01/12 11:15:15, stefan-webrtc (holmer) wrote: > On 2016/01/12 10:24:13, the sun wrote: > > Sorry for not thinking of this before, but you *could* add a default ctor to > CC > > and avoid some of the messy setting up. > > Yeah, I tried that too, but adding a default ctor to CC involves a lot of code > changes since the dtor assumes that the pointers are not null. I would prefer to > not have to introduce a lot of if statements in CC just to be able to simplify > the mock constructor. That is true. I'll consider doing that in the future.
The CQ bit was checked by stefan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1535963002/#ps200001 (title: "Comments addressed.")
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
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2797)
stefan@webrtc.org changed reviewers: + mflodman@webrtc.org
Magnus, can you take a look, specifically at webrtc/?
One comment, then LGTM. https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/congestion_c... File webrtc/call/congestion_controller.cc (right): https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/congestion_c... webrtc/call/congestion_controller.cc:172: RTC_DCHECK(bitrate_observer); I don't this check makes sense here, since it's not used in this class. Either remove or move to BitrateController.
https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/congestion_c... File webrtc/call/congestion_controller.cc (right): https://codereview.webrtc.org/1535963002/diff/200001/webrtc/call/congestion_c... webrtc/call/congestion_controller.cc:172: RTC_DCHECK(bitrate_observer); On 2016/01/12 12:42:53, mflodman wrote: > I don't this check makes sense here, since it's not used in this class. Either > remove or move to BitrateController. I'll remove it for now.
Comment addressed.
Message was sent while issue was closed.
Description was changed from ========== Wire-up BWE feedback for audio receive streams. Also wires up receiving transport sequence numbers. BUG=webrtc:5263 ========== to ========== 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://chromium.googlesource.com/external/webrtc/+/3842c5c7f73639527e653f41c... ==========
Message was sent while issue was closed.
Description was changed from ========== Wire-up BWE feedback for audio receive streams. Also wires up receiving transport sequence numbers. BUG=webrtc:5263 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/3842c5c7f73639527e653f41c65334245d2317a1 Cr-Commit-Position: refs/heads/master@{#11220}
Message was sent while issue was closed.
Committed patchset #12 (id:220001) manually as 3842c5c7f73639527e653f41c65334245d2317a1 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
