From 8d1b582f334f4ab32d37ed9fd89415fa4c73ec43 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Thu, 30 Aug 2018 11:14:05 +0200 Subject: [PATCH] Cleanup RtpPacketizerVp8 Merge SetPayloadData into constructor, Remove payload size member because now used only during construction. Remove member that should be constant Bug: None Change-Id: Ib2083439f466ad9151ce8e54fceede6cef51d955 Reviewed-on: https://webrtc-review.googlesource.com/96740 Commit-Queue: Danil Chapovalov Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#24491} --- modules/rtp_rtcp/source/rtp_format.cc | 5 +- modules/rtp_rtcp/source/rtp_format_vp8.cc | 90 +++++++------------ modules/rtp_rtcp/source/rtp_format_vp8.h | 22 ++--- .../source/rtp_format_vp8_test_helper.cc | 7 +- .../source/rtp_format_vp8_test_helper.h | 6 +- .../source/rtp_format_vp8_unittest.cc | 65 +++++++------- 6 files changed, 76 insertions(+), 119 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_format.cc b/modules/rtp_rtcp/source/rtp_format.cc index e79f174484..72beb171e5 100644 --- a/modules/rtp_rtcp/source/rtp_format.cc +++ b/modules/rtp_rtcp/source/rtp_format.cc @@ -41,10 +41,7 @@ std::unique_ptr RtpPacketizer::Create( case kVideoCodecVP8: { const auto& vp8 = absl::get(rtp_video_header.video_type_header); - auto packetizer = absl::make_unique( - vp8, limits.max_payload_len, limits.last_packet_reduction_len); - packetizer->SetPayloadData(payload.data(), payload.size(), nullptr); - return std::move(packetizer); + return absl::make_unique(payload, limits, vp8); } case kVideoCodecVP9: { const auto& vp9 = diff --git a/modules/rtp_rtcp/source/rtp_format_vp8.cc b/modules/rtp_rtcp/source/rtp_format_vp8.cc index 2996ce2f24..4a511fcaf2 100644 --- a/modules/rtp_rtcp/source/rtp_format_vp8.cc +++ b/modules/rtp_rtcp/source/rtp_format_vp8.cc @@ -22,6 +22,10 @@ namespace webrtc { namespace { + +// Length of VP8 payload descriptors' fixed part. +constexpr int kVp8FixedPayloadDescriptorSize = 1; + int ParseVP8PictureID(RTPVideoHeaderVP8* vp8, const uint8_t** data, size_t* data_length, @@ -158,31 +162,15 @@ bool ValidateHeader(const RTPVideoHeaderVP8& hdr_info) { } // namespace -RtpPacketizerVp8::RtpPacketizerVp8(const RTPVideoHeaderVP8& hdr_info, - size_t max_payload_len, - size_t last_packet_reduction_len) - : payload_data_(NULL), - payload_size_(0), - vp8_fixed_payload_descriptor_bytes_(1), - hdr_info_(hdr_info), - max_payload_len_(max_payload_len), - last_packet_reduction_len_(last_packet_reduction_len) { +RtpPacketizerVp8::RtpPacketizerVp8(rtc::ArrayView payload, + PayloadSizeLimits limits, + const RTPVideoHeaderVP8& hdr_info) + : payload_data_(payload.data()), hdr_info_(hdr_info), limits_(limits) { RTC_DCHECK(ValidateHeader(hdr_info)); + GeneratePackets(payload.size()); } -RtpPacketizerVp8::~RtpPacketizerVp8() {} - -size_t RtpPacketizerVp8::SetPayloadData( - const uint8_t* payload_data, - size_t payload_size, - const RTPFragmentationHeader* /* fragmentation */) { - payload_data_ = payload_data; - payload_size_ = payload_size; - if (GeneratePackets() < 0) { - return 0; - } - return packets_.size(); -} +RtpPacketizerVp8::~RtpPacketizerVp8() = default; size_t RtpPacketizerVp8::NumPackets() const { return packets_.size(); @@ -196,10 +184,12 @@ bool RtpPacketizerVp8::NextPacket(RtpPacketToSend* packet) { InfoStruct packet_info = packets_.front(); packets_.pop(); - uint8_t* buffer = packet->AllocatePayload( - packets_.empty() ? max_payload_len_ - last_packet_reduction_len_ - : max_payload_len_); - int bytes = WriteHeaderAndPayload(packet_info, buffer, max_payload_len_); + size_t packet_payload_len = + packets_.empty() + ? limits_.max_payload_len - limits_.last_packet_reduction_len + : limits_.max_payload_len; + uint8_t* buffer = packet->AllocatePayload(packet_payload_len); + int bytes = WriteHeaderAndPayload(packet_info, buffer, packet_payload_len); if (bytes < 0) { return false; } @@ -208,29 +198,20 @@ bool RtpPacketizerVp8::NextPacket(RtpPacketToSend* packet) { return true; } -int RtpPacketizerVp8::GeneratePackets() { - if (max_payload_len_ < vp8_fixed_payload_descriptor_bytes_ + - PayloadDescriptorExtraLength() + 1 + - last_packet_reduction_len_) { +void RtpPacketizerVp8::GeneratePackets(size_t payload_len) { + if (limits_.max_payload_len - limits_.last_packet_reduction_len < + kVp8FixedPayloadDescriptorSize + PayloadDescriptorExtraLength() + 1) { // The provided payload length is not long enough for the payload // descriptor and one payload byte in the last packet. - // Return an error. - return -1; + return; } - size_t per_packet_capacity = - max_payload_len_ - - (vp8_fixed_payload_descriptor_bytes_ + PayloadDescriptorExtraLength()); + size_t capacity = limits_.max_payload_len - (kVp8FixedPayloadDescriptorSize + + PayloadDescriptorExtraLength()); - GeneratePacketsSplitPayloadBalanced(payload_size_, per_packet_capacity); - return 0; -} - -void RtpPacketizerVp8::GeneratePacketsSplitPayloadBalanced(size_t payload_len, - size_t capacity) { // Last packet of the last partition is smaller. Pretend that it's the same // size, but we must write more payload to it. - size_t total_bytes = payload_len + last_packet_reduction_len_; + size_t total_bytes = payload_len + limits_.last_packet_reduction_len; // Integer divisions with rounding up. size_t num_packets_left = (total_bytes + capacity - 1) / capacity; size_t bytes_per_packet = total_bytes / num_packets_left; @@ -251,7 +232,7 @@ void RtpPacketizerVp8::GeneratePacketsSplitPayloadBalanced(size_t payload_len, --current_packet_bytes; } QueuePacket(payload_len - remaining_data, current_packet_bytes, - remaining_data == payload_len); + /*first_packet=*/remaining_data == payload_len); remaining_data -= current_packet_bytes; --num_packets_left; } @@ -299,19 +280,18 @@ int RtpPacketizerVp8::WriteHeaderAndPayload(const InfoStruct& packet_info, if (extension_length < 0) return -1; - memcpy(&buffer[vp8_fixed_payload_descriptor_bytes_ + extension_length], + memcpy(&buffer[kVp8FixedPayloadDescriptorSize + extension_length], &payload_data_[packet_info.payload_start_pos], packet_info.size); // Return total length of written data. - return packet_info.size + vp8_fixed_payload_descriptor_bytes_ + - extension_length; + return packet_info.size + kVp8FixedPayloadDescriptorSize + extension_length; } int RtpPacketizerVp8::WriteExtensionFields(uint8_t* buffer, size_t buffer_length) const { size_t extension_length = 0; if (XFieldPresent()) { - uint8_t* x_field = buffer + vp8_fixed_payload_descriptor_bytes_; + uint8_t* x_field = buffer + kVp8FixedPayloadDescriptorSize; *x_field = 0; extension_length = 1; // One octet for the X field. if (PictureIdPresent()) { @@ -343,10 +323,10 @@ int RtpPacketizerVp8::WritePictureIDFields(uint8_t* x_field, size_t* extension_length) const { *x_field |= kIBit; RTC_DCHECK_GE(buffer_length, - vp8_fixed_payload_descriptor_bytes_ + *extension_length); + kVp8FixedPayloadDescriptorSize + *extension_length); const int pic_id_length = WritePictureID( - buffer + vp8_fixed_payload_descriptor_bytes_ + *extension_length, - buffer_length - vp8_fixed_payload_descriptor_bytes_ - *extension_length); + buffer + kVp8FixedPayloadDescriptorSize + *extension_length, + buffer_length - kVp8FixedPayloadDescriptorSize - *extension_length); if (pic_id_length < 0) return -1; *extension_length += pic_id_length; @@ -372,12 +352,11 @@ int RtpPacketizerVp8::WriteTl0PicIdxFields(uint8_t* x_field, uint8_t* buffer, size_t buffer_length, size_t* extension_length) const { - if (buffer_length < - vp8_fixed_payload_descriptor_bytes_ + *extension_length + 1) { + if (buffer_length < kVp8FixedPayloadDescriptorSize + *extension_length + 1) { return -1; } *x_field |= kLBit; - buffer[vp8_fixed_payload_descriptor_bytes_ + *extension_length] = + buffer[kVp8FixedPayloadDescriptorSize + *extension_length] = hdr_info_.tl0PicIdx; ++*extension_length; return 0; @@ -387,12 +366,11 @@ int RtpPacketizerVp8::WriteTIDAndKeyIdxFields(uint8_t* x_field, uint8_t* buffer, size_t buffer_length, size_t* extension_length) const { - if (buffer_length < - vp8_fixed_payload_descriptor_bytes_ + *extension_length + 1) { + if (buffer_length < kVp8FixedPayloadDescriptorSize + *extension_length + 1) { return -1; } uint8_t* data_field = - &buffer[vp8_fixed_payload_descriptor_bytes_ + *extension_length]; + &buffer[kVp8FixedPayloadDescriptorSize + *extension_length]; *data_field = 0; if (TIDFieldPresent()) { *x_field |= kTBit; diff --git a/modules/rtp_rtcp/source/rtp_format_vp8.h b/modules/rtp_rtcp/source/rtp_format_vp8.h index e5579348fa..dd667626d1 100644 --- a/modules/rtp_rtcp/source/rtp_format_vp8.h +++ b/modules/rtp_rtcp/source/rtp_format_vp8.h @@ -40,16 +40,12 @@ class RtpPacketizerVp8 : public RtpPacketizer { public: // Initialize with payload from encoder. // The payload_data must be exactly one encoded VP8 frame. - RtpPacketizerVp8(const RTPVideoHeaderVP8& hdr_info, - size_t max_payload_len, - size_t last_packet_reduction_len); + RtpPacketizerVp8(rtc::ArrayView payload, + PayloadSizeLimits limits, + const RTPVideoHeaderVP8& hdr_info); ~RtpPacketizerVp8() override; - size_t SetPayloadData(const uint8_t* payload_data, - size_t payload_size, - const RTPFragmentationHeader* fragmentation); - size_t NumPackets() const override; // Get the next payload with VP8 payload header. @@ -77,11 +73,7 @@ class RtpPacketizerVp8 : public RtpPacketizer { static const int kYBit = 0x20; // Calculate all packet sizes and load to packet info queue. - int GeneratePackets(); - - // Splits given part of payload to packets with a given capacity. The last - // packet should be reduced by last_packet_reduction_len_. - void GeneratePacketsSplitPayloadBalanced(size_t payload_len, size_t capacity); + void GeneratePackets(size_t payload_len); // Insert packet into packet queue. void QueuePacket(size_t start_pos, size_t packet_size, bool first_packet); @@ -141,12 +133,8 @@ class RtpPacketizerVp8 : public RtpPacketizer { bool PictureIdPresent() const { return (PictureIdLength() > 0); } const uint8_t* payload_data_; - size_t payload_size_; - const size_t vp8_fixed_payload_descriptor_bytes_; // Length of VP8 payload - // descriptors' fixed part. const RTPVideoHeaderVP8 hdr_info_; - const size_t max_payload_len_; - const size_t last_packet_reduction_len_; + const PayloadSizeLimits limits_; InfoQueue packets_; RTC_DISALLOW_COPY_AND_ASSIGN(RtpPacketizerVp8); diff --git a/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc b/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc index 8dc7ba0a4c..abc0c8972a 100644 --- a/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc +++ b/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc @@ -22,7 +22,6 @@ RtpFormatVp8TestHelper::RtpFormatVp8TestHelper(const RTPVideoHeaderVP8* hdr) : packet_(kNoExtensions), payload_data_(NULL), data_ptr_(NULL), - fragmentation_(NULL), hdr_info_(hdr), payload_start_(0), payload_size_(0), @@ -30,7 +29,6 @@ RtpFormatVp8TestHelper::RtpFormatVp8TestHelper(const RTPVideoHeaderVP8* hdr) inited_(false) {} RtpFormatVp8TestHelper::~RtpFormatVp8TestHelper() { - delete fragmentation_; delete[] payload_data_; } @@ -38,8 +36,6 @@ bool RtpFormatVp8TestHelper::Init(const size_t* partition_sizes, size_t num_partitions) { if (inited_) return false; - fragmentation_ = new RTPFragmentationHeader; - fragmentation_->VerifyAndAllocateFragmentationHeader(num_partitions); payload_size_ = 0; // Calculate sum payload size. for (size_t p = 0; p < num_partitions; ++p) { @@ -49,8 +45,6 @@ bool RtpFormatVp8TestHelper::Init(const size_t* partition_sizes, size_t j = 0; // Loop through the partitions again. for (size_t p = 0; p < num_partitions; ++p) { - fragmentation_->fragmentationLength[p] = partition_sizes[p]; - fragmentation_->fragmentationOffset[p] = j; for (size_t i = 0; i < partition_sizes[p]; ++i) { assert(j < payload_size_); payload_data_[j++] = p; // Set the payload value to the partition index. @@ -124,6 +118,7 @@ void RtpFormatVp8TestHelper::CheckHeader(bool frag_start) { payload_start_ = 1; rtc::ArrayView buffer = packet_.payload(); EXPECT_BIT_EQ(buffer[0], 6, 0); // Check reserved bit. + EXPECT_PART_ID_EQ(buffer[0], 0); // In equal size mode, PartID is always 0. if (hdr_info_->pictureId != kNoPictureId || hdr_info_->temporalIdx != kNoTemporalIdx || diff --git a/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.h b/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.h index f8d5c34f9d..7d5a09029d 100644 --- a/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.h +++ b/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.h @@ -38,9 +38,10 @@ class RtpFormatVp8TestHelper { const bool* expected_frag_start, size_t expected_num_packets); - uint8_t* payload_data() const { return payload_data_; } + rtc::ArrayView payload() const { + return rtc::ArrayView(payload_data_, payload_size_); + } size_t payload_size() const { return payload_size_; } - RTPFragmentationHeader* fragmentation() const { return fragmentation_; } size_t buffer_size() const { static constexpr size_t kVp8PayloadDescriptorMaxSize = 6; return payload_size_ + kVp8PayloadDescriptorMaxSize; @@ -59,7 +60,6 @@ class RtpFormatVp8TestHelper { RtpPacketToSend packet_; uint8_t* payload_data_; uint8_t* data_ptr_; - RTPFragmentationHeader* fragmentation_; const RTPVideoHeaderVP8* hdr_info_; int payload_start_; size_t payload_size_; diff --git a/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc b/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc index 7da320c1b5..b1096d8e14 100644 --- a/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_format_vp8_unittest.cc @@ -106,10 +106,10 @@ TEST_F(RtpPacketizerVp8Test, TestEqualSizeModeFallback) { ASSERT_TRUE(Init(kSizeVector, kNumPartitions)); hdr_info_.pictureId = 200; // > 0x7F should produce 2-byte PictureID - const size_t kMaxPayloadSize = 12; // Small enough to produce 4 packets. - RtpPacketizerVp8 packetizer(hdr_info_, kMaxPayloadSize, 0); - size_t num_packets = packetizer.SetPayloadData( - helper_->payload_data(), helper_->payload_size(), nullptr); + RtpPacketizer::PayloadSizeLimits limits; + limits.max_payload_len = 12; // Small enough to produce 4 packets. + RtpPacketizerVp8 packetizer(helper_->payload(), limits, hdr_info_); + size_t num_packets = packetizer.NumPackets(); // Expecting three full packets, and one with the remainder. const size_t kExpectedSizes[] = {11, 11, 12, 12}; @@ -132,11 +132,11 @@ TEST_F(RtpPacketizerVp8Test, TestEqualSizeWithLastPacketReduction) { ASSERT_TRUE(Init(kSizeVector, kNumPartitions)); hdr_info_.pictureId = 200; - const size_t kMaxPayloadSize = 15; // Small enough to produce 5 packets. - const size_t kLastPacketReduction = 5; - RtpPacketizerVp8 packetizer(hdr_info_, kMaxPayloadSize, kLastPacketReduction); - size_t num_packets = packetizer.SetPayloadData( - helper_->payload_data(), helper_->payload_size(), nullptr); + RtpPacketizer::PayloadSizeLimits limits; + limits.max_payload_len = 15; // Small enough to produce 5 packets. + limits.last_packet_reduction_len = 5; + RtpPacketizerVp8 packetizer(helper_->payload(), limits, hdr_info_); + size_t num_packets = packetizer.NumPackets(); // Calculated by hand. VP8 payload descriptors are 4 byte each. 5 packets is // minimum possible to fit 43 payload bytes into packets with capacity of @@ -164,10 +164,10 @@ TEST_F(RtpPacketizerVp8Test, TestNonReferenceBit) { ASSERT_TRUE(Init(kSizeVector, kNumPartitions)); hdr_info_.nonReference = true; - const size_t kMaxPayloadSize = 25; // Small enough to produce two packets. - RtpPacketizerVp8 packetizer(hdr_info_, kMaxPayloadSize, 0); - size_t num_packets = packetizer.SetPayloadData( - helper_->payload_data(), helper_->payload_size(), nullptr); + RtpPacketizer::PayloadSizeLimits limits; + limits.max_payload_len = 25; // Small enough to produce two packets. + RtpPacketizerVp8 packetizer(helper_->payload(), limits, hdr_info_); + size_t num_packets = packetizer.NumPackets(); // EqualSize mode => First packet full; other not. const size_t kExpectedSizes[] = {16, 16}; @@ -193,12 +193,11 @@ TEST_F(RtpPacketizerVp8Test, TestTl0PicIdxAndTID) { hdr_info_.tl0PicIdx = 117; hdr_info_.temporalIdx = 2; hdr_info_.layerSync = true; - // kMaxPayloadSize is only limited by allocated buffer size. - const size_t kMaxPayloadSize = helper_->buffer_size(); - RtpPacketizerVp8 packetizer(hdr_info_, kMaxPayloadSize, 0); - size_t num_packets = packetizer.SetPayloadData(helper_->payload_data(), - helper_->payload_size(), - helper_->fragmentation()); + RtpPacketizer::PayloadSizeLimits limits; + // max_payload_len is only limited by allocated buffer size. + limits.max_payload_len = helper_->buffer_size(); + RtpPacketizerVp8 packetizer(helper_->payload(), limits, hdr_info_); + size_t num_packets = packetizer.NumPackets(); // Expect one single packet of payload_size() + 4 bytes header. const size_t kExpectedSizes[1] = {helper_->payload_size() + 4}; @@ -220,12 +219,11 @@ TEST_F(RtpPacketizerVp8Test, TestKeyIdx) { ASSERT_TRUE(Init(kSizeVector, kNumPartitions)); hdr_info_.keyIdx = 17; - // kMaxPayloadSize is only limited by allocated buffer size. - const size_t kMaxPayloadSize = helper_->buffer_size(); - RtpPacketizerVp8 packetizer(hdr_info_, kMaxPayloadSize, 0); - size_t num_packets = packetizer.SetPayloadData(helper_->payload_data(), - helper_->payload_size(), - helper_->fragmentation()); + RtpPacketizer::PayloadSizeLimits limits; + // max payload len is only limited by allocated buffer size. + limits.max_payload_len = helper_->buffer_size(); + RtpPacketizerVp8 packetizer(helper_->payload(), limits, hdr_info_); + size_t num_packets = packetizer.NumPackets(); // Expect one single packet of payload_size() + 3 bytes header. const size_t kExpectedSizes[1] = {helper_->payload_size() + 3}; @@ -248,12 +246,11 @@ TEST_F(RtpPacketizerVp8Test, TestTIDAndKeyIdx) { hdr_info_.temporalIdx = 1; hdr_info_.keyIdx = 5; - // kMaxPayloadSize is only limited by allocated buffer size. - const size_t kMaxPayloadSize = helper_->buffer_size(); - RtpPacketizerVp8 packetizer(hdr_info_, kMaxPayloadSize, 0); - size_t num_packets = packetizer.SetPayloadData(helper_->payload_data(), - helper_->payload_size(), - helper_->fragmentation()); + RtpPacketizer::PayloadSizeLimits limits; + // max_payload_len is only limited by allocated buffer size. + limits.max_payload_len = helper_->buffer_size(); + RtpPacketizerVp8 packetizer(helper_->payload(), limits, hdr_info_); + size_t num_packets = packetizer.NumPackets(); // Expect one single packet of payload_size() + 3 bytes header. const size_t kExpectedSizes[1] = {helper_->payload_size() + 3}; @@ -435,8 +432,10 @@ TEST_F(RtpDepacketizerVp8Test, TestWithPacketizer) { input_header.layerSync = false; input_header.tl0PicIdx = kNoTl0PicIdx; // Disable. input_header.keyIdx = 31; - RtpPacketizerVp8 packetizer(input_header, 20, 0); - EXPECT_EQ(packetizer.SetPayloadData(data, 10, NULL), 1u); + RtpPacketizer::PayloadSizeLimits limits; + limits.max_payload_len = 20; + RtpPacketizerVp8 packetizer(data, limits, input_header); + EXPECT_EQ(packetizer.NumPackets(), 1u); ASSERT_TRUE(packetizer.NextPacket(&packet)); EXPECT_TRUE(packet.Marker());