Fix memory leak in video_coding::PacketBuffer::InsertPacket.
BUG=webrtc:6788
Review-Url: https://codereview.webrtc.org/2535203002
Cr-Commit-Position: refs/heads/master@{#15314}
diff --git a/webrtc/modules/video_coding/packet_buffer.cc b/webrtc/modules/video_coding/packet_buffer.cc
index c9d787b..03aeb5a 100644
--- a/webrtc/modules/video_coding/packet_buffer.cc
+++ b/webrtc/modules/video_coding/packet_buffer.cc
@@ -56,11 +56,11 @@
Clear();
}
-bool PacketBuffer::InsertPacket(const VCMPacket& packet) {
+bool PacketBuffer::InsertPacket(VCMPacket* packet) {
std::vector<std::unique_ptr<RtpFrameObject>> found_frames;
{
rtc::CritScope lock(&crit_);
- uint16_t seq_num = packet.seqNum;
+ uint16_t seq_num = packet->seqNum;
size_t index = seq_num % size_;
if (!first_packet_received_) {
@@ -70,16 +70,22 @@
} 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;
+ packet->dataPtr = nullptr;
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;
+ packet->dataPtr = nullptr;
return true;
+ }
// The packet buffer is full, try to expand the buffer.
while (ExpandBufferSize() && sequence_buffer_[seq_num % size_].used) {
@@ -87,20 +93,24 @@
index = seq_num % size_;
// Packet buffer is still full.
- if (sequence_buffer_[index].used)
+ if (sequence_buffer_[index].used) {
+ delete[] packet->dataPtr;
+ packet->dataPtr = nullptr;
return false;
+ }
}
if (AheadOf(seq_num, last_seq_num_))
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].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;
+ data_buffer_[index] = *packet;
+ packet->dataPtr = nullptr;
found_frames = FindFrames(seq_num);
}
diff --git a/webrtc/modules/video_coding/packet_buffer.h b/webrtc/modules/video_coding/packet_buffer.h
index 36b3027..da7e80f 100644
--- a/webrtc/modules/video_coding/packet_buffer.h
+++ b/webrtc/modules/video_coding/packet_buffer.h
@@ -48,9 +48,10 @@
virtual ~PacketBuffer();
- // Returns true if |packet| is inserted into the packet buffer,
- // false otherwise. Made virtual for testing.
- virtual bool InsertPacket(const VCMPacket& packet);
+ // Returns true if |packet| is inserted into the packet buffer, false
+ // otherwise. The PacketBuffer will always take ownership of the
+ // |packet.dataPtr| when this function is called. Made virtual for testing.
+ virtual bool InsertPacket(VCMPacket* packet);
void ClearTo(uint16_t seq_num);
void Clear();
diff --git a/webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc b/webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc
index 551b06b..d67e1ba 100644
--- a/webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc
+++ b/webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc
@@ -33,8 +33,8 @@
return packet_it == packets_.end() ? nullptr : &packet_it->second;
}
- bool InsertPacket(const VCMPacket& packet) override {
- packets_[packet.seqNum] = packet;
+ bool InsertPacket(VCMPacket* packet) override {
+ packets_[packet->seqNum] = *packet;
return true;
}
@@ -83,11 +83,11 @@
packet.codec = kVideoCodecGeneric;
packet.seqNum = seq_num_start;
packet.frameType = keyframe ? kVideoFrameKey : kVideoFrameDelta;
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
packet.seqNum = seq_num_end;
packet.markerBit = true;
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(new RtpFrameObject(
ref_packet_buffer_, seq_num_start, seq_num_end, 0, 0, 0));
@@ -110,12 +110,12 @@
packet.video_header.codecHeader.VP8.temporalIdx = tid;
packet.video_header.codecHeader.VP8.tl0PicIdx = tl0;
packet.video_header.codecHeader.VP8.layerSync = sync;
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
if (seq_num_start != seq_num_end) {
packet.seqNum = seq_num_end;
packet.markerBit = true;
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
}
std::unique_ptr<RtpFrameObject> frame(new RtpFrameObject(
@@ -148,13 +148,13 @@
packet.video_header.codecHeader.VP9.ss_data_available = true;
packet.video_header.codecHeader.VP9.gof = *ss;
}
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
if (seq_num_start != seq_num_end) {
packet.markerBit = true;
packet.video_header.codecHeader.VP9.ss_data_available = false;
packet.seqNum = seq_num_end;
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
}
std::unique_ptr<RtpFrameObject> frame(new RtpFrameObject(
@@ -186,12 +186,12 @@
packet.video_header.codecHeader.VP9.num_ref_pics = refs.size();
for (size_t i = 0; i < refs.size(); ++i)
packet.video_header.codecHeader.VP9.pid_diff[i] = refs[i];
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
if (seq_num_start != seq_num_end) {
packet.seqNum = seq_num_end;
packet.markerBit = true;
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
}
std::unique_ptr<RtpFrameObject> frame(new RtpFrameObject(
@@ -1270,7 +1270,7 @@
packet.video_header.codecHeader.VP9.gof = ss;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1281,7 +1281,7 @@
packet.video_header.codecHeader.VP9.picture_id = 0;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1291,7 +1291,7 @@
packet.video_header.codecHeader.VP9.picture_id = 5000;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1323,7 +1323,7 @@
packet.video_header.codecHeader.VP9.ss_data_available = true;
packet.video_header.codecHeader.VP9.gof = ss;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1333,7 +1333,7 @@
packet.video_header.codecHeader.VP9.picture_id = 1;
packet.video_header.codecHeader.VP9.tl0_pic_idx = 0;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1344,7 +1344,7 @@
packet.video_header.codecHeader.VP9.picture_id = 2;
packet.video_header.codecHeader.VP9.tl0_pic_idx = 2;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1356,7 +1356,7 @@
packet.video_header.codecHeader.VP9.picture_id = 3;
packet.video_header.codecHeader.VP9.tl0_pic_idx = 129;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1389,7 +1389,7 @@
packet.video_header.codecHeader.VP9.ss_data_available = true;
packet.video_header.codecHeader.VP9.gof = ss;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1399,7 +1399,7 @@
packet.video_header.codecHeader.VP9.picture_id = 2;
packet.video_header.codecHeader.VP9.tl0_pic_idx = 2;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1409,7 +1409,7 @@
packet.video_header.codecHeader.VP9.picture_id = 3;
packet.video_header.codecHeader.VP9.tl0_pic_idx = 2;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1419,7 +1419,7 @@
packet.video_header.codecHeader.VP9.picture_id = 4;
packet.video_header.codecHeader.VP9.tl0_pic_idx = 1;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1452,7 +1452,7 @@
packet.video_header.codecHeader.VP9.ss_data_available = true;
packet.video_header.codecHeader.VP9.gof = ss;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1462,7 +1462,7 @@
packet.video_header.codecHeader.VP9.picture_id = 0;
packet.video_header.codecHeader.VP9.tl0_pic_idx = 2;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
@@ -1472,7 +1472,7 @@
packet.video_header.codecHeader.VP9.picture_id = 3;
packet.video_header.codecHeader.VP9.tl0_pic_idx = 2;
{
- ref_packet_buffer_->InsertPacket(packet);
+ ref_packet_buffer_->InsertPacket(&packet);
std::unique_ptr<RtpFrameObject> frame(
new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0));
reference_finder_->ManageFrame(std::move(frame));
diff --git a/webrtc/modules/video_coding/video_packet_buffer_unittest.cc b/webrtc/modules/video_coding/video_packet_buffer_unittest.cc
index a937af6..1e2ef3e 100644
--- a/webrtc/modules/video_coding/video_packet_buffer_unittest.cc
+++ b/webrtc/modules/video_coding/video_packet_buffer_unittest.cc
@@ -64,7 +64,7 @@
packet.sizeBytes = data_size;
packet.dataPtr = data;
- return packet_buffer_->InsertPacket(packet);
+ return packet_buffer_->InsertPacket(&packet);
}
void CheckFrame(uint16_t first_seq_num) {
@@ -142,21 +142,21 @@
packet.markerBit = false;
packet.timesNacked = 0;
- packet_buffer_->InsertPacket(packet);
+ packet_buffer_->InsertPacket(&packet);
packet.seqNum++;
packet.isFirstPacket = false;
packet.timesNacked = 1;
- packet_buffer_->InsertPacket(packet);
+ packet_buffer_->InsertPacket(&packet);
packet.seqNum++;
packet.timesNacked = 3;
- packet_buffer_->InsertPacket(packet);
+ packet_buffer_->InsertPacket(&packet);
packet.seqNum++;
packet.markerBit = true;
packet.timesNacked = 1;
- packet_buffer_->InsertPacket(packet);
+ packet_buffer_->InsertPacket(&packet);
ASSERT_EQ(1UL, frames_from_callback_.size());
RtpFrameObject* frame = frames_from_callback_.begin()->second.get();
@@ -365,7 +365,7 @@
packet.sizeBytes = sizeof(data_data);
packet.isFirstPacket = true;
packet.markerBit = true;
- packet_buffer_->InsertPacket(packet);
+ packet_buffer_->InsertPacket(&packet);
ASSERT_EQ(1UL, frames_from_callback_.size());
EXPECT_EQ(frames_from_callback_[seq_num]->EncodedImage()._length,
@@ -438,5 +438,27 @@
CheckFrame(9057);
}
+TEST_F(TestPacketBuffer, 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.
+ EXPECT_TRUE(Insert(2, kKeyFrame, kFirst, kNotLast, 5, data1));
+
+ // Expect to free data2 upon insertion.
+ EXPECT_TRUE(Insert(2, kKeyFrame, kFirst, kNotLast, 5, data2));
+
+ // Expect to free data3 upon insertion (old packet).
+ packet_buffer_->ClearTo(1);
+ EXPECT_FALSE(Insert(1, kKeyFrame, kFirst, kNotLast, 5, data3));
+
+ // Expect to free data4 upon insertion (packet buffer is full).
+ EXPECT_FALSE(Insert(2 + kMaxSize, kKeyFrame, kFirst, kNotLast, 5, data4));
+}
+
} // namespace video_coding
} // namespace webrtc
diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc
index bc4aa37..2f27d8d 100644
--- a/webrtc/video/rtp_stream_receiver.cc
+++ b/webrtc/video/rtp_stream_receiver.cc
@@ -274,7 +274,7 @@
packet.dataPtr = data;
}
- packet_buffer_->InsertPacket(packet);
+ packet_buffer_->InsertPacket(&packet);
} else {
if (video_receiver_->IncomingPacket(payload_data, payload_size,
rtp_header_with_ntp) != 0) {