From 3d200bd6ac61437ed8808ed395d425c71c829c4a Mon Sep 17 00:00:00 2001 From: brandtr Date: Mon, 16 Jan 2017 06:59:19 -0800 Subject: [PATCH] Remove FlexfecConfig and replace with specific struct in VideoSendStream. The existence of FlexfecConfig is due to a naive design. Now when it is not used on the receiving side (see https://codereview.webrtc.org/2542413002), it is time to remove it from the sending side as well. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2621573002 Cr-Commit-Position: refs/heads/master@{#16097} --- webrtc/config.cc | 38 ------------------- webrtc/config.h | 22 ----------- webrtc/media/engine/webrtcvideoengine2.cc | 4 +- .../engine/webrtcvideoengine2_unittest.cc | 26 ++++++------- webrtc/test/call_test.cc | 4 +- webrtc/video/end_to_end_tests.cc | 7 ++-- webrtc/video/send_statistics_proxy.cc | 6 +-- .../video/send_statistics_proxy_unittest.cc | 4 +- webrtc/video/video_quality_test.cc | 5 +-- webrtc/video/video_send_stream.cc | 22 ++++++++--- webrtc/video_send_stream.h | 19 ++++++++-- 11 files changed, 59 insertions(+), 98 deletions(-) diff --git a/webrtc/config.cc b/webrtc/config.cc index cb6dcb24a1..6ffd1c3fd1 100644 --- a/webrtc/config.cc +++ b/webrtc/config.cc @@ -37,44 +37,6 @@ bool UlpfecConfig::operator==(const UlpfecConfig& other) const { red_rtx_payload_type == other.red_rtx_payload_type; } -FlexfecConfig::FlexfecConfig() - : flexfec_payload_type(-1), flexfec_ssrc(0), protected_media_ssrcs() {} - -FlexfecConfig::~FlexfecConfig() = default; - -std::string FlexfecConfig::ToString() const { - std::stringstream ss; - ss << "{flexfec_payload_type: " << flexfec_payload_type; - ss << ", flexfec_ssrc: " << flexfec_ssrc; - ss << ", protected_media_ssrcs: ["; - size_t i = 0; - for (; i + 1 < protected_media_ssrcs.size(); ++i) - ss << protected_media_ssrcs[i] << ", "; - if (!protected_media_ssrcs.empty()) - ss << protected_media_ssrcs[i]; - ss << "]}"; - return ss.str(); -} - -bool FlexfecConfig::IsCompleteAndEnabled() const { - // Check if FlexFEC is enabled. - if (flexfec_payload_type < 0) - return false; - // Do we have the necessary SSRC information? - if (flexfec_ssrc == 0) - return false; - // TODO(brandtr): Update this check when we support multistream protection. - if (protected_media_ssrcs.size() != 1u) - return false; - return true; -} - -bool FlexfecConfig::operator==(const FlexfecConfig& other) const { - return flexfec_payload_type == other.flexfec_payload_type && - flexfec_ssrc == other.flexfec_ssrc && - protected_media_ssrcs == other.protected_media_ssrcs; -} - std::string RtpExtension::ToString() const { std::stringstream ss; ss << "{uri: " << uri; diff --git a/webrtc/config.h b/webrtc/config.h index e0883e671d..22b279c418 100644 --- a/webrtc/config.h +++ b/webrtc/config.h @@ -56,28 +56,6 @@ struct UlpfecConfig { int red_rtx_payload_type; }; -// Settings for FlexFEC forward error correction. -// Set the payload type to '-1' to disable. -struct FlexfecConfig { - FlexfecConfig(); - ~FlexfecConfig(); - std::string ToString() const; - bool IsCompleteAndEnabled() const; - bool operator==(const FlexfecConfig& other) const; - - // Payload type of FlexFEC. - int flexfec_payload_type; - - // SSRC of FlexFEC stream. - uint32_t flexfec_ssrc; - - // Vector containing a single element, corresponding to the SSRC of the media - // stream being protected by this FlexFEC stream. The vector MUST have size 1. - // - // TODO(brandtr): Update comment above when we support multistream protection. - std::vector protected_media_ssrcs; -}; - // RTP header extension, see RFC 5285. struct RtpExtension { RtpExtension() : id(0) {} diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 14e72d5574..996e2bdb00 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -1589,7 +1589,7 @@ WebRtcVideoChannel2::WebRtcVideoSendStream::WebRtcVideoSendStream( } flexfec_enabled = true; - parameters_.config.rtp.flexfec.flexfec_ssrc = flexfec_ssrc; + parameters_.config.rtp.flexfec.ssrc = flexfec_ssrc; parameters_.config.rtp.flexfec.protected_media_ssrcs = {primary_ssrc}; } } @@ -1720,7 +1720,7 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodec( parameters_.config.encoder_settings.internal_source = false; } parameters_.config.rtp.ulpfec = codec_settings.ulpfec; - parameters_.config.rtp.flexfec.flexfec_payload_type = + parameters_.config.rtp.flexfec.payload_type = codec_settings.flexfec_payload_type; // Set RTX payload type if RTX is enabled. diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index 7d80a0db63..bf01767017 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -2346,7 +2346,7 @@ TEST_F(WebRtcVideoChannel2Test, FlexfecCodecWithoutSsrcNotExposedByDefault) { FakeVideoSendStream* stream = AddSendStream(); webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); - EXPECT_EQ(-1, config.rtp.flexfec.flexfec_payload_type); + EXPECT_EQ(-1, config.rtp.flexfec.payload_type); } // TODO(brandtr): Remove when FlexFEC _is_ exposed by default. @@ -2355,7 +2355,7 @@ TEST_F(WebRtcVideoChannel2Test, FlexfecCodecWithSsrcNotExposedByDefault) { CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); - EXPECT_EQ(-1, config.rtp.flexfec.flexfec_payload_type); + EXPECT_EQ(-1, config.rtp.flexfec.payload_type); } // TODO(brandtr): When FlexFEC is no longer behind a field trial, merge all @@ -2381,9 +2381,9 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetDefaultSendCodecsWithoutSsrc) { FakeVideoSendStream* stream = AddSendStream(); webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); - EXPECT_EQ(GetEngineCodec("flexfec-03").id, - config.rtp.flexfec.flexfec_payload_type); - EXPECT_FALSE(config.rtp.flexfec.IsCompleteAndEnabled()); + EXPECT_EQ(GetEngineCodec("flexfec-03").id, config.rtp.flexfec.payload_type); + EXPECT_EQ(0U, config.rtp.flexfec.ssrc); + EXPECT_TRUE(config.rtp.flexfec.protected_media_ssrcs.empty()); } // TODO(brandtr): Merge into "non-field trial" test when FlexFEC is enabled @@ -2393,9 +2393,10 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetDefaultSendCodecsWithSsrc) { CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); - EXPECT_EQ(GetEngineCodec("flexfec-03").id, - config.rtp.flexfec.flexfec_payload_type); - EXPECT_TRUE(config.rtp.flexfec.IsCompleteAndEnabled()); + EXPECT_EQ(GetEngineCodec("flexfec-03").id, config.rtp.flexfec.payload_type); + EXPECT_EQ(kFlexfecSsrc, config.rtp.flexfec.ssrc); + ASSERT_EQ(1U, config.rtp.flexfec.protected_media_ssrcs.size()); + EXPECT_EQ(kSsrcs1[0], config.rtp.flexfec.protected_media_ssrcs[0]); } TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithoutFec) { @@ -2420,7 +2421,7 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendCodecsWithoutFec) { FakeVideoSendStream* stream = AddSendStream(); webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); - EXPECT_EQ(-1, config.rtp.flexfec.flexfec_payload_type); + EXPECT_EQ(-1, config.rtp.flexfec.payload_type); } TEST_F(WebRtcVideoChannel2Test, @@ -2492,9 +2493,8 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendCodecsWithoutFecDisablesFec) { CreatePrimaryWithFecFrStreamParams("cname", kSsrcs1[0], kFlexfecSsrc)); webrtc::VideoSendStream::Config config = stream->GetConfig().Copy(); - EXPECT_EQ(GetEngineCodec("flexfec-03").id, - config.rtp.flexfec.flexfec_payload_type); - EXPECT_EQ(kFlexfecSsrc, config.rtp.flexfec.flexfec_ssrc); + EXPECT_EQ(GetEngineCodec("flexfec-03").id, config.rtp.flexfec.payload_type); + EXPECT_EQ(kFlexfecSsrc, config.rtp.flexfec.ssrc); ASSERT_EQ(1U, config.rtp.flexfec.protected_media_ssrcs.size()); EXPECT_EQ(kSsrcs1[0], config.rtp.flexfec.protected_media_ssrcs[0]); @@ -2503,7 +2503,7 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendCodecsWithoutFecDisablesFec) { stream = fake_call_->GetVideoSendStreams()[0]; ASSERT_TRUE(stream != nullptr); config = stream->GetConfig().Copy(); - EXPECT_EQ(-1, config.rtp.flexfec.flexfec_payload_type) + EXPECT_EQ(-1, config.rtp.flexfec.payload_type) << "SetSendCodec without FlexFEC should disable current FlexFEC."; } diff --git a/webrtc/test/call_test.cc b/webrtc/test/call_test.cc index 2b7d2e7f08..290992a9ca 100644 --- a/webrtc/test/call_test.cc +++ b/webrtc/test/call_test.cc @@ -231,8 +231,8 @@ void CallTest::CreateSendConfig(size_t num_video_streams, // TODO(brandtr): Update this when we support multistream protection. if (num_flexfec_streams > 0) { - video_send_config_.rtp.flexfec.flexfec_payload_type = kFlexfecPayloadType; - video_send_config_.rtp.flexfec.flexfec_ssrc = kFlexfecSendSsrc; + video_send_config_.rtp.flexfec.payload_type = kFlexfecPayloadType; + video_send_config_.rtp.flexfec.ssrc = kFlexfecSendSsrc; video_send_config_.rtp.flexfec.protected_media_ssrcs = {kVideoSendSsrcs[0]}; } } diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 367b74ce0e..946d81d5df 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -3864,10 +3864,11 @@ void VerifyEmptyUlpfecConfig(const UlpfecConfig& config) { << "Enabling RTX in ULPFEC requires rtpmap: rtx negotiation."; } -void VerifyEmptyFlexfecConfig(const FlexfecConfig& config) { - EXPECT_EQ(-1, config.flexfec_payload_type) +void VerifyEmptyFlexfecConfig( + const VideoSendStream::Config::Rtp::Flexfec& config) { + EXPECT_EQ(-1, config.payload_type) << "Enabling FlexFEC requires rtpmap: flexfec negotiation."; - EXPECT_EQ(0U, config.flexfec_ssrc) + EXPECT_EQ(0U, config.ssrc) << "Enabling FlexFEC requires ssrc-group: FEC-FR negotiation."; EXPECT_TRUE(config.protected_media_ssrcs.empty()) << "Enabling FlexFEC requires ssrc-group: FEC-FR negotiation."; diff --git a/webrtc/video/send_statistics_proxy.cc b/webrtc/video/send_statistics_proxy.cc index bd1441f903..98b2b1d45d 100644 --- a/webrtc/video/send_statistics_proxy.cc +++ b/webrtc/video/send_statistics_proxy.cc @@ -365,7 +365,7 @@ void SendStatisticsProxy::UmaSamplesContainer::UpdateHistograms( static_cast(rtx.transmitted.TotalBytes() * 8 / elapsed_sec / 1000)); } - if (rtp_config.flexfec.flexfec_payload_type != -1 || + if (rtp_config.flexfec.payload_type != -1 || rtp_config.ulpfec.red_payload_type != -1) { RTC_HISTOGRAMS_COUNTS_10000(kIndex, uma_prefix_ + "FecBitrateSentInKbps", @@ -447,8 +447,8 @@ VideoSendStream::StreamStats* SendStatisticsProxy::GetStatsEntry( bool is_media = std::find(rtp_config_.ssrcs.begin(), rtp_config_.ssrcs.end(), ssrc) != rtp_config_.ssrcs.end(); - bool is_flexfec = rtp_config_.flexfec.flexfec_payload_type != -1 && - ssrc == rtp_config_.flexfec.flexfec_ssrc; + bool is_flexfec = rtp_config_.flexfec.payload_type != -1 && + ssrc == rtp_config_.flexfec.ssrc; bool is_rtx = std::find(rtp_config_.rtx.ssrcs.begin(), rtp_config_.rtx.ssrcs.end(), ssrc) != rtp_config_.rtx.ssrcs.end(); diff --git a/webrtc/video/send_statistics_proxy_unittest.cc b/webrtc/video/send_statistics_proxy_unittest.cc index 7fa3ae3638..915b8df177 100644 --- a/webrtc/video/send_statistics_proxy_unittest.cc +++ b/webrtc/video/send_statistics_proxy_unittest.cc @@ -75,8 +75,8 @@ class SendStatisticsProxyTest : public ::testing::Test { config.rtp.ssrcs.push_back(kSecondSsrc); config.rtp.rtx.ssrcs.push_back(kFirstRtxSsrc); config.rtp.rtx.ssrcs.push_back(kSecondRtxSsrc); - config.rtp.flexfec.flexfec_payload_type = 50; - config.rtp.flexfec.flexfec_ssrc = kFlexFecSsrc; + config.rtp.flexfec.payload_type = 50; + config.rtp.flexfec.ssrc = kFlexFecSsrc; return config; } diff --git a/webrtc/video/video_quality_test.cc b/webrtc/video/video_quality_test.cc index e37b555f7e..e3ae616df2 100644 --- a/webrtc/video/video_quality_test.cc +++ b/webrtc/video/video_quality_test.cc @@ -1117,9 +1117,8 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, // Set up the receive config manually instead. FlexfecReceiveStream::Config flexfec_receive_config(recv_transport); flexfec_receive_config.payload_type = - video_send_config_.rtp.flexfec.flexfec_payload_type; - flexfec_receive_config.remote_ssrc = - video_send_config_.rtp.flexfec.flexfec_ssrc; + video_send_config_.rtp.flexfec.payload_type; + flexfec_receive_config.remote_ssrc = video_send_config_.rtp.flexfec.ssrc; flexfec_receive_config.protected_media_ssrcs = video_send_config_.rtp.flexfec.protected_media_ssrcs; flexfec_receive_config.transport_cc = params_.call.send_side_bwe; diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 41f8b3f367..3b4238c47d 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -94,12 +94,12 @@ std::vector CreateRtpRtcpModules( // TODO(brandtr): Update this function when we support multistream protection. std::unique_ptr MaybeCreateFlexfecSender( const VideoSendStream::Config& config) { - if (config.rtp.flexfec.flexfec_payload_type < 0) { + if (config.rtp.flexfec.payload_type < 0) { return nullptr; } - RTC_DCHECK_GE(config.rtp.flexfec.flexfec_payload_type, 0); - RTC_DCHECK_LE(config.rtp.flexfec.flexfec_payload_type, 127); - if (config.rtp.flexfec.flexfec_ssrc == 0) { + RTC_DCHECK_GE(config.rtp.flexfec.payload_type, 0); + RTC_DCHECK_LE(config.rtp.flexfec.payload_type, 127); + if (config.rtp.flexfec.ssrc == 0) { LOG(LS_WARNING) << "FlexFEC is enabled, but no FlexFEC SSRC given. " "Therefore disabling FlexFEC."; return nullptr; @@ -128,7 +128,7 @@ std::unique_ptr MaybeCreateFlexfecSender( RTC_DCHECK_EQ(1U, config.rtp.flexfec.protected_media_ssrcs.size()); return std::unique_ptr(new FlexfecSender( - config.rtp.flexfec.flexfec_payload_type, config.rtp.flexfec.flexfec_ssrc, + config.rtp.flexfec.payload_type, config.rtp.flexfec.ssrc, config.rtp.flexfec.protected_media_ssrcs[0], config.rtp.extensions, Clock::GetRealTimeClock())); } @@ -184,7 +184,17 @@ std::string VideoSendStream::Config::Rtp::ToString() const { ss << ", nack: {rtp_history_ms: " << nack.rtp_history_ms << '}'; ss << ", ulpfec: " << ulpfec.ToString(); - ss << ", flexfec: " << flexfec.ToString(); + + ss << ", flexfec: {payload_type: " << flexfec.payload_type; + ss << ", ssrc: " << flexfec.ssrc; + ss << ", protected_media_ssrcs: ["; + for (size_t i = 0; i < flexfec.protected_media_ssrcs.size(); ++i) { + ss << flexfec.protected_media_ssrcs[i]; + if (i != flexfec.protected_media_ssrcs.size() - 1) + ss << ", "; + } + ss << ']'; + ss << ", rtx: " << rtx.ToString(); ss << ", c_name: " << c_name; ss << '}'; diff --git a/webrtc/video_send_stream.h b/webrtc/video_send_stream.h index fb5e9d877d..680eb02bc4 100644 --- a/webrtc/video_send_stream.h +++ b/webrtc/video_send_stream.h @@ -138,10 +138,21 @@ class VideoSendStream { // See UlpfecConfig for description. UlpfecConfig ulpfec; - // See FlexfecConfig for description. - // TODO(brandtr): Move this config to a new class FlexfecSendStream - // when we support multistream protection. - FlexfecConfig flexfec; + struct Flexfec { + // Payload type of FlexFEC. Set to -1 to disable sending FlexFEC. + int payload_type = -1; + + // SSRC of FlexFEC stream. + uint32_t ssrc = 0; + + // Vector containing a single element, corresponding to the SSRC of the + // media stream being protected by this FlexFEC stream. + // The vector MUST have size 1. + // + // TODO(brandtr): Update comment above when we support + // multistream protection. + std::vector protected_media_ssrcs; + } flexfec; // Settings for RTP retransmission payload format, see RFC 4588 for // details.