From 67cf2c1294e65d270e860a3103f7dd630656766d Mon Sep 17 00:00:00 2001 From: deadbeef Date: Wed, 13 Apr 2016 10:07:16 -0700 Subject: [PATCH] Removing `preference` field from `cricket::Codec`. This field only existed as an implementation detail for getting the codecs sorted, so it doesn't need to be in the public interface. It cluttered the code and undesirably affected codec comparisons, causing the video encoder to be reconfigured if a codec's preference changed but nothing else did. BUG=webrtc:5690 Review URL: https://codereview.webrtc.org/1845673002 Cr-Commit-Position: refs/heads/master@{#12349} --- webrtc/api/jsepsessiondescription.cc | 1 - webrtc/api/jsepsessiondescription.h | 1 - webrtc/api/jsepsessiondescription_unittest.cc | 4 +- webrtc/api/webrtcsdp.cc | 109 ++++++------- webrtc/api/webrtcsdp_unittest.cc | 26 ++-- webrtc/api/webrtcsession_unittest.cc | 8 +- webrtc/media/base/codec.cc | 44 ++---- webrtc/media/base/codec.h | 23 +-- webrtc/media/base/codec_unittest.cc | 144 +++++++++--------- webrtc/media/base/fakemediaengine.h | 4 +- webrtc/media/base/rtpdataengine.cc | 7 +- webrtc/media/base/videoengine_unittest.h | 2 +- webrtc/media/engine/webrtcvideoengine2.cc | 23 +-- .../engine/webrtcvideoengine2_unittest.cc | 59 ++++--- webrtc/media/engine/webrtcvoiceengine.cc | 49 +++--- .../engine/webrtcvoiceengine_unittest.cc | 80 +++++----- webrtc/media/sctp/sctpdataengine.cc | 2 +- webrtc/pc/channel_unittest.cc | 12 +- webrtc/pc/channelmanager_unittest.cc | 10 +- webrtc/pc/mediasession.cc | 30 ++-- webrtc/pc/mediasession.h | 8 +- webrtc/pc/mediasession_unittest.cc | 66 ++++---- 22 files changed, 333 insertions(+), 379 deletions(-) diff --git a/webrtc/api/jsepsessiondescription.cc b/webrtc/api/jsepsessiondescription.cc index eb776c86fa..ee0a8e14eb 100644 --- a/webrtc/api/jsepsessiondescription.cc +++ b/webrtc/api/jsepsessiondescription.cc @@ -57,7 +57,6 @@ const int JsepSessionDescription::kMaxVideoCodecHeight = 1280; const int JsepSessionDescription::kMaxVideoCodecWidth = 1920; const int JsepSessionDescription::kMaxVideoCodecHeight = 1080; #endif -const int JsepSessionDescription::kDefaultVideoCodecPreference = 1; SessionDescriptionInterface* CreateSessionDescription(const std::string& type, const std::string& sdp, diff --git a/webrtc/api/jsepsessiondescription.h b/webrtc/api/jsepsessiondescription.h index 9a0d873204..ca39b8a675 100644 --- a/webrtc/api/jsepsessiondescription.h +++ b/webrtc/api/jsepsessiondescription.h @@ -72,7 +72,6 @@ class JsepSessionDescription : public SessionDescriptionInterface { static const char kDefaultVideoCodecName[]; static const int kMaxVideoCodecWidth; static const int kMaxVideoCodecHeight; - static const int kDefaultVideoCodecPreference; private: rtc::scoped_ptr description_; diff --git a/webrtc/api/jsepsessiondescription_unittest.cc b/webrtc/api/jsepsessiondescription_unittest.cc index 3d87513731..8f9fc5427f 100644 --- a/webrtc/api/jsepsessiondescription_unittest.cc +++ b/webrtc/api/jsepsessiondescription_unittest.cc @@ -48,11 +48,11 @@ static cricket::SessionDescription* CreateCricketSessionDescription() { scoped_ptr video( new cricket::VideoContentDescription()); - audio->AddCodec(cricket::AudioCodec(103, "ISAC", 16000, 0, 0, 0)); + audio->AddCodec(cricket::AudioCodec(103, "ISAC", 16000, 0, 0)); desc->AddContent(cricket::CN_AUDIO, cricket::NS_JINGLE_RTP, audio.release()); - video->AddCodec(cricket::VideoCodec(120, "VP8", 640, 480, 30, 0)); + video->AddCodec(cricket::VideoCodec(120, "VP8", 640, 480, 30)); desc->AddContent(cricket::CN_VIDEO, cricket::NS_JINGLE_RTP, video.release()); diff --git a/webrtc/api/webrtcsdp.cc b/webrtc/api/webrtcsdp.cc index 90e0007628..8497f4c5fa 100644 --- a/webrtc/api/webrtcsdp.cc +++ b/webrtc/api/webrtcsdp.cc @@ -15,6 +15,7 @@ #include #include #include +#include #include #include "webrtc/api/jsepicecandidate.h" @@ -269,7 +270,7 @@ static bool ParseContent(const std::string& message, const MediaType media_type, int mline_index, const std::string& protocol, - const std::vector& codec_preference, + const std::vector& payload_types, size_t* pos, std::string* content_name, MediaContentDescription* media_desc, @@ -287,7 +288,7 @@ static bool ParseCryptoAttribute(const std::string& line, SdpParseError* error); static bool ParseRtpmapAttribute(const std::string& line, const MediaType media_type, - const std::vector& codec_preference, + const std::vector& payload_types, MediaContentDescription* media_desc, SdpParseError* error); static bool ParseFmtpAttributes(const std::string& line, @@ -1651,9 +1652,8 @@ bool AddSctpDataCodec(DataContentDescription* media_desc, NULL); } // Add the SCTP Port number as a pseudo-codec "port" parameter - cricket::DataCodec codec_port( - cricket::kGoogleSctpDataCodecId, cricket::kGoogleSctpDataCodecName, - 0); + cricket::DataCodec codec_port(cricket::kGoogleSctpDataCodecId, + cricket::kGoogleSctpDataCodecName); codec_port.SetParam(cricket::kCodecParamPort, sctp_port); LOG(INFO) << "AddSctpDataCodec: Got SCTP Port Number " << sctp_port; @@ -2179,9 +2179,8 @@ void MaybeCreateStaticPayloadAudioCodecs( if (!media_desc) { return; } - int preference = static_cast(fmts.size()); + RTC_DCHECK(media_desc->codecs().empty()); std::vector::const_iterator it = fmts.begin(); - bool add_new_codec = false; for (; it != fmts.end(); ++it) { int payload_type = *it; if (!media_desc->HasCodec(payload_type) && @@ -2191,14 +2190,8 @@ void MaybeCreateStaticPayloadAudioCodecs( int clock_rate = kStaticPayloadAudioCodecs[payload_type].clockrate; size_t channels = kStaticPayloadAudioCodecs[payload_type].channels; media_desc->AddCodec(cricket::AudioCodec(payload_type, encoding_name, - clock_rate, 0, channels, - preference)); - add_new_codec = true; + clock_rate, 0, channels)); } - --preference; - } - if (add_new_codec) { - media_desc->SortCodecs(); } } @@ -2207,7 +2200,7 @@ static C* ParseContentDescription(const std::string& message, const MediaType media_type, int mline_index, const std::string& protocol, - const std::vector& codec_preference, + const std::vector& payload_types, size_t* pos, std::string* content_name, TransportDescription* transport, @@ -2228,14 +2221,28 @@ static C* ParseContentDescription(const std::string& message, ASSERT(false); break; } - if (!ParseContent(message, media_type, mline_index, protocol, - codec_preference, pos, content_name, - media_desc, transport, candidates, error)) { + if (!ParseContent(message, media_type, mline_index, protocol, payload_types, + pos, content_name, media_desc, transport, candidates, + error)) { delete media_desc; return NULL; } // Sort the codecs according to the m-line fmt list. - media_desc->SortCodecs(); + std::unordered_map payload_type_preferences; + // "size + 1" so that the lowest preference payload type has a preference of + // 1, which is greater than the default (0) for payload types not in the fmt + // list. + int preference = static_cast(payload_types.size() + 1); + for (int pt : payload_types) { + payload_type_preferences[pt] = preference--; + } + std::vector codecs = media_desc->codecs(); + std::sort(codecs.begin(), codecs.end(), [&payload_type_preferences]( + const typename C::CodecType& a, + const typename C::CodecType& b) { + return payload_type_preferences[a.id] > payload_type_preferences[b.id]; + }); + media_desc->set_codecs(codecs); return media_desc; } @@ -2274,7 +2281,7 @@ bool ParseMediaDescription(const std::string& message, std::string protocol = fields[2]; // - std::vector codec_preference; + std::vector payload_types; if (IsRtp(protocol)) { for (size_t j = 3 ; j < fields.size(); ++j) { // TODO(wu): Remove when below bug is fixed. @@ -2287,7 +2294,7 @@ bool ParseMediaDescription(const std::string& message, if (!GetPayloadTypeFromString(line, fields[j], &pl, error)) { return false; } - codec_preference.push_back(pl); + payload_types.push_back(pl); } } @@ -2302,20 +2309,17 @@ bool ParseMediaDescription(const std::string& message, std::string content_name; if (HasAttribute(line, kMediaTypeVideo)) { content.reset(ParseContentDescription( - message, cricket::MEDIA_TYPE_VIDEO, mline_index, protocol, - codec_preference, pos, &content_name, - &transport, candidates, error)); + message, cricket::MEDIA_TYPE_VIDEO, mline_index, protocol, + payload_types, pos, &content_name, &transport, candidates, error)); } else if (HasAttribute(line, kMediaTypeAudio)) { content.reset(ParseContentDescription( - message, cricket::MEDIA_TYPE_AUDIO, mline_index, protocol, - codec_preference, pos, &content_name, - &transport, candidates, error)); + message, cricket::MEDIA_TYPE_AUDIO, mline_index, protocol, + payload_types, pos, &content_name, &transport, candidates, error)); } else if (HasAttribute(line, kMediaTypeData)) { DataContentDescription* data_desc = ParseContentDescription( - message, cricket::MEDIA_TYPE_DATA, mline_index, protocol, - codec_preference, pos, &content_name, - &transport, candidates, error); + message, cricket::MEDIA_TYPE_DATA, mline_index, protocol, + payload_types, pos, &content_name, &transport, candidates, error); content.reset(data_desc); int p; @@ -2522,7 +2526,7 @@ bool ParseContent(const std::string& message, const MediaType media_type, int mline_index, const std::string& protocol, - const std::vector& codec_preference, + const std::vector& payload_types, size_t* pos, std::string* content_name, MediaContentDescription* media_desc, @@ -2535,7 +2539,7 @@ bool ParseContent(const std::string& message, if (media_type == cricket::MEDIA_TYPE_AUDIO) { MaybeCreateStaticPayloadAudioCodecs( - codec_preference, static_cast(media_desc)); + payload_types, static_cast(media_desc)); } // The media level "ice-ufrag" and "ice-pwd". @@ -2670,8 +2674,8 @@ bool ParseContent(const std::string& message, return false; } } else if (HasAttribute(line, kAttributeRtpmap)) { - if (!ParseRtpmapAttribute(line, media_type, codec_preference, - media_desc, error)) { + if (!ParseRtpmapAttribute(line, media_type, payload_types, media_desc, + error)) { return false; } } else if (HasAttribute(line, kCodecParamMaxPTime)) { @@ -2927,9 +2931,12 @@ bool ParseCryptoAttribute(const std::string& line, } // Updates or creates a new codec entry in the audio description with according -// to |name|, |clockrate|, |bitrate|, |channels| and |preference|. -void UpdateCodec(int payload_type, const std::string& name, int clockrate, - int bitrate, size_t channels, int preference, +// to |name|, |clockrate|, |bitrate|, and |channels|. +void UpdateCodec(int payload_type, + const std::string& name, + int clockrate, + int bitrate, + size_t channels, AudioContentDescription* audio_desc) { // Codec may already be populated with (only) optional parameters // (from an fmtp). @@ -2939,15 +2946,17 @@ void UpdateCodec(int payload_type, const std::string& name, int clockrate, codec.clockrate = clockrate; codec.bitrate = bitrate; codec.channels = channels; - codec.preference = preference; AddOrReplaceCodec(audio_desc, codec); } // Updates or creates a new codec entry in the video description according to -// |name|, |width|, |height|, |framerate| and |preference|. -void UpdateCodec(int payload_type, const std::string& name, int width, - int height, int framerate, int preference, +// |name|, |width|, |height|, and |framerate|. +void UpdateCodec(int payload_type, + const std::string& name, + int width, + int height, + int framerate, VideoContentDescription* video_desc) { // Codec may already be populated with (only) optional parameters // (from an fmtp). @@ -2957,14 +2966,13 @@ void UpdateCodec(int payload_type, const std::string& name, int width, codec.width = width; codec.height = height; codec.framerate = framerate; - codec.preference = preference; AddOrReplaceCodec(video_desc, codec); } bool ParseRtpmapAttribute(const std::string& line, const MediaType media_type, - const std::vector& codec_preference, + const std::vector& payload_types, MediaContentDescription* media_desc, SdpParseError* error) { std::vector fields; @@ -2986,12 +2994,8 @@ bool ParseRtpmapAttribute(const std::string& line, return false; } - // Set the preference order depending on the order of the pl type in the - // of the m-line. - const int preference = codec_preference.end() - - std::find(codec_preference.begin(), codec_preference.end(), - payload_type); - if (preference == 0) { + if (std::find(payload_types.begin(), payload_types.end(), payload_type) == + payload_types.end()) { LOG(LS_WARNING) << "Ignore rtpmap line that did not appear in the " << " of the m-line: " << line; return true; @@ -3021,7 +3025,7 @@ bool ParseRtpmapAttribute(const std::string& line, JsepSessionDescription::kMaxVideoCodecWidth, JsepSessionDescription::kMaxVideoCodecHeight, JsepSessionDescription::kDefaultVideoCodecFramerate, - preference, video_desc); + video_desc); } else if (media_type == cricket::MEDIA_TYPE_AUDIO) { // RFC 4566 // For audio streams, indicates the number @@ -3049,12 +3053,11 @@ bool ParseRtpmapAttribute(const std::string& line, AudioContentDescription* audio_desc = static_cast(media_desc); UpdateCodec(payload_type, encoding_name, clock_rate, bitrate, channels, - preference, audio_desc); + audio_desc); } else if (media_type == cricket::MEDIA_TYPE_DATA) { DataContentDescription* data_desc = static_cast(media_desc); - data_desc->AddCodec(cricket::DataCodec(payload_type, encoding_name, - preference)); + data_desc->AddCodec(cricket::DataCodec(payload_type, encoding_name)); } return true; } diff --git a/webrtc/api/webrtcsdp_unittest.cc b/webrtc/api/webrtcsdp_unittest.cc index 3734ac5b86..2b63c798b0 100644 --- a/webrtc/api/webrtcsdp_unittest.cc +++ b/webrtc/api/webrtcsdp_unittest.cc @@ -1030,10 +1030,10 @@ class WebRtcSdpTest : public testing::Test { "inline:NzB4d1BINUAvLEw6UzF3WSJ+PSdFcGdUJShpX1Zj|2^20|1:32", "dummy_session_params")); audio->set_protocol(cricket::kMediaProtocolSavpf); - AudioCodec opus(111, "opus", 48000, 0, 2, 3); + AudioCodec opus(111, "opus", 48000, 0, 2); audio->AddCodec(opus); - audio->AddCodec(AudioCodec(103, "ISAC", 16000, 32000, 1, 2)); - audio->AddCodec(AudioCodec(104, "ISAC", 32000, 56000, 1, 1)); + audio->AddCodec(AudioCodec(103, "ISAC", 16000, 32000, 1)); + audio->AddCodec(AudioCodec(104, "ISAC", 32000, 56000, 1)); return audio; } @@ -1049,8 +1049,7 @@ class WebRtcSdpTest : public testing::Test { VideoCodec(120, JsepSessionDescription::kDefaultVideoCodecName, JsepSessionDescription::kMaxVideoCodecWidth, JsepSessionDescription::kMaxVideoCodecHeight, - JsepSessionDescription::kDefaultVideoCodecFramerate, - JsepSessionDescription::kDefaultVideoCodecPreference)); + JsepSessionDescription::kDefaultVideoCodecFramerate)); return video; } @@ -1400,7 +1399,7 @@ class WebRtcSdpTest : public testing::Test { data_desc_ = data.get(); data_desc_->set_protocol(cricket::kMediaProtocolDtlsSctp); DataCodec codec(cricket::kGoogleSctpDataCodecId, - cricket::kGoogleSctpDataCodecName, 0); + cricket::kGoogleSctpDataCodecName); codec.SetParam(cricket::kCodecParamPort, kDefaultSctpPort); data_desc_->AddCodec(codec); desc_.AddContent(kDataContentName, NS_JINGLE_DRAFT_SCTP, data.release()); @@ -1413,7 +1412,7 @@ class WebRtcSdpTest : public testing::Test { new DataContentDescription()); data_desc_ = data.get(); - data_desc_->AddCodec(DataCodec(101, "google-data", 1)); + data_desc_->AddCodec(DataCodec(101, "google-data")); StreamParams data_stream; data_stream.id = kDataChannelMsid; data_stream.cname = kDataChannelCname; @@ -1995,8 +1994,8 @@ TEST_F(WebRtcSdpTest, SerializeWithSctpDataChannelAndNewPort) { jsep_desc.description()->GetContentDescriptionByName(kDataContentName)); const int kNewPort = 1234; - cricket::DataCodec codec( - cricket::kGoogleSctpDataCodecId, cricket::kGoogleSctpDataCodecName, 0); + cricket::DataCodec codec(cricket::kGoogleSctpDataCodecId, + cricket::kGoogleSctpDataCodecName); codec.SetParam(cricket::kCodecParamPort, kNewPort); dcdesc->AddOrReplaceCodec(codec); @@ -2177,10 +2176,11 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithoutRtpmap) { static_cast( jdesc.description()->GetContentDescriptionByName(cricket::CN_AUDIO)); AudioCodecs ref_codecs; - // The codecs in the AudioContentDescription will be sorted by preference. - ref_codecs.push_back(AudioCodec(0, "PCMU", 8000, 0, 1, 3)); - ref_codecs.push_back(AudioCodec(18, "G729", 16000, 0, 1, 2)); - ref_codecs.push_back(AudioCodec(103, "ISAC", 16000, 32000, 1, 1)); + // The codecs in the AudioContentDescription should be in the same order as + // the payload types (s) on the m= line. + ref_codecs.push_back(AudioCodec(0, "PCMU", 8000, 0, 1)); + ref_codecs.push_back(AudioCodec(18, "G729", 16000, 0, 1)); + ref_codecs.push_back(AudioCodec(103, "ISAC", 16000, 32000, 1)); EXPECT_EQ(ref_codecs, audio->codecs()); } diff --git a/webrtc/api/webrtcsession_unittest.cc b/webrtc/api/webrtcsession_unittest.cc index 28f93dfd71..feb1518db0 100644 --- a/webrtc/api/webrtcsession_unittest.cc +++ b/webrtc/api/webrtcsession_unittest.cc @@ -450,8 +450,8 @@ class WebRtcSessionTest void InitWithDtmfCodec() { // Add kTelephoneEventCodec for dtmf test. - const cricket::AudioCodec kTelephoneEventCodec( - 106, "telephone-event", 8000, 0, 1, 0); + const cricket::AudioCodec kTelephoneEventCodec(106, "telephone-event", 8000, + 0, 1); std::vector codecs; codecs.push_back(kTelephoneEventCodec); media_engine_->SetAudioCodecs(codecs); @@ -1313,8 +1313,8 @@ class WebRtcSessionTest // Adds CN codecs to FakeMediaEngine and MediaDescriptionFactory. void AddCNCodecs() { - const cricket::AudioCodec kCNCodec1(102, "CN", 8000, 0, 1, 0); - const cricket::AudioCodec kCNCodec2(103, "CN", 16000, 0, 1, 0); + const cricket::AudioCodec kCNCodec1(102, "CN", 8000, 0, 1); + const cricket::AudioCodec kCNCodec2(103, "CN", 16000, 0, 1); // Add kCNCodec for dtmf test. std::vector codecs = media_engine_->audio_codecs();; diff --git a/webrtc/media/base/codec.cc b/webrtc/media/base/codec.cc index 287de0cdbf..9e54d2971a 100644 --- a/webrtc/media/base/codec.cc +++ b/webrtc/media/base/codec.cc @@ -71,12 +71,10 @@ bool FeedbackParams::HasDuplicateEntries() const { return false; } -Codec::Codec(int id, const std::string& name, int clockrate, int preference) - : id(id), name(name), clockrate(clockrate), preference(preference) { -} +Codec::Codec(int id, const std::string& name, int clockrate) + : id(id), name(name), clockrate(clockrate) {} -Codec::Codec() : id(0), clockrate(0), preference(0) { -} +Codec::Codec() : id(0), clockrate(0) {} Codec::Codec(const Codec& c) = default; @@ -86,7 +84,6 @@ Codec& Codec::operator=(const Codec& c) { this->id = c.id; // id is reserved in objective-c name = c.name; clockrate = c.clockrate; - preference = c.preference; params = c.params; feedback_params = c.feedback_params; return *this; @@ -94,8 +91,7 @@ Codec& Codec::operator=(const Codec& c) { bool Codec::operator==(const Codec& c) const { return this->id == c.id && // id is reserved in objective-c - name == c.name && clockrate == c.clockrate && - preference == c.preference && params == c.params && + name == c.name && clockrate == c.clockrate && params == c.params && feedback_params == c.feedback_params; } @@ -150,12 +146,8 @@ AudioCodec::AudioCodec(int id, const std::string& name, int clockrate, int bitrate, - size_t channels, - int preference) - : Codec(id, name, clockrate, preference), - bitrate(bitrate), - channels(channels) { -} + size_t channels) + : Codec(id, name, clockrate), bitrate(bitrate), channels(channels) {} AudioCodec::AudioCodec() : Codec(), bitrate(0), channels(0) { } @@ -193,14 +185,14 @@ bool AudioCodec::Matches(const AudioCodec& codec) const { std::string AudioCodec::ToString() const { std::ostringstream os; os << "AudioCodec[" << id << ":" << name << ":" << clockrate << ":" << bitrate - << ":" << channels << ":" << preference << "]"; + << ":" << channels << "]"; return os.str(); } std::string VideoCodec::ToString() const { std::ostringstream os; os << "VideoCodec[" << id << ":" << name << ":" << width << ":" << height - << ":" << framerate << ":" << preference << "]"; + << ":" << framerate << "]"; return os.str(); } @@ -208,20 +200,17 @@ VideoCodec::VideoCodec(int id, const std::string& name, int width, int height, - int framerate, - int preference) - : Codec(id, name, kVideoCodecClockrate, preference), + int framerate) + : Codec(id, name, kVideoCodecClockrate), width(width), height(height), - framerate(framerate) { -} + framerate(framerate) {} VideoCodec::VideoCodec(int id, const std::string& name) - : Codec(id, name, kVideoCodecClockrate, 0), + : Codec(id, name, kVideoCodecClockrate), width(0), height(0), - framerate(0) { -} + framerate(0) {} VideoCodec::VideoCodec() : Codec(), width(0), height(0), framerate(0) { clockrate = kVideoCodecClockrate; @@ -244,7 +233,7 @@ bool VideoCodec::operator==(const VideoCodec& c) const { VideoCodec VideoCodec::CreateRtxCodec(int rtx_payload_type, int associated_payload_type) { - VideoCodec rtx_codec(rtx_payload_type, kRtxCodecName, 0, 0, 0, 0); + VideoCodec rtx_codec(rtx_payload_type, kRtxCodecName, 0, 0, 0); rtx_codec.SetParam(kCodecParamAssociatedPayloadType, associated_payload_type); return rtx_codec; } @@ -291,9 +280,8 @@ bool VideoCodec::ValidateCodecFormat() const { return true; } -DataCodec::DataCodec(int id, const std::string& name, int preference) - : Codec(id, name, kDataCodecClockrate, preference) { -} +DataCodec::DataCodec(int id, const std::string& name) + : Codec(id, name, kDataCodecClockrate) {} DataCodec::DataCodec() : Codec() { clockrate = kDataCodecClockrate; diff --git a/webrtc/media/base/codec.h b/webrtc/media/base/codec.h index 7476c02694..c79a3fafc8 100644 --- a/webrtc/media/base/codec.h +++ b/webrtc/media/base/codec.h @@ -64,12 +64,11 @@ struct Codec { int id; std::string name; int clockrate; - int preference; CodecParameterMap params; FeedbackParams feedback_params; // Creates a codec with the given parameters. - Codec(int id, const std::string& name, int clockrate, int preference); + Codec(int id, const std::string& name, int clockrate); // Creates an empty codec. Codec(); Codec(const Codec& c); @@ -92,10 +91,6 @@ struct Codec { bool HasFeedbackParam(const FeedbackParam& param) const; void AddFeedbackParam(const FeedbackParam& param); - static bool Preferable(const Codec& first, const Codec& other) { - return first.preference > other.preference; - } - // Filter |this| feedbacks params such that only those shared by both |this| // and |other| are kept. void IntersectFeedbackParams(const Codec& other); @@ -118,8 +113,7 @@ struct AudioCodec : public Codec { const std::string& name, int clockrate, int bitrate, - size_t channels, - int preference); + size_t channels); // Creates an empty codec. AudioCodec(); AudioCodec(const AudioCodec& c); @@ -128,10 +122,6 @@ struct AudioCodec : public Codec { // Indicates if this codec is compatible with the specified codec. bool Matches(const AudioCodec& codec) const; - static bool Preferable(const AudioCodec& first, const AudioCodec& other) { - return first.preference > other.preference; - } - std::string ToString() const; AudioCodec& operator=(const AudioCodec& c); @@ -153,18 +143,13 @@ struct VideoCodec : public Codec { const std::string& name, int width, int height, - int framerate, - int preference); + int framerate); VideoCodec(int id, const std::string& name); // Creates an empty codec. VideoCodec(); VideoCodec(const VideoCodec& c); ~VideoCodec() = default; - static bool Preferable(const VideoCodec& first, const VideoCodec& other) { - return first.preference > other.preference; - } - std::string ToString() const; VideoCodec& operator=(const VideoCodec& c); @@ -193,7 +178,7 @@ struct VideoCodec : public Codec { }; struct DataCodec : public Codec { - DataCodec(int id, const std::string& name, int preference); + DataCodec(int id, const std::string& name); DataCodec(); DataCodec(const DataCodec& c); diff --git a/webrtc/media/base/codec_unittest.cc b/webrtc/media/base/codec_unittest.cc index 88d49632d0..26a6ddae18 100644 --- a/webrtc/media/base/codec_unittest.cc +++ b/webrtc/media/base/codec_unittest.cc @@ -26,7 +26,7 @@ class CodecTest : public testing::Test { }; TEST_F(CodecTest, TestCodecOperators) { - Codec c0(96, "D", 1000, 0); + Codec c0(96, "D", 1000); c0.SetParam("a", 1); Codec c1 = c0; @@ -49,36 +49,30 @@ TEST_F(CodecTest, TestCodecOperators) { c1.clockrate = 2000; EXPECT_TRUE(c0 != c1); - c1 = c0; - c1.preference = 1; - EXPECT_TRUE(c0 != c1); - c1 = c0; c1.SetParam("a", 2); EXPECT_TRUE(c0 != c1); Codec c5; - Codec c6(0, "", 0, 0); + Codec c6(0, "", 0); EXPECT_TRUE(c5 == c6); } TEST_F(CodecTest, TestAudioCodecOperators) { - AudioCodec c0(96, "A", 44100, 20000, 2, 3); - AudioCodec c1(95, "A", 44100, 20000, 2, 3); - AudioCodec c2(96, "x", 44100, 20000, 2, 3); - AudioCodec c3(96, "A", 48000, 20000, 2, 3); - AudioCodec c4(96, "A", 44100, 10000, 2, 3); - AudioCodec c5(96, "A", 44100, 20000, 1, 3); - AudioCodec c6(96, "A", 44100, 20000, 2, 1); + AudioCodec c0(96, "A", 44100, 20000, 2); + AudioCodec c1(95, "A", 44100, 20000, 2); + AudioCodec c2(96, "x", 44100, 20000, 2); + AudioCodec c3(96, "A", 48000, 20000, 2); + AudioCodec c4(96, "A", 44100, 10000, 2); + AudioCodec c5(96, "A", 44100, 20000, 1); EXPECT_TRUE(c0 != c1); EXPECT_TRUE(c0 != c2); EXPECT_TRUE(c0 != c3); EXPECT_TRUE(c0 != c4); EXPECT_TRUE(c0 != c5); - EXPECT_TRUE(c0 != c6); AudioCodec c7; - AudioCodec c8(0, "", 0, 0, 0, 0); + AudioCodec c8(0, "", 0, 0, 0); AudioCodec c9 = c0; EXPECT_TRUE(c8 == c7); EXPECT_TRUE(c9 != c7); @@ -103,61 +97,59 @@ TEST_F(CodecTest, TestAudioCodecOperators) { TEST_F(CodecTest, TestAudioCodecMatches) { // Test a codec with a static payload type. - AudioCodec c0(95, "A", 44100, 20000, 1, 3); - EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 44100, 20000, 1, 0))); - EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 44100, 20000, 0, 0))); - EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 44100, 0, 0, 0))); - EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 0, 0, 0, 0))); - EXPECT_FALSE(c0.Matches(AudioCodec(96, "", 44100, 20000, 1, 0))); - EXPECT_FALSE(c0.Matches(AudioCodec(95, "", 55100, 20000, 1, 0))); - EXPECT_FALSE(c0.Matches(AudioCodec(95, "", 44100, 30000, 1, 0))); - EXPECT_FALSE(c0.Matches(AudioCodec(95, "", 44100, 20000, 2, 0))); - EXPECT_FALSE(c0.Matches(AudioCodec(95, "", 55100, 30000, 2, 0))); + AudioCodec c0(95, "A", 44100, 20000, 1); + EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 44100, 20000, 1))); + EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 44100, 20000, 0))); + EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 44100, 0, 0))); + EXPECT_TRUE(c0.Matches(AudioCodec(95, "", 0, 0, 0))); + EXPECT_FALSE(c0.Matches(AudioCodec(96, "", 44100, 20000, 1))); + EXPECT_FALSE(c0.Matches(AudioCodec(95, "", 55100, 20000, 1))); + EXPECT_FALSE(c0.Matches(AudioCodec(95, "", 44100, 30000, 1))); + EXPECT_FALSE(c0.Matches(AudioCodec(95, "", 44100, 20000, 2))); + EXPECT_FALSE(c0.Matches(AudioCodec(95, "", 55100, 30000, 2))); // Test a codec with a dynamic payload type. - AudioCodec c1(96, "A", 44100, 20000, 1, 3); - EXPECT_TRUE(c1.Matches(AudioCodec(96, "A", 0, 0, 0, 0))); - EXPECT_TRUE(c1.Matches(AudioCodec(97, "A", 0, 0, 0, 0))); - EXPECT_TRUE(c1.Matches(AudioCodec(96, "a", 0, 0, 0, 0))); - EXPECT_TRUE(c1.Matches(AudioCodec(97, "a", 0, 0, 0, 0))); - EXPECT_FALSE(c1.Matches(AudioCodec(95, "A", 0, 0, 0, 0))); - EXPECT_FALSE(c1.Matches(AudioCodec(96, "", 44100, 20000, 2, 0))); - EXPECT_FALSE(c1.Matches(AudioCodec(96, "A", 55100, 30000, 1, 0))); + AudioCodec c1(96, "A", 44100, 20000, 1); + EXPECT_TRUE(c1.Matches(AudioCodec(96, "A", 0, 0, 0))); + EXPECT_TRUE(c1.Matches(AudioCodec(97, "A", 0, 0, 0))); + EXPECT_TRUE(c1.Matches(AudioCodec(96, "a", 0, 0, 0))); + EXPECT_TRUE(c1.Matches(AudioCodec(97, "a", 0, 0, 0))); + EXPECT_FALSE(c1.Matches(AudioCodec(95, "A", 0, 0, 0))); + EXPECT_FALSE(c1.Matches(AudioCodec(96, "", 44100, 20000, 2))); + EXPECT_FALSE(c1.Matches(AudioCodec(96, "A", 55100, 30000, 1))); // Test a codec with a dynamic payload type, and auto bitrate. - AudioCodec c2(97, "A", 16000, 0, 1, 3); + AudioCodec c2(97, "A", 16000, 0, 1); // Use default bitrate. - EXPECT_TRUE(c2.Matches(AudioCodec(97, "A", 16000, 0, 1, 0))); - EXPECT_TRUE(c2.Matches(AudioCodec(97, "A", 16000, 0, 0, 0))); + EXPECT_TRUE(c2.Matches(AudioCodec(97, "A", 16000, 0, 1))); + EXPECT_TRUE(c2.Matches(AudioCodec(97, "A", 16000, 0, 0))); // Use explicit bitrate. - EXPECT_TRUE(c2.Matches(AudioCodec(97, "A", 16000, 32000, 1, 0))); + EXPECT_TRUE(c2.Matches(AudioCodec(97, "A", 16000, 32000, 1))); // Backward compatibility with clients that might send "-1" (for default). - EXPECT_TRUE(c2.Matches(AudioCodec(97, "A", 16000, -1, 1, 0))); + EXPECT_TRUE(c2.Matches(AudioCodec(97, "A", 16000, -1, 1))); // Stereo doesn't match channels = 0. - AudioCodec c3(96, "A", 44100, 20000, 2, 3); - EXPECT_TRUE(c3.Matches(AudioCodec(96, "A", 44100, 20000, 2, 3))); - EXPECT_FALSE(c3.Matches(AudioCodec(96, "A", 44100, 20000, 1, 3))); - EXPECT_FALSE(c3.Matches(AudioCodec(96, "A", 44100, 20000, 0, 3))); + AudioCodec c3(96, "A", 44100, 20000, 2); + EXPECT_TRUE(c3.Matches(AudioCodec(96, "A", 44100, 20000, 2))); + EXPECT_FALSE(c3.Matches(AudioCodec(96, "A", 44100, 20000, 1))); + EXPECT_FALSE(c3.Matches(AudioCodec(96, "A", 44100, 20000, 0))); } TEST_F(CodecTest, TestVideoCodecOperators) { - VideoCodec c0(96, "V", 320, 200, 30, 3); - VideoCodec c1(95, "V", 320, 200, 30, 3); - VideoCodec c2(96, "x", 320, 200, 30, 3); - VideoCodec c3(96, "V", 120, 200, 30, 3); - VideoCodec c4(96, "V", 320, 100, 30, 3); - VideoCodec c5(96, "V", 320, 200, 10, 3); - VideoCodec c6(96, "V", 320, 200, 30, 1); + VideoCodec c0(96, "V", 320, 200, 30); + VideoCodec c1(95, "V", 320, 200, 30); + VideoCodec c2(96, "x", 320, 200, 30); + VideoCodec c3(96, "V", 120, 200, 30); + VideoCodec c4(96, "V", 320, 100, 30); + VideoCodec c5(96, "V", 320, 200, 10); EXPECT_TRUE(c0 != c1); EXPECT_TRUE(c0 != c2); EXPECT_TRUE(c0 != c3); EXPECT_TRUE(c0 != c4); EXPECT_TRUE(c0 != c5); - EXPECT_TRUE(c0 != c6); VideoCodec c7; - VideoCodec c8(0, "", 0, 0, 0, 0); + VideoCodec c8(0, "", 0, 0, 0); VideoCodec c9 = c0; EXPECT_TRUE(c8 == c7); EXPECT_TRUE(c9 != c7); @@ -182,34 +174,34 @@ TEST_F(CodecTest, TestVideoCodecOperators) { TEST_F(CodecTest, TestVideoCodecMatches) { // Test a codec with a static payload type. - VideoCodec c0(95, "V", 320, 200, 30, 3); - EXPECT_TRUE(c0.Matches(VideoCodec(95, "", 640, 400, 15, 0))); - EXPECT_FALSE(c0.Matches(VideoCodec(96, "", 320, 200, 30, 0))); + VideoCodec c0(95, "V", 320, 200, 30); + EXPECT_TRUE(c0.Matches(VideoCodec(95, "", 640, 400, 15))); + EXPECT_FALSE(c0.Matches(VideoCodec(96, "", 320, 200, 30))); // Test a codec with a dynamic payload type. - VideoCodec c1(96, "V", 320, 200, 30, 3); - EXPECT_TRUE(c1.Matches(VideoCodec(96, "V", 640, 400, 15, 0))); - EXPECT_TRUE(c1.Matches(VideoCodec(97, "V", 640, 400, 15, 0))); - EXPECT_TRUE(c1.Matches(VideoCodec(96, "v", 640, 400, 15, 0))); - EXPECT_TRUE(c1.Matches(VideoCodec(97, "v", 640, 400, 15, 0))); - EXPECT_FALSE(c1.Matches(VideoCodec(96, "", 320, 200, 30, 0))); - EXPECT_FALSE(c1.Matches(VideoCodec(95, "V", 640, 400, 15, 0))); + VideoCodec c1(96, "V", 320, 200, 30); + EXPECT_TRUE(c1.Matches(VideoCodec(96, "V", 640, 400, 15))); + EXPECT_TRUE(c1.Matches(VideoCodec(97, "V", 640, 400, 15))); + EXPECT_TRUE(c1.Matches(VideoCodec(96, "v", 640, 400, 15))); + EXPECT_TRUE(c1.Matches(VideoCodec(97, "v", 640, 400, 15))); + EXPECT_FALSE(c1.Matches(VideoCodec(96, "", 320, 200, 30))); + EXPECT_FALSE(c1.Matches(VideoCodec(95, "V", 640, 400, 15))); } TEST_F(CodecTest, TestDataCodecMatches) { // Test a codec with a static payload type. - DataCodec c0(95, "D", 0); - EXPECT_TRUE(c0.Matches(DataCodec(95, "", 0))); - EXPECT_FALSE(c0.Matches(DataCodec(96, "", 0))); + DataCodec c0(95, "D"); + EXPECT_TRUE(c0.Matches(DataCodec(95, ""))); + EXPECT_FALSE(c0.Matches(DataCodec(96, ""))); // Test a codec with a dynamic payload type. - DataCodec c1(96, "D", 3); - EXPECT_TRUE(c1.Matches(DataCodec(96, "D", 0))); - EXPECT_TRUE(c1.Matches(DataCodec(97, "D", 0))); - EXPECT_TRUE(c1.Matches(DataCodec(96, "d", 0))); - EXPECT_TRUE(c1.Matches(DataCodec(97, "d", 0))); - EXPECT_FALSE(c1.Matches(DataCodec(96, "", 0))); - EXPECT_FALSE(c1.Matches(DataCodec(95, "D", 0))); + DataCodec c1(96, "D"); + EXPECT_TRUE(c1.Matches(DataCodec(96, "D"))); + EXPECT_TRUE(c1.Matches(DataCodec(97, "D"))); + EXPECT_TRUE(c1.Matches(DataCodec(96, "d"))); + EXPECT_TRUE(c1.Matches(DataCodec(97, "d"))); + EXPECT_FALSE(c1.Matches(DataCodec(96, ""))); + EXPECT_FALSE(c1.Matches(DataCodec(95, "D"))); } TEST_F(CodecTest, TestSetParamGetParamAndRemoveParam) { @@ -254,10 +246,10 @@ TEST_F(CodecTest, TestIntersectFeedbackParams) { TEST_F(CodecTest, TestGetCodecType) { // Codec type comparison should be case insenstive on names. - const VideoCodec codec(96, "V", 320, 200, 30, 3); - const VideoCodec rtx_codec(96, "rTx", 320, 200, 30, 3); - const VideoCodec ulpfec_codec(96, "ulpFeC", 320, 200, 30, 3); - const VideoCodec red_codec(96, "ReD", 320, 200, 30, 3); + const VideoCodec codec(96, "V", 320, 200, 30); + const VideoCodec rtx_codec(96, "rTx", 320, 200, 30); + const VideoCodec ulpfec_codec(96, "ulpFeC", 320, 200, 30); + const VideoCodec red_codec(96, "ReD", 320, 200, 30); EXPECT_EQ(VideoCodec::CODEC_VIDEO, codec.GetCodecType()); EXPECT_EQ(VideoCodec::CODEC_RTX, rtx_codec.GetCodecType()); EXPECT_EQ(VideoCodec::CODEC_ULPFEC, ulpfec_codec.GetCodecType()); @@ -275,7 +267,7 @@ TEST_F(CodecTest, TestCreateRtxCodec) { } TEST_F(CodecTest, TestValidateCodecFormat) { - const VideoCodec codec(96, "V", 320, 200, 30, 3); + const VideoCodec codec(96, "V", 320, 200, 30); ASSERT_TRUE(codec.ValidateCodecFormat()); // Accept 0-127 as payload types. diff --git a/webrtc/media/base/fakemediaengine.h b/webrtc/media/base/fakemediaengine.h index 64f4adca50..fbd8f4c275 100644 --- a/webrtc/media/base/fakemediaengine.h +++ b/webrtc/media/base/fakemediaengine.h @@ -702,7 +702,7 @@ class FakeVoiceEngine : public FakeBaseEngine { : output_volume_(-1) { // Add a fake audio codec. Note that the name must not be "" as there are // sanity checks against that. - codecs_.push_back(AudioCodec(101, "fake_audio_codec", 0, 0, 1, 0)); + codecs_.push_back(AudioCodec(101, "fake_audio_codec", 0, 0, 1)); } rtc::scoped_refptr GetAudioState() const { return rtc::scoped_refptr(); @@ -763,7 +763,7 @@ class FakeVideoEngine : public FakeBaseEngine { FakeVideoEngine() : capture_(false) { // Add a fake video codec. Note that the name must not be "" as there are // sanity checks against that. - codecs_.push_back(VideoCodec(0, "fake_video_codec", 0, 0, 0, 0)); + codecs_.push_back(VideoCodec(0, "fake_video_codec", 0, 0, 0)); } void Init() {} bool SetOptions(const VideoOptions& options) { diff --git a/webrtc/media/base/rtpdataengine.cc b/webrtc/media/base/rtpdataengine.cc index ae36174555..4b6647c649 100644 --- a/webrtc/media/base/rtpdataengine.cc +++ b/webrtc/media/base/rtpdataengine.cc @@ -36,8 +36,7 @@ static const size_t kMaxSrtpHmacOverhead = 16; RtpDataEngine::RtpDataEngine() { data_codecs_.push_back( - DataCodec(kGoogleRtpDataCodecId, - kGoogleRtpDataCodecName, 0)); + DataCodec(kGoogleRtpDataCodecId, kGoogleRtpDataCodecName)); SetTiming(new rtc::Timing()); } @@ -92,7 +91,7 @@ void RtpClock::Tick(double now, int* seq_num, uint32_t* timestamp) { } const DataCodec* FindUnknownCodec(const std::vector& codecs) { - DataCodec data_codec(kGoogleRtpDataCodecId, kGoogleRtpDataCodecName, 0); + DataCodec data_codec(kGoogleRtpDataCodecId, kGoogleRtpDataCodecName); std::vector::const_iterator iter; for (iter = codecs.begin(); iter != codecs.end(); ++iter) { if (!iter->Matches(data_codec)) { @@ -103,7 +102,7 @@ const DataCodec* FindUnknownCodec(const std::vector& codecs) { } const DataCodec* FindKnownCodec(const std::vector& codecs) { - DataCodec data_codec(kGoogleRtpDataCodecId, kGoogleRtpDataCodecName, 0); + DataCodec data_codec(kGoogleRtpDataCodecId, kGoogleRtpDataCodecName); std::vector::const_iterator iter; for (iter = codecs.begin(); iter != codecs.end(); ++iter) { if (iter->Matches(data_codec)) { diff --git a/webrtc/media/base/videoengine_unittest.h b/webrtc/media/base/videoengine_unittest.h index 491149c7e2..394b9c9a8e 100644 --- a/webrtc/media/base/videoengine_unittest.h +++ b/webrtc/media/base/videoengine_unittest.h @@ -151,7 +151,7 @@ class VideoMediaChannelTest : public testing::Test, } bool SetOneCodec(int pt, const char* name, int w, int h, int fr) { - return SetOneCodec(cricket::VideoCodec(pt, name, w, h, fr, 0)); + return SetOneCodec(cricket::VideoCodec(pt, name, w, h, fr)); } bool SetOneCodec(const cricket::VideoCodec& codec) { cricket::VideoFormat capture_format(codec.width, codec.height, diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index b593027dbc..4914d9f51d 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -180,7 +180,7 @@ void AddDefaultFeedbackParams(VideoCodec* codec) { static VideoCodec MakeVideoCodecWithDefaultFeedbackParams(int payload_type, const char* name) { VideoCodec codec(payload_type, name, kDefaultVideoMaxWidth, - kDefaultVideoMaxHeight, kDefaultVideoMaxFramerate, 0); + kDefaultVideoMaxHeight, kDefaultVideoMaxFramerate); AddDefaultFeedbackParams(&codec); return codec; } @@ -646,12 +646,9 @@ std::vector WebRtcVideoEngine2::GetSupportedCodecs() const { const int kExternalVideoPayloadTypeBase = 120; size_t payload_type = kExternalVideoPayloadTypeBase + i; RTC_DCHECK(payload_type < 128); - VideoCodec codec(static_cast(payload_type), - codecs[i].name, - codecs[i].max_width, - codecs[i].max_height, - codecs[i].max_fps, - 0); + VideoCodec codec(static_cast(payload_type), codecs[i].name, + codecs[i].max_width, codecs[i].max_height, + codecs[i].max_fps); AddDefaultFeedbackParams(&codec); supported_codecs.push_back(codec); @@ -739,17 +736,7 @@ bool WebRtcVideoChannel2::ReceiveCodecsHaveChanged( }; std::sort(before.begin(), before.end(), comparison); std::sort(after.begin(), after.end(), comparison); - for (size_t i = 0; i < before.size(); ++i) { - // For the same reason that we sort the codecs, we also ignore the - // preference. We don't want a preference change on the receive - // side to cause recreation of the stream. - before[i].codec.preference = 0; - after[i].codec.preference = 0; - if (before[i] != after[i]) { - return true; - } - } - return false; + return before != after; } bool WebRtcVideoChannel2::GetChangedSendParameters( diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index 7865102159..d765f8232e 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -31,16 +31,16 @@ namespace { static const int kDefaultQpMax = 56; static const int kDefaultFramerate = 30; -static const cricket::VideoCodec kVp8Codec720p(100, "VP8", 1280, 720, 30, 0); -static const cricket::VideoCodec kVp8Codec360p(100, "VP8", 640, 360, 30, 0); -static const cricket::VideoCodec kVp8Codec270p(100, "VP8", 480, 270, 30, 0); +static const cricket::VideoCodec kVp8Codec720p(100, "VP8", 1280, 720, 30); +static const cricket::VideoCodec kVp8Codec360p(100, "VP8", 640, 360, 30); +static const cricket::VideoCodec kVp8Codec270p(100, "VP8", 480, 270, 30); -static const cricket::VideoCodec kVp8Codec(100, "VP8", 640, 400, 30, 0); -static const cricket::VideoCodec kVp9Codec(101, "VP9", 640, 400, 30, 0); -static const cricket::VideoCodec kH264Codec(102, "H264", 640, 400, 30, 0); +static const cricket::VideoCodec kVp8Codec(100, "VP8", 640, 400, 30); +static const cricket::VideoCodec kVp9Codec(101, "VP9", 640, 400, 30); +static const cricket::VideoCodec kH264Codec(102, "H264", 640, 400, 30); -static const cricket::VideoCodec kRedCodec(116, "red", 0, 0, 0, 0); -static const cricket::VideoCodec kUlpfecCodec(117, "ulpfec", 0, 0, 0, 0); +static const cricket::VideoCodec kRedCodec(116, "red", 0, 0, 0); +static const cricket::VideoCodec kUlpfecCodec(117, "ulpfec", 0, 0, 0); static const uint8_t kRedRtxPayloadType = 125; @@ -852,15 +852,15 @@ WEBRTC_BASE_TEST(SendsLowerResolutionOnSmallerFrames); WEBRTC_BASE_TEST(MultipleSendStreams); TEST_F(WebRtcVideoChannel2BaseTest, SendAndReceiveVp8Vga) { - SendAndReceive(cricket::VideoCodec(100, "VP8", 640, 400, 30, 0)); + SendAndReceive(cricket::VideoCodec(100, "VP8", 640, 400, 30)); } TEST_F(WebRtcVideoChannel2BaseTest, SendAndReceiveVp8Qvga) { - SendAndReceive(cricket::VideoCodec(100, "VP8", 320, 200, 30, 0)); + SendAndReceive(cricket::VideoCodec(100, "VP8", 320, 200, 30)); } TEST_F(WebRtcVideoChannel2BaseTest, SendAndReceiveVp8SvcQqvga) { - SendAndReceive(cricket::VideoCodec(100, "VP8", 160, 100, 30, 0)); + SendAndReceive(cricket::VideoCodec(100, "VP8", 160, 100, 30)); } TEST_F(WebRtcVideoChannel2BaseTest, TwoStreamsSendAndReceive) { @@ -2278,7 +2278,7 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithoutFec) { TEST_F(WebRtcVideoChannel2Test, SetSendCodecRejectsRtxWithoutAssociatedPayloadType) { cricket::VideoSendParameters parameters; - cricket::VideoCodec rtx_codec(96, "rtx", 0, 0, 0, 0); + cricket::VideoCodec rtx_codec(96, "rtx", 0, 0, 0); parameters.codecs.push_back(rtx_codec); EXPECT_FALSE(channel_->SetSendParameters(parameters)) << "RTX codec without associated payload type should be rejected."; @@ -2510,6 +2510,26 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsAcceptAllValidPayloadTypes) { } } +// Test that setting the a different set of codecs but with an identical front +// codec doesn't result in the stream being recreated. +// This may happen when a subsequent negotiation includes fewer codecs, as a +// result of one of the codecs being rejected. +TEST_F(WebRtcVideoChannel2Test, + SetSendCodecsIdenticalFirstCodecDoesntRecreateStream) { + cricket::VideoSendParameters parameters1; + parameters1.codecs.push_back(kVp8Codec); + parameters1.codecs.push_back(kVp9Codec); + EXPECT_TRUE(channel_->SetSendParameters(parameters1)); + + AddSendStream(); + EXPECT_EQ(1, fake_call_->GetNumCreatedSendStreams()); + + cricket::VideoSendParameters parameters2; + parameters2.codecs.push_back(kVp8Codec); + EXPECT_TRUE(channel_->SetSendParameters(parameters2)); + EXPECT_EQ(1, fake_call_->GetNumCreatedSendStreams()); +} + TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithOnlyVp8) { cricket::VideoRecvParameters parameters; parameters.codecs.push_back(kVp8Codec); @@ -2520,7 +2540,7 @@ TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithOnlyVp8) { TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithRtx) { cricket::VideoRecvParameters parameters; parameters.codecs.push_back(kVp8Codec); - cricket::VideoCodec rtx_codec(96, "rtx", 0, 0, 0, 0); + cricket::VideoCodec rtx_codec(96, "rtx", 0, 0, 0); parameters.codecs.push_back(rtx_codec); EXPECT_FALSE(channel_->SetRecvParameters(parameters)) << "RTX codec without associated payload should be rejected."; @@ -2532,7 +2552,7 @@ TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithRtx) { parameters.codecs[1].SetParam("apt", kVp8Codec.id); EXPECT_TRUE(channel_->SetRecvParameters(parameters)); - cricket::VideoCodec rtx_codec2(97, "rtx", 0, 0, 0, 0); + cricket::VideoCodec rtx_codec2(97, "rtx", 0, 0, 0); rtx_codec2.SetParam("apt", rtx_codec.id); parameters.codecs.push_back(rtx_codec2); @@ -2562,7 +2582,7 @@ TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsAcceptDefaultCodecs) { TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsRejectUnsupportedCodec) { cricket::VideoRecvParameters parameters; parameters.codecs.push_back(kVp8Codec); - parameters.codecs.push_back(VideoCodec(101, "WTF3", 640, 400, 30, 0)); + parameters.codecs.push_back(VideoCodec(101, "WTF3", 640, 400, 30)); EXPECT_FALSE(channel_->SetRecvParameters(parameters)); } @@ -2630,10 +2650,10 @@ TEST_F(WebRtcVideoChannel2Test, EXPECT_TRUE(channel_->SetRecvParameters(parameters)); } -// Test that setting the same codecs but with a different order and preference +// Test that setting the same codecs but with a different order // doesn't result in the stream being recreated. TEST_F(WebRtcVideoChannel2Test, - SetRecvCodecsDifferentOrderAndPreferenceDoesntRecreateStream) { + SetRecvCodecsDifferentOrderDoesntRecreateStream) { cricket::VideoRecvParameters parameters1; parameters1.codecs.push_back(kVp8Codec); parameters1.codecs.push_back(kRedCodec); @@ -2645,7 +2665,6 @@ TEST_F(WebRtcVideoChannel2Test, cricket::VideoRecvParameters parameters2; parameters2.codecs.push_back(kRedCodec); parameters2.codecs.push_back(kVp8Codec); - parameters2.codecs[1].preference += 1; EXPECT_TRUE(channel_->SetRecvParameters(parameters2)); EXPECT_EQ(1, fake_call_->GetNumCreatedReceiveStreams()); } @@ -2830,7 +2849,7 @@ TEST_F(WebRtcVideoChannel2Test, GetStatsTracksAdaptationStats) { channel_->SetSource(kSsrcs3[0], &video_capturer_vga); EXPECT_TRUE(video_capturer_vga.CaptureFrame()); - cricket::VideoCodec send_codec(100, "VP8", 640, 480, 30, 0); + cricket::VideoCodec send_codec(100, "VP8", 640, 480, 30); cricket::VideoSendParameters parameters; parameters.codecs.push_back(send_codec); EXPECT_TRUE(channel_->SetSendParameters(parameters)); @@ -2903,7 +2922,7 @@ TEST_F(WebRtcVideoChannel2Test, GetStatsTracksAdaptationAndBandwidthStats) { channel_->SetSource(kSsrcs3[0], &video_capturer_vga); EXPECT_TRUE(video_capturer_vga.CaptureFrame()); - cricket::VideoCodec send_codec(100, "VP8", 640, 480, 30, 0); + cricket::VideoCodec send_codec(100, "VP8", 640, 480, 30); cricket::VideoSendParameters parameters; parameters.codecs.push_back(send_codec); EXPECT_TRUE(channel_->SetSendParameters(parameters)); diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc index cef7cdf2be..46119311bc 100644 --- a/webrtc/media/engine/webrtcvoiceengine.cc +++ b/webrtc/media/engine/webrtcvoiceengine.cc @@ -250,31 +250,28 @@ class WebRtcVoiceCodecs final { // list and add a test which verifies VoE supports the listed codecs. static std::vector SupportedCodecs() { std::vector result; - for (webrtc::CodecInst voe_codec : webrtc::acm2::RentACodec::Database()) { - // Change the sample rate of G722 to 8000 to match SDP. - MaybeFixupG722(&voe_codec, 8000); - // Skip uncompressed formats. - if (IsCodec(voe_codec, kL16CodecName)) { - continue; - } - - const CodecPref* pref = NULL; - for (size_t j = 0; j < arraysize(kCodecPrefs); ++j) { - if (IsCodec(voe_codec, kCodecPrefs[j].name) && - kCodecPrefs[j].clockrate == voe_codec.plfreq && - kCodecPrefs[j].channels == voe_codec.channels) { - pref = &kCodecPrefs[j]; - break; + // Iterate first over our preferred codecs list, so that the results are + // added in order of preference. + for (size_t i = 0; i < arraysize(kCodecPrefs); ++i) { + const CodecPref* pref = &kCodecPrefs[i]; + for (webrtc::CodecInst voe_codec : webrtc::acm2::RentACodec::Database()) { + // Change the sample rate of G722 to 8000 to match SDP. + MaybeFixupG722(&voe_codec, 8000); + // Skip uncompressed formats. + if (IsCodec(voe_codec, kL16CodecName)) { + continue; } - } - if (pref) { - // Use the payload type that we've configured in our pref table; - // use the offset in our pref table to determine the sort order. - AudioCodec codec( - pref->payload_type, voe_codec.plname, voe_codec.plfreq, - voe_codec.rate, voe_codec.channels, - static_cast(arraysize(kCodecPrefs)) - (pref - kCodecPrefs)); + if (!IsCodec(voe_codec, pref->name) || + pref->clockrate != voe_codec.plfreq || + pref->channels != voe_codec.channels) { + // Not a match. + continue; + } + + AudioCodec codec(pref->payload_type, voe_codec.plname, voe_codec.plfreq, + voe_codec.rate, voe_codec.channels); + LOG(LS_INFO) << "Adding supported codec: " << ToString(codec); if (IsCodec(codec, kIsacCodecName)) { // Indicate auto-bitrate in signaling. codec.bitrate = 0; @@ -297,12 +294,8 @@ class WebRtcVoiceCodecs final { // when they can be set to values other than the default. } result.push_back(codec); - } else { - LOG(LS_INFO) << "[Unused] " << ToString(voe_codec); } } - // Make sure they are in local preference order. - std::sort(result.begin(), result.end(), &AudioCodec::Preferable); return result; } @@ -312,7 +305,7 @@ class WebRtcVoiceCodecs final { // Change the sample rate of G722 to 8000 to match SDP. MaybeFixupG722(&voe_codec, 8000); AudioCodec codec(voe_codec.pltype, voe_codec.plname, voe_codec.plfreq, - voe_codec.rate, voe_codec.channels, 0); + voe_codec.rate, voe_codec.channels); bool multi_rate = IsCodecMultiRate(voe_codec); // Allow arbitrary rates for ISAC to be specified. if (multi_rate) { diff --git a/webrtc/media/engine/webrtcvoiceengine_unittest.cc b/webrtc/media/engine/webrtcvoiceengine_unittest.cc index 1ed493b6d0..e17aea34b8 100644 --- a/webrtc/media/engine/webrtcvoiceengine_unittest.cc +++ b/webrtc/media/engine/webrtcvoiceengine_unittest.cc @@ -33,16 +33,19 @@ using testing::StrictMock; namespace { -const cricket::AudioCodec kPcmuCodec(0, "PCMU", 8000, 64000, 1, 0); -const cricket::AudioCodec kIsacCodec(103, "ISAC", 16000, 32000, 1, 0); -const cricket::AudioCodec kOpusCodec(111, "opus", 48000, 64000, 2, 0); -const cricket::AudioCodec kG722CodecVoE(9, "G722", 16000, 64000, 1, 0); -const cricket::AudioCodec kG722CodecSdp(9, "G722", 8000, 64000, 1, 0); -const cricket::AudioCodec kRedCodec(117, "red", 8000, 0, 1, 0); -const cricket::AudioCodec kCn8000Codec(13, "CN", 8000, 0, 1, 0); -const cricket::AudioCodec kCn16000Codec(105, "CN", 16000, 0, 1, 0); -const cricket::AudioCodec kTelephoneEventCodec(106, "telephone-event", 8000, 0, - 1, 0); +const cricket::AudioCodec kPcmuCodec(0, "PCMU", 8000, 64000, 1); +const cricket::AudioCodec kIsacCodec(103, "ISAC", 16000, 32000, 1); +const cricket::AudioCodec kOpusCodec(111, "opus", 48000, 64000, 2); +const cricket::AudioCodec kG722CodecVoE(9, "G722", 16000, 64000, 1); +const cricket::AudioCodec kG722CodecSdp(9, "G722", 8000, 64000, 1); +const cricket::AudioCodec kRedCodec(117, "red", 8000, 0, 1); +const cricket::AudioCodec kCn8000Codec(13, "CN", 8000, 0, 1); +const cricket::AudioCodec kCn16000Codec(105, "CN", 16000, 0, 1); +const cricket::AudioCodec kTelephoneEventCodec(106, + "telephone-event", + 8000, + 0, + 1); const uint32_t kSsrc1 = 0x99; const uint32_t kSsrc2 = 2; const uint32_t kSsrc3 = 3; @@ -483,19 +486,14 @@ TEST_F(WebRtcVoiceEngineTestFake, CreateChannel) { } // Tests that the list of supported codecs is created properly and ordered -// correctly -TEST_F(WebRtcVoiceEngineTestFake, CodecPreference) { +// correctly (such that opus appears first). +TEST_F(WebRtcVoiceEngineTestFake, CodecOrder) { const std::vector& codecs = engine_->codecs(); ASSERT_FALSE(codecs.empty()); EXPECT_STRCASEEQ("opus", codecs[0].name.c_str()); EXPECT_EQ(48000, codecs[0].clockrate); EXPECT_EQ(2, codecs[0].channels); EXPECT_EQ(64000, codecs[0].bitrate); - int pref = codecs[0].preference; - for (size_t i = 1; i < codecs.size(); ++i) { - EXPECT_GT(pref, codecs[i].preference); - pref = codecs[i].preference; - } } TEST_F(WebRtcVoiceEngineTestFake, OpusSupportsTransportCc) { @@ -579,7 +577,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetRecvCodecsUnsupportedCodec) { EXPECT_TRUE(SetupChannel()); cricket::AudioRecvParameters parameters; parameters.codecs.push_back(kIsacCodec); - parameters.codecs.push_back(cricket::AudioCodec(127, "XYZ", 32000, 0, 1, 0)); + parameters.codecs.push_back(cricket::AudioCodec(127, "XYZ", 32000, 0, 1)); EXPECT_FALSE(channel_->SetRecvParameters(parameters)); } @@ -3410,55 +3408,55 @@ TEST(WebRtcVoiceEngineTest, StartupShutdownWithExternalADM) { TEST(WebRtcVoiceEngineTest, HasCorrectCodecs) { // Check codecs by name. EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "OPUS", 48000, 0, 2, 0), nullptr)); + cricket::AudioCodec(96, "OPUS", 48000, 0, 2), nullptr)); EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "ISAC", 16000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "ISAC", 16000, 0, 1), nullptr)); EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "ISAC", 32000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "ISAC", 32000, 0, 1), nullptr)); // Check that name matching is case-insensitive. EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "ILBC", 8000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "ILBC", 8000, 0, 1), nullptr)); EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "iLBC", 8000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "iLBC", 8000, 0, 1), nullptr)); EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "PCMU", 8000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "PCMU", 8000, 0, 1), nullptr)); EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "PCMA", 8000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "PCMA", 8000, 0, 1), nullptr)); EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "G722", 8000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "G722", 8000, 0, 1), nullptr)); EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "red", 8000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "red", 8000, 0, 1), nullptr)); EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "CN", 32000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "CN", 32000, 0, 1), nullptr)); EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "CN", 16000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "CN", 16000, 0, 1), nullptr)); EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "CN", 8000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "CN", 8000, 0, 1), nullptr)); EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(96, "telephone-event", 8000, 0, 1, 0), nullptr)); + cricket::AudioCodec(96, "telephone-event", 8000, 0, 1), nullptr)); // Check codecs with an id by id. EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(0, "", 8000, 0, 1, 0), nullptr)); // PCMU + cricket::AudioCodec(0, "", 8000, 0, 1), nullptr)); // PCMU EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(8, "", 8000, 0, 1, 0), nullptr)); // PCMA + cricket::AudioCodec(8, "", 8000, 0, 1), nullptr)); // PCMA EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(9, "", 8000, 0, 1, 0), nullptr)); // G722 + cricket::AudioCodec(9, "", 8000, 0, 1), nullptr)); // G722 EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(13, "", 8000, 0, 1, 0), nullptr)); // CN + cricket::AudioCodec(13, "", 8000, 0, 1), nullptr)); // CN // Check sample/bitrate matching. EXPECT_TRUE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(0, "PCMU", 8000, 64000, 1, 0), nullptr)); + cricket::AudioCodec(0, "PCMU", 8000, 64000, 1), nullptr)); // Check that bad codecs fail. EXPECT_FALSE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(99, "ABCD", 0, 0, 1, 0), nullptr)); + cricket::AudioCodec(99, "ABCD", 0, 0, 1), nullptr)); EXPECT_FALSE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(88, "", 0, 0, 1, 0), nullptr)); + cricket::AudioCodec(88, "", 0, 0, 1), nullptr)); EXPECT_FALSE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(0, "", 0, 0, 2, 0), nullptr)); + cricket::AudioCodec(0, "", 0, 0, 2), nullptr)); EXPECT_FALSE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(0, "", 5000, 0, 1, 0), nullptr)); + cricket::AudioCodec(0, "", 5000, 0, 1), nullptr)); EXPECT_FALSE(cricket::WebRtcVoiceEngine::ToCodecInst( - cricket::AudioCodec(0, "", 0, 5000, 1, 0), nullptr)); + cricket::AudioCodec(0, "", 0, 5000, 1), nullptr)); // Verify the payload id of common audio codecs, including CN, ISAC, and G722. cricket::WebRtcVoiceEngine engine(nullptr); diff --git a/webrtc/media/sctp/sctpdataengine.cc b/webrtc/media/sctp/sctpdataengine.cc index ba3b8f2408..b7462de879 100644 --- a/webrtc/media/sctp/sctpdataengine.cc +++ b/webrtc/media/sctp/sctpdataengine.cc @@ -283,7 +283,7 @@ SctpDataEngine::SctpDataEngine() { } usrsctp_engines_count++; - cricket::DataCodec codec(kGoogleSctpDataCodecId, kGoogleSctpDataCodecName, 0); + cricket::DataCodec codec(kGoogleSctpDataCodecId, kGoogleSctpDataCodecName); codec.SetParam(kCodecParamPort, kSctpDefaultPort); codecs_.push_back(codec); } diff --git a/webrtc/pc/channel_unittest.cc b/webrtc/pc/channel_unittest.cc index d84076ec2d..5993a69f78 100644 --- a/webrtc/pc/channel_unittest.cc +++ b/webrtc/pc/channel_unittest.cc @@ -48,12 +48,12 @@ using cricket::StreamParams; using cricket::TransportChannel; using rtc::WindowId; -static const cricket::AudioCodec kPcmuCodec(0, "PCMU", 64000, 8000, 1, 0); -static const cricket::AudioCodec kPcmaCodec(8, "PCMA", 64000, 8000, 1, 0); -static const cricket::AudioCodec kIsacCodec(103, "ISAC", 40000, 16000, 1, 0); -static const cricket::VideoCodec kH264Codec(97, "H264", 640, 400, 30, 0); -static const cricket::VideoCodec kH264SvcCodec(99, "H264-SVC", 320, 200, 15, 0); -static const cricket::DataCodec kGoogleDataCodec(101, "google-data", 0); +static const cricket::AudioCodec kPcmuCodec(0, "PCMU", 64000, 8000, 1); +static const cricket::AudioCodec kPcmaCodec(8, "PCMA", 64000, 8000, 1); +static const cricket::AudioCodec kIsacCodec(103, "ISAC", 40000, 16000, 1); +static const cricket::VideoCodec kH264Codec(97, "H264", 640, 400, 30); +static const cricket::VideoCodec kH264SvcCodec(99, "H264-SVC", 320, 200, 15); +static const cricket::DataCodec kGoogleDataCodec(101, "google-data"); static const uint32_t kSsrc1 = 0x1111; static const uint32_t kSsrc2 = 0x2222; static const uint32_t kSsrc3 = 0x3333; diff --git a/webrtc/pc/channelmanager_unittest.cc b/webrtc/pc/channelmanager_unittest.cc index 1eb09198d9..8cb066b25d 100644 --- a/webrtc/pc/channelmanager_unittest.cc +++ b/webrtc/pc/channelmanager_unittest.cc @@ -22,14 +22,12 @@ namespace cricket { static const AudioCodec kAudioCodecs[] = { - AudioCodec(97, "voice", 1, 2, 3, 0), - AudioCodec(111, "OPUS", 48000, 32000, 2, 0), + AudioCodec(97, "voice", 1, 2, 3), AudioCodec(111, "OPUS", 48000, 32000, 2), }; static const VideoCodec kVideoCodecs[] = { - VideoCodec(99, "H264", 100, 200, 300, 0), - VideoCodec(100, "VP8", 100, 200, 300, 0), - VideoCodec(96, "rtx", 100, 200, 300, 0), + VideoCodec(99, "H264", 100, 200, 300), + VideoCodec(100, "VP8", 100, 200, 300), VideoCodec(96, "rtx", 100, 200, 300), }; class ChannelManagerTest : public testing::Test { @@ -198,7 +196,7 @@ TEST_F(ChannelManagerTest, GetSetOutputVolume) { TEST_F(ChannelManagerTest, SetVideoRtxEnabled) { std::vector codecs; - const VideoCodec rtx_codec(96, "rtx", 0, 0, 0, 0); + const VideoCodec rtx_codec(96, "rtx", 0, 0, 0); // By default RTX is disabled. cm_->GetSupportedVideoCodecs(&codecs); diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc index 382cdbc44f..f5995010b3 100644 --- a/webrtc/pc/mediasession.cc +++ b/webrtc/pc/mediasession.cc @@ -10,11 +10,12 @@ #include "webrtc/pc/mediasession.h" -#include // For std::find_if. +#include // For std::find_if, std::sort. #include #include #include #include +#include #include #include "webrtc/base/helpers.h" @@ -756,7 +757,6 @@ static bool CreateMediaContentOffer( StreamParamsVec* current_streams, MediaContentDescriptionImpl* offer) { offer->AddCodecs(codecs); - offer->SortCodecs(); if (secure_policy == SEC_REQUIRED) { offer->set_crypto_required(CT_SDES); @@ -817,6 +817,8 @@ static void NegotiateCodecs(const std::vector& local_codecs, std::vector* negotiated_codecs) { for (const C& ours : local_codecs) { C theirs; + // Note that we intentionally only find one matching codec for each of our + // local codecs, in case the remote offer contains duplicate codecs. if (FindMatchingCodec(local_codecs, offered_codecs, ours, &theirs)) { C negotiated = ours; negotiated.IntersectFeedbackParams(theirs); @@ -829,14 +831,23 @@ static void NegotiateCodecs(const std::vector& local_codecs, 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); } } + // 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. + std::unordered_map payload_type_preferences; + int preference = static_cast(offered_codecs.size() + 1); + for (const C& codec : offered_codecs) { + payload_type_preferences[codec.id] = preference--; + } + std::sort(negotiated_codecs->begin(), negotiated_codecs->end(), + [&payload_type_preferences](const C& a, const C& b) { + return payload_type_preferences[a.id] > + payload_type_preferences[b.id]; + }); } // Finds a codec in |codecs2| that matches |codec_to_match|, which is @@ -1049,7 +1060,6 @@ static bool CreateMediaContentAnswer( std::vector negotiated_codecs; NegotiateCodecs(local_codecs, offer->codecs(), &negotiated_codecs); answer->AddCodecs(negotiated_codecs); - answer->SortCodecs(); answer->set_protocol(offer->protocol()); RtpHeaderExtensions negotiated_rtp_extensions; NegotiateRtpHeaderExtensions(local_rtp_extenstions, @@ -1377,8 +1387,8 @@ SessionDescription* MediaSessionDescriptionFactory::CreateAnswer( const SessionDescription* offer, const MediaSessionOptions& options, const SessionDescription* current_description) const { // The answer contains the intersection of the codecs in the offer with the - // codecs we support, ordered by our local preference. As indicated by - // XEP-0167, we retain the same payload ids from the offer in the answer. + // 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()); StreamParamsVec current_streams; diff --git a/webrtc/pc/mediasession.h b/webrtc/pc/mediasession.h index ae221554e2..98a1f070d6 100644 --- a/webrtc/pc/mediasession.h +++ b/webrtc/pc/mediasession.h @@ -293,10 +293,9 @@ class MediaContentDescription : public ContentDescription { template class MediaContentDescriptionImpl : public MediaContentDescription { public: - struct PreferenceSort { - bool operator()(C a, C b) { return a.preference > b.preference; } - }; + typedef C CodecType; + // Codecs should be in preference order (most preferred codec first). const std::vector& codecs() const { return codecs_; } void set_codecs(const std::vector& codecs) { codecs_ = codecs; } virtual bool has_codecs() const { return !codecs_.empty(); } @@ -330,9 +329,6 @@ class MediaContentDescriptionImpl : public MediaContentDescription { AddCodec(*codec); } } - void SortCodecs() { - std::sort(codecs_.begin(), codecs_.end(), PreferenceSort()); - } private: std::vector codecs_; diff --git a/webrtc/pc/mediasession_unittest.cc b/webrtc/pc/mediasession_unittest.cc index 1b2aa4eb3b..4c3eb4f01e 100644 --- a/webrtc/pc/mediasession_unittest.cc +++ b/webrtc/pc/mediasession_unittest.cc @@ -75,53 +75,43 @@ using rtc::CS_AES_CM_128_HMAC_SHA1_32; using rtc::CS_AES_CM_128_HMAC_SHA1_80; static const AudioCodec kAudioCodecs1[] = { - AudioCodec(103, "ISAC", 16000, -1, 1, 6), - AudioCodec(102, "iLBC", 8000, 13300, 1, 5), - AudioCodec(0, "PCMU", 8000, 64000, 1, 4), - AudioCodec(8, "PCMA", 8000, 64000, 1, 3), - AudioCodec(117, "red", 8000, 0, 1, 2), - AudioCodec(107, "CN", 48000, 0, 1, 1) -}; + AudioCodec(103, "ISAC", 16000, -1, 1), + AudioCodec(102, "iLBC", 8000, 13300, 1), + AudioCodec(0, "PCMU", 8000, 64000, 1), + AudioCodec(8, "PCMA", 8000, 64000, 1), + AudioCodec(117, "red", 8000, 0, 1), + AudioCodec(107, "CN", 48000, 0, 1)}; static const AudioCodec kAudioCodecs2[] = { - AudioCodec(126, "speex", 16000, 22000, 1, 3), - AudioCodec(0, "PCMU", 8000, 64000, 1, 2), - AudioCodec(127, "iLBC", 8000, 13300, 1, 1), + AudioCodec(126, "speex", 16000, 22000, 1), + AudioCodec(0, "PCMU", 8000, 64000, 1), + AudioCodec(127, "iLBC", 8000, 13300, 1), }; static const AudioCodec kAudioCodecsAnswer[] = { - AudioCodec(102, "iLBC", 8000, 13300, 1, 5), - AudioCodec(0, "PCMU", 8000, 64000, 1, 4), + AudioCodec(102, "iLBC", 8000, 13300, 1), + AudioCodec(0, "PCMU", 8000, 64000, 1), }; static const VideoCodec kVideoCodecs1[] = { - VideoCodec(96, "H264-SVC", 320, 200, 30, 2), - VideoCodec(97, "H264", 320, 200, 30, 1) -}; + VideoCodec(96, "H264-SVC", 320, 200, 30), + VideoCodec(97, "H264", 320, 200, 30)}; static const VideoCodec kVideoCodecs2[] = { - VideoCodec(126, "H264", 320, 200, 30, 2), - VideoCodec(127, "H263", 320, 200, 30, 1) -}; + VideoCodec(126, "H264", 320, 200, 30), + VideoCodec(127, "H263", 320, 200, 30)}; static const VideoCodec kVideoCodecsAnswer[] = { - VideoCodec(97, "H264", 320, 200, 30, 1) -}; + VideoCodec(97, "H264", 320, 200, 30)}; -static const DataCodec kDataCodecs1[] = { - DataCodec(98, "binary-data", 2), - DataCodec(99, "utf8-text", 1) -}; +static const DataCodec kDataCodecs1[] = {DataCodec(98, "binary-data"), + DataCodec(99, "utf8-text")}; -static const DataCodec kDataCodecs2[] = { - DataCodec(126, "binary-data", 2), - DataCodec(127, "utf8-text", 1) -}; +static const DataCodec kDataCodecs2[] = {DataCodec(126, "binary-data"), + DataCodec(127, "utf8-text")}; -static const DataCodec kDataCodecsAnswer[] = { - DataCodec(98, "binary-data", 2), - DataCodec(99, "utf8-text", 1) -}; +static const DataCodec kDataCodecsAnswer[] = {DataCodec(98, "binary-data"), + DataCodec(99, "utf8-text")}; static const RtpHeaderExtension kAudioRtpExtension1[] = { RtpHeaderExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level", 8), @@ -1576,11 +1566,9 @@ TEST_F(MediaSessionDescriptionFactoryTest, EXPECT_EQ(expected_codecs, vcd->codecs()); - // Now, make sure we get same result, except for the preference order, - // if |f2_| creates an updated offer even though the default payload types - // are different from |f1_|. - expected_codecs[0].preference = f1_codecs[1].preference; - + // Now, make sure we get same result (except for the order) if |f2_| creates + // an updated offer even though the default payload types between |f1_| and + // |f2_| are different. std::unique_ptr updated_offer( f2_.CreateOffer(opts, answer.get())); ASSERT_TRUE(updated_offer); @@ -1700,7 +1688,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, RtxWithoutApt) { opts.recv_audio = false; std::vector f1_codecs = MAKE_VECTOR(kVideoCodecs1); // This creates RTX without associated payload type parameter. - AddRtxCodec(VideoCodec(126, cricket::kRtxCodecName, 0, 0, 0, 0), &f1_codecs); + AddRtxCodec(VideoCodec(126, cricket::kRtxCodecName, 0, 0, 0), &f1_codecs); f1_.set_video_codecs(f1_codecs); std::vector f2_codecs = MAKE_VECTOR(kVideoCodecs2); @@ -1848,7 +1836,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, SimSsrcsGenerateMultipleRtxSsrcs) { // Use a single real codec, and then add RTX for it. std::vector f1_codecs; - f1_codecs.push_back(VideoCodec(97, "H264", 320, 200, 30, 1)); + f1_codecs.push_back(VideoCodec(97, "H264", 320, 200, 30)); AddRtxCodec(VideoCodec::CreateRtxCodec(125, 97), &f1_codecs); f1_.set_video_codecs(f1_codecs);