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}
diff --git a/webrtc/api/jsepsessiondescription.cc b/webrtc/api/jsepsessiondescription.cc
index eb776c8..ee0a8e1 100644
--- a/webrtc/api/jsepsessiondescription.cc
+++ b/webrtc/api/jsepsessiondescription.cc
@@ -57,7 +57,6 @@
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 9a0d873..ca39b8a 100644
--- a/webrtc/api/jsepsessiondescription.h
+++ b/webrtc/api/jsepsessiondescription.h
@@ -72,7 +72,6 @@
static const char kDefaultVideoCodecName[];
static const int kMaxVideoCodecWidth;
static const int kMaxVideoCodecHeight;
- static const int kDefaultVideoCodecPreference;
private:
rtc::scoped_ptr<cricket::SessionDescription> description_;
diff --git a/webrtc/api/jsepsessiondescription_unittest.cc b/webrtc/api/jsepsessiondescription_unittest.cc
index 3d87513..8f9fc54 100644
--- a/webrtc/api/jsepsessiondescription_unittest.cc
+++ b/webrtc/api/jsepsessiondescription_unittest.cc
@@ -48,11 +48,11 @@
scoped_ptr<cricket::VideoContentDescription> 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 90e0007..8497f4c 100644
--- a/webrtc/api/webrtcsdp.cc
+++ b/webrtc/api/webrtcsdp.cc
@@ -15,6 +15,7 @@
#include <stdio.h>
#include <algorithm>
#include <string>
+#include <unordered_map>
#include <vector>
#include "webrtc/api/jsepicecandidate.h"
@@ -269,7 +270,7 @@
const MediaType media_type,
int mline_index,
const std::string& protocol,
- const std::vector<int>& codec_preference,
+ const std::vector<int>& payload_types,
size_t* pos,
std::string* content_name,
MediaContentDescription* media_desc,
@@ -287,7 +288,7 @@
SdpParseError* error);
static bool ParseRtpmapAttribute(const std::string& line,
const MediaType media_type,
- const std::vector<int>& codec_preference,
+ const std::vector<int>& payload_types,
MediaContentDescription* media_desc,
SdpParseError* error);
static bool ParseFmtpAttributes(const std::string& line,
@@ -1651,9 +1652,8 @@
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 @@
if (!media_desc) {
return;
}
- int preference = static_cast<int>(fmts.size());
+ RTC_DCHECK(media_desc->codecs().empty());
std::vector<int>::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 @@
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 @@
const MediaType media_type,
int mline_index,
const std::string& protocol,
- const std::vector<int>& codec_preference,
+ const std::vector<int>& payload_types,
size_t* pos,
std::string* content_name,
TransportDescription* transport,
@@ -2228,14 +2221,28 @@
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<int, int> 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<int>(payload_types.size() + 1);
+ for (int pt : payload_types) {
+ payload_type_preferences[pt] = preference--;
+ }
+ std::vector<typename C::CodecType> 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 @@
std::string protocol = fields[2];
// <fmt>
- std::vector<int> codec_preference;
+ std::vector<int> 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 @@
if (!GetPayloadTypeFromString(line, fields[j], &pl, error)) {
return false;
}
- codec_preference.push_back(pl);
+ payload_types.push_back(pl);
}
}
@@ -2302,20 +2309,17 @@
std::string content_name;
if (HasAttribute(line, kMediaTypeVideo)) {
content.reset(ParseContentDescription<VideoContentDescription>(
- 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<AudioContentDescription>(
- 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<DataContentDescription>(
- 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 @@
const MediaType media_type,
int mline_index,
const std::string& protocol,
- const std::vector<int>& codec_preference,
+ const std::vector<int>& payload_types,
size_t* pos,
std::string* content_name,
MediaContentDescription* media_desc,
@@ -2535,7 +2539,7 @@
if (media_type == cricket::MEDIA_TYPE_AUDIO) {
MaybeCreateStaticPayloadAudioCodecs(
- codec_preference, static_cast<AudioContentDescription*>(media_desc));
+ payload_types, static_cast<AudioContentDescription*>(media_desc));
}
// The media level "ice-ufrag" and "ice-pwd".
@@ -2670,8 +2674,8 @@
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 @@
}
// 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 @@
codec.clockrate = clockrate;
codec.bitrate = bitrate;
codec.channels = channels;
- codec.preference = preference;
AddOrReplaceCodec<AudioContentDescription, cricket::AudioCodec>(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 @@
codec.width = width;
codec.height = height;
codec.framerate = framerate;
- codec.preference = preference;
AddOrReplaceCodec<VideoContentDescription, cricket::VideoCodec>(video_desc,
codec);
}
bool ParseRtpmapAttribute(const std::string& line,
const MediaType media_type,
- const std::vector<int>& codec_preference,
+ const std::vector<int>& payload_types,
MediaContentDescription* media_desc,
SdpParseError* error) {
std::vector<std::string> fields;
@@ -2986,12 +2994,8 @@
return false;
}
- // Set the preference order depending on the order of the pl type in the
- // <fmt> 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 "
<< "<fmt> of the m-line: " << line;
return true;
@@ -3021,7 +3025,7 @@
JsepSessionDescription::kMaxVideoCodecWidth,
JsepSessionDescription::kMaxVideoCodecHeight,
JsepSessionDescription::kDefaultVideoCodecFramerate,
- preference, video_desc);
+ video_desc);
} else if (media_type == cricket::MEDIA_TYPE_AUDIO) {
// RFC 4566
// For audio streams, <encoding parameters> indicates the number
@@ -3049,12 +3053,11 @@
AudioContentDescription* audio_desc =
static_cast<AudioContentDescription*>(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<DataContentDescription*>(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 3734ac5..2b63c79 100644
--- a/webrtc/api/webrtcsdp_unittest.cc
+++ b/webrtc/api/webrtcsdp_unittest.cc
@@ -1030,10 +1030,10 @@
"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 @@
VideoCodec(120, JsepSessionDescription::kDefaultVideoCodecName,
JsepSessionDescription::kMaxVideoCodecWidth,
JsepSessionDescription::kMaxVideoCodecHeight,
- JsepSessionDescription::kDefaultVideoCodecFramerate,
- JsepSessionDescription::kDefaultVideoCodecPreference));
+ JsepSessionDescription::kDefaultVideoCodecFramerate));
return video;
}
@@ -1400,7 +1399,7 @@
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 @@
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 @@
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 @@
static_cast<AudioContentDescription*>(
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 (<fmt>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 28f93df..feb1518 100644
--- a/webrtc/api/webrtcsession_unittest.cc
+++ b/webrtc/api/webrtcsession_unittest.cc
@@ -450,8 +450,8 @@
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<cricket::AudioCodec> codecs;
codecs.push_back(kTelephoneEventCodec);
media_engine_->SetAudioCodecs(codecs);
@@ -1313,8 +1313,8 @@
// 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<cricket::AudioCodec> codecs = media_engine_->audio_codecs();;