Add RTX codecs for codecs only supported by external encoder.
Previously we were only adding these RTX codecs if the codec was
internally supported.
R=pbos@webrtc.org, pthatcher@webrtc.org
Review URL: https://codereview.webrtc.org/2088233004 .
Cr-Original-Commit-Position: refs/heads/master@{#13328}
Cr-Mirrored-From: https://chromium.googlesource.com/external/webrtc
Cr-Mirrored-Commit: 6c3e788dcfe694e382fee4d662ca3ea6c1d8c463
diff --git a/media/engine/webrtcvideoengine2.cc b/media/engine/webrtcvideoengine2.cc
index 005cb68..5da8e33 100644
--- a/media/engine/webrtcvideoengine2.cc
+++ b/media/engine/webrtcvideoengine2.cc
@@ -342,17 +342,37 @@
// Down grade resolution at most 2 times for CPU reasons.
static const int kMaxCpuDowngrades = 2;
+// Adds |codec| to |list|, and also adds an RTX codec if |codec|'s name is
+// recognized.
+// TODO(deadbeef): Should we add RTX codecs for external codecs whose names we
+// don't recognize?
+void AddCodecAndMaybeRtxCodec(const VideoCodec& codec,
+ std::vector<VideoCodec>* codecs) {
+ codecs->push_back(codec);
+ int rtx_payload_type = 0;
+ if (CodecNamesEq(codec.name, kVp8CodecName)) {
+ rtx_payload_type = kDefaultRtxVp8PlType;
+ } else if (CodecNamesEq(codec.name, kVp9CodecName)) {
+ rtx_payload_type = kDefaultRtxVp9PlType;
+ } else if (CodecNamesEq(codec.name, kH264CodecName)) {
+ rtx_payload_type = kDefaultRtxH264PlType;
+ } else if (CodecNamesEq(codec.name, kRedCodecName)) {
+ rtx_payload_type = kDefaultRtxRedPlType;
+ } else {
+ return;
+ }
+ codecs->push_back(VideoCodec::CreateRtxCodec(rtx_payload_type, codec.id));
+}
+
std::vector<VideoCodec> DefaultVideoCodecList() {
std::vector<VideoCodec> codecs;
- codecs.push_back(MakeVideoCodecWithDefaultFeedbackParams(kDefaultVp8PlType,
- kVp8CodecName));
- codecs.push_back(
- VideoCodec::CreateRtxCodec(kDefaultRtxVp8PlType, kDefaultVp8PlType));
+ AddCodecAndMaybeRtxCodec(
+ MakeVideoCodecWithDefaultFeedbackParams(kDefaultVp8PlType, kVp8CodecName),
+ &codecs);
if (CodecIsInternallySupported(kVp9CodecName)) {
- codecs.push_back(MakeVideoCodecWithDefaultFeedbackParams(kDefaultVp9PlType,
- kVp9CodecName));
- codecs.push_back(
- VideoCodec::CreateRtxCodec(kDefaultRtxVp9PlType, kDefaultVp9PlType));
+ AddCodecAndMaybeRtxCodec(MakeVideoCodecWithDefaultFeedbackParams(
+ kDefaultVp9PlType, kVp9CodecName),
+ &codecs);
}
if (CodecIsInternallySupported(kH264CodecName)) {
VideoCodec codec = MakeVideoCodecWithDefaultFeedbackParams(
@@ -366,13 +386,10 @@
kH264ProfileLevelConstrainedBaseline);
codec.SetParam(kH264FmtpLevelAsymmetryAllowed, "1");
codec.SetParam(kH264FmtpPacketizationMode, "1");
- codecs.push_back(codec);
- codecs.push_back(
- VideoCodec::CreateRtxCodec(kDefaultRtxH264PlType, kDefaultH264PlType));
+ AddCodecAndMaybeRtxCodec(codec, &codecs);
}
- codecs.push_back(VideoCodec(kDefaultRedPlType, kRedCodecName));
- codecs.push_back(
- VideoCodec::CreateRtxCodec(kDefaultRtxRedPlType, kDefaultRedPlType));
+ AddCodecAndMaybeRtxCodec(VideoCodec(kDefaultRedPlType, kRedCodecName),
+ &codecs);
codecs.push_back(VideoCodec(kDefaultUlpfecType, kUlpfecCodecName));
return codecs;
}
@@ -622,6 +639,12 @@
// External video encoders are given payloads 120-127. This also means that
// we only support up to 8 external payload types.
+ // TODO(deadbeef): mediasession.cc already has code to dynamically
+ // determine a payload type. We should be able to just leave the payload
+ // type empty and let mediasession determine it. However, currently RTX
+ // codecs are associated to codecs by payload type, meaning we DO need
+ // to allocate unique payload types here. So to make this change we would
+ // need to make RTX codecs associated by name instead.
const int kExternalVideoPayloadTypeBase = 120;
size_t payload_type = kExternalVideoPayloadTypeBase + i;
RTC_DCHECK(payload_type < 128);
@@ -630,7 +653,7 @@
codecs[i].max_fps);
AddDefaultFeedbackParams(&codec);
- supported_codecs.push_back(codec);
+ AddCodecAndMaybeRtxCodec(codec, &supported_codecs);
}
LOG(LS_INFO) << "Supported codecs (incl. external codecs): "
<< CodecVectorToString(supported_codecs);
diff --git a/media/engine/webrtcvideoengine2_unittest.cc b/media/engine/webrtcvideoengine2_unittest.cc
index bf03afa..24cd8fc 100644
--- a/media/engine/webrtcvideoengine2_unittest.cc
+++ b/media/engine/webrtcvideoengine2_unittest.cc
@@ -388,6 +388,34 @@
EXPECT_EQ(0u, encoder_factory.encoders().size());
}
+// Test that when an external encoder factory supports a codec we don't
+// internally support, we still add an RTX codec for it.
+// TODO(deadbeef): Currently this test is only effective if WebRTC is
+// built with no internal H264 support. This test should be updated
+// if/when we start adding RTX codecs for unrecognized codec names.
+TEST_F(WebRtcVideoEngine2Test, RtxCodecAddedForExternalCodec) {
+ cricket::FakeWebRtcVideoEncoderFactory encoder_factory;
+ encoder_factory.AddSupportedVideoCodecType(webrtc::kVideoCodecH264, "H264");
+ engine_.SetExternalEncoderFactory(&encoder_factory);
+ engine_.Init();
+
+ auto codecs = engine_.codecs();
+ // First figure out what payload type the test codec got assigned.
+ auto test_codec_it =
+ std::find_if(codecs.begin(), codecs.end(),
+ [](const VideoCodec& c) { return c.name == "H264"; });
+ ASSERT_NE(codecs.end(), test_codec_it);
+ // Now search for an RTX codec for it.
+ EXPECT_TRUE(std::any_of(codecs.begin(), codecs.end(),
+ [&test_codec_it](const VideoCodec& c) {
+ int associated_payload_type;
+ return c.name == "rtx" &&
+ c.GetParam(kCodecParamAssociatedPayloadType,
+ &associated_payload_type) &&
+ associated_payload_type == test_codec_it->id;
+ }));
+}
+
void WebRtcVideoEngine2Test::TestExtendedEncoderOveruse(
bool use_external_encoder) {
cricket::FakeWebRtcVideoEncoderFactory encoder_factory;
