From ee13e8919c20de5860a510e91fac71fd5a7e9b8d Mon Sep 17 00:00:00 2001 From: philipel Date: Wed, 2 Aug 2017 02:07:48 -0700 Subject: [PATCH] Fix off-by-one bugs in video_coding::PacketBuffer when the buffer is filled with a single frame. BUG=webrtc:8028 Review-Url: https://codereview.webrtc.org/2993513002 Cr-Commit-Position: refs/heads/master@{#19209} --- webrtc/modules/video_coding/frame_object.cc | 2 +- webrtc/modules/video_coding/frame_object.h | 2 +- webrtc/modules/video_coding/packet_buffer.cc | 26 +++++-- .../video_packet_buffer_unittest.cc | 72 ++++++++++++++++++- 4 files changed, 92 insertions(+), 10 deletions(-) diff --git a/webrtc/modules/video_coding/frame_object.cc b/webrtc/modules/video_coding/frame_object.cc index 4c4537cb45..663ea6f594 100644 --- a/webrtc/modules/video_coding/frame_object.cc +++ b/webrtc/modules/video_coding/frame_object.cc @@ -93,7 +93,7 @@ RtpFrameObject::RtpFrameObject(PacketBuffer* packet_buffer, frame_type_ = first_packet->frameType; } - GetBitstream(_buffer); + RTC_DCHECK(GetBitstream(_buffer)); _encodedWidth = first_packet->width; _encodedHeight = first_packet->height; diff --git a/webrtc/modules/video_coding/frame_object.h b/webrtc/modules/video_coding/frame_object.h index fb2cd33f18..0751077041 100644 --- a/webrtc/modules/video_coding/frame_object.h +++ b/webrtc/modules/video_coding/frame_object.h @@ -42,7 +42,7 @@ class FrameObject : public webrtc::VCMEncodedFrame { // been implemented. virtual bool delayed_by_retransmission() const { return 0; } - size_t size() { return _length; } + size_t size() const { return _length; } // The tuple (|picture_id|, |spatial_layer|) uniquely identifies a frame // object. For codec types that don't necessarily have picture ids they diff --git a/webrtc/modules/video_coding/packet_buffer.cc b/webrtc/modules/video_coding/packet_buffer.cc index 8b6de047ca..715a173e9c 100644 --- a/webrtc/modules/video_coding/packet_buffer.cc +++ b/webrtc/modules/video_coding/packet_buffer.cc @@ -251,14 +251,14 @@ std::vector> PacketBuffer::FindFrames( // Find the start index by searching backward until the packet with // the |frame_begin| flag is set. int start_index = index; + size_t tested_packets = 0; bool is_h264 = data_buffer_[start_index].codec == kVideoCodecH264; bool is_h264_keyframe = false; int64_t frame_timestamp = data_buffer_[start_index].timestamp; - // Since packet at |data_buffer_[index]| is already part of the frame - // we will have at most |size_ - 1| packets left to check. - for (size_t j = 0; j < size_ - 1; ++j) { + while (true) { + ++tested_packets; frame_size += data_buffer_[start_index].sizeBytes; max_nack_count = std::max(max_nack_count, data_buffer_[start_index].timesNacked); @@ -278,6 +278,9 @@ std::vector> PacketBuffer::FindFrames( } } + if (tested_packets == size_) + break; + start_index = start_index > 0 ? start_index - 1 : size_ - 1; // In the case of H264 we don't have a frame_begin bit (yes, @@ -345,19 +348,30 @@ bool PacketBuffer::GetBitstream(const RtpFrameObject& frame, 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) { + uint8_t* destination_end = destination + frame.size(); + + do { if (!sequence_buffer_[index].used || sequence_buffer_[index].seq_num != seq_num) { return false; } - const uint8_t* source = data_buffer_[index].dataPtr; + RTC_DCHECK_EQ(data_buffer_[index].seqNum, sequence_buffer_[index].seq_num); size_t length = data_buffer_[index].sizeBytes; + if (destination + length > destination_end) { + LOG(LS_WARNING) << "Frame (" << frame.picture_id << ":" + << static_cast(frame.spatial_layer) << ")" + << " bitstream buffer is not large enough."; + return false; + } + + const uint8_t* source = data_buffer_[index].dataPtr; memcpy(destination, source, length); destination += length; index = (index + 1) % size_; ++seq_num; - } + } while (index != end); + return true; } diff --git a/webrtc/modules/video_coding/video_packet_buffer_unittest.cc b/webrtc/modules/video_coding/video_packet_buffer_unittest.cc index e2537b8dc2..54a1871963 100644 --- a/webrtc/modules/video_coding/video_packet_buffer_unittest.cc +++ b/webrtc/modules/video_coding/video_packet_buffer_unittest.cc @@ -98,8 +98,8 @@ class TestPacketBuffer : public ::testing::Test, << "."; } - const int kStartSize = 16; - const int kMaxSize = 64; + static constexpr int kStartSize = 16; + static constexpr int kMaxSize = 64; Random rand_; std::unique_ptr clock_; @@ -374,10 +374,61 @@ TEST_F(TestPacketBuffer, GetBitstream) { ASSERT_EQ(1UL, frames_from_callback_.size()); CheckFrame(seq_num); + EXPECT_EQ(frames_from_callback_[seq_num]->size(), sizeof(result)); EXPECT_TRUE(frames_from_callback_[seq_num]->GetBitstream(result)); EXPECT_EQ(memcmp(result, "many bitstream, such data", sizeof(result)), 0); } +TEST_F(TestPacketBuffer, GetBitstreamOneFrameFullBuffer) { + uint8_t* data_arr[kStartSize]; + uint8_t expected[kStartSize]; + uint8_t result[kStartSize]; + + for (uint8_t i = 0; i < kStartSize; ++i) { + data_arr[i] = new uint8_t[1]; + data_arr[i][0] = i; + expected[i] = i; + } + + EXPECT_TRUE(Insert(0, kKeyFrame, kFirst, kNotLast, 1, data_arr[0])); + for (uint8_t i = 1; i < kStartSize - 1; ++i) + EXPECT_TRUE(Insert(i, kKeyFrame, kNotFirst, kNotLast, 1, data_arr[i])); + EXPECT_TRUE(Insert(kStartSize - 1, kKeyFrame, kNotFirst, kLast, 1, + data_arr[kStartSize - 1])); + + ASSERT_EQ(1UL, frames_from_callback_.size()); + CheckFrame(0); + EXPECT_EQ(frames_from_callback_[0]->size(), static_cast(kStartSize)); + EXPECT_TRUE(frames_from_callback_[0]->GetBitstream(result)); + EXPECT_EQ(memcmp(result, expected, kStartSize), 0); +} + +TEST_F(TestPacketBuffer, GetBitstreamOneFrameFullBufferH264) { + uint8_t* data_arr[kStartSize]; + uint8_t expected[kStartSize]; + uint8_t result[kStartSize]; + + for (uint8_t i = 0; i < kStartSize; ++i) { + data_arr[i] = new uint8_t[1]; + data_arr[i][0] = i; + expected[i] = i; + } + + EXPECT_TRUE(InsertH264(0, kKeyFrame, kFirst, kNotLast, 1, 1, data_arr[0])); + for (uint8_t i = 1; i < kStartSize - 1; ++i) { + EXPECT_TRUE( + InsertH264(i, kKeyFrame, kNotFirst, kNotLast, 1, 1, data_arr[i])); + } + EXPECT_TRUE(InsertH264(kStartSize - 1, kKeyFrame, kNotFirst, kLast, 1, 1, + data_arr[kStartSize - 1])); + + ASSERT_EQ(1UL, frames_from_callback_.size()); + CheckFrame(0); + EXPECT_EQ(frames_from_callback_[0]->size(), static_cast(kStartSize)); + EXPECT_TRUE(frames_from_callback_[0]->GetBitstream(result)); + EXPECT_EQ(memcmp(result, expected, kStartSize), 0); +} + TEST_F(TestPacketBuffer, GetBitstreamH264BufferPadding) { uint16_t seq_num = Rand(); uint8_t data_data[] = "some plain old data"; @@ -548,6 +599,23 @@ TEST_F(TestPacketBuffer, OneFrameFillBufferH264) { CheckFrame(0); } +TEST_F(TestPacketBuffer, CreateFramesAfterFilledBufferH264) { + InsertH264(kStartSize - 2, kKeyFrame, kFirst, kLast, 0); + ASSERT_EQ(1UL, frames_from_callback_.size()); + frames_from_callback_.clear(); + + InsertH264(kStartSize, kDeltaFrame, kFirst, kNotLast, 2000); + for (int i = 1; i < kStartSize; ++i) + InsertH264(kStartSize + i, kDeltaFrame, kNotFirst, kNotLast, 2000); + InsertH264(kStartSize + kStartSize, kDeltaFrame, kNotFirst, kLast, 2000); + ASSERT_EQ(0UL, frames_from_callback_.size()); + + InsertH264(kStartSize - 1, kKeyFrame, kFirst, kLast, 1000); + ASSERT_EQ(2UL, frames_from_callback_.size()); + CheckFrame(kStartSize - 1); + CheckFrame(kStartSize); +} + TEST_F(TestPacketBuffer, OneFrameMaxSeqNumH264) { InsertH264(65534, kKeyFrame, kFirst, kNotLast, 1000); InsertH264(65535, kKeyFrame, kNotFirst, kLast, 1000);