From 73b60b82ee791e1b9df403680bfbccc03439b267 Mon Sep 17 00:00:00 2001 From: Karl Wiberg Date: Thu, 21 Sep 2017 15:00:58 +0200 Subject: [PATCH] Remove the redundant method GetPayloadSpecifics It's in the way of a refactoring. Also change PayloadTypeToPayload---the method all callers can use instead---to return Optional instead of const Payload* (for thread safety reasons: an object that protects itself with a mutex shouldn't be handing out pointers to parts of itself). BUG=webrtc:8159 Change-Id: I7ef0d545077ffdea016b309f2165e3c4955a2928 Reviewed-on: https://webrtc-review.googlesource.com/2360 Commit-Queue: Karl Wiberg Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#19917} --- .../rtp_rtcp/include/rtp_payload_registry.h | 6 ++-- .../rtp_rtcp/source/rtp_payload_registry.cc | 30 ++++-------------- .../source/rtp_payload_registry_unittest.cc | 31 ++++++++++--------- modules/rtp_rtcp/source/rtp_receiver_impl.cc | 4 +-- modules/rtp_rtcp/test/testAPI/test_api.cc | 8 ++--- .../rtp_rtcp/test/testAPI/test_api_video.cc | 11 +++---- video/rtp_video_stream_receiver.cc | 19 ++++++------ voice_engine/channel.cc | 8 ++--- 8 files changed, 49 insertions(+), 68 deletions(-) diff --git a/modules/rtp_rtcp/include/rtp_payload_registry.h b/modules/rtp_rtcp/include/rtp_payload_registry.h index 6212802a62..8c4d5970c0 100644 --- a/modules/rtp_rtcp/include/rtp_payload_registry.h +++ b/modules/rtp_rtcp/include/rtp_payload_registry.h @@ -15,6 +15,7 @@ #include #include "api/audio_codecs/audio_format.h" +#include "api/optional.h" #include "modules/rtp_rtcp/source/rtp_utility.h" #include "rtc_base/criticalsection.h" @@ -55,11 +56,10 @@ class RTPPayloadRegistry { bool IsRed(const RTPHeader& header) const; - bool GetPayloadSpecifics(uint8_t payload_type, PayloadUnion* payload) const; - int GetPayloadTypeFrequency(uint8_t payload_type) const; - const RtpUtility::Payload* PayloadTypeToPayload(uint8_t payload_type) const; + rtc::Optional PayloadTypeToPayload( + uint8_t payload_type) const; void ResetLastReceivedPayloadTypes() { rtc::CritScope cs(&crit_sect_); diff --git a/modules/rtp_rtcp/source/rtp_payload_registry.cc b/modules/rtp_rtcp/source/rtp_payload_registry.cc index 6619f5c28e..fcd0276c01 100644 --- a/modules/rtp_rtcp/source/rtp_payload_registry.cc +++ b/modules/rtp_rtcp/source/rtp_payload_registry.cc @@ -303,22 +303,9 @@ bool RTPPayloadRegistry::IsRed(const RTPHeader& header) const { return it != payload_type_map_.end() && _stricmp(it->second.name, "red") == 0; } -bool RTPPayloadRegistry::GetPayloadSpecifics(uint8_t payload_type, - PayloadUnion* payload) const { - rtc::CritScope cs(&crit_sect_); - auto it = payload_type_map_.find(payload_type); - - // Check that this is a registered payload type. - if (it == payload_type_map_.end()) { - return false; - } - *payload = it->second.typeSpecific; - return true; -} - int RTPPayloadRegistry::GetPayloadTypeFrequency( uint8_t payload_type) const { - const RtpUtility::Payload* payload = PayloadTypeToPayload(payload_type); + const auto payload = PayloadTypeToPayload(payload_type); if (!payload) { return -1; } @@ -327,18 +314,13 @@ int RTPPayloadRegistry::GetPayloadTypeFrequency( : kVideoPayloadTypeFrequency; } -const RtpUtility::Payload* RTPPayloadRegistry::PayloadTypeToPayload( +rtc::Optional RTPPayloadRegistry::PayloadTypeToPayload( uint8_t payload_type) const { rtc::CritScope cs(&crit_sect_); - - auto it = payload_type_map_.find(payload_type); - - // Check that this is a registered payload type. - if (it == payload_type_map_.end()) { - return nullptr; - } - - return &it->second; + const auto it = payload_type_map_.find(payload_type); + return it == payload_type_map_.end() + ? rtc::Optional() + : rtc::Optional(it->second); } void RTPPayloadRegistry::SetIncomingPayloadType(const RTPHeader& header) { diff --git a/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc b/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc index d5fa13c42f..8a1db7ab95 100644 --- a/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc @@ -42,7 +42,7 @@ TEST(RtpPayloadRegistryTest, EXPECT_EQ(0, rtp_payload_registry.RegisterReceivePayload(video_codec)); - const RtpUtility::Payload* retrieved_payload = + const auto retrieved_payload = rtp_payload_registry.PayloadTypeToPayload(payload_type); EXPECT_TRUE(retrieved_payload); @@ -68,7 +68,7 @@ TEST(RtpPayloadRegistryTest, EXPECT_TRUE(new_payload_created) << "A new payload WAS created."; - const RtpUtility::Payload* retrieved_payload = + const auto retrieved_payload = rtp_payload_registry.PayloadTypeToPayload(payload_type); EXPECT_TRUE(retrieved_payload); @@ -102,7 +102,7 @@ TEST(RtpPayloadRegistryTest, AudioRedWorkProperly) { EXPECT_EQ(kRedPayloadType, rtp_payload_registry.red_payload_type()); - const RtpUtility::Payload* retrieved_payload = + const auto retrieved_payload = rtp_payload_registry.PayloadTypeToPayload(kRedPayloadType); EXPECT_TRUE(retrieved_payload); EXPECT_TRUE(retrieved_payload->audio); @@ -140,22 +140,23 @@ TEST(RtpPayloadRegistryTest, << "With a different payload type is fine though."; // Ensure both payloads are preserved. - const RtpUtility::Payload* retrieved_payload = + const auto retrieved_payload1 = rtp_payload_registry.PayloadTypeToPayload(payload_type); - EXPECT_TRUE(retrieved_payload); - EXPECT_STREQ(kTypicalPayloadName, retrieved_payload->name); - EXPECT_TRUE(retrieved_payload->audio); - EXPECT_EQ(kTypicalFrequency, retrieved_payload->typeSpecific.Audio.frequency); - EXPECT_EQ(kTypicalChannels, retrieved_payload->typeSpecific.Audio.channels); + EXPECT_TRUE(retrieved_payload1); + EXPECT_STREQ(kTypicalPayloadName, retrieved_payload1->name); + EXPECT_TRUE(retrieved_payload1->audio); + EXPECT_EQ(kTypicalFrequency, + retrieved_payload1->typeSpecific.Audio.frequency); + EXPECT_EQ(kTypicalChannels, retrieved_payload1->typeSpecific.Audio.channels); - retrieved_payload = + const auto retrieved_payload2 = rtp_payload_registry.PayloadTypeToPayload(payload_type - 1); - EXPECT_TRUE(retrieved_payload); - EXPECT_STREQ(kTypicalPayloadName, retrieved_payload->name); - EXPECT_TRUE(retrieved_payload->audio); + EXPECT_TRUE(retrieved_payload2); + EXPECT_STREQ(kTypicalPayloadName, retrieved_payload2->name); + EXPECT_TRUE(retrieved_payload2->audio); EXPECT_EQ(kTypicalFrequency + 1, - retrieved_payload->typeSpecific.Audio.frequency); - EXPECT_EQ(kTypicalChannels, retrieved_payload->typeSpecific.Audio.channels); + retrieved_payload2->typeSpecific.Audio.frequency); + EXPECT_EQ(kTypicalChannels, retrieved_payload2->typeSpecific.Audio.channels); // Ok, update the rate for one of the codecs. If either the incoming rate or // the stored rate is zero it's not really an error to register the same diff --git a/modules/rtp_rtcp/source/rtp_receiver_impl.cc b/modules/rtp_rtcp/source/rtp_receiver_impl.cc index dd04614526..040ece3ec3 100644 --- a/modules/rtp_rtcp/source/rtp_receiver_impl.cc +++ b/modules/rtp_rtcp/source/rtp_receiver_impl.cc @@ -294,7 +294,7 @@ void RtpReceiverImpl::CheckSSRCChanged(const RTPHeader& rtp_header) { if (rtp_header.payloadType == last_received_payload_type) { re_initialize_decoder = true; - const Payload* payload = rtp_payload_registry_->PayloadTypeToPayload( + const auto payload = rtp_payload_registry_->PayloadTypeToPayload( rtp_header.payloadType); if (!payload) { return; @@ -382,7 +382,7 @@ int32_t RtpReceiverImpl::CheckPayloadChanged(const RTPHeader& rtp_header, return 0; } - const Payload* payload = + const auto payload = rtp_payload_registry_->PayloadTypeToPayload(payload_type); if (!payload) { // Not a registered payload type. diff --git a/modules/rtp_rtcp/test/testAPI/test_api.cc b/modules/rtp_rtcp/test/testAPI/test_api.cc index 61375e9284..c247fc704a 100644 --- a/modules/rtp_rtcp/test/testAPI/test_api.cc +++ b/modules/rtp_rtcp/test/testAPI/test_api.cc @@ -48,9 +48,9 @@ bool LoopBackTransport::SendRtp(const uint8_t* data, if (!parser->Parse(data, len, &header)) { return false; } - PayloadUnion payload_specific; - if (!rtp_payload_registry_->GetPayloadSpecifics(header.payloadType, - &payload_specific)) { + const auto pl = + rtp_payload_registry_->PayloadTypeToPayload(header.payloadType); + if (!pl) { return false; } const uint8_t* payload = data + header.headerLength; @@ -58,7 +58,7 @@ bool LoopBackTransport::SendRtp(const uint8_t* data, const size_t payload_length = len - header.headerLength; receive_statistics_->IncomingPacket(header, len, false); return rtp_receiver_->IncomingRtpPacket(header, payload, payload_length, - payload_specific, true); + pl->typeSpecific, true); } bool LoopBackTransport::SendRtcp(const uint8_t* data, size_t len) { diff --git a/modules/rtp_rtcp/test/testAPI/test_api_video.cc b/modules/rtp_rtcp/test/testAPI/test_api_video.cc index 37bd9748ef..26964fdba5 100644 --- a/modules/rtp_rtcp/test/testAPI/test_api_video.cc +++ b/modules/rtp_rtcp/test/testAPI/test_api_video.cc @@ -165,14 +165,13 @@ TEST_F(RtpRtcpVideoTest, PaddingOnlyFrames) { RTPHeader header; std::unique_ptr parser(RtpHeaderParser::Create()); EXPECT_TRUE(parser->Parse(padding_packet, packet_size, &header)); - PayloadUnion payload_specific; - EXPECT_TRUE(rtp_payload_registry_.GetPayloadSpecifics(header.payloadType, - &payload_specific)); + const auto pl = + rtp_payload_registry_.PayloadTypeToPayload(header.payloadType); + EXPECT_TRUE(pl); const uint8_t* payload = padding_packet + header.headerLength; const size_t payload_length = packet_size - header.headerLength; - EXPECT_TRUE(rtp_receiver_->IncomingRtpPacket(header, payload, - payload_length, - payload_specific, true)); + EXPECT_TRUE(rtp_receiver_->IncomingRtpPacket( + header, payload, payload_length, pl->typeSpecific, true)); EXPECT_EQ(0u, receiver_->payload_size()); EXPECT_EQ(payload_length, receiver_->rtp_header().header.paddingLength); } diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index 8611721aac..7a65d8bb41 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -460,13 +460,12 @@ void RtpVideoStreamReceiver::ReceivePacket(const uint8_t* packet, const uint8_t* payload = packet + header.headerLength; assert(packet_length >= header.headerLength); size_t payload_length = packet_length - header.headerLength; - PayloadUnion payload_specific; - if (!rtp_payload_registry_.GetPayloadSpecifics(header.payloadType, - &payload_specific)) { - return; + const auto pl = + rtp_payload_registry_.PayloadTypeToPayload(header.payloadType); + if (pl) { + rtp_receiver_->IncomingRtpPacket(header, payload, payload_length, + pl->typeSpecific, in_order); } - rtp_receiver_->IncomingRtpPacket(header, payload, payload_length, - payload_specific, in_order); } void RtpVideoStreamReceiver::ParseAndHandleEncapsulatingHeader( @@ -501,13 +500,13 @@ void RtpVideoStreamReceiver::NotifyReceiverOfFecPacket( rtp_header.header = header; rtp_header.header.payloadType = last_media_payload_type; rtp_header.header.paddingLength = 0; - PayloadUnion payload_specific; - if (!rtp_payload_registry_.GetPayloadSpecifics(last_media_payload_type, - &payload_specific)) { + const auto pl = + rtp_payload_registry_.PayloadTypeToPayload(last_media_payload_type); + if (!pl) { LOG(LS_WARNING) << "Failed to get payload specifics."; return; } - rtp_header.type.Video.codec = payload_specific.Video.videoCodecType; + rtp_header.type.Video.codec = pl->typeSpecific.Video.videoCodecType; rtp_header.type.Video.rotation = kVideoRotation_0; if (header.extension.hasVideoRotation) { rtp_header.type.Video.rotation = header.extension.videoRotation; diff --git a/voice_engine/channel.cc b/voice_engine/channel.cc index e95a468d4d..85b14fd1f2 100644 --- a/voice_engine/channel.cc +++ b/voice_engine/channel.cc @@ -1391,13 +1391,13 @@ bool Channel::ReceivePacket(const uint8_t* packet, const uint8_t* payload = packet + header.headerLength; assert(packet_length >= header.headerLength); size_t payload_length = packet_length - header.headerLength; - PayloadUnion payload_specific; - if (!rtp_payload_registry_->GetPayloadSpecifics(header.payloadType, - &payload_specific)) { + const auto pl = + rtp_payload_registry_->PayloadTypeToPayload(header.payloadType); + if (!pl) { return false; } return rtp_receiver_->IncomingRtpPacket(header, payload, payload_length, - payload_specific, in_order); + pl->typeSpecific, in_order); } bool Channel::IsPacketInOrder(const RTPHeader& header) const {