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

@@ -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;

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<uint32_t> protected_media_ssrcs;
-};
-
 // RTP header extension, see RFC 5285.
 struct RtpExtension {
   RtpExtension() : id(0) {}

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.

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.";
 }
 

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]};
   }
 }

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.";

webrtc/video/send_statistics_proxy.cc

@@ -365,7 +365,7 @@ void SendStatisticsProxy::UmaSamplesContainer::UpdateHistograms(
             static_cast<int>(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();

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;
   }
 

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;

webrtc/video/video_send_stream.cc

@@ -94,12 +94,12 @@ std::vector<RtpRtcp*> CreateRtpRtcpModules(
 // TODO(brandtr): Update this function when we support multistream protection.
 std::unique_ptr<FlexfecSender> 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<FlexfecSender> MaybeCreateFlexfecSender(
 
   RTC_DCHECK_EQ(1U, config.rtp.flexfec.protected_media_ssrcs.size());
   return std::unique_ptr<FlexfecSender>(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 << '}';

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<uint32_t> protected_media_ssrcs;
+      } flexfec;
 
       // Settings for RTP retransmission payload format, see RFC 4588 for
       // details.