Enable payload type based demuxing with multiple tracks when applicable.
This fixes regressions caused by:
https://webrtc-review.googlesource.com/c/src/+/183120
... which disabled payload type demuxing when multiple video tracks are
present, to avoid one channel creating a default track intended for
another channel.
However, this isn't an issue when not bundling, as each track will be
delivered on separate transport.
And it's also not an issue when each track uses a distinct set of
payload types (e.g., VP8 is mapped to PT 96 in one m= section, and PT 97
in another).
This CL addresses both of those cases; PT demuxing is only disabled
when two bundled m= sections have overlapping payload types.
Bug: chromium:1139052, webrtc:12029
Change-Id: Ied844bffac2a5fac29147c11b56a5f83a95ecb36
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/187560
Commit-Queue: Taylor <deadbeef@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32419}
diff --git a/pc/channel.cc b/pc/channel.cc
index 0f6cdd7..02ee9d2 100644
--- a/pc/channel.cc
+++ b/pc/channel.cc
@@ -290,9 +290,9 @@
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<void>(
+ return InvokeOnWorker<bool>(
RTC_FROM_HERE,
Bind(&BaseChannel::SetPayloadTypeDemuxingEnabled_w, this, enabled));
}
@@ -595,11 +595,12 @@
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 @@
// 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<StreamParams>& streams,
@@ -754,6 +767,7 @@
// 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 @@
if (payload_type_demuxing_enabled_) {
demuxer_criteria_.payload_types.insert(static_cast<uint8_t>(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<uint8_t>(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 e99ba24..51cc40f 100644
--- a/pc/channel.h
+++ b/pc/channel.h
@@ -134,7 +134,7 @@
// 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 @@
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 @@
webrtc::RtpTransceiverDirection remote_content_direction_ =
webrtc::RtpTransceiverDirection::kInactive;
+ // Cached list of payload types, used if payload type demuxing is re-enabled.
+ std::set<uint8_t> 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 f510c94..68b6486 100644
--- a/pc/channel_interface.h
+++ b/pc/channel_interface.h
@@ -52,7 +52,7 @@
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<StreamParams>& local_streams() const = 0;
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc
index 0a298b1..a72dbc6 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -14,6 +14,7 @@
#include <stdio.h>
+#include <algorithm>
#include <functional>
#include <list>
#include <map>
@@ -2786,6 +2787,106 @@
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<cricket::VideoCodec> 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 022e5e5..d03c292 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -12,6 +12,7 @@
#include <algorithm>
#include <queue>
+#include <set>
#include "api/media_stream_proxy.h"
#include "api/uma_metrics.h"
@@ -4033,7 +4034,13 @@
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 @@
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<int> audio_payload_types;
+ std::set<int> 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 @@
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 @@
transceiver->internal()->channel());
}
- if (!channels_to_update.empty()) {
- pc_->worker_thread()->Invoke<void>(
- 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<bool>(
+ 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 bd49e34..6483ef4 100644
--- a/pc/sdp_offer_answer.h
+++ b/pc/sdp_offer_answer.h
@@ -490,7 +490,7 @@
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 3c3a4ee..1be4dcb0c 100644
--- a/pc/test/mock_channel_interface.h
+++ b/pc/test/mock_channel_interface.h
@@ -46,7 +46,7 @@
webrtc::SdpType,
std::string*),
(override));
- MOCK_METHOD(void, SetPayloadTypeDemuxingEnabled, (bool), (override));
+ MOCK_METHOD(bool, SetPayloadTypeDemuxingEnabled, (bool), (override));
MOCK_METHOD(const std::vector<StreamParams>&,
local_streams,
(),
