From 2788373528ec05ccb3e3107ee67fb12819011f01 Mon Sep 17 00:00:00 2001 From: erikvarga Date: Wed, 17 May 2017 05:08:38 -0700 Subject: [PATCH] Remove hardcoded kValueSizeBytes values from variable-length header extensions. Since the RtpStreamId and RepairedRtpStreamId extensions can have variable length, it makes no sense for them to have a constant valueSize field. The header length calculation in RtpHeaderExtensionMap needed to be changed for this because it previously worked with the assumption that all header types have a constant size. Now it's the caller's job to specify the length of the extensions that it might use. BUG=webrtc:7433 Review-Url: https://codereview.webrtc.org/2867713003 Cr-Commit-Position: refs/heads/master@{#18179} --- .../modules/rtp_rtcp/include/flexfec_sender.h | 3 + webrtc/modules/rtp_rtcp/include/rtp_rtcp.h | 5 -- webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 1 - .../modules/rtp_rtcp/source/flexfec_sender.cc | 8 ++- .../source/flexfec_sender_unittest.cc | 58 +++++++++++++++---- .../rtp_rtcp/source/rtp_header_extension.cc | 37 +++++------- .../rtp_rtcp/source/rtp_header_extension.h | 18 +++--- .../source/rtp_header_extension_unittest.cc | 10 +++- .../rtp_rtcp/source/rtp_header_extensions.cc | 2 - .../rtp_rtcp/source/rtp_header_extensions.h | 6 -- .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 4 -- .../modules/rtp_rtcp/source/rtp_rtcp_impl.h | 2 - webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 41 ++++++++----- webrtc/modules/rtp_rtcp/source/rtp_sender.h | 8 ++- .../rtp_rtcp/source/rtp_sender_unittest.cc | 12 +++- .../modules/rtp_rtcp/test/testAPI/test_api.cc | 1 - webrtc/test/fuzzers/flexfec_sender_fuzzer.cc | 4 +- webrtc/video/video_send_stream.cc | 3 +- 18 files changed, 133 insertions(+), 90 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/include/flexfec_sender.h b/webrtc/modules/rtp_rtcp/include/flexfec_sender.h index e69daf3ff0..a2c7a4ab1e 100644 --- a/webrtc/modules/rtp_rtcp/include/flexfec_sender.h +++ b/webrtc/modules/rtp_rtcp/include/flexfec_sender.h @@ -14,6 +14,7 @@ #include #include +#include "webrtc/base/array_view.h" #include "webrtc/base/basictypes.h" #include "webrtc/base/random.h" #include "webrtc/base/sequenced_task_checker.h" @@ -38,6 +39,7 @@ class FlexfecSender { uint32_t ssrc, uint32_t protected_media_ssrc, const std::vector& rtp_header_extensions, + rtc::ArrayView extension_sizes, Clock* clock); ~FlexfecSender(); @@ -79,6 +81,7 @@ class FlexfecSender { // Implementation. UlpfecGenerator ulpfec_generator_; const RtpHeaderExtensionMap rtp_header_extension_map_; + const size_t header_extensions_size_; }; } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h b/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h index 10c947bd84..6fc6b862b8 100644 --- a/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h @@ -125,11 +125,6 @@ class RtpRtcp : public Module { // Sets the maximum size of an RTP packet, including RTP headers. virtual void SetMaxRtpPacketSize(size_t size) = 0; - // Returns max payload length. - // Does not account for RTP headers and FEC/ULP/RED overhead (when FEC is - // enabled). - virtual size_t MaxPayloadSize() const = 0; - // Returns max RTP packet size. Takes into account RTP headers and // FEC/ULP/RED overhead (when FEC is enabled). virtual size_t MaxRtpPacketSize() const = 0; diff --git a/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index c9ea49d478..1f494bfa62 100644 --- a/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -56,7 +56,6 @@ class MockRtpRtcp : public RtpRtcp { MOCK_METHOD1(RegisterSendTransport, int32_t(Transport* outgoing_transport)); MOCK_METHOD1(SetMaxRtpPacketSize, void(size_t size)); MOCK_METHOD1(SetTransportOverhead, void(int transport_overhead_per_packet)); - MOCK_CONST_METHOD0(MaxPayloadSize, size_t()); MOCK_CONST_METHOD0(MaxRtpPacketSize, size_t()); MOCK_METHOD1(RegisterSendPayload, int32_t(const CodecInst& voice_codec)); MOCK_METHOD1(RegisterSendPayload, int32_t(const VideoCodec& video_codec)); diff --git a/webrtc/modules/rtp_rtcp/source/flexfec_sender.cc b/webrtc/modules/rtp_rtcp/source/flexfec_sender.cc index 4aec05662f..a2c9ae39ad 100644 --- a/webrtc/modules/rtp_rtcp/source/flexfec_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/flexfec_sender.cc @@ -64,6 +64,7 @@ FlexfecSender::FlexfecSender( uint32_t ssrc, uint32_t protected_media_ssrc, const std::vector& rtp_header_extensions, + rtc::ArrayView extension_sizes, Clock* clock) : clock_(clock), random_(clock_->TimeInMicroseconds()), @@ -76,7 +77,9 @@ FlexfecSender::FlexfecSender( protected_media_ssrc_(protected_media_ssrc), seq_num_(random_.Rand(1, kMaxInitRtpSeqNumber)), ulpfec_generator_(ForwardErrorCorrection::CreateFlexfec()), - rtp_header_extension_map_(RegisterBweExtensions(rtp_header_extensions)) { + rtp_header_extension_map_(RegisterBweExtensions(rtp_header_extensions)), + header_extensions_size_( + rtp_header_extension_map_.GetTotalLengthInBytes(extension_sizes)) { // This object should not have been instantiated if FlexFEC is disabled. RTC_DCHECK_GE(payload_type, 0); RTC_DCHECK_LE(payload_type, 127); @@ -148,8 +151,7 @@ std::vector> FlexfecSender::GetFecPackets() { // The overhead is BWE RTP header extensions and FlexFEC header. size_t FlexfecSender::MaxPacketOverhead() const { - return rtp_header_extension_map_.GetTotalLengthInBytes() + - kFlexfecMaxHeaderSize; + return header_extensions_size_ + kFlexfecMaxHeaderSize; } } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc index c906750fe3..da3efc9f18 100644 --- a/webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc @@ -16,6 +16,8 @@ #include "webrtc/modules/rtp_rtcp/source/fec_test_helper.h" #include "webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h" #include "webrtc/modules/rtp_rtcp/source/rtp_packet_to_send.h" +#include "webrtc/modules/rtp_rtcp/source/rtp_sender.h" +#include "webrtc/modules/rtp_rtcp/source/rtp_utility.h" #include "webrtc/system_wrappers/include/clock.h" #include "webrtc/test/gtest.h" @@ -23,6 +25,7 @@ namespace webrtc { namespace { +using RtpUtility::Word32Align; using test::fec::AugmentedPacket; using test::fec::AugmentedPacketGenerator; @@ -30,6 +33,7 @@ constexpr int kFlexfecPayloadType = 123; constexpr uint32_t kMediaSsrc = 1234; constexpr uint32_t kFlexfecSsrc = 5678; const std::vector kNoRtpHeaderExtensions; +const std::vector kNoRtpHeaderExtensionSizes; // Assume a single protected media SSRC. constexpr size_t kFlexfecMaxHeaderSize = 32; constexpr size_t kPayloadLength = 50; @@ -73,7 +77,8 @@ std::unique_ptr GenerateSingleFlexfecPacket( TEST(FlexfecSenderTest, Ssrc) { SimulatedClock clock(kInitialSimulatedClockTime); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kNoRtpHeaderExtensions, &clock); + kNoRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); EXPECT_EQ(kFlexfecSsrc, sender.ssrc()); } @@ -81,7 +86,8 @@ TEST(FlexfecSenderTest, Ssrc) { TEST(FlexfecSenderTest, NoFecAvailableBeforeMediaAdded) { SimulatedClock clock(kInitialSimulatedClockTime); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kNoRtpHeaderExtensions, &clock); + kNoRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); EXPECT_FALSE(sender.FecAvailable()); auto fec_packets = sender.GetFecPackets(); @@ -91,7 +97,8 @@ TEST(FlexfecSenderTest, NoFecAvailableBeforeMediaAdded) { TEST(FlexfecSenderTest, ProtectOneFrameWithOneFecPacket) { SimulatedClock clock(kInitialSimulatedClockTime); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kNoRtpHeaderExtensions, &clock); + kNoRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); auto fec_packet = GenerateSingleFlexfecPacket(&sender); EXPECT_EQ(kRtpHeaderSize, fec_packet->headers_size()); @@ -113,7 +120,8 @@ TEST(FlexfecSenderTest, ProtectTwoFramesWithOneFecPacket) { constexpr size_t kNumPacketsPerFrame = 2; SimulatedClock clock(kInitialSimulatedClockTime); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kNoRtpHeaderExtensions, &clock); + kNoRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); sender.SetFecParameters(params); AugmentedPacketGenerator packet_generator(kMediaSsrc); @@ -152,7 +160,8 @@ TEST(FlexfecSenderTest, ProtectTwoFramesWithTwoFecPackets) { constexpr size_t kNumPacketsPerFrame = 2; SimulatedClock clock(kInitialSimulatedClockTime); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kNoRtpHeaderExtensions, &clock); + kNoRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); sender.SetFecParameters(params); AugmentedPacketGenerator packet_generator(kMediaSsrc); @@ -187,7 +196,8 @@ TEST(FlexfecSenderTest, NoRtpHeaderExtensionsForBweByDefault) { const std::vector kRtpHeaderExtensions{}; SimulatedClock clock(kInitialSimulatedClockTime); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kRtpHeaderExtensions, &clock); + kRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); auto fec_packet = GenerateSingleFlexfecPacket(&sender); EXPECT_FALSE(fec_packet->HasExtension()); @@ -200,7 +210,8 @@ TEST(FlexfecSenderTest, RegisterAbsoluteSendTimeRtpHeaderExtension) { {RtpExtension::kAbsSendTimeUri, 1}}; SimulatedClock clock(kInitialSimulatedClockTime); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kRtpHeaderExtensions, &clock); + kRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); auto fec_packet = GenerateSingleFlexfecPacket(&sender); EXPECT_TRUE(fec_packet->HasExtension()); @@ -213,7 +224,8 @@ TEST(FlexfecSenderTest, RegisterTransmissionOffsetRtpHeaderExtension) { {RtpExtension::kTimestampOffsetUri, 1}}; SimulatedClock clock(kInitialSimulatedClockTime); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kRtpHeaderExtensions, &clock); + kRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); auto fec_packet = GenerateSingleFlexfecPacket(&sender); EXPECT_FALSE(fec_packet->HasExtension()); @@ -226,7 +238,8 @@ TEST(FlexfecSenderTest, RegisterTransportSequenceNumberRtpHeaderExtension) { {RtpExtension::kTransportSequenceNumberUri, 1}}; SimulatedClock clock(kInitialSimulatedClockTime); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kRtpHeaderExtensions, &clock); + kRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); auto fec_packet = GenerateSingleFlexfecPacket(&sender); EXPECT_FALSE(fec_packet->HasExtension()); @@ -241,7 +254,8 @@ TEST(FlexfecSenderTest, RegisterAllRtpHeaderExtensionsForBwe) { {RtpExtension::kTransportSequenceNumberUri, 3}}; SimulatedClock clock(kInitialSimulatedClockTime); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kRtpHeaderExtensions, &clock); + kRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); auto fec_packet = GenerateSingleFlexfecPacket(&sender); EXPECT_TRUE(fec_packet->HasExtension()); @@ -252,9 +266,31 @@ TEST(FlexfecSenderTest, RegisterAllRtpHeaderExtensionsForBwe) { TEST(FlexfecSenderTest, MaxPacketOverhead) { SimulatedClock clock(kInitialSimulatedClockTime); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kNoRtpHeaderExtensions, &clock); + kNoRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); EXPECT_EQ(kFlexfecMaxHeaderSize, sender.MaxPacketOverhead()); } +TEST(FlexfecSenderTest, MaxPacketOverheadWithExtensions) { + const std::vector kRtpHeaderExtensions{ + {RtpExtension::kAbsSendTimeUri, 1}, + {RtpExtension::kTimestampOffsetUri, 2}, + {RtpExtension::kTransportSequenceNumberUri, 3}}; + SimulatedClock clock(kInitialSimulatedClockTime); + const size_t kExtensionHeaderLength = 1; + const size_t kRtpOneByteHeaderLength = 4; + const size_t kExtensionsTotalSize = Word32Align( + kRtpOneByteHeaderLength + + kExtensionHeaderLength + AbsoluteSendTime::kValueSizeBytes + + kExtensionHeaderLength + TransmissionOffset::kValueSizeBytes + + kExtensionHeaderLength + TransportSequenceNumber::kValueSizeBytes); + FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, + kRtpHeaderExtensions, RTPSender::FecExtensionSizes(), + &clock); + + EXPECT_EQ(kExtensionsTotalSize + kFlexfecMaxHeaderSize, + sender.MaxPacketOverhead()); +} + } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc b/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc index 26762b0e96..5764c7f491 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc @@ -23,13 +23,12 @@ using RtpUtility::Word32Align; struct ExtensionInfo { RTPExtensionType type; - size_t value_size; const char* uri; }; template constexpr ExtensionInfo CreateExtensionInfo() { - return {Extension::kId, Extension::kValueSizeBytes, Extension::kUri}; + return {Extension::kId, Extension::kUri}; } constexpr ExtensionInfo kExtensions[] = { @@ -50,15 +49,6 @@ static_assert(arraysize(kExtensions) == static_cast(kRtpExtensionNumberOfExtensions) - 1, "kExtensions expect to list all known extensions"); -size_t ValueSize(RTPExtensionType type) { - for (const ExtensionInfo& extension : kExtensions) - if (type == extension.type) - return extension.value_size; - - RTC_NOTREACHED(); - return 0; -} - } // namespace constexpr RTPExtensionType RtpHeaderExtensionMap::kInvalidType; @@ -67,7 +57,6 @@ constexpr uint8_t RtpHeaderExtensionMap::kMinId; constexpr uint8_t RtpHeaderExtensionMap::kMaxId; RtpHeaderExtensionMap::RtpHeaderExtensionMap() { - total_values_size_bytes_ = 0; for (auto& type : types_) type = kInvalidType; for (auto& id : ids_) @@ -84,7 +73,7 @@ RtpHeaderExtensionMap::RtpHeaderExtensionMap( bool RtpHeaderExtensionMap::RegisterByType(uint8_t id, RTPExtensionType type) { for (const ExtensionInfo& extension : kExtensions) if (type == extension.type) - return Register(id, extension.type, extension.value_size, extension.uri); + return Register(id, extension.type, extension.uri); RTC_NOTREACHED(); return false; } @@ -92,23 +81,31 @@ bool RtpHeaderExtensionMap::RegisterByType(uint8_t id, RTPExtensionType type) { bool RtpHeaderExtensionMap::RegisterByUri(uint8_t id, const std::string& uri) { for (const ExtensionInfo& extension : kExtensions) if (uri == extension.uri) - return Register(id, extension.type, extension.value_size, extension.uri); + return Register(id, extension.type, extension.uri); LOG(LS_WARNING) << "Unknown extension uri:'" << uri << "', id: " << static_cast(id) << '.'; return false; } -size_t RtpHeaderExtensionMap::GetTotalLengthInBytes() const { +size_t RtpHeaderExtensionMap::GetTotalLengthInBytes( + rtc::ArrayView extensions) const { + // Header size of the extension block, see RFC3550 Section 5.3.1 static constexpr size_t kRtpOneByteHeaderLength = 4; - if (total_values_size_bytes_ == 0) + // Header size of each individual extension, see RFC5285 Section 4.2 + static constexpr size_t kExtensionHeaderLength = 1; + size_t values_size = 0; + for (const RtpExtensionSize& extension : extensions) { + if (IsRegistered(extension.type)) + values_size += extension.value_size + kExtensionHeaderLength; + } + if (values_size == 0) return 0; - return Word32Align(kRtpOneByteHeaderLength + total_values_size_bytes_); + return Word32Align(kRtpOneByteHeaderLength + values_size); } int32_t RtpHeaderExtensionMap::Deregister(RTPExtensionType type) { if (IsRegistered(type)) { uint8_t id = GetId(type); - total_values_size_bytes_ -= (ValueSize(type) + 1); types_[id] = kInvalidType; ids_[type] = kInvalidId; } @@ -117,12 +114,9 @@ int32_t RtpHeaderExtensionMap::Deregister(RTPExtensionType type) { bool RtpHeaderExtensionMap::Register(uint8_t id, RTPExtensionType type, - size_t value_size, const char* uri) { RTC_DCHECK_GT(type, kRtpExtensionNone); RTC_DCHECK_LT(type, kRtpExtensionNumberOfExtensions); - RTC_DCHECK_GE(value_size, 1U); - RTC_DCHECK_LE(value_size, 16U); if (id < kMinId || id > kMaxId) { LOG(LS_WARNING) << "Failed to register extension uri:'" << uri @@ -147,7 +141,6 @@ bool RtpHeaderExtensionMap::Register(uint8_t id, types_[id] = type; ids_[type] = id; - total_values_size_bytes_ += (value_size + 1); return true; } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h b/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h index 1b5a7ab3e6..f3aef0c2a5 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h @@ -21,6 +21,11 @@ namespace webrtc { +struct RtpExtensionSize { + RTPExtensionType type; + uint8_t value_size; +}; + class RtpHeaderExtensionMap { public: static constexpr RTPExtensionType kInvalidType = kRtpExtensionNone; @@ -31,8 +36,7 @@ class RtpHeaderExtensionMap { template bool Register(uint8_t id) { - return Register(id, Extension::kId, Extension::kValueSizeBytes, - Extension::kUri); + return Register(id, Extension::kId, Extension::kUri); } bool RegisterByType(uint8_t id, RTPExtensionType type); bool RegisterByUri(uint8_t id, const std::string& uri); @@ -53,7 +57,8 @@ class RtpHeaderExtensionMap { return ids_[type]; } - size_t GetTotalLengthInBytes() const; + size_t GetTotalLengthInBytes( + rtc::ArrayView extensions) const; // TODO(danilchap): Remove use of the functions below. int32_t Register(RTPExtensionType type, uint8_t id) { @@ -64,12 +69,8 @@ class RtpHeaderExtensionMap { private: static constexpr uint8_t kMinId = 1; static constexpr uint8_t kMaxId = 14; - bool Register(uint8_t id, - RTPExtensionType type, - size_t value_size, - const char* uri); + bool Register(uint8_t id, RTPExtensionType type, const char* uri); - size_t total_values_size_bytes_ = 0; RTPExtensionType types_[kMaxId + 1]; uint8_t ids_[kRtpExtensionNumberOfExtensions]; }; @@ -77,4 +78,3 @@ class RtpHeaderExtensionMap { } // namespace webrtc #endif // WEBRTC_MODULES_RTP_RTCP_SOURCE_RTP_HEADER_EXTENSION_H_ - diff --git a/webrtc/modules/rtp_rtcp/source/rtp_header_extension_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_header_extension_unittest.cc index 179d27985f..0d4a5c803c 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_header_extension_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extension_unittest.cc @@ -85,11 +85,15 @@ TEST(RtpHeaderExtensionTest, NonUniqueId) { TEST(RtpHeaderExtensionTest, GetTotalLength) { RtpHeaderExtensionMap map; - EXPECT_EQ(0u, map.GetTotalLengthInBytes()); + constexpr RtpExtensionSize kExtensionSizes[] = { + {TransmissionOffset::kId, TransmissionOffset::kValueSizeBytes} + }; + EXPECT_EQ(0u, map.GetTotalLengthInBytes(kExtensionSizes)); EXPECT_TRUE(map.Register(3)); static constexpr size_t kRtpOneByteHeaderLength = 4; - EXPECT_EQ(kRtpOneByteHeaderLength + (TransmissionOffset::kValueSizeBytes + 1), - map.GetTotalLengthInBytes()); + EXPECT_EQ( + kRtpOneByteHeaderLength + (TransmissionOffset::kValueSizeBytes + 1), + map.GetTotalLengthInBytes(kExtensionSizes)); } TEST(RtpHeaderExtensionTest, GetType) { diff --git a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc index ba512ef349..3bb9bc5553 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc @@ -246,7 +246,6 @@ bool VideoContentTypeExtension::Write(uint8_t* data, // RtpStreamId. constexpr RTPExtensionType RtpStreamId::kId; -constexpr uint8_t RtpStreamId::kValueSizeBytes; constexpr const char* RtpStreamId::kUri; bool RtpStreamId::Parse(rtc::ArrayView data, StreamId* rsid) { @@ -284,7 +283,6 @@ bool RtpStreamId::Write(uint8_t* data, const std::string& rsid) { // RepairedRtpStreamId. constexpr RTPExtensionType RepairedRtpStreamId::kId; -constexpr uint8_t RepairedRtpStreamId::kValueSizeBytes; constexpr const char* RepairedRtpStreamId::kUri; // RtpStreamId and RepairedRtpStreamId use the same format to store rsid. diff --git a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h index df68b21735..a1f69dd0f9 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h @@ -129,9 +129,6 @@ class VideoContentTypeExtension { class RtpStreamId { public: static constexpr RTPExtensionType kId = kRtpExtensionRtpStreamId; - // TODO(danilchap): Implement write support of dynamic size extension that - // allows to remove the ValueSize constant. - static constexpr uint8_t kValueSizeBytes = 1; static constexpr const char* kUri = "urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id"; @@ -147,9 +144,6 @@ class RtpStreamId { class RepairedRtpStreamId { public: static constexpr RTPExtensionType kId = kRtpExtensionRepairedRtpStreamId; - // TODO(danilchap): Implement write support of dynamic size extension that - // allows to remove the ValueSize constant. - static constexpr uint8_t kValueSizeBytes = 1; static constexpr const char* kUri = "urn:ietf:params:rtp-hdrext:sdes:repaired-rtp-stream-id"; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index d6c54d033c..faaf4c4f78 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -417,10 +417,6 @@ size_t ModuleRtpRtcpImpl::TimeToSendPadding( return rtp_sender_->TimeToSendPadding(bytes, pacing_info); } -size_t ModuleRtpRtcpImpl::MaxPayloadSize() const { - return rtp_sender_->MaxPayloadSize(); -} - size_t ModuleRtpRtcpImpl::MaxRtpPacketSize() const { return rtp_sender_->MaxRtpPacketSize(); } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 13bdac020b..0c46e40e1e 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -209,8 +209,6 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { void SetTmmbn(std::vector bounding_set) override; - size_t MaxPayloadSize() const override; - size_t MaxRtpPacketSize() const override; void SetMaxRtpPacketSize(size_t max_packet_size) override; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index 6fdac6f63f..17bcc7c625 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -13,6 +13,7 @@ #include #include +#include "webrtc/base/arraysize.h" #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" #include "webrtc/base/rate_limiter.h" @@ -45,6 +46,19 @@ constexpr int kBitrateStatisticsWindowMs = 1000; constexpr size_t kMinFlexfecPacketsToStoreForPacing = 50; +template +constexpr RtpExtensionSize CreateExtensionSize() { + return {Extension::kId, Extension::kValueSizeBytes}; +} + +// Size info for header extensions that might be used in padding or FEC packets. +constexpr RtpExtensionSize kExtensionSizes[] = { + CreateExtensionSize(), + CreateExtensionSize(), + CreateExtensionSize(), + CreateExtensionSize(), +}; + const char* FrameTypeToString(FrameType frame_type) { switch (frame_type) { case kEmptyFrame: @@ -157,6 +171,10 @@ RTPSender::~RTPSender() { } } +rtc::ArrayView RTPSender::FecExtensionSizes() { + return rtc::MakeArrayView(kExtensionSizes, arraysize(kExtensionSizes)); +} + uint16_t RTPSender::ActualSendBitrateKbit() const { rtc::CritScope cs(&statistics_crit_); return static_cast( @@ -277,16 +295,6 @@ void RTPSender::SetMaxRtpPacketSize(size_t max_packet_size) { max_packet_size_ = max_packet_size; } -size_t RTPSender::MaxPayloadSize() const { - if (audio_configured_) { - return max_packet_size_ - RtpHeaderLength(); - } else { - return max_packet_size_ - RtpHeaderLength() // RTP overhead. - - video_->FecPacketOverhead() // FEC/ULP/RED overhead. - - (RtxStatus() ? kRtxHeaderSize : 0); // RTX overhead. - } -} - size_t RTPSender::MaxRtpPacketSize() const { return max_packet_size_; } @@ -456,10 +464,12 @@ size_t RTPSender::TrySendRedundantPayloads(size_t bytes_to_send, size_t RTPSender::SendPadData(size_t bytes, const PacedPacketInfo& pacing_info) { size_t padding_bytes_in_packet; + if (audio_configured_) { // Allow smaller padding packets for audio. + size_t max_payload_size = max_packet_size_ - RtpHeaderLength(); padding_bytes_in_packet = - std::min(std::max(bytes, kMinAudioPaddingLength), MaxPayloadSize()); + std::min(std::max(bytes, kMinAudioPaddingLength), max_payload_size); if (padding_bytes_in_packet > kMaxPaddingLength) padding_bytes_in_packet = kMaxPaddingLength; } else { @@ -467,7 +477,11 @@ size_t RTPSender::SendPadData(size_t bytes, // RtpPacketSender, which will make sure we don't send too much padding even // if a single packet is larger than requested. // We do this to avoid frequently sending small packets on higher bitrates. - padding_bytes_in_packet = std::min(MaxPayloadSize(), kMaxPaddingLength); + size_t max_payload_size = + max_packet_size_ - RtpHeaderLength() // RTP overhead. + - video_->FecPacketOverhead() // FEC/ULP/RED overhead. + - (RtxStatus() ? kRtxHeaderSize : 0); // RTX overhead. + padding_bytes_in_packet = std::min(max_payload_size, kMaxPaddingLength); } size_t bytes_sent = 0; while (bytes_sent < bytes) { @@ -961,7 +975,8 @@ size_t RTPSender::RtpHeaderLength() const { rtc::CritScope lock(&send_critsect_); size_t rtp_header_length = kRtpHeaderLength; rtp_header_length += sizeof(uint32_t) * csrcs_.size(); - rtp_header_length += rtp_header_extension_map_.GetTotalLengthInBytes(); + rtp_header_length += + rtp_header_extension_map_.GetTotalLengthInBytes(kExtensionSizes); return rtp_header_length; } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.h b/webrtc/modules/rtp_rtcp/source/rtp_sender.h index dc35a038e1..b0165c628d 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.h @@ -17,6 +17,7 @@ #include #include "webrtc/api/call/transport.h" +#include "webrtc/base/array_view.h" #include "webrtc/base/constructormagic.h" #include "webrtc/base/criticalsection.h" #include "webrtc/base/deprecation.h" @@ -71,9 +72,6 @@ class RTPSender { uint32_t FecOverheadRate() const; uint32_t NackOverheadRate() const; - // Excluding size of RTP and FEC headers. - size_t MaxPayloadSize() const; - int32_t RegisterPayload(const char* payload_name, const int8_t payload_type, const uint32_t frequency, @@ -148,6 +146,9 @@ class RTPSender { void SetRtxPayloadType(int payload_type, int associated_payload_type); + // Size info for header extensions used by FEC packets. + static rtc::ArrayView FecExtensionSizes(); + // Create empty packet, fills ssrc, csrcs and reserve place for header // extensions RtpSender updates before sending. std::unique_ptr AllocatePacket() const; @@ -156,6 +157,7 @@ class RTPSender { // Return false if sending was turned off. bool AssignSequenceNumber(RtpPacketToSend* packet); + // Used for padding and FEC packets only. size_t RtpHeaderLength() const; uint16_t AllocateSequenceNumber(uint16_t packets_to_send); // Including RTP headers. diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 961b53a139..70f27d0651 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -843,8 +843,10 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { constexpr uint32_t kMediaSsrc = 1234; constexpr uint32_t kFlexfecSsrc = 5678; const std::vector kNoRtpExtensions; + const std::vector kNoRtpExtensionSizes; FlexfecSender flexfec_sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kNoRtpExtensions, &fake_clock_); + kNoRtpExtensions, kNoRtpExtensionSizes, + &fake_clock_); // Reset |rtp_sender_| to use FlexFEC. rtp_sender_.reset(new RTPSender( @@ -898,8 +900,10 @@ TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { constexpr uint32_t kMediaSsrc = 1234; constexpr uint32_t kFlexfecSsrc = 5678; const std::vector kNoRtpExtensions; + const std::vector kNoRtpExtensionSizes; FlexfecSender flexfec_sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kNoRtpExtensions, &fake_clock_); + kNoRtpExtensions, kNoRtpExtensionSizes, + &fake_clock_); // Reset |rtp_sender_| to use FlexFEC. rtp_sender_.reset(new RTPSender(false, &fake_clock_, &transport_, nullptr, @@ -937,8 +941,10 @@ TEST_P(RtpSenderTest, FecOverheadRate) { constexpr uint32_t kMediaSsrc = 1234; constexpr uint32_t kFlexfecSsrc = 5678; const std::vector kNoRtpExtensions; + const std::vector kNoRtpExtensionSizes; FlexfecSender flexfec_sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kNoRtpExtensions, &fake_clock_); + kNoRtpExtensions, kNoRtpExtensionSizes, + &fake_clock_); // Reset |rtp_sender_| to use FlexFEC. rtp_sender_.reset(new RTPSender( diff --git a/webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc b/webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc index b53574c625..4eac882bb0 100644 --- a/webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc +++ b/webrtc/modules/rtp_rtcp/test/testAPI/test_api.cc @@ -136,7 +136,6 @@ TEST_F(RtpRtcpAPITest, Basic) { TEST_F(RtpRtcpAPITest, PacketSize) { module_->SetMaxRtpPacketSize(1234); EXPECT_EQ(1234u, module_->MaxRtpPacketSize()); - EXPECT_EQ(1234u - 12u /* Minimum RTP header */, module_->MaxPayloadSize()); } TEST_F(RtpRtcpAPITest, SSRC) { diff --git a/webrtc/test/fuzzers/flexfec_sender_fuzzer.cc b/webrtc/test/fuzzers/flexfec_sender_fuzzer.cc index bc486dda07..e0fb892e8f 100644 --- a/webrtc/test/fuzzers/flexfec_sender_fuzzer.cc +++ b/webrtc/test/fuzzers/flexfec_sender_fuzzer.cc @@ -24,6 +24,7 @@ constexpr int kFlexfecPayloadType = 123; constexpr uint32_t kMediaSsrc = 1234; constexpr uint32_t kFlexfecSsrc = 5678; const std::vector kNoRtpHeaderExtensions; +const std::vector kNoRtpHeaderExtensionSizes; } // namespace @@ -35,7 +36,8 @@ void FuzzOneInput(const uint8_t* data, size_t size) { SimulatedClock clock(1 + data[i++]); FlexfecSender sender(kFlexfecPayloadType, kFlexfecSsrc, kMediaSsrc, - kNoRtpHeaderExtensions, &clock); + kNoRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, + &clock); FecProtectionParams params = { data[i++], static_cast(data[i++] % 100), data[i++] <= 127 ? kFecMaskRandom : kFecMaskBursty}; diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index c7cc872611..9c70789960 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -29,6 +29,7 @@ #include "webrtc/modules/congestion_controller/include/send_side_congestion_controller.h" #include "webrtc/modules/pacing/packet_router.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h" +#include "webrtc/modules/rtp_rtcp/source/rtp_sender.h" #include "webrtc/modules/utility/include/process_thread.h" #include "webrtc/modules/video_coding/utility/ivf_file_writer.h" #include "webrtc/system_wrappers/include/field_trial.h" @@ -131,7 +132,7 @@ std::unique_ptr MaybeCreateFlexfecSender( return std::unique_ptr(new FlexfecSender( config.rtp.flexfec.payload_type, config.rtp.flexfec.ssrc, config.rtp.flexfec.protected_media_ssrcs[0], config.rtp.extensions, - Clock::GetRealTimeClock())); + RTPSender::FecExtensionSizes(), Clock::GetRealTimeClock())); } } // namespace