diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 8aa194781e..2bfb61a15f 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -60,6 +60,7 @@ #include "rtc_base/strings/string_builder.h" #include "rtc_base/third_party/sigslot/sigslot.h" #include "rtc_base/trace_event.h" +#include "system_wrappers/include/field_trial.h" #include "system_wrappers/include/metrics.h" using cricket::ContentInfo; @@ -87,6 +88,9 @@ namespace { typedef webrtc::PeerConnectionInterface::RTCOfferAnswerOptions RTCOfferAnswerOptions; +constexpr const char* kAlwaysAllowPayloadTypeDemuxingFieldTrialName = + "WebRTC-AlwaysAllowPayloadTypeDemuxing"; + // Error messages const char kInvalidSdp[] = "Invalid session description."; const char kInvalidCandidates[] = "Description contains invalid candidates."; @@ -738,6 +742,17 @@ rtc::scoped_refptr LookupDtlsTransportByMid( [controller, &mid] { return controller->LookupDtlsTransportByMid(mid); }); } +bool ContentHasHeaderExtension(const cricket::ContentInfo& content_info, + absl::string_view header_extension_uri) { + for (const RtpExtension& rtp_header_extension : + content_info.media_description()->rtp_header_extensions()) { + if (rtp_header_extension.uri == header_extension_uri) { + return true; + } + } + return false; +} + } // namespace // Used by parameterless SetLocalDescription() to create an offer or answer. @@ -4840,10 +4855,13 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( struct PayloadTypes { std::set audio_payload_types; std::set video_payload_types; - bool pt_demuxing_enabled_audio = true; - bool pt_demuxing_enabled_video = true; + bool pt_demuxing_possible_audio = true; + bool pt_demuxing_possible_video = true; }; std::map payload_types_by_bundle; + // If the MID is missing from *any* receiving m= section, this is set to true. + bool mid_header_extension_missing_audio = false; + bool mid_header_extension_missing_video = false; for (auto& content_info : sdesc->description()->contents()) { auto it = bundle_groups_by_mid.find(content_info.name); const cricket::ContentGroup* bundle_group = @@ -4867,26 +4885,34 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( } switch (content_info.media_description()->type()) { case cricket::MediaType::MEDIA_TYPE_AUDIO: { + if (!mid_header_extension_missing_audio) { + mid_header_extension_missing_audio = + !ContentHasHeaderExtension(content_info, RtpExtension::kMidUri); + } const cricket::AudioContentDescription* audio_desc = content_info.media_description()->as_audio(); for (const cricket::AudioCodec& audio : audio_desc->codecs()) { if (payload_types->audio_payload_types.count(audio.id)) { // Two m= sections are using the same payload type, thus demuxing // by payload type is not possible. - payload_types->pt_demuxing_enabled_audio = false; + payload_types->pt_demuxing_possible_audio = false; } payload_types->audio_payload_types.insert(audio.id); } break; } case cricket::MediaType::MEDIA_TYPE_VIDEO: { + if (!mid_header_extension_missing_video) { + mid_header_extension_missing_video = + !ContentHasHeaderExtension(content_info, RtpExtension::kMidUri); + } const cricket::VideoContentDescription* video_desc = content_info.media_description()->as_video(); for (const cricket::VideoCodec& video : video_desc->codecs()) { if (payload_types->video_payload_types.count(video.id)) { // Two m= sections are using the same payload type, thus demuxing // by payload type is not possible. - payload_types->pt_demuxing_enabled_video = false; + payload_types->pt_demuxing_possible_video = false; } payload_types->video_payload_types.insert(video.id); } @@ -4920,9 +4946,39 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( if (channels_to_update.empty()) { return true; } + + // In Unified Plan, payload type demuxing is useful for legacy endpoints that + // don't support the MID header extension, but it can also cause incorrrect + // forwarding of packets when going from one m= section to multiple m= + // sections in the same BUNDLE. This only happens if media arrives prior to + // negotiation, but this can cause missing video and unsignalled ssrc bugs + // severe enough to warrant disabling PT demuxing in such cases. Therefore, if + // a MID header extension is present on all m= sections for a given kind + // (audio/video) then we use that as an OK to disable payload type demuxing in + // BUNDLEs of that kind. However if PT demuxing was ever turned on (e.g. MID + // was ever removed on ANY m= section of that kind) then we continue to allow + // PT demuxing in order to prevent disabling it in follow-up O/A exchanges and + // allowing early media by PT. + bool bundled_pt_demux_allowed_audio = !IsUnifiedPlan() || + mid_header_extension_missing_audio || + pt_demuxing_has_been_used_audio_; + bool bundled_pt_demux_allowed_video = !IsUnifiedPlan() || + mid_header_extension_missing_video || + pt_demuxing_has_been_used_video_; + // Kill switch for the above change. + if (field_trial::IsEnabled(kAlwaysAllowPayloadTypeDemuxingFieldTrialName)) { + // TODO(https://crbug.com/webrtc/12814): If disabling PT-based demux does + // not trigger regressions, remove this kill switch. + bundled_pt_demux_allowed_audio = true; + bundled_pt_demux_allowed_video = true; + } + return pc_->worker_thread()->Invoke( RTC_FROM_HERE, - [&channels_to_update, &bundle_groups_by_mid, &payload_types_by_bundle]() { + [&channels_to_update, &bundle_groups_by_mid, &payload_types_by_bundle, + bundled_pt_demux_allowed_audio, bundled_pt_demux_allowed_video, + pt_demuxing_has_been_used_audio = &pt_demuxing_has_been_used_audio_, + pt_demuxing_has_been_used_video = &pt_demuxing_has_been_used_video_]() { for (const auto& it : channels_to_update) { RtpTransceiverDirection local_direction = it.first; cricket::ChannelInterface* channel = it.second; @@ -4932,17 +4988,27 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( bundle_it != bundle_groups_by_mid.end() ? bundle_it->second : nullptr; if (media_type == cricket::MediaType::MEDIA_TYPE_AUDIO) { - if (!channel->SetPayloadTypeDemuxingEnabled( - (!bundle_group || payload_types_by_bundle[bundle_group] - .pt_demuxing_enabled_audio) && - RtpTransceiverDirectionHasRecv(local_direction))) { + bool pt_demux_enabled = + RtpTransceiverDirectionHasRecv(local_direction) && + (!bundle_group || (bundled_pt_demux_allowed_audio && + payload_types_by_bundle[bundle_group] + .pt_demuxing_possible_audio)); + if (pt_demux_enabled) { + *pt_demuxing_has_been_used_audio = true; + } + if (!channel->SetPayloadTypeDemuxingEnabled(pt_demux_enabled)) { return false; } } else if (media_type == cricket::MediaType::MEDIA_TYPE_VIDEO) { - if (!channel->SetPayloadTypeDemuxingEnabled( - (!bundle_group || payload_types_by_bundle[bundle_group] - .pt_demuxing_enabled_video) && - RtpTransceiverDirectionHasRecv(local_direction))) { + bool pt_demux_enabled = + RtpTransceiverDirectionHasRecv(local_direction) && + (!bundle_group || (bundled_pt_demux_allowed_video && + payload_types_by_bundle[bundle_group] + .pt_demuxing_possible_video)); + if (pt_demux_enabled) { + *pt_demuxing_has_been_used_video = true; + } + if (!channel->SetPayloadTypeDemuxingEnabled(pt_demux_enabled)) { return false; } } diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 1ef124baec..e5b39b83e7 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -629,6 +629,11 @@ class SdpOfferAnswerHandler : public SdpStateProvider, uint32_t negotiation_needed_event_id_ = 0; bool update_negotiation_needed_on_empty_chain_ RTC_GUARDED_BY(signaling_thread()) = false; + // If PT demuxing is successfully negotiated one time we will allow PT + // demuxing for the rest of the session so that PT-based apps default to PT + // demuxing in follow-up O/A exchanges. + bool pt_demuxing_has_been_used_audio_ = false; + bool pt_demuxing_has_been_used_video_ = false; // In Unified Plan, if we encounter remote SDP that does not contain an a=msid // line we create and use a stream with a random ID for our receivers. This is diff --git a/test/peer_scenario/tests/unsignaled_stream_test.cc b/test/peer_scenario/tests/unsignaled_stream_test.cc index 95510a24bd..edcfb36ea3 100644 --- a/test/peer_scenario/tests/unsignaled_stream_test.cc +++ b/test/peer_scenario/tests/unsignaled_stream_test.cc @@ -24,6 +24,31 @@ namespace webrtc { namespace test { namespace { +enum class MidTestConfiguration { + // Legacy endpoint setup where PT demuxing is used. + kMidNotNegotiated, + // MID is negotiated but missing from packets. PT demuxing is disabled, so + // SSRCs have to be added to the SDP for WebRTC to forward packets correctly. + // Happens when client is spec compliant but the SFU isn't. Popular legacy. + kMidNegotiatedButMissingFromPackets, + // Fully spec-compliant: MID is present so we can safely drop packets with + // unknown MIDs. + kMidNegotiatedAndPresentInPackets, +}; + +// Gives the parameterized test a readable suffix. +std::string TestParametersMidTestConfigurationToString( + testing::TestParamInfo info) { + switch (info.param) { + case MidTestConfiguration::kMidNotNegotiated: + return "MidNotNegotiated"; + case MidTestConfiguration::kMidNegotiatedButMissingFromPackets: + return "MidNegotiatedButMissingFromPackets"; + case MidTestConfiguration::kMidNegotiatedAndPresentInPackets: + return "MidNegotiatedAndPresentInPackets"; + } +} + class FrameObserver : public rtc::VideoSinkInterface { public: FrameObserver() : frame_observed_(false) {} @@ -53,19 +78,24 @@ void set_ssrc(SessionDescriptionInterface* offer, size_t index, uint32_t ssrc) { } // namespace -TEST(UnsignaledStreamTest, ReplacesUnsignaledStreamOnCompletedSignaling) { +class UnsignaledStreamTest + : public ::testing::Test, + public ::testing::WithParamInterface {}; + +TEST_P(UnsignaledStreamTest, ReplacesUnsignaledStreamOnCompletedSignaling) { // This test covers a scenario that might occur if a remote client starts - // sending media packets before negotiation has completed. These packets will - // trigger an unsignalled default stream to be created, and connects that to - // a default video sink. - // In some edge cases using unified plan, the default stream is create in a - // different transceiver to where the media SSRC will actually be used. - // This test verifies that the default stream is removed properly, and that - // packets are demuxed and video frames reach the desired sink. + // sending media packets before negotiation has completed. Depending on setup, + // these packets either get dropped or trigger an unsignalled default stream + // to be created, and connects that to a default video sink. + // In some edge cases using Unified Plan and PT demuxing, the default stream + // is create in a different transceiver to where the media SSRC will actually + // be used. This test verifies that the default stream is removed properly, + // and that packets are demuxed and video frames reach the desired sink. + const MidTestConfiguration kMidTestConfiguration = GetParam(); // Defined before PeerScenario so it gets destructed after, to avoid use after // free. - PeerScenario s(*test_info_); + PeerScenario s(*::testing::UnitTest::GetInstance()->current_test_info()); PeerScenarioClient::Config config = PeerScenarioClient::Config(); // Disable encryption so that we can inject a fake early media packet without @@ -93,14 +123,61 @@ TEST(UnsignaledStreamTest, ReplacesUnsignaledStreamOnCompletedSignaling) { std::atomic got_unsignaled_packet(false); // We will capture the media ssrc of the first added stream, and preemptively - // inject a new media packet using a different ssrc. - // This will create "default stream" for the second ssrc and connected it to - // the default video sink (not set in this test). + // inject a new media packet using a different ssrc. What happens depends on + // the test configuration. + // + // MidTestConfiguration::kMidNotNegotiated: + // - MID is not negotiated which means PT-based demuxing is enabled. Because + // the packets have no MID, the second ssrc packet gets forwarded to the + // first m= section. This will create a "default stream" for the second ssrc + // and connect it to the default video sink (not set in this test). The test + // verifies we can recover from this when we later get packets for the first + // ssrc. + // + // MidTestConfiguration::kMidNegotiatedButMissingFromPackets: + // - MID is negotiated wich means PT-based demuxing is disabled. Because we + // modify the packets not to contain the MID anyway (simulating a legacy SFU + // that does not negotiate properly) unknown SSRCs are dropped but do not + // otherwise cause any issues. + // + // MidTestConfiguration::kMidNegotiatedAndPresentInPackets: + // - MID is negotiated which means PT-based demuxing is enabled. In this case + // the packets have the MID so they either get forwarded or dropped + // depending on if the MID is known. The spec-compliant way is also the most + // straight-forward one. + uint32_t first_ssrc = 0; uint32_t second_ssrc = 0; + absl::optional mid_header_extension_id = absl::nullopt; signaling.NegotiateSdp( - /* munge_sdp = */ {}, + /* munge_sdp = */ + [&](SessionDescriptionInterface* offer) { + // Obtain the MID header extension ID and if we want the + // MidTestConfiguration::kMidNotNegotiated setup then we remove the MID + // header extension through SDP munging (otherwise SDP is not modified). + for (cricket::ContentInfo& content_info : + offer->description()->contents()) { + std::vector header_extensions = + content_info.media_description()->rtp_header_extensions(); + for (auto it = header_extensions.begin(); + it != header_extensions.end(); ++it) { + if (it->uri == RtpExtension::kMidUri) { + // MID header extension found! + mid_header_extension_id = it->id; + if (kMidTestConfiguration == + MidTestConfiguration::kMidNotNegotiated) { + // Munge away the extension. + header_extensions.erase(it); + } + break; + } + } + content_info.media_description()->set_rtp_header_extensions( + std::move(header_extensions)); + } + ASSERT_TRUE(mid_header_extension_id.has_value()); + }, /* modify_sdp = */ [&](SessionDescriptionInterface* offer) { first_ssrc = get_ssrc(offer, 0); @@ -113,9 +190,40 @@ TEST(UnsignaledStreamTest, ReplacesUnsignaledStreamOnCompletedSignaling) { if (ByteReader::ReadBigEndian(&(packet.cdata()[8])) == first_ssrc && !got_unsignaled_packet) { - rtc::CopyOnWriteBuffer updated_buffer = packet.data; - ByteWriter::WriteBigEndian( - updated_buffer.MutableData() + 8, second_ssrc); + // Parse packet and modify the SSRC to simulate a second m= + // section that has not been negotiated yet. + std::vector extensions; + extensions.emplace_back(RtpExtension::kMidUri, + mid_header_extension_id.value()); + RtpHeaderExtensionMap extensions_map(extensions); + RtpPacket parsed_packet; + parsed_packet.IdentifyExtensions(extensions_map); + ASSERT_TRUE(parsed_packet.Parse(packet.data)); + parsed_packet.SetSsrc(second_ssrc); + // The MID extension is present if and only if it was negotiated. + // If present, we either want to remove it or modify it depending + // on setup. + switch (kMidTestConfiguration) { + case MidTestConfiguration::kMidNotNegotiated: + EXPECT_FALSE(parsed_packet.HasExtension()); + break; + case MidTestConfiguration::kMidNegotiatedButMissingFromPackets: + EXPECT_TRUE(parsed_packet.HasExtension()); + ASSERT_TRUE(parsed_packet.RemoveExtension(RtpMid::kId)); + break; + case MidTestConfiguration::kMidNegotiatedAndPresentInPackets: + EXPECT_TRUE(parsed_packet.HasExtension()); + // The simulated second m= section would have a different MID. + // If we don't modify it here then |second_ssrc| would end up + // being mapped to the first m= section which would cause SSRC + // conflicts if we later add the same SSRC to a second m= + // section. Hidden assumption: first m= section does not use + // MID:1. + ASSERT_TRUE(parsed_packet.SetExtension("1")); + break; + } + // Inject the modified packet. + rtc::CopyOnWriteBuffer updated_buffer = parsed_packet.Buffer(); EmulatedIpPacket updated_packet( packet.from, packet.to, updated_buffer, packet.arrival_time); send_node->OnPacketReceived(std::move(updated_packet)); @@ -153,5 +261,13 @@ TEST(UnsignaledStreamTest, ReplacesUnsignaledStreamOnCompletedSignaling) { EXPECT_TRUE(s.WaitAndProcess(&second_sink.frame_observed_)); } +INSTANTIATE_TEST_SUITE_P( + All, + UnsignaledStreamTest, + ::testing::Values(MidTestConfiguration::kMidNotNegotiated, + MidTestConfiguration::kMidNegotiatedButMissingFromPackets, + MidTestConfiguration::kMidNegotiatedAndPresentInPackets), + TestParametersMidTestConfigurationToString); + } // namespace test } // namespace webrtc