Simplify {,Set}UlpfecConfig interface.
Prior to this change, we signalled that ULPFEC was disabled
through a bool, but that RED was disabled by setting its
payload type to -1. The latter is consistent with how we
disable RED/ULPFEC in the config, so this CL removes the
ULPFEC bool from the {,Set}UlpfecConfig chain of member
functions.
BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/2460533002
Cr-Original-Commit-Position: refs/heads/master@{#14944}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: f1bb47605098f3e7c6583ff935f8570b3bb55a28
diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h
index be3d0f4..68429c6 100644
--- a/modules/rtp_rtcp/include/rtp_rtcp.h
+++ b/modules/rtp_rtcp/include/rtp_rtcp.h
@@ -434,9 +434,16 @@
// Video
// **************************************************************************
- // Turn on/off ULPFEC.
- virtual void SetUlpfecConfig(bool enabled,
- int red_payload_type,
+ // 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.
+ virtual void SetUlpfecConfig(int red_payload_type,
int ulpfec_payload_type) = 0;
virtual int32_t SetFecParameters(const FecProtectionParams* delta_params,
diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
index 4f396e7..ff8d90f 100644
--- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
+++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h
@@ -186,10 +186,8 @@
int32_t(bool enable, uint8_t id));
MOCK_METHOD1(SetAudioLevel, int32_t(uint8_t level_dbov));
MOCK_METHOD1(SetTargetSendBitrate, void(uint32_t bitrate_bps));
- MOCK_METHOD3(SetUlpfecConfig,
- void(bool ulpfec_enabled,
- int red_payload_type,
- int fec_payload_type));
+ MOCK_METHOD2(SetUlpfecConfig,
+ void(int red_payload_type, int fec_payload_type));
MOCK_METHOD2(SetFecParameters,
int32_t(const FecProtectionParams* delta_params,
const FecProtectionParams* key_params));
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
index 9f4e165..2a2bf0d 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -789,10 +789,9 @@
GetFeedbackState(), kRtcpSli, 0, 0, false, picture_id);
}
-void ModuleRtpRtcpImpl::SetUlpfecConfig(bool enabled,
- int red_payload_type,
+void ModuleRtpRtcpImpl::SetUlpfecConfig(int red_payload_type,
int ulpfec_payload_type) {
- rtp_sender_.SetUlpfecConfig(enabled, red_payload_type, ulpfec_payload_type);
+ rtp_sender_.SetUlpfecConfig(red_payload_type, ulpfec_payload_type);
}
int32_t ModuleRtpRtcpImpl::SetFecParameters(
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h
index c89de9b..f055bbe 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h
@@ -277,9 +277,7 @@
// Send a request for a keyframe.
int32_t RequestKeyFrame() override;
- void SetUlpfecConfig(bool enabled,
- int red_payload_type,
- int ulpfec_payload_type) override;
+ void SetUlpfecConfig(int red_payload_type, int ulpfec_payload_type) override;
int32_t SetFecParameters(const FecProtectionParams* delta_params,
const FecProtectionParams* key_params) override;
diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc
index f0e323a..a3593c6 100644
--- a/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/modules/rtp_rtcp/source/rtp_sender.cc
@@ -789,10 +789,10 @@
if (!video_) {
return false;
}
- bool fec_enabled;
int pt_red;
int pt_fec;
- video_->GetUlpfecConfig(&fec_enabled, &pt_red, &pt_fec);
+ video_->GetUlpfecConfig(&pt_red, &pt_fec);
+ const bool fec_enabled = (pt_fec != -1);
return fec_enabled && static_cast<int>(packet.PayloadType()) == pt_red &&
static_cast<int>(packet.payload()[0]) == pt_fec;
}
@@ -1131,11 +1131,9 @@
return video_->VideoCodecType();
}
-void RTPSender::SetUlpfecConfig(bool enabled,
- int red_payload_type,
- int ulpfec_payload_type) {
+void RTPSender::SetUlpfecConfig(int red_payload_type, int ulpfec_payload_type) {
RTC_DCHECK(!audio_configured_);
- video_->SetUlpfecConfig(enabled, red_payload_type, ulpfec_payload_type);
+ video_->SetUlpfecConfig(red_payload_type, ulpfec_payload_type);
}
int32_t RTPSender::SetFecParameters(
diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h
index 75132b2..dafddc5 100644
--- a/modules/rtp_rtcp/source/rtp_sender.h
+++ b/modules/rtp_rtcp/source/rtp_sender.h
@@ -183,10 +183,8 @@
uint32_t MaxConfiguredBitrateVideo() const;
- // FEC.
- void SetUlpfecConfig(bool enabled,
- int red_payload_type,
- int ulpfec_payload_type);
+ // ULPFEC.
+ void SetUlpfecConfig(int red_payload_type, int ulpfec_payload_type);
int32_t SetFecParameters(const FecProtectionParams *delta_params,
const FecProtectionParams *key_params);
diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
index 16bc271..fa8db8a 100644
--- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
@@ -1123,8 +1123,8 @@
expected.transmitted.packets = 3;
callback.Matches(ssrc, expected);
- // Send FEC.
- rtp_sender_->SetUlpfecConfig(true, kRedPayloadType, kUlpfecPayloadType);
+ // Send ULPFEC.
+ rtp_sender_->SetUlpfecConfig(kRedPayloadType, kUlpfecPayloadType);
FecProtectionParams fec_params;
fec_params.fec_mask_type = kFecMaskRandom;
fec_params.fec_rate = 1;
diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc
index dd19bb2..2450a43 100644
--- a/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -50,9 +50,8 @@
video_type_(kRtpVideoGeneric),
retransmission_settings_(kRetransmitBaseLayer),
last_rotation_(kVideoRotation_0),
- fec_enabled_(false),
red_payload_type_(-1),
- fec_payload_type_(-1),
+ ulpfec_payload_type_(-1),
delta_fec_params_{0, 1, kFecMaskRandom},
key_fec_params_{0, 1, kFecMaskRandom},
fec_bitrate_(1000, RateStatistics::kBpsScale),
@@ -139,7 +138,7 @@
uint16_t first_fec_sequence_number =
rtp_sender_->AllocateSequenceNumber(num_fec_packets);
fec_packets = ulpfec_generator_.GetUlpfecPacketsAsRed(
- red_payload_type_, fec_payload_type_, first_fec_sequence_number,
+ red_payload_type_, ulpfec_payload_type_, first_fec_sequence_number,
media_packet->headers_size());
RTC_DCHECK_EQ(num_fec_packets, fec_packets.size());
if (retransmission_settings_ & kRetransmitFECPackets)
@@ -179,36 +178,42 @@
}
}
-void RTPSenderVideo::SetUlpfecConfig(bool enabled,
- int red_payload_type,
+void RTPSenderVideo::SetUlpfecConfig(int red_payload_type,
int ulpfec_payload_type) {
- RTC_DCHECK(!enabled || red_payload_type > 0);
+ // Sanity check. Per the definition of UlpfecConfig (see config.h),
+ // a payload type of -1 means that the corresponding feature is
+ // turned off.
+ RTC_DCHECK_GE(red_payload_type, -1);
RTC_DCHECK_LE(red_payload_type, 127);
+ RTC_DCHECK_GE(ulpfec_payload_type, -1);
RTC_DCHECK_LE(ulpfec_payload_type, 127);
rtc::CritScope cs(&crit_);
- fec_enabled_ = enabled;
red_payload_type_ = red_payload_type;
- fec_payload_type_ = ulpfec_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());
// Reset FEC rates.
delta_fec_params_ = FecProtectionParams{0, 1, kFecMaskRandom};
key_fec_params_ = FecProtectionParams{0, 1, kFecMaskRandom};
}
-void RTPSenderVideo::GetUlpfecConfig(bool* enabled,
- int* red_payload_type,
+void RTPSenderVideo::GetUlpfecConfig(int* red_payload_type,
int* ulpfec_payload_type) const {
rtc::CritScope cs(&crit_);
- *enabled = fec_enabled_;
*red_payload_type = red_payload_type_;
- *ulpfec_payload_type = fec_payload_type_;
+ *ulpfec_payload_type = ulpfec_payload_type_;
}
size_t RTPSenderVideo::FecPacketOverhead() const {
rtc::CritScope cs(&crit_);
size_t overhead = 0;
- if (red_payload_type_ != -1) {
+ if (red_enabled()) {
// Overhead is FEC headers plus RED for FEC header plus anything in RTP
// header beyond the 12 bytes base header (CSRC list, extensions...)
// This reason for the header extensions to be included here is that
@@ -217,7 +222,7 @@
return ulpfec_generator_.MaxPacketOverhead() + kRedForFecHeaderLength +
(rtp_sender_->RtpHeaderLength() - kRtpHeaderSize);
}
- if (fec_enabled_)
+ if (ulpfec_enabled())
overhead += ulpfec_generator_.MaxPacketOverhead();
return overhead;
}
@@ -227,7 +232,7 @@
rtc::CritScope cs(&crit_);
RTC_DCHECK(delta_params);
RTC_DCHECK(key_params);
- if (fec_enabled_) {
+ if (ulpfec_enabled()) {
delta_fec_params_ = *delta_params;
key_fec_params_ = *key_params;
}
@@ -283,7 +288,7 @@
video_header ? &(video_header->codecHeader) : nullptr, frame_type));
StorageType storage;
- int red_payload_type;
+ bool red_enabled;
bool first_frame = first_frame_sent_();
{
rtc::CritScope cs(&crit_);
@@ -291,7 +296,7 @@
frame_type == kVideoFrameKey ? &key_fec_params_ : &delta_fec_params_;
ulpfec_generator_.SetFecParameters(fec_params);
storage = packetizer->GetStorageType(retransmission_settings_);
- red_payload_type = red_payload_type_;
+ red_enabled = this->red_enabled();
}
// TODO(changbin): we currently don't support to configure the codec to
@@ -318,7 +323,7 @@
if (!rtp_sender_->AssignSequenceNumber(packet.get()))
return false;
- if (red_payload_type != -1) {
+ if (red_enabled) {
SendVideoPacketAsRed(std::move(packet), storage,
packetizer->GetProtectionType() == kProtectedPacket);
} else {
diff --git a/modules/rtp_rtcp/source/rtp_sender_video.h b/modules/rtp_rtcp/source/rtp_sender_video.h
index 56be39d..2cb40e3 100644
--- a/modules/rtp_rtcp/source/rtp_sender_video.h
+++ b/modules/rtp_rtcp/source/rtp_sender_video.h
@@ -59,14 +59,9 @@
void SetVideoCodecType(RtpVideoCodecTypes type);
- // FEC
- void SetUlpfecConfig(bool enabled,
- int red_payload_type,
- int ulpfec_payload_type);
-
- void GetUlpfecConfig(bool* enabled,
- int* red_payload_type,
- int* ulpfec_payload_type) const;
+ // ULPFEC.
+ void SetUlpfecConfig(int red_payload_type, int ulpfec_payload_type);
+ void GetUlpfecConfig(int* red_payload_type, int* ulpfec_payload_type) const;
void SetFecParameters(const FecProtectionParams* delta_params,
const FecProtectionParams* key_params);
@@ -85,6 +80,14 @@
StorageType media_packet_storage,
bool protect);
+ bool red_enabled() const EXCLUSIVE_LOCKS_REQUIRED(crit_) {
+ return red_payload_type_ >= 0;
+ }
+
+ bool ulpfec_enabled() const EXCLUSIVE_LOCKS_REQUIRED(crit_) {
+ return ulpfec_payload_type_ >= 0;
+ }
+
RTPSender* const rtp_sender_;
Clock* const clock_;
@@ -96,10 +99,9 @@
int32_t retransmission_settings_ GUARDED_BY(crit_);
VideoRotation last_rotation_ GUARDED_BY(encoder_checker_);
- // FEC
- bool fec_enabled_ GUARDED_BY(crit_);
+ // RED/ULPFEC.
int red_payload_type_ GUARDED_BY(crit_);
- int fec_payload_type_ GUARDED_BY(crit_);
+ int ulpfec_payload_type_ GUARDED_BY(crit_);
FecProtectionParams delta_fec_params_ GUARDED_BY(crit_);
FecProtectionParams key_fec_params_ GUARDED_BY(crit_);
UlpfecGenerator ulpfec_generator_ GUARDED_BY(crit_);
diff --git a/video/rtp_stream_receiver.cc b/video/rtp_stream_receiver.cc
index cf108e1..576543b 100644
--- a/video/rtp_stream_receiver.cc
+++ b/video/rtp_stream_receiver.cc
@@ -178,8 +178,10 @@
config_.rtp.ulpfec.red_rtx_payload_type,
config_.rtp.ulpfec.red_payload_type);
}
-
- rtp_rtcp_->SetUlpfecConfig(true, config_.rtp.ulpfec.red_payload_type,
+ // TODO(brandtr): It doesn't seem that |rtp_rtcp_| is used for sending any
+ // RTP packets. Investigate if this is the case, and if so, remove this
+ // call, since there are no RTP packets to protect with RED+ULPFEC.
+ rtp_rtcp_->SetUlpfecConfig(config_.rtp.ulpfec.red_payload_type,
config_.rtp.ulpfec.ulpfec_payload_type);
}
diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc
index 14fd72f..98baf05 100644
--- a/video/video_send_stream.cc
+++ b/video/video_send_stream.cc
@@ -934,7 +934,8 @@
RTC_DCHECK_RUN_ON(worker_queue_);
// Enable NACK, FEC or both.
const bool enable_protection_nack = config_->rtp.nack.rtp_history_ms > 0;
- bool enable_protection_fec = config_->rtp.ulpfec.ulpfec_payload_type != -1;
+ const int red_payload_type = config_->rtp.ulpfec.red_payload_type;
+ int ulpfec_payload_type = config_->rtp.ulpfec.ulpfec_payload_type;
// Payload types without picture ID cannot determine that a stream is complete
// without retransmitting FEC, so using FEC + NACK for H.264 (for instance) is
// a waste of bandwidth since FEC packets still have to be transmitted. Note
@@ -945,7 +946,7 @@
LOG(LS_WARNING) << "Transmitting payload type without picture ID using"
"NACK+FEC is a waste of bandwidth since FEC packets "
"also have to be retransmitted. Disabling FEC.";
- enable_protection_fec = false;
+ ulpfec_payload_type = -1;
}
// TODO(brandtr): Remove the workaround described below.
@@ -961,13 +962,13 @@
// using the wrong payload type.
// Verify validity of provided payload types.
- if (config_->rtp.ulpfec.red_payload_type != -1) {
- RTC_DCHECK_GE(config_->rtp.ulpfec.red_payload_type, 0);
- RTC_DCHECK_LE(config_->rtp.ulpfec.red_payload_type, 127);
+ if (red_payload_type != -1) {
+ RTC_DCHECK_GE(red_payload_type, 0);
+ RTC_DCHECK_LE(red_payload_type, 127);
}
- if (config_->rtp.ulpfec.ulpfec_payload_type != -1) {
- RTC_DCHECK_GE(config_->rtp.ulpfec.ulpfec_payload_type, 0);
- RTC_DCHECK_LE(config_->rtp.ulpfec.ulpfec_payload_type, 127);
+ if (ulpfec_payload_type != -1) {
+ RTC_DCHECK_GE(ulpfec_payload_type, 0);
+ RTC_DCHECK_LE(ulpfec_payload_type, 127);
}
for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
@@ -977,12 +978,11 @@
kMinSendSidePacketHistorySize);
// Set FEC.
for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
- rtp_rtcp->SetUlpfecConfig(enable_protection_fec,
- config_->rtp.ulpfec.red_payload_type,
- config_->rtp.ulpfec.ulpfec_payload_type);
+ rtp_rtcp->SetUlpfecConfig(red_payload_type, ulpfec_payload_type);
}
}
+ const bool enable_protection_fec = (ulpfec_payload_type != -1);
protection_bitrate_calculator_.SetProtectionMethod(enable_protection_fec,
enable_protection_nack);
}
