diff --git a/media/engine/internalencoderfactory.cc b/media/engine/internalencoderfactory.cc index fa6783f64a..21640df6a6 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 @@ InternalEncoderFactory::InternalEncoderFactory() { 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 d2457f8734..88d25b5896 100644 --- a/media/engine/webrtcvideoengine.cc +++ b/media/engine/webrtcvideoengine.cc @@ -155,25 +155,18 @@ class NullVideoDecoder : public webrtc::VideoDecoder { const char* ImplementationName() const override { return "NullVideoDecoder"; } }; -std::vector AssignPayloadTypesAndAddAssociatedRtxCodecs( - const std::vector& input_formats); - -std::vector AssignPayloadTypesAndAddAssociatedRtxCodecs( - const webrtc::VideoEncoderFactory* encoder_factory) { - return encoder_factory ? AssignPayloadTypesAndAddAssociatedRtxCodecs( - encoder_factory->GetSupportedFormats()) - : std::vector(); -} - // 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 @@ void AddDefaultFeedbackParams(VideoCodec* codec) { 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 AssignPayloadTypesAndDefaultCodecs( + std::vector input_formats) { + if (input_formats.empty()) + return std::vector(); + 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 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 AssignPayloadTypesAndDefaultCodecs( + const webrtc::VideoEncoderFactory* encoder_factory) { + return encoder_factory ? AssignPayloadTypesAndDefaultCodecs( + encoder_factory->GetSupportedFormats()) + : std::vector(); +} + static std::string CodecVectorToString(const std::vector& codecs) { std::stringstream out; out << '{'; @@ -497,7 +554,7 @@ WebRtcVideoChannel* WebRtcVideoEngine::CreateChannel( } std::vector WebRtcVideoEngine::codecs() const { - return AssignPayloadTypesAndAddAssociatedRtxCodecs(encoder_factory_.get()); + return AssignPayloadTypesAndDefaultCodecs(encoder_factory_.get()); } RtpCapabilities WebRtcVideoEngine::GetCapabilities() const { @@ -526,48 +583,6 @@ RtpCapabilities WebRtcVideoEngine::GetCapabilities() const { 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 AssignPayloadTypesAndAddAssociatedRtxCodecs( - const std::vector& input_formats) { - static const int kFirstDynamicPayloadType = 96; - static const int kLastDynamicPayloadType = 127; - int payload_type = kFirstDynamicPayloadType; - std::vector 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 @@ WebRtcVideoChannel::WebRtcVideoChannel( 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 @@ rtc::Optional WebRtcVideoChannel::SelectSendVideoCodec( const std::vector& remote_mapped_codecs) const { const std::vector 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 @@ bool WebRtcVideoChannel::GetChangedRecvParameters( // Verify that every mapped codec is supported locally. const std::vector 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 c3542a2018..d3a9b9b2e3 100644 --- a/media/engine/webrtcvideoengine_unittest.cc +++ b/media/engine/webrtcvideoengine_unittest.cc @@ -181,9 +181,13 @@ class WebRtcVideoEngineTest : public ::testing::Test { } 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 @@ TEST_F(WebRtcVideoEngineTest, PropagatesInputFrameTimestamp) { 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 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 @@ cricket::VideoCodec WebRtcVideoEngineTest::GetEngineCodec( 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 @@ TEST_F(WebRtcVideoEngineTest, 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 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"; + + // Set up external encoder factory with first codec, and initialize engine. + encoder_factory_->AddSupportedVideoCodecType(kFakeExternalCodecName1); - // The first external codec will appear last in the vector. std::vector codecs_before(engine_.codecs()); - EXPECT_EQ("FakeExternalCodec1", codecs_before.back().name); // Add second codec. - encoder_factory_->AddSupportedVideoCodecType("FakeExternalCodec2"); + encoder_factory_->AddSupportedVideoCodecType(kFakeExternalCodecName2); std::vector 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 @@ TEST(WebRtcVideoEngineNewVideoCodecFactoryTest, Vp8) { // Verify the codecs from the engine. const std::vector 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));