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 volatile the
+  // data has to be copied to its own buffer.
+  if (packet.sizeBytes) {
+    uint8_t* payload = new uint8_t[packet.sizeBytes];
+    memcpy(payload, packet.dataPtr, packet.sizeBytes);

[brandtr, 2016/09/01 12:45:13]

In this block, the copied VCMPacket essentially takes ownership of the data by
copying it. Would it make sense to make this functionality a member function of
VCMPacket, or to add an explicit copy constructor?

If the data pointed to by dataPtr is volatile, should it be locked during
copying?

[philipel, 2016/09/01 12:49:07]

We don't need to lock the data at this points.

I guess volatile is the wrong word, but If I don't copy the payload here the
data is lost. Maybe non-persistent is a better word to use, WDYT?

[brandtr, 2016/09/01 13:21:53]

"Non-persistent" sounds good.

[philipel, 2016/09/01 13:28:00]

Done.

+    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