From 0fb07f8c90ae99f70990c105655830975c077ba6 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 27 Feb 2020 20:21:37 +0100 Subject: [PATCH] Deprecate use of cricket::MediaContentDescription::Copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One should use a std::unique_ptr to the object, as returned by Clone() instead, not a naked pointer. Bug: webrtc:10701 Change-Id: I10ab309207f2cb5aec83a6d09336699ed7b26f50 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/169342 Reviewed-by: Karl Wiberg Reviewed-by: Henrik Boström Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#30646} --- pc/media_session_unittest.cc | 10 ++----- pc/session_description.h | 50 +++++++++++++++++++++++++--------- test/pc/e2e/sdp/sdp_changer.cc | 2 +- 3 files changed, 40 insertions(+), 22 deletions(-) diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index 49a8aa8c34..b217051054 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc @@ -3371,18 +3371,12 @@ TEST(MediaSessionDescription, CopySessionDescription) { std::make_unique(); acd->set_codecs(MAKE_VECTOR(kAudioCodecs1)); acd->AddLegacyStream(1); - std::unique_ptr acd_passed = - absl::WrapUnique(acd->Copy()); - source.AddContent(cricket::CN_AUDIO, MediaProtocolType::kRtp, - std::move(acd_passed)); + source.AddContent(cricket::CN_AUDIO, MediaProtocolType::kRtp, acd->Clone()); std::unique_ptr vcd = std::make_unique(); vcd->set_codecs(MAKE_VECTOR(kVideoCodecs1)); vcd->AddLegacyStream(2); - std::unique_ptr vcd_passed = - absl::WrapUnique(vcd->Copy()); - source.AddContent(cricket::CN_VIDEO, MediaProtocolType::kRtp, - std::move(vcd_passed)); + source.AddContent(cricket::CN_VIDEO, MediaProtocolType::kRtp, vcd->Clone()); std::unique_ptr copy = source.Clone(); ASSERT_TRUE(copy.get() != NULL); diff --git a/pc/session_description.h b/pc/session_description.h index b2506a4248..b7da8e05b4 100644 --- a/pc/session_description.h +++ b/pc/session_description.h @@ -87,9 +87,15 @@ class MediaContentDescription { virtual bool has_codecs() const = 0; - virtual MediaContentDescription* Copy() const = 0; - virtual std::unique_ptr Clone() const { - return absl::WrapUnique(Copy()); + RTC_DEPRECATED virtual MediaContentDescription* Copy() const { + return CloneInternal(); + } + // Copy operator that returns an unique_ptr. + // Not a virtual function. + // If a type-specific variant of Clone() is desired, override it, or + // simply use std::make_unique(*this) instead of Clone(). + std::unique_ptr Clone() const { + return absl::WrapUnique(CloneInternal()); } // |protocol| is the expected media transport protocol, such as RTP/AVPF, @@ -280,6 +286,12 @@ class MediaContentDescription { std::vector receive_rids_; absl::optional alt_protocol_; + + private: + // Copy function that returns a raw pointer. Caller will assert ownership. + // Should only be called by the Clone() function. Must be implemented + // by each final subclass. + virtual MediaContentDescription* CloneInternal() const = 0; }; // TODO(bugs.webrtc.org/8620): Remove this alias once downstream projects have @@ -337,34 +349,46 @@ class AudioContentDescription : public MediaContentDescriptionImpl { public: AudioContentDescription() {} - virtual AudioContentDescription* Copy() const { - return new AudioContentDescription(*this); + RTC_DEPRECATED virtual AudioContentDescription* Copy() const { + return CloneInternal(); } virtual MediaType type() const { return MEDIA_TYPE_AUDIO; } virtual AudioContentDescription* as_audio() { return this; } virtual const AudioContentDescription* as_audio() const { return this; } + + private: + virtual AudioContentDescription* CloneInternal() const { + return new AudioContentDescription(*this); + } }; class VideoContentDescription : public MediaContentDescriptionImpl { public: - virtual VideoContentDescription* Copy() const { - return new VideoContentDescription(*this); + RTC_DEPRECATED virtual VideoContentDescription* Copy() const { + return CloneInternal(); } virtual MediaType type() const { return MEDIA_TYPE_VIDEO; } virtual VideoContentDescription* as_video() { return this; } virtual const VideoContentDescription* as_video() const { return this; } + + private: + virtual VideoContentDescription* CloneInternal() const { + return new VideoContentDescription(*this); + } }; class RtpDataContentDescription : public MediaContentDescriptionImpl { public: RtpDataContentDescription() {} - RtpDataContentDescription* Copy() const override { - return new RtpDataContentDescription(*this); - } MediaType type() const override { return MEDIA_TYPE_DATA; } RtpDataContentDescription* as_rtp_data() override { return this; } const RtpDataContentDescription* as_rtp_data() const override { return this; } + + private: + RtpDataContentDescription* CloneInternal() const override { + return new RtpDataContentDescription(*this); + } }; class SctpDataContentDescription : public MediaContentDescription { @@ -375,9 +399,6 @@ class SctpDataContentDescription : public MediaContentDescription { use_sctpmap_(o.use_sctpmap_), port_(o.port_), max_message_size_(o.max_message_size_) {} - SctpDataContentDescription* Copy() const override { - return new SctpDataContentDescription(*this); - } MediaType type() const override { return MEDIA_TYPE_DATA; } SctpDataContentDescription* as_sctp() override { return this; } const SctpDataContentDescription* as_sctp() const override { return this; } @@ -398,6 +419,9 @@ class SctpDataContentDescription : public MediaContentDescription { } private: + SctpDataContentDescription* CloneInternal() const override { + return new SctpDataContentDescription(*this); + } bool use_sctpmap_ = true; // Note: "true" is no longer conformant. // Defaults should be constants imported from SCTP. Quick hack. int port_ = 5000; diff --git a/test/pc/e2e/sdp/sdp_changer.cc b/test/pc/e2e/sdp/sdp_changer.cc index a2bf4c543e..16391788a4 100644 --- a/test/pc/e2e/sdp/sdp_changer.cc +++ b/test/pc/e2e/sdp/sdp_changer.cc @@ -204,7 +204,7 @@ LocalAndRemoteSdp SignalingInterceptor::PatchVp8Offer( // single simulcast section will be converted. Do it before removing content // because otherwise description will be deleted. std::unique_ptr prototype_media_desc = - absl::WrapUnique(simulcast_content->media_description()->Copy()); + simulcast_content->media_description()->Clone(); // Remove simulcast video section from offer. RTC_CHECK(desc->RemoveContentByName(simulcast_content->mid()));