Cleanup passing rtp packet to ulpfec receiver.

Pass RtpPacket class of header and raw packet separately

Bug: None
Change-Id: Id6d107db0e3751ff3dec87321ce6f850da0ee33a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/153740
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Sam Zackrisson <saza@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29254}

modules/rtp_rtcp/include/ulpfec_receiver.h

@@ -16,6 +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"
 
 namespace webrtc {
 
@@ -43,10 +44,8 @@ class UlpfecReceiver {
   //
   // TODO(brandtr): Set |ulpfec_payload_type| during constructor call,
   // rather than as a parameter here.
-  virtual int32_t AddReceivedRedPacket(const RTPHeader& rtp_header,
-                                       const uint8_t* incoming_rtp_packet,
-                                       size_t packet_length,
-                                       uint8_t ulpfec_payload_type) = 0;
+  virtual bool AddReceivedRedPacket(const RtpPacket& rtp_packet,
+                                    uint8_t ulpfec_payload_type) = 0;
 
   // Sends the received packets to the FEC and returns all packets
   // (both original media and recovered) through the callback.

modules/rtp_rtcp/source/fec_test_helper.cc

@@ -15,6 +15,7 @@
 
 #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "modules/rtp_rtcp/source/byte_io.h"
+#include "modules/rtp_rtcp/source/rtp_packet.h"
 #include "modules/rtp_rtcp/source/rtp_utility.h"
 #include "rtc_base/checks.h"
 
@@ -180,52 +181,44 @@ std::unique_ptr<AugmentedPacket> FlexfecPacketGenerator::BuildFlexfecPacket(
 UlpfecPacketGenerator::UlpfecPacketGenerator(uint32_t ssrc)
     : AugmentedPacketGenerator(ssrc) {}
 
-std::unique_ptr<AugmentedPacket> UlpfecPacketGenerator::BuildMediaRedPacket(
+RtpPacket UlpfecPacketGenerator::BuildMediaRedPacket(
     const AugmentedPacket& packet) {
-  std::unique_ptr<AugmentedPacket> red_packet(new AugmentedPacket());
-
-  const size_t kHeaderLength = packet.header.headerLength;
-  red_packet->header = packet.header;
-  red_packet->data.SetSize(packet.data.size() + 1);
+  RtpPacket red_packet;
   // Copy RTP header.
-  memcpy(red_packet->data.data(), packet.data.cdata(), kHeaderLength);
-  SetRedHeader(red_packet->data[1] & 0x7f, kHeaderLength, red_packet.get());
-  memcpy(red_packet->data.data() + kHeaderLength + 1,
-         packet.data.cdata() + kHeaderLength,
+  const size_t kHeaderLength = packet.header.headerLength;
+  red_packet.Parse(packet.data.cdata(), kHeaderLength);
+  RTC_DCHECK_EQ(red_packet.headers_size(), kHeaderLength);
+  uint8_t* rtp_payload =
+      red_packet.AllocatePayload(packet.data.size() + 1 - kHeaderLength);
+  // Move payload type into rtp payload.
+  rtp_payload[0] = red_packet.PayloadType();
+  red_packet.SetPayloadType(kRedPayloadType);
+  // Copy the payload.
+  memcpy(rtp_payload + 1, packet.data.cdata() + kHeaderLength,
          packet.data.size() - kHeaderLength);
 
   return red_packet;
 }
 
-std::unique_ptr<AugmentedPacket> UlpfecPacketGenerator::BuildUlpfecRedPacket(
+RtpPacket 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> red_packet =
+  std::unique_ptr<AugmentedPacket> fake_packet =
       NextPacket(0, packet.data.size() + 1);
 
-  red_packet->data[1] &= ~0x80;  // Clear marker bit.
-  const size_t kHeaderLength = red_packet->header.headerLength;
-  red_packet->data.SetSize(kHeaderLength + 1 + packet.data.size());
-  SetRedHeader(kFecPayloadType, kHeaderLength, red_packet.get());
-  memcpy(red_packet->data.data() + kHeaderLength + 1, packet.data.cdata(),
-         packet.data.size());
+  RtpPacket 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());
 
   return red_packet;
 }
 
-void UlpfecPacketGenerator::SetRedHeader(uint8_t payload_type,
-                                         size_t header_length,
-                                         AugmentedPacket* red_packet) {
-  uint8_t* data = red_packet->data.data();
-  // Replace payload type.
-  data[1] &= 0x80;             // Reset.
-  data[1] += kRedPayloadType;  // Replace.
-
-  // Add RED header, f-bit always 0.
-  data[header_length] = payload_type;
-}
-
 }  // namespace fec
 }  // namespace test
 }  // namespace webrtc

modules/rtp_rtcp/source/fec_test_helper.h

@@ -105,21 +105,14 @@ class UlpfecPacketGenerator : public AugmentedPacketGenerator {
  public:
   explicit UlpfecPacketGenerator(uint32_t ssrc);
 
-  // Creates a new AugmentedPacket with the RED header added to the packet.
-  static std::unique_ptr<AugmentedPacket> BuildMediaRedPacket(
-      const AugmentedPacket& packet);
+  // Creates a new RtpPacket with the RED header added to the packet.
+  static RtpPacket BuildMediaRedPacket(const AugmentedPacket& packet);
 
-  // Creates a new AugmentedPacket with FEC payload and RED header. Does this by
+  // 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|.
-  std::unique_ptr<AugmentedPacket> BuildUlpfecRedPacket(
-      const ForwardErrorCorrection::Packet& packet);
-
- private:
-  static void SetRedHeader(uint8_t payload_type,
-                           size_t header_length,
-                           AugmentedPacket* red_packet);
+  RtpPacket BuildUlpfecRedPacket(const ForwardErrorCorrection::Packet& packet);
 };
 
 }  // namespace fec

modules/rtp_rtcp/source/ulpfec_receiver_impl.cc

@@ -10,13 +10,10 @@
 
 #include "modules/rtp_rtcp/source/ulpfec_receiver_impl.h"
 
-#include <string.h>
-
 #include <memory>
 #include <utility>
 
 #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"
@@ -77,88 +74,75 @@ FecPacketCounter UlpfecReceiverImpl::GetPacketCounter() const {
 //    block length:  10 bits Length in bytes of the corresponding data
 //        block excluding header.
 
-int32_t UlpfecReceiverImpl::AddReceivedRedPacket(
-    const RTPHeader& header,
-    const uint8_t* incoming_rtp_packet,
-    size_t packet_length,
-    uint8_t ulpfec_payload_type) {
-  if (header.ssrc != ssrc_) {
+bool UlpfecReceiverImpl::AddReceivedRedPacket(const RtpPacket& 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.";
-    return -1;
+    return false;
   }
-  if (packet_length > IP_PACKET_SIZE) {
+  if (rtp_packet.size() > IP_PACKET_SIZE) {
     RTC_LOG(LS_WARNING) << "Received RED packet with length exceeds maximum IP "
                            "packet size; dropping.";
-    return -1;
+    return false;
   }
   rtc::CritScope cs(&crit_sect_);
 
-  uint8_t red_header_length = 1;
-  size_t payload_data_length = packet_length - header.headerLength;
+  static constexpr uint8_t kRedHeaderLength = 1;
 
-  if (payload_data_length == 0) {
+  if (rtp_packet.payload_size() == 0) {
     RTC_LOG(LS_WARNING) << "Corrupt/truncated FEC packet.";
-    return -1;
+    return false;
   }
 
   // Remove RED header of incoming packet and store as a virtual RTP packet.
-  std::unique_ptr<ForwardErrorCorrection::ReceivedPacket> received_packet(
-      new ForwardErrorCorrection::ReceivedPacket());
+  auto received_packet =
+      std::make_unique<ForwardErrorCorrection::ReceivedPacket>();
   received_packet->pkt = new ForwardErrorCorrection::Packet();
 
   // Get payload type from RED header and sequence number from RTP header.
-  uint8_t payload_type = incoming_rtp_packet[header.headerLength] & 0x7f;
+  uint8_t payload_type = rtp_packet.payload()[0] & 0x7f;
   received_packet->is_fec = payload_type == ulpfec_payload_type;
-  received_packet->ssrc = header.ssrc;
-  received_packet->seq_num = header.sequenceNumber;
+  received_packet->ssrc = rtp_packet.Ssrc();
+  received_packet->seq_num = rtp_packet.SequenceNumber();
 
-  if (incoming_rtp_packet[header.headerLength] & 0x80) {
+  if (rtp_packet.payload()[0] & 0x80) {
     // f bit set in RED header, i.e. there are more than one RED header blocks.
     // WebRTC never generates multiple blocks in a RED packet for FEC.
     RTC_LOG(LS_WARNING) << "More than 1 block in RED packet is not supported.";
-    return -1;
+    return false;
   }
 
   ++packet_counter_.num_packets;
-  packet_counter_.num_bytes += packet_length;
+  packet_counter_.num_bytes += rtp_packet.size();
   if (packet_counter_.first_packet_time_ms == -1) {
     packet_counter_.first_packet_time_ms = rtc::TimeMillis();
   }
 
+  auto red_payload = rtp_packet.payload().subview(kRedHeaderLength);
   if (received_packet->is_fec) {
     ++packet_counter_.num_fec_packets;
 
     // everything behind the RED header
-    received_packet->pkt->data.SetData(
-        incoming_rtp_packet + header.headerLength + red_header_length,
-        payload_data_length - red_header_length);
-    received_packet->ssrc =
-        ByteReader<uint32_t>::ReadBigEndian(&incoming_rtp_packet[8]);
-
+    received_packet->pkt->data.SetData(red_payload.data(), red_payload.size());
   } else {
-    received_packet->pkt->data.SetSize(header.headerLength +
-                                       payload_data_length - red_header_length);
+    received_packet->pkt->data.EnsureCapacity(rtp_packet.headers_size() +
+                                              red_payload.size());
     // Copy RTP header.
-    memcpy(received_packet->pkt->data.data(), incoming_rtp_packet,
-           header.headerLength);
+    received_packet->pkt->data.SetData(rtp_packet.data(),
+                                       rtp_packet.headers_size());
     // Set payload type.
     received_packet->pkt->data[1] &= 0x80;          // Reset RED payload type.
     received_packet->pkt->data[1] += payload_type;  // Set media payload type.
     // Copy payload data.
-    if (payload_data_length > red_header_length) {
-      memcpy(received_packet->pkt->data.data() + header.headerLength,
-             incoming_rtp_packet + header.headerLength + red_header_length,
-             payload_data_length - red_header_length);
-    }
+    received_packet->pkt->data.AppendData(red_payload.data(),
+                                          red_payload.size());
   }
 
-  if (received_packet->pkt->data.size() == 0) {
-    return 0;
+  if (received_packet->pkt->data.size() > 0) {
+    received_packets_.push_back(std::move(received_packet));
   }
-
-  received_packets_.push_back(std::move(received_packet));
-  return 0;
+  return true;
 }
 
 // TODO(nisse): Drop always-zero return value.

modules/rtp_rtcp/source/ulpfec_receiver_impl.h

@@ -17,11 +17,11 @@
 #include <memory>
 #include <vector>
 
-#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"
+#include "modules/rtp_rtcp/source/rtp_packet.h"
 #include "rtc_base/critical_section.h"
 
 namespace webrtc {
@@ -33,10 +33,8 @@ class UlpfecReceiverImpl : public UlpfecReceiver {
                               rtc::ArrayView<const RtpExtension> extensions);
   ~UlpfecReceiverImpl() override;
 
-  int32_t AddReceivedRedPacket(const RTPHeader& rtp_header,
-                               const uint8_t* incoming_rtp_packet,
-                               size_t packet_length,
-                               uint8_t ulpfec_payload_type) override;
+  bool AddReceivedRedPacket(const RtpPacket& rtp_packet,
+                            uint8_t ulpfec_payload_type) override;
 
   int32_t ProcessReceivedFec() override;
 

modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc

@@ -22,7 +22,6 @@
 #include "modules/rtp_rtcp/source/forward_error_correction.h"
 #include "test/gmock.h"
 #include "test/gtest.h"
-#include "test/rtp_header_parser.h"
 
 namespace webrtc {
 
@@ -122,19 +121,13 @@ void UlpfecReceiverTest::PacketizeFrame(
 }
 
 void UlpfecReceiverTest::BuildAndAddRedMediaPacket(AugmentedPacket* packet) {
-  std::unique_ptr<AugmentedPacket> red_packet(
-      packet_generator_.BuildMediaRedPacket(*packet));
-  EXPECT_EQ(0, receiver_fec_->AddReceivedRedPacket(
-                   red_packet->header, red_packet->data.cdata(),
-                   red_packet->data.size(), kFecPayloadType));
+  RtpPacket red_packet = packet_generator_.BuildMediaRedPacket(*packet);
+  EXPECT_TRUE(receiver_fec_->AddReceivedRedPacket(red_packet, kFecPayloadType));
 }
 
 void UlpfecReceiverTest::BuildAndAddRedFecPacket(Packet* packet) {
-  std::unique_ptr<AugmentedPacket> red_packet(
-      packet_generator_.BuildUlpfecRedPacket(*packet));
-  EXPECT_EQ(0, receiver_fec_->AddReceivedRedPacket(
-                   red_packet->header, red_packet->data.cdata(),
-                   red_packet->data.size(), kFecPayloadType));
+  RtpPacket red_packet = packet_generator_.BuildUlpfecRedPacket(*packet);
+  EXPECT_TRUE(receiver_fec_->AddReceivedRedPacket(red_packet, kFecPayloadType));
 }
 
 void UlpfecReceiverTest::VerifyReconstructedMediaPacket(
@@ -177,15 +170,13 @@ void UlpfecReceiverTest::InjectGarbagePacketLength(size_t fec_garbage_offset) {
 void UlpfecReceiverTest::SurvivesMaliciousPacket(const uint8_t* data,
                                                  size_t length,
                                                  uint8_t ulpfec_payload_type) {
-  RTPHeader header;
-  std::unique_ptr<RtpHeaderParser> parser(RtpHeaderParser::CreateForTest());
-  ASSERT_TRUE(parser->Parse(data, length, &header));
-
   NullRecoveredPacketReceiver null_callback;
   std::unique_ptr<UlpfecReceiver> receiver_fec(
       UlpfecReceiver::Create(kMediaSsrc, &null_callback, {}));
 
-  receiver_fec->AddReceivedRedPacket(header, data, length, ulpfec_payload_type);
+  RtpPacket rtp_packet;
+  ASSERT_TRUE(rtp_packet.Parse(data, length));
+  receiver_fec->AddReceivedRedPacket(rtp_packet, ulpfec_payload_type);
 }
 
 TEST_F(UlpfecReceiverTest, TwoMediaOneFec) {

test/fuzzers/BUILD.gn

@@ -187,6 +187,7 @@ webrtc_fuzzer_test("ulpfec_receiver_fuzzer") {
     "ulpfec_receiver_fuzzer.cc",
   ]
   deps = [
+    ":fuzz_data_helper",
     "../../modules/rtp_rtcp",
     "../../modules/rtp_rtcp:rtp_rtcp_format",
     "../../rtc_base:rtc_base_approved",

test/fuzzers/ulpfec_receiver_fuzzer.cc

@@ -14,6 +14,7 @@
 #include "modules/rtp_rtcp/include/ulpfec_receiver.h"
 #include "modules/rtp_rtcp/source/byte_io.h"
 #include "modules/rtp_rtcp/source/rtp_packet_received.h"
+#include "test/fuzzers/fuzz_data_helper.h"
 
 namespace webrtc {
 
@@ -38,38 +39,31 @@ void FuzzOneInput(const uint8_t* data, size_t size) {
   std::unique_ptr<UlpfecReceiver> receiver(
       UlpfecReceiver::Create(ulpfec_ssrc, &callback, {}));
 
-  std::unique_ptr<uint8_t[]> packet;
-  size_t packet_length;
-  size_t i = kMinDataNeeded;
-  while (i < size) {
-    packet_length = kRtpHeaderSize + data[i++];
-    packet = std::unique_ptr<uint8_t[]>(new uint8_t[packet_length]);
-    if (i + packet_length >= size) {
-      break;
-    }
-    memcpy(packet.get(), data + i, packet_length);
-    i += packet_length;
-    // Overwrite the RTPHeader fields for the sequence number and SSRC with
+  test::FuzzDataHelper fuzz_data(rtc::MakeArrayView(data, size));
+  while (fuzz_data.CanReadBytes(kMinDataNeeded)) {
+    size_t packet_length = kRtpHeaderSize + fuzz_data.Read<uint8_t>();
+    auto raw_packet = fuzz_data.ReadByteArray(packet_length);
+
+    RtpPacket parsed_packet;
+    if (!parsed_packet.Parse(raw_packet))
+      continue;
+
+    // Overwrite the fields for the sequence number and SSRC with
     // consistent values for either a received UlpFEC packet or received media
     // packet. (We're still relying on libfuzzer to manage to generate packet
     // headers that interact together; this just ensures that we have two
     // consistent streams).
-    if (i < size && data[i++] % 2 == 0) {
+    if (fuzz_data.ReadOrDefaultValue<uint8_t>(0) % 2 == 0) {
       // Simulate UlpFEC packet.
-      ByteWriter<uint16_t>::WriteBigEndian(packet.get() + 2, ulpfec_seq_num++);
-      ByteWriter<uint32_t>::WriteBigEndian(packet.get() + 8, ulpfec_ssrc);
+      parsed_packet.SetSequenceNumber(ulpfec_seq_num++);
+      parsed_packet.SetSsrc(ulpfec_ssrc);
     } else {
       // Simulate media packet.
-      ByteWriter<uint16_t>::WriteBigEndian(packet.get() + 2, media_seq_num++);
-      ByteWriter<uint32_t>::WriteBigEndian(packet.get() + 8, media_ssrc);
-    }
-    RtpPacketReceived parsed_packet;
-    RTPHeader parsed_header;
-    if (parsed_packet.Parse(packet.get(), packet_length)) {
-      parsed_packet.GetHeader(&parsed_header);
-      receiver->AddReceivedRedPacket(parsed_header, packet.get(), packet_length,
-                                     0);
+      parsed_packet.SetSequenceNumber(media_seq_num++);
+      parsed_packet.SetSsrc(media_ssrc);
     }
+
+    receiver->AddReceivedRedPacket(parsed_packet, 0);
   }
 
   receiver->ProcessReceivedFec();

video/rtp_video_stream_receiver.cc

@@ -732,11 +732,8 @@ void RtpVideoStreamReceiver::ParseAndHandleEncapsulatingHeader(
       // packets.
       NotifyReceiverOfEmptyPacket(packet.SequenceNumber());
     }
-    RTPHeader header;
-    packet.GetHeader(&header);
-    if (ulpfec_receiver_->AddReceivedRedPacket(
-            header, packet.data(), packet.size(),
-            config_.rtp.ulpfec_payload_type) != 0) {
+    if (!ulpfec_receiver_->AddReceivedRedPacket(
+            packet, config_.rtp.ulpfec_payload_type)) {
       return;
     }
     ulpfec_receiver_->ProcessReceivedFec();