From 4654d204e42d00dea43ce1e5b2200063e8272c8b Mon Sep 17 00:00:00 2001 From: Stefan Holmer Date: Tue, 8 Dec 2015 09:10:43 +0100 Subject: [PATCH] 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} --- .../source/forward_error_correction.cc | 28 ++- .../rtp_rtcp/source/rtp_sender_video.cc | 30 +-- .../rtp_rtcp/source/rtp_sender_video.h | 8 +- webrtc/video/video_send_stream_tests.cc | 177 +++++++++++------- 4 files changed, 141 insertions(+), 102 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc index 7a9a8beecf..e28dc27c22 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 @@ int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list, 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 @@ int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list, 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 @@ int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list, // -- 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 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 3e2a2b8034..bec3623656 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 @@ RTPSenderVideo::RTPSenderVideo(Clock* clock, RTPSenderInterface* rtpSender) _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 @@ void RTPSenderVideo::SendVideoPacketAsRed(uint8_t* data_buffer, // 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 @@ void RTPSenderVideo::SendVideoPacketAsRed(uint8_t* data_buffer, 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 @@ void RTPSenderVideo::SetGenericFECStatus(const bool enable, 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 @@ void RTPSenderVideo::GenericFECStatus(bool& enable, 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 @@ int32_t RTPSenderVideo::SendVideo(const RtpVideoCodecTypes videoType, 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 03ed6000da..a1e7976418 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 @@ private: 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 47a961369b..00322300e4 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -127,12 +127,11 @@ TEST_F(VideoSendStreamTest, SupportsCName) { } 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 @@ TEST_F(VideoSendStreamTest, SupportsAbsoluteSendTime) { std::vector* 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 @@ class FakeReceiveStatistics : public NullReceiveStatistics { 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(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(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* 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 transport_adapter_; - int send_count_; - bool received_media_; - bool received_fec_; - } test; + return SEND_PACKET; + } + + void ModifyConfigs(VideoSendStream::Config* send_config, + std::vector* 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 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()); }