From ff1b1bf0944d42700edadae68bd774835a7a13f0 Mon Sep 17 00:00:00 2001 From: "wu@webrtc.org" Date: Fri, 20 Jun 2014 20:57:42 +0000 Subject: [PATCH] When creating an answer, takes the codec preference from the offer. This change is based on 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." BUG=2868 TEST=unit tests and manually with munge-sdp test R=juberti@google.com Review URL: https://webrtc-codereview.appspot.com/14589004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6514 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/media/base/codec.cc | 1 + talk/media/base/codec.h | 2 + talk/session/media/mediasession.cc | 5 + talk/session/media/mediasession_unittest.cc | 23 ++- talk/session/media/mediasessionclient.cc | 10 ++ .../media/mediasessionclient_unittest.cc | 157 +++++------------- 6 files changed, 72 insertions(+), 126 deletions(-) diff --git a/talk/media/base/codec.cc b/talk/media/base/codec.cc index bc7401a78f..c6f0ea5839 100644 --- a/talk/media/base/codec.cc +++ b/talk/media/base/codec.cc @@ -38,6 +38,7 @@ namespace cricket { static const int kMaxStaticPayloadId = 95; +const int kMaxPayloadId = 127; bool FeedbackParam::operator==(const FeedbackParam& other) const { return _stricmp(other.id().c_str(), id().c_str()) == 0 && diff --git a/talk/media/base/codec.h b/talk/media/base/codec.h index 863e289e8b..861962058f 100644 --- a/talk/media/base/codec.h +++ b/talk/media/base/codec.h @@ -39,6 +39,8 @@ namespace cricket { typedef std::map CodecParameterMap; +extern const int kMaxPayloadId; + class FeedbackParam { public: FeedbackParam(const std::string& id, const std::string& param) diff --git a/talk/session/media/mediasession.cc b/talk/session/media/mediasession.cc index 006975f931..a5b1eb0c6f 100644 --- a/talk/session/media/mediasession.cc +++ b/talk/session/media/mediasession.cc @@ -775,6 +775,11 @@ static void NegotiateCodecs(const std::vector& local_codecs, negotiated.SetParam(kCodecParamAssociatedPayloadType, 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); } } diff --git a/talk/session/media/mediasession_unittest.cc b/talk/session/media/mediasession_unittest.cc index cf492a65ee..b76cce48cf 100644 --- a/talk/session/media/mediasession_unittest.cc +++ b/talk/session/media/mediasession_unittest.cc @@ -31,6 +31,7 @@ #include "talk/base/gunit.h" #include "talk/base/fakesslidentity.h" #include "talk/base/messagedigest.h" +#include "talk/base/ssladapter.h" #include "talk/media/base/codec.h" #include "talk/media/base/testutils.h" #include "talk/p2p/base/constants.h" @@ -97,13 +98,13 @@ static const AudioCodec kAudioCodecs1[] = { static const AudioCodec kAudioCodecs2[] = { AudioCodec(126, "speex", 16000, 22000, 1, 3), - AudioCodec(127, "iLBC", 8000, 13300, 1, 2), - AudioCodec(0, "PCMU", 8000, 64000, 1, 1), + AudioCodec(0, "PCMU", 8000, 64000, 1, 2), + AudioCodec(127, "iLBC", 8000, 13300, 1, 1), }; static const AudioCodec kAudioCodecsAnswer[] = { - AudioCodec(102, "iLBC", 8000, 13300, 1, 2), - AudioCodec(0, "PCMU", 8000, 64000, 1, 1), + AudioCodec(102, "iLBC", 8000, 13300, 1, 5), + AudioCodec(0, "PCMU", 8000, 64000, 1, 4), }; static const VideoCodec kVideoCodecs1[] = { @@ -117,7 +118,7 @@ static const VideoCodec kVideoCodecs2[] = { }; static const VideoCodec kVideoCodecsAnswer[] = { - VideoCodec(97, "H264", 320, 200, 30, 2) + VideoCodec(97, "H264", 320, 200, 30, 1) }; static const DataCodec kDataCodecs1[] = { @@ -196,6 +197,14 @@ class MediaSessionDescriptionFactoryTest : public testing::Test { tdf2_.set_identity(&id2_); } + static void SetUpTestCase() { + talk_base::InitializeSSL(); + } + + static void TearDownTestCase() { + talk_base::CleanupSSL(); + } + // Create a video StreamParamsVec object with: // - one video stream with 3 simulcast streams and FEC, StreamParamsVec CreateComplexVideoStreamParamsVec() { @@ -1379,10 +1388,12 @@ TEST_F(MediaSessionDescriptionFactoryTest, // The expected audio codecs are the common audio codecs from the first // offer/answer exchange plus the audio codecs only |f2_| offer, sorted in // preference order. + // TODO(wu): |updated_offer| should not include the codec + // (i.e. |kAudioCodecs2[0]|) the other side doesn't support. const AudioCodec kUpdatedAudioCodecOffer[] = { - kAudioCodecs2[0], kAudioCodecsAnswer[0], kAudioCodecsAnswer[1], + kAudioCodecs2[0], }; // The expected video codecs are the common video codecs from the first diff --git a/talk/session/media/mediasessionclient.cc b/talk/session/media/mediasessionclient.cc index a5a652cc78..2ada987c46 100644 --- a/talk/session/media/mediasessionclient.cc +++ b/talk/session/media/mediasessionclient.cc @@ -373,6 +373,7 @@ bool ParseGingleAudioContent(const buzz::XmlElement* content_elem, ParseError* error) { AudioContentDescription* audio = new AudioContentDescription(); + int preference = kMaxPayloadId; if (content_elem->FirstElement()) { for (const buzz::XmlElement* codec_elem = content_elem->FirstNamed(QN_GINGLE_AUDIO_PAYLOADTYPE); @@ -380,6 +381,7 @@ bool ParseGingleAudioContent(const buzz::XmlElement* content_elem, codec_elem = codec_elem->NextNamed(QN_GINGLE_AUDIO_PAYLOADTYPE)) { AudioCodec codec; if (ParseGingleAudioCodec(codec_elem, &codec)) { + codec.preference = preference--; audio->AddCodec(codec); } } @@ -406,12 +408,14 @@ bool ParseGingleVideoContent(const buzz::XmlElement* content_elem, ParseError* error) { VideoContentDescription* video = new VideoContentDescription(); + int preference = kMaxPayloadId; for (const buzz::XmlElement* codec_elem = content_elem->FirstNamed(QN_GINGLE_VIDEO_PAYLOADTYPE); codec_elem != NULL; codec_elem = codec_elem->NextNamed(QN_GINGLE_VIDEO_PAYLOADTYPE)) { VideoCodec codec; if (ParseGingleVideoCodec(codec_elem, &codec)) { + codec.preference = preference--; video->AddCodec(codec); } } @@ -571,6 +575,7 @@ bool ParseJingleAudioContent(const buzz::XmlElement* content_elem, FeedbackParams content_feedback_params; ParseFeedbackParams(content_elem, &content_feedback_params); + int preference = kMaxPayloadId; for (const buzz::XmlElement* payload_elem = content_elem->FirstNamed(QN_JINGLE_RTP_PAYLOADTYPE); payload_elem != NULL; @@ -578,6 +583,7 @@ bool ParseJingleAudioContent(const buzz::XmlElement* content_elem, AudioCodec codec; if (ParseJingleAudioCodec(payload_elem, &codec)) { AddFeedbackParams(content_feedback_params, &codec.feedback_params); + codec.preference = preference--; audio->AddCodec(codec); } } @@ -611,6 +617,7 @@ bool ParseJingleVideoContent(const buzz::XmlElement* content_elem, FeedbackParams content_feedback_params; ParseFeedbackParams(content_elem, &content_feedback_params); + int preference = kMaxPayloadId; for (const buzz::XmlElement* payload_elem = content_elem->FirstNamed(QN_JINGLE_RTP_PAYLOADTYPE); payload_elem != NULL; @@ -618,6 +625,7 @@ bool ParseJingleVideoContent(const buzz::XmlElement* content_elem, VideoCodec codec; if (ParseJingleVideoCodec(payload_elem, &codec)) { AddFeedbackParams(content_feedback_params, &codec.feedback_params); + codec.preference = preference--; video->AddCodec(codec); } } @@ -681,6 +689,7 @@ bool ParseJingleRtpDataContent(const buzz::XmlElement* content_elem, FeedbackParams content_feedback_params; ParseFeedbackParams(content_elem, &content_feedback_params); + int preference = kMaxPayloadId; for (const buzz::XmlElement* payload_elem = content_elem->FirstNamed(QN_JINGLE_RTP_PAYLOADTYPE); payload_elem != NULL; @@ -688,6 +697,7 @@ bool ParseJingleRtpDataContent(const buzz::XmlElement* content_elem, DataCodec codec; if (ParseJingleDataCodec(payload_elem, &codec)) { AddFeedbackParams(content_feedback_params, &codec.feedback_params); + codec.preference = preference--; data->AddCodec(codec); } } diff --git a/talk/session/media/mediasessionclient_unittest.cc b/talk/session/media/mediasessionclient_unittest.cc index 7cb1ec97e3..3f3c4fa49c 100644 --- a/talk/session/media/mediasessionclient_unittest.cc +++ b/talk/session/media/mediasessionclient_unittest.cc @@ -32,6 +32,7 @@ #include "talk/base/logging.h" #include "talk/base/scoped_ptr.h" #include "talk/media/base/fakemediaengine.h" +#include "talk/media/base/testutils.h" #include "talk/media/devices/fakedevicemanager.h" #include "talk/p2p/base/constants.h" #include "talk/p2p/client/basicportallocator.h" @@ -74,6 +75,29 @@ static const cricket::AudioCodec kAudioCodecs[] = { cricket::AudioCodec(106, "telephone-event", 8000, 0, 1, 1) }; +// The codecs that our FakeMediaEngine will support with a different order of +// supported codecs. +static const cricket::AudioCodec kAudioCodecsDifferentPreference[] = { + cricket::AudioCodec(104, "ISAC", 32000, -1, 1, 17), + cricket::AudioCodec(97, "IPCMWB", 16000, 80000, 1, 14), + cricket::AudioCodec(9, "G722", 16000, 64000, 1, 13), + cricket::AudioCodec(119, "ISACLC", 16000, 40000, 1, 16), + cricket::AudioCodec(103, "ISAC", 16000, -1, 1, 18), + cricket::AudioCodec(99, "speex", 16000, 22000, 1, 15), + cricket::AudioCodec(100, "EG711U", 8000, 64000, 1, 9), + cricket::AudioCodec(101, "EG711A", 8000, 64000, 1, 8), + cricket::AudioCodec(0, "PCMU", 8000, 64000, 1, 7), + cricket::AudioCodec(8, "PCMA", 8000, 64000, 1, 6), + cricket::AudioCodec(102, "iLBC", 8000, 13300, 1, 12), + cricket::AudioCodec(3, "GSM", 8000, 13000, 1, 10), + cricket::AudioCodec(98, "speex", 8000, 11000, 1, 11), + cricket::AudioCodec(126, "CN", 32000, 0, 1, 5), + cricket::AudioCodec(105, "CN", 16000, 0, 1, 4), + cricket::AudioCodec(13, "CN", 8000, 0, 1, 3), + cricket::AudioCodec(117, "red", 8000, 0, 1, 2), + cricket::AudioCodec(106, "telephone-event", 8000, 0, 1, 1) +}; + static const cricket::VideoCodec kVideoCodecs[] = { cricket::VideoCodec(96, "H264-SVC", 320, 200, 30, 1) }; @@ -270,123 +294,6 @@ const std::string kJingleInitiate( " " \ " "); -// Initiate string with a different order of supported codecs. -// Should accept the supported ones, but with our desired order. -const std::string kGingleInitiateDifferentPreference( - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " "); - -const std::string kJingleInitiateDifferentPreference( - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " " \ - " "); - const std::string kJingleInitiateWithRtcpFb( " " \ @@ -2793,6 +2700,8 @@ class MediaSessionClientTest : public sigslot::has_slots<> { expected_data_fb_params_ = params_nack; } + cricket::FakeMediaEngine* fme() { return fme_; } + private: void OnSendStanza(cricket::SessionManager* manager, const buzz::XmlElement* stanza) { @@ -2949,11 +2858,15 @@ TEST(MediaSessionTest, JingleGoodInitiateAllSupportedAudioCodecs) { test->TestHasAllSupportedAudioCodecs(elem.get()); } +// Changes the codecs that our FakeMediaEngine will support with a different +// preference order than the incoming offer. +// Verifies the answer accepts the preference order of the remote peer. TEST(MediaSessionTest, JingleGoodInitiateDifferentPreferenceAudioCodecs) { talk_base::scoped_ptr test(JingleTest()); + test->fme()->SetAudioCodecs(MAKE_VECTOR(kAudioCodecsDifferentPreference)); talk_base::scoped_ptr elem; test->TestGoodIncomingInitiate( - kJingleInitiateDifferentPreference, AudioCallOptions(), elem.use()); + kJingleInitiate, AudioCallOptions(), elem.use()); test->TestHasAllSupportedAudioCodecs(elem.get()); } @@ -3213,11 +3126,15 @@ TEST(MediaSessionTest, GingleGoodInitiateAllSupportedAudioCodecsWithCrypto) { test->TestHasAllSupportedAudioCodecs(elem.get()); } +// Changes the codecs that our FakeMediaEngine will support with a different +// preference order than the incoming offer. +// Verifies the answer accepts the preference order of the remote peer. TEST(MediaSessionTest, GingleGoodInitiateDifferentPreferenceAudioCodecs) { - talk_base::scoped_ptr elem; talk_base::scoped_ptr test(GingleTest()); + test->fme()->SetAudioCodecs(MAKE_VECTOR(kAudioCodecsDifferentPreference)); + talk_base::scoped_ptr elem; test->TestGoodIncomingInitiate( - kGingleInitiateDifferentPreference, AudioCallOptions(), elem.use()); + kGingleInitiate, AudioCallOptions(), elem.use()); test->TestHasAllSupportedAudioCodecs(elem.get()); }