diff --git a/modules/rtp_rtcp/include/ulpfec_receiver.h b/modules/rtp_rtcp/include/ulpfec_receiver.h index fce6e888cc..30fac726fa 100644 --- a/modules/rtp_rtcp/include/ulpfec_receiver.h +++ b/modules/rtp_rtcp/include/ulpfec_receiver.h @@ -11,6 +11,10 @@ #ifndef MODULES_RTP_RTCP_INCLUDE_ULPFEC_RECEIVER_H_ #define MODULES_RTP_RTCP_INCLUDE_ULPFEC_RECEIVER_H_ +#include + +#include "api/array_view.h" +#include "api/rtp_parameters.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" namespace webrtc { @@ -30,8 +34,10 @@ struct FecPacketCounter { class UlpfecReceiver { public: - static UlpfecReceiver* Create(uint32_t ssrc, - RecoveredPacketReceiver* callback); + static std::unique_ptr Create( + uint32_t ssrc, + RecoveredPacketReceiver* callback, + rtc::ArrayView extensions); virtual ~UlpfecReceiver() {} diff --git a/modules/rtp_rtcp/source/flexfec_receiver.cc b/modules/rtp_rtcp/source/flexfec_receiver.cc index 27dacac249..4c788f4b22 100644 --- a/modules/rtp_rtcp/source/flexfec_receiver.cc +++ b/modules/rtp_rtcp/source/flexfec_receiver.cc @@ -123,10 +123,10 @@ FlexfecReceiver::AddReceivedPacket(const RtpPacketReceived& packet) { received_packet->is_fec = false; // Insert entire packet into erasure code. - // TODO(brandtr): Remove this memcpy too. received_packet->pkt = rtc::scoped_refptr( new ForwardErrorCorrection::Packet()); - memcpy(received_packet->pkt->data, packet.data(), packet.size()); + // Create a copy and fill with zeros all mutable extensions. + packet.CopyAndZeroMutableExtensions(received_packet->pkt->data); received_packet->pkt->length = packet.size(); } diff --git a/modules/rtp_rtcp/source/rtp_packet.cc b/modules/rtp_rtcp/source/rtp_packet.cc index 7e7a6975ac..282bea2a85 100644 --- a/modules/rtp_rtcp/source/rtp_packet.cc +++ b/modules/rtp_rtcp/source/rtp_packet.cc @@ -156,6 +156,51 @@ void RtpPacket::SetSsrc(uint32_t ssrc) { ByteWriter::WriteBigEndian(WriteAt(8), ssrc); } +void RtpPacket::CopyAndZeroMutableExtensions( + rtc::ArrayView buffer) const { + RTC_CHECK_GE(buffer.size(), buffer_.size()); + memcpy(buffer.data(), buffer_.cdata(), buffer_.size()); + for (const ExtensionInfo& extension : extension_entries_) { + switch (extensions_.GetType(extension.id)) { + case RTPExtensionType::kRtpExtensionNone: { + RTC_LOG(LS_WARNING) << "Unidentified extension in the packet."; + break; + } + case RTPExtensionType::kRtpExtensionVideoTiming: { + // Nullify 3 last entries: packetization delay and 2 network timestamps. + // Each of them is 2 bytes. + memset(buffer.data() + extension.offset + + VideoSendTiming::kPacerExitDeltaOffset, + 0, 6); + break; + } + case RTPExtensionType::kRtpExtensionTransportSequenceNumber: + case RTPExtensionType::kRtpExtensionTransportSequenceNumber02: + case RTPExtensionType::kRtpExtensionTransmissionTimeOffset: + case RTPExtensionType::kRtpExtensionAbsoluteSendTime: { + // Nullify whole extension, as it's filled in the pacer. + memset(buffer.data() + extension.offset, 0, extension.length); + break; + } + case RTPExtensionType::kRtpExtensionAudioLevel: + case RTPExtensionType::kRtpExtensionColorSpace: + case RTPExtensionType::kRtpExtensionFrameMarking: + case RTPExtensionType::kRtpExtensionGenericFrameDescriptor00: + case RTPExtensionType::kRtpExtensionGenericFrameDescriptor01: + case RTPExtensionType::kRtpExtensionMid: + case RTPExtensionType::kRtpExtensionNumberOfExtensions: + case RTPExtensionType::kRtpExtensionPlayoutDelay: + case RTPExtensionType::kRtpExtensionRepairedRtpStreamId: + case RTPExtensionType::kRtpExtensionRtpStreamId: + case RTPExtensionType::kRtpExtensionVideoContentType: + case RTPExtensionType::kRtpExtensionVideoRotation: { + // Non-mutable extension. Don't change it. + break; + } + } + } +} + void RtpPacket::SetCsrcs(rtc::ArrayView csrcs) { RTC_DCHECK_EQ(extensions_size_, 0); RTC_DCHECK_EQ(payload_size_, 0); diff --git a/modules/rtp_rtcp/source/rtp_packet.h b/modules/rtp_rtcp/source/rtp_packet.h index a0ecf1e74d..8c4ddf318e 100644 --- a/modules/rtp_rtcp/source/rtp_packet.h +++ b/modules/rtp_rtcp/source/rtp_packet.h @@ -85,6 +85,10 @@ class RtpPacket { void SetTimestamp(uint32_t timestamp); void SetSsrc(uint32_t ssrc); + // Copies the buffer with zero-ed mutable extensions, + // which are modified after FEC protection is generated. + void CopyAndZeroMutableExtensions(rtc::ArrayView buffer) const; + // Writes csrc list. Assumes: // a) There is enough room left in buffer. // b) Extension headers, payload or padding data has not already been added. diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index cebc813719..7cd1367b96 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -1324,114 +1324,6 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { EXPECT_EQ(kFlexfecSsrc, flexfec_packet.Ssrc()); } -// TODO(ilnik): because of webrtc:7859. Once FEC moved below pacer, this test -// should be removed. -TEST_P(RtpSenderTest, NoFlexfecForTimingFrames) { - constexpr uint32_t kTimestamp = 1234; - const int64_t kCaptureTimeMs = fake_clock_.TimeInMilliseconds(); - constexpr int kMediaPayloadType = 127; - constexpr int kFlexfecPayloadType = 118; - constexpr uint32_t kMediaSsrc = 1234; - constexpr uint32_t kFlexfecSsrc = 5678; - const std::vector kNoRtpExtensions; - const std::vector kNoRtpExtensionSizes; - - FlexfecSender flexfec_sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kNoMid, kNoRtpExtensions, kNoRtpExtensionSizes, - nullptr /* rtp_state */, &fake_clock_); - - // Reset |rtp_sender_| to use FlexFEC. - rtp_sender_.reset(new RTPSender( - false, &fake_clock_, &transport_, &mock_paced_sender_, - flexfec_sender.ssrc(), &seq_num_allocator_, nullptr, nullptr, nullptr, - &mock_rtc_event_log_, &send_packet_observer_, - &retransmission_rate_limiter_, nullptr, false, nullptr, false, false, - FieldTrialBasedConfig())); - rtp_sender_->SetSSRC(kMediaSsrc); - rtp_sender_->SetSequenceNumber(kSeqNum); - rtp_sender_->SetStorePacketsStatus(true, 10); - - PlayoutDelayOracle playout_delay_oracle; - RTPSenderVideo rtp_sender_video( - &fake_clock_, rtp_sender_.get(), &flexfec_sender, &playout_delay_oracle, - nullptr, false, false, FieldTrialBasedConfig()); - rtp_sender_video.RegisterPayloadType(kMediaPayloadType, "GENERIC", - /*raw_payload=*/false); - - // Need extension to be registered for timing frames to be sent. - ASSERT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( - kRtpExtensionVideoTiming, kVideoTimingExtensionId)); - - // Parameters selected to generate a single FEC packet per media packet. - FecProtectionParams params; - params.fec_rate = 15; - params.max_fec_frames = 1; - params.fec_mask_type = kFecMaskRandom; - rtp_sender_video.SetFecParameters(params, params); - - EXPECT_CALL(mock_paced_sender_, - InsertPacket(RtpPacketSender::kLowPriority, kMediaSsrc, kSeqNum, - _, _, false)); - EXPECT_CALL(mock_paced_sender_, InsertPacket(RtpPacketSender::kLowPriority, - kFlexfecSsrc, _, _, _, false)) - .Times(0); // Not called because packet should not be protected. - - RTPVideoHeader video_header; - video_header.video_timing.flags = VideoSendTiming::kTriggeredByTimer; - EXPECT_TRUE(rtp_sender_video.SendVideo( - VideoFrameType::kVideoFrameKey, kMediaPayloadType, kTimestamp, - kCaptureTimeMs, kPayloadData, sizeof(kPayloadData), nullptr, - &video_header, kDefaultExpectedRetransmissionTimeMs)); - - EXPECT_CALL(mock_rtc_event_log_, - LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing))) - .Times(1); - EXPECT_EQ(RtpPacketSendResult::kSuccess, - rtp_sender_->TimeToSendPacket(kMediaSsrc, kSeqNum, - fake_clock_.TimeInMilliseconds(), - false, PacedPacketInfo())); - ASSERT_EQ(1, transport_.packets_sent()); - const RtpPacketReceived& media_packet = transport_.sent_packets_[0]; - EXPECT_EQ(kMediaPayloadType, media_packet.PayloadType()); - EXPECT_EQ(kSeqNum, media_packet.SequenceNumber()); - EXPECT_EQ(kMediaSsrc, media_packet.Ssrc()); - - // Now try to send not a timing frame. - uint16_t flexfec_seq_num; - EXPECT_CALL(mock_paced_sender_, InsertPacket(RtpPacketSender::kLowPriority, - kFlexfecSsrc, _, _, _, false)) - .WillOnce(SaveArg<2>(&flexfec_seq_num)); - EXPECT_CALL(mock_paced_sender_, - InsertPacket(RtpPacketSender::kLowPriority, kMediaSsrc, - kSeqNum + 1, _, _, false)); - video_header.video_timing.flags = VideoSendTiming::kInvalid; - EXPECT_TRUE(rtp_sender_video.SendVideo( - VideoFrameType::kVideoFrameKey, kMediaPayloadType, kTimestamp + 1, - kCaptureTimeMs + 1, kPayloadData, sizeof(kPayloadData), nullptr, - &video_header, kDefaultExpectedRetransmissionTimeMs)); - - EXPECT_CALL(mock_rtc_event_log_, - LogProxy(SameRtcEventTypeAs(RtcEvent::Type::RtpPacketOutgoing))) - .Times(2); - EXPECT_EQ(RtpPacketSendResult::kSuccess, - rtp_sender_->TimeToSendPacket(kMediaSsrc, kSeqNum + 1, - fake_clock_.TimeInMilliseconds(), - false, PacedPacketInfo())); - EXPECT_EQ(RtpPacketSendResult::kSuccess, - rtp_sender_->TimeToSendPacket(kFlexfecSsrc, flexfec_seq_num, - fake_clock_.TimeInMilliseconds(), - false, PacedPacketInfo())); - ASSERT_EQ(3, transport_.packets_sent()); - const RtpPacketReceived& media_packet2 = transport_.sent_packets_[1]; - EXPECT_EQ(kMediaPayloadType, media_packet2.PayloadType()); - EXPECT_EQ(kSeqNum + 1, media_packet2.SequenceNumber()); - EXPECT_EQ(kMediaSsrc, media_packet2.Ssrc()); - const RtpPacketReceived& flexfec_packet = transport_.sent_packets_[2]; - EXPECT_EQ(kFlexfecPayloadType, flexfec_packet.PayloadType()); - EXPECT_EQ(flexfec_seq_num, flexfec_packet.SequenceNumber()); - EXPECT_EQ(kFlexfecSsrc, flexfec_packet.Ssrc()); -} - TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { constexpr uint32_t kTimestamp = 1234; constexpr int kMediaPayloadType = 127; diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index bbadda80ba..750dcf59d1 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -692,12 +692,12 @@ bool RTPSenderVideo::SendVideo( // Put packetization finish timestamp into extension. if (packet->HasExtension()) { packet->set_packetization_finish_time_ms(clock_->TimeInMilliseconds()); - // TODO(ilnik): Due to webrtc:7859, packets with timing extensions are not - // protected by FEC. It reduces FEC efficiency a bit. When FEC is moved - // below the pacer, it can be re-enabled for these packets. - // NOTE: Any RTP stream processor in the network, modifying 'network' - // timestamps in the timing frames extension have to be an end-point for - // FEC, otherwise recovered by FEC packets will be corrupted. + // TODO(webrtc:10750): wait a couple of months and remove the statement + // below. For now we can't use packets with VideoTimingFrame extensions in + // Fec because the extension is modified after FEC is calculated by pacer + // and network. This may cause corruptions in video payload and header. + // The fix in receive code is implemented, but until all the receivers + // are updated, senders can't send potentially breaking packets. protect_packet = false; } diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc index 614d4aad22..8efcaf223f 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc +++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc @@ -14,21 +14,28 @@ #include #include +#include "absl/memory/memory.h" #include "api/scoped_refptr.h" #include "modules/rtp_rtcp/source/byte_io.h" +#include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "rtc_base/logging.h" #include "rtc_base/time_utils.h" namespace webrtc { -UlpfecReceiver* UlpfecReceiver::Create(uint32_t ssrc, - RecoveredPacketReceiver* callback) { - return new UlpfecReceiverImpl(ssrc, callback); +std::unique_ptr UlpfecReceiver::Create( + uint32_t ssrc, + RecoveredPacketReceiver* callback, + rtc::ArrayView extensions) { + return absl::make_unique(ssrc, callback, extensions); } -UlpfecReceiverImpl::UlpfecReceiverImpl(uint32_t ssrc, - RecoveredPacketReceiver* callback) +UlpfecReceiverImpl::UlpfecReceiverImpl( + uint32_t ssrc, + RecoveredPacketReceiver* callback, + rtc::ArrayView extensions) : ssrc_(ssrc), + extensions_(extensions), recovered_packet_callback_(callback), fec_(ForwardErrorCorrection::CreateUlpfec(ssrc_)) {} @@ -243,6 +250,13 @@ int32_t UlpfecReceiverImpl::ProcessReceivedFec() { recovered_packet_callback_->OnRecoveredPacket(packet->data, packet->length); crit_sect_.Enter(); + RtpPacketReceived rtp_packet; + // TODO(ilnik): move extension nullifying out of RtpPacket, so there's no + // need to create one here, and avoid two memcpy calls below. + rtp_packet.Parse(packet->data, packet->length); // Does memcopy. + rtp_packet.IdentifyExtensions(extensions_); + rtp_packet.CopyAndZeroMutableExtensions( // Does memcopy. + rtc::MakeArrayView(packet->data, packet->length)); } fec_->DecodeFec(*received_packet, &recovered_packets_); } diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_impl.h b/modules/rtp_rtcp/source/ulpfec_receiver_impl.h index 2821513f0e..fca80c4dd4 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.h +++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.h @@ -17,6 +17,7 @@ #include #include "api/rtp_headers.h" +#include "modules/rtp_rtcp/include/rtp_header_extension_map.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/include/ulpfec_receiver.h" #include "modules/rtp_rtcp/source/forward_error_correction.h" @@ -26,7 +27,9 @@ namespace webrtc { class UlpfecReceiverImpl : public UlpfecReceiver { public: - explicit UlpfecReceiverImpl(uint32_t ssrc, RecoveredPacketReceiver* callback); + explicit UlpfecReceiverImpl(uint32_t ssrc, + RecoveredPacketReceiver* callback, + rtc::ArrayView extensions); ~UlpfecReceiverImpl() override; int32_t AddReceivedRedPacket(const RTPHeader& rtp_header, @@ -40,6 +43,7 @@ class UlpfecReceiverImpl : public UlpfecReceiver { private: const uint32_t ssrc_; + const RtpHeaderExtensionMap extensions_; rtc::CriticalSection crit_sect_; RecoveredPacketReceiver* recovered_packet_callback_; diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc b/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc index 2203fc8fd7..dd33d6b6e9 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc @@ -49,8 +49,9 @@ class UlpfecReceiverTest : public ::testing::Test { protected: UlpfecReceiverTest() : fec_(ForwardErrorCorrection::CreateUlpfec(kMediaSsrc)), - receiver_fec_( - UlpfecReceiver::Create(kMediaSsrc, &recovered_packet_receiver_)), + receiver_fec_(UlpfecReceiver::Create(kMediaSsrc, + &recovered_packet_receiver_, + {})), packet_generator_(kMediaSsrc) {} // Generates |num_fec_packets| FEC packets, given |media_packets|. @@ -180,7 +181,7 @@ void UlpfecReceiverTest::SurvivesMaliciousPacket(const uint8_t* data, NullRecoveredPacketReceiver null_callback; std::unique_ptr receiver_fec( - UlpfecReceiver::Create(kMediaSsrc, &null_callback)); + UlpfecReceiver::Create(kMediaSsrc, &null_callback, {})); receiver_fec->AddReceivedRedPacket(header, data, length, ulpfec_payload_type); } diff --git a/test/fuzzers/ulpfec_receiver_fuzzer.cc b/test/fuzzers/ulpfec_receiver_fuzzer.cc index 7cb0fc61b5..1124e01112 100644 --- a/test/fuzzers/ulpfec_receiver_fuzzer.cc +++ b/test/fuzzers/ulpfec_receiver_fuzzer.cc @@ -36,7 +36,7 @@ void FuzzOneInput(const uint8_t* data, size_t size) { DummyCallback callback; std::unique_ptr receiver( - UlpfecReceiver::Create(ulpfec_ssrc, &callback)); + UlpfecReceiver::Create(ulpfec_ssrc, &callback, {})); std::unique_ptr packet; size_t packet_length; diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index 237a4c2c51..547c2cf47d 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -179,7 +179,9 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( ntp_estimator_(clock), rtp_header_extensions_(config_.rtp.extensions), rtp_receive_statistics_(rtp_receive_statistics), - ulpfec_receiver_(UlpfecReceiver::Create(config->rtp.remote_ssrc, this)), + ulpfec_receiver_(UlpfecReceiver::Create(config->rtp.remote_ssrc, + this, + config->rtp.extensions)), receiving_(false), last_packet_log_ms_(-1), rtp_rtcp_(CreateRtpRtcpModule(clock, diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc index fc5a5b9ff7..77a63f6528 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc @@ -591,6 +591,7 @@ class UlpfecObserver : public test::EndToEndTest { send_config->rtp.extensions.push_back( RtpExtension(RtpExtension::kAbsSendTimeUri, kAbsSendTimeExtensionId)); } + (*receive_configs)[0].rtp.extensions = send_config->rtp.extensions; encoder_config->codec_type = PayloadStringToCodecType(payload_name_); (*receive_configs)[0].rtp.red_payload_type = send_config->rtp.ulpfec.red_payload_type; @@ -787,6 +788,7 @@ class FlexfecObserver : public test::EndToEndTest { } else { send_config->rtp.extensions.clear(); } + (*receive_configs)[0].rtp.extensions = send_config->rtp.extensions; encoder_config->codec_type = PayloadStringToCodecType(payload_name_); } @@ -2033,8 +2035,7 @@ TEST_F(VideoSendStreamTest, CanReconfigureToUseStartBitrateAbovePreviousMax) { class StartBitrateObserver : public test::FakeEncoder { public: StartBitrateObserver() - : FakeEncoder(Clock::GetRealTimeClock()), - start_bitrate_kbps_(0) {} + : FakeEncoder(Clock::GetRealTimeClock()), start_bitrate_kbps_(0) {} int32_t InitEncode(const VideoCodec* config, const Settings& settings) override { rtc::CritScope lock(&crit_);