From d908d74fac46c58b80c1120b982d3f1b407292f3 Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Wed, 5 Jan 2022 10:44:26 +0000 Subject: [PATCH] Make error param non-optional when setting local/remote content. This is a slight refactoring while doing some other changes, so not strictly necessary, but the error param is always supplied in practice so it made sense to update the tests to reflect that, test that error values are reported in (at least) some cases and remove the additional code that checks for whether or not error information is requested. Bug: none Change-Id: Ia5739a18ea2beb6970eabf9d809c24dfa43466b1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/244097 Reviewed-by: Harald Alvestrand Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#35632} --- pc/channel.cc | 168 ++++++++++++---------------- pc/channel.h | 20 ++-- pc/channel_interface.h | 4 +- pc/channel_unittest.cc | 184 +++++++++++++++++-------------- pc/sdp_offer_answer.cc | 4 +- pc/test/mock_channel_interface.h | 4 +- 6 files changed, 190 insertions(+), 194 deletions(-) diff --git a/pc/channel.cc b/pc/channel.cc index 925d459b41..a164e0ae04 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -30,7 +30,7 @@ #include "rtc_base/copy_on_write_buffer.h" #include "rtc_base/logging.h" #include "rtc_base/network_route.h" -#include "rtc_base/strings/string_builder.h" +#include "rtc_base/strings/string_format.h" #include "rtc_base/synchronization/mutex.h" #include "rtc_base/task_utils/pending_task_safety_flag.h" #include "rtc_base/task_utils/to_queued_task.h" @@ -39,6 +39,7 @@ namespace cricket { namespace { +using ::rtc::StringFormat; using ::rtc::UniqueRandomIdGenerator; using ::webrtc::PendingTaskSafetyFlag; using ::webrtc::SdpType; @@ -79,12 +80,6 @@ struct StreamFinder { } // namespace -static void SafeSetError(const std::string& message, std::string* error_desc) { - if (error_desc) { - *error_desc = message; - } -} - template void RtpParametersFromMediaDescription( const MediaContentDescriptionImpl* desc, @@ -132,8 +127,9 @@ BaseChannel::BaseChannel(rtc::Thread* worker_thread, demuxer_criteria_(content_name), ssrc_generator_(ssrc_generator) { RTC_DCHECK_RUN_ON(worker_thread_); + RTC_DCHECK(media_channel_); RTC_DCHECK(ssrc_generator_); - RTC_LOG(LS_INFO) << "Created channel: " << ToString(); + RTC_DLOG(LS_INFO) << "Created channel: " << ToString(); } BaseChannel::~BaseChannel() { @@ -148,13 +144,8 @@ BaseChannel::~BaseChannel() { } std::string BaseChannel::ToString() const { - rtc::StringBuilder sb; - sb << "{mid: " << content_name(); - if (media_channel_) { - sb << ", media_type: " << MediaTypeToString(media_channel_->media_type()); - } - sb << "}"; - return sb.Release(); + return StringFormat("{mid: %s, media_type: %s}", content_name().c_str(), + MediaTypeToString(media_channel_->media_type()).c_str()); } bool BaseChannel::ConnectToRtpTransport_n() { @@ -268,7 +259,7 @@ void BaseChannel::Enable(bool enable) { bool BaseChannel::SetLocalContent(const MediaContentDescription* content, SdpType type, - std::string* error_desc) { + std::string& error_desc) { RTC_DCHECK_RUN_ON(worker_thread()); TRACE_EVENT0("webrtc", "BaseChannel::SetLocalContent"); return SetLocalContent_w(content, type, error_desc); @@ -276,7 +267,7 @@ bool BaseChannel::SetLocalContent(const MediaContentDescription* content, bool BaseChannel::SetRemoteContent(const MediaContentDescription* content, SdpType type, - std::string* error_desc) { + std::string& error_desc) { RTC_DCHECK_RUN_ON(worker_thread()); TRACE_EVENT0("webrtc", "BaseChannel::SetRemoteContent"); return SetRemoteContent_w(content, type, error_desc); @@ -586,7 +577,7 @@ bool BaseChannel::SetPayloadTypeDemuxingEnabled_w(bool enabled) { bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, SdpType type, - std::string* error_desc) { + std::string& error_desc) { // In the case of RIDs (where SSRCs are not negotiated), this method will // generate an SSRC for each layer in StreamParams. That representation will // be stored internally in `local_streams_`. @@ -606,11 +597,10 @@ bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, continue; } if (!media_channel()->RemoveSendStream(old_stream.first_ssrc())) { - rtc::StringBuilder desc; - desc << "Failed to remove send stream with ssrc " - << old_stream.first_ssrc() << " from m-section with mid='" - << content_name() << "'."; - SafeSetError(desc.str(), error_desc); + error_desc = StringFormat( + "Failed to remove send stream with ssrc %u from m-section with " + "mid='%s'.", + old_stream.first_ssrc(), content_name().c_str()); ret = false; } } @@ -633,11 +623,10 @@ bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, RTC_DCHECK(new_stream.has_ssrcs() || new_stream.has_rids()); if (new_stream.has_ssrcs() && new_stream.has_rids()) { - rtc::StringBuilder desc; - desc << "Failed to add send stream: " << new_stream.first_ssrc() - << " into m-section with mid='" << content_name() - << "'. Stream has both SSRCs and RIDs."; - SafeSetError(desc.str(), error_desc); + error_desc = StringFormat( + "Failed to add send stream: %u into m-section with mid='%s'. Stream " + "has both SSRCs and RIDs.", + new_stream.first_ssrc(), content_name().c_str()); ret = false; continue; } @@ -654,10 +643,9 @@ bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, RTC_LOG(LS_INFO) << "Add send stream ssrc: " << new_stream.ssrcs[0] << " into " << ToString(); } else { - rtc::StringBuilder desc; - desc << "Failed to add send stream ssrc: " << new_stream.first_ssrc() - << " into m-section with mid='" << content_name() << "'"; - SafeSetError(desc.str(), error_desc); + error_desc = StringFormat( + "Failed to add send stream ssrc: %u into m-section with mid='%s'", + new_stream.first_ssrc(), content_name().c_str()); ret = false; } } @@ -668,7 +656,7 @@ bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, bool BaseChannel::UpdateRemoteStreams_w( const std::vector& streams, SdpType type, - std::string* error_desc) { + std::string& error_desc) { // Check for streams that have been removed. bool ret = true; for (const StreamParams& old_stream : remote_streams_) { @@ -684,11 +672,10 @@ bool BaseChannel::UpdateRemoteStreams_w( RTC_LOG(LS_INFO) << "Remove remote ssrc: " << old_stream.first_ssrc() << " from " << ToString() << "."; } else { - rtc::StringBuilder desc; - desc << "Failed to remove remote stream with ssrc " - << old_stream.first_ssrc() << " from m-section with mid='" - << content_name() << "'."; - SafeSetError(desc.str(), error_desc); + error_desc = StringFormat( + "Failed to remove remote stream with ssrc %u from m-section with " + "mid='%s'.", + old_stream.first_ssrc(), content_name().c_str()); ret = false; } } @@ -708,13 +695,12 @@ bool BaseChannel::UpdateRemoteStreams_w( : "unsignaled") << " to " << ToString(); } else { - rtc::StringBuilder desc; - desc << "Failed to add remote stream ssrc: " - << (new_stream.has_ssrcs() - ? std::to_string(new_stream.first_ssrc()) - : "unsignaled") - << " to " << ToString(); - SafeSetError(desc.str(), error_desc); + error_desc = + StringFormat("Failed to add remote stream ssrc: %s to %s", + new_stream.has_ssrcs() + ? std::to_string(new_stream.first_ssrc()).c_str() + : "unsignaled", + ToString().c_str()); ret = false; } } @@ -801,7 +787,7 @@ void VoiceChannel::UpdateMediaSendRecvState_w() { bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content, SdpType type, - std::string* error_desc) { + std::string& error_desc) { TRACE_EVENT0("webrtc", "VoiceChannel::SetLocalContent_w"); RTC_LOG(LS_INFO) << "Setting local voice description for " << ToString(); @@ -819,11 +805,10 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content, &recv_params); if (!media_channel()->SetRecvParameters(recv_params)) { - SafeSetError( + error_desc = StringFormat( "Failed to set local audio description recv parameters for m-section " - "with mid='" + - content_name() + "'.", - error_desc); + "with mid='%s'.", + content_name().c_str()); return false; } @@ -834,6 +819,8 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content, // Need to re-register the sink to update the handled payload. if (!RegisterRtpDemuxerSink_w()) { RTC_LOG(LS_ERROR) << "Failed to set up audio demuxing for " << ToString(); + error_desc = StringFormat("Failed to set up audio demuxing for mid='%s'.", + content_name().c_str()); return false; } } @@ -845,11 +832,7 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content, // description too (without a remote description, we won't be able // to send them anyway). if (!UpdateLocalStreams_w(content->as_audio()->streams(), type, error_desc)) { - SafeSetError( - "Failed to set local audio description streams for m-section with " - "mid='" + - content_name() + "'.", - error_desc); + RTC_DCHECK(!error_desc.empty()); return false; } @@ -860,7 +843,7 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content, bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content, SdpType type, - std::string* error_desc) { + std::string& error_desc) { TRACE_EVENT0("webrtc", "VoiceChannel::SetRemoteContent_w"); RTC_LOG(LS_INFO) << "Setting remote voice description for " << ToString(); @@ -877,11 +860,10 @@ bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content, bool parameters_applied = media_channel()->SetSendParameters(send_params); if (!parameters_applied) { - SafeSetError( + error_desc = StringFormat( "Failed to set remote audio description send parameters for m-section " - "with mid='" + - content_name() + "'.", - error_desc); + "with mid='%s'.", + content_name().c_str()); return false; } last_send_params_ = send_params; @@ -902,11 +884,10 @@ bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content, // description too (without a local description, we won't be able to // recv them anyway). if (!UpdateRemoteStreams_w(audio->streams(), type, error_desc)) { - SafeSetError( + error_desc = StringFormat( "Failed to set remote audio description streams for m-section with " - "mid='" + - content_name() + "'.", - error_desc); + "mid='%s'.", + content_name().c_str()); return false; } @@ -960,7 +941,7 @@ void VideoChannel::FillBitrateInfo(BandwidthEstimationInfo* bwe_info) { bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, SdpType type, - std::string* error_desc) { + std::string& error_desc) { TRACE_EVENT0("webrtc", "VideoChannel::SetLocalContent_w"); RTC_LOG(LS_INFO) << "Setting local video description for " << ToString(); @@ -987,11 +968,10 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, send_codec.packetization.reset(); needs_send_params_update = true; } else if (recv_codec->packetization != send_codec.packetization) { - SafeSetError( + error_desc = StringFormat( "Failed to set local answer due to invalid codec packetization " - "specified in m-section with mid='" + - content_name() + "'.", - error_desc); + "specified in m-section with mid='%s'.", + content_name().c_str()); return false; } } @@ -999,11 +979,10 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, } if (!media_channel()->SetRecvParameters(recv_params)) { - SafeSetError( + error_desc = StringFormat( "Failed to set local video description recv parameters for m-section " - "with mid='" + - content_name() + "'.", - error_desc); + "with mid='%s'.", + content_name().c_str()); return false; } @@ -1014,6 +993,8 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, // Need to re-register the sink to update the handled payload. if (!RegisterRtpDemuxerSink_w()) { RTC_LOG(LS_ERROR) << "Failed to set up video demuxing for " << ToString(); + error_desc = StringFormat("Failed to set up video demuxing for mid='%s'.", + content_name().c_str()); return false; } } @@ -1022,9 +1003,9 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, if (needs_send_params_update) { if (!media_channel()->SetSendParameters(send_params)) { - SafeSetError("Failed to set send parameters for m-section with mid='" + - content_name() + "'.", - error_desc); + error_desc = StringFormat( + "Failed to set send parameters for m-section with mid='%s'.", + content_name().c_str()); return false; } last_send_params_ = send_params; @@ -1035,11 +1016,7 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, // description too (without a remote description, we won't be able // to send them anyway). if (!UpdateLocalStreams_w(content->as_video()->streams(), type, error_desc)) { - SafeSetError( - "Failed to set local video description streams for m-section with " - "mid='" + - content_name() + "'.", - error_desc); + RTC_DCHECK(!error_desc.empty()); return false; } @@ -1050,7 +1027,7 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, SdpType type, - std::string* error_desc) { + std::string& error_desc) { TRACE_EVENT0("webrtc", "VideoChannel::SetRemoteContent_w"); RTC_LOG(LS_INFO) << "Setting remote video description for " << ToString(); @@ -1079,11 +1056,10 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, recv_codec.packetization.reset(); needs_recv_params_update = true; } else if (send_codec->packetization != recv_codec.packetization) { - SafeSetError( + error_desc = StringFormat( "Failed to set remote answer due to invalid codec packetization " - "specifid in m-section with mid='" + - content_name() + "'.", - error_desc); + "specifid in m-section with mid='%s'.", + content_name().c_str()); return false; } } @@ -1091,20 +1067,19 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, } if (!media_channel()->SetSendParameters(send_params)) { - SafeSetError( + error_desc = StringFormat( "Failed to set remote video description send parameters for m-section " - "with mid='" + - content_name() + "'.", - error_desc); + "with mid='%s'.", + content_name().c_str()); return false; } last_send_params_ = send_params; if (needs_recv_params_update) { if (!media_channel()->SetRecvParameters(recv_params)) { - SafeSetError("Failed to set recv parameters for m-section with mid='" + - content_name() + "'.", - error_desc); + error_desc = StringFormat( + "Failed to set recv parameters for m-section with mid='%s'.", + content_name().c_str()); return false; } last_recv_params_ = recv_params; @@ -1126,11 +1101,10 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, // description too (without a local description, we won't be able to // recv them anyway). if (!UpdateRemoteStreams_w(video->streams(), type, error_desc)) { - SafeSetError( + error_desc = StringFormat( "Failed to set remote video description streams for m-section with " - "mid='" + - content_name() + "'.", - error_desc); + "mid='%s'.", + content_name().c_str()); return false; } set_remote_content_direction(content->direction()); diff --git a/pc/channel.h b/pc/channel.h index 76f6b54f5e..e0bf3baa1a 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -153,10 +153,10 @@ class BaseChannel : public ChannelInterface, // Channel control bool SetLocalContent(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) override; + std::string& error_desc) override; bool SetRemoteContent(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) override; + std::string& error_desc) override; // Controls whether this channel will receive packets on the basis of // matching payload type alone. This is needed for legacy endpoints that // don't signal SSRCs or use MID/RID, but doesn't make sense if there is @@ -267,19 +267,19 @@ class BaseChannel : public ChannelInterface, bool UpdateLocalStreams_w(const std::vector& streams, webrtc::SdpType type, - std::string* error_desc) + std::string& error_desc) RTC_RUN_ON(worker_thread()); bool UpdateRemoteStreams_w(const std::vector& streams, webrtc::SdpType type, - std::string* error_desc) + std::string& error_desc) RTC_RUN_ON(worker_thread()); virtual bool SetLocalContent_w(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) + std::string& error_desc) RTC_RUN_ON(worker_thread()) = 0; virtual bool SetRemoteContent_w(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) + std::string& error_desc) RTC_RUN_ON(worker_thread()) = 0; // Returns a list of RTP header extensions where any extension URI is unique. @@ -402,11 +402,11 @@ class VoiceChannel : public BaseChannel { void UpdateMediaSendRecvState_w() RTC_RUN_ON(worker_thread()) override; bool SetLocalContent_w(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) + std::string& error_desc) RTC_RUN_ON(worker_thread()) override; bool SetRemoteContent_w(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) + std::string& error_desc) RTC_RUN_ON(worker_thread()) override; // Last AudioSendParameters sent down to the media_channel() via @@ -446,11 +446,11 @@ class VideoChannel : public BaseChannel { void UpdateMediaSendRecvState_w() RTC_RUN_ON(worker_thread()) override; bool SetLocalContent_w(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) + std::string& error_desc) RTC_RUN_ON(worker_thread()) override; bool SetRemoteContent_w(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) + std::string& error_desc) RTC_RUN_ON(worker_thread()) override; // Last VideoSendParameters sent down to the media_channel() via diff --git a/pc/channel_interface.h b/pc/channel_interface.h index 08cbff33ff..6185b69e6a 100644 --- a/pc/channel_interface.h +++ b/pc/channel_interface.h @@ -51,10 +51,10 @@ class ChannelInterface { // Channel control virtual bool SetLocalContent(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) = 0; + std::string& error_desc) = 0; virtual bool SetRemoteContent(const MediaContentDescription* content, webrtc::SdpType type, - std::string* error_desc) = 0; + std::string& error_desc) = 0; virtual bool SetPayloadTypeDemuxingEnabled(bool enabled) = 0; // Access to the local and remote streams that were set on the channel. diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index a16a9f041d..4818267f18 100644 --- a/pc/channel_unittest.cc +++ b/pc/channel_unittest.cc @@ -331,17 +331,18 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { } bool SendInitiate() { + std::string err; bool result = channel1_->SetLocalContent(&local_media_content1_, - SdpType::kOffer, NULL); + SdpType::kOffer, err); if (result) { channel1_->Enable(true); FlushCurrentThread(); result = channel2_->SetRemoteContent(&remote_media_content1_, - SdpType::kOffer, NULL); + SdpType::kOffer, err); if (result) { ConnectFakeTransports(); result = channel2_->SetLocalContent(&local_media_content2_, - SdpType::kAnswer, NULL); + SdpType::kAnswer, err); } } return result; @@ -350,39 +351,44 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { bool SendAccept() { channel2_->Enable(true); FlushCurrentThread(); + std::string err; return channel1_->SetRemoteContent(&remote_media_content2_, - SdpType::kAnswer, NULL); + SdpType::kAnswer, err); } bool SendOffer() { + std::string err; bool result = channel1_->SetLocalContent(&local_media_content1_, - SdpType::kOffer, NULL); + SdpType::kOffer, err); if (result) { channel1_->Enable(true); result = channel2_->SetRemoteContent(&remote_media_content1_, - SdpType::kOffer, NULL); + SdpType::kOffer, err); } return result; } bool SendProvisionalAnswer() { + std::string err; bool result = channel2_->SetLocalContent(&local_media_content2_, - SdpType::kPrAnswer, NULL); + SdpType::kPrAnswer, err); if (result) { channel2_->Enable(true); result = channel1_->SetRemoteContent(&remote_media_content2_, - SdpType::kPrAnswer, NULL); + SdpType::kPrAnswer, err); ConnectFakeTransports(); } return result; } bool SendFinalAnswer() { + std::string err; bool result = channel2_->SetLocalContent(&local_media_content2_, - SdpType::kAnswer, NULL); - if (result) + SdpType::kAnswer, err); + if (result) { result = channel1_->SetRemoteContent(&remote_media_content2_, - SdpType::kAnswer, NULL); + SdpType::kAnswer, err); + } return result; } @@ -540,9 +546,10 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { CreateChannels(0, 0); typename T::Content content; CreateContent(0, kPcmuCodec, kH264Codec, &content); - EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kOffer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kOffer, err)); EXPECT_EQ(0U, media_channel1()->codecs().size()); - EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kAnswer, NULL)); + EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kAnswer, err)); ASSERT_EQ(1U, media_channel1()->codecs().size()); EXPECT_TRUE( CodecMatches(content.codecs()[0], media_channel1()->codecs()[0])); @@ -559,9 +566,10 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { auto offer_enum = offer ? (T::Content::kSession) : (T::Content::kNo); auto answer_enum = answer ? (T::Content::kSession) : (T::Content::kNo); content.set_extmap_allow_mixed_enum(offer_enum); - EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kOffer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kOffer, err)); content.set_extmap_allow_mixed_enum(answer_enum); - EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kAnswer, NULL)); + EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kAnswer, err)); EXPECT_EQ(answer, media_channel1()->ExtmapAllowMixed()); } void TestSetContentsExtmapAllowMixedCallee(bool offer, bool answer) { @@ -573,9 +581,10 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { auto offer_enum = offer ? (T::Content::kSession) : (T::Content::kNo); auto answer_enum = answer ? (T::Content::kSession) : (T::Content::kNo); content.set_extmap_allow_mixed_enum(offer_enum); - EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kOffer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kOffer, err)); content.set_extmap_allow_mixed_enum(answer_enum); - EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kAnswer, NULL)); + EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kAnswer, err)); EXPECT_EQ(answer, media_channel1()->ExtmapAllowMixed()); } @@ -584,10 +593,11 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { void TestSetContentsNullOffer() { CreateChannels(0, 0); typename T::Content content; - EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kOffer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kOffer, err)); CreateContent(0, kPcmuCodec, kH264Codec, &content); EXPECT_EQ(0U, media_channel1()->codecs().size()); - EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kAnswer, NULL)); + EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kAnswer, err)); ASSERT_EQ(1U, media_channel1()->codecs().size()); EXPECT_TRUE( CodecMatches(content.codecs()[0], media_channel1()->codecs()[0])); @@ -601,12 +611,13 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { CreateContent(0, kPcmuCodec, kH264Codec, &content); // Both sides agree on mux. Should no longer be a separate RTCP channel. content.set_rtcp_mux(true); - EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kOffer, NULL)); - EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kAnswer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kOffer, err)); + EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kAnswer, err)); // Only initiator supports mux. Should still have a separate RTCP channel. - EXPECT_TRUE(channel2_->SetLocalContent(&content, SdpType::kOffer, NULL)); + EXPECT_TRUE(channel2_->SetLocalContent(&content, SdpType::kOffer, err)); content.set_rtcp_mux(false); - EXPECT_TRUE(channel2_->SetRemoteContent(&content, SdpType::kAnswer, NULL)); + EXPECT_TRUE(channel2_->SetRemoteContent(&content, SdpType::kAnswer, err)); } // Test that SetLocalContent and SetRemoteContent properly @@ -628,20 +639,21 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { typename T::Content content1; CreateContent(0, kPcmuCodec, kH264Codec, &content1); content1.AddStream(stream1); - EXPECT_TRUE(channel1_->SetLocalContent(&content1, SdpType::kOffer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetLocalContent(&content1, SdpType::kOffer, err)); channel1_->Enable(true); EXPECT_EQ(1u, media_channel1()->send_streams().size()); - EXPECT_TRUE(channel2_->SetRemoteContent(&content1, SdpType::kOffer, NULL)); + EXPECT_TRUE(channel2_->SetRemoteContent(&content1, SdpType::kOffer, err)); EXPECT_EQ(1u, media_channel2()->recv_streams().size()); ConnectFakeTransports(); // Channel 2 do not send anything. typename T::Content content2; CreateContent(0, kPcmuCodec, kH264Codec, &content2); - EXPECT_TRUE(channel1_->SetRemoteContent(&content2, SdpType::kAnswer, NULL)); + EXPECT_TRUE(channel1_->SetRemoteContent(&content2, SdpType::kAnswer, err)); EXPECT_EQ(0u, media_channel1()->recv_streams().size()); - EXPECT_TRUE(channel2_->SetLocalContent(&content2, SdpType::kAnswer, NULL)); + EXPECT_TRUE(channel2_->SetLocalContent(&content2, SdpType::kAnswer, err)); channel2_->Enable(true); EXPECT_EQ(0u, media_channel2()->send_streams().size()); @@ -653,21 +665,21 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { typename T::Content content3; CreateContent(0, kPcmuCodec, kH264Codec, &content3); content3.AddStream(stream2); - EXPECT_TRUE(channel2_->SetLocalContent(&content3, SdpType::kOffer, NULL)); + EXPECT_TRUE(channel2_->SetLocalContent(&content3, SdpType::kOffer, err)); ASSERT_EQ(1u, media_channel2()->send_streams().size()); EXPECT_EQ(stream2, media_channel2()->send_streams()[0]); - EXPECT_TRUE(channel1_->SetRemoteContent(&content3, SdpType::kOffer, NULL)); + EXPECT_TRUE(channel1_->SetRemoteContent(&content3, SdpType::kOffer, err)); ASSERT_EQ(1u, media_channel1()->recv_streams().size()); EXPECT_EQ(stream2, media_channel1()->recv_streams()[0]); // Channel 1 replies but stop sending stream1. typename T::Content content4; CreateContent(0, kPcmuCodec, kH264Codec, &content4); - EXPECT_TRUE(channel1_->SetLocalContent(&content4, SdpType::kAnswer, NULL)); + EXPECT_TRUE(channel1_->SetLocalContent(&content4, SdpType::kAnswer, err)); EXPECT_EQ(0u, media_channel1()->send_streams().size()); - EXPECT_TRUE(channel2_->SetRemoteContent(&content4, SdpType::kAnswer, NULL)); + EXPECT_TRUE(channel2_->SetRemoteContent(&content4, SdpType::kAnswer, err)); EXPECT_EQ(0u, media_channel2()->recv_streams().size()); SendCustomRtp2(kSsrc2, 0); @@ -692,20 +704,21 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { EXPECT_FALSE(media_channel1()->playout()); } EXPECT_FALSE(media_channel1()->sending()); + std::string err; EXPECT_TRUE(channel1_->SetLocalContent(&local_media_content1_, - SdpType::kOffer, NULL)); + SdpType::kOffer, err)); if (verify_playout_) { EXPECT_TRUE(media_channel1()->playout()); } EXPECT_FALSE(media_channel1()->sending()); EXPECT_TRUE(channel2_->SetRemoteContent(&local_media_content1_, - SdpType::kOffer, NULL)); + SdpType::kOffer, err)); if (verify_playout_) { EXPECT_FALSE(media_channel2()->playout()); } EXPECT_FALSE(media_channel2()->sending()); EXPECT_TRUE(channel2_->SetLocalContent(&local_media_content2_, - SdpType::kAnswer, NULL)); + SdpType::kAnswer, err)); if (verify_playout_) { EXPECT_FALSE(media_channel2()->playout()); } @@ -726,7 +739,7 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { } EXPECT_TRUE(media_channel2()->sending()); EXPECT_TRUE(channel1_->SetRemoteContent(&local_media_content2_, - SdpType::kAnswer, NULL)); + SdpType::kAnswer, err)); if (verify_playout_) { EXPECT_TRUE(media_channel1()->playout()); } @@ -756,12 +769,12 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { } EXPECT_FALSE(media_channel2()->sending()); - EXPECT_TRUE(channel1_->SetLocalContent(&content1, SdpType::kOffer, NULL)); - EXPECT_TRUE(channel2_->SetRemoteContent(&content1, SdpType::kOffer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetLocalContent(&content1, SdpType::kOffer, err)); + EXPECT_TRUE(channel2_->SetRemoteContent(&content1, SdpType::kOffer, err)); + EXPECT_TRUE(channel2_->SetLocalContent(&content2, SdpType::kPrAnswer, err)); EXPECT_TRUE( - channel2_->SetLocalContent(&content2, SdpType::kPrAnswer, NULL)); - EXPECT_TRUE( - channel1_->SetRemoteContent(&content2, SdpType::kPrAnswer, NULL)); + channel1_->SetRemoteContent(&content2, SdpType::kPrAnswer, err)); ConnectFakeTransports(); if (verify_playout_) { @@ -775,10 +788,9 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { // Update `content2` to be RecvOnly. content2.set_direction(RtpTransceiverDirection::kRecvOnly); + EXPECT_TRUE(channel2_->SetLocalContent(&content2, SdpType::kPrAnswer, err)); EXPECT_TRUE( - channel2_->SetLocalContent(&content2, SdpType::kPrAnswer, NULL)); - EXPECT_TRUE( - channel1_->SetRemoteContent(&content2, SdpType::kPrAnswer, NULL)); + channel1_->SetRemoteContent(&content2, SdpType::kPrAnswer, err)); if (verify_playout_) { EXPECT_TRUE(media_channel1()->playout()); @@ -791,8 +803,8 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { // Update `content2` to be SendRecv. content2.set_direction(RtpTransceiverDirection::kSendRecv); - EXPECT_TRUE(channel2_->SetLocalContent(&content2, SdpType::kAnswer, NULL)); - EXPECT_TRUE(channel1_->SetRemoteContent(&content2, SdpType::kAnswer, NULL)); + EXPECT_TRUE(channel2_->SetLocalContent(&content2, SdpType::kAnswer, err)); + EXPECT_TRUE(channel1_->SetRemoteContent(&content2, SdpType::kAnswer, err)); if (verify_playout_) { EXPECT_TRUE(media_channel1()->playout()); @@ -1092,17 +1104,17 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { media_channel1()->set_fail_set_recv_codecs(true); EXPECT_FALSE( - channel1_->SetLocalContent(content.get(), SdpType::kOffer, &err)); + channel1_->SetLocalContent(content.get(), SdpType::kOffer, err)); EXPECT_FALSE( - channel1_->SetLocalContent(content.get(), SdpType::kAnswer, &err)); + channel1_->SetLocalContent(content.get(), SdpType::kAnswer, err)); media_channel1()->set_fail_set_send_codecs(true); EXPECT_FALSE( - channel1_->SetRemoteContent(content.get(), SdpType::kOffer, &err)); + channel1_->SetRemoteContent(content.get(), SdpType::kOffer, err)); media_channel1()->set_fail_set_send_codecs(true); EXPECT_FALSE( - channel1_->SetRemoteContent(content.get(), SdpType::kAnswer, &err)); + channel1_->SetRemoteContent(content.get(), SdpType::kAnswer, err)); } void TestSendTwoOffers() { @@ -1112,13 +1124,13 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { std::unique_ptr content1( CreateMediaContentWithStream(1)); EXPECT_TRUE( - channel1_->SetLocalContent(content1.get(), SdpType::kOffer, &err)); + channel1_->SetLocalContent(content1.get(), SdpType::kOffer, err)); EXPECT_TRUE(media_channel1()->HasSendStream(1)); std::unique_ptr content2( CreateMediaContentWithStream(2)); EXPECT_TRUE( - channel1_->SetLocalContent(content2.get(), SdpType::kOffer, &err)); + channel1_->SetLocalContent(content2.get(), SdpType::kOffer, err)); EXPECT_FALSE(media_channel1()->HasSendStream(1)); EXPECT_TRUE(media_channel1()->HasSendStream(2)); } @@ -1130,13 +1142,13 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { std::unique_ptr content1( CreateMediaContentWithStream(1)); EXPECT_TRUE( - channel1_->SetRemoteContent(content1.get(), SdpType::kOffer, &err)); + channel1_->SetRemoteContent(content1.get(), SdpType::kOffer, err)); EXPECT_TRUE(media_channel1()->HasRecvStream(1)); std::unique_ptr content2( CreateMediaContentWithStream(2)); EXPECT_TRUE( - channel1_->SetRemoteContent(content2.get(), SdpType::kOffer, &err)); + channel1_->SetRemoteContent(content2.get(), SdpType::kOffer, err)); EXPECT_FALSE(media_channel1()->HasRecvStream(1)); EXPECT_TRUE(media_channel1()->HasRecvStream(2)); } @@ -1149,14 +1161,14 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { std::unique_ptr content1( CreateMediaContentWithStream(1)); EXPECT_TRUE( - channel1_->SetRemoteContent(content1.get(), SdpType::kOffer, &err)); + channel1_->SetRemoteContent(content1.get(), SdpType::kOffer, err)); EXPECT_TRUE(media_channel1()->HasRecvStream(1)); // Send PR answer std::unique_ptr content2( CreateMediaContentWithStream(2)); EXPECT_TRUE( - channel1_->SetLocalContent(content2.get(), SdpType::kPrAnswer, &err)); + channel1_->SetLocalContent(content2.get(), SdpType::kPrAnswer, err)); EXPECT_TRUE(media_channel1()->HasRecvStream(1)); EXPECT_TRUE(media_channel1()->HasSendStream(2)); @@ -1164,7 +1176,7 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { std::unique_ptr content3( CreateMediaContentWithStream(3)); EXPECT_TRUE( - channel1_->SetLocalContent(content3.get(), SdpType::kAnswer, &err)); + channel1_->SetLocalContent(content3.get(), SdpType::kAnswer, err)); EXPECT_TRUE(media_channel1()->HasRecvStream(1)); EXPECT_FALSE(media_channel1()->HasSendStream(2)); EXPECT_TRUE(media_channel1()->HasSendStream(3)); @@ -1178,14 +1190,14 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { std::unique_ptr content1( CreateMediaContentWithStream(1)); EXPECT_TRUE( - channel1_->SetLocalContent(content1.get(), SdpType::kOffer, &err)); + channel1_->SetLocalContent(content1.get(), SdpType::kOffer, err)); EXPECT_TRUE(media_channel1()->HasSendStream(1)); // Receive PR answer std::unique_ptr content2( CreateMediaContentWithStream(2)); EXPECT_TRUE( - channel1_->SetRemoteContent(content2.get(), SdpType::kPrAnswer, &err)); + channel1_->SetRemoteContent(content2.get(), SdpType::kPrAnswer, err)); EXPECT_TRUE(media_channel1()->HasSendStream(1)); EXPECT_TRUE(media_channel1()->HasRecvStream(2)); @@ -1193,7 +1205,7 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { std::unique_ptr content3( CreateMediaContentWithStream(3)); EXPECT_TRUE( - channel1_->SetRemoteContent(content3.get(), SdpType::kAnswer, &err)); + channel1_->SetRemoteContent(content3.get(), SdpType::kAnswer, err)); EXPECT_TRUE(media_channel1()->HasSendStream(1)); EXPECT_FALSE(media_channel1()->HasRecvStream(2)); EXPECT_TRUE(media_channel1()->HasRecvStream(3)); @@ -1237,8 +1249,9 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { void DefaultMaxBitrateIsUnlimited() { CreateChannels(0, 0); + std::string err; EXPECT_TRUE(channel1_->SetLocalContent(&local_media_content1_, - SdpType::kOffer, NULL)); + SdpType::kOffer, err)); EXPECT_EQ(media_channel1()->max_bps(), -1); VerifyMaxBitrate(media_channel1()->GetRtpSendParameters(kSsrc1), absl::nullopt); @@ -1312,15 +1325,14 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { CreateChannels(0, 0); typename T::Content content1, content2, content3; CreateSimulcastContent({"f", "h", "q"}, &content1); - EXPECT_TRUE( - channel1_->SetLocalContent(&content1, SdpType::kOffer, nullptr)); + std::string err; + EXPECT_TRUE(channel1_->SetLocalContent(&content1, SdpType::kOffer, err)); VerifySimulcastStreamParams(content1.streams()[0], channel1_.get()); StreamParams stream1 = channel1_->local_streams()[0]; // Create a similar offer. SetLocalContent should not remove and add. CreateSimulcastContent({"f", "h", "q"}, &content2); - EXPECT_TRUE( - channel1_->SetLocalContent(&content2, SdpType::kOffer, nullptr)); + EXPECT_TRUE(channel1_->SetLocalContent(&content2, SdpType::kOffer, err)); VerifySimulcastStreamParams(content2.streams()[0], channel1_.get()); StreamParams stream2 = channel1_->local_streams()[0]; // Check that the streams are identical (SSRCs didn't change). @@ -1328,8 +1340,7 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { // Create third offer that has same RIDs in different order. CreateSimulcastContent({"f", "q", "h"}, &content3); - EXPECT_TRUE( - channel1_->SetLocalContent(&content3, SdpType::kOffer, nullptr)); + EXPECT_TRUE(channel1_->SetLocalContent(&content3, SdpType::kOffer, err)); VerifySimulcastStreamParams(content3.streams()[0], channel1_.get()); } @@ -1969,7 +1980,8 @@ TEST_F(VideoChannelSingleThreadTest, TestSetLocalOfferWithPacketization) { CreateChannels(0, 0); - EXPECT_TRUE(channel1_->SetLocalContent(&video, SdpType::kOffer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetLocalContent(&video, SdpType::kOffer, err)); EXPECT_THAT(media_channel1()->send_codecs(), testing::IsEmpty()); ASSERT_THAT(media_channel1()->recv_codecs(), testing::SizeIs(2)); EXPECT_TRUE(media_channel1()->recv_codecs()[0].Matches(kVp8Codec)); @@ -1988,7 +2000,9 @@ TEST_F(VideoChannelSingleThreadTest, TestSetRemoteOfferWithPacketization) { CreateChannels(0, 0); - EXPECT_TRUE(channel1_->SetRemoteContent(&video, SdpType::kOffer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetRemoteContent(&video, SdpType::kOffer, err)); + EXPECT_TRUE(err.empty()); EXPECT_THAT(media_channel1()->recv_codecs(), testing::IsEmpty()); ASSERT_THAT(media_channel1()->send_codecs(), testing::SizeIs(2)); EXPECT_TRUE(media_channel1()->send_codecs()[0].Matches(kVp8Codec)); @@ -2007,8 +2021,11 @@ TEST_F(VideoChannelSingleThreadTest, TestSetAnswerWithPacketization) { CreateChannels(0, 0); - EXPECT_TRUE(channel1_->SetLocalContent(&video, SdpType::kOffer, NULL)); - EXPECT_TRUE(channel1_->SetRemoteContent(&video, SdpType::kAnswer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetLocalContent(&video, SdpType::kOffer, err)); + EXPECT_TRUE(err.empty()); + EXPECT_TRUE(channel1_->SetRemoteContent(&video, SdpType::kAnswer, err)); + EXPECT_TRUE(err.empty()); ASSERT_THAT(media_channel1()->recv_codecs(), testing::SizeIs(2)); EXPECT_TRUE(media_channel1()->recv_codecs()[0].Matches(kVp8Codec)); EXPECT_EQ(media_channel1()->recv_codecs()[0].packetization, absl::nullopt); @@ -2034,9 +2051,9 @@ TEST_F(VideoChannelSingleThreadTest, TestSetLocalAnswerWithoutPacketization) { CreateChannels(0, 0); - EXPECT_TRUE( - channel1_->SetRemoteContent(&remote_video, SdpType::kOffer, NULL)); - EXPECT_TRUE(channel1_->SetLocalContent(&local_video, SdpType::kAnswer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetRemoteContent(&remote_video, SdpType::kOffer, err)); + EXPECT_TRUE(channel1_->SetLocalContent(&local_video, SdpType::kAnswer, err)); ASSERT_THAT(media_channel1()->recv_codecs(), testing::SizeIs(1)); EXPECT_EQ(media_channel1()->recv_codecs()[0].packetization, absl::nullopt); ASSERT_THAT(media_channel1()->send_codecs(), testing::SizeIs(1)); @@ -2054,9 +2071,10 @@ TEST_F(VideoChannelSingleThreadTest, TestSetRemoteAnswerWithoutPacketization) { CreateChannels(0, 0); - EXPECT_TRUE(channel1_->SetLocalContent(&local_video, SdpType::kOffer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetLocalContent(&local_video, SdpType::kOffer, err)); EXPECT_TRUE( - channel1_->SetRemoteContent(&remote_video, SdpType::kAnswer, NULL)); + channel1_->SetRemoteContent(&remote_video, SdpType::kAnswer, err)); ASSERT_THAT(media_channel1()->recv_codecs(), testing::SizeIs(1)); EXPECT_EQ(media_channel1()->recv_codecs()[0].packetization, absl::nullopt); ASSERT_THAT(media_channel1()->send_codecs(), testing::SizeIs(1)); @@ -2076,9 +2094,12 @@ TEST_F(VideoChannelSingleThreadTest, CreateChannels(0, 0); - EXPECT_TRUE(channel1_->SetLocalContent(&local_video, SdpType::kOffer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetLocalContent(&local_video, SdpType::kOffer, err)); + EXPECT_TRUE(err.empty()); EXPECT_FALSE( - channel1_->SetRemoteContent(&remote_video, SdpType::kAnswer, NULL)); + channel1_->SetRemoteContent(&remote_video, SdpType::kAnswer, err)); + EXPECT_FALSE(err.empty()); ASSERT_THAT(media_channel1()->recv_codecs(), testing::SizeIs(1)); EXPECT_EQ(media_channel1()->recv_codecs()[0].packetization, cricket::kPacketizationParamRaw); @@ -2097,10 +2118,11 @@ TEST_F(VideoChannelSingleThreadTest, CreateChannels(0, 0); - EXPECT_TRUE( - channel1_->SetRemoteContent(&remote_video, SdpType::kOffer, NULL)); - EXPECT_FALSE( - channel1_->SetLocalContent(&local_video, SdpType::kAnswer, NULL)); + std::string err; + EXPECT_TRUE(channel1_->SetRemoteContent(&remote_video, SdpType::kOffer, err)); + EXPECT_TRUE(err.empty()); + EXPECT_FALSE(channel1_->SetLocalContent(&local_video, SdpType::kAnswer, err)); + EXPECT_FALSE(err.empty()); EXPECT_THAT(media_channel1()->recv_codecs(), testing::IsEmpty()); ASSERT_THAT(media_channel1()->send_codecs(), testing::SizeIs(1)); EXPECT_EQ(media_channel1()->send_codecs()[0].packetization, absl::nullopt); diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 56eb97584d..12ed0cce4f 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -4271,8 +4271,8 @@ RTCError SdpOfferAnswerHandler::PushdownMediaDescription( std::string error; bool success = (source == cricket::CS_LOCAL) - ? entry.first->SetLocalContent(entry.second, type, &error) - : entry.first->SetRemoteContent(entry.second, type, &error); + ? entry.first->SetLocalContent(entry.second, type, error) + : entry.first->SetRemoteContent(entry.second, type, error); if (!success) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, error); } diff --git a/pc/test/mock_channel_interface.h b/pc/test/mock_channel_interface.h index c964b6ca71..de59ebd75b 100644 --- a/pc/test/mock_channel_interface.h +++ b/pc/test/mock_channel_interface.h @@ -37,13 +37,13 @@ class MockChannelInterface : public cricket::ChannelInterface { SetLocalContent, (const cricket::MediaContentDescription*, webrtc::SdpType, - std::string*), + std::string&), (override)); MOCK_METHOD(bool, SetRemoteContent, (const cricket::MediaContentDescription*, webrtc::SdpType, - std::string*), + std::string&), (override)); MOCK_METHOD(bool, SetPayloadTypeDemuxingEnabled, (bool), (override)); MOCK_METHOD(const std::vector&,