From fd77b78821ab5021b078a2c06ddea563854f8ca4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Mon, 6 Aug 2018 12:40:58 +0200 Subject: [PATCH] Delete RtpReceiverImpl::CheckPayloadChanged. Also delete related code in RtpReceiverAudio, RtpReceiverVideo and RtpPayloadRegistry. Only intended change in behavior is that packets with unknown payload types are not discarded at this level of the stack. They are discarded higher up, in Channel::ReceivePacket (audio) and RtpVideoStreamReceiver::ReceivePacket (video). Bug: webrtc:8995 Change-Id: I807997120bb40a95b0575c55db6e20a0cac651bf Reviewed-on: https://webrtc-review.googlesource.com/92087 Commit-Queue: Niels Moller Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#24196} --- .../rtp_rtcp/include/rtp_payload_registry.h | 15 ------ .../rtp_rtcp/source/rtp_payload_registry.cc | 14 ++--- .../source/rtp_payload_registry_unittest.cc | 15 ------ modules/rtp_rtcp/source/rtp_receiver_audio.cc | 7 --- modules/rtp_rtcp/source/rtp_receiver_audio.h | 6 --- modules/rtp_rtcp/source/rtp_receiver_impl.cc | 52 ++----------------- modules/rtp_rtcp/source/rtp_receiver_impl.h | 2 - .../rtp_rtcp/source/rtp_receiver_strategy.cc | 7 --- .../rtp_rtcp/source/rtp_receiver_strategy.h | 6 --- 9 files changed, 6 insertions(+), 118 deletions(-) diff --git a/modules/rtp_rtcp/include/rtp_payload_registry.h b/modules/rtp_rtcp/include/rtp_payload_registry.h index be296da957..229c71b0a2 100644 --- a/modules/rtp_rtcp/include/rtp_payload_registry.h +++ b/modules/rtp_rtcp/include/rtp_payload_registry.h @@ -45,20 +45,6 @@ class RTPPayloadRegistry { absl::optional PayloadTypeToPayload( uint8_t payload_type) const; - void ResetLastReceivedPayloadTypes() { - rtc::CritScope cs(&crit_sect_); - last_received_payload_type_ = -1; - } - - int8_t last_received_payload_type() const { - rtc::CritScope cs(&crit_sect_); - return last_received_payload_type_; - } - void set_last_received_payload_type(int8_t last_received_payload_type) { - rtc::CritScope cs(&crit_sect_); - last_received_payload_type_ = last_received_payload_type; - } - private: // Prunes the payload type map of the specific payload type, if it exists. void DeregisterAudioCodecOrRedTypeRegardlessOfPayloadType( @@ -66,7 +52,6 @@ class RTPPayloadRegistry { rtc::CriticalSection crit_sect_; std::map payload_type_map_; - int8_t last_received_payload_type_; // As a first step in splitting this class up in separate cases for audio and // video, DCHECK that no instance is used for both audio and video. diff --git a/modules/rtp_rtcp/source/rtp_payload_registry.cc b/modules/rtp_rtcp/source/rtp_payload_registry.cc index f69940a037..74a7788261 100644 --- a/modules/rtp_rtcp/source/rtp_payload_registry.cc +++ b/modules/rtp_rtcp/source/rtp_payload_registry.cc @@ -91,7 +91,7 @@ bool IsPayloadTypeValid(int8_t payload_type) { } // namespace -RTPPayloadRegistry::RTPPayloadRegistry() : last_received_payload_type_(-1) {} +RTPPayloadRegistry::RTPPayloadRegistry() = default; RTPPayloadRegistry::~RTPPayloadRegistry() = default; @@ -112,10 +112,6 @@ void RTPPayloadRegistry::SetAudioReceivePayloads( payload_type_map_.emplace(rtp_payload_type, CreatePayloadType(audio_format)); } - - // Clear the value of last received payload type since it might mean - // something else now. - last_received_payload_type_ = -1; } int32_t RTPPayloadRegistry::RegisterReceivePayload( @@ -153,9 +149,7 @@ int32_t RTPPayloadRegistry::RegisterReceivePayload( RTC_DCHECK(insert_status.second); // Insertion succeeded. *created_new_payload = true; - // Successful set of payload type, clear the value of last received payload - // type since it might mean something else. - last_received_payload_type_ = -1; + // Successful set of payload type. return 0; } @@ -186,9 +180,7 @@ int32_t RTPPayloadRegistry::RegisterReceivePayload( video_codec.plType, CreatePayloadType(video_codec)); RTC_DCHECK(insert_status.second); // Insertion succeeded. - // Successful set of payload type, clear the value of last received payload - // type since it might mean something else. - last_received_payload_type_ = -1; + // Successful set of payload type. return 0; } diff --git a/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc b/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc index e7362c7553..9ae6aef027 100644 --- a/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc @@ -149,21 +149,6 @@ TEST(RtpPayloadRegistryTest, << "Not compatible; both payloads should be kept."; } -TEST(RtpPayloadRegistryTest, - LastReceivedCodecTypesAreResetWhenRegisteringNewPayloadTypes) { - RTPPayloadRegistry rtp_payload_registry; - rtp_payload_registry.set_last_received_payload_type(17); - EXPECT_EQ(17, rtp_payload_registry.last_received_payload_type()); - - bool ignored; - constexpr int payload_type = 34; - const SdpAudioFormat audio_format("name", 44000, 1); - EXPECT_EQ(0, rtp_payload_registry.RegisterReceivePayload( - payload_type, audio_format, &ignored)); - - EXPECT_EQ(-1, rtp_payload_registry.last_received_payload_type()); -} - class ParameterizedRtpPayloadRegistryTest : public ::testing::TestWithParam {}; diff --git a/modules/rtp_rtcp/source/rtp_receiver_audio.cc b/modules/rtp_rtcp/source/rtp_receiver_audio.cc index 434a53e0e6..eac50e2877 100644 --- a/modules/rtp_rtcp/source/rtp_receiver_audio.cc +++ b/modules/rtp_rtcp/source/rtp_receiver_audio.cc @@ -130,13 +130,6 @@ RTPAliveType RTPReceiverAudio::ProcessDeadOrAlive( } } -void RTPReceiverAudio::CheckPayloadChanged(int8_t payload_type, - PayloadUnion* /* specific_payload */, - bool* should_discard_changes) { - *should_discard_changes = - TelephoneEventPayloadType(payload_type) || CNGPayloadType(payload_type); -} - // We are not allowed to have any critsects when calling data_callback. int32_t RTPReceiverAudio::ParseAudioCodecSpecific( WebRtcRTPHeader* rtp_header, diff --git a/modules/rtp_rtcp/source/rtp_receiver_audio.h b/modules/rtp_rtcp/source/rtp_receiver_audio.h index e582446ae8..b4304bc307 100644 --- a/modules/rtp_rtcp/source/rtp_receiver_audio.h +++ b/modules/rtp_rtcp/source/rtp_receiver_audio.h @@ -41,12 +41,6 @@ class RTPReceiverAudio : public RTPReceiverStrategy { int32_t OnNewPayloadTypeCreated(int payload_type, const SdpAudioFormat& audio_format) override; - // We need to look out for special payload types here and sometimes reset - // statistics. In addition we sometimes need to tweak the frequency. - void CheckPayloadChanged(int8_t payload_type, - PayloadUnion* specific_payload, - bool* should_discard_changes) override; - private: int32_t ParseAudioCodecSpecific(WebRtcRTPHeader* rtp_header, const uint8_t* payload_data, diff --git a/modules/rtp_rtcp/source/rtp_receiver_impl.cc b/modules/rtp_rtcp/source/rtp_receiver_impl.cc index 1cdbfdf447..54ac6ea854 100644 --- a/modules/rtp_rtcp/source/rtp_receiver_impl.cc +++ b/modules/rtp_rtcp/source/rtp_receiver_impl.cc @@ -150,15 +150,10 @@ bool RtpReceiverImpl::IncomingRtpPacket(const RTPHeader& rtp_header, // Trigger our callbacks. CheckSSRCChanged(rtp_header); - if (CheckPayloadChanged(rtp_header, &payload_specific) == -1) { - if (payload_length == 0) { - // OK, keep-alive packet. - return true; - } - RTC_LOG(LS_WARNING) << "Receiving invalid payload type."; - return false; + if (payload_length == 0) { + // OK, keep-alive packet. + return true; } - WebRtcRTPHeader webrtc_rtp_header{}; webrtc_rtp_header.header = rtp_header; CheckCSRC(webrtc_rtp_header); @@ -248,47 +243,6 @@ void RtpReceiverImpl::CheckSSRCChanged(const RTPHeader& rtp_header) { ssrc_ = rtp_header.ssrc; } -// Implementation note: must not hold critsect when called. -// TODO(phoglund): Move as much as possible of this code path into the media -// specific receivers. Basically this method goes through a lot of trouble to -// compute something which is only used by the media specific parts later. If -// this code path moves we can get rid of some of the rtp_receiver -> -// media_specific interface (such as CheckPayloadChange, possibly get/set -// last known payload). -int32_t RtpReceiverImpl::CheckPayloadChanged(const RTPHeader& rtp_header, - PayloadUnion* specific_payload) { - int8_t payload_type = rtp_header.payloadType; - - { - rtc::CritScope lock(&critical_section_rtp_receiver_); - - int8_t last_received_payload_type = - rtp_payload_registry_->last_received_payload_type(); - // TODO(holmer): Remove this code when RED parsing has been broken out from - // RtpReceiverAudio. - if (payload_type != last_received_payload_type) { - bool should_discard_changes = false; - - rtp_media_receiver_->CheckPayloadChanged(payload_type, specific_payload, - &should_discard_changes); - - if (should_discard_changes) { - return 0; - } - - const auto payload = - rtp_payload_registry_->PayloadTypeToPayload(payload_type); - if (!payload) { - // Not a registered payload type. - return -1; - } - rtp_payload_registry_->set_last_received_payload_type(payload_type); - } - } // End critsect. - - return 0; -} - // Implementation note: must not hold critsect when called. void RtpReceiverImpl::CheckCSRC(const WebRtcRTPHeader& rtp_header) { const uint8_t num_csrcs = rtp_header.header.numCSRCs; diff --git a/modules/rtp_rtcp/source/rtp_receiver_impl.h b/modules/rtp_rtcp/source/rtp_receiver_impl.h index 55a57eaa8d..d898d7371e 100644 --- a/modules/rtp_rtcp/source/rtp_receiver_impl.h +++ b/modules/rtp_rtcp/source/rtp_receiver_impl.h @@ -66,8 +66,6 @@ class RtpReceiverImpl : public RtpReceiver { private: void CheckSSRCChanged(const RTPHeader& rtp_header); void CheckCSRC(const WebRtcRTPHeader& rtp_header); - int32_t CheckPayloadChanged(const RTPHeader& rtp_header, - PayloadUnion* payload); void UpdateSources(const absl::optional& ssrc_audio_level); void RemoveOutdatedSources(int64_t now_ms); diff --git a/modules/rtp_rtcp/source/rtp_receiver_strategy.cc b/modules/rtp_rtcp/source/rtp_receiver_strategy.cc index bcce3e3907..647eceaf22 100644 --- a/modules/rtp_rtcp/source/rtp_receiver_strategy.cc +++ b/modules/rtp_rtcp/source/rtp_receiver_strategy.cc @@ -19,11 +19,4 @@ RTPReceiverStrategy::RTPReceiverStrategy(RtpData* data_callback) RTPReceiverStrategy::~RTPReceiverStrategy() = default; -void RTPReceiverStrategy::CheckPayloadChanged(int8_t payload_type, - PayloadUnion* specific_payload, - bool* should_discard_changes) { - // Default: Keep changes. - *should_discard_changes = false; -} - } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_receiver_strategy.h b/modules/rtp_rtcp/source/rtp_receiver_strategy.h index 3a8d200624..fc3ee8d22d 100644 --- a/modules/rtp_rtcp/source/rtp_receiver_strategy.h +++ b/modules/rtp_rtcp/source/rtp_receiver_strategy.h @@ -50,12 +50,6 @@ class RTPReceiverStrategy { int payload_type, const SdpAudioFormat& audio_format) = 0; - // Checks if the payload type has changed, and returns whether we should - // reset statistics and/or discard this packet. - virtual void CheckPayloadChanged(int8_t payload_type, - PayloadUnion* specific_payload, - bool* should_discard_changes); - protected: // The data callback is where we should send received payload data. // See ParseRtpPacket. This class does not claim ownership of the callback.