From 6817394eac2e6f3b65bbcbb84160c0831c88a308 Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Wed, 11 Mar 2020 12:59:07 +0100 Subject: [PATCH] Fix: don't use recovered packets in UlpFEC recovery Bug: b/141915452 Change-Id: I75324651694e5c3233bc3627269289d3f0a91514 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/170225 Commit-Queue: Artem Titov Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#30760} --- modules/rtp_rtcp/include/ulpfec_receiver.h | 4 +- modules/rtp_rtcp/source/fec_test_helper.cc | 14 ++--- modules/rtp_rtcp/source/fec_test_helper.h | 7 ++- .../source/forward_error_correction.h | 1 + .../rtp_rtcp/source/ulpfec_receiver_impl.cc | 14 +++-- .../rtp_rtcp/source/ulpfec_receiver_impl.h | 4 +- .../source/ulpfec_receiver_unittest.cc | 52 +++++++++++++++++-- test/fuzzers/ulpfec_receiver_fuzzer.cc | 2 +- 8 files changed, 77 insertions(+), 21 deletions(-) diff --git a/modules/rtp_rtcp/include/ulpfec_receiver.h b/modules/rtp_rtcp/include/ulpfec_receiver.h index eb55deca23..d3981dfac3 100644 --- a/modules/rtp_rtcp/include/ulpfec_receiver.h +++ b/modules/rtp_rtcp/include/ulpfec_receiver.h @@ -16,7 +16,7 @@ #include "api/array_view.h" #include "api/rtp_parameters.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" -#include "modules/rtp_rtcp/source/rtp_packet.h" +#include "modules/rtp_rtcp/source/rtp_packet_received.h" namespace webrtc { @@ -44,7 +44,7 @@ class UlpfecReceiver { // // TODO(brandtr): Set |ulpfec_payload_type| during constructor call, // rather than as a parameter here. - virtual bool AddReceivedRedPacket(const RtpPacket& rtp_packet, + virtual bool AddReceivedRedPacket(const RtpPacketReceived& rtp_packet, uint8_t ulpfec_payload_type) = 0; // Sends the received packets to the FEC and returns all packets diff --git a/modules/rtp_rtcp/source/fec_test_helper.cc b/modules/rtp_rtcp/source/fec_test_helper.cc index 1941e213ab..f8579b48ff 100644 --- a/modules/rtp_rtcp/source/fec_test_helper.cc +++ b/modules/rtp_rtcp/source/fec_test_helper.cc @@ -181,9 +181,10 @@ std::unique_ptr FlexfecPacketGenerator::BuildFlexfecPacket( UlpfecPacketGenerator::UlpfecPacketGenerator(uint32_t ssrc) : AugmentedPacketGenerator(ssrc) {} -RtpPacket UlpfecPacketGenerator::BuildMediaRedPacket( - const AugmentedPacket& packet) { - RtpPacket red_packet; +RtpPacketReceived UlpfecPacketGenerator::BuildMediaRedPacket( + const AugmentedPacket& packet, + bool is_recovered) { + RtpPacketReceived red_packet; // Copy RTP header. const size_t kHeaderLength = packet.header.headerLength; red_packet.Parse(packet.data.cdata(), kHeaderLength); @@ -196,25 +197,26 @@ RtpPacket UlpfecPacketGenerator::BuildMediaRedPacket( // Copy the payload. memcpy(rtp_payload + 1, packet.data.cdata() + kHeaderLength, packet.data.size() - kHeaderLength); + red_packet.set_recovered(is_recovered); return red_packet; } -RtpPacket UlpfecPacketGenerator::BuildUlpfecRedPacket( +RtpPacketReceived UlpfecPacketGenerator::BuildUlpfecRedPacket( const ForwardErrorCorrection::Packet& packet) { // Create a fake media packet to get a correct header. 1 byte RED header. ++num_packets_; std::unique_ptr fake_packet = NextPacket(0, packet.data.size() + 1); - RtpPacket red_packet; + RtpPacketReceived red_packet; red_packet.Parse(fake_packet->data); red_packet.SetMarker(false); uint8_t* rtp_payload = red_packet.AllocatePayload(packet.data.size() + 1); rtp_payload[0] = kFecPayloadType; red_packet.SetPayloadType(kRedPayloadType); - memcpy(rtp_payload + 1, packet.data.cdata(), packet.data.size()); + red_packet.set_recovered(false); return red_packet; } diff --git a/modules/rtp_rtcp/source/fec_test_helper.h b/modules/rtp_rtcp/source/fec_test_helper.h index e66e6ca0dc..b661fa8300 100644 --- a/modules/rtp_rtcp/source/fec_test_helper.h +++ b/modules/rtp_rtcp/source/fec_test_helper.h @@ -14,6 +14,7 @@ #include #include "modules/rtp_rtcp/source/forward_error_correction.h" +#include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "rtc_base/random.h" namespace webrtc { @@ -106,13 +107,15 @@ class UlpfecPacketGenerator : public AugmentedPacketGenerator { explicit UlpfecPacketGenerator(uint32_t ssrc); // Creates a new RtpPacket with the RED header added to the packet. - static RtpPacket BuildMediaRedPacket(const AugmentedPacket& packet); + static RtpPacketReceived BuildMediaRedPacket(const AugmentedPacket& packet, + bool is_recovered); // Creates a new RtpPacket with FEC payload and RED header. Does this by // creating a new fake media AugmentedPacket, clears the marker bit and adds a // RED header. Finally replaces the payload with the content of // |packet->data|. - RtpPacket BuildUlpfecRedPacket(const ForwardErrorCorrection::Packet& packet); + RtpPacketReceived BuildUlpfecRedPacket( + const ForwardErrorCorrection::Packet& packet); }; } // namespace fec diff --git a/modules/rtp_rtcp/source/forward_error_correction.h b/modules/rtp_rtcp/source/forward_error_correction.h index 100f532389..566ce7428a 100644 --- a/modules/rtp_rtcp/source/forward_error_correction.h +++ b/modules/rtp_rtcp/source/forward_error_correction.h @@ -85,6 +85,7 @@ class ForwardErrorCorrection { bool is_fec; // Set to true if this is an FEC packet and false // otherwise. + bool is_recovered; rtc::scoped_refptr pkt; // Pointer to the packet storage. }; diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc index ea85422ffe..4395d8ea6b 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc +++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc @@ -74,8 +74,9 @@ FecPacketCounter UlpfecReceiverImpl::GetPacketCounter() const { // block length: 10 bits Length in bytes of the corresponding data // block excluding header. -bool UlpfecReceiverImpl::AddReceivedRedPacket(const RtpPacket& rtp_packet, - uint8_t ulpfec_payload_type) { +bool UlpfecReceiverImpl::AddReceivedRedPacket( + const RtpPacketReceived& rtp_packet, + uint8_t ulpfec_payload_type) { if (rtp_packet.Ssrc() != ssrc_) { RTC_LOG(LS_WARNING) << "Received RED packet with different SSRC than expected; dropping."; @@ -103,6 +104,7 @@ bool UlpfecReceiverImpl::AddReceivedRedPacket(const RtpPacket& rtp_packet, // Get payload type from RED header and sequence number from RTP header. uint8_t payload_type = rtp_packet.payload()[0] & 0x7f; received_packet->is_fec = payload_type == ulpfec_payload_type; + received_packet->is_recovered = rtp_packet.recovered(); received_packet->ssrc = rtp_packet.Ssrc(); received_packet->seq_num = rtp_packet.SequenceNumber(); @@ -185,7 +187,13 @@ int32_t UlpfecReceiverImpl::ProcessReceivedFec() { RTC_DCHECK_EQ(packet->data.cdata(), original_data); } } - fec_->DecodeFec(*received_packet, &recovered_packets_); + if (!received_packet->is_recovered) { + // Do not pass recovered packets to FEC. Recovered packet might have + // different set of the RTP header extensions and thus different byte + // representation than the original packet, That will corrupt + // FEC calculation. + fec_->DecodeFec(*received_packet, &recovered_packets_); + } } // Send any recovered media packets to VCM. diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_impl.h b/modules/rtp_rtcp/source/ulpfec_receiver_impl.h index 7223696650..9e4e5b8f0b 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.h +++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.h @@ -21,7 +21,7 @@ #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" -#include "modules/rtp_rtcp/source/rtp_packet.h" +#include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "rtc_base/critical_section.h" namespace webrtc { @@ -33,7 +33,7 @@ class UlpfecReceiverImpl : public UlpfecReceiver { rtc::ArrayView extensions); ~UlpfecReceiverImpl() override; - bool AddReceivedRedPacket(const RtpPacket& rtp_packet, + bool AddReceivedRedPacket(const RtpPacketReceived& rtp_packet, uint8_t ulpfec_payload_type) override; int32_t ProcessReceivedFec() override; diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc b/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc index 0ef8085b63..4d6aa3d2c9 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc @@ -20,6 +20,7 @@ #include "modules/rtp_rtcp/source/byte_io.h" #include "modules/rtp_rtcp/source/fec_test_helper.h" #include "modules/rtp_rtcp/source/forward_error_correction.h" +#include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "test/gmock.h" #include "test/gtest.h" @@ -66,7 +67,8 @@ class UlpfecReceiverTest : public ::testing::Test { // Build a media packet using |packet_generator_| and add it // to the receiver. - void BuildAndAddRedMediaPacket(AugmentedPacket* packet); + void BuildAndAddRedMediaPacket(AugmentedPacket* packet, + bool is_recovered = false); // Build a FEC packet using |packet_generator_| and add it // to the receiver. @@ -120,13 +122,16 @@ void UlpfecReceiverTest::PacketizeFrame( } } -void UlpfecReceiverTest::BuildAndAddRedMediaPacket(AugmentedPacket* packet) { - RtpPacket red_packet = packet_generator_.BuildMediaRedPacket(*packet); +void UlpfecReceiverTest::BuildAndAddRedMediaPacket(AugmentedPacket* packet, + bool is_recovered) { + RtpPacketReceived red_packet = + packet_generator_.BuildMediaRedPacket(*packet, is_recovered); EXPECT_TRUE(receiver_fec_->AddReceivedRedPacket(red_packet, kFecPayloadType)); } void UlpfecReceiverTest::BuildAndAddRedFecPacket(Packet* packet) { - RtpPacket red_packet = packet_generator_.BuildUlpfecRedPacket(*packet); + RtpPacketReceived red_packet = + packet_generator_.BuildUlpfecRedPacket(*packet); EXPECT_TRUE(receiver_fec_->AddReceivedRedPacket(red_packet, kFecPayloadType)); } @@ -174,7 +179,7 @@ void UlpfecReceiverTest::SurvivesMaliciousPacket(const uint8_t* data, std::unique_ptr receiver_fec( UlpfecReceiver::Create(kMediaSsrc, &null_callback, {})); - RtpPacket rtp_packet; + RtpPacketReceived rtp_packet; ASSERT_TRUE(rtp_packet.Parse(data, length)); receiver_fec->AddReceivedRedPacket(rtp_packet, ulpfec_payload_type); } @@ -217,6 +222,43 @@ TEST_F(UlpfecReceiverTest, TwoMediaOneFec) { EXPECT_EQ(first_packet_time_ms, counter.first_packet_time_ms); } +TEST_F(UlpfecReceiverTest, TwoMediaOneFecNotUsesRecoveredPackets) { + constexpr size_t kNumFecPackets = 1u; + std::list augmented_media_packets; + ForwardErrorCorrection::PacketList media_packets; + PacketizeFrame(2, 0, &augmented_media_packets, &media_packets); + std::list fec_packets; + EncodeFec(media_packets, kNumFecPackets, &fec_packets); + + FecPacketCounter counter = receiver_fec_->GetPacketCounter(); + EXPECT_EQ(0u, counter.num_packets); + EXPECT_EQ(-1, counter.first_packet_time_ms); + + // Recovery + auto it = augmented_media_packets.begin(); + BuildAndAddRedMediaPacket(*it, /*is_recovered=*/true); + VerifyReconstructedMediaPacket(**it, 1); + EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); + counter = receiver_fec_->GetPacketCounter(); + EXPECT_EQ(1u, counter.num_packets); + EXPECT_EQ(0u, counter.num_fec_packets); + EXPECT_EQ(0u, counter.num_recovered_packets); + const int64_t first_packet_time_ms = counter.first_packet_time_ms; + EXPECT_NE(-1, first_packet_time_ms); + + // Drop one media packet. + auto fec_it = fec_packets.begin(); + BuildAndAddRedFecPacket(*fec_it); + ++it; + EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); + + counter = receiver_fec_->GetPacketCounter(); + EXPECT_EQ(2u, counter.num_packets); + EXPECT_EQ(1u, counter.num_fec_packets); + EXPECT_EQ(0u, counter.num_recovered_packets); + EXPECT_EQ(first_packet_time_ms, counter.first_packet_time_ms); +} + TEST_F(UlpfecReceiverTest, InjectGarbageFecHeaderLengthRecovery) { // Byte offset 8 is the 'length recovery' field of the FEC header. InjectGarbagePacketLength(8); diff --git a/test/fuzzers/ulpfec_receiver_fuzzer.cc b/test/fuzzers/ulpfec_receiver_fuzzer.cc index 9c76976290..042aa5d112 100644 --- a/test/fuzzers/ulpfec_receiver_fuzzer.cc +++ b/test/fuzzers/ulpfec_receiver_fuzzer.cc @@ -44,7 +44,7 @@ void FuzzOneInput(const uint8_t* data, size_t size) { size_t packet_length = kRtpHeaderSize + fuzz_data.Read(); auto raw_packet = fuzz_data.ReadByteArray(packet_length); - RtpPacket parsed_packet; + RtpPacketReceived parsed_packet; if (!parsed_packet.Parse(raw_packet)) continue;