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&,