admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fixing some issues with payload type mappings.

Browse Source 
This fixes a couple major issues.

#1: If the payload type that an RTX codec refers to has been reassigned, and then the RTX codec is added in a subsequent offer, it refers to the wrong payload type.

#2: If we receive an offer with two payload types referring to the same codec (which we support), our answer contains both (instead of just one), which causes issues down the road since the video engine only supports one payload type per codec.

BUG=webrtc:5450,webrtc:5499
R=pthatcher@webrtc.org

Review URL: https://codereview.webrtc.org/1616033002 .

Cr-Commit-Position: refs/heads/master@{#11880}
This commit is contained in:
Taylor Brandstetter 2016-03-04 16:47:56 -08:00
parent e26e78784b
commit 6ec641b0ee
2 changed files with 172 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

174
webrtc/pc/mediasession.cc
View File

@ -10,6 +10,7 @@
#include "webrtc/pc/mediasession.h"
#include <algorithm> // For std::find_if.
#include <functional>
#include <map>
#include <set>
@ -810,57 +811,57 @@ template <class C>
static void NegotiateCodecs(const std::vector<C>& local_codecs,
const std::vector<C>& offered_codecs,
std::vector<C>* negotiated_codecs) {
typename std::vector<C>::const_iterator ours;
for (ours = local_codecs.begin();
ours != local_codecs.end(); ++ours) {
typename std::vector<C>::const_iterator theirs;
for (theirs = offered_codecs.begin();
theirs != offered_codecs.end(); ++theirs) {
if (ours->Matches(*theirs)) {
C negotiated = *ours;
negotiated.IntersectFeedbackParams(*theirs);
if (IsRtxCodec(negotiated)) {
std::string offered_apt_value;
std::string local_apt_value;
if (!ours->GetParam(kCodecParamAssociatedPayloadType,
&local_apt_value) ||
!theirs->GetParam(kCodecParamAssociatedPayloadType,
&offered_apt_value)) {
LOG(LS_WARNING) << "RTX missing associated payload type.";
continue;
}
// Only negotiate RTX if kCodecParamAssociatedPayloadType has been
// set in local and remote codecs, and they match.
if (!ReferencedCodecsMatch(local_codecs, local_apt_value,
offered_codecs, offered_apt_value)) {
LOG(LS_WARNING) << "RTX associated codecs don't match.";
continue;
}
negotiated.SetParam(kCodecParamAssociatedPayloadType,
offered_apt_value);
}
negotiated.id = theirs->id;
// RFC3264: Although the answerer MAY list the formats in their desired
// order of preference, it is RECOMMENDED that unless there is a
// specific reason, the answerer list formats in the same relative order
// they were present in the offer.
negotiated.preference = theirs->preference;
negotiated_codecs->push_back(negotiated);
for (const C& ours : local_codecs) {
C theirs;
if (FindMatchingCodec(local_codecs, offered_codecs, ours, &theirs)) {
C negotiated = ours;
negotiated.IntersectFeedbackParams(theirs);
if (IsRtxCodec(negotiated)) {
std::string offered_apt_value;
theirs.GetParam(kCodecParamAssociatedPayloadType, &offered_apt_value);
// FindMatchingCodec shouldn't return something with no apt value.
RTC_DCHECK(!offered_apt_value.empty());
negotiated.SetParam(kCodecParamAssociatedPayloadType,
offered_apt_value);
}
negotiated.id = theirs.id;
// RFC3264: Although the answerer MAY list the formats in their desired
// order of preference, it is RECOMMENDED that unless there is a
// specific reason, the answerer list formats in the same relative order
// they were present in the offer.
negotiated.preference = theirs.preference;
negotiated_codecs->push_back(negotiated);
}
}
}
// Finds a codec in |codecs2| that matches |codec_to_match|, which is
// a member of |codecs1|. If |codec_to_match| is an RTX codec, both
// the codecs themselves and their associated codecs must match.
template <class C>
static bool FindMatchingCodec(const std::vector<C>& codecs,
static bool FindMatchingCodec(const std::vector<C>& codecs1,
const std::vector<C>& codecs2,
const C& codec_to_match,
C* found_codec) {
for (typename std::vector<C>::const_iterator it = codecs.begin();
it != codecs.end(); ++it) {
if (it->Matches(codec_to_match)) {
if (found_codec != NULL) {
*found_codec= *it;
for (const C& potential_match : codecs2) {
if (potential_match.Matches(codec_to_match)) {
if (IsRtxCodec(codec_to_match)) {
std::string apt_value_1;
std::string apt_value_2;
if (!codec_to_match.GetParam(kCodecParamAssociatedPayloadType,
&apt_value_1) ||
!potential_match.GetParam(kCodecParamAssociatedPayloadType,
&apt_value_2)) {
LOG(LS_WARNING) << "RTX missing associated payload type.";
continue;
}
if (!ReferencedCodecsMatch(codecs1, apt_value_1, codecs2,
apt_value_2)) {
continue;
}
}
if (found_codec) {
*found_codec = potential_match;
}
return true;
}
@ -877,49 +878,64 @@ static void FindCodecsToOffer(
std::vector<C>* offered_codecs,
UsedPayloadTypes* used_pltypes) {
typedef std::map<int, C> RtxCodecReferences;
RtxCodecReferences new_rtx_codecs;
// Find all new RTX codecs.
for (typename std::vector<C>::const_iterator it = reference_codecs.begin();
it != reference_codecs.end(); ++it) {
if (!FindMatchingCodec<C>(*offered_codecs, *it, NULL) && IsRtxCodec(*it)) {
C rtx_codec = *it;
int referenced_pl_type =
rtc::FromString<int>(0,
rtx_codec.params[kCodecParamAssociatedPayloadType]);
new_rtx_codecs.insert(std::pair<int, C>(referenced_pl_type,
rtx_codec));
}
}
// Add all new codecs that are not RTX codecs.
for (typename std::vector<C>::const_iterator it = reference_codecs.begin();
it != reference_codecs.end(); ++it) {
if (!FindMatchingCodec<C>(*offered_codecs, *it, NULL) && !IsRtxCodec(*it)) {
C codec = *it;
int original_payload_id = codec.id;
for (const C& reference_codec : reference_codecs) {
if (!IsRtxCodec(reference_codec) &&
!FindMatchingCodec<C>(reference_codecs, *offered_codecs,
reference_codec, nullptr)) {
C codec = reference_codec;
used_pltypes->FindAndSetIdUsed(&codec);
offered_codecs->push_back(codec);
// If this codec is referenced by a new RTX codec, update the reference
// in the RTX codec with the new payload type.
typename RtxCodecReferences::iterator rtx_it =
new_rtx_codecs.find(original_payload_id);
if (rtx_it != new_rtx_codecs.end()) {
C& rtx_codec = rtx_it->second;
rtx_codec.params[kCodecParamAssociatedPayloadType] =
rtc::ToString(codec.id);
}
}
}
// Add all new RTX codecs.
for (typename RtxCodecReferences::iterator it = new_rtx_codecs.begin();
it != new_rtx_codecs.end(); ++it) {
C& rtx_codec = it->second;
used_pltypes->FindAndSetIdUsed(&rtx_codec);
offered_codecs->push_back(rtx_codec);
for (const C& reference_codec : reference_codecs) {
if (IsRtxCodec(reference_codec) &&
!FindMatchingCodec<C>(reference_codecs, *offered_codecs,
reference_codec, nullptr)) {
C rtx_codec = reference_codec;
std::string associated_pt_str;
if (!rtx_codec.GetParam(kCodecParamAssociatedPayloadType,
&associated_pt_str)) {
LOG(LS_WARNING) << "RTX codec " << rtx_codec.name
<< " is missing an associated payload type.";
continue;
}
int associated_pt;
if (!rtc::FromString(associated_pt_str, &associated_pt)) {
LOG(LS_WARNING) << "Couldn't convert payload type " << associated_pt_str
<< " of RTX codec " << rtx_codec.name
<< " to an integer.";
continue;
}
// Find the associated reference codec for the reference RTX codec.
C associated_codec;
if (!FindCodecById(reference_codecs, associated_pt, &associated_codec)) {
LOG(LS_WARNING) << "Couldn't find associated codec with payload type "
<< associated_pt << " for RTX codec " << rtx_codec.name
<< ".";
continue;
}
// Find a codec in the offered list that matches the reference codec.
// Its payload type may be different than the reference codec.
C matching_codec;
if (!FindMatchingCodec<C>(reference_codecs, *offered_codecs,
associated_codec, &matching_codec)) {
LOG(LS_WARNING) << "Couldn't find matching " << associated_codec.name
<< " codec.";
continue;
}
rtx_codec.params[kCodecParamAssociatedPayloadType] =
rtc::ToString(matching_codec.id);
used_pltypes->FindAndSetIdUsed(&rtx_codec);
offered_codecs->push_back(rtx_codec);
}
}
}

77
webrtc/pc/mediasession_unittest.cc
View File

@ -1654,6 +1654,48 @@ TEST_F(MediaSessionDescriptionFactoryTest,
EXPECT_EQ(new_h264_pl_type, pt_referenced_by_rtx);
}
// Create an updated offer with RTX after creating an answer to an offer
// without RTX, and with different default payload types.
// Verify that the added RTX codec references the correct payload type.
TEST_F(MediaSessionDescriptionFactoryTest,
RespondentCreatesOfferWithRtxAfterCreatingAnswerWithoutRtx) {
MediaSessionOptions opts;
opts.recv_video = true;
opts.recv_audio = true;
std::vector<VideoCodec> f2_codecs = MAKE_VECTOR(kVideoCodecs2);
// This creates rtx for H264 with the payload type |f2_| uses.
AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs2[0].id), &f2_codecs);
f2_.set_video_codecs(f2_codecs);
rtc::scoped_ptr<SessionDescription> offer(f1_.CreateOffer(opts, nullptr));
ASSERT_TRUE(offer.get() != nullptr);
rtc::scoped_ptr<SessionDescription> answer(
f2_.CreateAnswer(offer.get(), opts, nullptr));
const VideoContentDescription* vcd =
GetFirstVideoContentDescription(answer.get());
std::vector<VideoCodec> expected_codecs = MAKE_VECTOR(kVideoCodecsAnswer);
EXPECT_EQ(expected_codecs, vcd->codecs());
// Now, ensure that the RTX codec is created correctly when |f2_| creates an
// updated offer, even though the default payload types are different from
// those of |f1_|.
rtc::scoped_ptr<SessionDescription> updated_offer(
f2_.CreateOffer(opts, answer.get()));
ASSERT_TRUE(updated_offer);
const VideoContentDescription* updated_vcd =
GetFirstVideoContentDescription(updated_offer.get());
// New offer should attempt to add H263, and RTX for H264.
expected_codecs.push_back(kVideoCodecs2[1]);
AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs1[1].id),
&expected_codecs);
EXPECT_EQ(expected_codecs, updated_vcd->codecs());
}
// Test that RTX is ignored when there is no associated payload type parameter.
TEST_F(MediaSessionDescriptionFactoryTest, RtxWithoutApt) {
MediaSessionOptions opts;
@ -1762,6 +1804,41 @@ TEST_F(MediaSessionDescriptionFactoryTest,
EXPECT_EQ(expected_codecs, vcd->codecs());
}
// Test that after one RTX codec has been negotiated, a new offer can attempt
// to add another.
TEST_F(MediaSessionDescriptionFactoryTest, AddSecondRtxInNewOffer) {
MediaSessionOptions opts;
opts.recv_video = true;
opts.recv_audio = false;
std::vector<VideoCodec> f1_codecs = MAKE_VECTOR(kVideoCodecs1);
// This creates RTX for H264 for the offerer.
AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), &f1_codecs);
f1_.set_video_codecs(f1_codecs);
rtc::scoped_ptr<SessionDescription> offer(f1_.CreateOffer(opts, nullptr));
ASSERT_TRUE(offer);
const VideoContentDescription* vcd =
GetFirstVideoContentDescription(offer.get());
std::vector<VideoCodec> expected_codecs = MAKE_VECTOR(kVideoCodecs1);
AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id),
&expected_codecs);
EXPECT_EQ(expected_codecs, vcd->codecs());
// Now, attempt to add RTX for H264-SVC.
AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs1[0].id), &f1_codecs);
f1_.set_video_codecs(f1_codecs);
rtc::scoped_ptr<SessionDescription> updated_offer(
f1_.CreateOffer(opts, offer.get()));
ASSERT_TRUE(updated_offer);
vcd = GetFirstVideoContentDescription(updated_offer.get());
AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs1[0].id),
&expected_codecs);
EXPECT_EQ(expected_codecs, vcd->codecs());
}
// Test that when RTX is used in conjunction with simulcast, an RTX ssrc is
// generated for each simulcast ssrc and correctly grouped.
TEST_F(MediaSessionDescriptionFactoryTest, SimSsrcsGenerateMultipleRtxSsrcs) {