diff --git a/pc/channel.cc b/pc/channel.cc index 7b6c0c8704..7c386c8188 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -302,6 +302,12 @@ bool BaseChannel::SetRemoteContent(const MediaContentDescription* content, } 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 4332cd6df8..235d508c35 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -4947,29 +4947,6 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( } } - // Gather all updates ahead of time so that all channels can be updated in a - // single Invoke; necessary due to thread guards. - std::vector> - 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 @@ bool SdpOfferAnswerHandler::UpdatePayloadTypeDemuxingState( 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> 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( - 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;