Fix memory leak in video_coding::PacketBuffer::InsertPacket.

webrtc/modules/video_coding/packet_buffer.cc

Index: webrtc/modules/video_coding/packet_buffer.cc
diff --git a/webrtc/modules/video_coding/packet_buffer.cc b/webrtc/modules/video_coding/packet_buffer.cc
index c9d787b6e66184b0f1c0690227d5743397d5c54b..fe211e0044b4e718242e9fa365e6331a8dcb41dd 100644
--- a/webrtc/modules/video_coding/packet_buffer.cc
+++ b/webrtc/modules/video_coding/packet_buffer.cc
@@ -70,16 +70,20 @@ bool PacketBuffer::InsertPacket(const VCMPacket& packet) {
     } else if (AheadOf(first_seq_num_, seq_num)) {
       // If we have explicitly cleared past this packet then it's old,
       // don't insert it.
-      if (is_cleared_to_first_seq_num_)
+      if (is_cleared_to_first_seq_num_) {
+        delete[] packet.dataPtr;

[nisse-webrtc, 2016/11/29 11:59:30]

Ownership looks a bit confused here. Argument is a const ref to a VCMPacket. And
this cl deallocates the data belonging to that packet. This isn't formally
violating the constness (if it did, the compiler would have complained), but
it's not something that I'd expect to happen to a const-ref argument.

I think there are three sane ways to handle ownership for InsertPacket, and the
choice should be clearly documented:

1. Caller always owns the data.

2. Callee (i.e., InsertFrame) always takes ownership.

3. Callee sometimes takes ownership, and the decision is reflected in the return
value. When the callee isn't retaining the data, it's the caller's
responsibility to deallocate it.

And it would be even better to convert VCMPacket to use a unique_ptr for the
data.

[philipel, 2016/11/29 12:28:45]

You are absolutely right, so I went for option 2 (PacketBuffer always take
ownership).

         return false;
+      }
 
       first_seq_num_ = seq_num;
     }
 
     if (sequence_buffer_[index].used) {
-      // Duplicate packet, do nothing.
-      if (data_buffer_[index].seqNum == packet.seqNum)
+      // Duplicate packet, just delete the payload.
+      if (data_buffer_[index].seqNum == packet.seqNum) {
+        delete[] packet.dataPtr;
         return true;
+      }
 
       // The packet buffer is full, try to expand the buffer.
       while (ExpandBufferSize() && sequence_buffer_[seq_num % size_].used) {
@@ -87,8 +91,10 @@ bool PacketBuffer::InsertPacket(const VCMPacket& packet) {
       index = seq_num % size_;
 
       // Packet buffer is still full.
-      if (sequence_buffer_[index].used)
+      if (sequence_buffer_[index].used) {
+        delete[] packet.dataPtr;
         return false;
+      }
     }
 
     if (AheadOf(seq_num, last_seq_num_))