diff --git a/pc/mediasession.cc b/pc/mediasession.cc index 7239af86c3..263f3693db 100644 --- a/pc/mediasession.cc +++ b/pc/mediasession.cc @@ -18,6 +18,7 @@ #include #include +#include "absl/memory/memory.h" #include "absl/strings/match.h" #include "absl/types/optional.h" #include "api/cryptoparams.h" @@ -289,20 +290,15 @@ static void GenerateSsrcs(const StreamParamsVec& params_vec, } // Finds all StreamParams of all media types and attach them to stream_params. -static void GetCurrentStreamParams(const SessionDescription* sdesc, - StreamParamsVec* stream_params) { - RTC_DCHECK(stream_params); - if (!sdesc) { - return; - } - for (const ContentInfo& content : sdesc->contents()) { - if (!content.media_description()) { - continue; - } - for (const StreamParams& params : content.media_description()->streams()) { - stream_params->push_back(params); +static StreamParamsVec GetCurrentStreamParams( + const std::vector& active_local_contents) { + StreamParamsVec stream_params; + for (const ContentInfo* content : active_local_contents) { + for (const StreamParams& params : content->media_description()->streams()) { + stream_params.push_back(params); } } + return stream_params; } // Filters the data codecs for the data channel type. @@ -642,6 +638,23 @@ static bool UpdateCryptoParamsForBundle(const ContentGroup& bundle_group, return true; } +static std::vector GetActiveContents( + const SessionDescription& description, + const MediaSessionOptions& session_options) { + std::vector active_contents; + for (size_t i = 0; i < description.contents().size(); ++i) { + RTC_DCHECK_LT(i, session_options.media_description_options.size()); + const ContentInfo& content = description.contents()[i]; + const MediaDescriptionOptions& media_options = + session_options.media_description_options[i]; + if (!content.rejected && !media_options.stopped && + content.name == media_options.mid) { + active_contents.push_back(&content); + } + } + return active_contents; +} + template static bool ContainsRtxCodec(const std::vector& codecs) { for (const auto& codec : codecs) { @@ -1265,17 +1278,28 @@ void MediaSessionDescriptionFactory::set_audio_codecs( SessionDescription* MediaSessionDescriptionFactory::CreateOffer( const MediaSessionOptions& session_options, const SessionDescription* current_description) const { - std::unique_ptr offer(new SessionDescription()); + // Must have options for each existing section. + if (current_description) { + RTC_DCHECK_LE(current_description->contents().size(), + session_options.media_description_options.size()); + } IceCredentialsIterator ice_credentials( session_options.pooled_ice_credentials); - StreamParamsVec current_streams; - GetCurrentStreamParams(current_description, ¤t_streams); + + std::vector current_active_contents; + if (current_description) { + current_active_contents = + GetActiveContents(*current_description, session_options); + } + + StreamParamsVec current_streams = + GetCurrentStreamParams(current_active_contents); AudioCodecs offer_audio_codecs; VideoCodecs offer_video_codecs; DataCodecs offer_data_codecs; - GetCodecsForOffer(current_description, &offer_audio_codecs, + GetCodecsForOffer(current_active_contents, &offer_audio_codecs, &offer_video_codecs, &offer_data_codecs); if (!session_options.vad_enabled) { @@ -1287,14 +1311,10 @@ SessionDescription* MediaSessionDescriptionFactory::CreateOffer( RtpHeaderExtensions audio_rtp_extensions; RtpHeaderExtensions video_rtp_extensions; - GetRtpHdrExtsToOffer(session_options, current_description, + GetRtpHdrExtsToOffer(current_active_contents, session_options.is_unified_plan, &audio_rtp_extensions, &video_rtp_extensions); - // Must have options for each existing section. - if (current_description) { - RTC_DCHECK(current_description->contents().size() <= - session_options.media_description_options.size()); - } + auto offer = absl::make_unique(); // Iterate through the media description options, matching with existing media // descriptions in |current_description|. @@ -1306,7 +1326,7 @@ SessionDescription* MediaSessionDescriptionFactory::CreateOffer( msection_index < current_description->contents().size()) { current_content = ¤t_description->contents()[msection_index]; // Media type must match unless this media section is being recycled. - RTC_DCHECK(current_content->rejected || + RTC_DCHECK(current_content->name != media_description_options.mid || IsMediaContentOfType(current_content, media_description_options.type)); } @@ -1391,25 +1411,21 @@ SessionDescription* MediaSessionDescriptionFactory::CreateAnswer( return nullptr; } + // Must have options for exactly as many sections as in the offer. + RTC_DCHECK_EQ(offer->contents().size(), + session_options.media_description_options.size()); + IceCredentialsIterator ice_credentials( session_options.pooled_ice_credentials); - // The answer contains the intersection of the codecs in the offer with the - // codecs we support. As indicated by XEP-0167, we retain the same payload ids - // from the offer in the answer. - std::unique_ptr answer(new SessionDescription()); + std::vector current_active_contents; + if (current_description) { + current_active_contents = + GetActiveContents(*current_description, session_options); + } - StreamParamsVec current_streams; - GetCurrentStreamParams(current_description, ¤t_streams); - - // If the offer supports BUNDLE, and we want to use it too, create a BUNDLE - // group in the answer with the appropriate content names. - const ContentGroup* offer_bundle = offer->GetGroupByName(GROUP_TYPE_BUNDLE); - ContentGroup answer_bundle(GROUP_TYPE_BUNDLE); - // Transport info shared by the bundle group. - std::unique_ptr bundle_transport; - - answer->set_extmap_allow_mixed(offer->extmap_allow_mixed()); + StreamParamsVec current_streams = + GetCurrentStreamParams(current_active_contents); // Get list of all possible codecs that respects existing payload type // mappings and uses a single payload type space. @@ -1420,7 +1436,7 @@ SessionDescription* MediaSessionDescriptionFactory::CreateAnswer( AudioCodecs answer_audio_codecs; VideoCodecs answer_video_codecs; DataCodecs answer_data_codecs; - GetCodecsForAnswer(current_description, offer, &answer_audio_codecs, + GetCodecsForAnswer(current_active_contents, *offer, &answer_audio_codecs, &answer_video_codecs, &answer_data_codecs); if (!session_options.vad_enabled) { @@ -1430,9 +1446,17 @@ SessionDescription* MediaSessionDescriptionFactory::CreateAnswer( FilterDataCodecs(&answer_data_codecs, session_options.data_channel_type == DCT_SCTP); - // Must have options for exactly as many sections as in the offer. - RTC_DCHECK(offer->contents().size() == - session_options.media_description_options.size()); + auto answer = absl::make_unique(); + + // If the offer supports BUNDLE, and we want to use it too, create a BUNDLE + // group in the answer with the appropriate content names. + const ContentGroup* offer_bundle = offer->GetGroupByName(GROUP_TYPE_BUNDLE); + ContentGroup answer_bundle(GROUP_TYPE_BUNDLE); + // Transport info shared by the bundle group. + std::unique_ptr bundle_transport; + + answer->set_extmap_allow_mixed(offer->extmap_allow_mixed()); + // Iterate through the media description options, matching with existing // media descriptions in |current_description|. size_t msection_index = 0; @@ -1589,24 +1613,24 @@ const AudioCodecs& MediaSessionDescriptionFactory::GetAudioCodecsForAnswer( return audio_sendrecv_codecs_; } -void MergeCodecsFromDescription(const SessionDescription* description, - AudioCodecs* audio_codecs, - VideoCodecs* video_codecs, - DataCodecs* data_codecs, - UsedPayloadTypes* used_pltypes) { - RTC_DCHECK(description); - for (const ContentInfo& content : description->contents()) { - if (IsMediaContentOfType(&content, MEDIA_TYPE_AUDIO)) { +void MergeCodecsFromDescription( + const std::vector& current_active_contents, + AudioCodecs* audio_codecs, + VideoCodecs* video_codecs, + DataCodecs* data_codecs, + UsedPayloadTypes* used_pltypes) { + for (const ContentInfo* content : current_active_contents) { + if (IsMediaContentOfType(content, MEDIA_TYPE_AUDIO)) { const AudioContentDescription* audio = - content.media_description()->as_audio(); + content->media_description()->as_audio(); MergeCodecs(audio->codecs(), audio_codecs, used_pltypes); - } else if (IsMediaContentOfType(&content, MEDIA_TYPE_VIDEO)) { + } else if (IsMediaContentOfType(content, MEDIA_TYPE_VIDEO)) { const VideoContentDescription* video = - content.media_description()->as_video(); + content->media_description()->as_video(); MergeCodecs(video->codecs(), video_codecs, used_pltypes); - } else if (IsMediaContentOfType(&content, MEDIA_TYPE_DATA)) { + } else if (IsMediaContentOfType(content, MEDIA_TYPE_DATA)) { const DataContentDescription* data = - content.media_description()->as_data(); + content->media_description()->as_data(); MergeCodecs(data->codecs(), data_codecs, used_pltypes); } } @@ -1619,24 +1643,18 @@ void MergeCodecsFromDescription(const SessionDescription* description, // 3. For each individual media description (m= section), filter codecs based // on the directional attribute (happens in another method). void MediaSessionDescriptionFactory::GetCodecsForOffer( - const SessionDescription* current_description, + const std::vector& current_active_contents, AudioCodecs* audio_codecs, VideoCodecs* video_codecs, DataCodecs* data_codecs) const { - UsedPayloadTypes used_pltypes; - audio_codecs->clear(); - video_codecs->clear(); - data_codecs->clear(); - // First - get all codecs from the current description if the media type // is used. Add them to |used_pltypes| so the payload type is not reused if a // new media type is added. - if (current_description) { - MergeCodecsFromDescription(current_description, audio_codecs, video_codecs, - data_codecs, &used_pltypes); - } + UsedPayloadTypes used_pltypes; + MergeCodecsFromDescription(current_active_contents, audio_codecs, + video_codecs, data_codecs, &used_pltypes); - // Add our codecs that are not in |current_description|. + // Add our codecs that are not in the current description. MergeCodecs(all_audio_codecs_, audio_codecs, &used_pltypes); MergeCodecs(video_codecs_, video_codecs, &used_pltypes); MergeCodecs(data_codecs_, data_codecs, &used_pltypes); @@ -1650,29 +1668,23 @@ void MediaSessionDescriptionFactory::GetCodecsForOffer( // 4. For each individual media description (m= section), filter codecs based // on the directional attribute (happens in another method). void MediaSessionDescriptionFactory::GetCodecsForAnswer( - const SessionDescription* current_description, - const SessionDescription* remote_offer, + const std::vector& current_active_contents, + const SessionDescription& remote_offer, AudioCodecs* audio_codecs, VideoCodecs* video_codecs, DataCodecs* data_codecs) const { - UsedPayloadTypes used_pltypes; - audio_codecs->clear(); - video_codecs->clear(); - data_codecs->clear(); - // First - get all codecs from the current description if the media type // is used. Add them to |used_pltypes| so the payload type is not reused if a // new media type is added. - if (current_description) { - MergeCodecsFromDescription(current_description, audio_codecs, video_codecs, - data_codecs, &used_pltypes); - } + UsedPayloadTypes used_pltypes; + MergeCodecsFromDescription(current_active_contents, audio_codecs, + video_codecs, data_codecs, &used_pltypes); // Second - filter out codecs that we don't support at all and should ignore. AudioCodecs filtered_offered_audio_codecs; VideoCodecs filtered_offered_video_codecs; DataCodecs filtered_offered_data_codecs; - for (const ContentInfo& content : remote_offer->contents()) { + for (const ContentInfo& content : remote_offer.contents()) { if (IsMediaContentOfType(&content, MEDIA_TYPE_AUDIO)) { const AudioContentDescription* audio = content.media_description()->as_audio(); @@ -1712,7 +1724,7 @@ void MediaSessionDescriptionFactory::GetCodecsForAnswer( } } - // Add codecs that are not in |current_description| but were in + // Add codecs that are not in the current description but were in // |remote_offer|. MergeCodecs(filtered_offered_audio_codecs, audio_codecs, &used_pltypes); @@ -1722,9 +1734,10 @@ void MediaSessionDescriptionFactory::GetCodecsForAnswer( &used_pltypes); } +// TODO(steveanton): Replace |is_unified_plan| flag with a member variable. void MediaSessionDescriptionFactory::GetRtpHdrExtsToOffer( - const MediaSessionOptions& session_options, - const SessionDescription* current_description, + const std::vector& current_active_contents, + bool is_unified_plan, RtpHeaderExtensions* offer_audio_extensions, RtpHeaderExtensions* offer_video_extensions) const { // All header extensions allocated from the same range to avoid potential @@ -1732,43 +1745,40 @@ void MediaSessionDescriptionFactory::GetRtpHdrExtsToOffer( UsedRtpHeaderExtensionIds used_ids; RtpHeaderExtensions all_regular_extensions; RtpHeaderExtensions all_encrypted_extensions; - offer_audio_extensions->clear(); - offer_video_extensions->clear(); // First - get all extensions from the current description if the media type // is used. // Add them to |used_ids| so the local ids are not reused if a new media // type is added. - if (current_description) { - for (const ContentInfo& content : current_description->contents()) { - if (IsMediaContentOfType(&content, MEDIA_TYPE_AUDIO)) { - const AudioContentDescription* audio = - content.media_description()->as_audio(); - MergeRtpHdrExts(audio->rtp_header_extensions(), offer_audio_extensions, - &all_regular_extensions, &all_encrypted_extensions, - &used_ids); - } else if (IsMediaContentOfType(&content, MEDIA_TYPE_VIDEO)) { - const VideoContentDescription* video = - content.media_description()->as_video(); - MergeRtpHdrExts(video->rtp_header_extensions(), offer_video_extensions, - &all_regular_extensions, &all_encrypted_extensions, - &used_ids); - } + for (const ContentInfo* content : current_active_contents) { + if (IsMediaContentOfType(content, MEDIA_TYPE_AUDIO)) { + const AudioContentDescription* audio = + content->media_description()->as_audio(); + MergeRtpHdrExts(audio->rtp_header_extensions(), offer_audio_extensions, + &all_regular_extensions, &all_encrypted_extensions, + &used_ids); + } else if (IsMediaContentOfType(content, MEDIA_TYPE_VIDEO)) { + const VideoContentDescription* video = + content->media_description()->as_video(); + MergeRtpHdrExts(video->rtp_header_extensions(), offer_video_extensions, + &all_regular_extensions, &all_encrypted_extensions, + &used_ids); } } - // Add our default RTP header extensions that are not in - // |current_description|. - MergeRtpHdrExts(audio_rtp_header_extensions(session_options.is_unified_plan), + // Add our default RTP header extensions that are not in the current + // description. + MergeRtpHdrExts(audio_rtp_header_extensions(is_unified_plan), offer_audio_extensions, &all_regular_extensions, &all_encrypted_extensions, &used_ids); - MergeRtpHdrExts(video_rtp_header_extensions(session_options.is_unified_plan), + MergeRtpHdrExts(video_rtp_header_extensions(is_unified_plan), offer_video_extensions, &all_regular_extensions, &all_encrypted_extensions, &used_ids); // TODO(jbauch): Support adding encrypted header extensions to existing // sessions. - if (enable_encrypted_rtp_header_extensions_ && !current_description) { + if (enable_encrypted_rtp_header_extensions_ && + current_active_contents.empty()) { AddEncryptedVersionsOfHdrExts(offer_audio_extensions, &all_encrypted_extensions, &used_ids); AddEncryptedVersionsOfHdrExts(offer_video_extensions, @@ -1858,8 +1868,10 @@ bool MediaSessionDescriptionFactory::AddAudioContentForOffer( GetAudioCodecsForOffer(media_description_options.direction); AudioCodecs filtered_codecs; - // Add the codecs from current content if it exists and is not being recycled. - if (current_content && !current_content->rejected) { + // Add the codecs from current content if it exists and is not rejected nor + // recycled. + if (current_content && !current_content->rejected && + current_content->name == media_description_options.mid) { RTC_CHECK(IsMediaContentOfType(current_content, MEDIA_TYPE_AUDIO)); const AudioContentDescription* acd = current_content->media_description()->as_audio(); @@ -1934,8 +1946,10 @@ bool MediaSessionDescriptionFactory::AddVideoContentForOffer( &crypto_suites); VideoCodecs filtered_codecs; - // Add the codecs from current content if it exists and is not being recycled. - if (current_content && !current_content->rejected) { + // Add the codecs from current content if it exists and is not rejected nor + // recycled. + if (current_content && !current_content->rejected && + current_content->name == media_description_options.mid) { RTC_CHECK(IsMediaContentOfType(current_content, MEDIA_TYPE_VIDEO)); const VideoContentDescription* vcd = current_content->media_description()->as_video(); @@ -2097,8 +2111,10 @@ bool MediaSessionDescriptionFactory::AddAudioContentForAnswer( GetAudioCodecsForAnswer(offer_rtd, answer_rtd); AudioCodecs filtered_codecs; - // Add the codecs from current content if it exists and is not being recycled. - if (current_content && !current_content->rejected) { + // Add the codecs from current content if it exists and is not rejected nor + // recycled. + if (current_content && !current_content->rejected && + current_content->name == media_description_options.mid) { RTC_CHECK(IsMediaContentOfType(current_content, MEDIA_TYPE_AUDIO)); const AudioContentDescription* acd = current_content->media_description()->as_audio(); @@ -2183,8 +2199,10 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer( } VideoCodecs filtered_codecs; - // Add the codecs from current content if it exists and is not being recycled. - if (current_content && !current_content->rejected) { + // Add the codecs from current content if it exists and is not rejected nor + // recycled. + if (current_content && !current_content->rejected && + current_content->name == media_description_options.mid) { RTC_CHECK(IsMediaContentOfType(current_content, MEDIA_TYPE_VIDEO)); const VideoContentDescription* vcd = current_content->media_description()->as_video(); diff --git a/pc/mediasession.h b/pc/mediasession.h index 5904605f10..9495bdc293 100644 --- a/pc/mediasession.h +++ b/pc/mediasession.h @@ -173,19 +173,22 @@ class MediaSessionDescriptionFactory { const AudioCodecs& GetAudioCodecsForAnswer( const webrtc::RtpTransceiverDirection& offer, const webrtc::RtpTransceiverDirection& answer) const; - void GetCodecsForOffer(const SessionDescription* current_description, - AudioCodecs* audio_codecs, - VideoCodecs* video_codecs, - DataCodecs* data_codecs) const; - void GetCodecsForAnswer(const SessionDescription* current_description, - const SessionDescription* remote_offer, - AudioCodecs* audio_codecs, - VideoCodecs* video_codecs, - DataCodecs* data_codecs) const; - void GetRtpHdrExtsToOffer(const MediaSessionOptions& session_options, - const SessionDescription* current_description, - RtpHeaderExtensions* audio_extensions, - RtpHeaderExtensions* video_extensions) const; + void GetCodecsForOffer( + const std::vector& current_active_contents, + AudioCodecs* audio_codecs, + VideoCodecs* video_codecs, + DataCodecs* data_codecs) const; + void GetCodecsForAnswer( + const std::vector& current_active_contents, + const SessionDescription& remote_offer, + AudioCodecs* audio_codecs, + VideoCodecs* video_codecs, + DataCodecs* data_codecs) const; + void GetRtpHdrExtsToOffer( + const std::vector& current_active_contents, + bool is_unified_plan, + RtpHeaderExtensions* audio_extensions, + RtpHeaderExtensions* video_extensions) const; bool AddTransportOffer(const std::string& content_name, const TransportOptions& transport_options, const SessionDescription* current_desc, diff --git a/pc/mediasession_unittest.cc b/pc/mediasession_unittest.cc index 076ad12a5b..2098ec0819 100644 --- a/pc/mediasession_unittest.cc +++ b/pc/mediasession_unittest.cc @@ -2020,6 +2020,110 @@ TEST_F(MediaSessionDescriptionFactoryTest, EXPECT_THAT(updated_vcd->codecs(), ElementsAreArray(kUpdatedVideoCodecOffer)); } +// Test that a reoffer does not reuse audio codecs from a previous media section +// that is being recycled. +TEST_F(MediaSessionDescriptionFactoryTest, + ReOfferDoesNotReUseRecycledAudioCodecs) { + f1_.set_video_codecs({}); + f2_.set_video_codecs({}); + + MediaSessionOptions opts; + AddMediaSection(MEDIA_TYPE_AUDIO, "a0", RtpTransceiverDirection::kSendRecv, + kActive, &opts); + auto offer = absl::WrapUnique(f1_.CreateOffer(opts, nullptr)); + auto answer = absl::WrapUnique(f2_.CreateAnswer(offer.get(), opts, nullptr)); + + // Recycle the media section by changing its mid. + opts.media_description_options[0].mid = "a1"; + auto reoffer = absl::WrapUnique(f2_.CreateOffer(opts, answer.get())); + + // Expect that the results of the first negotiation are ignored. If the m= + // section was not recycled the payload types would match the initial offerer. + const AudioContentDescription* acd = + GetFirstAudioContentDescription(reoffer.get()); + EXPECT_THAT(acd->codecs(), ElementsAreArray(kAudioCodecs2)); +} + +// Test that a reoffer does not reuse video codecs from a previous media section +// that is being recycled. +TEST_F(MediaSessionDescriptionFactoryTest, + ReOfferDoesNotReUseRecycledVideoCodecs) { + f1_.set_audio_codecs({}, {}); + f2_.set_audio_codecs({}, {}); + + MediaSessionOptions opts; + AddMediaSection(MEDIA_TYPE_VIDEO, "v0", RtpTransceiverDirection::kSendRecv, + kActive, &opts); + auto offer = absl::WrapUnique(f1_.CreateOffer(opts, nullptr)); + auto answer = absl::WrapUnique(f2_.CreateAnswer(offer.get(), opts, nullptr)); + + // Recycle the media section by changing its mid. + opts.media_description_options[0].mid = "v1"; + auto reoffer = absl::WrapUnique(f2_.CreateOffer(opts, answer.get())); + + // Expect that the results of the first negotiation are ignored. If the m= + // section was not recycled the payload types would match the initial offerer. + const VideoContentDescription* vcd = + GetFirstVideoContentDescription(reoffer.get()); + EXPECT_THAT(vcd->codecs(), ElementsAreArray(kVideoCodecs2)); +} + +// Test that a reanswer does not reuse audio codecs from a previous media +// section that is being recycled. +TEST_F(MediaSessionDescriptionFactoryTest, + ReAnswerDoesNotReUseRecycledAudioCodecs) { + f1_.set_video_codecs({}); + f2_.set_video_codecs({}); + + // Perform initial offer/answer in reverse (|f2_| as offerer) so that the + // second offer/answer is forward (|f1_| as offerer). + MediaSessionOptions opts; + AddMediaSection(MEDIA_TYPE_AUDIO, "a0", RtpTransceiverDirection::kSendRecv, + kActive, &opts); + auto offer = absl::WrapUnique(f2_.CreateOffer(opts, nullptr)); + auto answer = absl::WrapUnique(f1_.CreateAnswer(offer.get(), opts, nullptr)); + + // Recycle the media section by changing its mid. + opts.media_description_options[0].mid = "a1"; + auto reoffer = absl::WrapUnique(f1_.CreateOffer(opts, answer.get())); + auto reanswer = + absl::WrapUnique(f2_.CreateAnswer(reoffer.get(), opts, offer.get())); + + // Expect that the results of the first negotiation are ignored. If the m= + // section was not recycled the payload types would match the initial offerer. + const AudioContentDescription* acd = + GetFirstAudioContentDescription(reanswer.get()); + EXPECT_THAT(acd->codecs(), ElementsAreArray(kAudioCodecsAnswer)); +} + +// Test that a reanswer does not reuse video codecs from a previous media +// section that is being recycled. +TEST_F(MediaSessionDescriptionFactoryTest, + ReAnswerDoesNotReUseRecycledVideoCodecs) { + f1_.set_audio_codecs({}, {}); + f2_.set_audio_codecs({}, {}); + + // Perform initial offer/answer in reverse (|f2_| as offerer) so that the + // second offer/answer is forward (|f1_| as offerer). + MediaSessionOptions opts; + AddMediaSection(MEDIA_TYPE_VIDEO, "v0", RtpTransceiverDirection::kSendRecv, + kActive, &opts); + auto offer = absl::WrapUnique(f2_.CreateOffer(opts, nullptr)); + auto answer = absl::WrapUnique(f1_.CreateAnswer(offer.get(), opts, nullptr)); + + // Recycle the media section by changing its mid. + opts.media_description_options[0].mid = "v1"; + auto reoffer = absl::WrapUnique(f1_.CreateOffer(opts, answer.get())); + auto reanswer = + absl::WrapUnique(f2_.CreateAnswer(reoffer.get(), opts, offer.get())); + + // Expect that the results of the first negotiation are ignored. If the m= + // section was not recycled the payload types would match the initial offerer. + const VideoContentDescription* vcd = + GetFirstVideoContentDescription(reanswer.get()); + EXPECT_THAT(vcd->codecs(), ElementsAreArray(kVideoCodecsAnswer)); +} + // Create an updated offer after creating an answer to the original offer and // verify that the codecs that were part of the original answer are not changed // in the updated offer. In this test Rtx is enabled. diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index e166d1eb82..c341888200 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -3947,6 +3947,17 @@ GetMediaDescriptionOptionsForTransceiver( return media_description_options; } +// Returns the ContentInfo at mline index |i|, or null if none exists. +static const ContentInfo* GetContentByIndex( + const SessionDescriptionInterface* sdesc, + size_t i) { + if (!sdesc) { + return nullptr; + } + const ContentInfos& contents = sdesc->description()->contents(); + return (i < contents.size() ? &contents[i] : nullptr); +} + void PeerConnection::GetOptionsForUnifiedPlanOffer( const RTCOfferAnswerOptions& offer_answer_options, cricket::MediaSessionOptions* session_options) { @@ -3974,10 +3985,15 @@ void PeerConnection::GetOptionsForUnifiedPlanOffer( // Either |local_content| or |remote_content| is non-null. const ContentInfo* local_content = (i < local_contents.size() ? &local_contents[i] : nullptr); + const ContentInfo* current_local_content = + GetContentByIndex(current_local_description(), i); const ContentInfo* remote_content = (i < remote_contents.size() ? &remote_contents[i] : nullptr); - bool had_been_rejected = (local_content && local_content->rejected) || - (remote_content && remote_content->rejected); + const ContentInfo* current_remote_content = + GetContentByIndex(current_remote_description(), i); + bool had_been_rejected = + (current_local_content && current_local_content->rejected) || + (current_remote_content && current_remote_content->rejected); const std::string& mid = (local_content ? local_content->name : remote_content->name); cricket::MediaType media_type = @@ -3988,7 +4004,7 @@ void PeerConnection::GetOptionsForUnifiedPlanOffer( auto transceiver = GetAssociatedTransceiver(mid); RTC_CHECK(transceiver); // A media section is considered eligible for recycling if it is marked as - // rejected in either the local or remote description. + // rejected in either the current local or current remote description. if (had_been_rejected && transceiver->stopped()) { session_options->media_description_options.push_back( cricket::MediaDescriptionOptions(transceiver->media_type(), mid, @@ -4180,12 +4196,13 @@ void PeerConnection::GenerateMediaDescriptionOptions( session_options->media_description_options.push_back( cricket::MediaDescriptionOptions( cricket::MEDIA_TYPE_AUDIO, content.name, - RtpTransceiverDirection::kInactive, true)); + RtpTransceiverDirection::kInactive, /*stopped=*/true)); } else { + bool stopped = (audio_direction == RtpTransceiverDirection::kInactive); session_options->media_description_options.push_back( - cricket::MediaDescriptionOptions( - cricket::MEDIA_TYPE_AUDIO, content.name, audio_direction, - audio_direction == RtpTransceiverDirection::kInactive)); + cricket::MediaDescriptionOptions(cricket::MEDIA_TYPE_AUDIO, + content.name, audio_direction, + stopped)); *audio_index = session_options->media_description_options.size() - 1; } } else if (IsVideoContent(&content)) { @@ -4194,12 +4211,13 @@ void PeerConnection::GenerateMediaDescriptionOptions( session_options->media_description_options.push_back( cricket::MediaDescriptionOptions( cricket::MEDIA_TYPE_VIDEO, content.name, - RtpTransceiverDirection::kInactive, true)); + RtpTransceiverDirection::kInactive, /*stopped=*/true)); } else { + bool stopped = (video_direction == RtpTransceiverDirection::kInactive); session_options->media_description_options.push_back( - cricket::MediaDescriptionOptions( - cricket::MEDIA_TYPE_VIDEO, content.name, video_direction, - video_direction == RtpTransceiverDirection::kInactive)); + cricket::MediaDescriptionOptions(cricket::MEDIA_TYPE_VIDEO, + content.name, video_direction, + stopped)); *video_index = session_options->media_description_options.size() - 1; } } else { diff --git a/pc/peerconnection_jsep_unittest.cc b/pc/peerconnection_jsep_unittest.cc index fe878b05f7..f0c9111885 100644 --- a/pc/peerconnection_jsep_unittest.cc +++ b/pc/peerconnection_jsep_unittest.cc @@ -700,6 +700,12 @@ TEST_F(PeerConnectionJsepTest, CreateOfferRecyclesWhenOfferingTwice) { // Test that the offer/answer and transceivers for both the caller and callee // side are generated/updated correctly when recycling an audio/video media // section as a media section of either the same or opposite type. +// Correct recycling works as follows: +// - The m= section is re-offered with a new MID value and the new media type. +// - The previously-associated transceiver is dissociated when the new offer is +// set as a local description on the offerer or as a remote description on +// the answerer. +// - The new transceiver is associated with the new MID value. class RecycleMediaSectionTest : public PeerConnectionJsepTest, public testing::WithParamInterface< @@ -714,7 +720,9 @@ class RecycleMediaSectionTest cricket::MediaType second_type_; }; -TEST_P(RecycleMediaSectionTest, VerifyOfferAnswerAndTransceivers) { +// Test that recycling works properly when a new transceiver recycles an m= +// section that was rejected in both the current local and remote descriptions. +TEST_P(RecycleMediaSectionTest, CurrentLocalAndCurrentRemoteRejected) { auto caller = CreatePeerConnection(); auto first_transceiver = caller->AddTransceiver(first_type_); auto callee = CreatePeerConnection(); @@ -774,6 +782,285 @@ TEST_P(RecycleMediaSectionTest, VerifyOfferAnswerAndTransceivers) { ASSERT_EQ(2u, callee->pc()->GetTransceivers().size()); } +// Test that recycling works properly when a new transceiver recycles an m= +// section that was rejected in only the current remote description. +TEST_P(RecycleMediaSectionTest, CurrentRemoteOnlyRejected) { + auto caller = CreatePeerConnection(); + auto caller_first_transceiver = caller->AddTransceiver(first_type_); + auto callee = CreatePeerConnection(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + std::string first_mid = *caller_first_transceiver->mid(); + ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); + auto callee_first_transceiver = callee->pc()->GetTransceivers()[0]; + callee_first_transceiver->Stop(); + + // The answer will have a rejected m= section. + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + + // The offer should reuse the previous media section but allocate a new MID + // and change the media type. + auto caller_second_transceiver = caller->AddTransceiver(second_type_); + auto offer = caller->CreateOffer(); + const auto& offer_contents = offer->description()->contents(); + ASSERT_EQ(1u, offer_contents.size()); + EXPECT_FALSE(offer_contents[0].rejected); + EXPECT_EQ(second_type_, offer_contents[0].media_description()->type()); + std::string second_mid = offer_contents[0].name; + EXPECT_NE(first_mid, second_mid); + + // Setting the local offer will dissociate the previous transceiver and set + // the MID for the new transceiver. + ASSERT_TRUE( + caller->SetLocalDescription(CloneSessionDescription(offer.get()))); + EXPECT_EQ(absl::nullopt, caller_first_transceiver->mid()); + EXPECT_EQ(second_mid, caller_second_transceiver->mid()); + + // Setting the remote offer will dissociate the previous transceiver and + // create a new transceiver for the media section. + ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer))); + auto callee_transceivers = callee->pc()->GetTransceivers(); + ASSERT_EQ(2u, callee_transceivers.size()); + EXPECT_EQ(absl::nullopt, callee_transceivers[0]->mid()); + EXPECT_EQ(first_type_, callee_transceivers[0]->media_type()); + EXPECT_EQ(second_mid, callee_transceivers[1]->mid()); + EXPECT_EQ(second_type_, callee_transceivers[1]->media_type()); + + // The answer should have only one media section for the new transceiver. + auto answer = callee->CreateAnswer(); + auto answer_contents = answer->description()->contents(); + ASSERT_EQ(1u, answer_contents.size()); + EXPECT_FALSE(answer_contents[0].rejected); + EXPECT_EQ(second_mid, answer_contents[0].name); + EXPECT_EQ(second_type_, answer_contents[0].media_description()->type()); + + // Setting the local answer should succeed. + ASSERT_TRUE( + callee->SetLocalDescription(CloneSessionDescription(answer.get()))); + + // Setting the remote answer should succeed and not create any new + // transceivers. + ASSERT_TRUE(caller->SetRemoteDescription(std::move(answer))); + ASSERT_EQ(2u, caller->pc()->GetTransceivers().size()); + ASSERT_EQ(2u, callee->pc()->GetTransceivers().size()); +} + +// Test that recycling works properly when a new transceiver recycles an m= +// section that was rejected only in the current local description. +TEST_P(RecycleMediaSectionTest, CurrentLocalOnlyRejected) { + auto caller = CreatePeerConnection(); + auto caller_first_transceiver = caller->AddTransceiver(first_type_); + auto callee = CreatePeerConnection(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + std::string first_mid = *caller_first_transceiver->mid(); + ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); + auto callee_first_transceiver = callee->pc()->GetTransceivers()[0]; + callee_first_transceiver->Stop(); + + // The answer will have a rejected m= section. + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + + // The offer should reuse the previous media section but allocate a new MID + // and change the media type. + auto callee_second_transceiver = callee->AddTransceiver(second_type_); + auto offer = callee->CreateOffer(); + const auto& offer_contents = offer->description()->contents(); + ASSERT_EQ(1u, offer_contents.size()); + EXPECT_FALSE(offer_contents[0].rejected); + EXPECT_EQ(second_type_, offer_contents[0].media_description()->type()); + std::string second_mid = offer_contents[0].name; + EXPECT_NE(first_mid, second_mid); + + // Setting the local offer will dissociate the previous transceiver and set + // the MID for the new transceiver. + ASSERT_TRUE( + callee->SetLocalDescription(CloneSessionDescription(offer.get()))); + EXPECT_EQ(absl::nullopt, callee_first_transceiver->mid()); + EXPECT_EQ(second_mid, callee_second_transceiver->mid()); + + // Setting the remote offer will dissociate the previous transceiver and + // create a new transceiver for the media section. + ASSERT_TRUE(caller->SetRemoteDescription(std::move(offer))); + auto caller_transceivers = caller->pc()->GetTransceivers(); + ASSERT_EQ(2u, caller_transceivers.size()); + EXPECT_EQ(absl::nullopt, caller_transceivers[0]->mid()); + EXPECT_EQ(first_type_, caller_transceivers[0]->media_type()); + EXPECT_EQ(second_mid, caller_transceivers[1]->mid()); + EXPECT_EQ(second_type_, caller_transceivers[1]->media_type()); + + // The answer should have only one media section for the new transceiver. + auto answer = caller->CreateAnswer(); + auto answer_contents = answer->description()->contents(); + ASSERT_EQ(1u, answer_contents.size()); + EXPECT_FALSE(answer_contents[0].rejected); + EXPECT_EQ(second_mid, answer_contents[0].name); + EXPECT_EQ(second_type_, answer_contents[0].media_description()->type()); + + // Setting the local answer should succeed. + ASSERT_TRUE( + caller->SetLocalDescription(CloneSessionDescription(answer.get()))); + + // Setting the remote answer should succeed and not create any new + // transceivers. + ASSERT_TRUE(callee->SetRemoteDescription(std::move(answer))); + ASSERT_EQ(2u, callee->pc()->GetTransceivers().size()); + ASSERT_EQ(2u, caller->pc()->GetTransceivers().size()); +} + +// Test that a m= section is *not* recycled if the media section is only +// rejected in the pending local description and there is no current remote +// description. +TEST_P(RecycleMediaSectionTest, PendingLocalRejectedAndNoRemote) { + auto caller = CreatePeerConnection(); + auto caller_first_transceiver = caller->AddTransceiver(first_type_); + + ASSERT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); + + std::string first_mid = *caller_first_transceiver->mid(); + caller_first_transceiver->Stop(); + + // The reoffer will have a rejected m= section. + ASSERT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); + + auto caller_second_transceiver = caller->AddTransceiver(second_type_); + + // The reoffer should not recycle the existing m= section since it is not + // rejected in either the *current* local or *current* remote description. + auto reoffer = caller->CreateOffer(); + auto reoffer_contents = reoffer->description()->contents(); + ASSERT_EQ(2u, reoffer_contents.size()); + EXPECT_TRUE(reoffer_contents[0].rejected); + EXPECT_EQ(first_type_, reoffer_contents[0].media_description()->type()); + EXPECT_EQ(first_mid, reoffer_contents[0].name); + EXPECT_FALSE(reoffer_contents[1].rejected); + EXPECT_EQ(second_type_, reoffer_contents[1].media_description()->type()); + std::string second_mid = reoffer_contents[1].name; + EXPECT_NE(first_mid, second_mid); + + ASSERT_TRUE(caller->SetLocalDescription(std::move(reoffer))); + + // Both RtpTransceivers are associated. + EXPECT_EQ(first_mid, caller_first_transceiver->mid()); + EXPECT_EQ(second_mid, caller_second_transceiver->mid()); +} + +// Test that a m= section is *not* recycled if the media section is only +// rejected in the pending local description and not rejected in the current +// remote description. +TEST_P(RecycleMediaSectionTest, PendingLocalRejectedAndNotRejectedRemote) { + auto caller = CreatePeerConnection(); + auto caller_first_transceiver = caller->AddTransceiver(first_type_); + auto callee = CreatePeerConnection(); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + + std::string first_mid = *caller_first_transceiver->mid(); + caller_first_transceiver->Stop(); + + // The reoffer will have a rejected m= section. + ASSERT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); + + auto caller_second_transceiver = caller->AddTransceiver(second_type_); + + // The reoffer should not recycle the existing m= section since it is not + // rejected in either the *current* local or *current* remote description. + auto reoffer = caller->CreateOffer(); + auto reoffer_contents = reoffer->description()->contents(); + ASSERT_EQ(2u, reoffer_contents.size()); + EXPECT_TRUE(reoffer_contents[0].rejected); + EXPECT_EQ(first_type_, reoffer_contents[0].media_description()->type()); + EXPECT_EQ(first_mid, reoffer_contents[0].name); + EXPECT_FALSE(reoffer_contents[1].rejected); + EXPECT_EQ(second_type_, reoffer_contents[1].media_description()->type()); + std::string second_mid = reoffer_contents[1].name; + EXPECT_NE(first_mid, second_mid); + + ASSERT_TRUE(caller->SetLocalDescription(std::move(reoffer))); + + // Both RtpTransceivers are associated. + EXPECT_EQ(first_mid, caller_first_transceiver->mid()); + EXPECT_EQ(second_mid, caller_second_transceiver->mid()); +} + +// Test that an m= section is *not* recycled if the media section is only +// rejected in the pending remote description and there is no current local +// description. +TEST_P(RecycleMediaSectionTest, PendingRemoteRejectedAndNoLocal) { + auto caller = CreatePeerConnection(); + auto caller_first_transceiver = caller->AddTransceiver(first_type_); + auto callee = CreatePeerConnection(); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); + auto callee_first_transceiver = callee->pc()->GetTransceivers()[0]; + std::string first_mid = *callee_first_transceiver->mid(); + caller_first_transceiver->Stop(); + + // The reoffer will have a rejected m= section. + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + auto callee_second_transceiver = callee->AddTransceiver(second_type_); + + // The reoffer should not recycle the existing m= section since it is not + // rejected in either the *current* local or *current* remote description. + auto reoffer = callee->CreateOffer(); + auto reoffer_contents = reoffer->description()->contents(); + ASSERT_EQ(2u, reoffer_contents.size()); + EXPECT_TRUE(reoffer_contents[0].rejected); + EXPECT_EQ(first_type_, reoffer_contents[0].media_description()->type()); + EXPECT_EQ(first_mid, reoffer_contents[0].name); + EXPECT_FALSE(reoffer_contents[1].rejected); + EXPECT_EQ(second_type_, reoffer_contents[1].media_description()->type()); + std::string second_mid = reoffer_contents[1].name; + EXPECT_NE(first_mid, second_mid); + + // Note: Cannot actually set the reoffer since the callee is in the signaling + // state 'have-remote-offer'. +} + +// Test that an m= section is *not* recycled if the media section is only +// rejected in the pending remote description and not rejected in the current +// local description. +TEST_P(RecycleMediaSectionTest, PendingRemoteRejectedAndNotRejectedLocal) { + auto caller = CreatePeerConnection(); + auto caller_first_transceiver = caller->AddTransceiver(first_type_); + auto callee = CreatePeerConnection(); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + + ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); + auto callee_first_transceiver = callee->pc()->GetTransceivers()[0]; + std::string first_mid = *callee_first_transceiver->mid(); + caller_first_transceiver->Stop(); + + // The reoffer will have a rejected m= section. + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + auto callee_second_transceiver = callee->AddTransceiver(second_type_); + + // The reoffer should not recycle the existing m= section since it is not + // rejected in either the *current* local or *current* remote description. + auto reoffer = callee->CreateOffer(); + auto reoffer_contents = reoffer->description()->contents(); + ASSERT_EQ(2u, reoffer_contents.size()); + EXPECT_TRUE(reoffer_contents[0].rejected); + EXPECT_EQ(first_type_, reoffer_contents[0].media_description()->type()); + EXPECT_EQ(first_mid, reoffer_contents[0].name); + EXPECT_FALSE(reoffer_contents[1].rejected); + EXPECT_EQ(second_type_, reoffer_contents[1].media_description()->type()); + std::string second_mid = reoffer_contents[1].name; + EXPECT_NE(first_mid, second_mid); + + // Note: Cannot actually set the reoffer since the callee is in the signaling + // state 'have-remote-offer'. +} + // Test all combinations of audio and video as the first and second media type // for the media section. This is needed for full test coverage because // MediaSession has separate functions for processing audio and video media