Add test which verifies that the RTP header extensions are set correctly for FEC packets.
Also taking the opportunity to do a little bit of clean up.
BUG=webrtc:705
R=pbos@webrtc.org
Review URL: https://codereview.webrtc.org/1506863002 .
Cr-Commit-Position: refs/heads/master@{#10927}
diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
index 7a9a8be..e28dc27 100644
--- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
+++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
@@ -129,9 +129,7 @@
int num_maskBytes = l_bit ? kMaskSizeLBitSet : kMaskSizeLBitClear;
// Do some error checking on the media packets.
- PacketList::const_iterator media_list_it = media_packet_list.begin();
- while (media_list_it != media_packet_list.end()) {
- Packet* media_packet = *media_list_it;
+ for (Packet* media_packet : media_packet_list) {
assert(media_packet);
if (media_packet->length < kRtpHeaderSize) {
@@ -146,7 +144,6 @@
LOG(LS_WARNING) << "Media packet " << media_packet->length << " bytes "
<< "with overhead is larger than " << IP_PACKET_SIZE;
}
- media_list_it++;
}
int num_fec_packets =
@@ -167,29 +164,30 @@
// -- Generate packet masks --
// Always allocate space for a large mask.
- uint8_t* packet_mask = new uint8_t[num_fec_packets * kMaskSizeLBitSet];
- memset(packet_mask, 0, num_fec_packets * num_maskBytes);
+ rtc::scoped_ptr<uint8_t[]> packet_mask(
+ new uint8_t[num_fec_packets * kMaskSizeLBitSet]);
+ memset(packet_mask.get(), 0, num_fec_packets * num_maskBytes);
internal::GeneratePacketMasks(num_media_packets, num_fec_packets,
num_important_packets, use_unequal_protection,
- mask_table, packet_mask);
+ mask_table, packet_mask.get());
- int num_maskBits = InsertZerosInBitMasks(media_packet_list, packet_mask,
- num_maskBytes, num_fec_packets);
+ int num_mask_bits = InsertZerosInBitMasks(
+ media_packet_list, packet_mask.get(), num_maskBytes, num_fec_packets);
- l_bit = (num_maskBits > 8 * kMaskSizeLBitClear);
+ l_bit = (num_mask_bits > 8 * kMaskSizeLBitClear);
- if (num_maskBits < 0) {
- delete[] packet_mask;
+ if (num_mask_bits < 0) {
return -1;
}
if (l_bit) {
num_maskBytes = kMaskSizeLBitSet;
}
- GenerateFecBitStrings(media_packet_list, packet_mask, num_fec_packets, l_bit);
- GenerateFecUlpHeaders(media_packet_list, packet_mask, l_bit, num_fec_packets);
+ GenerateFecBitStrings(media_packet_list, packet_mask.get(), num_fec_packets,
+ l_bit);
+ GenerateFecUlpHeaders(media_packet_list, packet_mask.get(), l_bit,
+ num_fec_packets);
- delete[] packet_mask;
return 0;
}
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
index 3e2a2b8..bec3623 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -42,13 +42,13 @@
_retransmissionSettings(kRetransmitBaseLayer),
// Generic FEC
- _fec(),
- _fecEnabled(false),
- _payloadTypeRED(-1),
- _payloadTypeFEC(-1),
+ fec_(),
+ fec_enabled_(false),
+ red_payload_type_(-1),
+ fec_payload_type_(-1),
delta_fec_params_(),
key_fec_params_(),
- producer_fec_(&_fec),
+ producer_fec_(&fec_),
_fecOverheadRate(clock, NULL),
_videoBitrate(clock, NULL) {
memset(&delta_fec_params_, 0, sizeof(delta_fec_params_));
@@ -130,7 +130,7 @@
// Only protect while creating RED and FEC packets, not when sending.
CriticalSectionScoped cs(crit_.get());
red_packet.reset(producer_fec_.BuildRedPacket(
- data_buffer, payload_length, rtp_header_length, _payloadTypeRED));
+ data_buffer, payload_length, rtp_header_length, red_payload_type_));
if (protect) {
producer_fec_.AddRtpPacketAndGenerateFec(data_buffer, payload_length,
rtp_header_length);
@@ -140,7 +140,7 @@
next_fec_sequence_number =
_rtpSender.AllocateSequenceNumber(num_fec_packets);
fec_packets = producer_fec_.GetFecPackets(
- _payloadTypeRED, _payloadTypeFEC, next_fec_sequence_number,
+ red_payload_type_, fec_payload_type_, next_fec_sequence_number,
rtp_header_length);
RTC_DCHECK_EQ(num_fec_packets, fec_packets.size());
if (_retransmissionSettings & kRetransmitFECPackets)
@@ -180,9 +180,9 @@
const uint8_t payloadTypeRED,
const uint8_t payloadTypeFEC) {
CriticalSectionScoped cs(crit_.get());
- _fecEnabled = enable;
- _payloadTypeRED = payloadTypeRED;
- _payloadTypeFEC = payloadTypeFEC;
+ fec_enabled_ = enable;
+ red_payload_type_ = payloadTypeRED;
+ fec_payload_type_ = payloadTypeFEC;
memset(&delta_fec_params_, 0, sizeof(delta_fec_params_));
memset(&key_fec_params_, 0, sizeof(key_fec_params_));
delta_fec_params_.max_fec_frames = key_fec_params_.max_fec_frames = 1;
@@ -194,14 +194,14 @@
uint8_t& payloadTypeRED,
uint8_t& payloadTypeFEC) const {
CriticalSectionScoped cs(crit_.get());
- enable = _fecEnabled;
- payloadTypeRED = _payloadTypeRED;
- payloadTypeFEC = _payloadTypeFEC;
+ enable = fec_enabled_;
+ payloadTypeRED = red_payload_type_;
+ payloadTypeFEC = fec_payload_type_;
}
size_t RTPSenderVideo::FECPacketOverhead() const {
CriticalSectionScoped cs(crit_.get());
- if (_fecEnabled) {
+ if (fec_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
@@ -247,7 +247,7 @@
frameType == kVideoFrameKey ? &key_fec_params_ : &delta_fec_params_;
producer_fec_.SetFecParameters(fec_params, 0);
storage = packetizer->GetStorageType(_retransmissionSettings);
- fec_enabled = _fecEnabled;
+ fec_enabled = fec_enabled_;
}
// Register CVO rtp header extension at the first time when we receive a frame
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.h b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.h
index 03ed600..a1e7976 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.h
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.h
@@ -110,10 +110,10 @@
int32_t _retransmissionSettings GUARDED_BY(crit_);
// FEC
- ForwardErrorCorrection _fec;
- bool _fecEnabled GUARDED_BY(crit_);
- int8_t _payloadTypeRED GUARDED_BY(crit_);
- int8_t _payloadTypeFEC GUARDED_BY(crit_);
+ ForwardErrorCorrection fec_;
+ bool fec_enabled_ GUARDED_BY(crit_);
+ int8_t red_payload_type_ GUARDED_BY(crit_);
+ int8_t fec_payload_type_ GUARDED_BY(crit_);
FecProtectionParams delta_fec_params_ GUARDED_BY(crit_);
FecProtectionParams key_fec_params_ GUARDED_BY(crit_);
ProducerFec producer_fec_ GUARDED_BY(crit_);
diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc
index 47a9613..0032230 100644
--- a/webrtc/video/video_send_stream_tests.cc
+++ b/webrtc/video/video_send_stream_tests.cc
@@ -127,12 +127,11 @@
}
TEST_F(VideoSendStreamTest, SupportsAbsoluteSendTime) {
- static const uint8_t kAbsSendTimeExtensionId = 13;
class AbsoluteSendTimeObserver : public test::SendTest {
public:
AbsoluteSendTimeObserver() : SendTest(kDefaultTimeoutMs) {
EXPECT_TRUE(parser_->RegisterRtpHeaderExtension(
- kRtpExtensionAbsoluteSendTime, kAbsSendTimeExtensionId));
+ kRtpExtensionAbsoluteSendTime, test::kAbsSendTimeExtensionId));
}
Action OnSendRtp(const uint8_t* packet, size_t length) override {
@@ -152,8 +151,8 @@
std::vector<VideoReceiveStream::Config>* receive_configs,
VideoEncoderConfig* encoder_config) override {
send_config->rtp.extensions.clear();
- send_config->rtp.extensions.push_back(
- RtpExtension(RtpExtension::kAbsSendTime, kAbsSendTimeExtensionId));
+ send_config->rtp.extensions.push_back(RtpExtension(
+ RtpExtension::kAbsSendTime, test::kAbsSendTimeExtensionId));
}
void PerformTest() override {
@@ -310,81 +309,123 @@
StatisticianMap stats_map_;
};
-TEST_F(VideoSendStreamTest, SupportsFec) {
- class FecObserver : public test::SendTest {
- public:
- FecObserver()
- : SendTest(kDefaultTimeoutMs),
- send_count_(0),
- received_media_(false),
- received_fec_(false) {
+class FecObserver : public test::SendTest {
+ public:
+ explicit FecObserver(bool header_extensions_enabled)
+ : SendTest(VideoSendStreamTest::kDefaultTimeoutMs),
+ send_count_(0),
+ received_media_(false),
+ received_fec_(false),
+ header_extensions_enabled_(header_extensions_enabled) {}
+
+ private:
+ Action OnSendRtp(const uint8_t* packet, size_t length) override {
+ RTPHeader header;
+ EXPECT_TRUE(parser_->Parse(packet, length, &header));
+
+ // Send lossy receive reports to trigger FEC enabling.
+ if (send_count_++ % 2 != 0) {
+ // Receive statistics reporting having lost 50% of the packets.
+ FakeReceiveStatistics lossy_receive_stats(
+ VideoSendStreamTest::kSendSsrcs[0], header.sequenceNumber,
+ send_count_ / 2, 127);
+ RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(),
+ &lossy_receive_stats, nullptr,
+ transport_adapter_.get());
+
+ rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize);
+ rtcp_sender.SetRemoteSSRC(VideoSendStreamTest::kSendSsrcs[0]);
+
+ RTCPSender::FeedbackState feedback_state;
+
+ EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr));
}
- private:
- Action OnSendRtp(const uint8_t* packet, size_t length) override {
- RTPHeader header;
- EXPECT_TRUE(parser_->Parse(packet, length, &header));
+ int encapsulated_payload_type = -1;
+ if (header.payloadType == VideoSendStreamTest::kRedPayloadType) {
+ encapsulated_payload_type = static_cast<int>(packet[header.headerLength]);
+ if (encapsulated_payload_type !=
+ VideoSendStreamTest::kFakeSendPayloadType)
+ EXPECT_EQ(VideoSendStreamTest::kUlpfecPayloadType,
+ encapsulated_payload_type);
+ } else {
+ EXPECT_EQ(VideoSendStreamTest::kFakeSendPayloadType, header.payloadType);
+ }
- // Send lossy receive reports to trigger FEC enabling.
- if (send_count_++ % 2 != 0) {
- // Receive statistics reporting having lost 50% of the packets.
- FakeReceiveStatistics lossy_receive_stats(
- kSendSsrcs[0], header.sequenceNumber, send_count_ / 2, 127);
- RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(),
- &lossy_receive_stats, nullptr,
- transport_adapter_.get());
-
- rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize);
- rtcp_sender.SetRemoteSSRC(kSendSsrcs[0]);
-
- RTCPSender::FeedbackState feedback_state;
-
- EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr));
- }
-
- int encapsulated_payload_type = -1;
- if (header.payloadType == kRedPayloadType) {
- encapsulated_payload_type =
- static_cast<int>(packet[header.headerLength]);
- if (encapsulated_payload_type != kFakeSendPayloadType)
- EXPECT_EQ(kUlpfecPayloadType, encapsulated_payload_type);
+ if (header_extensions_enabled_) {
+ EXPECT_TRUE(header.extension.hasAbsoluteSendTime);
+ uint32_t kHalf24BitsSpace = 0xFFFFFF / 2;
+ if (header.extension.absoluteSendTime <= kHalf24BitsSpace &&
+ prev_header_.extension.absoluteSendTime > kHalf24BitsSpace) {
+ // 24 bits wrap.
+ EXPECT_GT(prev_header_.extension.absoluteSendTime,
+ header.extension.absoluteSendTime);
} else {
- EXPECT_EQ(kFakeSendPayloadType, header.payloadType);
+ EXPECT_GE(header.extension.absoluteSendTime,
+ prev_header_.extension.absoluteSendTime);
}
+ EXPECT_TRUE(header.extension.hasTransportSequenceNumber);
+ uint16_t seq_num_diff = header.extension.transportSequenceNumber -
+ prev_header_.extension.transportSequenceNumber;
+ EXPECT_EQ(1, seq_num_diff);
+ }
- if (encapsulated_payload_type != -1) {
- if (encapsulated_payload_type == kUlpfecPayloadType) {
- received_fec_ = true;
- } else {
- received_media_ = true;
- }
+ if (encapsulated_payload_type != -1) {
+ if (encapsulated_payload_type ==
+ VideoSendStreamTest::kUlpfecPayloadType) {
+ received_fec_ = true;
+ } else {
+ received_media_ = true;
}
-
- if (received_media_ && received_fec_)
- observation_complete_->Set();
-
- return SEND_PACKET;
}
- void ModifyConfigs(VideoSendStream::Config* send_config,
- std::vector<VideoReceiveStream::Config>* receive_configs,
- VideoEncoderConfig* encoder_config) override {
- transport_adapter_.reset(
- new internal::TransportAdapter(send_config->send_transport));
- transport_adapter_->Enable();
- send_config->rtp.fec.red_payload_type = kRedPayloadType;
- send_config->rtp.fec.ulpfec_payload_type = kUlpfecPayloadType;
- }
+ if (received_media_ && received_fec_ && send_count_ > 100)
+ observation_complete_->Set();
- void PerformTest() override {
- EXPECT_TRUE(Wait()) << "Timed out waiting for FEC and media packets.";
- }
+ prev_header_ = header;
- rtc::scoped_ptr<internal::TransportAdapter> transport_adapter_;
- int send_count_;
- bool received_media_;
- bool received_fec_;
- } test;
+ return SEND_PACKET;
+ }
+
+ void ModifyConfigs(VideoSendStream::Config* send_config,
+ std::vector<VideoReceiveStream::Config>* receive_configs,
+ VideoEncoderConfig* encoder_config) override {
+ transport_adapter_.reset(
+ new internal::TransportAdapter(send_config->send_transport));
+ transport_adapter_->Enable();
+ send_config->rtp.fec.red_payload_type =
+ VideoSendStreamTest::kRedPayloadType;
+ send_config->rtp.fec.ulpfec_payload_type =
+ VideoSendStreamTest::kUlpfecPayloadType;
+ if (header_extensions_enabled_) {
+ send_config->rtp.extensions.push_back(RtpExtension(
+ RtpExtension::kAbsSendTime, test::kAbsSendTimeExtensionId));
+ send_config->rtp.extensions.push_back(
+ RtpExtension(RtpExtension::kTransportSequenceNumber,
+ test::kTransportSequenceNumberExtensionId));
+ }
+ }
+
+ void PerformTest() override {
+ EXPECT_TRUE(Wait()) << "Timed out waiting for FEC and media packets.";
+ }
+
+ rtc::scoped_ptr<internal::TransportAdapter> transport_adapter_;
+ int send_count_;
+ bool received_media_;
+ bool received_fec_;
+ bool header_extensions_enabled_;
+ RTPHeader prev_header_;
+};
+
+TEST_F(VideoSendStreamTest, SupportsFecWithExtensions) {
+ FecObserver test(true);
+
+ RunBaseTest(&test, FakeNetworkPipe::Config());
+}
+
+TEST_F(VideoSendStreamTest, SupportsFecWithoutExtensions) {
+ FecObserver test(false);
RunBaseTest(&test, FakeNetworkPipe::Config());
}
