From bd3f30535c333d79806c23b7a2f7dd53d71ce689 Mon Sep 17 00:00:00 2001 From: Johannes Kron Date: Thu, 1 Aug 2019 15:45:54 +0200 Subject: [PATCH] Request a new key frame if packet buffer is cleared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:10843 Change-Id: I1eab0891f3e68b7d504dc637790604a25c243856 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/147721 Commit-Queue: Johannes Kron Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#28735} --- modules/video_coding/packet_buffer.cc | 13 ++++++---- modules/video_coding/packet_buffer.h | 7 ++--- .../video_packet_buffer_unittest.cc | 19 +++++++------- video/rtp_video_stream_receiver.cc | 4 ++- video/rtp_video_stream_receiver_unittest.cc | 26 +++++++++++++++++++ 5 files changed, 50 insertions(+), 19 deletions(-) diff --git a/modules/video_coding/packet_buffer.cc b/modules/video_coding/packet_buffer.cc index 0d7a828f5b..e487f8c1db 100644 --- a/modules/video_coding/packet_buffer.cc +++ b/modules/video_coding/packet_buffer.cc @@ -82,11 +82,11 @@ bool PacketBuffer::InsertPacket(VCMPacket* packet) { 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 @@ bool PacketBuffer::InsertPacket(VCMPacket* packet) { } 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 @@ int PacketBuffer::GetUniqueFramesSeen() const { 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 20e0bffae4..b5264bcc08 100644 --- a/modules/video_coding/packet_buffer.h +++ b/modules/video_coding/packet_buffer.h @@ -49,9 +49,10 @@ class PacketBuffer { 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 d5432b889a..f8d0bb85a5 100644 --- a/modules/video_coding/video_packet_buffer_unittest.cc +++ b/modules/video_coding/video_packet_buffer_unittest.cc @@ -153,7 +153,7 @@ TEST_F(TestPacketBuffer, InsertOldPackets) { 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 @@ TEST_F(TestPacketBuffer, HasHistoryOfUniqueFrames) { 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 @@ TEST_F(TestPacketBuffer, ExpandBufferOverflow) { 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 @@ TEST_F(TestPacketBuffer, DontLeakPayloadData) { // 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) { diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index 05d7303ce2..2fd4f50713 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -382,7 +382,9 @@ int32_t RtpVideoStreamReceiver::OnReceivedPayloadData( } rtcp_feedback_buffer_.SendBufferedRtcpFeedback(); - packet_buffer_->InsertPacket(&packet); + if (!packet_buffer_->InsertPacket(&packet)) { + RequestKeyFrame(); + } return 0; } diff --git a/video/rtp_video_stream_receiver_unittest.cc b/video/rtp_video_stream_receiver_unittest.cc index a688351bf7..cbf2efa35d 100644 --- a/video/rtp_video_stream_receiver_unittest.cc +++ b/video/rtp_video_stream_receiver_unittest.cc @@ -560,6 +560,32 @@ TEST_F(RtpVideoStreamReceiverTest, RequestKeyframeIfFirstFrameIsDelta) { data.data(), data.size(), rtp_header, video_header, absl::nullopt, false); } +TEST_F(RtpVideoStreamReceiverTest, RequestKeyframeWhenPacketBufferGetsFull) { + constexpr int kPacketBufferMaxSize = 2048; + + RTPHeader rtp_header; + RTPVideoHeader video_header; + const std::vector data({1, 2, 3, 4}); + video_header.is_first_packet_in_frame = true; + // Incomplete frames so that the packet buffer is filling up. + video_header.is_last_packet_in_frame = false; + video_header.codec = kVideoCodecGeneric; + video_header.frame_type = VideoFrameType::kVideoFrameDelta; + uint16_t start_sequence_number = 1234; + rtp_header.sequenceNumber = start_sequence_number; + while (rtp_header.sequenceNumber - start_sequence_number < + kPacketBufferMaxSize) { + rtp_video_stream_receiver_->OnReceivedPayloadData(data.data(), data.size(), + rtp_header, video_header, + absl::nullopt, false); + rtp_header.sequenceNumber += 2; + } + + EXPECT_CALL(mock_key_frame_request_sender_, RequestKeyFrame()); + rtp_video_stream_receiver_->OnReceivedPayloadData( + data.data(), data.size(), rtp_header, video_header, absl::nullopt, false); +} + TEST_F(RtpVideoStreamReceiverTest, SecondarySinksGetRtpNotifications) { rtp_video_stream_receiver_->StartReceive();