Refactor UpdatePayloadTypeDemuxingState and add documentation.
This simplifies the work that happens on the worker thread in
preparation of avoiding having to go to the worker at all.
Bug: webrtc:11993
Change-Id: I13f063bdecce8efdb978ef1976c819019f30e020
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/244082
Auto-Submit: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35610}
diff --git a/pc/channel.cc b/pc/channel.cc
index 7b6c0c8..7c386c8 100644
--- a/pc/channel.cc
+++ b/pc/channel.cc
@@ -302,6 +302,12 @@
}
bool BaseChannel::SetPayloadTypeDemuxingEnabled(bool enabled) {
+ // TODO(bugs.webrtc.org/11993): The demuxer state needs to be managed on the
+ // network thread. At the moment there's a workaround for inconsistent state
+ // between the worker and network thread because of this (see
+ // OnDemuxerCriteriaUpdatePending elsewhere in this file) and
+ // SetPayloadTypeDemuxingEnabled_w has an Invoke over to the network thread
+ // to apply state updates.
RTC_DCHECK_RUN_ON(worker_thread());
TRACE_EVENT0("webrtc", "BaseChannel::SetPayloadTypeDemuxingEnabled");
return SetPayloadTypeDemuxingEnabled_w(enabled);
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index 4332cd6..235d508 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -4947,29 +4947,6 @@
}
}
- // Gather all updates ahead of time so that all channels can be updated in a
- // single Invoke; necessary due to thread guards.
- std::vector<std::pair<RtpTransceiverDirection, cricket::ChannelInterface*>>
- channels_to_update;
- for (const auto& transceiver : transceivers()->ListInternal()) {
- cricket::ChannelInterface* channel = transceiver->channel();
- const ContentInfo* content =
- FindMediaSectionForTransceiver(transceiver, sdesc);
- if (!channel || !content) {
- continue;
- }
- RtpTransceiverDirection local_direction =
- content->media_description()->direction();
- if (source == cricket::CS_REMOTE) {
- local_direction = RtpTransceiverDirectionReversed(local_direction);
- }
- channels_to_update.emplace_back(local_direction, transceiver->channel());
- }
-
- 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=
@@ -4996,44 +4973,71 @@
bundled_pt_demux_allowed_video = true;
}
+ // Gather all updates ahead of time so that all channels can be updated in a
+ // single Invoke; necessary due to thread guards.
+ std::vector<std::pair<bool, cricket::ChannelInterface*>> channels_to_update;
+ for (const auto& transceiver : transceivers()->ListInternal()) {
+ cricket::ChannelInterface* channel = transceiver->channel();
+ const ContentInfo* content =
+ FindMediaSectionForTransceiver(transceiver, sdesc);
+ if (!channel || !content) {
+ continue;
+ }
+
+ const cricket::MediaType media_type = channel->media_type();
+ if (media_type != cricket::MediaType::MEDIA_TYPE_AUDIO &&
+ media_type != cricket::MediaType::MEDIA_TYPE_VIDEO) {
+ continue;
+ }
+
+ RtpTransceiverDirection local_direction =
+ content->media_description()->direction();
+ if (source == cricket::CS_REMOTE) {
+ local_direction = RtpTransceiverDirectionReversed(local_direction);
+ }
+
+ auto bundle_it = bundle_groups_by_mid.find(channel->content_name());
+ const cricket::ContentGroup* bundle_group =
+ bundle_it != bundle_groups_by_mid.end() ? bundle_it->second : nullptr;
+ bool pt_demux_enabled = RtpTransceiverDirectionHasRecv(local_direction);
+ if (media_type == cricket::MediaType::MEDIA_TYPE_AUDIO) {
+ pt_demux_enabled &=
+ !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;
+ }
+ } else {
+ RTC_DCHECK_EQ(media_type, cricket::MediaType::MEDIA_TYPE_VIDEO);
+ pt_demux_enabled &=
+ !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;
+ }
+ }
+
+ channels_to_update.emplace_back(pt_demux_enabled, transceiver->channel());
+ }
+
+ if (channels_to_update.empty()) {
+ return true;
+ }
+
+ // TODO(bugs.webrtc.org/11993): This Invoke() will also invoke on the network
+ // thread for every demuxer sink that needs to be updated. The demuxer state
+ // needs to be fully (and only) managed on the network thread and once that's
+ // the case, there's no need to stop by on the worker. Ideally we could also
+ // do this without blocking.
return pc_->worker_thread()->Invoke<bool>(
- RTC_FROM_HERE,
- [&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_]() {
+ RTC_FROM_HERE, [&channels_to_update]() {
for (const auto& it : channels_to_update) {
- RtpTransceiverDirection local_direction = it.first;
- cricket::ChannelInterface* channel = it.second;
- cricket::MediaType media_type = channel->media_type();
- auto bundle_it = bundle_groups_by_mid.find(channel->content_name());
- const cricket::ContentGroup* bundle_group =
- bundle_it != bundle_groups_by_mid.end() ? bundle_it->second
- : nullptr;
- if (media_type == cricket::MediaType::MEDIA_TYPE_AUDIO) {
- 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) {
- 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;
- }
+ if (!it.second->SetPayloadTypeDemuxingEnabled(it.first)) {
+ // Note that the state has already been irrevocably changed at this
+ // point. Is it useful to stop the loop?
+ return false;
}
}
return true;
