admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Disable PT based demuxing if MID header extension is present.

Browse Source 
We want to turn off PT based demux because SSRC-based endpoints that
send media prematurely (which is a popular non-standard behavior still
heavily in use) can otherwise get incorrect mappings and unsignalled
ssrc issues because of the PT demux path.

This CL disables PT based demuxing when the MID header extension is
present on all m= sections in the SDP for that kind (audio/video), not
caring if it was in the offer or answer. However if PT demuxing has been
used in the past then it is always allowed. This ensures PT is off by
default but that either offer or answer can enable PT and once it has
been on it is also possible to get early media with PT.

- Want PT-based demux? The MID header extension has to be removed in
  either the offer or the answer. Follow-up O/As allow PT demuxing if
  possible.
- Want to use MID or SSRC demuxing? Great, you don't need PT-based demux
  and won't mind that we turned it off for you.

The reason for disabling PT demux at offer time (if MID is present)
instead of waiting for the SDP answer is because by the time the SDP
answer arrives, early media could have triggered PT demux and caused
incorrect mappings. The safe thing is to assume a spec-compliant
endpoint until proven otherwise.

However if PT demux is ever enabled, then from that point on we always
allow PT-based demux in follow-up O/A exchanges. This ensures we don't
drop packets in follow-up exchanges. The fact that PT-based demux is
disabled during the initial offer should not matter because before the
initial O/A exchange we don't have fingerprints.

This change only affects Unified Plan and bundled groups. Existing test
coverage ensuring we do not break legacy endpoints:
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/pc/peer_connection_integrationtest.cc;l=1156
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/webrtc/protocol/rtp-demuxing.html;l=59

UnsignaledStreamTest is also updated to test the interesting setups.
A kill-switch is added in case we want to disable this change.

Bug: webrtc:12814
Change-Id: I807a82a543325753633aaef698e06cb4c9dfebaa
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/221101
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34251}
This commit is contained in:
Henrik Boström 2021-06-09 10:29:50 +02:00 committed by WebRTC LUCI CQ
parent 33e75bb874
commit 4ea80f35f1
3 changed files with 216 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

92
pc/sdp_offer_answer.cc
View File

@ -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<webrtc::DtlsTransport> 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<int> audio_payload_types;
std::set<int> 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<const cricket::ContentGroup*, PayloadTypes> 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<bool>(
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;
}
}

5
pc/sdp_offer_answer.h
View File

@ -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

148
test/peer_scenario/tests/unsignaled_stream_test.cc
View File

@ -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<MidTestConfiguration> info) {
switch (info.param) {
case MidTestConfiguration::kMidNotNegotiated:
return "MidNotNegotiated";
case MidTestConfiguration::kMidNegotiatedButMissingFromPackets:
return "MidNegotiatedButMissingFromPackets";
case MidTestConfiguration::kMidNegotiatedAndPresentInPackets:
return "MidNegotiatedAndPresentInPackets";
}
}
class FrameObserver : public rtc::VideoSinkInterface<VideoFrame> {
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<MidTestConfiguration> {};
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<bool> 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<int> 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<RtpExtension> 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<uint32_t>::ReadBigEndian(&(packet.cdata()[8])) ==
first_ssrc &&
!got_unsignaled_packet) {
rtc::CopyOnWriteBuffer updated_buffer = packet.data;
ByteWriter<uint32_t>::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<RtpExtension> 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<RtpMid>());
break;
case MidTestConfiguration::kMidNegotiatedButMissingFromPackets:
EXPECT_TRUE(parsed_packet.HasExtension<RtpMid>());
ASSERT_TRUE(parsed_packet.RemoveExtension(RtpMid::kId));
break;
case MidTestConfiguration::kMidNegotiatedAndPresentInPackets:
EXPECT_TRUE(parsed_packet.HasExtension<RtpMid>());
// 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<RtpMid>("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