From 83d3ec177ce1c4860e2b48088aa02d1ab508770b Mon Sep 17 00:00:00 2001 From: Karl Wiberg Date: Thu, 28 Sep 2017 19:54:38 +0200 Subject: [PATCH] Convert PayloadUnion from a union to a class, step 1 I need to replace the audio part of PayloadUnion with SdpAudioFormat, but that's a non-trivially-deletable class and those just don't work well in unions, especially unions that don't have a discriminator that says which member is currently active. This CL converts the union to a class, adds a discriminator, and provides accessor functions. CL #2 in the series will change all outsiders to use the accessors instead of the public member variables directly, and CL #3 will remove the public member variables. (It needs to be done in separate steps like this because PayloadUnion is unfortunately part of the API, and just changing it all in one go would break users.) BUG=webrtc:8159 Change-Id: I38c44bbb21a2d38600cff59bf37d8d47dfdbce21 Reviewed-on: https://webrtc-review.googlesource.com/4340 Reviewed-by: Danil Chapovalov Commit-Queue: Karl Wiberg Cr-Commit-Position: refs/heads/master@{#20025} --- modules/rtp_rtcp/include/rtp_rtcp_defines.h | 36 +++++++++++++++++-- .../rtp_rtcp/source/rtp_payload_registry.cc | 32 ++++++++--------- modules/rtp_rtcp/source/rtp_receiver_audio.cc | 2 +- .../rtp_rtcp/source/rtp_receiver_strategy.cc | 10 +++--- .../rtp_rtcp/source/rtp_receiver_strategy.h | 2 +- .../rtp_rtcp/source/rtp_receiver_unittest.cc | 10 +++--- modules/rtp_rtcp/source/rtp_sender_audio.cc | 9 ++--- modules/rtp_rtcp/source/rtp_sender_video.cc | 9 ++--- modules/rtp_rtcp/source/rtp_utility.h | 6 ++++ 9 files changed, 70 insertions(+), 46 deletions(-) diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 6d19cea885..64980051c4 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -51,9 +51,39 @@ struct VideoPayload { H264::Profile h264_profile; }; -union PayloadUnion { - AudioPayload Audio; - VideoPayload Video; +class PayloadUnion { + public: + explicit PayloadUnion(const AudioPayload& payload) + : Audio(payload), is_audio_(true) {} + explicit PayloadUnion(const VideoPayload& payload) + : Video(payload), is_audio_(false) {} + + bool is_audio() const { return is_audio_; } + bool is_video() const { return !is_audio_; } + const AudioPayload& audio_payload() const { + RTC_DCHECK(is_audio_); + return Audio; + } + const VideoPayload& video_payload() const { + RTC_DCHECK(!is_audio_); + return Video; + } + AudioPayload& audio_payload() { + RTC_DCHECK(is_audio_); + return Audio; + } + VideoPayload& video_payload() { + RTC_DCHECK(!is_audio_); + return Video; + } + + public: + // These two are public for backwards compatibilty. They'll go private soon. + AudioPayload Audio; + VideoPayload Video; + + private: + bool is_audio_; }; enum RTPAliveType { kRtpDead = 0, kRtpNoRtp = 1, kRtpAlive = 2 }; diff --git a/modules/rtp_rtcp/source/rtp_payload_registry.cc b/modules/rtp_rtcp/source/rtp_payload_registry.cc index fcd0276c01..9effcc291c 100644 --- a/modules/rtp_rtcp/source/rtp_payload_registry.cc +++ b/modules/rtp_rtcp/source/rtp_payload_registry.cc @@ -46,15 +46,11 @@ bool PayloadIsCompatible(const RtpUtility::Payload& payload, } RtpUtility::Payload CreatePayloadType(const CodecInst& audio_codec) { - RtpUtility::Payload payload; - payload.name[RTP_PAYLOAD_NAME_SIZE - 1] = 0; - strncpy(payload.name, audio_codec.plname, RTP_PAYLOAD_NAME_SIZE - 1); RTC_DCHECK_GE(audio_codec.plfreq, 1000); - payload.typeSpecific.Audio.frequency = audio_codec.plfreq; - payload.typeSpecific.Audio.channels = audio_codec.channels; - payload.typeSpecific.Audio.rate = 0; - payload.audio = true; - return payload; + return {audio_codec.plname, + PayloadUnion( + AudioPayload{rtc::dchecked_cast(audio_codec.plfreq), + audio_codec.channels, 0})}; } RtpVideoCodecTypes ConvertToRtpVideoCodecType(VideoCodecType type) { @@ -74,15 +70,11 @@ RtpVideoCodecTypes ConvertToRtpVideoCodecType(VideoCodecType type) { } RtpUtility::Payload CreatePayloadType(const VideoCodec& video_codec) { - RtpUtility::Payload payload; - payload.name[RTP_PAYLOAD_NAME_SIZE - 1] = 0; - strncpy(payload.name, video_codec.plName, RTP_PAYLOAD_NAME_SIZE - 1); - payload.typeSpecific.Video.videoCodecType = - ConvertToRtpVideoCodecType(video_codec.codecType); + VideoPayload p; + p.videoCodecType = ConvertToRtpVideoCodecType(video_codec.codecType); if (video_codec.codecType == kVideoCodecH264) - payload.typeSpecific.Video.h264_profile = video_codec.H264().profile; - payload.audio = false; - return payload; + p.h264_profile = video_codec.H264().profile; + return {video_codec.plName, PayloadUnion(p)}; } bool IsPayloadTypeValid(int8_t payload_type) { @@ -172,7 +164,9 @@ int32_t RTPPayloadRegistry::RegisterReceivePayload(const CodecInst& audio_codec, // Audio codecs must be unique. DeregisterAudioCodecOrRedTypeRegardlessOfPayloadType(audio_codec); - payload_type_map_[audio_codec.pltype] = CreatePayloadType(audio_codec); + const auto insert_status = payload_type_map_.emplace( + audio_codec.pltype, CreatePayloadType(audio_codec)); + RTC_DCHECK(insert_status.second); // Insertion succeeded. *created_new_payload = true; // Successful set of payload type, clear the value of last received payload @@ -205,7 +199,9 @@ int32_t RTPPayloadRegistry::RegisterReceivePayload( return -1; } - payload_type_map_[video_codec.plType] = CreatePayloadType(video_codec); + const auto insert_status = payload_type_map_.emplace( + 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. diff --git a/modules/rtp_rtcp/source/rtp_receiver_audio.cc b/modules/rtp_rtcp/source/rtp_receiver_audio.cc index d399ad53a9..9ccc3ec44b 100644 --- a/modules/rtp_rtcp/source/rtp_receiver_audio.cc +++ b/modules/rtp_rtcp/source/rtp_receiver_audio.cc @@ -35,7 +35,7 @@ RTPReceiverAudio::RTPReceiverAudio(RtpData* data_callback) cng_fb_payload_type_(-1), num_energy_(0), current_remote_energy_() { - last_payload_.Audio.channels = 1; + last_payload_.emplace(AudioPayload{0, 1, 0}); memset(current_remote_energy_, 0, sizeof(current_remote_energy_)); } diff --git a/modules/rtp_rtcp/source/rtp_receiver_strategy.cc b/modules/rtp_rtcp/source/rtp_receiver_strategy.cc index de58b615e6..6db24c9a02 100644 --- a/modules/rtp_rtcp/source/rtp_receiver_strategy.cc +++ b/modules/rtp_rtcp/source/rtp_receiver_strategy.cc @@ -15,20 +15,20 @@ namespace webrtc { RTPReceiverStrategy::RTPReceiverStrategy(RtpData* data_callback) - : data_callback_(data_callback) { - memset(&last_payload_, 0, sizeof(last_payload_)); -} + : data_callback_(data_callback) {} void RTPReceiverStrategy::GetLastMediaSpecificPayload( PayloadUnion* payload) const { rtc::CritScope cs(&crit_sect_); - memcpy(payload, &last_payload_, sizeof(*payload)); + if (last_payload_) { + *payload = *last_payload_; + } } void RTPReceiverStrategy::SetLastMediaSpecificPayload( const PayloadUnion& payload) { rtc::CritScope cs(&crit_sect_); - memcpy(&last_payload_, &payload, sizeof(last_payload_)); + last_payload_.emplace(payload); } void RTPReceiverStrategy::CheckPayloadChanged(int8_t payload_type, diff --git a/modules/rtp_rtcp/source/rtp_receiver_strategy.h b/modules/rtp_rtcp/source/rtp_receiver_strategy.h index af1868e9f5..cc1d1e6e17 100644 --- a/modules/rtp_rtcp/source/rtp_receiver_strategy.h +++ b/modules/rtp_rtcp/source/rtp_receiver_strategy.h @@ -89,7 +89,7 @@ class RTPReceiverStrategy { explicit RTPReceiverStrategy(RtpData* data_callback); rtc::CriticalSection crit_sect_; - PayloadUnion last_payload_; + rtc::Optional last_payload_; RtpData* data_callback_; }; } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_receiver_unittest.cc b/modules/rtp_rtcp/source/rtp_receiver_unittest.cc index c1cde0f599..f1d5233e20 100644 --- a/modules/rtp_rtcp/source/rtp_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_receiver_unittest.cc @@ -90,7 +90,7 @@ TEST_F(RtpReceiverTest, GetSources) { header.numCSRCs = 2; header.arrOfCSRCs[0] = kCsrc1; header.arrOfCSRCs[1] = kCsrc2; - PayloadUnion payload_specific = {AudioPayload()}; + const PayloadUnion payload_specific{AudioPayload()}; EXPECT_TRUE(rtp_receiver_->IncomingRtpPacket( header, kTestPayload, sizeof(kTestPayload), payload_specific, !kInOrder)); @@ -140,7 +140,7 @@ TEST_F(RtpReceiverTest, GetSourcesChangeSSRC) { header.payloadType = kPcmuPayloadType; header.ssrc = kSsrc1; header.timestamp = rtp_timestamp(now_ms); - PayloadUnion payload_specific = {AudioPayload()}; + const PayloadUnion payload_specific{AudioPayload()}; EXPECT_TRUE(rtp_receiver_->IncomingRtpPacket( header, kTestPayload, sizeof(kTestPayload), payload_specific, !kInOrder)); @@ -191,7 +191,7 @@ TEST_F(RtpReceiverTest, GetSourcesRemoveOutdatedSource) { RTPHeader header; header.payloadType = kPcmuPayloadType; header.timestamp = rtp_timestamp(now_ms); - PayloadUnion payload_specific = {AudioPayload()}; + const PayloadUnion payload_specific{AudioPayload()}; header.numCSRCs = 1; size_t kSourceListSize = 20; @@ -265,7 +265,7 @@ TEST_F(RtpReceiverTest, GetSourcesContainsAudioLevelExtension) { header.timestamp = rtp_timestamp(time1_ms); header.extension.hasAudioLevel = true; header.extension.audioLevel = 10; - PayloadUnion payload_specific = {AudioPayload()}; + const PayloadUnion payload_specific{AudioPayload()}; EXPECT_TRUE(rtp_receiver_->IncomingRtpPacket( header, kTestPayload, sizeof(kTestPayload), payload_specific, !kInOrder)); @@ -317,7 +317,7 @@ TEST_F(RtpReceiverTest, header.timestamp = rtp_timestamp(time1_ms); header.extension.hasAudioLevel = true; header.extension.audioLevel = 10; - PayloadUnion payload_specific = {AudioPayload()}; + const PayloadUnion payload_specific{AudioPayload()}; EXPECT_TRUE(rtp_receiver_->IncomingRtpPacket( header, kTestPayload, sizeof(kTestPayload), payload_specific, !kInOrder)); diff --git a/modules/rtp_rtcp/source/rtp_sender_audio.cc b/modules/rtp_rtcp/source/rtp_sender_audio.cc index f75452fde9..dadf30b532 100644 --- a/modules/rtp_rtcp/source/rtp_sender_audio.cc +++ b/modules/rtp_rtcp/source/rtp_sender_audio.cc @@ -65,13 +65,8 @@ int32_t RTPSenderAudio::RegisterAudioPayload( dtmf_payload_freq_ = frequency; return 0; } - *payload = new RtpUtility::Payload; - (*payload)->typeSpecific.Audio.frequency = frequency; - (*payload)->typeSpecific.Audio.channels = channels; - (*payload)->typeSpecific.Audio.rate = rate; - (*payload)->audio = true; - (*payload)->name[RTP_PAYLOAD_NAME_SIZE - 1] = '\0'; - strncpy((*payload)->name, payloadName, RTP_PAYLOAD_NAME_SIZE - 1); + *payload = new RtpUtility::Payload( + payloadName, PayloadUnion(AudioPayload{frequency, channels, rate})); return 0; } diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 888dc7277b..60636d1e26 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -92,12 +92,9 @@ RtpUtility::Payload* RTPSenderVideo::CreateVideoPayload( } else { video_type = kRtpVideoGeneric; } - RtpUtility::Payload* payload = new RtpUtility::Payload(); - payload->name[RTP_PAYLOAD_NAME_SIZE - 1] = 0; - strncpy(payload->name, payload_name, RTP_PAYLOAD_NAME_SIZE - 1); - payload->typeSpecific.Video.videoCodecType = video_type; - payload->audio = false; - return payload; + VideoPayload vp; + vp.videoCodecType = video_type; + return new RtpUtility::Payload(payload_name, PayloadUnion(vp)); } void RTPSenderVideo::SendVideoPacket(std::unique_ptr packet, diff --git a/modules/rtp_rtcp/source/rtp_utility.h b/modules/rtp_rtcp/source/rtp_utility.h index cd4968effd..04eb4383d3 100644 --- a/modules/rtp_rtcp/source/rtp_utility.h +++ b/modules/rtp_rtcp/source/rtp_utility.h @@ -11,6 +11,7 @@ #ifndef MODULES_RTP_RTCP_SOURCE_RTP_UTILITY_H_ #define MODULES_RTP_RTCP_SOURCE_RTP_UTILITY_H_ +#include #include #include "modules/rtp_rtcp/include/receive_statistics.h" @@ -29,6 +30,11 @@ RtpFeedback* NullObjectRtpFeedback(); namespace RtpUtility { struct Payload { + Payload(const char* name, const PayloadUnion& pu) + : audio(pu.is_audio()), typeSpecific(pu) { + std::strncpy(this->name, name, sizeof(this->name) - 1); + this->name[sizeof(this->name) - 1] = '\0'; + } char name[RTP_PAYLOAD_NAME_SIZE]; bool audio; PayloadUnion typeSpecific;