From 31bd224f356b6dff18664370abd475351de4c683 Mon Sep 17 00:00:00 2001 From: brandtr Date: Fri, 19 May 2017 05:47:46 -0700 Subject: [PATCH] Reduce VideoSendStream recreations due to FlexFEC. This CL reduces the number of VideoSendStream recreations during SDP renegotiation by checking the FlexFEC field trials before, and not after, the SDP codec diffing logic. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2882433003 Cr-Commit-Position: refs/heads/master@{#18211} --- webrtc/media/engine/webrtcvideoengine2.cc | 34 ++-- .../engine/webrtcvideoengine2_unittest.cc | 159 +++++++++++++----- 2 files changed, 138 insertions(+), 55 deletions(-) diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 2465aab778..4980153490 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -43,14 +43,18 @@ using DegradationPreference = webrtc::VideoSendStream::DegradationPreference; namespace cricket { namespace { // If this field trial is enabled, we will enable sending FlexFEC and disable -// sending ULPFEC whenever the former has been negotiated. -// FlexFEC can only be negotiated when the "flexfec-03" SDP codec is enabled, -// which is done by enabling the "WebRTC-FlexFEC-03-Advertised" field trial; see -// internalencoderfactory.cc. +// 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. +bool IsFlexfecAdvertisedFieldTrialEnabled() { + return webrtc::field_trial::IsEnabled("WebRTC-FlexFEC-03-Advertised"); +} + // If this field trial is enabled, we will report VideoContentType RTP extension // in capabilities (thus, it will end up in the default SDP and extension will // be sent for all key-frames). @@ -705,7 +709,7 @@ bool WebRtcVideoChannel2::GetChangedSendParameters( } // Select one of the remote codecs that will be used as send codec. - const rtc::Optional selected_send_codec = + rtc::Optional selected_send_codec = SelectSendVideoCodec(MapCodecs(params.codecs)); if (!selected_send_codec) { @@ -713,6 +717,15 @@ bool WebRtcVideoChannel2::GetChangedSendParameters( return false; } + // Never enable sending FlexFEC, unless we are in the experiment. + if (!IsFlexfecFieldTrialEnabled()) { + if (selected_send_codec->flexfec_payload_type != -1) { + LOG(LS_INFO) << "Remote supports flexfec-03, but we will not send since " + << "WebRTC-FlexFEC-03 field trial is not enabled."; + } + selected_send_codec->flexfec_payload_type = -1; + } + if (!send_codec_ || *selected_send_codec != *send_codec_) changed_params->codec = selected_send_codec; @@ -1270,7 +1283,8 @@ void WebRtcVideoChannel2::ConfigureReceiverRtp( config->rtp.extensions = recv_rtp_extensions_; // TODO(brandtr): Generalize when we add support for multistream protection. - if (sp.GetFecFrSsrc(ssrc, &flexfec_config->remote_ssrc)) { + if (IsFlexfecAdvertisedFieldTrialEnabled() && + sp.GetFecFrSsrc(ssrc, &flexfec_config->remote_ssrc)) { flexfec_config->protected_media_ssrcs = {ssrc}; flexfec_config->local_ssrc = config->rtp.local_ssrc; flexfec_config->rtcp_mode = config->rtp.rtcp_mode; @@ -1606,7 +1620,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream( for (uint32_t primary_ssrc : parameters_.config.rtp.ssrcs) { if (sp.GetFecFrSsrc(primary_ssrc, &flexfec_ssrc)) { if (flexfec_enabled) { - LOG(LS_INFO) << "Multiple FlexFEC streams proposed by remote, but " + LOG(LS_INFO) << "Multiple FlexFEC streams in local SDP, but " "our implementation only supports a single FlexFEC " "stream. Will not enable FlexFEC for proposed " "stream with SSRC: " @@ -1781,10 +1795,8 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodec( parameters_.config.encoder_settings.internal_source = false; } parameters_.config.rtp.ulpfec = codec_settings.ulpfec; - if (IsFlexfecFieldTrialEnabled()) { - parameters_.config.rtp.flexfec.payload_type = - codec_settings.flexfec_payload_type; - } + parameters_.config.rtp.flexfec.payload_type = + codec_settings.flexfec_payload_type; // Set RTX payload type if RTX is enabled. if (!parameters_.config.rtp.rtx.ssrcs.empty()) { diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index 3165be1e2d..46664c4d94 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -2430,45 +2430,99 @@ TEST_F(WebRtcVideoChannel2Test, SetDefaultSendCodecs) { // TODO(juberti): Check RTCP, PLI, TMMBR. } -// TODO(brandtr): Remove when FlexFEC _is_ exposed by default. -TEST_F(WebRtcVideoChannel2Test, FlexfecCodecWithoutSsrcNotExposedByDefault) { +// The following four tests ensures that FlexFEC is not activated by default +// when the field trials are not enabled. +// TODO(brandtr): Remove or update these tests when FlexFEC _is_ enabled by +// default. +TEST_F(WebRtcVideoChannel2Test, + FlexfecSendCodecWithoutSsrcNotExposedByDefault) { FakeVideoSendStream* stream = AddSendStream(); webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); EXPECT_EQ(-1, config.rtp.flexfec.payload_type); + EXPECT_EQ(0U, config.rtp.flexfec.ssrc); + EXPECT_TRUE(config.rtp.flexfec.protected_media_ssrcs.empty()); } -// TODO(brandtr): Remove when FlexFEC _is_ exposed by default. -TEST_F(WebRtcVideoChannel2Test, FlexfecCodecWithSsrcNotExposedByDefault) { +TEST_F(WebRtcVideoChannel2Test, FlexfecSendCodecWithSsrcNotExposedByDefault) { FakeVideoSendStream* stream = AddSendStream( CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); EXPECT_EQ(-1, config.rtp.flexfec.payload_type); + EXPECT_EQ(0U, config.rtp.flexfec.ssrc); + EXPECT_TRUE(config.rtp.flexfec.protected_media_ssrcs.empty()); +} + +TEST_F(WebRtcVideoChannel2Test, + FlexfecRecvCodecWithoutSsrcNotExposedByDefault) { + AddRecvStream(); + + const std::vector& streams = + fake_call_->GetFlexfecReceiveStreams(); + EXPECT_TRUE(streams.empty()); +} + +TEST_F(WebRtcVideoChannel2Test, FlexfecRecvCodecWithSsrcNotExposedByDefault) { + AddRecvStream( + CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); + + const std::vector& streams = + fake_call_->GetFlexfecReceiveStreams(); + EXPECT_TRUE(streams.empty()); } // TODO(brandtr): When FlexFEC is no longer behind a field trial, merge all // tests that use this test fixture into the corresponding "non-field trial" // tests. -class WebRtcVideoChannel2FlexfecTest : public WebRtcVideoChannel2Test { +class WebRtcVideoChannel2FlexfecRecvTest : public WebRtcVideoChannel2Test { public: - WebRtcVideoChannel2FlexfecTest() - : WebRtcVideoChannel2Test( - "WebRTC-FlexFEC-03-Advertised/Enabled/WebRTC-FlexFEC-03/Enabled/") { - } + WebRtcVideoChannel2FlexfecRecvTest() + : WebRtcVideoChannel2Test("WebRTC-FlexFEC-03-Advertised/Enabled/") {} }; -// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled -// by default. -TEST_F(WebRtcVideoChannel2FlexfecTest, +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, DefaultFlexfecCodecHasTransportCcAndRembFeedbackParam) { EXPECT_TRUE(cricket::HasTransportCc(GetEngineCodec("flexfec-03"))); EXPECT_TRUE(cricket::HasRemb(GetEngineCodec("flexfec-03"))); } -// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled -// by default. -TEST_F(WebRtcVideoChannel2FlexfecTest, SetDefaultSendCodecsWithoutSsrc) { +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, SetDefaultRecvCodecsWithoutSsrc) { + AddRecvStream(); + + const std::vector& streams = + fake_call_->GetFlexfecReceiveStreams(); + EXPECT_TRUE(streams.empty()); +} + +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, SetDefaultRecvCodecsWithSsrc) { + AddRecvStream( + CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); + + const std::vector& streams = + fake_call_->GetFlexfecReceiveStreams(); + ASSERT_EQ(1U, streams.size()); + const FakeFlexfecReceiveStream* stream = streams.front(); + const webrtc::FlexfecReceiveStream::Config& config = stream->GetConfig(); + EXPECT_EQ(GetEngineCodec("flexfec-03").id, config.payload_type); + EXPECT_EQ(kFlexfecSsrc, config.remote_ssrc); + ASSERT_EQ(1U, config.protected_media_ssrcs.size()); + EXPECT_EQ(kSsrcs1[0], config.protected_media_ssrcs[0]); +} + +// TODO(brandtr): When FlexFEC is no longer behind a field trial, merge all +// tests that use this test fixture into the corresponding "non-field trial" +// tests. +class WebRtcVideoChannel2FlexfecSendRecvTest : public WebRtcVideoChannel2Test { + public: + WebRtcVideoChannel2FlexfecSendRecvTest() + : WebRtcVideoChannel2Test( + "WebRTC-FlexFEC-03-Advertised/Enabled/WebRTC-FlexFEC-03/Enabled/") { + } +}; + +TEST_F(WebRtcVideoChannel2FlexfecSendRecvTest, + SetDefaultSendCodecsWithoutSsrc) { FakeVideoSendStream* stream = AddSendStream(); webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); @@ -2477,9 +2531,7 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetDefaultSendCodecsWithoutSsrc) { EXPECT_TRUE(config.rtp.flexfec.protected_media_ssrcs.empty()); } -// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled -// by default. -TEST_F(WebRtcVideoChannel2FlexfecTest, SetDefaultSendCodecsWithSsrc) { +TEST_F(WebRtcVideoChannel2FlexfecSendRecvTest, SetDefaultSendCodecsWithSsrc) { FakeVideoSendStream* stream = AddSendStream( CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); @@ -2502,9 +2554,7 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithoutFec) { EXPECT_EQ(-1, config.rtp.ulpfec.red_payload_type); } -// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled -// by default. -TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendCodecsWithoutFec) { +TEST_F(WebRtcVideoChannel2FlexfecSendRecvTest, SetSendCodecsWithoutFec) { cricket::VideoSendParameters parameters; parameters.codecs.push_back(GetEngineCodec("VP8")); ASSERT_TRUE(channel_->SetSendParameters(parameters)); @@ -2515,7 +2565,7 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendCodecsWithoutFec) { EXPECT_EQ(-1, config.rtp.flexfec.payload_type); } -TEST_F(WebRtcVideoChannel2FlexfecTest, SetRecvCodecsWithFec) { +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, SetRecvCodecsWithFec) { AddRecvStream( CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); @@ -2554,6 +2604,40 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetRecvCodecsWithFec) { flexfec_stream_config.rtp_header_extensions); } +// We should not send FlexFEC, even if we advertise it, unless the right +// field trial is set. +// TODO(brandtr): Remove when FlexFEC is enabled by default. +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, + SetSendCodecsWithoutSsrcWithFecDoesNotEnableFec) { + cricket::VideoSendParameters parameters; + parameters.codecs.push_back(GetEngineCodec("VP8")); + parameters.codecs.push_back(GetEngineCodec("flexfec-03")); + ASSERT_TRUE(channel_->SetSendParameters(parameters)); + + FakeVideoSendStream* stream = AddSendStream(); + webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); + + EXPECT_EQ(-1, config.rtp.flexfec.payload_type); + EXPECT_EQ(0, config.rtp.flexfec.ssrc); + EXPECT_TRUE(config.rtp.flexfec.protected_media_ssrcs.empty()); +} + +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, + SetSendCodecsWithSsrcWithFecDoesNotEnableFec) { + cricket::VideoSendParameters parameters; + parameters.codecs.push_back(GetEngineCodec("VP8")); + parameters.codecs.push_back(GetEngineCodec("flexfec-03")); + ASSERT_TRUE(channel_->SetSendParameters(parameters)); + + FakeVideoSendStream* stream = AddSendStream( + CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); + webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); + + EXPECT_EQ(-1, config.rtp.flexfec.payload_type); + EXPECT_EQ(0, config.rtp.flexfec.ssrc); + EXPECT_TRUE(config.rtp.flexfec.protected_media_ssrcs.empty()); +} + TEST_F(WebRtcVideoChannel2Test, SetSendCodecRejectsRtxWithoutAssociatedPayloadType) { const int kUnusedPayloadType = 127; @@ -2648,9 +2732,8 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithoutFecDisablesFec) { << "SetSendCodec without ULPFEC should disable current ULPFEC."; } -// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled -// by default. -TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendCodecsWithoutFecDisablesFec) { +TEST_F(WebRtcVideoChannel2FlexfecSendRecvTest, + SetSendCodecsWithoutFecDisablesFec) { cricket::VideoSendParameters parameters; parameters.codecs.push_back(GetEngineCodec("VP8")); parameters.codecs.push_back(GetEngineCodec("flexfec-03")); @@ -3054,14 +3137,7 @@ TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithoutFecDisablesFec) { << "SetSendCodec without ULPFEC should disable current ULPFEC."; } -// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled -// by default. -TEST_F(WebRtcVideoChannel2FlexfecTest, SetRecvParamsWithoutFecDisablesFec) { - cricket::VideoSendParameters send_parameters; - send_parameters.codecs.push_back(GetEngineCodec("VP8")); - send_parameters.codecs.push_back(GetEngineCodec("flexfec-03")); - ASSERT_TRUE(channel_->SetSendParameters(send_parameters)); - +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, SetRecvParamsWithoutFecDisablesFec) { AddRecvStream( CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); const std::vector& streams = @@ -3108,9 +3184,8 @@ TEST_F(WebRtcVideoChannel2Test, SetSendParamsWithFecEnablesFec) { << "ULPFEC should be enabled on the receive stream."; } -// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled -// by default. -TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendParamsWithFecEnablesFec) { +TEST_F(WebRtcVideoChannel2FlexfecSendRecvTest, + SetSendRecvParamsWithFecEnablesFec) { AddRecvStream( CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); const std::vector& streams = @@ -3145,7 +3220,7 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendParamsWithFecEnablesFec) { stream_with_send_params->GetConfig().protected_media_ssrcs[0]); } -TEST_F(WebRtcVideoChannel2Test, SetSendCodecsRejectDuplicateFecPayloads) { +TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsRejectDuplicateFecPayloads) { cricket::VideoRecvParameters parameters; parameters.codecs.push_back(GetEngineCodec("VP8")); parameters.codecs.push_back(GetEngineCodec("red")); @@ -3153,10 +3228,8 @@ TEST_F(WebRtcVideoChannel2Test, SetSendCodecsRejectDuplicateFecPayloads) { EXPECT_FALSE(channel_->SetRecvParameters(parameters)); } -// TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled -// by default. -TEST_F(WebRtcVideoChannel2FlexfecTest, - SetSendCodecsRejectDuplicateFecPayloads) { +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, + SetRecvCodecsRejectDuplicateFecPayloads) { cricket::VideoRecvParameters parameters; parameters.codecs.push_back(GetEngineCodec("VP8")); parameters.codecs.push_back(GetEngineCodec("flexfec-03")); @@ -3809,9 +3882,7 @@ TEST_F(WebRtcVideoChannel2Test, UlpfecPacketDoesntCreateUnsignalledStream) { false /* expect_created_receive_stream */); } -// TODO(brandtr): Change to "non-field trial" test when FlexFEC is enabled -// by default. -TEST_F(WebRtcVideoChannel2FlexfecTest, +TEST_F(WebRtcVideoChannel2FlexfecRecvTest, FlexfecPacketDoesntCreateUnsignalledStream) { TestReceiveUnsignaledSsrcPacket(GetEngineCodec("flexfec-03").id, false /* expect_created_receive_stream */);