generic-library/webrtc
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Check associated payload type when negotiate RTX codecs.

Browse Source 
At the moment, only payload name is checked when match two RTX codecs.
This will cause wrong behavior of codec negotiation if multiple RTX codecs
are added.

BUG=
R=pthatcher@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/34189004

Cr-Commit-Position: refs/heads/master@{#8727}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8727 4adac7df-926f-26a2-2b94-8c16560cd09d
This commit is contained in:
changbin.shao@webrtc.org 2015-03-16 04:14:34 +00:00
parent eb44fd6e81
commit 2d25b44f47
6 changed files with 196 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
talk/app/webrtc/webrtcsdp.cc
View File

@ -2380,18 +2380,14 @@ void AddFeedbackParameters(const cricket::FeedbackParams& feedback_params,
}
// Gets the current codec setting associated with |payload_type|. If there
// is no AudioCodec associated with that payload type it returns an empty codec
// is no Codec associated with that payload type it returns an empty codec
// with that payload type.
template <class T>
T GetCodec(const std::vector<T>& codecs, int payload_type) {
for (typename std::vector<T>::const_iterator codec = codecs.begin();
codec != codecs.end(); ++codec) {
if (codec->id == payload_type) {
return *codec;
}
T GetCodecWithPayloadType(const std::vector<T>& codecs, int payload_type) {
T ret_val;
if (!FindCodecById(codecs, payload_type, &ret_val)) {
ret_val.id = payload_type;
}
T ret_val = T();
ret_val.id = payload_type;
return ret_val;
}
@ -2423,7 +2419,8 @@ template <class T, class U>
void UpdateCodec(MediaContentDescription* content_desc, int payload_type,
const cricket::CodecParameterMap& parameters) {
// Codec might already have been populated (from rtpmap).
U new_codec = GetCodec(static_cast<T*>(content_desc)->codecs(), payload_type);
U new_codec = GetCodecWithPayloadType(static_cast<T*>(content_desc)->codecs(),
payload_type);
AddParameters(parameters, &new_codec);
AddOrReplaceCodec<T, U>(content_desc, new_codec);
}
@ -2434,7 +2431,8 @@ template <class T, class U>
void UpdateCodec(MediaContentDescription* content_desc, int payload_type,
const cricket::FeedbackParam& feedback_param) {
// Codec might already have been populated (from rtpmap).
U new_codec = GetCodec(static_cast<T*>(content_desc)->codecs(), payload_type);
U new_codec = GetCodecWithPayloadType(static_cast<T*>(content_desc)->codecs(),
payload_type);
AddFeedbackParameter(feedback_param, &new_codec);
AddOrReplaceCodec<T, U>(content_desc, new_codec);
}
@ -2889,7 +2887,8 @@ void UpdateCodec(int payload_type, const std::string& name, int clockrate,
AudioContentDescription* audio_desc) {
// Codec may already be populated with (only) optional parameters
// (from an fmtp).
cricket::AudioCodec codec = GetCodec(audio_desc->codecs(), payload_type);
cricket::AudioCodec codec =
GetCodecWithPayloadType(audio_desc->codecs(), payload_type);
codec.name = name;
codec.clockrate = clockrate;
codec.bitrate = bitrate;
@ -2906,7 +2905,8 @@ void UpdateCodec(int payload_type, const std::string& name, int width,
VideoContentDescription* video_desc) {
// Codec may already be populated with (only) optional parameters
// (from an fmtp).
cricket::VideoCodec codec = GetCodec(video_desc->codecs(), payload_type);
cricket::VideoCodec codec =
GetCodecWithPayloadType(video_desc->codecs(), payload_type);
codec.name = name;
codec.width = width;
codec.height = height;

42
talk/app/webrtc/webrtcsession_unittest.cc
View File

@ -136,6 +136,26 @@ static const char kTooLongIceUfragPwd[] =
"IceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfrag"
"IceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfragIceUfrag";
static const char kSdpWithRtx[] =
"v=0\r\n"
"o=- 4104004319237231850 2 IN IP4 127.0.0.1\r\n"
"s=-\r\n"
"t=0 0\r\n"
"a=msid-semantic: WMS stream1\r\n"
"m=video 9 RTP/SAVPF 0 96\r\n"
"c=IN IP4 0.0.0.0\r\n"
"a=rtcp:9 IN IP4 0.0.0.0\r\n"
"a=ice-ufrag:CerjGp19G7wpXwl7\r\n"
"a=ice-pwd:cMvOlFvQ6ochez1ZOoC2uBEC\r\n"
"a=mid:video\r\n"
"a=sendrecv\r\n"
"a=rtcp-mux\r\n"
"a=crypto:1 AES_CM_128_HMAC_SHA1_80 "
"inline:5/4N5CDvMiyDArHtBByUM71VIkguH17ZNoX60GrA\r\n"
"a=rtpmap:0 fake_video_codec/90000\r\n"
"a=rtpmap:96 rtx/90000\r\n"
"a=fmtp:96 apt=0\r\n";
// Add some extra |newlines| to the |message| after |line|.
static void InjectAfter(const std::string& line,
const std::string& newlines,
@ -3546,6 +3566,28 @@ TEST_F(WebRtcSessionTest, TestRenegotiateNewMediaWithCandidatesSeparated) {
answer = CreateAnswer(NULL);
SetLocalDescriptionWithoutError(answer);
}
// Tests that RTX codec is removed from the answer when it isn't supported
// by local side.
TEST_F(WebRtcSessionTest, TestRtxRemovedByCreateAnswer) {
Init();
mediastream_signaling_.SendAudioVideoStream1();
std::string offer_sdp(kSdpWithRtx);
SessionDescriptionInterface* offer =
CreateSessionDescription(JsepSessionDescription::kOffer, offer_sdp, NULL);
EXPECT_TRUE(offer->ToString(&offer_sdp));
// Offer SDP contains the RTX codec.
EXPECT_TRUE(offer_sdp.find("rtx") != std::string::npos);
SetRemoteDescriptionWithoutError(offer);
SessionDescriptionInterface* answer = CreateAnswer(NULL);
std::string answer_sdp;
answer->ToString(&answer_sdp);
// Answer SDP removes the unsupported RTX codec.
EXPECT_TRUE(answer_sdp.find("rtx") == std::string::npos);
SetLocalDescriptionWithoutError(answer);
}
// This verifies that the voice channel after bundle has both options from video
// and voice channels.

15
talk/media/base/codec.h
View File

@ -252,6 +252,21 @@ struct VideoEncoderConfig {
int cpu_profile;
};
// Get the codec setting associated with |payload_type|. If there
// is no codec associated with that payload type it returns false.
template <class Codec>
bool FindCodecById(const std::vector<Codec>& codecs,
int payload_type,
Codec* codec_out) {
for (const auto& codec : codecs) {
if (codec.id == payload_type) {
*codec_out = codec;
return true;
}
}
return false;
}
} // namespace cricket
#endif // TALK_MEDIA_BASE_CODEC_H_

14
talk/media/base/rtpdataengine.cc
View File

@ -66,20 +66,6 @@ DataMediaChannel* RtpDataEngine::CreateChannel(
return new RtpDataMediaChannel(timing_.get());
}
// TODO(pthatcher): Should we move these find/get functions somewhere
// common?
bool FindCodecById(const std::vector<DataCodec>& codecs,
int id, DataCodec* codec_out) {
std::vector<DataCodec>::const_iterator iter;
for (iter = codecs.begin(); iter != codecs.end(); ++iter) {
if (iter->id == id) {
*codec_out = *iter;
return true;
}
}
return false;
}
bool FindCodecByName(const std::vector<DataCodec>& codecs,
const std::string& name, DataCodec* codec_out) {
std::vector<DataCodec>::const_iterator iter;

39
talk/session/media/mediasession.cc
View File

@ -751,6 +751,24 @@ static bool CreateMediaContentOffer(
return true;
}
template <class C>
static bool ReferencedCodecsMatch(const std::vector<C>& codecs1,
const std::string& codec1_id_str,
const std::vector<C>& codecs2,
const std::string& codec2_id_str) {
int codec1_id;
int codec2_id;
C codec1;
C codec2;
if (!rtc::FromString(codec1_id_str, &codec1_id) ||
!rtc::FromString(codec2_id_str, &codec2_id) ||
!FindCodecById(codecs1, codec1_id, &codec1) ||
!FindCodecById(codecs2, codec2_id, &codec2)) {
return false;
}
return codec1.Matches(codec2);
}
template <class C>
static void NegotiateCodecs(const std::vector<C>& local_codecs,
const std::vector<C>& offered_codecs,
@ -765,15 +783,26 @@ static void NegotiateCodecs(const std::vector<C>& local_codecs,
C negotiated = *ours;
negotiated.IntersectFeedbackParams(*theirs);
if (IsRtxCodec(negotiated)) {
// Only negotiate RTX if kCodecParamAssociatedPayloadType has been
// set.
std::string apt_value;
if (!theirs->GetParam(kCodecParamAssociatedPayloadType, &apt_value)) {
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;
}
negotiated.SetParam(kCodecParamAssociatedPayloadType, apt_value);
// 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

137
talk/session/media/mediasession_unittest.cc
View File

@ -197,6 +197,22 @@ GetMediaDirection(const ContentInfo* content) {
return desc->direction();
}
static void AddRtxCodec(const VideoCodec& rtx_codec,
std::vector<VideoCodec>* codecs) {
VideoCodec rtx;
ASSERT_FALSE(cricket::FindCodecById(*codecs, rtx_codec.id, &rtx));
codecs->push_back(rtx_codec);
}
template <class T>
static std::vector<std::string> GetCodecNames(const std::vector<T>& codecs) {
std::vector<std::string> codec_names;
for (const auto& codec : codecs) {
codec_names.push_back(codec.name);
}
return codec_names;
}
class MediaSessionDescriptionFactoryTest : public testing::Test {
public:
MediaSessionDescriptionFactoryTest()
@ -1545,25 +1561,13 @@ TEST_F(MediaSessionDescriptionFactoryTest,
opts.recv_video = true;
opts.recv_audio = false;
std::vector<VideoCodec> f1_codecs = MAKE_VECTOR(kVideoCodecs1);
VideoCodec rtx_f1;
rtx_f1.id = 126;
rtx_f1.name = cricket::kRtxCodecName;
// This creates rtx for H264 with the payload type |f1_| uses.
rtx_f1.params[cricket::kCodecParamAssociatedPayloadType] =
rtc::ToString<int>(kVideoCodecs1[1].id);
f1_codecs.push_back(rtx_f1);
AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), &f1_codecs);
f1_.set_video_codecs(f1_codecs);
std::vector<VideoCodec> f2_codecs = MAKE_VECTOR(kVideoCodecs2);
VideoCodec rtx_f2;
rtx_f2.id = 127;
rtx_f2.name = cricket::kRtxCodecName;
// This creates rtx for H264 with the payload type |f2_| uses.
rtx_f2.params[cricket::kCodecParamAssociatedPayloadType] =
rtc::ToString<int>(kVideoCodecs2[0].id);
f2_codecs.push_back(rtx_f2);
AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs2[0].id), &f2_codecs);
f2_.set_video_codecs(f2_codecs);
rtc::scoped_ptr<SessionDescription> offer(f1_.CreateOffer(opts, NULL));
@ -1575,7 +1579,8 @@ TEST_F(MediaSessionDescriptionFactoryTest,
GetFirstVideoContentDescription(answer.get());
std::vector<VideoCodec> expected_codecs = MAKE_VECTOR(kVideoCodecsAnswer);
expected_codecs.push_back(rtx_f1);
AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id),
&expected_codecs);
EXPECT_EQ(expected_codecs, vcd->codecs());
@ -1603,14 +1608,8 @@ TEST_F(MediaSessionDescriptionFactoryTest,
TEST_F(MediaSessionDescriptionFactoryTest,
RespondentCreatesOfferWithVideoAndRtxAfterCreatingAudioAnswer) {
std::vector<VideoCodec> f1_codecs = MAKE_VECTOR(kVideoCodecs1);
VideoCodec rtx_f1;
rtx_f1.id = 126;
rtx_f1.name = cricket::kRtxCodecName;
// This creates rtx for H264 with the payload type |f1_| uses.
rtx_f1.params[cricket::kCodecParamAssociatedPayloadType] =
rtc::ToString<int>(kVideoCodecs1[1].id);
f1_codecs.push_back(rtx_f1);
AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), &f1_codecs);
f1_.set_video_codecs(f1_codecs);
MediaSessionOptions opts;
@ -1634,12 +1633,7 @@ TEST_F(MediaSessionDescriptionFactoryTest,
std::vector<VideoCodec> f2_codecs = MAKE_VECTOR(kVideoCodecs2);
int used_pl_type = acd->codecs()[0].id;
f2_codecs[0].id = used_pl_type; // Set the payload type for H264.
VideoCodec rtx_f2;
rtx_f2.id = 127;
rtx_f2.name = cricket::kRtxCodecName;
rtx_f2.params[cricket::kCodecParamAssociatedPayloadType] =
rtc::ToString<int>(used_pl_type);
f2_codecs.push_back(rtx_f2);
AddRtxCodec(VideoCodec::CreateRtxCodec(125, used_pl_type), &f2_codecs);
f2_.set_video_codecs(f2_codecs);
rtc::scoped_ptr<SessionDescription> updated_offer(
@ -1671,22 +1665,13 @@ TEST_F(MediaSessionDescriptionFactoryTest, RtxWithoutApt) {
opts.recv_video = true;
opts.recv_audio = false;
std::vector<VideoCodec> f1_codecs = MAKE_VECTOR(kVideoCodecs1);
VideoCodec rtx_f1;
rtx_f1.id = 126;
rtx_f1.name = cricket::kRtxCodecName;
f1_codecs.push_back(rtx_f1);
// This creates RTX without associated payload type parameter.
AddRtxCodec(VideoCodec(126, cricket::kRtxCodecName, 0, 0, 0, 0), &f1_codecs);
f1_.set_video_codecs(f1_codecs);
std::vector<VideoCodec> f2_codecs = MAKE_VECTOR(kVideoCodecs2);
VideoCodec rtx_f2;
rtx_f2.id = 127;
rtx_f2.name = cricket::kRtxCodecName;
// This creates rtx for H264 with the payload type |f2_| uses.
rtx_f2.SetParam(cricket::kCodecParamAssociatedPayloadType,
rtc::ToString<int>(kVideoCodecs2[0].id));
f2_codecs.push_back(rtx_f2);
// 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, NULL));
@ -1711,13 +1696,75 @@ TEST_F(MediaSessionDescriptionFactoryTest, RtxWithoutApt) {
rtc::scoped_ptr<SessionDescription> answer(
f2_.CreateAnswer(offer.get(), opts, NULL));
std::vector<std::string> codec_names =
GetCodecNames(GetFirstVideoContentDescription(answer.get())->codecs());
EXPECT_EQ(codec_names.end(), std::find(codec_names.begin(), codec_names.end(),
cricket::kRtxCodecName));
}
// Test that RTX will be filtered out in the answer if its associated payload
// type doesn't match the local value.
TEST_F(MediaSessionDescriptionFactoryTest, FilterOutRtxIfAptDoesntMatch) {
MediaSessionOptions opts;
opts.recv_video = true;
opts.recv_audio = false;
std::vector<VideoCodec> f1_codecs = MAKE_VECTOR(kVideoCodecs1);
// This creates RTX for H264 in sender.
AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), &f1_codecs);
f1_.set_video_codecs(f1_codecs);
std::vector<VideoCodec> f2_codecs = MAKE_VECTOR(kVideoCodecs2);
// This creates RTX for H263 in receiver.
AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs2[1].id), &f2_codecs);
f2_.set_video_codecs(f2_codecs);
rtc::scoped_ptr<SessionDescription> offer(f1_.CreateOffer(opts, NULL));
ASSERT_TRUE(offer.get() != NULL);
// Associated payload type doesn't match, therefore, RTX codec is removed in
// the answer.
rtc::scoped_ptr<SessionDescription> answer(
f2_.CreateAnswer(offer.get(), opts, NULL));
std::vector<std::string> codec_names =
GetCodecNames(GetFirstVideoContentDescription(answer.get())->codecs());
EXPECT_EQ(codec_names.end(), std::find(codec_names.begin(), codec_names.end(),
cricket::kRtxCodecName));
}
// Test that when multiple RTX codecs are offered, only the matched RTX codec
// is added in the answer, and the unsupported RTX codec is filtered out.
TEST_F(MediaSessionDescriptionFactoryTest,
FilterOutUnsupportedRtxWhenCreatingAnswer) {
MediaSessionOptions opts;
opts.recv_video = true;
opts.recv_audio = false;
std::vector<VideoCodec> f1_codecs = MAKE_VECTOR(kVideoCodecs1);
// This creates RTX for H264-SVC in sender.
AddRtxCodec(VideoCodec::CreateRtxCodec(125, kVideoCodecs1[0].id), &f1_codecs);
f1_.set_video_codecs(f1_codecs);
// This creates RTX for H264 in sender.
AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id), &f1_codecs);
f1_.set_video_codecs(f1_codecs);
std::vector<VideoCodec> f2_codecs = MAKE_VECTOR(kVideoCodecs2);
// This creates RTX for H264 in receiver.
AddRtxCodec(VideoCodec::CreateRtxCodec(124, kVideoCodecs2[0].id), &f2_codecs);
f2_.set_video_codecs(f2_codecs);
// H264-SVC codec is removed in the answer, therefore, associated RTX codec
// for H264-SVC should also be removed.
rtc::scoped_ptr<SessionDescription> offer(f1_.CreateOffer(opts, NULL));
ASSERT_TRUE(offer.get() != NULL);
rtc::scoped_ptr<SessionDescription> answer(
f2_.CreateAnswer(offer.get(), opts, NULL));
const VideoContentDescription* vcd =
GetFirstVideoContentDescription(answer.get());
std::vector<VideoCodec> expected_codecs = MAKE_VECTOR(kVideoCodecsAnswer);
AddRtxCodec(VideoCodec::CreateRtxCodec(126, kVideoCodecs1[1].id),
&expected_codecs);
for (std::vector<VideoCodec>::const_iterator iter = vcd->codecs().begin();
iter != vcd->codecs().end(); ++iter) {
ASSERT_STRNE(iter->name.c_str(), cricket::kRtxCodecName);
}
EXPECT_EQ(expected_codecs, vcd->codecs());
}
// Create an updated offer after creating an answer to the original offer and