From f7fcaf0885e74f05f3dd45e14ea024ded8b1e2a0 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Wed, 10 Oct 2018 14:56:01 +0200 Subject: [PATCH] Use zero octets for rtp packet padding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC3550 Section 4 mention "Octets designated as padding have the value zero." Bug: None Change-Id: Ife4c6226143c79ad7d152bc6099ba1d81f5492dd Reviewed-on: https://webrtc-review.googlesource.com/c/103983 Commit-Queue: Danil Chapovalov Reviewed-by: Björn Terelius Cr-Commit-Position: refs/heads/master@{#25109} --- .../rtc_event_log_unittest_helper.cc | 2 +- .../source/receive_statistics_unittest.cc | 5 +-- modules/rtp_rtcp/source/rtp_packet.cc | 15 +++---- modules/rtp_rtcp/source/rtp_packet.h | 8 +++- .../rtp_rtcp/source/rtp_packet_unittest.cc | 45 ++++++++++++++++--- modules/rtp_rtcp/source/rtp_sender.cc | 2 +- 6 files changed, 56 insertions(+), 21 deletions(-) diff --git a/logging/rtc_event_log/rtc_event_log_unittest_helper.cc b/logging/rtc_event_log/rtc_event_log_unittest_helper.cc index 9f6240cbe1..1a405ec3d9 100644 --- a/logging/rtc_event_log/rtc_event_log_unittest_helper.cc +++ b/logging/rtc_event_log/rtc_event_log_unittest_helper.cc @@ -299,7 +299,7 @@ void EventGenerator::RandomizeRtpPacket( for (size_t i = 0; i < payload_size; i++) { payload[i] = prng_.Rand(); } - RTC_CHECK(rtp_packet->SetPadding(padding_size, &prng_)); + RTC_CHECK(rtp_packet->SetPadding(padding_size)); } std::unique_ptr EventGenerator::NewRtpPacketIncoming( diff --git a/modules/rtp_rtcp/source/receive_statistics_unittest.cc b/modules/rtp_rtcp/source/receive_statistics_unittest.cc index 18448cb77e..578d81fc3d 100644 --- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc +++ b/modules/rtp_rtcp/source/receive_statistics_unittest.cc @@ -48,10 +48,7 @@ RtpPacketReceived CreateRtpPacket(uint32_t ssrc, packet.SetCsrcs(csrcs); } packet.SetPayloadSize(payload_size); - if (padding_size > 0) { - Random random(17); - packet.SetPadding(padding_size, &random); - } + packet.SetPadding(padding_size); return packet; } diff --git a/modules/rtp_rtcp/source/rtp_packet.cc b/modules/rtp_rtcp/source/rtp_packet.cc index 0b9ed80af5..8cb8fd8cee 100644 --- a/modules/rtp_rtcp/source/rtp_packet.cc +++ b/modules/rtp_rtcp/source/rtp_packet.cc @@ -19,7 +19,6 @@ #include "rtc_base/checks.h" #include "rtc_base/logging.h" #include "rtc_base/numerics/safe_conversions.h" -#include "rtc_base/random.h" namespace webrtc { namespace { @@ -359,22 +358,20 @@ uint8_t* RtpPacket::SetPayloadSize(size_t size_bytes) { return WriteAt(payload_offset_); } -bool RtpPacket::SetPadding(uint8_t size_bytes, Random* random) { - RTC_DCHECK(random); - if (payload_offset_ + payload_size_ + size_bytes > capacity()) { - RTC_LOG(LS_WARNING) << "Cannot set padding size " << size_bytes << ", only " +bool RtpPacket::SetPadding(size_t padding_bytes) { + if (payload_offset_ + payload_size_ + padding_bytes > capacity()) { + RTC_LOG(LS_WARNING) << "Cannot set padding size " << padding_bytes + << ", only " << (capacity() - payload_offset_ - payload_size_) << " bytes left in buffer."; return false; } - padding_size_ = size_bytes; + padding_size_ = rtc::dchecked_cast(padding_bytes); buffer_.SetSize(payload_offset_ + payload_size_ + padding_size_); if (padding_size_ > 0) { size_t padding_offset = payload_offset_ + payload_size_; size_t padding_end = padding_offset + padding_size_; - for (size_t offset = padding_offset; offset < padding_end - 1; ++offset) { - WriteAt(offset, random->Rand()); - } + memset(WriteAt(padding_offset), 0, padding_size_ - 1); WriteAt(padding_end - 1, padding_size_); WriteAt(0, data()[0] | 0x20); // Set padding bit. } else { diff --git a/modules/rtp_rtcp/source/rtp_packet.h b/modules/rtp_rtcp/source/rtp_packet.h index 15c3865b35..3d7d90c13a 100644 --- a/modules/rtp_rtcp/source/rtp_packet.h +++ b/modules/rtp_rtcp/source/rtp_packet.h @@ -16,6 +16,7 @@ #include "modules/rtp_rtcp/include/rtp_header_extension_map.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "rtc_base/copyonwritebuffer.h" +#include "rtc_base/deprecation.h" namespace webrtc { class Random; @@ -111,7 +112,12 @@ class RtpPacket { uint8_t* SetPayloadSize(size_t size_bytes); // Same as SetPayloadSize but doesn't guarantee to keep current payload. uint8_t* AllocatePayload(size_t size_bytes); - bool SetPadding(uint8_t size_bytes, Random* random); + RTC_DEPRECATED + bool SetPadding(uint8_t size_bytes, Random* random) { + return SetPadding(size_bytes); + } + + bool SetPadding(size_t padding_size); private: struct ExtensionInfo { diff --git a/modules/rtp_rtcp/source/rtp_packet_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_unittest.cc index 125000d2dc..648ddc33c6 100644 --- a/modules/rtp_rtcp/source/rtp_packet_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_unittest.cc @@ -19,6 +19,7 @@ namespace webrtc { namespace { +using ::testing::Each; using ::testing::ElementsAre; using ::testing::ElementsAreArray; using ::testing::IsEmpty; @@ -367,11 +368,10 @@ TEST(RtpPacketTest, CreatePurePadding) { packet.SetSequenceNumber(kSeqNum); packet.SetTimestamp(kTimestamp); packet.SetSsrc(kSsrc); - Random random(0x123456789); EXPECT_LT(packet.size(), packet.capacity()); - EXPECT_FALSE(packet.SetPadding(kPaddingSize + 1, &random)); - EXPECT_TRUE(packet.SetPadding(kPaddingSize, &random)); + EXPECT_FALSE(packet.SetPadding(kPaddingSize + 1)); + EXPECT_TRUE(packet.SetPadding(kPaddingSize)); EXPECT_EQ(packet.size(), packet.capacity()); } @@ -383,13 +383,48 @@ TEST(RtpPacketTest, CreateUnalignedPadding) { packet.SetTimestamp(kTimestamp); packet.SetSsrc(kSsrc); packet.SetPayloadSize(kPayloadSize); - Random r(0x123456789); EXPECT_LT(packet.size(), packet.capacity()); - EXPECT_TRUE(packet.SetPadding(kMaxPaddingSize, &r)); + EXPECT_TRUE(packet.SetPadding(kMaxPaddingSize)); EXPECT_EQ(packet.size(), packet.capacity()); } +TEST(RtpPacketTest, WritesPaddingSizeToLastByte) { + const size_t kPaddingSize = 5; + RtpPacket packet; + + EXPECT_TRUE(packet.SetPadding(kPaddingSize)); + EXPECT_EQ(packet.data()[packet.size() - 1], kPaddingSize); +} + +TEST(RtpPacketTest, UsesZerosForPadding) { + const size_t kPaddingSize = 5; + RtpPacket packet; + + EXPECT_TRUE(packet.SetPadding(kPaddingSize)); + EXPECT_THAT(rtc::MakeArrayView(packet.data() + 12, kPaddingSize - 1), + Each(0)); +} + +TEST(RtpPacketTest, CreateOneBytePadding) { + size_t kPayloadSize = 123; + RtpPacket packet(nullptr, 12 + kPayloadSize + 1); + packet.SetPayloadSize(kPayloadSize); + + EXPECT_TRUE(packet.SetPadding(1)); + + EXPECT_EQ(packet.size(), 12 + kPayloadSize + 1); + EXPECT_EQ(packet.padding_size(), 1u); +} + +TEST(RtpPacketTest, FailsToAddPaddingWithoutCapacity) { + size_t kPayloadSize = 123; + RtpPacket packet(nullptr, 12 + kPayloadSize); + packet.SetPayloadSize(kPayloadSize); + + EXPECT_FALSE(packet.SetPadding(1)); +} + TEST(RtpPacketTest, ParseMinimum) { RtpPacketReceived packet; EXPECT_TRUE(packet.Parse(kMinimumPacket, sizeof(kMinimumPacket))); diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 31887bdafd..1678a0385d 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -622,7 +622,7 @@ size_t RTPSender::SendPadData(size_t bytes, has_transport_seq_num || force_part_of_allocation_; options.included_in_feedback = has_transport_seq_num; } - padding_packet.SetPadding(padding_bytes_in_packet, &random_); + padding_packet.SetPadding(padding_bytes_in_packet); if (has_transport_seq_num) { AddPacketToTransportFeedback(options.packet_id, padding_packet, pacing_info);