Always send RED headers if configured.
This shouldn't be needed, but because the receiver assumes RTX packets
contain RED if configured to receive them (due to an incompatibility
issue), we also have to make sure we send them for now.
BUG=webrtc:5675
Review-Url: https://codereview.webrtc.org/2033763002
Cr-Commit-Position: refs/heads/master@{#13024}
(cherry picked from commit 8f4c77fea10010b5879a01bf6fcd485318594a85)
TBR=stefan@webrtc.org
Review URL: https://codereview.webrtc.org/2061943003 .
Cr-Commit-Position: refs/branch-heads/52@{#5}
Cr-Branched-From: a376e70cf9d0df3c35d53533b454da542661775b-refs/heads/master@{#12798}
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
index 0bf95b7..e10b5b2 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -36,8 +36,8 @@
// Generic FEC
fec_(),
fec_enabled_(false),
- red_payload_type_(-1),
- fec_payload_type_(-1),
+ red_payload_type_(0),
+ fec_payload_type_(0),
delta_fec_params_(),
key_fec_params_(),
producer_fec_(&fec_),
@@ -191,16 +191,19 @@
size_t RTPSenderVideo::FECPacketOverhead() const {
rtc::CritScope cs(&crit_);
- if (fec_enabled_) {
+ size_t overhead = 0;
+ if (red_payload_type_ != 0) {
// 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
// from an FEC viewpoint, they are part of the payload to be protected.
// (The base RTP header is already protected by the FEC header.)
- return ForwardErrorCorrection::PacketOverhead() + REDForFECHeaderLength +
- (_rtpSender.RTPHeaderLength() - kRtpHeaderSize);
+ overhead = REDForFECHeaderLength + (_rtpSender.RTPHeaderLength() -
+ kRtpHeaderSize);
}
- return 0;
+ if (fec_enabled_)
+ overhead += ForwardErrorCorrection::PacketOverhead();
+ return overhead;
}
void RTPSenderVideo::SetFecParameters(const FecProtectionParams* delta_params,
@@ -208,8 +211,10 @@
rtc::CritScope cs(&crit_);
RTC_DCHECK(delta_params);
RTC_DCHECK(key_params);
- delta_fec_params_ = *delta_params;
- key_fec_params_ = *key_params;
+ if (fec_enabled_) {
+ delta_fec_params_ = *delta_params;
+ key_fec_params_ = *key_params;
+ }
}
int32_t RTPSenderVideo::SendVideo(const RtpVideoCodecTypes videoType,
@@ -230,7 +235,7 @@
video_header ? &(video_header->codecHeader) : nullptr, frameType));
StorageType storage;
- bool fec_enabled;
+ int red_payload_type;
bool first_frame = first_frame_sent_();
{
rtc::CritScope cs(&crit_);
@@ -238,7 +243,7 @@
frameType == kVideoFrameKey ? &key_fec_params_ : &delta_fec_params_;
producer_fec_.SetFecParameters(fec_params, 0);
storage = packetizer->GetStorageType(_retransmissionSettings);
- fec_enabled = fec_enabled_;
+ red_payload_type = red_payload_type_;
}
// Register CVO rtp header extension at the first time when we receive a frame
@@ -301,7 +306,7 @@
_rtpSender.UpdateVideoRotation(dataBuffer, packetSize, rtp_header,
video_header->rotation);
}
- if (fec_enabled) {
+ if (red_payload_type != 0) {
SendVideoPacketAsRed(dataBuffer, payload_bytes_in_packet,
rtp_header_length, _rtpSender.SequenceNumber(),
captureTimeStamp, capture_time_ms, storage,
diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc
index 9555270..ec8fbea 100644
--- a/webrtc/video/video_send_stream.cc
+++ b/webrtc/video/video_send_stream.cc
@@ -654,7 +654,7 @@
void VideoSendStream::ConfigureProtection() {
// Enable NACK, FEC or both.
const bool enable_protection_nack = config_.rtp.nack.rtp_history_ms > 0;
- bool enable_protection_fec = config_.rtp.fec.red_payload_type != -1;
+ bool enable_protection_fec = config_.rtp.fec.ulpfec_payload_type != -1;
// 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
@@ -671,22 +671,25 @@
// Set to valid uint8_ts to be castable later without signed overflows.
uint8_t payload_type_red = 0;
uint8_t payload_type_fec = 0;
+
// TODO(changbin): Should set RTX for RED mapping in RTP sender in future.
// Validate payload types. If either RED or FEC payload types are set then
// both should be. If FEC is enabled then they both have to be set.
- if (enable_protection_fec || config_.rtp.fec.red_payload_type != -1 ||
- config_.rtp.fec.ulpfec_payload_type != -1) {
+ if (config_.rtp.fec.red_payload_type != -1) {
RTC_DCHECK_GE(config_.rtp.fec.red_payload_type, 0);
- RTC_DCHECK_GE(config_.rtp.fec.ulpfec_payload_type, 0);
RTC_DCHECK_LE(config_.rtp.fec.red_payload_type, 127);
- RTC_DCHECK_LE(config_.rtp.fec.ulpfec_payload_type, 127);
+ // TODO(holmer): We should only enable red if ulpfec is also enabled, but
+ // but due to an incompatibility issue with previous versions the receiver
+ // assumes rtx packets are containing red if it has been configured to
+ // receive red. Remove this in a few versions once the incompatibility
+ // issue is resolved (M53 timeframe).
payload_type_red = static_cast<uint8_t>(config_.rtp.fec.red_payload_type);
+ }
+ if (config_.rtp.fec.ulpfec_payload_type != -1) {
+ RTC_DCHECK_GE(config_.rtp.fec.ulpfec_payload_type, 0);
+ RTC_DCHECK_LE(config_.rtp.fec.ulpfec_payload_type, 127);
payload_type_fec =
static_cast<uint8_t>(config_.rtp.fec.ulpfec_payload_type);
- } else {
- // Payload types unset.
- RTC_DCHECK_EQ(config_.rtp.fec.red_payload_type, -1);
- RTC_DCHECK_EQ(config_.rtp.fec.ulpfec_payload_type, -1);
}
for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {
diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc
index 095ff4e..1d4acfc 100644
--- a/webrtc/video/video_send_stream_tests.cc
+++ b/webrtc/video/video_send_stream_tests.cc
@@ -308,11 +308,13 @@
FecObserver(bool header_extensions_enabled,
bool use_nack,
bool expect_red,
+ bool expect_fec,
const std::string& codec)
: EndToEndTest(VideoSendStreamTest::kDefaultTimeoutMs),
payload_name_(codec),
use_nack_(use_nack),
expect_red_(expect_red),
+ expect_fec_(expect_fec),
send_count_(0),
received_media_(false),
received_fec_(false),
@@ -375,6 +377,7 @@
if (encapsulated_payload_type != -1) {
if (encapsulated_payload_type ==
VideoSendStreamTest::kUlpfecPayloadType) {
+ EXPECT_TRUE(expect_fec_);
received_fec_ = true;
} else {
received_media_ = true;
@@ -382,7 +385,7 @@
}
if (send_count_ > 100 && received_media_) {
- if (received_fec_ || !expect_red_)
+ if (received_fec_ || !expect_fec_)
observation_complete_.Set();
}
@@ -442,6 +445,7 @@
const std::string payload_name_;
const bool use_nack_;
const bool expect_red_;
+ const bool expect_fec_;
int send_count_;
bool received_media_;
bool received_fec_;
@@ -450,12 +454,12 @@
};
TEST_F(VideoSendStreamTest, SupportsFecWithExtensions) {
- FecObserver test(true, false, true, "VP8");
+ FecObserver test(true, false, true, true, "VP8");
RunBaseTest(&test);
}
TEST_F(VideoSendStreamTest, SupportsFecWithoutExtensions) {
- FecObserver test(false, false, true, "VP8");
+ FecObserver test(false, false, true, true, "VP8");
RunBaseTest(&test);
}
@@ -463,25 +467,25 @@
// since we'll still have to re-request FEC packets, effectively wasting
// bandwidth since the receiver has to wait for FEC retransmissions to determine
// that the received state is actually decodable.
-TEST_F(VideoSendStreamTest, DoesNotUtilizeRedForH264WithNackEnabled) {
- FecObserver test(false, true, false, "H264");
+TEST_F(VideoSendStreamTest, DoesNotUtilizeFecForH264WithNackEnabled) {
+ FecObserver test(false, true, true, false, "H264");
RunBaseTest(&test);
}
// Without retransmissions FEC for H264 is fine.
TEST_F(VideoSendStreamTest, DoesUtilizeRedForH264WithoutNackEnabled) {
- FecObserver test(false, false, true, "H264");
+ FecObserver test(false, false, true, true, "H264");
RunBaseTest(&test);
}
TEST_F(VideoSendStreamTest, DoesUtilizeRedForVp8WithNackEnabled) {
- FecObserver test(false, true, true, "VP8");
+ FecObserver test(false, true, true, true, "VP8");
RunBaseTest(&test);
}
#if !defined(RTC_DISABLE_VP9)
TEST_F(VideoSendStreamTest, DoesUtilizeRedForVp9WithNackEnabled) {
- FecObserver test(false, true, true, "VP9");
+ FecObserver test(false, true, true, true, "VP9");
RunBaseTest(&test);
}
#endif // !defined(RTC_DISABLE_VP9)
