From f39c815a1d65c6b1c387a4f33f92bfc5fd143251 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Mon, 14 Oct 2019 17:32:21 +0200 Subject: [PATCH] Cleanup: Replacing set extension status bool with CHECK. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was just checked in all places were it was used, moving the check into RtpRtcp reduces the boiler plate required at the call sites. Also changing to always register and unregister extensions by URI to synchronize the code in AudioSendStream with the code in RtpVideoSender. This prepares for reducing the scope of ChannelSend. Bug: webrtc:9883 Change-Id: Ia64d79f20eb98f46cbbbe8318770e4fcf9caa1ec Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/155620 Reviewed-by: Erik Språng Reviewed-by: Oskar Sundbom Commit-Queue: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#29490} --- audio/audio_send_stream.cc | 4 +-- audio/channel_send.cc | 36 +++++++------------ call/rtp_video_sender.cc | 2 +- .../include/rtp_header_extension_map.h | 3 +- modules/rtp_rtcp/include/rtp_rtcp.h | 10 +++--- modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 4 +-- .../source/rtp_header_extension_map.cc | 11 +++++- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 9 +++-- modules/rtp_rtcp/source/rtp_rtcp_impl.h | 4 +-- modules/rtp_rtcp/source/rtp_sender.cc | 8 ++++- modules/rtp_rtcp/source/rtp_sender.h | 3 +- 11 files changed, 53 insertions(+), 41 deletions(-) diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc index e86667ded7..3fd8a8afe6 100644 --- a/audio/audio_send_stream.cc +++ b/audio/audio_send_stream.cc @@ -296,8 +296,8 @@ void AudioSendStream::ConfigureStream( channel_send_->GetRtpRtcp()->DeregisterSendRtpHeaderExtension( kRtpExtensionAbsoluteSendTime); if (new_ids.abs_send_time) { - channel_send_->GetRtpRtcp()->RegisterSendRtpHeaderExtension( - kRtpExtensionAbsoluteSendTime, new_ids.abs_send_time); + rtp_rtcp_module_->RegisterRtpHeaderExtension(AbsoluteSendTime::kUri, + new_ids.abs_send_time); } } diff --git a/audio/channel_send.cc b/audio/channel_send.cc index f803bf9f63..569615bad6 100644 --- a/audio/channel_send.cc +++ b/audio/channel_send.cc @@ -189,7 +189,7 @@ class ChannelSend : public ChannelSendInterface, void OnUplinkPacketLossRate(float packet_loss_rate); bool InputMute() const; - int SetSendRtpHeaderExtension(bool enable, RTPExtensionType type, int id); + void SetSendRtpHeaderExtension(bool enable, absl::string_view uri, int id); int32_t SendRtpAudio(AudioFrameType frameType, uint8_t payloadType, @@ -894,22 +894,18 @@ void ChannelSend::SetRid(const std::string& rid, int repaired_extension_id) { RTC_DCHECK_RUN_ON(&worker_thread_checker_); if (extension_id != 0) { - int ret = SetSendRtpHeaderExtension(!rid.empty(), kRtpExtensionRtpStreamId, - extension_id); - RTC_DCHECK_EQ(0, ret); + SetSendRtpHeaderExtension(!rid.empty(), RtpStreamId::kUri, extension_id); } if (repaired_extension_id != 0) { - int ret = SetSendRtpHeaderExtension(!rid.empty(), kRtpExtensionRtpStreamId, - repaired_extension_id); - RTC_DCHECK_EQ(0, ret); + SetSendRtpHeaderExtension(!rid.empty(), RtpStreamId::kUri, + repaired_extension_id); } _rtpRtcpModule->SetRid(rid); } void ChannelSend::SetMid(const std::string& mid, int extension_id) { RTC_DCHECK_RUN_ON(&worker_thread_checker_); - int ret = SetSendRtpHeaderExtension(true, kRtpExtensionMid, extension_id); - RTC_DCHECK_EQ(0, ret); + SetSendRtpHeaderExtension(true, RtpMid::kUri, extension_id); _rtpRtcpModule->SetMid(mid); } @@ -921,15 +917,12 @@ void ChannelSend::SetExtmapAllowMixed(bool extmap_allow_mixed) { void ChannelSend::SetSendAudioLevelIndicationStatus(bool enable, int id) { RTC_DCHECK_RUN_ON(&worker_thread_checker_); _includeAudioLevelIndication = enable; - int ret = SetSendRtpHeaderExtension(enable, kRtpExtensionAudioLevel, id); - RTC_DCHECK_EQ(0, ret); + SetSendRtpHeaderExtension(enable, AudioLevel::kUri, id); } void ChannelSend::EnableSendTransportSequenceNumber(int id) { RTC_DCHECK_RUN_ON(&worker_thread_checker_); - int ret = - SetSendRtpHeaderExtension(true, kRtpExtensionTransportSequenceNumber, id); - RTC_DCHECK_EQ(0, ret); + SetSendRtpHeaderExtension(true, TransportSequenceNumber::kUri, id); } void ChannelSend::RegisterSenderCongestionControlObjects( @@ -1093,18 +1086,13 @@ RtpRtcp* ChannelSend::GetRtpRtcp() const { return _rtpRtcpModule.get(); } -int ChannelSend::SetSendRtpHeaderExtension(bool enable, - RTPExtensionType type, - int id) { - int error = 0; - _rtpRtcpModule->DeregisterSendRtpHeaderExtension(type); +void ChannelSend::SetSendRtpHeaderExtension(bool enable, + absl::string_view uri, + int id) { + _rtpRtcpModule->DeregisterSendRtpHeaderExtension(uri); if (enable) { - // TODO(nisse): RtpRtcp::RegisterSendRtpHeaderExtension to take an int - // argument. Currently it wants an uint8_t. - error = _rtpRtcpModule->RegisterSendRtpHeaderExtension( - type, rtc::dchecked_cast(id)); + _rtpRtcpModule->RegisterRtpHeaderExtension(uri, id); } - return error; } int64_t ChannelSend::GetRTT() const { diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 73e356d3e8..2b2a0a0dce 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -381,7 +381,7 @@ RtpVideoSender::RtpVideoSender( int id = rtp_config_.extensions[i].id; RTC_DCHECK(RtpExtension::IsSupportedForVideo(extension)); for (const RtpStreamSender& stream : rtp_streams_) { - RTC_CHECK(stream.rtp_rtcp->RegisterRtpHeaderExtension(extension, id)); + stream.rtp_rtcp->RegisterRtpHeaderExtension(extension, id); } } diff --git a/modules/rtp_rtcp/include/rtp_header_extension_map.h b/modules/rtp_rtcp/include/rtp_header_extension_map.h index e945ce89ba..360a619f82 100644 --- a/modules/rtp_rtcp/include/rtp_header_extension_map.h +++ b/modules/rtp_rtcp/include/rtp_header_extension_map.h @@ -37,7 +37,7 @@ class RtpHeaderExtensionMap { return Register(id, Extension::kId, Extension::kUri); } bool RegisterByType(int id, RTPExtensionType type); - bool RegisterByUri(int id, const std::string& uri); + bool RegisterByUri(int id, absl::string_view uri); bool IsRegistered(RTPExtensionType type) const { return GetId(type) != kInvalidId; @@ -56,6 +56,7 @@ class RtpHeaderExtensionMap { return RegisterByType(id, type) ? 0 : -1; } int32_t Deregister(RTPExtensionType type); + void Deregister(absl::string_view uri); // Corresponds to the SDP attribute extmap-allow-mixed, see RFC8285. // Set to true if it's allowed to mix one- and two-byte RTP header extensions diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index 69ca8f81b3..efb216e8df 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -164,12 +164,14 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { // (De)registers RTP header extension type and id. // Returns -1 on failure else 0. - virtual int32_t RegisterSendRtpHeaderExtension(RTPExtensionType type, - uint8_t id) = 0; - // Register extension by uri, returns false on failure. - virtual bool RegisterRtpHeaderExtension(const std::string& uri, int id) = 0; + RTC_DEPRECATED virtual int32_t RegisterSendRtpHeaderExtension( + RTPExtensionType type, + uint8_t id) = 0; + // Register extension by uri, triggers CHECK on falure. + virtual void RegisterRtpHeaderExtension(absl::string_view uri, int id) = 0; virtual int32_t DeregisterSendRtpHeaderExtension(RTPExtensionType type) = 0; + virtual void DeregisterSendRtpHeaderExtension(absl::string_view uri) = 0; // Returns true if RTP module is send media, and any of the extensions // required for bandwidth estimation is registered. diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index bf280f3239..3b64de7fe3 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -45,10 +45,10 @@ class MockRtpRtcp : public RtpRtcp { MOCK_METHOD1(SetExtmapAllowMixed, void(bool extmap_allow_mixed)); MOCK_METHOD2(RegisterSendRtpHeaderExtension, int32_t(RTPExtensionType type, uint8_t id)); - MOCK_METHOD2(RegisterRtpHeaderExtension, - bool(const std::string& uri, int id)); + MOCK_METHOD2(RegisterRtpHeaderExtension, void(absl::string_view uri, int id)); MOCK_METHOD1(DeregisterSendRtpHeaderExtension, int32_t(RTPExtensionType type)); + MOCK_METHOD1(DeregisterSendRtpHeaderExtension, void(absl::string_view uri)); MOCK_CONST_METHOD0(SupportsPadding, bool()); MOCK_CONST_METHOD0(SupportsRtxPayloadPadding, bool()); MOCK_CONST_METHOD0(StartTimestamp, uint32_t()); diff --git a/modules/rtp_rtcp/source/rtp_header_extension_map.cc b/modules/rtp_rtcp/source/rtp_header_extension_map.cc index 3ff6f45103..dbcdff6e4e 100644 --- a/modules/rtp_rtcp/source/rtp_header_extension_map.cc +++ b/modules/rtp_rtcp/source/rtp_header_extension_map.cc @@ -85,7 +85,7 @@ bool RtpHeaderExtensionMap::RegisterByType(int id, RTPExtensionType type) { return false; } -bool RtpHeaderExtensionMap::RegisterByUri(int id, const std::string& uri) { +bool RtpHeaderExtensionMap::RegisterByUri(int id, absl::string_view uri) { for (const ExtensionInfo& extension : kExtensions) if (uri == extension.uri) return Register(id, extension.type, extension.uri); @@ -113,6 +113,15 @@ int32_t RtpHeaderExtensionMap::Deregister(RTPExtensionType type) { return 0; } +void RtpHeaderExtensionMap::Deregister(absl::string_view uri) { + for (const ExtensionInfo& extension : kExtensions) { + if (extension.uri == uri) { + ids_[extension.type] = kInvalidId; + break; + } + } +} + bool RtpHeaderExtensionMap::Register(int id, RTPExtensionType type, const char* uri) { diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 7d8e33868a..b96a56c0ac 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -538,15 +538,20 @@ int32_t ModuleRtpRtcpImpl::RegisterSendRtpHeaderExtension( return rtp_sender_->RegisterRtpHeaderExtension(type, id); } -bool ModuleRtpRtcpImpl::RegisterRtpHeaderExtension(const std::string& uri, +void ModuleRtpRtcpImpl::RegisterRtpHeaderExtension(absl::string_view uri, int id) { - return rtp_sender_->RegisterRtpHeaderExtension(uri, id); + bool registered = rtp_sender_->RegisterRtpHeaderExtension(uri, id); + RTC_CHECK(registered); } int32_t ModuleRtpRtcpImpl::DeregisterSendRtpHeaderExtension( const RTPExtensionType type) { return rtp_sender_->DeregisterRtpHeaderExtension(type); } +void ModuleRtpRtcpImpl::DeregisterSendRtpHeaderExtension( + absl::string_view uri) { + rtp_sender_->DeregisterRtpHeaderExtension(uri); +} // (TMMBR) Temporary Max Media Bit Rate. bool ModuleRtpRtcpImpl::TMMBR() const { diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 9ec481c842..2793945ca9 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -71,9 +71,9 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { // Register RTP header extension. int32_t RegisterSendRtpHeaderExtension(RTPExtensionType type, uint8_t id) override; - bool RegisterRtpHeaderExtension(const std::string& uri, int id) override; - + void RegisterRtpHeaderExtension(absl::string_view uri, int id) override; int32_t DeregisterSendRtpHeaderExtension(RTPExtensionType type) override; + void DeregisterSendRtpHeaderExtension(absl::string_view uri) override; bool SupportsPadding() const override; bool SupportsRtxPayloadPadding() const override; diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index c88e0e20b0..9c3dbd60a5 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -227,7 +227,7 @@ int32_t RTPSender::RegisterRtpHeaderExtension(RTPExtensionType type, return registered ? 0 : -1; } -bool RTPSender::RegisterRtpHeaderExtension(const std::string& uri, int id) { +bool RTPSender::RegisterRtpHeaderExtension(absl::string_view uri, int id) { rtc::CritScope lock(&send_critsect_); bool registered = rtp_header_extension_map_.RegisterByUri(id, uri); supports_bwe_extension_ = HasBweExtension(rtp_header_extension_map_); @@ -246,6 +246,12 @@ int32_t RTPSender::DeregisterRtpHeaderExtension(RTPExtensionType type) { return deregistered; } +void RTPSender::DeregisterRtpHeaderExtension(absl::string_view uri) { + rtc::CritScope lock(&send_critsect_); + rtp_header_extension_map_.Deregister(uri); + supports_bwe_extension_ = HasBweExtension(rtp_header_extension_map_); +} + void RTPSender::SetMaxRtpPacketSize(size_t max_packet_size) { RTC_DCHECK_GE(max_packet_size, 100); RTC_DCHECK_LE(max_packet_size, IP_PACKET_SIZE); diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index d0a8396973..5489217841 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -85,9 +85,10 @@ class RTPSender { // RTP header extension int32_t RegisterRtpHeaderExtension(RTPExtensionType type, uint8_t id); - bool RegisterRtpHeaderExtension(const std::string& uri, int id); + bool RegisterRtpHeaderExtension(absl::string_view uri, int id); bool IsRtpHeaderExtensionRegistered(RTPExtensionType type) const; int32_t DeregisterRtpHeaderExtension(RTPExtensionType type); + void DeregisterRtpHeaderExtension(absl::string_view uri); // Tries to send packet to transport. Also updates any timing extensions, // calls observers waiting for packet send events, and updates stats.