diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index 773075e5c9..94237e0acf 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -426,13 +426,8 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { // Set RED and ULPFEC payload types. A payload type of -1 means that the // corresponding feature is turned off. Note that we DO NOT support enabling - // ULPFEC without enabling RED. However, we DO support enabling RED without - // enabling ULPFEC. This is due to an RED/RTX workaround, where the receiver - // assumes that RTX packets carry RED if RED has been configured in the SDP, - // regardless of what RTX payload type mapping was negotiated in the SDP. - // TODO(brandtr): Update this comment when we have removed the RED/RTX - // send-side workaround, i.e., when we do not support enabling RED without - // enabling ULPFEC. + // ULPFEC without enabling RED, and RED is only ever used when ULPFEC is + // enabled. virtual void SetUlpfecConfig(int red_payload_type, int ulpfec_payload_type) = 0; diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index a38888d6b1..a7f3df097a 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -218,10 +218,7 @@ void RTPSenderVideo::SetUlpfecConfig(int red_payload_type, ulpfec_payload_type_ = ulpfec_payload_type; // Must not enable ULPFEC without RED. - // TODO(brandtr): We currently support enabling RED without ULPFEC. Change - // this when we have removed the RED/RTX send-side workaround, so that we - // ensure that RED and ULPFEC are only enabled together. - RTC_DCHECK(red_enabled() || !ulpfec_enabled()); + RTC_DCHECK(!(red_enabled() ^ ulpfec_enabled())); // Reset FEC parameters. delta_fec_params_ = FecProtectionParams{0, 1, kFecMaskRandom}; diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index 462f243719..3ca1da3bd9 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc @@ -724,29 +724,24 @@ void VideoSendStreamImpl::ConfigureProtection() { // Shorthands. auto IsRedEnabled = [&]() { return red_payload_type >= 0; }; - auto DisableRed = [&]() { red_payload_type = -1; }; auto IsUlpfecEnabled = [&]() { return ulpfec_payload_type >= 0; }; - auto DisableUlpfec = [&]() { ulpfec_payload_type = -1; }; + auto DisableRedAndUlpfec = [&]() { + red_payload_type = -1; + ulpfec_payload_type = -1; + }; if (webrtc::field_trial::IsEnabled("WebRTC-DisableUlpFecExperiment")) { RTC_LOG(LS_INFO) << "Experiment to disable sending ULPFEC is enabled."; - DisableUlpfec(); + DisableRedAndUlpfec(); } // If enabled, FlexFEC takes priority over RED+ULPFEC. if (flexfec_enabled) { - // We can safely disable RED here, because if the remote supports FlexFEC, - // we know that it has a receiver without the RED/RTX workaround. - // See http://crbug.com/webrtc/6650 for more information. - if (IsRedEnabled()) { - RTC_LOG(LS_INFO) << "Both FlexFEC and RED are configured. Disabling RED."; - DisableRed(); - } if (IsUlpfecEnabled()) { RTC_LOG(LS_INFO) << "Both FlexFEC and ULPFEC are configured. Disabling ULPFEC."; - DisableUlpfec(); } + DisableRedAndUlpfec(); } // Payload types without picture ID cannot determine that a stream is complete @@ -759,29 +754,14 @@ void VideoSendStreamImpl::ConfigureProtection() { << "Transmitting payload type without picture ID using " "NACK+ULPFEC is a waste of bandwidth since ULPFEC packets " "also have to be retransmitted. Disabling ULPFEC."; - DisableUlpfec(); + DisableRedAndUlpfec(); } // Verify payload types. - // - // Due to how old receivers work, we need to always send RED if it has been - // negotiated. This is a remnant of an old RED/RTX workaround, see - // https://codereview.webrtc.org/2469093003. - // TODO(brandtr): This change went into M56, so we can remove it in ~M59. - // At that time, we can disable RED whenever ULPFEC is disabled, as there is - // no point in using RED without ULPFEC. - if (IsRedEnabled()) { - RTC_DCHECK_GE(red_payload_type, 0); - RTC_DCHECK_LE(red_payload_type, 127); - } - if (IsUlpfecEnabled()) { - RTC_DCHECK_GE(ulpfec_payload_type, 0); - RTC_DCHECK_LE(ulpfec_payload_type, 127); - if (!IsRedEnabled()) { - RTC_LOG(LS_WARNING) - << "ULPFEC is enabled but RED is disabled. Disabling ULPFEC."; - DisableUlpfec(); - } + if (IsUlpfecEnabled() ^ IsRedEnabled()) { + RTC_LOG(LS_WARNING) + << "Only RED or only ULPFEC enabled, but not both. Disabling both."; + DisableRedAndUlpfec(); } for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc index 107a3bf547..d088f42fdc 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc @@ -602,7 +602,7 @@ class VideoSendStreamWithoutUlpfecTest : public VideoSendStreamTest { TEST_F(VideoSendStreamWithoutUlpfecTest, NoUlpfecIfDisabledThroughFieldTrial) { test::FunctionVideoEncoderFactory encoder_factory( []() { return VP8Encoder::Create(); }); - UlpfecObserver test(false, false, true, false, "VP8", &encoder_factory); + UlpfecObserver test(false, false, false, false, "VP8", &encoder_factory); RunBaseTest(&test); } @@ -614,7 +614,7 @@ TEST_F(VideoSendStreamTest, DoesNotUtilizeUlpfecForH264WithNackEnabled) { test::FunctionVideoEncoderFactory encoder_factory([]() { return absl::make_unique(Clock::GetRealTimeClock()); }); - UlpfecObserver test(false, true, true, false, "H264", &encoder_factory); + UlpfecObserver test(false, true, false, false, "H264", &encoder_factory); RunBaseTest(&test); }