diff --git a/pc/channel.cc b/pc/channel.cc index 0f6cdd728b..02ee9d2492 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -290,9 +290,9 @@ bool BaseChannel::SetRemoteContent(const MediaContentDescription* content, Bind(&BaseChannel::SetRemoteContent_w, this, content, type, error_desc)); } -void BaseChannel::SetPayloadTypeDemuxingEnabled(bool enabled) { +bool BaseChannel::SetPayloadTypeDemuxingEnabled(bool enabled) { TRACE_EVENT0("webrtc", "BaseChannel::SetPayloadTypeDemuxingEnabled"); - InvokeOnWorker( + return InvokeOnWorker( RTC_FROM_HERE, Bind(&BaseChannel::SetPayloadTypeDemuxingEnabled_w, this, enabled)); } @@ -595,11 +595,12 @@ void BaseChannel::ResetUnsignaledRecvStream_w() { media_channel()->ResetUnsignaledRecvStream(); } -void BaseChannel::SetPayloadTypeDemuxingEnabled_w(bool enabled) { +bool BaseChannel::SetPayloadTypeDemuxingEnabled_w(bool enabled) { RTC_DCHECK_RUN_ON(worker_thread()); if (enabled == payload_type_demuxing_enabled_) { - return; + return true; } + payload_type_demuxing_enabled_ = enabled; if (!enabled) { // TODO(crbug.com/11477): This will remove *all* unsignaled streams (those // without an explicitly signaled SSRC), which may include streams that @@ -607,10 +608,22 @@ void BaseChannel::SetPayloadTypeDemuxingEnabled_w(bool enabled) { // streams that were matched based on payload type alone, but currently // there is no straightforward way to identify those streams. media_channel()->ResetUnsignaledRecvStream(); - ClearHandledPayloadTypes(); - RegisterRtpDemuxerSink(); + demuxer_criteria_.payload_types.clear(); + if (!RegisterRtpDemuxerSink()) { + RTC_LOG(LS_ERROR) << "Failed to disable payload type demuxing for " + << ToString(); + return false; + } + } else if (!payload_types_.empty()) { + demuxer_criteria_.payload_types.insert(payload_types_.begin(), + payload_types_.end()); + if (!RegisterRtpDemuxerSink()) { + RTC_LOG(LS_ERROR) << "Failed to enable payload type demuxing for " + << ToString(); + return false; + } } - payload_type_demuxing_enabled_ = enabled; + return true; } bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, @@ -754,6 +767,7 @@ bool BaseChannel::UpdateRemoteStreams_w( // Re-register the sink to update the receiving ssrcs. if (!RegisterRtpDemuxerSink()) { RTC_LOG(LS_ERROR) << "Failed to set up demuxing for " << ToString(); + ret = false; } remote_streams_ = streams; return ret; @@ -799,10 +813,14 @@ void BaseChannel::MaybeAddHandledPayloadType(int payload_type) { if (payload_type_demuxing_enabled_) { demuxer_criteria_.payload_types.insert(static_cast(payload_type)); } + // Even if payload type demuxing is currently disabled, we need to remember + // the payload types in case it's re-enabled later. + payload_types_.insert(static_cast(payload_type)); } void BaseChannel::ClearHandledPayloadTypes() { demuxer_criteria_.payload_types.clear(); + payload_types_.clear(); } void BaseChannel::FlushRtcpMessages_n() { diff --git a/pc/channel.h b/pc/channel.h index e99ba2428b..51cc40fc53 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -134,7 +134,7 @@ class BaseChannel : public ChannelInterface, // This method will also remove any existing streams that were bound to this // channel on the basis of payload type, since one of these streams might // actually belong to a new channel. See: crbug.com/webrtc/11477 - void SetPayloadTypeDemuxingEnabled(bool enabled) override; + bool SetPayloadTypeDemuxingEnabled(bool enabled) override; bool Enable(bool enable) override; @@ -224,7 +224,7 @@ class BaseChannel : public ChannelInterface, bool AddRecvStream_w(const StreamParams& sp); bool RemoveRecvStream_w(uint32_t ssrc); void ResetUnsignaledRecvStream_w(); - void SetPayloadTypeDemuxingEnabled_w(bool enabled); + bool SetPayloadTypeDemuxingEnabled_w(bool enabled); bool AddSendStream_w(const StreamParams& sp); bool RemoveSendStream_w(uint32_t ssrc); @@ -322,6 +322,8 @@ class BaseChannel : public ChannelInterface, webrtc::RtpTransceiverDirection remote_content_direction_ = webrtc::RtpTransceiverDirection::kInactive; + // Cached list of payload types, used if payload type demuxing is re-enabled. + std::set payload_types_ RTC_GUARDED_BY(worker_thread()); webrtc::RtpDemuxerCriteria demuxer_criteria_; // This generator is used to generate SSRCs for local streams. // This is needed in cases where SSRCs are not negotiated or set explicitly diff --git a/pc/channel_interface.h b/pc/channel_interface.h index f510c9442d..68b6486304 100644 --- a/pc/channel_interface.h +++ b/pc/channel_interface.h @@ -52,7 +52,7 @@ class ChannelInterface { virtual bool SetRemoteContent(const MediaContentDescription* content, webrtc::SdpType type, std::string* error_desc) = 0; - virtual void SetPayloadTypeDemuxingEnabled(bool enabled) = 0; + virtual bool SetPayloadTypeDemuxingEnabled(bool enabled) = 0; // Access to the local and remote streams that were set on the channel. virtual const std::vector& local_streams() const = 0; diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 0a298b19ad..a72dbc66d9 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -14,6 +14,7 @@ #include +#include #include #include #include @@ -2786,6 +2787,106 @@ TEST_F(PeerConnectionIntegrationTestUnifiedPlan, EXPECT_TRUE(ExpectNewFrames(media_expectations)); } +// Used for the test below. +void RemoveBundleGroupSsrcsAndMidExtension(cricket::SessionDescription* desc) { + RemoveSsrcsAndKeepMsids(desc); + desc->RemoveGroupByName("BUNDLE"); + for (ContentInfo& content : desc->contents()) { + cricket::MediaContentDescription* media = content.media_description(); + cricket::RtpHeaderExtensions extensions = media->rtp_header_extensions(); + extensions.erase(std::remove_if(extensions.begin(), extensions.end(), + [](const RtpExtension& extension) { + return extension.uri == + RtpExtension::kMidUri; + }), + extensions.end()); + media->set_rtp_header_extensions(extensions); + } +} + +// Tests that video flows between multiple video tracks when BUNDLE is not used, +// SSRCs are not signaled and the MID RTP header extension is not used. This +// relies on demuxing by payload type, which normally doesn't work if you have +// multiple media sections using the same payload type, but which should work as +// long as the media sections aren't bundled. +// Regression test for: http://crbug.com/webrtc/12023 +TEST_F(PeerConnectionIntegrationTestUnifiedPlan, + EndToEndCallWithTwoVideoTracksNoBundleNoSignaledSsrcAndNoMid) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->AddVideoTrack(); + caller()->AddVideoTrack(); + callee()->AddVideoTrack(); + callee()->AddVideoTrack(); + caller()->SetReceivedSdpMunger(&RemoveBundleGroupSsrcsAndMidExtension); + callee()->SetReceivedSdpMunger(&RemoveBundleGroupSsrcsAndMidExtension); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + ASSERT_EQ(2u, caller()->pc()->GetReceivers().size()); + ASSERT_EQ(2u, callee()->pc()->GetReceivers().size()); + // Make sure we are not bundled. + ASSERT_NE(caller()->pc()->GetSenders()[0]->dtls_transport(), + caller()->pc()->GetSenders()[1]->dtls_transport()); + + // Expect video to be received in both directions on both tracks. + MediaExpectations media_expectations; + media_expectations.ExpectBidirectionalVideo(); + EXPECT_TRUE(ExpectNewFrames(media_expectations)); +} + +// Used for the test below. +void ModifyPayloadTypesAndRemoveMidExtension( + cricket::SessionDescription* desc) { + int pt = 96; + for (ContentInfo& content : desc->contents()) { + cricket::MediaContentDescription* media = content.media_description(); + cricket::RtpHeaderExtensions extensions = media->rtp_header_extensions(); + extensions.erase(std::remove_if(extensions.begin(), extensions.end(), + [](const RtpExtension& extension) { + return extension.uri == + RtpExtension::kMidUri; + }), + extensions.end()); + media->set_rtp_header_extensions(extensions); + cricket::VideoContentDescription* video = media->as_video(); + ASSERT_TRUE(video != nullptr); + std::vector codecs = {{pt++, "VP8"}}; + video->set_codecs(codecs); + } +} + +// Tests that two video tracks can be demultiplexed by payload type alone, by +// using different payload types for the same codec in different m= sections. +// This practice is discouraged but historically has been supported. +// Regression test for: http://crbug.com/webrtc/12029 +TEST_F(PeerConnectionIntegrationTestUnifiedPlan, + EndToEndCallWithTwoVideoTracksDemultiplexedByPayloadType) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->AddVideoTrack(); + caller()->AddVideoTrack(); + callee()->AddVideoTrack(); + callee()->AddVideoTrack(); + caller()->SetGeneratedSdpMunger(&ModifyPayloadTypesAndRemoveMidExtension); + callee()->SetGeneratedSdpMunger(&ModifyPayloadTypesAndRemoveMidExtension); + // We can't remove SSRCs from the generated SDP because then no send streams + // would be created. + caller()->SetReceivedSdpMunger(&RemoveSsrcsAndKeepMsids); + callee()->SetReceivedSdpMunger(&RemoveSsrcsAndKeepMsids); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + ASSERT_EQ(2u, caller()->pc()->GetReceivers().size()); + ASSERT_EQ(2u, callee()->pc()->GetReceivers().size()); + // Make sure we are bundled. + ASSERT_EQ(caller()->pc()->GetSenders()[0]->dtls_transport(), + caller()->pc()->GetSenders()[1]->dtls_transport()); + + // Expect video to be received in both directions on both tracks. + MediaExpectations media_expectations; + media_expectations.ExpectBidirectionalVideo(); + EXPECT_TRUE(ExpectNewFrames(media_expectations)); +} + TEST_F(PeerConnectionIntegrationTestUnifiedPlan, NoStreamsMsidLinePresent) { ASSERT_TRUE(CreatePeerConnectionWrappers()); ConnectFakeSignaling(); diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 022e5e5846..d03c29265d 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -12,6 +12,7 @@ #include #include +#include #include "api/media_stream_proxy.h" #include "api/uma_metrics.h" @@ -4033,7 +4034,13 @@ RTCError SdpOfferAnswerHandler::PushdownMediaDescription( RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(sdesc); - UpdatePayloadTypeDemuxingState(source); + if (!UpdatePayloadTypeDemuxingState(source)) { + // Note that this is never expected to fail, since RtpDemuxer doesn't return + // an error when changing payload type demux criteria, which is all this + // does. + LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR, + "Failed to update payload type demuxing state."); + } // Push down the new SDP media section for each audio/video transceiver. for (const auto& transceiver : transceivers().List()) { @@ -4700,21 +4707,33 @@ const std::string SdpOfferAnswerHandler::GetTransportName( return ""; } -void SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( +bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( cricket::ContentSource source) { RTC_DCHECK_RUN_ON(signaling_thread()); // We may need to delete any created default streams and disable creation of // new ones on the basis of payload type. This is needed to avoid SSRC // collisions in Call's RtpDemuxer, in the case that a transceiver has // created a default stream, and then some other channel gets the SSRC - // signaled in the corresponding Unified Plan "m=" section. For more context + // signaled in the corresponding Unified Plan "m=" section. Specifically, we + // need to disable payload type based demuxing when two bundled "m=" sections + // are using the same payload type(s). For more context // see https://bugs.chromium.org/p/webrtc/issues/detail?id=11477 const SessionDescriptionInterface* sdesc = (source == cricket::CS_LOCAL ? local_description() : remote_description()); - size_t num_receiving_video_transceivers = 0; - size_t num_receiving_audio_transceivers = 0; + const cricket::ContentGroup* bundle_group = + sdesc->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + std::set audio_payload_types; + std::set video_payload_types; + bool pt_demuxing_enabled_audio = true; + bool pt_demuxing_enabled_video = true; for (auto& content_info : sdesc->description()->contents()) { + // If this m= section isn't bundled, it's safe to demux by payload type + // since other m= sections using the same payload type will also be using + // different transports. + if (!bundle_group || !bundle_group->HasContentName(content_info.name)) { + continue; + } if (content_info.rejected || (source == cricket::ContentSource::CS_LOCAL && !RtpTransceiverDirectionHasRecv( @@ -4726,19 +4745,37 @@ void SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( continue; } switch (content_info.media_description()->type()) { - case cricket::MediaType::MEDIA_TYPE_AUDIO: - ++num_receiving_audio_transceivers; + case cricket::MediaType::MEDIA_TYPE_AUDIO: { + const cricket::AudioContentDescription* audio_desc = + content_info.media_description()->as_audio(); + for (const cricket::AudioCodec& audio : audio_desc->codecs()) { + if (audio_payload_types.count(audio.id)) { + // Two m= sections are using the same payload type, thus demuxing + // by payload type is not possible. + pt_demuxing_enabled_audio = false; + } + audio_payload_types.insert(audio.id); + } break; - case cricket::MediaType::MEDIA_TYPE_VIDEO: - ++num_receiving_video_transceivers; + } + case cricket::MediaType::MEDIA_TYPE_VIDEO: { + const cricket::VideoContentDescription* video_desc = + content_info.media_description()->as_video(); + for (const cricket::VideoCodec& video : video_desc->codecs()) { + if (video_payload_types.count(video.id)) { + // Two m= sections are using the same payload type, thus demuxing + // by payload type is not possible. + pt_demuxing_enabled_video = false; + } + video_payload_types.insert(video.id); + } break; + } default: // Ignore data channels. continue; } } - bool pt_demuxing_enabled_video = num_receiving_video_transceivers <= 1; - bool pt_demuxing_enabled_audio = num_receiving_audio_transceivers <= 1; // Gather all updates ahead of time so that all channels can be updated in a // single Invoke; necessary due to thread guards. @@ -4760,26 +4797,34 @@ void SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( transceiver->internal()->channel()); } - if (!channels_to_update.empty()) { - pc_->worker_thread()->Invoke( - RTC_FROM_HERE, [&channels_to_update, pt_demuxing_enabled_audio, - pt_demuxing_enabled_video]() { - for (const auto& it : channels_to_update) { - RtpTransceiverDirection local_direction = it.first; - cricket::ChannelInterface* channel = it.second; - cricket::MediaType media_type = channel->media_type(); - if (media_type == cricket::MediaType::MEDIA_TYPE_AUDIO) { - channel->SetPayloadTypeDemuxingEnabled( - pt_demuxing_enabled_audio && - RtpTransceiverDirectionHasRecv(local_direction)); - } else if (media_type == cricket::MediaType::MEDIA_TYPE_VIDEO) { - channel->SetPayloadTypeDemuxingEnabled( - pt_demuxing_enabled_video && - RtpTransceiverDirectionHasRecv(local_direction)); + if (channels_to_update.empty()) { + return true; + } + return pc_->worker_thread()->Invoke( + RTC_FROM_HERE, [&channels_to_update, bundle_group, + pt_demuxing_enabled_audio, pt_demuxing_enabled_video]() { + for (const auto& it : channels_to_update) { + RtpTransceiverDirection local_direction = it.first; + cricket::ChannelInterface* channel = it.second; + cricket::MediaType media_type = channel->media_type(); + bool in_bundle_group = (bundle_group && bundle_group->HasContentName( + channel->content_name())); + if (media_type == cricket::MediaType::MEDIA_TYPE_AUDIO) { + if (!channel->SetPayloadTypeDemuxingEnabled( + (!in_bundle_group || pt_demuxing_enabled_audio) && + RtpTransceiverDirectionHasRecv(local_direction))) { + return false; + } + } else if (media_type == cricket::MediaType::MEDIA_TYPE_VIDEO) { + if (!channel->SetPayloadTypeDemuxingEnabled( + (!in_bundle_group || pt_demuxing_enabled_video) && + RtpTransceiverDirectionHasRecv(local_direction))) { + return false; } } - }); - } + } + return true; + }); } } // namespace webrtc diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index bd49e3479b..6483ef4b21 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -490,7 +490,7 @@ class SdpOfferAnswerHandler { const std::string GetTransportName(const std::string& content_name); // Based on number of transceivers per media type, enabled or disable // payload type based demuxing in the affected channels. - void UpdatePayloadTypeDemuxingState(cricket::ContentSource source); + bool UpdatePayloadTypeDemuxingState(cricket::ContentSource source); // ================================================================== // Access to pc_ variables diff --git a/pc/test/mock_channel_interface.h b/pc/test/mock_channel_interface.h index 3c3a4ee67e..1be4dcb0ce 100644 --- a/pc/test/mock_channel_interface.h +++ b/pc/test/mock_channel_interface.h @@ -46,7 +46,7 @@ class MockChannelInterface : public cricket::ChannelInterface { webrtc::SdpType, std::string*), (override)); - MOCK_METHOD(void, SetPayloadTypeDemuxingEnabled, (bool), (override)); + MOCK_METHOD(bool, SetPayloadTypeDemuxingEnabled, (bool), (override)); MOCK_METHOD(const std::vector&, local_streams, (),