Request a new key frame if packet buffer is cleared
Bug: webrtc:10843
Change-Id: I1eab0891f3e68b7d504dc637790604a25c243856
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/147721
Commit-Queue: Johannes Kron <kron@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28735}
diff --git a/modules/video_coding/packet_buffer.cc b/modules/video_coding/packet_buffer.cc
index 0d7a828..e487f8c 100644
--- a/modules/video_coding/packet_buffer.cc
+++ b/modules/video_coding/packet_buffer.cc
@@ -82,11 +82,11 @@
first_packet_received_ = true;
} else if (AheadOf(first_seq_num_, seq_num)) {
// If we have explicitly cleared past this packet then it's old,
- // don't insert it.
+ // don't insert it, just silently ignore it.
if (is_cleared_to_first_seq_num_) {
delete[] packet->dataPtr;
packet->dataPtr = nullptr;
- return false;
+ return true;
}
first_seq_num_ = seq_num;
@@ -105,8 +105,12 @@
}
index = seq_num % size_;
- // Packet buffer is still full.
+ // Packet buffer is still full since we were unable to expand the buffer.
if (sequence_buffer_[index].used) {
+ // Clear the buffer, delete payload, and return false to signal that a
+ // new keyframe is needed.
+ RTC_LOG(LS_WARNING) << "Clear PacketBuffer and request key frame.";
+ Clear();
delete[] packet->dataPtr;
packet->dataPtr = nullptr;
return false;
@@ -224,8 +228,7 @@
bool PacketBuffer::ExpandBufferSize() {
if (size_ == max_size_) {
RTC_LOG(LS_WARNING) << "PacketBuffer is already at max size (" << max_size_
- << "), failed to increase size. Clearing PacketBuffer.";
- Clear();
+ << "), failed to increase size.";
return false;
}
diff --git a/modules/video_coding/packet_buffer.h b/modules/video_coding/packet_buffer.h
index 20e0bff..b5264bc 100644
--- a/modules/video_coding/packet_buffer.h
+++ b/modules/video_coding/packet_buffer.h
@@ -49,9 +49,10 @@
virtual ~PacketBuffer();
- // 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.
+ // Returns true unless the packet buffer is cleared, which means that a key
+ // frame request should be sent. 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/modules/video_coding/video_packet_buffer_unittest.cc b/modules/video_coding/video_packet_buffer_unittest.cc
index d5432b8..f8d0bb8 100644
--- a/modules/video_coding/video_packet_buffer_unittest.cc
+++ b/modules/video_coding/video_packet_buffer_unittest.cc
@@ -153,7 +153,7 @@
EXPECT_TRUE(Insert(seq_num + 2, kDeltaFrame, kFirst, kLast));
packet_buffer_->ClearTo(seq_num + 2);
- EXPECT_FALSE(Insert(seq_num + 2, kDeltaFrame, kFirst, kLast));
+ EXPECT_TRUE(Insert(seq_num + 2, kDeltaFrame, kFirst, kLast));
EXPECT_TRUE(Insert(seq_num + 3, kDeltaFrame, kFirst, kLast));
ASSERT_EQ(2UL, frames_from_callback_.size());
}
@@ -246,21 +246,20 @@
const uint32_t timestamp = 0xFFFFFFF0; // Large enough to cause wrap-around.
for (int i = 0; i < kNumFrames; ++i) {
- EXPECT_TRUE(Insert(seq_num + i, kKeyFrame, kFirst, kNotLast, 0, nullptr,
- timestamp + 10 * i));
+ Insert(seq_num + i, kKeyFrame, kFirst, kNotLast, 0, nullptr,
+ timestamp + 10 * i);
}
ASSERT_EQ(kNumFrames, packet_buffer_->GetUniqueFramesSeen());
// Old packets within history should not affect number of seen unique frames.
for (int i = kNumFrames - kRequiredHistoryLength; i < kNumFrames; ++i) {
- EXPECT_TRUE(Insert(seq_num + i, kKeyFrame, kFirst, kNotLast, 0, nullptr,
- timestamp + 10 * i));
+ Insert(seq_num + i, kKeyFrame, kFirst, kNotLast, 0, nullptr,
+ timestamp + 10 * i);
}
ASSERT_EQ(kNumFrames, packet_buffer_->GetUniqueFramesSeen());
// Very old packets should be treated as unique.
- EXPECT_TRUE(
- Insert(seq_num, kKeyFrame, kFirst, kNotLast, 0, nullptr, timestamp));
+ Insert(seq_num, kKeyFrame, kFirst, kNotLast, 0, nullptr, timestamp);
ASSERT_EQ(kNumFrames + 1, packet_buffer_->GetUniqueFramesSeen());
}
@@ -289,7 +288,7 @@
for (int i = 0; i < kMaxSize; ++i)
EXPECT_TRUE(Insert(seq_num + i, kKeyFrame, kFirst, kLast));
- EXPECT_TRUE(Insert(seq_num + kMaxSize + 1, kKeyFrame, kFirst, kLast));
+ EXPECT_FALSE(Insert(seq_num + kMaxSize + 1, kKeyFrame, kFirst, kLast));
}
TEST_F(TestPacketBuffer, OnePacketOneFrame) {
@@ -728,10 +727,10 @@
// Expect to free data3 upon insertion (old packet).
packet_buffer_->ClearTo(1);
- EXPECT_FALSE(Insert(1, kKeyFrame, kFirst, kNotLast, 5, data3));
+ EXPECT_TRUE(Insert(1, kKeyFrame, kFirst, kNotLast, 5, data3));
// Expect to free data4 upon insertion (packet buffer is full).
- EXPECT_TRUE(Insert(2 + kMaxSize, kKeyFrame, kFirst, kNotLast, 5, data4));
+ EXPECT_FALSE(Insert(2 + kMaxSize, kKeyFrame, kFirst, kNotLast, 5, data4));
}
TEST_F(TestPacketBuffer, ContinuousSeqNumDoubleMarkerBit) {