From 2d821c3cbc664371294fe37c3f7197af2b80fc41 Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Wed, 26 Jun 2019 14:39:36 +0200 Subject: [PATCH] Allow VideoTimingExtension to be used with FEC This CL allows for FEC protection of packets with VideoTimingExtension by zero-ing out data, which is changed after FEC protection is generated (i.e in the pacer or by the SFU). Actual FEC protection of these packets would be enabled later, when all modern receivers have this change. Bug: webrtc:10750 Change-Id: If4785392204d68cb8527629727b5c062f9fb6600 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/143760 Reviewed-by: Niels Moller Reviewed-by: Rasmus Brandt Reviewed-by: Danil Chapovalov Commit-Queue: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#28396} --- modules/rtp_rtcp/include/ulpfec_receiver.h | 10 +- modules/rtp_rtcp/source/flexfec_receiver.cc | 4 +- modules/rtp_rtcp/source/rtp_packet.cc | 45 ++++++++ modules/rtp_rtcp/source/rtp_packet.h | 4 + .../rtp_rtcp/source/rtp_sender_unittest.cc | 108 ------------------ modules/rtp_rtcp/source/rtp_sender_video.cc | 12 +- .../rtp_rtcp/source/ulpfec_receiver_impl.cc | 24 +++- .../rtp_rtcp/source/ulpfec_receiver_impl.h | 6 +- .../source/ulpfec_receiver_unittest.cc | 7 +- test/fuzzers/ulpfec_receiver_fuzzer.cc | 2 +- video/rtp_video_stream_receiver.cc | 4 +- video/video_send_stream_tests.cc | 5 +- 12 files changed, 100 insertions(+), 131 deletions(-) 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_);