[Unified Plan] Fix issues with recycling m= sections
Previously, the PeerConnection would look at the pending local
and remote descriptions also to determine if an m= section is
recycled. That is not quite spec compliant and breaks down under
some edge cases. This changes the PeerConnection to look only at
the *current* local or remote description (i.e., the descriptions
from the last time the PeerConnection was in a stable signaling
state) to determine if an m= section is recycled.
Additionally, the MediaSessionFactory only looked at the local
description to determine if an m= section is recycled. The full
criteria requires looking at the current local and current remote
m= sections. This change adds a state enum to the
MediaDescriptionOptions so that the MediaSessionFactory knows if
a media section is being recycled without duplicating the logic
in PeerConnection.
Tests are added to cover additional edge cases.
Bug: chromium:899680
Change-Id: I5bcf0f88957a61653269ed8bb50b2018500bc1d5
Reviewed-on: https://webrtc-review.googlesource.com/c/111293
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Seth Hampson <shampson@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25959}diff --git a/pc/mediasession.cc b/pc/mediasession.cc
index 7239af8..263f369 100644
--- a/pc/mediasession.cc
+++ b/pc/mediasession.cc
@@ -18,6 +18,7 @@
#include <unordered_map>
#include <utility>
+#include "absl/memory/memory.h"
#include "absl/strings/match.h"
#include "absl/types/optional.h"
#include "api/cryptoparams.h"
@@ -289,20 +290,15 @@
}
// 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<const ContentInfo*>& 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 @@
return true;
}
+static std::vector<const ContentInfo*> GetActiveContents(
+ const SessionDescription& description,
+ const MediaSessionOptions& session_options) {
+ std::vector<const ContentInfo*> 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 <class C>
static bool ContainsRtxCodec(const std::vector<C>& codecs) {
for (const auto& codec : codecs) {
@@ -1265,17 +1278,28 @@
SessionDescription* MediaSessionDescriptionFactory::CreateOffer(
const MediaSessionOptions& session_options,
const SessionDescription* current_description) const {
- std::unique_ptr<SessionDescription> 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<const ContentInfo*> 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 @@
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<SessionDescription>();
// Iterate through the media description options, matching with existing media
// descriptions in |current_description|.
@@ -1306,7 +1326,7 @@
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 @@
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<SessionDescription> answer(new SessionDescription());
+ std::vector<const ContentInfo*> 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<TransportInfo> 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 @@
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 @@
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<SessionDescription>();
+
+ // 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<TransportInfo> 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 @@
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<const ContentInfo*>& 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<AudioCodec>(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<VideoCodec>(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<DataCodec>(data->codecs(), data_codecs, used_pltypes);
}
}
@@ -1619,24 +1643,18 @@
// 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<const ContentInfo*>& 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<AudioCodec>(all_audio_codecs_, audio_codecs, &used_pltypes);
MergeCodecs<VideoCodec>(video_codecs_, video_codecs, &used_pltypes);
MergeCodecs<DataCodec>(data_codecs_, data_codecs, &used_pltypes);
@@ -1650,29 +1668,23 @@
// 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<const ContentInfo*>& 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 @@
}
}
- // 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<AudioCodec>(filtered_offered_audio_codecs, audio_codecs,
&used_pltypes);
@@ -1722,9 +1734,10 @@
&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<const ContentInfo*>& 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 @@
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 @@
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 @@
&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 @@
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 @@
}
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 5904605..9495bdc 100644
--- a/pc/mediasession.h
+++ b/pc/mediasession.h
@@ -173,19 +173,22 @@
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<const ContentInfo*>& current_active_contents,
+ AudioCodecs* audio_codecs,
+ VideoCodecs* video_codecs,
+ DataCodecs* data_codecs) const;
+ void GetCodecsForAnswer(
+ const std::vector<const ContentInfo*>& current_active_contents,
+ const SessionDescription& remote_offer,
+ AudioCodecs* audio_codecs,
+ VideoCodecs* video_codecs,
+ DataCodecs* data_codecs) const;
+ void GetRtpHdrExtsToOffer(
+ const std::vector<const ContentInfo*>& 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 076ad12..2098ec0 100644
--- a/pc/mediasession_unittest.cc
+++ b/pc/mediasession_unittest.cc
@@ -2020,6 +2020,110 @@
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 e166d1e..c341888 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -3947,6 +3947,17 @@
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 @@
// 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 @@
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 @@
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 @@
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 fe878b0..f0c9111 100644
--- a/pc/peerconnection_jsep_unittest.cc
+++ b/pc/peerconnection_jsep_unittest.cc
@@ -700,6 +700,12 @@
// 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 @@
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 @@
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
