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 <danilchap@webrtc.org>
Commit-Queue: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20025}diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h
index 6d19cea..6498005 100644
--- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h
+++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h
@@ -51,9 +51,39 @@
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 fcd0276..9effcc2 100644
--- a/modules/rtp_rtcp/source/rtp_payload_registry.cc
+++ b/modules/rtp_rtcp/source/rtp_payload_registry.cc
@@ -46,15 +46,11 @@
}
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<uint32_t>(audio_codec.plfreq),
+ audio_codec.channels, 0})};
}
RtpVideoCodecTypes ConvertToRtpVideoCodecType(VideoCodecType type) {
@@ -74,15 +70,11 @@
}
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 @@
// 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 @@
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 d399ad5..9ccc3ec 100644
--- a/modules/rtp_rtcp/source/rtp_receiver_audio.cc
+++ b/modules/rtp_rtcp/source/rtp_receiver_audio.cc
@@ -35,7 +35,7 @@
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 de58b61..6db24c9 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 af1868e..cc1d1e6 100644
--- a/modules/rtp_rtcp/source/rtp_receiver_strategy.h
+++ b/modules/rtp_rtcp/source/rtp_receiver_strategy.h
@@ -89,7 +89,7 @@
explicit RTPReceiverStrategy(RtpData* data_callback);
rtc::CriticalSection crit_sect_;
- PayloadUnion last_payload_;
+ rtc::Optional<PayloadUnion> 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 c1cde0f..f1d5233 100644
--- a/modules/rtp_rtcp/source/rtp_receiver_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_receiver_unittest.cc
@@ -90,7 +90,7 @@
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 @@
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 @@
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 @@
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 @@
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 f75452f..dadf30b 100644
--- a/modules/rtp_rtcp/source/rtp_sender_audio.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_audio.cc
@@ -65,13 +65,8 @@
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 888dc72..60636d1 100644
--- a/modules/rtp_rtcp/source/rtp_sender_video.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_video.cc
@@ -92,12 +92,9 @@
} 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<RtpPacketToSend> packet,
diff --git a/modules/rtp_rtcp/source/rtp_utility.h b/modules/rtp_rtcp/source/rtp_utility.h
index cd4968e..04eb438 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 <cstring>
#include <map>
#include "modules/rtp_rtcp/include/receive_statistics.h"
@@ -29,6 +30,11 @@
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;
