From 629de6f7ed60fb7e7a25399548d0d386eaecb3ee Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Thu, 16 Jan 2020 18:59:01 +0100 Subject: [PATCH] Merge RtpPacket HasExtension and IsExtensionReserved functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RtpPacket doesn't keep difference between reserved and set extension. Bug: None Change-Id: I1c79f4ebd7ba20ae5da0194c3faa418050db7d8e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166340 Reviewed-by: Erik Språng Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#30316} --- modules/pacing/packet_router.cc | 2 +- modules/rtp_rtcp/source/rtp_packet.cc | 5 ----- modules/rtp_rtcp/source/rtp_packet.h | 9 --------- modules/rtp_rtcp/source/rtp_packet_unittest.cc | 8 ++++---- modules/rtp_rtcp/source/rtp_sender_egress.cc | 4 ++-- modules/rtp_rtcp/source/rtp_sender_unittest.cc | 18 +++++++++--------- 6 files changed, 16 insertions(+), 30 deletions(-) diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index 32df52583a..fa64331493 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -139,7 +139,7 @@ void PacketRouter::SendPacket(std::unique_ptr packet, rtc::CritScope cs(&modules_crit_); // With the new pacer code path, transport sequence numbers are only set here, // on the pacer thread. Therefore we don't need atomics/synchronization. - if (packet->IsExtensionReserved()) { + if (packet->HasExtension()) { packet->SetExtension((++transport_seq_) & 0xFFFF); } diff --git a/modules/rtp_rtcp/source/rtp_packet.cc b/modules/rtp_rtcp/source/rtp_packet.cc index 8ca232b5e8..56438283e4 100644 --- a/modules/rtp_rtcp/source/rtp_packet.cc +++ b/modules/rtp_rtcp/source/rtp_packet.cc @@ -618,11 +618,6 @@ rtc::ArrayView RtpPacket::AllocateExtension(ExtensionType type, } bool RtpPacket::HasExtension(ExtensionType type) const { - // TODO(webrtc:7990): Add support for empty extensions (length==0). - return !FindExtension(type).empty(); -} - -bool RtpPacket::IsExtensionReserved(ExtensionType type) const { uint8_t id = extensions_.GetId(type); if (id == ExtensionManager::kInvalidId) { // Extension not registered. diff --git a/modules/rtp_rtcp/source/rtp_packet.h b/modules/rtp_rtcp/source/rtp_packet.h index 862399f866..809af0c327 100644 --- a/modules/rtp_rtcp/source/rtp_packet.h +++ b/modules/rtp_rtcp/source/rtp_packet.h @@ -112,10 +112,6 @@ class RtpPacket { bool HasExtension() const; bool HasExtension(ExtensionType type) const; - template - bool IsExtensionReserved() const; - bool IsExtensionReserved(ExtensionType type) const; - template bool GetExtension(FirstValue, Values...) const; @@ -207,11 +203,6 @@ bool RtpPacket::HasExtension() const { return HasExtension(Extension::kId); } -template -bool RtpPacket::IsExtensionReserved() const { - return IsExtensionReserved(Extension::kId); -} - template bool RtpPacket::GetExtension(FirstValue first, Values... values) const { auto raw = FindExtension(Extension::kId); diff --git a/modules/rtp_rtcp/source/rtp_packet_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_unittest.cc index 3f9fcd1113..1bb4358c6d 100644 --- a/modules/rtp_rtcp/source/rtp_packet_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_unittest.cc @@ -996,7 +996,7 @@ TEST(RtpPacketTest, kFeedbackRequest->sequence_count); } -TEST(RtpPacketTest, IsExtensionReserved) { +TEST(RtpPacketTest, ReservedExtensionsCountedAsSetExtension) { // Register two extensions. RtpPacketToSend::ExtensionManager extensions; extensions.Register(kTransmissionOffsetExtensionId); @@ -1011,9 +1011,9 @@ TEST(RtpPacketTest, IsExtensionReserved) { // Only the extension that is both registered and reserved matches // IsExtensionReserved(). - EXPECT_FALSE(packet.IsExtensionReserved()); - EXPECT_FALSE(packet.IsExtensionReserved()); - EXPECT_TRUE(packet.IsExtensionReserved()); + EXPECT_FALSE(packet.HasExtension()); + EXPECT_FALSE(packet.HasExtension()); + EXPECT_TRUE(packet.HasExtension()); } // Tests that RtpPacket::RemoveExtension can successfully remove extensions. diff --git a/modules/rtp_rtcp/source/rtp_sender_egress.cc b/modules/rtp_rtcp/source/rtp_sender_egress.cc index b602eb6c1e..2244927291 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress.cc @@ -124,10 +124,10 @@ void RtpSenderEgress::SendPacket(RtpPacketToSend* packet, // data after rtp header may be corrupted if these packets are protected by // the FEC. int64_t diff_ms = now_ms - packet->capture_time_ms(); - if (packet->IsExtensionReserved()) { + if (packet->HasExtension()) { packet->SetExtension(kTimestampTicksPerMs * diff_ms); } - if (packet->IsExtensionReserved()) { + if (packet->HasExtension()) { packet->SetExtension( AbsoluteSendTime::MsTo24Bits(now_ms)); } diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 25b7c56abd..5ca4e70de8 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -2239,9 +2239,9 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) { EXPECT_EQ(padding_packet->Ssrc(), kRtxSsrc); EXPECT_EQ(padding_packet->payload_size(), kPayloadPacketSize + kRtxHeaderSize); - EXPECT_TRUE(padding_packet->IsExtensionReserved()); - EXPECT_TRUE(padding_packet->IsExtensionReserved()); - EXPECT_TRUE(padding_packet->IsExtensionReserved()); + EXPECT_TRUE(padding_packet->HasExtension()); + EXPECT_TRUE(padding_packet->HasExtension()); + EXPECT_TRUE(padding_packet->HasExtension()); // Verify all header extensions are received. rtp_egress()->SendPacket(padding_packet.get(), PacedPacketInfo()); @@ -2265,9 +2265,9 @@ TEST_P(RtpSenderTest, GeneratePaddingResendsOldPacketsWithRtx) { EXPECT_GT(packet->padding_size(), 0u); padding_bytes_generated += packet->padding_size(); - EXPECT_TRUE(packet->IsExtensionReserved()); - EXPECT_TRUE(packet->IsExtensionReserved()); - EXPECT_TRUE(packet->IsExtensionReserved()); + EXPECT_TRUE(packet->HasExtension()); + EXPECT_TRUE(packet->HasExtension()); + EXPECT_TRUE(packet->HasExtension()); // Verify all header extensions are received. rtp_egress()->SendPacket(packet.get(), PacedPacketInfo()); @@ -2323,9 +2323,9 @@ TEST_P(RtpSenderTest, GeneratePaddingCreatesPurePaddingWithoutRtx) { EXPECT_EQ(packet->payload_size(), 0u); EXPECT_GT(packet->padding_size(), 0u); padding_bytes_generated += packet->padding_size(); - EXPECT_TRUE(packet->IsExtensionReserved()); - EXPECT_TRUE(packet->IsExtensionReserved()); - EXPECT_TRUE(packet->IsExtensionReserved()); + EXPECT_TRUE(packet->HasExtension()); + EXPECT_TRUE(packet->HasExtension()); + EXPECT_TRUE(packet->HasExtension()); // Verify all header extensions are received. rtp_egress()->SendPacket(packet.get(), PacedPacketInfo());