Copy payload data when inserting packets into video_coding::PacketBuffer.

webrtc/modules/video_coding/packet_buffer.cc

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 72 matching lines @@
     last_seq_num_ = seq_num;
 
   sequence_buffer_[index].frame_begin = packet.isFirstPacket;
   sequence_buffer_[index].frame_end = packet.markerBit;
   sequence_buffer_[index].seq_num = packet.seqNum;
   sequence_buffer_[index].continuous = false;
   sequence_buffer_[index].frame_created = false;
   sequence_buffer_[index].used = true;
   data_buffer_[index] = packet;
 
+  // Since the data pointed to by |packet.dataPtr| is non-persistent the
+  // data has to be copied to its own buffer.

[stefan-webrtc, 2016/09/13 08:16:10]

Maybe add a TODO mentioning that we'd like to pass the ownership of this data
over from the socket to the packet buffer at some point. That'd avoid the
memcpy, which should be doable since AsyncUDPSocket and friends own the packet
memory at the moment.

[philipel, 2016/09/13 11:12:07]

Done.

+  if (packet.sizeBytes) {
+    uint8_t* payload = new uint8_t[packet.sizeBytes];

[stefan-webrtc, 2016/09/13 08:16:10]

Not super happy about this as we have to ensure we delete the memory at all
possible places (currently only 2 it seems, but still). Can we do something to
make this better by e.g. using unique_ptr?

[philipel, 2016/09/13 11:12:07]

Since the packet buffer have a fixed vector of packets the lifetime management
is manual. I guess using a unique_ptr could help but I'm not sure how much work
that requires.

Also, the lifetime management is fairly contained so it's not particularly
complex, so I'm pretty certain there is no leak (famous last words).

+    memcpy(payload, packet.dataPtr, packet.sizeBytes);
+    data_buffer_[index].dataPtr = payload;
+  }
+
   FindFrames(seq_num);
   return true;
 }
 
 void PacketBuffer::ClearTo(uint16_t seq_num) {
   rtc::CritScope lock(&crit_);
   size_t index = first_seq_num_ % size_;
   while (AheadOf<uint16_t>(seq_num, first_seq_num_ + 1)) {
     index = (index + 1) % size_;
-    first_seq_num_ = Add<1 << 16>(first_seq_num_, 1);
+    ++first_seq_num_;
+    delete[] data_buffer_[index].dataPtr;
+    data_buffer_[index].dataPtr = nullptr;
     sequence_buffer_[index].used = false;
   }
 }
 
 bool PacketBuffer::ExpandBufferSize() {
   if (size_ == max_size_)
     return false;
 
   size_t new_size = std::min(max_size_, 2 * size_);
   std::vector<VCMPacket> new_data_buffer(new_size);
@@ skipping 71 matching lines @@
     ++seq_num;
   }
 }
 
 void PacketBuffer::ReturnFrame(RtpFrameObject* frame) {
   rtc::CritScope lock(&crit_);
   size_t index = frame->first_seq_num() % size_;
   size_t end = (frame->last_seq_num() + 1) % size_;
   uint16_t seq_num = frame->first_seq_num();
   while (index != end) {
-    if (sequence_buffer_[index].seq_num == seq_num)
+    if (sequence_buffer_[index].seq_num == seq_num) {
+      delete[] data_buffer_[index].dataPtr;
+      data_buffer_[index].dataPtr = nullptr;
       sequence_buffer_[index].used = false;
+    }
 
     index = (index + 1) % size_;
     ++seq_num;
   }
 
   index = first_seq_num_ % size_;
   while (AheadOf<uint16_t>(last_seq_num_, first_seq_num_) &&
          !sequence_buffer_[index].used) {
     ++first_seq_num_;
     index = (index + 1) % size_;
@@ skipping 48 matching lines @@
 int PacketBuffer::Release() const {
   int count = rtc::AtomicOps::Decrement(&ref_count_);
   if (!count) {
     delete this;
   }
   return count;
 }
 
 }  // namespace video_coding
 }  // namespace webrtc