From cc348338098133849063f2cab8023946a5cf42cb Mon Sep 17 00:00:00 2001 From: danilchap Date: Tue, 25 Oct 2016 03:12:28 -0700 Subject: [PATCH] Remove now unused code in RtpHeaderExtensionMap Remove functions to enumerate all extensions, Remove concept of the inactive extension. Decision if extension should be included into rtp header is done by rtp_sender GetTotalLengthInBytes now calculates all extension, included or not. That is used only for calculating how much space to reserve for fec. Since extension might suddenly be included in the next packet (which still might belong to same fec group), it is safer to calculate all registered extension. BUG=webrtc:5565, webrtc:1994 Review-Url: https://codereview.webrtc.org/2431253003 Cr-Commit-Position: refs/heads/master@{#14763} --- .../rtp_rtcp/source/rtp_header_extension.cc | 108 ++---------------- .../rtp_rtcp/source/rtp_header_extension.h | 32 +----- .../source/rtp_header_extension_unittest.cc | 39 +------ webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 66 ----------- webrtc/modules/rtp_rtcp/source/rtp_sender.h | 18 --- .../rtp_rtcp/source/rtp_sender_unittest.cc | 5 - .../rtp_rtcp/source/rtp_sender_video.cc | 7 +- 7 files changed, 13 insertions(+), 262 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc b/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc index 2c2a0a1356..27bbda4cc9 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc @@ -32,19 +32,7 @@ void RtpHeaderExtensionMap::Erase() { } } -int32_t RtpHeaderExtensionMap::Register(const RTPExtensionType type, - const uint8_t id) { - return Register(type, id, true); -} - -int32_t RtpHeaderExtensionMap::RegisterInactive(const RTPExtensionType type, - const uint8_t id) { - return Register(type, id, false); -} - -int32_t RtpHeaderExtensionMap::Register(const RTPExtensionType type, - const uint8_t id, - bool active) { +int32_t RtpHeaderExtensionMap::Register(RTPExtensionType type, uint8_t id) { if (id < 1 || id > 14) { return -1; } @@ -58,24 +46,12 @@ int32_t RtpHeaderExtensionMap::Register(const RTPExtensionType type, } // This extension type is already registered with this id, // so return success. - it->second->active = active; return 0; } - extensionMap_[id] = new HeaderExtension(type, active); + extensionMap_[id] = new HeaderExtension(type); return 0; } -bool RtpHeaderExtensionMap::SetActive(const RTPExtensionType type, - bool active) { - for (auto& kv : extensionMap_) { - if (kv.second->type == type) { - kv.second->active = active; - return true; - } - } - return false; -} - int32_t RtpHeaderExtensionMap::Deregister(const RTPExtensionType type) { uint8_t id; if (GetId(type, &id) != 0) { @@ -148,88 +124,18 @@ uint8_t RtpHeaderExtensionMap::GetId(RTPExtensionType type) const { size_t RtpHeaderExtensionMap::GetTotalLengthInBytes() const { // Get length for each extension block. size_t length = 0; - std::map::const_iterator it = - extensionMap_.begin(); - while (it != extensionMap_.end()) { - HeaderExtension* extension = it->second; - if (extension->active) { - length += extension->length; - } - it++; - } + for (const auto& kv : extensionMap_) + length += kv.second->length; // Add RTP extension header length. - if (length > 0) { + if (length > 0) length += kRtpOneByteHeaderLength; - } // Pad up to nearest 32bit word. length = RtpUtility::Word32Align(length); return length; } -int32_t RtpHeaderExtensionMap::GetLengthUntilBlockStartInBytes( - const RTPExtensionType type) const { - uint8_t id; - if (GetId(type, &id) != 0) { - // Not registered. - return -1; - } - // Get length until start of extension block type. - uint16_t length = kRtpOneByteHeaderLength; - - std::map::const_iterator it = - extensionMap_.begin(); - while (it != extensionMap_.end()) { - HeaderExtension* extension = it->second; - if (extension->type == type) { - if (!extension->active) { - return -1; - } - break; - } else if (extension->active) { - length += extension->length; - } - it++; - } - return length; -} - int32_t RtpHeaderExtensionMap::Size() const { - int32_t count = 0; - for (auto& kv : extensionMap_) { - if (kv.second->active) { - count++; - } - } - return count; -} - -RTPExtensionType RtpHeaderExtensionMap::First() const { - for (auto& kv : extensionMap_) { - if (kv.second->active) { - return kv.second->type; - } - } - - return kRtpExtensionNone; -} - -RTPExtensionType RtpHeaderExtensionMap::Next(RTPExtensionType type) const { - uint8_t id; - if (GetId(type, &id) != 0) { - return kRtpExtensionNone; - } - std::map::const_iterator it = - extensionMap_.find(id); - if (it == extensionMap_.end() || !it->second->active) { - return kRtpExtensionNone; - } - while ((++it) != extensionMap_.end()) { - if (it->second->active) { - return it->second->type; - } - } - - return kRtpExtensionNone; + return extensionMap_.size(); } void RtpHeaderExtensionMap::GetCopy(RtpHeaderExtensionMap* map) const { @@ -238,7 +144,7 @@ void RtpHeaderExtensionMap::GetCopy(RtpHeaderExtensionMap* map) const { extensionMap_.begin(); while (it != extensionMap_.end()) { HeaderExtension* extension = it->second; - map->Register(extension->type, it->first, extension->active); + map->Register(extension->type, it->first); it++; } } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h b/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h index f7df4bbe4c..b053f7f8ca 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extension.h @@ -37,12 +37,7 @@ const int kPlayoutDelayMaxMs = 40950; struct HeaderExtension { explicit HeaderExtension(RTPExtensionType extension_type) - : type(extension_type), length(0), active(true) { - Init(); - } - - HeaderExtension(RTPExtensionType extension_type, bool active) - : type(extension_type), length(0), active(active) { + : type(extension_type), length(0) { Init(); } @@ -76,7 +71,6 @@ struct HeaderExtension { const RTPExtensionType type; uint8_t length; - bool active; }; class RtpHeaderExtensionMap { @@ -88,44 +82,26 @@ class RtpHeaderExtensionMap { void Erase(); - int32_t Register(const RTPExtensionType type, const uint8_t id); + int32_t Register(RTPExtensionType type, uint8_t id); - // Active on an extension indicates whether it is currently being added on - // on the RTP packets. The active/inactive status on an extension can change - // dynamically depending on the need to convey new information. - int32_t RegisterInactive(const RTPExtensionType type, const uint8_t id); - bool SetActive(const RTPExtensionType type, bool active); - - int32_t Deregister(const RTPExtensionType type); + int32_t Deregister(RTPExtensionType type); bool IsRegistered(RTPExtensionType type) const; - int32_t GetType(const uint8_t id, RTPExtensionType* type) const; + int32_t GetType(uint8_t id, RTPExtensionType* type) const; // Return kInvalidType if not found. RTPExtensionType GetType(uint8_t id) const; int32_t GetId(const RTPExtensionType type, uint8_t* id) const; // Return kInvalidId if not found. uint8_t GetId(RTPExtensionType type) const; - - // - // Methods below ignore any inactive rtp header extensions. - // - size_t GetTotalLengthInBytes() const; - int32_t GetLengthUntilBlockStartInBytes(const RTPExtensionType type) const; - void GetCopy(RtpHeaderExtensionMap* map) const; int32_t Size() const; - RTPExtensionType First() const; - - RTPExtensionType Next(RTPExtensionType type) const; - private: - int32_t Register(const RTPExtensionType type, const uint8_t id, bool active); std::map extensionMap_; }; } // namespace webrtc 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 9ced70da20..50a913efbc 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_header_extension_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_header_extension_unittest.cc @@ -33,12 +33,6 @@ TEST_F(RtpHeaderExtensionTest, Register) { EXPECT_EQ(1, map_.Size()); EXPECT_EQ(0, map_.Deregister(kRtpExtensionTransmissionTimeOffset)); EXPECT_EQ(0, map_.Size()); - - EXPECT_EQ(0, map_.RegisterInactive(kRtpExtensionTransmissionTimeOffset, kId)); - EXPECT_EQ(0, map_.Size()); - EXPECT_TRUE(map_.IsRegistered(kRtpExtensionTransmissionTimeOffset)); - EXPECT_TRUE(map_.SetActive(kRtpExtensionTransmissionTimeOffset, true)); - EXPECT_EQ(1, map_.Size()); } TEST_F(RtpHeaderExtensionTest, RegisterIllegalArg) { @@ -57,32 +51,15 @@ TEST_F(RtpHeaderExtensionTest, Idempotent) { TEST_F(RtpHeaderExtensionTest, NonUniqueId) { EXPECT_EQ(0, map_.Register(kRtpExtensionTransmissionTimeOffset, kId)); EXPECT_EQ(-1, map_.Register(kRtpExtensionAudioLevel, kId)); - EXPECT_EQ(-1, map_.RegisterInactive(kRtpExtensionAudioLevel, kId)); } TEST_F(RtpHeaderExtensionTest, GetTotalLength) { EXPECT_EQ(0u, map_.GetTotalLengthInBytes()); - EXPECT_EQ(0, map_.RegisterInactive(kRtpExtensionTransmissionTimeOffset, kId)); - EXPECT_EQ(0u, map_.GetTotalLengthInBytes()); - EXPECT_EQ(0, map_.Register(kRtpExtensionTransmissionTimeOffset, kId)); EXPECT_EQ(kRtpOneByteHeaderLength + kTransmissionTimeOffsetLength, map_.GetTotalLengthInBytes()); } -TEST_F(RtpHeaderExtensionTest, GetLengthUntilBlockStart) { - EXPECT_EQ(-1, map_.GetLengthUntilBlockStartInBytes( - kRtpExtensionTransmissionTimeOffset)); - EXPECT_EQ(0, map_.RegisterInactive(kRtpExtensionTransmissionTimeOffset, kId)); - EXPECT_EQ(-1, map_.GetLengthUntilBlockStartInBytes( - kRtpExtensionTransmissionTimeOffset)); - - EXPECT_TRUE(map_.SetActive(kRtpExtensionTransmissionTimeOffset, true)); - EXPECT_EQ(static_cast(kRtpOneByteHeaderLength), - map_.GetLengthUntilBlockStartInBytes( - kRtpExtensionTransmissionTimeOffset)); -} - TEST_F(RtpHeaderExtensionTest, GetType) { RTPExtensionType typeOut; EXPECT_EQ(-1, map_.GetType(kId, &typeOut)); @@ -101,27 +78,13 @@ TEST_F(RtpHeaderExtensionTest, GetId) { EXPECT_EQ(kId, idOut); } -TEST_F(RtpHeaderExtensionTest, IterateTypes) { - EXPECT_EQ(kRtpExtensionNone, map_.First()); - EXPECT_EQ(kRtpExtensionNone, map_.Next(kRtpExtensionTransmissionTimeOffset)); - - EXPECT_EQ(0, map_.RegisterInactive(kRtpExtensionTransmissionTimeOffset, kId)); - - EXPECT_EQ(kRtpExtensionNone, map_.First()); - - EXPECT_TRUE(map_.SetActive(kRtpExtensionTransmissionTimeOffset, true)); - - EXPECT_EQ(kRtpExtensionTransmissionTimeOffset, map_.First()); - EXPECT_EQ(kRtpExtensionNone, map_.Next(kRtpExtensionTransmissionTimeOffset)); -} - TEST_F(RtpHeaderExtensionTest, GetCopy) { EXPECT_EQ(0, map_.Register(kRtpExtensionTransmissionTimeOffset, kId)); RtpHeaderExtensionMap mapOut; map_.GetCopy(&mapOut); EXPECT_EQ(1, mapOut.Size()); - EXPECT_EQ(kRtpExtensionTransmissionTimeOffset, mapOut.First()); + EXPECT_EQ(kId, mapOut.GetId(kRtpExtensionTransmissionTimeOffset)); } TEST_F(RtpHeaderExtensionTest, Erase) { diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index 3e364021ab..f3fac77d54 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -92,8 +92,6 @@ RTPSender::RTPSender( payload_type_(-1), payload_type_map_(), rtp_header_extension_map_(), - video_rotation_active_(false), - playout_delay_active_(false), packet_history_(clock), // Statistics rtp_stats_callback_(nullptr), @@ -185,11 +183,7 @@ int32_t RTPSender::RegisterRtpHeaderExtension(RTPExtensionType type, rtc::CritScope lock(&send_critsect_); switch (type) { case kRtpExtensionVideoRotation: - video_rotation_active_ = false; - return rtp_header_extension_map_.RegisterInactive(type, id); case kRtpExtensionPlayoutDelay: - playout_delay_active_ = false; - return rtp_header_extension_map_.RegisterInactive(type, id); case kRtpExtensionTransmissionTimeOffset: case kRtpExtensionAbsoluteSendTime: case kRtpExtensionAudioLevel: @@ -378,16 +372,6 @@ int32_t RTPSender::CheckPayloadType(int8_t payload_type, return 0; } -bool RTPSender::ActivateCVORtpHeaderExtension() { - if (!video_rotation_active_) { - rtc::CritScope lock(&send_critsect_); - if (rtp_header_extension_map_.SetActive(kRtpExtensionVideoRotation, true)) { - video_rotation_active_ = true; - } - } - return video_rotation_active_; -} - bool RTPSender::SendOutgoingData(FrameType frame_type, int8_t payload_type, uint32_t capture_timestamp, @@ -440,18 +424,6 @@ bool RTPSender::SendOutgoingData(FrameType frame_type, sequence_number); } - // Update the active/inactive status of playout delay extension based - // on what the oracle indicates. - { - rtc::CritScope lock(&send_critsect_); - bool send_playout_delay = playout_delay_oracle_.send_playout_delay(); - if (playout_delay_active_ != send_playout_delay) { - playout_delay_active_ = send_playout_delay; - rtp_header_extension_map_.SetActive(kRtpExtensionPlayoutDelay, - playout_delay_active_); - } - } - result = video_->SendVideo(video_type, frame_type, payload_type, rtp_timestamp, capture_time_ms, payload_data, payload_size, fragmentation, rtp_header); @@ -1027,44 +999,6 @@ bool RTPSender::AssignSequenceNumber(RtpPacketToSend* packet) { return true; } -bool RTPSender::FindHeaderExtensionPosition(RTPExtensionType type, - const uint8_t* rtp_packet, - size_t rtp_packet_length, - const RTPHeader& rtp_header, - size_t* position) const { - // Get length until start of header extension block. - int extension_block_pos = - rtp_header_extension_map_.GetLengthUntilBlockStartInBytes(type); - if (extension_block_pos < 0) { - LOG(LS_WARNING) << "Failed to find extension position for " << type - << " as it is not registered."; - return false; - } - - HeaderExtension header_extension(type); - - size_t extension_pos = - kRtpHeaderLength + rtp_header.numCSRCs * sizeof(uint32_t); - size_t block_pos = extension_pos + extension_block_pos; - if (rtp_packet_length < block_pos + header_extension.length || - rtp_header.headerLength < block_pos + header_extension.length) { - LOG(LS_WARNING) << "Failed to find extension position for " << type - << " as the length is invalid."; - return false; - } - - // Verify that header contains extension. - if (!(rtp_packet[extension_pos] == 0xBE && - rtp_packet[extension_pos + 1] == 0xDE)) { - LOG(LS_WARNING) << "Failed to find extension position for " << type - << "as hdr extension not found."; - return false; - } - - *position = block_pos; - return true; -} - bool RTPSender::UpdateTransportSequenceNumber(RtpPacketToSend* packet, int* packet_id) const { RTC_DCHECK(packet); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.h b/webrtc/modules/rtp_rtcp/source/rtp_sender.h index 6b5a6c740a..f31dde9961 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.h @@ -211,7 +211,6 @@ class RTPSender { RtpState GetRtpState() const; void SetRtxRtpState(const RtpState& rtp_state); RtpState GetRtxRtpState() const; - bool ActivateCVORtpHeaderExtension(); protected: int32_t CheckPayloadType(int8_t payload_type, RtpVideoCodecTypes* video_type); @@ -250,24 +249,9 @@ class RTPSender { int64_t capture_time_ms, uint32_t ssrc); - // Find the byte position of the RTP extension as indicated by |type| in - // |rtp_packet|. Return false if such extension doesn't exist. - bool FindHeaderExtensionPosition(RTPExtensionType type, - const uint8_t* rtp_packet, - size_t rtp_packet_length, - const RTPHeader& rtp_header, - size_t* position) const - EXCLUSIVE_LOCKS_REQUIRED(send_critsect_); - bool UpdateTransportSequenceNumber(RtpPacketToSend* packet, int* packet_id) const; - void UpdatePlayoutDelayLimits(uint8_t* rtp_packet, - size_t rtp_packet_length, - const RTPHeader& rtp_header, - uint16_t min_playout_delay, - uint16_t max_playout_delay) const; - void UpdateRtpStats(const RtpPacketToSend& packet, bool is_rtx, bool is_retransmit); @@ -296,13 +280,11 @@ class RTPSender { std::map payload_type_map_; RtpHeaderExtensionMap rtp_header_extension_map_ GUARDED_BY(send_critsect_); - bool video_rotation_active_; // Tracks the current request for playout delay limits from application // and decides whether the current RTP frame should include the playout // delay extension on header. PlayoutDelayOracle playout_delay_oracle_; - bool playout_delay_active_ GUARDED_BY(send_critsect_); RtpPacketHistory packet_history_; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 62fd743d2f..2f664d74fc 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -332,7 +332,6 @@ TEST_F(RtpSenderTestWithoutPacer, RegisterRtpHeaderExtensions) { rtp_sender_->RtpHeaderExtensionLength()); EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( kRtpExtensionVideoRotation, kVideoRotationExtensionId)); - EXPECT_TRUE(rtp_sender_->ActivateCVORtpHeaderExtension()); EXPECT_EQ(RtpUtility::Word32Align(kRtpOneByteHeaderLength + kTransmissionTimeOffsetLength + kAbsoluteSendTimeLength + @@ -365,9 +364,7 @@ TEST_F(RtpSenderTestWithoutPacer, RegisterRtpVideoRotationHeaderExtension) { EXPECT_EQ(0u, rtp_sender_->RtpHeaderExtensionLength()); EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( kRtpExtensionVideoRotation, kVideoRotationExtensionId)); - EXPECT_EQ(0u, rtp_sender_->RtpHeaderExtensionLength()); - EXPECT_TRUE(rtp_sender_->ActivateCVORtpHeaderExtension()); EXPECT_EQ( RtpUtility::Word32Align(kRtpOneByteHeaderLength + kVideoRotationLength), rtp_sender_->RtpHeaderExtensionLength()); @@ -1435,8 +1432,6 @@ TEST_F(RtpSenderVideoTest, SendVideoWithCVO) { EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension( kRtpExtensionVideoRotation, kVideoRotationExtensionId)); - EXPECT_TRUE(rtp_sender_->ActivateCVORtpHeaderExtension()); - EXPECT_EQ( RtpUtility::Word32Align(kRtpOneByteHeaderLength + kVideoRotationLength), rtp_sender_->RtpHeaderExtensionLength()); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc index bb41b83aaa..a4a2b150b4 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -247,13 +247,8 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type, // (e.g. a P-Frame) only if the current value is different from the previous // value sent. // Here we are adding it to every packet of every frame at this point. - if (video_header && video_header->rotation != kVideoRotation_0) { - // TODO(danilchap): Remove next call together with concept - // of inactive extension. Now it helps to calulate total maximum size - // or rtp header extensions that is used in FECPacketOverhead() function. - rtp_sender_->ActivateCVORtpHeaderExtension(); + if (video_header && video_header->rotation != kVideoRotation_0) rtp_header->SetExtension(video_header->rotation); - } size_t packet_capacity = rtp_sender_->MaxPayloadLength() - FecPacketOverhead() -