From 6c3e788dcfe694e382fee4d662ca3ea6c1d8c463 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Wed, 29 Jun 2016 11:14:19 -0700 Subject: [PATCH] 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-Commit-Position: refs/heads/master@{#13328} --- webrtc/media/engine/webrtcvideoengine2.cc | 53 +++++++++++++------ .../engine/webrtcvideoengine2_unittest.cc | 28 ++++++++++ 2 files changed, 66 insertions(+), 15 deletions(-) diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 005cb68f9b..5da8e33e1e 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -342,17 +342,37 @@ static const int kDefaultRtcpReceiverReportSsrc = 1; // 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* 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 DefaultVideoCodecList() { std::vector 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 @@ std::vector DefaultVideoCodecList() { 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 @@ std::vector WebRtcVideoEngine2::GetSupportedCodecs() const { // 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 @@ std::vector WebRtcVideoEngine2::GetSupportedCodecs() const { 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/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index bf03afac0f..24cd8fc957 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -388,6 +388,34 @@ TEST_F(WebRtcVideoEngine2Test, UseExternalFactoryForVp8WhenSupported) { 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;