Revert "Do all BaseChannel operations within a single Thread::Invoke." This reverts commit c1ad1ff178f0d0dfcde42843c51ae703005aaca1. Reason for revert: This blocks the worker thread for a longer contiguous period of time which can lead to delays in processing packets. And due to other recent changes, the need to speed up SetLocalDescription/SetRemoteDescription is reduced. Still plan to reland some of the changes from the CL, just not the part that groups the Invokes. Original change's description: > Do all BaseChannel operations within a single Thread::Invoke. > > Instead of doing a separate Invoke for each channel, this CL first > gathers a list of operations to be performed on the signaling thread, > then does a single Invoke on the worker thread (and nested Invoke > on the network thread) to update all channels at once. > > This includes the methods: > * Enable > * SetLocalContent/SetRemoteContent > * RegisterRtpDemuxerSink > * UpdateRtpHeaderExtensionMap > > Also, removed the need for a network thread Invoke in > IsReadyToSendMedia_w by moving ownership of was_ever_writable_ to the > worker thread. > > Bug: webrtc:12266 > Change-Id: I31e61fe0758aeb053b09db84f234deb58dfb3d05 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/194181 > Commit-Queue: Taylor <deadbeef@webrtc.org> > Reviewed-by: Harald Alvestrand <hta@webrtc.org> > Cr-Commit-Position: refs/heads/master@{#32817} TBR=deadbeef@webrtc.org,hta@webrtc.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:12266 Change-Id: I40ec519a614dc740133219f775b5638a488529b1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/203860 Reviewed-by: Taylor <deadbeef@webrtc.org> Reviewed-by: Harald Alvestrand <hta@webrtc.org> Commit-Queue: Taylor <deadbeef@webrtc.org> Cr-Commit-Position: refs/heads/master@{#33111}
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index cf28491..4dd5b6f 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc
@@ -2470,6 +2470,11 @@ // But all call-sites should be verifying this before calling us! RTC_DCHECK(session_error() == SessionError::kNone); + // If this is answer-ish we're ready to let media flow. + if (type == SdpType::kPrAnswer || type == SdpType::kAnswer) { + EnableSending(); + } + // Update the signaling state according to the specified state machine (see // https://w3c.github.io/webrtc-pc/#rtcsignalingstate-enum). if (type == SdpType::kOffer) { @@ -4191,6 +4196,21 @@ } } +void SdpOfferAnswerHandler::EnableSending() { + RTC_DCHECK_RUN_ON(signaling_thread()); + for (const auto& transceiver : transceivers()->List()) { + cricket::ChannelInterface* channel = transceiver->internal()->channel(); + if (channel && !channel->enabled()) { + channel->Enable(true); + } + } + + if (data_channel_controller()->rtp_data_channel() && + !data_channel_controller()->rtp_data_channel()->enabled()) { + data_channel_controller()->rtp_data_channel()->Enable(true); + } +} + RTCError SdpOfferAnswerHandler::PushdownMediaDescription( SdpType type, cricket::ContentSource source) { @@ -4200,13 +4220,15 @@ RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(sdesc); - // Gather lists of updates to be made on cricket channels on the signaling - // thread, before performing them all at once on the worker thread. Necessary - // due to threading restrictions. - auto payload_type_demuxing_updates = GetPayloadTypeDemuxingUpdates(source); - std::vector<ContentUpdate> content_updates; + 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."); + } - // Collect updates for each audio/video transceiver. + // Push down the new SDP media section for each audio/video transceiver. for (const auto& transceiver : transceivers()->List()) { const ContentInfo* content_info = FindMediaSectionForTransceiver(transceiver, sdesc); @@ -4216,12 +4238,19 @@ } const MediaContentDescription* content_desc = content_info->media_description(); - if (content_desc) { - content_updates.emplace_back(channel, content_desc); + if (!content_desc) { + continue; + } + std::string error; + bool success = (source == cricket::CS_LOCAL) + ? channel->SetLocalContent(content_desc, type, &error) + : channel->SetRemoteContent(content_desc, type, &error); + if (!success) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, error); } } - // If using the RtpDataChannel, add it to the list of updates. + // If using the RtpDataChannel, push down the new SDP section for it too. if (data_channel_controller()->rtp_data_channel()) { const ContentInfo* data_content = cricket::GetFirstDataContent(sdesc->description()); @@ -4229,23 +4258,21 @@ const MediaContentDescription* data_desc = data_content->media_description(); if (data_desc) { - content_updates.push_back( - {data_channel_controller()->rtp_data_channel(), data_desc}); + std::string error; + bool success = (source == cricket::CS_LOCAL) + ? data_channel_controller() + ->rtp_data_channel() + ->SetLocalContent(data_desc, type, &error) + : data_channel_controller() + ->rtp_data_channel() + ->SetRemoteContent(data_desc, type, &error); + if (!success) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, error); + } } } } - RTCError error = pc_->worker_thread()->Invoke<RTCError>( - RTC_FROM_HERE, - [this, type, source, &payload_type_demuxing_updates, &content_updates] { - return ApplyChannelUpdates(type, source, - std::move(payload_type_demuxing_updates), - std::move(content_updates)); - }); - if (!error.ok()) { - return error; - } - // Need complete offer/answer with an SCTP m= section before starting SCTP, // according to https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-19 if (pc_->sctp_mid() && local_description() && remote_description()) { @@ -4274,49 +4301,6 @@ return RTCError::OK(); } -RTCError SdpOfferAnswerHandler::ApplyChannelUpdates( - SdpType type, - cricket::ContentSource source, - std::vector<PayloadTypeDemuxingUpdate> payload_type_demuxing_updates, - std::vector<ContentUpdate> content_updates) { - RTC_DCHECK_RUN_ON(pc_->worker_thread()); - // If this is answer-ish we're ready to let media flow. - bool enable_sending = type == SdpType::kPrAnswer || type == SdpType::kAnswer; - std::set<cricket::ChannelInterface*> modified_channels; - for (const auto& update : payload_type_demuxing_updates) { - modified_channels.insert(update.channel); - update.channel->SetPayloadTypeDemuxingEnabled(update.enabled); - } - for (const auto& update : content_updates) { - modified_channels.insert(update.channel); - std::string error; - bool success = (source == cricket::CS_LOCAL) - ? update.channel->SetLocalContent( - update.content_description, type, &error) - : update.channel->SetRemoteContent( - update.content_description, type, &error); - if (!success) { - LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, error); - } - if (enable_sending && !update.channel->enabled()) { - update.channel->Enable(true); - } - } - // The above calls may have modified properties of the channel (header - // extension mappings, demuxer criteria) which still need to be applied to the - // RtpTransport. - return pc_->network_thread()->Invoke<RTCError>( - RTC_FROM_HERE, [modified_channels] { - for (auto channel : modified_channels) { - std::string error; - if (!channel->UpdateRtpTransport(&error)) { - LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, error); - } - } - return RTCError::OK(); - }); -} - RTCError SdpOfferAnswerHandler::PushdownTransportDescription( cricket::ContentSource source, SdpType type) { @@ -4909,8 +4893,7 @@ return ""; } -std::vector<SdpOfferAnswerHandler::PayloadTypeDemuxingUpdate> -SdpOfferAnswerHandler::GetPayloadTypeDemuxingUpdates( +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 @@ -4982,7 +4965,8 @@ // Gather all updates ahead of time so that all channels can be updated in a // single Invoke; necessary due to thread guards. - std::vector<PayloadTypeDemuxingUpdate> channel_updates; + std::vector<std::pair<RtpTransceiverDirection, cricket::ChannelInterface*>> + channels_to_update; for (const auto& transceiver : transceivers()->List()) { cricket::ChannelInterface* channel = transceiver->internal()->channel(); const ContentInfo* content = @@ -4995,22 +4979,38 @@ if (source == cricket::CS_REMOTE) { local_direction = RtpTransceiverDirectionReversed(local_direction); } - cricket::MediaType media_type = channel->media_type(); - bool in_bundle_group = - (bundle_group && bundle_group->HasContentName(channel->content_name())); - bool payload_type_demuxing_enabled = false; - if (media_type == cricket::MediaType::MEDIA_TYPE_AUDIO) { - payload_type_demuxing_enabled = - (!in_bundle_group || pt_demuxing_enabled_audio) && - RtpTransceiverDirectionHasRecv(local_direction); - } else if (media_type == cricket::MediaType::MEDIA_TYPE_VIDEO) { - payload_type_demuxing_enabled = - (!in_bundle_group || pt_demuxing_enabled_video) && - RtpTransceiverDirectionHasRecv(local_direction); - } - channel_updates.emplace_back(channel, payload_type_demuxing_enabled); + channels_to_update.emplace_back(local_direction, + transceiver->internal()->channel()); } - return channel_updates; + + 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