From 7d5418233d794cc42e64ac44b1fd388a45e811de Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Thu, 17 Jun 2021 15:27:12 +0200 Subject: [PATCH] Avoid assembling complicated but unused video rtp header extensions Bug: chromium:1219407 Change-Id: I017de10813a1e80f4af0ba55d8d1aa73077dd131 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/222615 Reviewed-by: Philip Eliasson Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#34326} --- modules/rtp_rtcp/source/rtp_packet.h | 10 +++++++ .../rtp_rtcp/source/rtp_packet_unittest.cc | 29 +++++++++++++++++++ modules/rtp_rtcp/source/rtp_sender_video.cc | 9 ++++-- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_packet.h b/modules/rtp_rtcp/source/rtp_packet.h index 1c31f92ce9..81ca02c851 100644 --- a/modules/rtp_rtcp/source/rtp_packet.h +++ b/modules/rtp_rtcp/source/rtp_packet.h @@ -114,6 +114,11 @@ class RtpPacket { bool HasExtension() const; bool HasExtension(ExtensionType type) const; + // Returns whether there is an associated id for the extension and thus it is + // possible to set the extension. + template + bool IsRegistered() const; + template bool GetExtension(FirstValue, Values...) const; @@ -207,6 +212,11 @@ bool RtpPacket::HasExtension() const { return HasExtension(Extension::kId); } +template +bool RtpPacket::IsRegistered() const { + return extensions_.IsRegistered(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 f7f21af41d..533e509d33 100644 --- a/modules/rtp_rtcp/source/rtp_packet_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_unittest.cc @@ -354,6 +354,35 @@ TEST(RtpPacketTest, CreateWithMaxSizeHeaderExtension) { EXPECT_EQ(read, kValue); } +TEST(RtpPacketTest, SetsRegisteredExtension) { + RtpPacketToSend::ExtensionManager extensions; + extensions.Register(kTransmissionOffsetExtensionId); + RtpPacketToSend packet(&extensions); + + EXPECT_TRUE(packet.IsRegistered()); + EXPECT_FALSE(packet.HasExtension()); + + // Try to set the extensions. + EXPECT_TRUE(packet.SetExtension(kTimeOffset)); + + EXPECT_TRUE(packet.HasExtension()); + EXPECT_EQ(packet.GetExtension(), kTimeOffset); +} + +TEST(RtpPacketTest, FailsToSetUnregisteredExtension) { + RtpPacketToSend::ExtensionManager extensions; + extensions.Register(kTransmissionOffsetExtensionId); + RtpPacketToSend packet(&extensions); + + EXPECT_FALSE(packet.IsRegistered()); + EXPECT_FALSE(packet.HasExtension()); + + EXPECT_FALSE(packet.SetExtension(42)); + + EXPECT_FALSE(packet.HasExtension()); + EXPECT_EQ(packet.GetExtension(), absl::nullopt); +} + TEST(RtpPacketTest, SetReservedExtensionsAfterPayload) { const size_t kPayloadSize = 4; RtpPacketToSend::ExtensionManager extensions; diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index d344695fdf..4919e3ebf4 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -361,7 +361,8 @@ void RTPSenderVideo::AddRtpHeaderExtensions( if (video_header.generic) { bool extension_is_set = false; - if (video_structure_ != nullptr) { + if (packet->IsRegistered() && + video_structure_ != nullptr) { DependencyDescriptor descriptor; descriptor.first_packet_in_frame = first_packet; descriptor.last_packet_in_frame = last_packet; @@ -407,7 +408,8 @@ void RTPSenderVideo::AddRtpHeaderExtensions( } // Do not use generic frame descriptor when dependency descriptor is stored. - if (!extension_is_set) { + if (packet->IsRegistered() && + !extension_is_set) { RtpGenericFrameDescriptor generic_descriptor; generic_descriptor.SetFirstPacketInSubFrame(first_packet); generic_descriptor.SetLastPacketInSubFrame(last_packet); @@ -437,7 +439,8 @@ void RTPSenderVideo::AddRtpHeaderExtensions( } } - if (first_packet && + if (packet->IsRegistered() && + first_packet && send_allocation_ != SendVideoLayersAllocation::kDontSend && (video_header.frame_type == VideoFrameType::kVideoFrameKey || PacketWillLikelyBeRequestedForRestransmitionIfLost(video_header))) {