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}
diff --git a/webrtc/config.cc b/webrtc/config.cc
index cb6dcb2..6ffd1c3 100644
--- a/webrtc/config.cc
+++ b/webrtc/config.cc
@@ -37,44 +37,6 @@
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 e0883e6..22b279c 100644
--- a/webrtc/config.h
+++ b/webrtc/config.h
@@ -56,28 +56,6 @@
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) {}
diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc
index 14e72d5..996e2bd 100644
--- a/webrtc/media/engine/webrtcvideoengine2.cc
+++ b/webrtc/media/engine/webrtcvideoengine2.cc
@@ -1589,7 +1589,7 @@
}
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 @@
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 7d80a0d..bf01767 100644
--- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc
+++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc
@@ -2346,7 +2346,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 2b7d2e7..290992a 100644
--- a/webrtc/test/call_test.cc
+++ b/webrtc/test/call_test.cc
@@ -231,8 +231,8 @@
// 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 367b74c..946d81d 100644
--- a/webrtc/video/end_to_end_tests.cc
+++ b/webrtc/video/end_to_end_tests.cc
@@ -3864,10 +3864,11 @@
<< "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 bd1441f..98b2b1d 100644
--- a/webrtc/video/send_statistics_proxy.cc
+++ b/webrtc/video/send_statistics_proxy.cc
@@ -365,7 +365,7 @@
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 @@
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 7fa3ae3..915b8df 100644
--- a/webrtc/video/send_statistics_proxy_unittest.cc
+++ b/webrtc/video/send_statistics_proxy_unittest.cc
@@ -75,8 +75,8 @@
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 e37b555..e3ae616 100644
--- a/webrtc/video/video_quality_test.cc
+++ b/webrtc/video/video_quality_test.cc
@@ -1117,9 +1117,8 @@
// 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 41f8b3f..3b4238c 100644
--- a/webrtc/video/video_send_stream.cc
+++ b/webrtc/video/video_send_stream.cc
@@ -94,12 +94,12 @@
// 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 @@
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 @@
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 fb5e9d8..680eb02 100644
--- a/webrtc/video_send_stream.h
+++ b/webrtc/video_send_stream.h
@@ -138,10 +138,21 @@
// 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.
