in PacketBuffer::Packet pass payload using smart buffer

Together with RtpDepacketizer refactoring that would reduce
number of memcpy while handling an rtp packet

Bug: webrtc:11152
Change-Id: I6f4e09c93af5e2a9314967a15eac8ced57ec712e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/161087
Reviewed-by: Philip Eliasson <philipel@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29985}

modules/video_coding/h264_sps_pps_tracker.cc

@@ -148,22 +148,16 @@ H264SpsPpsTracker::FixedBitstream H264SpsPpsTracker::CopyAndFixBitstream(
 
   // Then we copy to the new buffer.
   H264SpsPpsTracker::FixedBitstream fixed;
-  fixed.data = std::make_unique<uint8_t[]>(required_size);
+  fixed.bitstream.EnsureCapacity(required_size);
-  fixed.size = required_size;
-  uint8_t* insert_at = fixed.data.get();
 
   if (append_sps_pps) {
     // Insert SPS.
-    memcpy(insert_at, start_code_h264, sizeof(start_code_h264));
+    fixed.bitstream.AppendData(start_code_h264);
-    insert_at += sizeof(start_code_h264);
+    fixed.bitstream.AppendData(sps->second.data.get(), sps->second.size);
-    memcpy(insert_at, sps->second.data.get(), sps->second.size);
-    insert_at += sps->second.size;
 
     // Insert PPS.
-    memcpy(insert_at, start_code_h264, sizeof(start_code_h264));
+    fixed.bitstream.AppendData(start_code_h264);
-    insert_at += sizeof(start_code_h264);
+    fixed.bitstream.AppendData(pps->second.data.get(), pps->second.size);
-    memcpy(insert_at, pps->second.data.get(), pps->second.size);
-    insert_at += pps->second.size;
 
     // Update codec header to reflect the newly added SPS and PPS.
     NaluInfo sps_info;
@@ -187,8 +181,7 @@ H264SpsPpsTracker::FixedBitstream H264SpsPpsTracker::CopyAndFixBitstream(
   if (h264_header.packetization_type == kH264StapA) {
     const uint8_t* nalu_ptr = bitstream.data() + 1;
     while (nalu_ptr < bitstream.data() + bitstream.size()) {
-      memcpy(insert_at, start_code_h264, sizeof(start_code_h264));
+      fixed.bitstream.AppendData(start_code_h264);
-      insert_at += sizeof(start_code_h264);
 
       // The first two bytes describe the length of a segment.
       uint16_t segment_length = nalu_ptr[0] << 8 | nalu_ptr[1];
@@ -199,16 +192,14 @@ H264SpsPpsTracker::FixedBitstream H264SpsPpsTracker::CopyAndFixBitstream(
         return {kDrop};
       }
 
-      memcpy(insert_at, nalu_ptr, segment_length);
+      fixed.bitstream.AppendData(nalu_ptr, segment_length);
-      insert_at += segment_length;
       nalu_ptr += segment_length;
     }
   } else {
     if (h264_header.nalus_length > 0) {
-      memcpy(insert_at, start_code_h264, sizeof(start_code_h264));
+      fixed.bitstream.AppendData(start_code_h264);
-      insert_at += sizeof(start_code_h264);
     }
-    memcpy(insert_at, bitstream.data(), bitstream.size());
+    fixed.bitstream.AppendData(bitstream.data(), bitstream.size());
   }
 
   fixed.action = kInsert;

modules/video_coding/h264_sps_pps_tracker.h

@@ -19,6 +19,7 @@
 
 #include "api/array_view.h"
 #include "modules/rtp_rtcp/source/rtp_video_header.h"
+#include "rtc_base/copy_on_write_buffer.h"
 
 namespace webrtc {
 namespace video_coding {
@@ -28,8 +29,7 @@ class H264SpsPpsTracker {
   enum PacketAction { kInsert, kDrop, kRequestKeyframe };
   struct FixedBitstream {
     PacketAction action;
-    std::unique_ptr<uint8_t[]> data;
+    rtc::CopyOnWriteBuffer bitstream;
-    size_t size;
   };
 
   H264SpsPpsTracker();

modules/video_coding/h264_sps_pps_tracker_unittest.cc

@@ -32,7 +32,7 @@ const uint8_t start_code[] = {0, 0, 0, 1};
 
 rtc::ArrayView<const uint8_t> Bitstream(
     const H264SpsPpsTracker::FixedBitstream& fixed) {
-  return rtc::MakeArrayView(fixed.data.get(), fixed.size);
+  return fixed.bitstream;
 }
 
 void ExpectSpsPpsIdr(const RTPVideoHeaderH264& codec_header,

modules/video_coding/packet_buffer.cc

@@ -93,8 +93,6 @@ PacketBuffer::InsertResult PacketBuffer::InsertPacket(
     // If we have explicitly cleared past this packet then it's old,
     // don't insert it, just silently ignore it.
     if (is_cleared_to_first_seq_num_) {
-      delete[] packet->data;
-      packet->data = nullptr;
       return result;
     }
 
@@ -104,8 +102,6 @@ PacketBuffer::InsertResult PacketBuffer::InsertPacket(
   if (buffer_[index].used) {
     // Duplicate packet, just delete the payload.
     if (buffer_[index].seq_num() == packet->seq_num) {
-      delete[] packet->data;
-      packet->data = nullptr;
       return result;
     }
 
@@ -120,8 +116,6 @@ PacketBuffer::InsertResult PacketBuffer::InsertPacket(
       // new keyframe is needed.
       RTC_LOG(LS_WARNING) << "Clear PacketBuffer and request key frame.";
       Clear();
-      delete[] packet->data;
-      packet->data = nullptr;
       result.buffer_cleared = true;
       return result;
     }
@@ -136,7 +130,6 @@ PacketBuffer::InsertResult PacketBuffer::InsertPacket(
   new_entry.continuous = false;
   new_entry.used = true;
   new_entry.data = std::move(*packet);
-  packet->data = nullptr;
 
   UpdateMissingPackets(seq_num);
 
@@ -164,8 +157,7 @@ void PacketBuffer::ClearTo(uint16_t seq_num) {
   for (size_t i = 0; i < iterations; ++i) {
     size_t index = first_seq_num_ % buffer_.size();
     if (AheadOf<uint16_t>(seq_num, buffer_[index].seq_num())) {
-      delete[] buffer_[index].data.data;
+      buffer_[index].data.video_payload = {};
-      buffer_[index].data.data = nullptr;
       buffer_[index].used = false;
     }
     ++first_seq_num_;
@@ -191,8 +183,7 @@ void PacketBuffer::ClearInterval(uint16_t start_seq_num,
   for (size_t i = 0; i < iterations; ++i) {
     size_t index = seq_num % buffer_.size();
     RTC_DCHECK_EQ(buffer_[index].seq_num(), seq_num);
-    delete[] buffer_[index].data.data;
+    buffer_[index].data.video_payload = {};
-    buffer_[index].data.data = nullptr;
     buffer_[index].used = false;
 
     ++seq_num;
@@ -202,8 +193,7 @@ void PacketBuffer::ClearInterval(uint16_t start_seq_num,
 void PacketBuffer::Clear() {
   rtc::CritScope lock(&crit_);
   for (StoredPacket& entry : buffer_) {
-    delete[] entry.data.data;
+    entry.data.video_payload = {};
-    entry.data.data = nullptr;
     entry.used = false;
   }
 
@@ -439,8 +429,8 @@ std::unique_ptr<RtpFrameObject> PacketBuffer::AssembleFrame(
         std::min(min_recv_time, packet.packet_info.receive_time_ms());
     max_recv_time =
         std::max(max_recv_time, packet.packet_info.receive_time_ms());
-    frame_size += packet.size_bytes;
+    frame_size += packet.video_payload.size();
-    payloads.emplace_back(packet.data, packet.size_bytes);
+    payloads.emplace_back(packet.video_payload);
     packet_infos.push_back(packet.packet_info);
   }
 

modules/video_coding/packet_buffer.h

@@ -23,6 +23,7 @@
 #include "modules/rtp_rtcp/source/rtp_packet_received.h"
 #include "modules/rtp_rtcp/source/rtp_video_header.h"
 #include "modules/video_coding/frame_object.h"
+#include "rtc_base/copy_on_write_buffer.h"
 #include "rtc_base/critical_section.h"
 #include "rtc_base/numerics/sequence_number_util.h"
 #include "rtc_base/thread_annotations.h"
@@ -64,9 +65,7 @@ class PacketBuffer {
     int64_t ntp_time_ms = -1;
     int times_nacked = -1;
 
-    const uint8_t* data = nullptr;
+    rtc::CopyOnWriteBuffer video_payload;
-    size_t size_bytes = 0;
-
     RTPVideoHeader video_header;
     absl::optional<RtpGenericFrameDescriptor> generic_descriptor;
 

modules/video_coding/packet_buffer_unittest.cc

@@ -96,8 +96,8 @@ class PacketBufferTest : public ::testing::Test {
   explicit PacketBufferTest(std::string field_trials = "")
       : scoped_field_trials_(field_trials),
         rand_(0x7732213),
-        clock_(new SimulatedClock(0)),
+        clock_(0),
-        packet_buffer_(clock_.get(), kStartSize, kMaxSize) {}
+        packet_buffer_(&clock_, kStartSize, kMaxSize) {}
 
   uint16_t Rand() { return rand_.Rand<uint16_t>(); }
 
@@ -109,8 +109,7 @@ class PacketBufferTest : public ::testing::Test {
                                   IsKeyFrame keyframe,  // is keyframe
                                   IsFirst first,  // is first packet of frame
                                   IsLast last,    // is last packet of frame
-                                  int data_size = 0,            // size of data
+                                  rtc::ArrayView<const uint8_t> data = {},
-                                  uint8_t* data = nullptr,      // data pointer
                                   uint32_t timestamp = 123u) {  // rtp timestamp
     PacketBuffer::Packet packet;
     packet.video_header.codec = kVideoCodecGeneric;
@@ -121,15 +120,14 @@ class PacketBufferTest : public ::testing::Test {
                                          : VideoFrameType::kVideoFrameDelta;
     packet.video_header.is_first_packet_in_frame = first == kFirst;
     packet.video_header.is_last_packet_in_frame = last == kLast;
-    packet.size_bytes = data_size;
+    packet.video_payload.SetData(data.data(), data.size());
-    packet.data = data;
 
     return PacketBufferInsertResult(packet_buffer_.InsertPacket(&packet));
   }
 
   const test::ScopedFieldTrials scoped_field_trials_;
   Random rand_;
-  std::unique_ptr<SimulatedClock> clock_;
+  SimulatedClock clock_;
   PacketBuffer packet_buffer_;
 };
 
@@ -213,15 +211,15 @@ TEST_F(PacketBufferTest, NackCount) {
 
 TEST_F(PacketBufferTest, FrameSize) {
   const uint16_t seq_num = Rand();
-  uint8_t* data1 = new uint8_t[5]();
+  uint8_t data1[5] = {};
-  uint8_t* data2 = new uint8_t[5]();
+  uint8_t data2[5] = {};
-  uint8_t* data3 = new uint8_t[5]();
+  uint8_t data3[5] = {};
-  uint8_t* data4 = new uint8_t[5]();
+  uint8_t data4[5] = {};
 
-  Insert(seq_num, kKeyFrame, kFirst, kNotLast, 5, data1);
+  Insert(seq_num, kKeyFrame, kFirst, kNotLast, data1);
-  Insert(seq_num + 1, kKeyFrame, kNotFirst, kNotLast, 5, data2);
+  Insert(seq_num + 1, kKeyFrame, kNotFirst, kNotLast, data2);
-  Insert(seq_num + 2, kKeyFrame, kNotFirst, kNotLast, 5, data3);
+  Insert(seq_num + 2, kKeyFrame, kNotFirst, kNotLast, data3);
-  EXPECT_THAT(Insert(seq_num + 3, kKeyFrame, kNotFirst, kLast, 5, data4).frames,
+  EXPECT_THAT(Insert(seq_num + 3, kKeyFrame, kNotFirst, kLast, data4).frames,
               ElementsAre(Pointee(SizeIs(20))));
 }
 
@@ -377,32 +375,18 @@ TEST_F(PacketBufferTest, FramesReordered) {
 
 TEST_F(PacketBufferTest, GetBitstream) {
   // "many bitstream, such data" with null termination.
-  uint8_t many_data[] = {0x6d, 0x61, 0x6e, 0x79, 0x20};
+  uint8_t many[] = {0x6d, 0x61, 0x6e, 0x79, 0x20};
-  uint8_t bitstream_data[] = {0x62, 0x69, 0x74, 0x73, 0x74, 0x72,
+  uint8_t bitstream[] = {0x62, 0x69, 0x74, 0x73, 0x74, 0x72,
-                              0x65, 0x61, 0x6d, 0x2c, 0x20};
+                         0x65, 0x61, 0x6d, 0x2c, 0x20};
-  uint8_t such_data[] = {0x73, 0x75, 0x63, 0x68, 0x20};
+  uint8_t such[] = {0x73, 0x75, 0x63, 0x68, 0x20};
-  uint8_t data_data[] = {0x64, 0x61, 0x74, 0x61, 0x0};
+  uint8_t data[] = {0x64, 0x61, 0x74, 0x61, 0x0};
-
-  uint8_t* many = new uint8_t[sizeof(many_data)];
-  uint8_t* bitstream = new uint8_t[sizeof(bitstream_data)];
-  uint8_t* such = new uint8_t[sizeof(such_data)];
-  uint8_t* data = new uint8_t[sizeof(data_data)];
-
-  memcpy(many, many_data, sizeof(many_data));
-  memcpy(bitstream, bitstream_data, sizeof(bitstream_data));
-  memcpy(such, such_data, sizeof(such_data));
-  memcpy(data, data_data, sizeof(data_data));
 
   const uint16_t seq_num = Rand();
 
-  Insert(seq_num, kKeyFrame, kFirst, kNotLast, sizeof(many_data), many);
+  Insert(seq_num, kKeyFrame, kFirst, kNotLast, many);
-  Insert(seq_num + 1, kDeltaFrame, kNotFirst, kNotLast, sizeof(bitstream_data),
+  Insert(seq_num + 1, kDeltaFrame, kNotFirst, kNotLast, bitstream);
-         bitstream);
+  Insert(seq_num + 2, kDeltaFrame, kNotFirst, kNotLast, such);
-  Insert(seq_num + 2, kDeltaFrame, kNotFirst, kNotLast, sizeof(such_data),
+  auto frames = Insert(seq_num + 3, kDeltaFrame, kNotFirst, kLast, data).frames;
-         such);
-  auto frames = Insert(seq_num + 3, kDeltaFrame, kNotFirst, kLast,
-                       sizeof(data_data), data)
-                    .frames;
 
   ASSERT_THAT(frames, SizeIs(1));
   EXPECT_EQ(frames[0]->first_seq_num(), seq_num);
@@ -411,31 +395,27 @@ TEST_F(PacketBufferTest, GetBitstream) {
 }
 
 TEST_F(PacketBufferTest, GetBitstreamOneFrameOnePacket) {
-  uint8_t bitstream_data[] = "All the bitstream data for this frame!";
+  uint8_t bitstream[] = "All the bitstream data for this frame!";
-  uint8_t* data = new uint8_t[sizeof(bitstream_data)];
-  memcpy(data, bitstream_data, sizeof(bitstream_data));
 
-  auto frames =
+  auto frames = Insert(0, kKeyFrame, kFirst, kLast, bitstream).frames;
-      Insert(0, kKeyFrame, kFirst, kLast, sizeof(bitstream_data), data).frames;
   ASSERT_THAT(StartSeqNums(frames), ElementsAre(0));
   EXPECT_THAT(rtc::MakeArrayView(frames[0]->data(), frames[0]->size()),
-              ElementsAreArray(bitstream_data));
+              ElementsAreArray(bitstream));
 }
 
 TEST_F(PacketBufferTest, GetBitstreamOneFrameFullBuffer) {
-  uint8_t* data_arr[kStartSize];
+  uint8_t data_arr[kStartSize][1];
   uint8_t expected[kStartSize];
 
   for (uint8_t i = 0; i < kStartSize; ++i) {
-    data_arr[i] = new uint8_t[1];
     data_arr[i][0] = i;
     expected[i] = i;
   }
 
-  Insert(0, kKeyFrame, kFirst, kNotLast, 1, data_arr[0]);
+  Insert(0, kKeyFrame, kFirst, kNotLast, data_arr[0]);
   for (uint8_t i = 1; i < kStartSize - 1; ++i)
-    Insert(i, kKeyFrame, kNotFirst, kNotLast, 1, data_arr[i]);
+    Insert(i, kKeyFrame, kNotFirst, kNotLast, data_arr[i]);
-  auto frames = Insert(kStartSize - 1, kKeyFrame, kNotFirst, kLast, 1,
+  auto frames = Insert(kStartSize - 1, kKeyFrame, kNotFirst, kLast,
                        data_arr[kStartSize - 1])
                     .frames;
 
@@ -448,18 +428,12 @@ TEST_F(PacketBufferTest, GetBitstreamAv1) {
   const uint8_t data1[] = {0b01'01'0000, 0b0'0100'000, 'm', 'a', 'n', 'y', ' '};
   const uint8_t data2[] = {0b10'01'0000, 'b', 'i', 't', 's', 0};
 
-  uint8_t* new_data1 = new uint8_t[sizeof(data1)];
-  memcpy(new_data1, data1, sizeof(data1));
-  uint8_t* new_data2 = new uint8_t[sizeof(data2)];
-  memcpy(new_data2, data2, sizeof(data2));
-
   PacketBuffer::Packet packet1;
   packet1.video_header.codec = kVideoCodecAV1;
   packet1.seq_num = 13;
   packet1.video_header.is_first_packet_in_frame = true;
   packet1.video_header.is_last_packet_in_frame = false;
-  packet1.size_bytes = sizeof(data1);
+  packet1.video_payload = data1;
-  packet1.data = new_data1;
   auto frames = packet_buffer_.InsertPacket(&packet1).frames;
   EXPECT_THAT(frames, IsEmpty());
 
@@ -468,8 +442,7 @@ TEST_F(PacketBufferTest, GetBitstreamAv1) {
   packet2.seq_num = 14;
   packet2.video_header.is_first_packet_in_frame = false;
   packet2.video_header.is_last_packet_in_frame = true;
-  packet2.size_bytes = sizeof(data2);
+  packet2.video_payload = data2;
-  packet2.data = new_data2;
   frames = packet_buffer_.InsertPacket(&packet2).frames;
 
   ASSERT_THAT(frames, SizeIs(1));
@@ -485,18 +458,12 @@ TEST_F(PacketBufferTest, GetBitstreamInvalidAv1) {
   const uint8_t data1[] = {0b01'01'0000, 0b0'0100'000, 'm', 'a', 'n', 'y', ' '};
   const uint8_t data2[] = {0b00'01'0000, 'b', 'i', 't', 's', 0};
 
-  uint8_t* new_data1 = new uint8_t[sizeof(data1)];
-  memcpy(new_data1, data1, sizeof(data1));
-  uint8_t* new_data2 = new uint8_t[sizeof(data2)];
-  memcpy(new_data2, data2, sizeof(data2));
-
   PacketBuffer::Packet packet1;
   packet1.video_header.codec = kVideoCodecAV1;
   packet1.seq_num = 13;
   packet1.video_header.is_first_packet_in_frame = true;
   packet1.video_header.is_last_packet_in_frame = false;
-  packet1.size_bytes = sizeof(data1);
+  packet1.video_payload = data1;
-  packet1.data = new_data1;
   auto frames = packet_buffer_.InsertPacket(&packet1).frames;
   EXPECT_THAT(frames, IsEmpty());
 
@@ -505,8 +472,7 @@ TEST_F(PacketBufferTest, GetBitstreamInvalidAv1) {
   packet2.seq_num = 14;
   packet2.video_header.is_first_packet_in_frame = false;
   packet2.video_header.is_last_packet_in_frame = true;
-  packet2.size_bytes = sizeof(data2);
+  packet2.video_payload = data2;
-  packet2.data = new_data2;
   frames = packet_buffer_.InsertPacket(&packet2).frames;
 
   EXPECT_THAT(frames, IsEmpty());
@@ -521,22 +487,21 @@ TEST_F(PacketBufferTest, InsertPacketAfterSequenceNumberWrapAround) {
   // Loop until seq_num wraps around.
   SeqNumUnwrapper<uint16_t> unwrapper;
   while (unwrapper.Unwrap(seq_num) < std::numeric_limits<uint16_t>::max()) {
-    Insert(seq_num++, kKeyFrame, kFirst, kNotLast, 0, nullptr, timestamp);
+    Insert(seq_num++, kKeyFrame, kFirst, kNotLast, {}, timestamp);
     for (int i = 0; i < 5; ++i) {
-      Insert(seq_num++, kKeyFrame, kNotFirst, kNotLast, 0, nullptr, timestamp);
+      Insert(seq_num++, kKeyFrame, kNotFirst, kNotLast, {}, timestamp);
     }
-    Insert(seq_num++, kKeyFrame, kNotFirst, kLast, 0, nullptr, timestamp);
+    Insert(seq_num++, kKeyFrame, kNotFirst, kLast, {}, timestamp);
     timestamp += kTimestampDelta;
   }
 
   // Receive frame with overlapping sequence numbers.
-  Insert(seq_num++, kKeyFrame, kFirst, kNotLast, 0, nullptr, timestamp);
+  Insert(seq_num++, kKeyFrame, kFirst, kNotLast, {}, timestamp);
   for (int i = 0; i < 5; ++i) {
-    Insert(seq_num++, kKeyFrame, kNotFirst, kNotLast, 0, nullptr, timestamp);
+    Insert(seq_num++, kKeyFrame, kNotFirst, kNotLast, {}, timestamp);
   }
   EXPECT_THAT(
-      Insert(seq_num++, kKeyFrame, kNotFirst, kLast, 0, nullptr, timestamp)
+      Insert(seq_num++, kKeyFrame, kNotFirst, kLast, {}, timestamp).frames,
-          .frames,
       SizeIs(1));
 }
 
@@ -553,15 +518,14 @@ class PacketBufferH264Test : public PacketBufferTest {
         sps_pps_idr_is_keyframe_(sps_pps_idr_is_keyframe) {}
 
   PacketBufferInsertResult InsertH264(
-      uint16_t seq_num,         // packet sequence number
+      uint16_t seq_num,     // packet sequence number
-      IsKeyFrame keyframe,      // is keyframe
+      IsKeyFrame keyframe,  // is keyframe
-      IsFirst first,            // is first packet of frame
+      IsFirst first,        // is first packet of frame
-      IsLast last,              // is last packet of frame
+      IsLast last,          // is last packet of frame
-      uint32_t timestamp,       // rtp timestamp
+      uint32_t timestamp,   // rtp timestamp
-      int data_size = 0,        // size of data
+      rtc::ArrayView<const uint8_t> data = {},
-      uint8_t* data = nullptr,  // data pointer
+      uint32_t width = 0,     // width of frame (SPS/IDR)
-      uint32_t width = 0,       // width of frame (SPS/IDR)
+      uint32_t height = 0) {  // height of frame (SPS/IDR)
-      uint32_t height = 0) {    // height of frame (SPS/IDR)
     PacketBuffer::Packet packet;
     packet.video_header.codec = kVideoCodecH264;
     auto& h264_header =
@@ -583,22 +547,20 @@ class PacketBufferH264Test : public PacketBufferTest {
     packet.video_header.height = height;
     packet.video_header.is_first_packet_in_frame = first == kFirst;
     packet.video_header.is_last_packet_in_frame = last == kLast;
-    packet.size_bytes = data_size;
+    packet.video_payload.SetData(data.data(), data.size());
-    packet.data = data;
 
     return PacketBufferInsertResult(packet_buffer_.InsertPacket(&packet));
   }
 
   PacketBufferInsertResult InsertH264KeyFrameWithAud(
-      uint16_t seq_num,         // packet sequence number
+      uint16_t seq_num,     // packet sequence number
-      IsKeyFrame keyframe,      // is keyframe
+      IsKeyFrame keyframe,  // is keyframe
-      IsFirst first,            // is first packet of frame
+      IsFirst first,        // is first packet of frame
-      IsLast last,              // is last packet of frame
+      IsLast last,          // is last packet of frame
-      uint32_t timestamp,       // rtp timestamp
+      uint32_t timestamp,   // rtp timestamp
-      int data_size = 0,        // size of data
+      rtc::ArrayView<const uint8_t> data = {},
-      uint8_t* data = nullptr,  // data pointer
+      uint32_t width = 0,     // width of frame (SPS/IDR)
-      uint32_t width = 0,       // width of frame (SPS/IDR)
+      uint32_t height = 0) {  // height of frame (SPS/IDR)
-      uint32_t height = 0) {    // height of frame (SPS/IDR)
     PacketBuffer::Packet packet;
     packet.video_header.codec = kVideoCodecH264;
     auto& h264_header =
@@ -614,12 +576,10 @@ class PacketBufferH264Test : public PacketBufferTest {
     h264_header.nalus_length = 1;
     packet.video_header.is_first_packet_in_frame = true;
     packet.video_header.is_last_packet_in_frame = false;
-    packet.size_bytes = 0;
-    packet.data = nullptr;
     IgnoreResult(packet_buffer_.InsertPacket(&packet));
     // insert IDR
-    return InsertH264(seq_num + 1, keyframe, kNotFirst, last, timestamp,
+    return InsertH264(seq_num + 1, keyframe, kNotFirst, last, timestamp, data,
-                      data_size, data, width, height);
+                      width, height);
   }
 
   const bool sps_pps_idr_is_keyframe_;
@@ -648,21 +608,20 @@ TEST_P(PacketBufferH264ParameterizedTest, DontRemoveMissingPacketOnClearTo) {
 }
 
 TEST_P(PacketBufferH264ParameterizedTest, GetBitstreamOneFrameFullBuffer) {
-  uint8_t* data_arr[kStartSize];
+  uint8_t data_arr[kStartSize][1];
   uint8_t expected[kStartSize];
 
   for (uint8_t i = 0; i < kStartSize; ++i) {
-    data_arr[i] = new uint8_t[1];
     data_arr[i][0] = i;
     expected[i] = i;
   }
 
-  InsertH264(0, kKeyFrame, kFirst, kNotLast, 1, 1, data_arr[0]);
+  InsertH264(0, kKeyFrame, kFirst, kNotLast, 1, data_arr[0]);
   for (uint8_t i = 1; i < kStartSize - 1; ++i) {
-    InsertH264(i, kKeyFrame, kNotFirst, kNotLast, 1, 1, data_arr[i]);
+    InsertH264(i, kKeyFrame, kNotFirst, kNotLast, 1, data_arr[i]);
   }
 
-  auto frames = InsertH264(kStartSize - 1, kKeyFrame, kNotFirst, kLast, 1, 1,
+  auto frames = InsertH264(kStartSize - 1, kKeyFrame, kNotFirst, kLast, 1,
                            data_arr[kStartSize - 1])
                     .frames;
   ASSERT_THAT(StartSeqNums(frames), ElementsAre(0));
@@ -672,9 +631,7 @@ TEST_P(PacketBufferH264ParameterizedTest, GetBitstreamOneFrameFullBuffer) {
 
 TEST_P(PacketBufferH264ParameterizedTest, GetBitstreamBufferPadding) {
   uint16_t seq_num = Rand();
-  uint8_t data_data[] = "some plain old data";
+  uint8_t data[] = "some plain old data";
-  uint8_t* data = new uint8_t[sizeof(data_data)];
-  memcpy(data, data_data, sizeof(data_data));
 
   PacketBuffer::Packet packet;
   auto& h264_header =
@@ -684,62 +641,56 @@ TEST_P(PacketBufferH264ParameterizedTest, GetBitstreamBufferPadding) {
   h264_header.packetization_type = kH264SingleNalu;
   packet.seq_num = seq_num;
   packet.video_header.codec = kVideoCodecH264;
-  packet.data = data;
+  packet.video_payload = data;
-  packet.size_bytes = sizeof(data_data);
   packet.video_header.is_first_packet_in_frame = true;
   packet.video_header.is_last_packet_in_frame = true;
   auto frames = packet_buffer_.InsertPacket(&packet).frames;
 
   ASSERT_THAT(frames, SizeIs(1));
   EXPECT_EQ(frames[0]->first_seq_num(), seq_num);
-  EXPECT_EQ(frames[0]->EncodedImage().size(), sizeof(data_data));
+  EXPECT_EQ(frames[0]->EncodedImage().size(), sizeof(data));
-  EXPECT_EQ(frames[0]->EncodedImage().capacity(), sizeof(data_data));
+  EXPECT_EQ(frames[0]->EncodedImage().capacity(), sizeof(data));
   EXPECT_THAT(rtc::MakeArrayView(frames[0]->data(), frames[0]->size()),
-              ElementsAreArray(data_data));
+              ElementsAreArray(data));
 }
 
 TEST_P(PacketBufferH264ParameterizedTest, FrameResolution) {
   uint16_t seq_num = 100;
-  uint8_t data_data[] = "some plain old data";
+  uint8_t data[] = "some plain old data";
-  uint8_t* data = new uint8_t[sizeof(data_data)];
-  memcpy(data, data_data, sizeof(data_data));
   uint32_t width = 640;
   uint32_t height = 360;
   uint32_t timestamp = 1000;
 
-  auto frames = InsertH264(seq_num, kKeyFrame, kFirst, kLast, timestamp,
+  auto frames = InsertH264(seq_num, kKeyFrame, kFirst, kLast, timestamp, data,
-                           sizeof(data_data), data, width, height)
+                           width, height)
                     .frames;
 
   ASSERT_THAT(frames, SizeIs(1));
   EXPECT_THAT(rtc::MakeArrayView(frames[0]->data(), frames[0]->size()),
-              ElementsAreArray(data_data));
+              ElementsAreArray(data));
   EXPECT_EQ(frames[0]->EncodedImage()._encodedWidth, width);
   EXPECT_EQ(frames[0]->EncodedImage()._encodedHeight, height);
 }
 
 TEST_P(PacketBufferH264ParameterizedTest, FrameResolutionNaluBeforeSPS) {
   uint16_t seq_num = 100;
-  uint8_t data_data[] = "some plain old data";
+  uint8_t data[] = "some plain old data";
-  uint8_t* data = new uint8_t[sizeof(data_data)];
-  memcpy(data, data_data, sizeof(data_data));
   uint32_t width = 640;
   uint32_t height = 360;
   uint32_t timestamp = 1000;
 
-  auto frames =
+  auto frames = InsertH264KeyFrameWithAud(seq_num, kKeyFrame, kFirst, kLast,
-      InsertH264KeyFrameWithAud(seq_num, kKeyFrame, kFirst, kLast, timestamp,
+                                          timestamp, data, width, height)
-                                sizeof(data_data), data, width, height)
+                    .frames;
-          .frames;
 
   ASSERT_THAT(StartSeqNums(frames), ElementsAre(seq_num));
 
-  EXPECT_EQ(frames[0]->EncodedImage().size(), sizeof(data_data));
+  EXPECT_EQ(frames[0]->EncodedImage().size(), sizeof(data));
-  EXPECT_EQ(frames[0]->EncodedImage().capacity(), sizeof(data_data));
+  EXPECT_EQ(frames[0]->EncodedImage().capacity(), sizeof(data));
   EXPECT_EQ(frames[0]->EncodedImage()._encodedWidth, width);
   EXPECT_EQ(frames[0]->EncodedImage()._encodedHeight, height);
   EXPECT_THAT(rtc::MakeArrayView(frames[0]->data(), frames[0]->size()),
-              ElementsAreArray(data_data));
+              ElementsAreArray(data));
 }
 
 TEST_F(PacketBufferTest, FreeSlotsOnFrameCreation) {
@@ -783,33 +734,11 @@ TEST_F(PacketBufferTest, FramesAfterClear) {
 }
 
 TEST_F(PacketBufferTest, SameFrameDifferentTimestamps) {
-  Insert(0, kKeyFrame, kFirst, kNotLast, 0, nullptr, 1000);
+  Insert(0, kKeyFrame, kFirst, kNotLast, {}, 1000);
-  EXPECT_THAT(Insert(1, kKeyFrame, kNotFirst, kLast, 0, nullptr, 1001).frames,
+  EXPECT_THAT(Insert(1, kKeyFrame, kNotFirst, kLast, {}, 1001).frames,
               IsEmpty());
 }
 
-TEST_F(PacketBufferTest, DontLeakPayloadData) {
-  // NOTE! Any eventual leak is suppose to be detected by valgrind
-  //       or any other similar tool.
-  uint8_t* data1 = new uint8_t[5];
-  uint8_t* data2 = new uint8_t[5];
-  uint8_t* data3 = new uint8_t[5];
-  uint8_t* data4 = new uint8_t[5];
-
-  // Expected to free data1 upon PacketBuffer destruction.
-  Insert(2, kKeyFrame, kFirst, kNotLast, 5, data1);
-
-  // Expect to free data2 upon insertion.
-  Insert(2, kKeyFrame, kFirst, kNotLast, 5, data2);
-
-  // Expect to free data3 upon insertion (old packet).
-  packet_buffer_.ClearTo(1);
-  Insert(1, kKeyFrame, kFirst, kNotLast, 5, data3);
-
-  // Expect to free data4 upon insertion (packet buffer is full).
-  Insert(2 + kMaxSize, kKeyFrame, kFirst, kNotLast, 5, data4);
-}
-
 TEST_F(PacketBufferTest, ContinuousSeqNumDoubleMarkerBit) {
   Insert(2, kKeyFrame, kNotFirst, kNotLast);
   Insert(1, kKeyFrame, kFirst, kLast);
@@ -825,7 +754,7 @@ TEST_F(PacketBufferTest, PacketTimestamps) {
   EXPECT_FALSE(packet_ms);
   EXPECT_FALSE(packet_keyframe_ms);
 
-  int64_t keyframe_ms = clock_->TimeInMilliseconds();
+  int64_t keyframe_ms = clock_.TimeInMilliseconds();
   Insert(100, kKeyFrame, kFirst, kLast);
   packet_ms = packet_buffer_.LastReceivedPacketMs();
   packet_keyframe_ms = packet_buffer_.LastReceivedKeyframePacketMs();
@@ -834,8 +763,8 @@ TEST_F(PacketBufferTest, PacketTimestamps) {
   EXPECT_EQ(keyframe_ms, *packet_ms);
   EXPECT_EQ(keyframe_ms, *packet_keyframe_ms);
 
-  clock_->AdvanceTimeMilliseconds(100);
+  clock_.AdvanceTimeMilliseconds(100);
-  int64_t delta_ms = clock_->TimeInMilliseconds();
+  int64_t delta_ms = clock_.TimeInMilliseconds();
   Insert(101, kDeltaFrame, kFirst, kLast);
   packet_ms = packet_buffer_.LastReceivedPacketMs();
   packet_keyframe_ms = packet_buffer_.LastReceivedKeyframePacketMs();
@@ -855,8 +784,6 @@ TEST_F(PacketBufferTest, IncomingCodecChange) {
   PacketBuffer::Packet packet;
   packet.video_header.is_first_packet_in_frame = true;
   packet.video_header.is_last_packet_in_frame = true;
-  packet.size_bytes = 0;
-  packet.data = nullptr;
 
   packet.video_header.codec = kVideoCodecVP8;
   packet.video_header.video_type_header.emplace<RTPVideoHeaderVP8>();
@@ -892,8 +819,6 @@ TEST_F(PacketBufferTest, TooManyNalusInPacket) {
   auto& h264_header =
       packet.video_header.video_type_header.emplace<RTPVideoHeaderH264>();
   h264_header.nalus_length = kMaxNalusPerPacket;
-  packet.size_bytes = 0;
-  packet.data = nullptr;
   EXPECT_THAT(packet_buffer_.InsertPacket(&packet).frames, IsEmpty());
 }
 

test/fuzzers/packet_buffer_fuzzer.cc

@@ -21,39 +21,23 @@ void FuzzOneInput(const uint8_t* data, size_t size) {
   if (size > 200000) {
     return;
   }
-  video_coding::PacketBuffer::Packet packet;
   SimulatedClock clock(0);
   video_coding::PacketBuffer packet_buffer(&clock, 8, 1024);
   test::FuzzDataHelper helper(rtc::ArrayView<const uint8_t>(data, size));
 
   while (helper.BytesLeft()) {
-    // Complex types (e.g. non-POD-like types) can't be bit-wise fuzzed with
+    video_coding::PacketBuffer::Packet packet;
-    // random data or it will put them in an invalid state. We therefore backup
+    // Fuzz POD members of the packet.
-    // their byte-patterns before the fuzzing and restore them after.
+    helper.CopyTo(&packet.marker_bit);
-    uint8_t video_header_backup[sizeof(packet.video_header)];
+    helper.CopyTo(&packet.payload_type);
-    memcpy(&video_header_backup, &packet.video_header,
+    helper.CopyTo(&packet.seq_num);
-           sizeof(packet.video_header));
+    helper.CopyTo(&packet.timestamp);
-    uint8_t generic_descriptor_backup[sizeof(packet.generic_descriptor)];
+    helper.CopyTo(&packet.ntp_time_ms);
-    memcpy(&generic_descriptor_backup, &packet.generic_descriptor,
+    helper.CopyTo(&packet.times_nacked);
-           sizeof(packet.generic_descriptor));
-    uint8_t packet_info_backup[sizeof(packet.packet_info)];
-    memcpy(&packet_info_backup, &packet.packet_info,
-           sizeof(packet.packet_info));
 
-    helper.CopyTo(&packet);
+    // Fuzz non-POD member of the packet.
-
+    packet.video_payload.SetSize(helper.ReadOrDefaultValue<uint8_t>(0));
-    memcpy(&packet.video_header, &video_header_backup,
+    // TODO(danilchap): Fuzz other non-POD members of the |packet|.
-           sizeof(packet.video_header));
-    memcpy(&packet.generic_descriptor, &generic_descriptor_backup,
-           sizeof(packet.generic_descriptor));
-    memcpy(&packet.packet_info, &packet_info_backup,
-           sizeof(packet.packet_info));
-
-    // The packet buffer owns the payload of the packet.
-    uint8_t payload_size;
-    helper.CopyTo(&payload_size);
-    packet.size_bytes = payload_size;
-    packet.data = new uint8_t[payload_size];
 
     IgnoreResult(packet_buffer.InsertPacket(&packet));
   }

video/rtp_video_stream_receiver.cc

@@ -466,16 +466,12 @@ void RtpVideoStreamReceiver::OnReceivedPayloadData(
       case video_coding::H264SpsPpsTracker::kDrop:
         return;
       case video_coding::H264SpsPpsTracker::kInsert:
-        packet.data = fixed.data.release();
+        packet.video_payload = std::move(fixed.bitstream);
-        packet.size_bytes = fixed.size;
         break;
     }
 
   } else {
-    packet.size_bytes = codec_payload.size();
+    packet.video_payload.SetData(codec_payload.data(), codec_payload.size());
-    uint8_t* data = new uint8_t[packet.size_bytes];
-    memcpy(data, codec_payload.data(), codec_payload.size());
-    packet.data = data;
   }
 
   rtcp_feedback_buffer_.SendBufferedRtcpFeedback();