Skip to content

Commit

Permalink
Always disable RED when ULPFEC is disabled.
Browse files Browse the repository at this point in the history
This prevents a lot of unnecessary processing taking place when we are
not using FEC.

This CL also removes the FieldTrial that was used to disable ulpfec, as it's no longer used.

Bug: webrtc:9514
Change-Id: I8285b933f71eea971f5932cd19833455a42c8639
Reviewed-on: https://webrtc-review.googlesource.com/87848
Reviewed-by: Stefan Holmer <[email protected]>
Reviewed-by: Rasmus Brandt <[email protected]>
Commit-Queue: Kári Helgason <[email protected]>
Cr-Commit-Position: refs/heads/master@{#23952}
  • Loading branch information
kthelgason authored and Commit Bot committed Jul 12, 2018
1 parent 4238628 commit 798ee75
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 44 deletions.
9 changes: 2 additions & 7 deletions modules/rtp_rtcp/include/rtp_rtcp.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
5 changes: 1 addition & 4 deletions modules/rtp_rtcp/source/rtp_sender_video.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
42 changes: 11 additions & 31 deletions video/video_send_stream_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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_) {
Expand Down
4 changes: 2 additions & 2 deletions video/video_send_stream_tests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -614,7 +614,7 @@ TEST_F(VideoSendStreamTest, DoesNotUtilizeUlpfecForH264WithNackEnabled) {
test::FunctionVideoEncoderFactory encoder_factory([]() {
return absl::make_unique<test::FakeH264Encoder>(Clock::GetRealTimeClock());
});
UlpfecObserver test(false, true, true, false, "H264", &encoder_factory);
UlpfecObserver test(false, true, false, false, "H264", &encoder_factory);
RunBaseTest(&test);
}

Expand Down

0 comments on commit 798ee75

Please sign in to comment.