diff --git a/media/engine/internalencoderfactory.cc b/media/engine/internalencoderfactory.cc index 21640df6a6..fa6783f64a 100644 --- a/media/engine/internalencoderfactory.cc +++ b/media/engine/internalencoderfactory.cc @@ -15,9 +15,24 @@ #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()) @@ -25,6 +40,19 @@ 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 88d25b5896..d2457f8734 100644 --- a/media/engine/webrtcvideoengine.cc +++ b/media/engine/webrtcvideoengine.cc @@ -155,18 +155,25 @@ 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 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. +// 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. bool IsFlexfecAdvertisedFieldTrialEnabled() { return webrtc::field_trial::IsEnabled("WebRTC-FlexFEC-03-Advertised"); } @@ -186,70 +193,6 @@ 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 << '{'; @@ -554,7 +497,7 @@ WebRtcVideoChannel* WebRtcVideoEngine::CreateChannel( } std::vector WebRtcVideoEngine::codecs() const { - return AssignPayloadTypesAndDefaultCodecs(encoder_factory_.get()); + return AssignPayloadTypesAndAddAssociatedRtxCodecs(encoder_factory_.get()); } RtpCapabilities WebRtcVideoEngine::GetCapabilities() const { @@ -583,6 +526,48 @@ 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, @@ -602,7 +587,7 @@ WebRtcVideoChannel::WebRtcVideoChannel( rtcp_receiver_report_ssrc_ = kDefaultRtcpReceiverReportSsrc; sending_ = false; recv_codecs_ = - MapCodecs(AssignPayloadTypesAndDefaultCodecs(encoder_factory_)); + MapCodecs(AssignPayloadTypesAndAddAssociatedRtxCodecs(encoder_factory_)); recv_flexfec_payload_type_ = recv_codecs_.front().flexfec_payload_type; } @@ -617,7 +602,7 @@ rtc::Optional WebRtcVideoChannel::SelectSendVideoCodec( const std::vector& remote_mapped_codecs) const { const std::vector local_supported_codecs = - AssignPayloadTypesAndDefaultCodecs(encoder_factory_); + AssignPayloadTypesAndAddAssociatedRtxCodecs(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 @@ -934,7 +919,7 @@ bool WebRtcVideoChannel::GetChangedRecvParameters( // Verify that every mapped codec is supported locally. const std::vector local_supported_codecs = - AssignPayloadTypesAndDefaultCodecs(encoder_factory_); + AssignPayloadTypesAndAddAssociatedRtxCodecs(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 d3a9b9b2e3..c3542a2018 100644 --- a/media/engine/webrtcvideoengine_unittest.cc +++ b/media/engine/webrtcvideoengine_unittest.cc @@ -181,13 +181,9 @@ 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) const; + cricket::VideoCodec GetEngineCodec(const std::string& name); VideoMediaChannel* SetUpForExternalEncoderFactory(); @@ -569,10 +565,9 @@ TEST_F(WebRtcVideoEngineTest, PropagatesInputFrameTimestamp) { EXPECT_TRUE(channel->RemoveSendStream(kSsrc)); } -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]; +cricket::VideoCodec WebRtcVideoEngineTest::GetEngineCodec( + const std::string& name) { + for (const cricket::VideoCodec& engine_codec : engine_.codecs()) { if (!CodecNamesEq(name, engine_codec.name)) continue; // The tests only use H264 Constrained Baseline. Make sure we don't return @@ -585,16 +580,11 @@ int WebRtcVideoEngineTest::GetEngineCodecIndex(const std::string& name) const { continue; } } - return i; + return engine_codec; } // This point should never be reached. ADD_FAILURE() << "Unrecognized codec name: " << name; - return -1; -} - -cricket::VideoCodec WebRtcVideoEngineTest::GetEngineCodec( - const std::string& name) const { - return engine_.codecs()[GetEngineCodecIndex(name)]; + return cricket::VideoCodec(); } VideoMediaChannel* WebRtcVideoEngineTest::SetUpForExternalEncoderFactory() { @@ -806,39 +796,35 @@ TEST_F(WebRtcVideoEngineTest, std::find_if(codecs_after.begin(), codecs_after.end(), is_flexfec)); } -// Test that external codecs are added after internal SW codecs. +// Test that external codecs are added to the end of the supported codec list. TEST_F(WebRtcVideoEngineTest, ReportSupportedExternalCodecs) { - const char* kFakeExternalCodecName = "FakeExternalCodec"; - encoder_factory_->AddSupportedVideoCodecType(kFakeExternalCodecName); + encoder_factory_->AddSupportedVideoCodecType("FakeExternalCodec"); - // 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); + 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); } // 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) { - const char* kFakeExternalCodecName1 = "FakeExternalCodec1"; - const char* kFakeExternalCodecName2 = "FakeExternalCodec2"; - // Set up external encoder factory with first codec, and initialize engine. - encoder_factory_->AddSupportedVideoCodecType(kFakeExternalCodecName1); + encoder_factory_->AddSupportedVideoCodecType("FakeExternalCodec1"); + // 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(kFakeExternalCodecName2); + encoder_factory_->AddSupportedVideoCodecType("FakeExternalCodec2"); std::vector codecs_after(engine_.codecs()); EXPECT_EQ(codecs_before.size() + 1, codecs_after.size()); - - // 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); + EXPECT_EQ("FakeExternalCodec2", codecs_after.back().name); } TEST_F(WebRtcVideoEngineTest, RegisterExternalDecodersIfSupported) { @@ -924,28 +910,10 @@ TEST(WebRtcVideoEngineNewVideoCodecFactoryTest, Vp8) { // Verify the codecs from the engine. const std::vector engine_codecs = engine.codecs(); - // Verify default codecs has been added correctly. - EXPECT_EQ(5u, engine_codecs.size()); + // Verify an RTX codec has been added correctly. + EXPECT_EQ(2u, 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));