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 webrtc-uwp#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 <[email protected]> Commit-Queue: Karl Wiberg <[email protected]> Cr-Commit-Position: refs/heads/master@{#20025}
tylergibson · Sep 28, 2017 · 83d3ec1 · 83d3ec1
1 parent 27bafec
commit 83d3ec1
Show file tree

Hide file tree

Showing 9 changed files with 70 additions and 46 deletions.
diff --git 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
@@ -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<uint32_t>(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
@@ -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
@@ -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
@@ -89,7 +89,7 @@ class RTPReceiverStrategy {
   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
@@ -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
@@ -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
@@ -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<RtpPacketToSend> packet,

diff --git 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 @@ 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;