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 <titovartem@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30760}

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

modules/rtp_rtcp/source/fec_test_helper.cc

@@ -181,9 +181,10 @@ std::unique_ptr<AugmentedPacket> 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<AugmentedPacket> 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;
 }

modules/rtp_rtcp/source/fec_test_helper.h

@@ -14,6 +14,7 @@
 #include <memory>
 
 #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

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<Packet> pkt;  // Pointer to the packet storage.
   };
 

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.

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<const RtpExtension> 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;

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<UlpfecReceiver> 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<AugmentedPacket*> augmented_media_packets;
+  ForwardErrorCorrection::PacketList media_packets;
+  PacketizeFrame(2, 0, &augmented_media_packets, &media_packets);
+  std::list<ForwardErrorCorrection::Packet*> 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);

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<uint8_t>();
     auto raw_packet = fuzz_data.ReadByteArray(packet_length);
 
-    RtpPacket parsed_packet;
+    RtpPacketReceived parsed_packet;
     if (!parsed_packet.Parse(raw_packet))
       continue;