Move ulpfec, red, and flexfec codec to video engine
These codecs are currently being added in the internal encoder factory.
This means that the new injectable video codec factories will miss them.
This CL moves adding them into the video engine so that both factory
types will get them.
This CL makes a functional change in that RED, ULPFEC, and FlexFec will
be placed after both the internal and external codecs. Previously,
it was placed between the internal and external codecs.
Bug: webrtc:8527
Change-Id: I5aa7a3ca674f621b17cf3aa095a225c753488e09
Reviewed-on: https://webrtc-review.googlesource.com/22964
Commit-Queue: Magnus Jedvert <magjed@webrtc.org>
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20700}diff --git a/media/engine/internalencoderfactory.cc b/media/engine/internalencoderfactory.cc
index fa6783f6..21640df 100644
--- a/media/engine/internalencoderfactory.cc
+++ b/media/engine/internalencoderfactory.cc
@@ -15,24 +15,9 @@
#include "modules/video_coding/codecs/h264/include/h264.h"
#include "modules/video_coding/codecs/vp8/include/vp8.h"
#include "modules/video_coding/codecs/vp9/include/vp9.h"
-#include "system_wrappers/include/field_trial.h"
namespace cricket {
-namespace {
-
-// If this field trial is enabled, the "flexfec-03" codec will be advertised
-// as being supported by the InternalEncoderFactory. This means that
-// "flexfec-03" will appear in the default SDP offer, and we therefore need to
-// be ready to receive FlexFEC packets from the remote. It also means that
-// FlexFEC SSRCs will be generated by MediaSession and added as "a=ssrc:" and
-// "a=ssrc-group:" lines in the local SDP.
-bool IsFlexfecAdvertisedFieldTrialEnabled() {
- return webrtc::field_trial::IsEnabled("WebRTC-FlexFEC-03-Advertised");
-}
-
-} // namespace
-
InternalEncoderFactory::InternalEncoderFactory() {
supported_codecs_.push_back(VideoCodec(kVp8CodecName));
if (webrtc::VP9Encoder::IsSupported())
@@ -40,19 +25,6 @@
for (const webrtc::SdpVideoFormat& format : webrtc::SupportedH264Codecs())
supported_codecs_.push_back(VideoCodec(format));
-
- supported_codecs_.push_back(VideoCodec(kRedCodecName));
- supported_codecs_.push_back(VideoCodec(kUlpfecCodecName));
-
- if (IsFlexfecAdvertisedFieldTrialEnabled()) {
- VideoCodec flexfec_codec(kFlexfecCodecName);
- // This value is currently arbitrarily set to 10 seconds. (The unit
- // is microseconds.) This parameter MUST be present in the SDP, but
- // we never use the actual value anywhere in our code however.
- // TODO(brandtr): Consider honouring this value in the sender and receiver.
- flexfec_codec.SetParam(kFlexfecFmtpRepairWindow, "10000000");
- supported_codecs_.push_back(flexfec_codec);
- }
}
InternalEncoderFactory::~InternalEncoderFactory() {}
diff --git a/media/engine/webrtcvideoengine.cc b/media/engine/webrtcvideoengine.cc
index d2457f8..88d25b5 100644
--- a/media/engine/webrtcvideoengine.cc
+++ b/media/engine/webrtcvideoengine.cc
@@ -155,25 +155,18 @@
const char* ImplementationName() const override { return "NullVideoDecoder"; }
};
-std::vector<VideoCodec> AssignPayloadTypesAndAddAssociatedRtxCodecs(
- const std::vector<webrtc::SdpVideoFormat>& input_formats);
-
-std::vector<VideoCodec> AssignPayloadTypesAndAddAssociatedRtxCodecs(
- const webrtc::VideoEncoderFactory* encoder_factory) {
- return encoder_factory ? AssignPayloadTypesAndAddAssociatedRtxCodecs(
- encoder_factory->GetSupportedFormats())
- : std::vector<VideoCodec>();
-}
-
// If this field trial is enabled, we will enable sending FlexFEC and disable
// sending ULPFEC whenever the former has been negotiated in the SDPs.
bool IsFlexfecFieldTrialEnabled() {
return webrtc::field_trial::IsEnabled("WebRTC-FlexFEC-03");
}
-// If this field trial is enabled, the "flexfec-03" codec may have been
-// advertised as being supported in the local SDP. That means that we must be
-// ready to receive FlexFEC packets. See internalencoderfactory.cc.
+// If this field trial is enabled, the "flexfec-03" codec will be advertised
+// as being supported. This means that "flexfec-03" will appear in the default
+// SDP offer, and we therefore need to be ready to receive FlexFEC packets from
+// the remote. It also means that FlexFEC SSRCs will be generated by
+// MediaSession and added as "a=ssrc:" and "a=ssrc-group:" lines in the local
+// SDP.
bool IsFlexfecAdvertisedFieldTrialEnabled() {
return webrtc::field_trial::IsEnabled("WebRTC-FlexFEC-03-Advertised");
}
@@ -193,6 +186,70 @@
codec->AddFeedbackParam(FeedbackParam(kRtcpFbParamNack, kRtcpFbNackParamPli));
}
+
+// This function will assign dynamic payload types (in the range [96, 127]) to
+// the input codecs, and also add ULPFEC, RED, FlexFEC, and associated RTX
+// codecs for recognized codecs (VP8, VP9, H264, and RED). It will also add
+// default feedback params to the codecs.
+std::vector<VideoCodec> AssignPayloadTypesAndDefaultCodecs(
+ std::vector<webrtc::SdpVideoFormat> input_formats) {
+ if (input_formats.empty())
+ return std::vector<VideoCodec>();
+ static const int kFirstDynamicPayloadType = 96;
+ static const int kLastDynamicPayloadType = 127;
+ int payload_type = kFirstDynamicPayloadType;
+
+ input_formats.push_back(webrtc::SdpVideoFormat(kRedCodecName));
+ input_formats.push_back(webrtc::SdpVideoFormat(kUlpfecCodecName));
+
+ if (IsFlexfecAdvertisedFieldTrialEnabled()) {
+ webrtc::SdpVideoFormat flexfec_format(kFlexfecCodecName);
+ // This value is currently arbitrarily set to 10 seconds. (The unit
+ // is microseconds.) This parameter MUST be present in the SDP, but
+ // we never use the actual value anywhere in our code however.
+ // TODO(brandtr): Consider honouring this value in the sender and receiver.
+ flexfec_format.parameters = {{kFlexfecFmtpRepairWindow, "10000000"}};
+ input_formats.push_back(flexfec_format);
+ }
+
+ std::vector<VideoCodec> output_codecs;
+ for (const webrtc::SdpVideoFormat& format : input_formats) {
+ VideoCodec codec(format);
+ codec.id = payload_type;
+ AddDefaultFeedbackParams(&codec);
+ output_codecs.push_back(codec);
+
+ // Increment payload type.
+ ++payload_type;
+ if (payload_type > kLastDynamicPayloadType)
+ break;
+
+ // Add associated RTX codec for recognized codecs.
+ // TODO(deadbeef): Should we add RTX codecs for external codecs whose names
+ // we don't recognize?
+ if (CodecNamesEq(codec.name, kVp8CodecName) ||
+ CodecNamesEq(codec.name, kVp9CodecName) ||
+ CodecNamesEq(codec.name, kH264CodecName) ||
+ CodecNamesEq(codec.name, kRedCodecName)) {
+ output_codecs.push_back(
+ VideoCodec::CreateRtxCodec(payload_type, codec.id));
+
+ // Increment payload type.
+ ++payload_type;
+ if (payload_type > kLastDynamicPayloadType)
+ break;
+ }
+ }
+ return output_codecs;
+}
+
+std::vector<VideoCodec> AssignPayloadTypesAndDefaultCodecs(
+ const webrtc::VideoEncoderFactory* encoder_factory) {
+ return encoder_factory ? AssignPayloadTypesAndDefaultCodecs(
+ encoder_factory->GetSupportedFormats())
+ : std::vector<VideoCodec>();
+}
+
static std::string CodecVectorToString(const std::vector<VideoCodec>& codecs) {
std::stringstream out;
out << '{';
@@ -497,7 +554,7 @@
}
std::vector<VideoCodec> WebRtcVideoEngine::codecs() const {
- return AssignPayloadTypesAndAddAssociatedRtxCodecs(encoder_factory_.get());
+ return AssignPayloadTypesAndDefaultCodecs(encoder_factory_.get());
}
RtpCapabilities WebRtcVideoEngine::GetCapabilities() const {
@@ -526,48 +583,6 @@
return capabilities;
}
-namespace {
-// This function will assign dynamic payload types (in the range [96, 127]) to
-// the input codecs, and also add associated RTX codecs for recognized codecs
-// (VP8, VP9, H264, and RED). It will also add default feedback params to the
-// codecs.
-std::vector<VideoCodec> AssignPayloadTypesAndAddAssociatedRtxCodecs(
- const std::vector<webrtc::SdpVideoFormat>& input_formats) {
- static const int kFirstDynamicPayloadType = 96;
- static const int kLastDynamicPayloadType = 127;
- int payload_type = kFirstDynamicPayloadType;
- std::vector<VideoCodec> output_codecs;
- for (const webrtc::SdpVideoFormat& format : input_formats) {
- VideoCodec codec(format);
- codec.id = payload_type;
- AddDefaultFeedbackParams(&codec);
- output_codecs.push_back(codec);
-
- // Increment payload type.
- ++payload_type;
- if (payload_type > kLastDynamicPayloadType)
- break;
-
- // Add associated RTX codec for recognized codecs.
- // TODO(deadbeef): Should we add RTX codecs for external codecs whose names
- // we don't recognize?
- if (CodecNamesEq(codec.name, kVp8CodecName) ||
- CodecNamesEq(codec.name, kVp9CodecName) ||
- CodecNamesEq(codec.name, kH264CodecName) ||
- CodecNamesEq(codec.name, kRedCodecName)) {
- output_codecs.push_back(
- VideoCodec::CreateRtxCodec(payload_type, codec.id));
-
- // Increment payload type.
- ++payload_type;
- if (payload_type > kLastDynamicPayloadType)
- break;
- }
- }
- return output_codecs;
-}
-} // namespace
-
WebRtcVideoChannel::WebRtcVideoChannel(
webrtc::Call* call,
const MediaConfig& config,
@@ -587,7 +602,7 @@
rtcp_receiver_report_ssrc_ = kDefaultRtcpReceiverReportSsrc;
sending_ = false;
recv_codecs_ =
- MapCodecs(AssignPayloadTypesAndAddAssociatedRtxCodecs(encoder_factory_));
+ MapCodecs(AssignPayloadTypesAndDefaultCodecs(encoder_factory_));
recv_flexfec_payload_type_ = recv_codecs_.front().flexfec_payload_type;
}
@@ -602,7 +617,7 @@
WebRtcVideoChannel::SelectSendVideoCodec(
const std::vector<VideoCodecSettings>& remote_mapped_codecs) const {
const std::vector<VideoCodec> local_supported_codecs =
- AssignPayloadTypesAndAddAssociatedRtxCodecs(encoder_factory_);
+ AssignPayloadTypesAndDefaultCodecs(encoder_factory_);
// Select the first remote codec that is supported locally.
for (const VideoCodecSettings& remote_mapped_codec : remote_mapped_codecs) {
// For H264, we will limit the encode level to the remote offered level
@@ -919,7 +934,7 @@
// Verify that every mapped codec is supported locally.
const std::vector<VideoCodec> local_supported_codecs =
- AssignPayloadTypesAndAddAssociatedRtxCodecs(encoder_factory_);
+ AssignPayloadTypesAndDefaultCodecs(encoder_factory_);
for (const VideoCodecSettings& mapped_codec : mapped_codecs) {
if (!FindMatchingCodec(local_supported_codecs, mapped_codec.codec)) {
RTC_LOG(LS_ERROR)
diff --git a/media/engine/webrtcvideoengine_unittest.cc b/media/engine/webrtcvideoengine_unittest.cc
index c3542a2..d3a9b9b 100644
--- a/media/engine/webrtcvideoengine_unittest.cc
+++ b/media/engine/webrtcvideoengine_unittest.cc
@@ -181,9 +181,13 @@
}
protected:
+ // Find the index of the codec in the engine with the given name. The codec
+ // must be present.
+ int GetEngineCodecIndex(const std::string& name) const;
+
// Find the codec in the engine with the given name. The codec must be
// present.
- cricket::VideoCodec GetEngineCodec(const std::string& name);
+ cricket::VideoCodec GetEngineCodec(const std::string& name) const;
VideoMediaChannel* SetUpForExternalEncoderFactory();
@@ -565,9 +569,10 @@
EXPECT_TRUE(channel->RemoveSendStream(kSsrc));
}
-cricket::VideoCodec WebRtcVideoEngineTest::GetEngineCodec(
- const std::string& name) {
- for (const cricket::VideoCodec& engine_codec : engine_.codecs()) {
+int WebRtcVideoEngineTest::GetEngineCodecIndex(const std::string& name) const {
+ const std::vector<cricket::VideoCodec> codecs = engine_.codecs();
+ for (size_t i = 0; i < codecs.size(); ++i) {
+ const cricket::VideoCodec engine_codec = codecs[i];
if (!CodecNamesEq(name, engine_codec.name))
continue;
// The tests only use H264 Constrained Baseline. Make sure we don't return
@@ -580,11 +585,16 @@
continue;
}
}
- return engine_codec;
+ return i;
}
// This point should never be reached.
ADD_FAILURE() << "Unrecognized codec name: " << name;
- return cricket::VideoCodec();
+ return -1;
+}
+
+cricket::VideoCodec WebRtcVideoEngineTest::GetEngineCodec(
+ const std::string& name) const {
+ return engine_.codecs()[GetEngineCodecIndex(name)];
}
VideoMediaChannel* WebRtcVideoEngineTest::SetUpForExternalEncoderFactory() {
@@ -796,35 +806,39 @@
std::find_if(codecs_after.begin(), codecs_after.end(), is_flexfec));
}
-// Test that external codecs are added to the end of the supported codec list.
+// Test that external codecs are added after internal SW codecs.
TEST_F(WebRtcVideoEngineTest, ReportSupportedExternalCodecs) {
- encoder_factory_->AddSupportedVideoCodecType("FakeExternalCodec");
+ const char* kFakeExternalCodecName = "FakeExternalCodec";
+ encoder_factory_->AddSupportedVideoCodecType(kFakeExternalCodecName);
- std::vector<cricket::VideoCodec> codecs(engine_.codecs());
- ASSERT_GE(codecs.size(), 2u);
- cricket::VideoCodec internal_codec = codecs.front();
- cricket::VideoCodec external_codec = codecs.back();
-
- // The external codec will appear last in the vector.
- EXPECT_EQ("VP8", internal_codec.name);
- EXPECT_EQ("FakeExternalCodec", external_codec.name);
+ // The external codec should appear after the internal codec in the vector.
+ const int vp8_index = GetEngineCodecIndex("VP8");
+ const int fake_external_codec_index =
+ GetEngineCodecIndex(kFakeExternalCodecName);
+ EXPECT_LT(vp8_index, fake_external_codec_index);
}
// Test that an external codec that was added after the engine was initialized
// does show up in the codec list after it was added.
TEST_F(WebRtcVideoEngineTest, ReportSupportedExternalCodecsWithAddedCodec) {
- // Set up external encoder factory with first codec, and initialize engine.
- encoder_factory_->AddSupportedVideoCodecType("FakeExternalCodec1");
+ const char* kFakeExternalCodecName1 = "FakeExternalCodec1";
+ const char* kFakeExternalCodecName2 = "FakeExternalCodec2";
- // The first external codec will appear last in the vector.
+ // Set up external encoder factory with first codec, and initialize engine.
+ encoder_factory_->AddSupportedVideoCodecType(kFakeExternalCodecName1);
+
std::vector<cricket::VideoCodec> codecs_before(engine_.codecs());
- EXPECT_EQ("FakeExternalCodec1", codecs_before.back().name);
// Add second codec.
- encoder_factory_->AddSupportedVideoCodecType("FakeExternalCodec2");
+ encoder_factory_->AddSupportedVideoCodecType(kFakeExternalCodecName2);
std::vector<cricket::VideoCodec> codecs_after(engine_.codecs());
EXPECT_EQ(codecs_before.size() + 1, codecs_after.size());
- EXPECT_EQ("FakeExternalCodec2", codecs_after.back().name);
+
+ // Check that both fake codecs are present and that the second fake codec
+ // appears after the first fake codec.
+ const int fake_codec_index1 = GetEngineCodecIndex(kFakeExternalCodecName1);
+ const int fake_codec_index2 = GetEngineCodecIndex(kFakeExternalCodecName2);
+ EXPECT_LT(fake_codec_index1, fake_codec_index2);
}
TEST_F(WebRtcVideoEngineTest, RegisterExternalDecodersIfSupported) {
@@ -910,10 +924,28 @@
// Verify the codecs from the engine.
const std::vector<VideoCodec> engine_codecs = engine.codecs();
- // Verify an RTX codec has been added correctly.
- EXPECT_EQ(2u, engine_codecs.size());
+ // Verify default codecs has been added correctly.
+ EXPECT_EQ(5u, engine_codecs.size());
EXPECT_EQ("VP8", engine_codecs.at(0).name);
+
+ // RTX codec for VP8.
EXPECT_EQ("rtx", engine_codecs.at(1).name);
+ int vp8_associated_payload;
+ EXPECT_TRUE(engine_codecs.at(1).GetParam(kCodecParamAssociatedPayloadType,
+ &vp8_associated_payload));
+ EXPECT_EQ(vp8_associated_payload, engine_codecs.at(0).id);
+
+ EXPECT_EQ(kRedCodecName, engine_codecs.at(2).name);
+
+ // RTX codec for RED.
+ EXPECT_EQ("rtx", engine_codecs.at(3).name);
+ int red_associated_payload;
+ EXPECT_TRUE(engine_codecs.at(3).GetParam(kCodecParamAssociatedPayloadType,
+ &red_associated_payload));
+ EXPECT_EQ(red_associated_payload, engine_codecs.at(2).id);
+
+ EXPECT_EQ(kUlpfecCodecName, engine_codecs.at(4).name);
+
int associated_payload_type;
EXPECT_TRUE(engine_codecs.at(1).GetParam(
cricket::kCodecParamAssociatedPayloadType, &associated_payload_type));
