From 7bf7a427bfcc9f398d90632aad54dc32372d8578 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Fri, 13 Sep 2019 08:31:45 +0200 Subject: [PATCH] Delete flag VideoReceiveStream::Config::Rtp::remb This flag became unused in https://codereview.webrtc.org/2789843002; it was set, but the setting had no effect. Bug: webrtc:7135 Change-Id: I012a7c3600bc7a371c7a589695823b30ed5647a5 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/152661 Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Danil Chapovalov Reviewed-by: Rasmus Brandt Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#29192} --- call/bitrate_estimator_tests.cc | 1 - call/call.cc | 1 - call/rampup_tests.cc | 1 - call/video_receive_stream.cc | 1 - call/video_receive_stream.h | 12 ---------- media/engine/webrtc_video_engine.cc | 10 ++------ media/engine/webrtc_video_engine.h | 1 - media/engine/webrtc_video_engine_unittest.cc | 25 -------------------- test/call_config_utils.cc | 2 -- test/call_config_utils_unittest.cc | 2 -- test/call_test.cc | 1 - test/fuzzers/vp8_replay_fuzzer.cc | 1 - test/fuzzers/vp9_replay_fuzzer.cc | 1 - test/scenario/video_stream.cc | 1 - video/end_to_end_tests/bandwidth_tests.cc | 2 -- video/end_to_end_tests/config_tests.cc | 2 -- 16 files changed, 2 insertions(+), 62 deletions(-) diff --git a/call/bitrate_estimator_tests.cc b/call/bitrate_estimator_tests.cc index 324685adbb..803e1f98be 100644 --- a/call/bitrate_estimator_tests.cc +++ b/call/bitrate_estimator_tests.cc @@ -138,7 +138,6 @@ class BitrateEstimatorTest : public test::CallTest { // receive_config_.decoders will be set by every stream separately. receive_config_.rtp.remote_ssrc = GetVideoSendConfig()->rtp.ssrcs[0]; receive_config_.rtp.local_ssrc = kReceiverLocalVideoSsrc; - receive_config_.rtp.remb = true; receive_config_.rtp.extensions.push_back( RtpExtension(RtpExtension::kTimestampOffsetUri, kTOFExtensionId)); receive_config_.rtp.extensions.push_back( diff --git a/call/call.cc b/call/call.cc index e5eef1970b..3964171560 100644 --- a/call/call.cc +++ b/call/call.cc @@ -117,7 +117,6 @@ std::unique_ptr CreateRtcLogStreamConfig( rtclog_config->local_ssrc = config.rtp.local_ssrc; rtclog_config->rtx_ssrc = config.rtp.rtx_ssrc; rtclog_config->rtcp_mode = config.rtp.rtcp_mode; - rtclog_config->remb = config.rtp.remb; rtclog_config->rtp_extensions = config.rtp.extensions; for (const auto& d : config.decoders) { diff --git a/call/rampup_tests.cc b/call/rampup_tests.cc index b5cf651f3c..b3f206325a 100644 --- a/call/rampup_tests.cc +++ b/call/rampup_tests.cc @@ -228,7 +228,6 @@ void RampUpTester::ModifyVideoConfigs( size_t i = 0; for (VideoReceiveStream::Config& recv_config : *receive_configs) { - recv_config.rtp.remb = remb; recv_config.rtp.transport_cc = transport_cc; recv_config.rtp.extensions = send_config->rtp.extensions; recv_config.decoders.reserve(1); diff --git a/call/video_receive_stream.cc b/call/video_receive_stream.cc index 261e5def5d..acda498b1d 100644 --- a/call/video_receive_stream.cc +++ b/call/video_receive_stream.cc @@ -117,7 +117,6 @@ std::string VideoReceiveStream::Config::Rtp::ToString() const { ss << "{receiver_reference_time_report: " << (rtcp_xr.receiver_reference_time_report ? "on" : "off"); ss << '}'; - ss << ", remb: " << (remb ? "on" : "off"); ss << ", transport_cc: " << (transport_cc ? "on" : "off"); ss << ", lntf: {enabled: " << (lntf.enabled ? "true" : "false") << '}'; ss << ", nack: {rtp_history_ms: " << nack.rtp_history_ms << '}'; diff --git a/call/video_receive_stream.h b/call/video_receive_stream.h index b1d45ace1f..6e087383ba 100644 --- a/call/video_receive_stream.h +++ b/call/video_receive_stream.h @@ -173,18 +173,6 @@ class VideoReceiveStream { bool receiver_reference_time_report = false; } rtcp_xr; - // TODO(nisse): This remb setting is currently set but never - // applied. REMB logic is now the responsibility of - // PacketRouter, and it will generate REMB feedback if - // OnReceiveBitrateChanged is used, which depends on how the - // estimators belonging to the ReceiveSideCongestionController - // are configured. Decide if this setting should be deleted, and - // if it needs to be replaced by a setting in PacketRouter to - // disable REMB feedback. - - // See draft-alvestrand-rmcat-remb for information. - bool remb = false; - // See draft-holmer-rmcat-transport-wide-cc-extensions for details. bool transport_cc = false; diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index f31d69f5a0..2a1f65dbc3 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -842,7 +842,7 @@ bool WebRtcVideoChannel::ApplyChangedParams( RTC_DCHECK(kv.second != nullptr); kv.second->SetFeedbackParameters( HasLntf(send_codec_->codec), HasNack(send_codec_->codec), - HasRemb(send_codec_->codec), HasTransportCc(send_codec_->codec), + HasTransportCc(send_codec_->codec), send_params_.rtcp.reduced_size ? webrtc::RtcpMode::kReducedSize : webrtc::RtcpMode::kCompound); } @@ -1340,7 +1340,6 @@ void WebRtcVideoChannel::ConfigureReceiverRtp( ? webrtc::RtcpMode::kReducedSize : webrtc::RtcpMode::kCompound; - config->rtp.remb = send_codec_ ? HasRemb(send_codec_->codec) : false; config->rtp.transport_cc = send_codec_ ? HasTransportCc(send_codec_->codec) : false; @@ -2613,24 +2612,20 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetLocalSsrc( void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetFeedbackParameters( bool lntf_enabled, bool nack_enabled, - bool remb_enabled, bool transport_cc_enabled, webrtc::RtcpMode rtcp_mode) { int nack_history_ms = nack_enabled ? kNackHistoryMs : 0; if (config_.rtp.lntf.enabled == lntf_enabled && config_.rtp.nack.rtp_history_ms == nack_history_ms && - config_.rtp.remb == remb_enabled && config_.rtp.transport_cc == transport_cc_enabled && config_.rtp.rtcp_mode == rtcp_mode) { RTC_LOG(LS_INFO) << "Ignoring call to SetFeedbackParameters because parameters are " "unchanged; lntf=" << lntf_enabled << ", nack=" << nack_enabled - << ", remb=" << remb_enabled << ", transport_cc=" << transport_cc_enabled; return; } - config_.rtp.remb = remb_enabled; config_.rtp.lntf.enabled = lntf_enabled; config_.rtp.nack.rtp_history_ms = nack_history_ms; config_.rtp.transport_cc = transport_cc_enabled; @@ -2641,8 +2636,7 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetFeedbackParameters( flexfec_config_.rtcp_mode = config_.rtp.rtcp_mode; RTC_LOG(LS_INFO) << "RecreateWebRtcStream (recv) because of SetFeedbackParameters; nack=" - << nack_enabled << ", remb=" << remb_enabled - << ", transport_cc=" << transport_cc_enabled; + << nack_enabled << ", transport_cc=" << transport_cc_enabled; MaybeRecreateWebRtcFlexfecStream(); RecreateWebRtcVideoStream(); } diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index f8d92d4d21..1bd8edd56b 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -427,7 +427,6 @@ class WebRtcVideoChannel : public VideoMediaChannel, // TODO(deadbeef): Move these feedback parameters into the recv parameters. void SetFeedbackParameters(bool lntf_enabled, bool nack_enabled, - bool remb_enabled, bool transport_cc_enabled, webrtc::RtcpMode rtcp_mode); void SetRecvParameters(const ChangedRecvParameters& recv_params); diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 50dd8d8fb7..e7949fec43 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -2765,36 +2765,11 @@ TEST_F(WebRtcVideoChannelTest, RtcpIsCompoundByDefault) { EXPECT_EQ(webrtc::RtcpMode::kCompound, stream->GetConfig().rtp.rtcp_mode); } -TEST_F(WebRtcVideoChannelTest, RembIsEnabledByDefault) { - FakeVideoReceiveStream* stream = AddRecvStream(); - EXPECT_TRUE(stream->GetConfig().rtp.remb); -} - TEST_F(WebRtcVideoChannelTest, TransportCcIsEnabledByDefault) { FakeVideoReceiveStream* stream = AddRecvStream(); EXPECT_TRUE(stream->GetConfig().rtp.transport_cc); } -TEST_F(WebRtcVideoChannelTest, RembCanBeEnabledAndDisabled) { - FakeVideoReceiveStream* stream = AddRecvStream(); - EXPECT_TRUE(stream->GetConfig().rtp.remb); - - // Verify that REMB is turned off when send(!) codecs without REMB are set. - cricket::VideoSendParameters parameters; - parameters.codecs.push_back(RemoveFeedbackParams(GetEngineCodec("VP8"))); - EXPECT_TRUE(parameters.codecs[0].feedback_params.params().empty()); - EXPECT_TRUE(channel_->SetSendParameters(parameters)); - stream = fake_call_->GetVideoReceiveStreams()[0]; - EXPECT_FALSE(stream->GetConfig().rtp.remb); - - // Verify that REMB is turned on when setting default codecs since the - // default codecs have REMB enabled. - parameters.codecs = engine_.codecs(); - EXPECT_TRUE(channel_->SetSendParameters(parameters)); - stream = fake_call_->GetVideoReceiveStreams()[0]; - EXPECT_TRUE(stream->GetConfig().rtp.remb); -} - TEST_F(WebRtcVideoChannelTest, TransportCcCanBeEnabledAndDisabled) { FakeVideoReceiveStream* stream = AddRecvStream(); EXPECT_TRUE(stream->GetConfig().rtp.transport_cc); diff --git a/test/call_config_utils.cc b/test/call_config_utils.cc index 155fad1c24..ab41a274fb 100644 --- a/test/call_config_utils.cc +++ b/test/call_config_utils.cc @@ -43,7 +43,6 @@ VideoReceiveStream::Config ParseVideoReceiveStreamJsonConfig( json["rtp"]["rtcp_mode"].asString() == "RtcpMode::kCompound" ? RtcpMode::kCompound : RtcpMode::kReducedSize; - receive_config.rtp.remb = json["rtp"]["remb"].asBool(); receive_config.rtp.transport_cc = json["rtp"]["transport_cc"].asBool(); receive_config.rtp.lntf.enabled = json["rtp"]["lntf"]["enabled"].asInt64(); receive_config.rtp.nack.rtp_history_ms = @@ -93,7 +92,6 @@ Json::Value GenerateVideoReceiveStreamJsonConfig( rtp_json["rtcp_mode"] = config.rtp.rtcp_mode == RtcpMode::kCompound ? "RtcpMode::kCompound" : "RtcpMode::kReducedSize"; - rtp_json["remb"] = config.rtp.remb; rtp_json["transport_cc"] = config.rtp.transport_cc; rtp_json["lntf"]["enabled"] = config.rtp.lntf.enabled; rtp_json["nack"]["rtp_history_ms"] = config.rtp.nack.rtp_history_ms; diff --git a/test/call_config_utils_unittest.cc b/test/call_config_utils_unittest.cc index bb834394f6..c6d219c9ca 100644 --- a/test/call_config_utils_unittest.cc +++ b/test/call_config_utils_unittest.cc @@ -29,7 +29,6 @@ TEST(CallConfigUtils, MarshalUnmarshalProcessSameObject) { recv_config.rtp.remote_ssrc = 100; recv_config.rtp.local_ssrc = 101; recv_config.rtp.rtcp_mode = RtcpMode::kCompound; - recv_config.rtp.remb = false; recv_config.rtp.transport_cc = false; recv_config.rtp.lntf.enabled = false; recv_config.rtp.nack.rtp_history_ms = 150; @@ -53,7 +52,6 @@ TEST(CallConfigUtils, MarshalUnmarshalProcessSameObject) { EXPECT_EQ(recv_config.rtp.remote_ssrc, unmarshaled_config.rtp.remote_ssrc); EXPECT_EQ(recv_config.rtp.local_ssrc, unmarshaled_config.rtp.local_ssrc); EXPECT_EQ(recv_config.rtp.rtcp_mode, unmarshaled_config.rtp.rtcp_mode); - EXPECT_EQ(recv_config.rtp.remb, unmarshaled_config.rtp.remb); EXPECT_EQ(recv_config.rtp.transport_cc, unmarshaled_config.rtp.transport_cc); EXPECT_EQ(recv_config.rtp.lntf.enabled, unmarshaled_config.rtp.lntf.enabled); EXPECT_EQ(recv_config.rtp.nack.rtp_history_ms, diff --git a/test/call_test.cc b/test/call_test.cc index 8eba13b3eb..20c8892de6 100644 --- a/test/call_test.cc +++ b/test/call_test.cc @@ -379,7 +379,6 @@ void CallTest::AddMatchingVideoReceiveConfigs( int rtp_history_ms) { RTC_DCHECK(!video_send_config.rtp.ssrcs.empty()); VideoReceiveStream::Config default_config(rtcp_send_transport); - default_config.rtp.remb = !send_side_bwe; default_config.rtp.transport_cc = send_side_bwe; default_config.rtp.local_ssrc = kReceiverLocalVideoSsrc; for (const RtpExtension& extension : video_send_config.rtp.extensions) diff --git a/test/fuzzers/vp8_replay_fuzzer.cc b/test/fuzzers/vp8_replay_fuzzer.cc index 7f8299b5f7..2ba5cfeeb3 100644 --- a/test/fuzzers/vp8_replay_fuzzer.cc +++ b/test/fuzzers/vp8_replay_fuzzer.cc @@ -29,7 +29,6 @@ void FuzzOneInput(const uint8_t* data, size_t size) { vp8_config.rtp.remote_ssrc = 1337; vp8_config.rtp.rtx_ssrc = 100; vp8_config.rtp.transport_cc = true; - vp8_config.rtp.remb = true; vp8_config.rtp.nack.rtp_history_ms = 1000; vp8_config.rtp.lntf.enabled = true; diff --git a/test/fuzzers/vp9_replay_fuzzer.cc b/test/fuzzers/vp9_replay_fuzzer.cc index ff7c7237e6..8e046f11ce 100644 --- a/test/fuzzers/vp9_replay_fuzzer.cc +++ b/test/fuzzers/vp9_replay_fuzzer.cc @@ -29,7 +29,6 @@ void FuzzOneInput(const uint8_t* data, size_t size) { vp9_config.rtp.remote_ssrc = 1337; vp9_config.rtp.rtx_ssrc = 100; vp9_config.rtp.transport_cc = true; - vp9_config.rtp.remb = true; vp9_config.rtp.nack.rtp_history_ms = 1000; std::vector replay_configs; diff --git a/test/scenario/video_stream.cc b/test/scenario/video_stream.cc index ee389f1425..a466162d27 100644 --- a/test/scenario/video_stream.cc +++ b/test/scenario/video_stream.cc @@ -318,7 +318,6 @@ VideoReceiveStream::Config CreateVideoReceiveStreamConfig( uint32_t ssrc, uint32_t rtx_ssrc) { VideoReceiveStream::Config recv(feedback_transport); - recv.rtp.remb = !config.stream.packet_feedback; recv.rtp.transport_cc = config.stream.packet_feedback; recv.rtp.local_ssrc = local_ssrc; recv.rtp.extensions = GetVideoRtpExtensions(config); diff --git a/video/end_to_end_tests/bandwidth_tests.cc b/video/end_to_end_tests/bandwidth_tests.cc index f1b35c0490..ecdc0e9074 100644 --- a/video/end_to_end_tests/bandwidth_tests.cc +++ b/video/end_to_end_tests/bandwidth_tests.cc @@ -49,7 +49,6 @@ TEST_F(BandwidthEndToEndTest, ReceiveStreamSendsRemb) { send_config->rtp.extensions.clear(); send_config->rtp.extensions.push_back( RtpExtension(RtpExtension::kAbsSendTimeUri, kAbsSendTimeExtensionId)); - (*receive_configs)[0].rtp.remb = true; (*receive_configs)[0].rtp.transport_cc = false; } @@ -97,7 +96,6 @@ class BandwidthStatsTest : public test::EndToEndTest { send_config->rtp.extensions.clear(); send_config->rtp.extensions.push_back( RtpExtension(RtpExtension::kAbsSendTimeUri, kAbsSendTimeExtensionId)); - (*receive_configs)[0].rtp.remb = true; (*receive_configs)[0].rtp.transport_cc = false; } } diff --git a/video/end_to_end_tests/config_tests.cc b/video/end_to_end_tests/config_tests.cc index 7d9221a5f2..bf63e2a51f 100644 --- a/video/end_to_end_tests/config_tests.cc +++ b/video/end_to_end_tests/config_tests.cc @@ -78,8 +78,6 @@ TEST_F(ConfigEndToEndTest, VerifyDefaultVideoReceiveConfigParameters) { << "Reduced-size RTCP require rtcp-rsize to be negotiated."; EXPECT_FALSE(default_receive_config.rtp.lntf.enabled) << "Enabling LNTF require rtcp-fb: goog-lntf negotiation."; - EXPECT_FALSE(default_receive_config.rtp.remb) - << "REMB require rtcp-fb: goog-remb to be negotiated."; EXPECT_FALSE( default_receive_config.rtp.rtcp_xr.receiver_reference_time_report) << "RTCP XR settings require rtcp-xr to be negotiated.";