From 43c31e7afe091fbf01d307f3ff338225e1e6d59e Mon Sep 17 00:00:00 2001
From: brandtr <brandtr@webrtc.org>
Date: Tue, 15 Nov 2016 05:26:45 -0800
Subject: [PATCH] Make configuration logic harsher in FlexfecReceiveStream.

Before this change, the configuration logic in FlexfecReceiveStream
tried to make unsupported configurations work, e.g., by dropping the
protection of some media streams when multiple media streams were
protected by a single FlexFEC stream. This CL makes the configuration logic
return more errors on such unsupported configurations.
This harmonizes the logic with the new configuration logic in
VideoSendStream, for the FlexfecSender.

BUG=webrtc:5654

Review-Url: https://codereview.webrtc.org/2499963002
Cr-Commit-Position: refs/heads/master@{#15083}
---
 webrtc/call/flexfec_receive_stream.cc         | 46 ++++++++++++-------
 webrtc/call/flexfec_receive_stream.h          |  2 +-
 .../call/flexfec_receive_stream_unittest.cc   | 14 ------
 3 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/webrtc/call/flexfec_receive_stream.cc b/webrtc/call/flexfec_receive_stream.cc
index fb078f4e7b..3d11403b10 100644
--- a/webrtc/call/flexfec_receive_stream.cc
+++ b/webrtc/call/flexfec_receive_stream.cc
@@ -25,26 +25,39 @@ std::string FlexfecReceiveStream::Stats::ToString(int64_t time_ms) const {
 namespace {
 
 // TODO(brandtr): Update this function when we support multistream protection.
-std::unique_ptr<FlexfecReceiver> MaybeUpdateConfigAndCreateFlexfecReceiver(
-    FlexfecReceiveStream::Config* config,
+std::unique_ptr<FlexfecReceiver> MaybeCreateFlexfecReceiver(
+    const FlexfecReceiveStream::Config& config,
     RecoveredPacketReceiver* recovered_packet_callback) {
-  if (config->protected_media_ssrcs.empty()) {
-    LOG(LS_ERROR) << "No protected media SSRC supplied. "
-                  << "This FlexfecReceiveStream will therefore be useless.";
+  if (config.flexfec_payload_type < 0) {
+    LOG(LS_WARNING) << "Invalid FlexFEC payload type given. "
+                    << "This FlexfecReceiveStream will therefore be useless.";
     return nullptr;
-  } else if (config->protected_media_ssrcs.size() > 1) {
+  }
+  RTC_DCHECK_GE(config.flexfec_payload_type, 0);
+  RTC_DCHECK_LE(config.flexfec_payload_type, 127);
+  if (config.flexfec_ssrc == 0) {
+    LOG(LS_WARNING) << "Invalid FlexFEC SSRC given. "
+                    << "This FlexfecReceiveStream will therefore be useless.";
+    return nullptr;
+  }
+  if (config.protected_media_ssrcs.empty()) {
+    LOG(LS_WARNING) << "No protected media SSRC supplied. "
+                    << "This FlexfecReceiveStream will therefore be useless.";
+    return nullptr;
+  }
+
+  if (config.protected_media_ssrcs.size() > 1) {
     LOG(LS_WARNING)
         << "The supplied FlexfecConfig contained multiple protected "
            "media streams, but our implementation currently only "
-           "supports protecting a single media stream. This "
-           "FlexfecReceiveStream will therefore only accept media "
-           "packets from the first supplied media stream, with SSRC "
-        << config->protected_media_ssrcs[0] << ".";
-    config->protected_media_ssrcs.resize(1);
+           "supports protecting a single media stream. "
+           "To avoid confusion, disabling FlexFEC completely.";
+    return nullptr;
   }
-  return std::unique_ptr<FlexfecReceiver>(new FlexfecReceiver(
-      config->flexfec_ssrc, config->protected_media_ssrcs[0],
-      recovered_packet_callback));
+  RTC_DCHECK_EQ(1U, config.protected_media_ssrcs.size());
+  return std::unique_ptr<FlexfecReceiver>(
+      new FlexfecReceiver(config.flexfec_ssrc, config.protected_media_ssrcs[0],
+                          recovered_packet_callback));
 }
 
 }  // namespace
@@ -56,9 +69,8 @@ FlexfecReceiveStream::FlexfecReceiveStream(
     RecoveredPacketReceiver* recovered_packet_callback)
     : started_(false),
       config_(configuration),
-      receiver_(MaybeUpdateConfigAndCreateFlexfecReceiver(
-          &config_,
-          recovered_packet_callback)) {
+      receiver_(
+          MaybeCreateFlexfecReceiver(config_, recovered_packet_callback)) {
   LOG(LS_INFO) << "FlexfecReceiveStream: " << config_.ToString();
 }
 
diff --git a/webrtc/call/flexfec_receive_stream.h b/webrtc/call/flexfec_receive_stream.h
index 19bb9f873b..4f7aee0a76 100644
--- a/webrtc/call/flexfec_receive_stream.h
+++ b/webrtc/call/flexfec_receive_stream.h
@@ -42,7 +42,7 @@ class FlexfecReceiveStream : public webrtc::FlexfecReceiveStream {
   rtc::CriticalSection crit_;
   bool started_ GUARDED_BY(crit_);
 
-  Config config_;
+  const Config config_;
   const std::unique_ptr<FlexfecReceiver> receiver_;
 };
 
diff --git a/webrtc/call/flexfec_receive_stream_unittest.cc b/webrtc/call/flexfec_receive_stream_unittest.cc
index a7a3158ee5..a41b2a54e8 100644
--- a/webrtc/call/flexfec_receive_stream_unittest.cc
+++ b/webrtc/call/flexfec_receive_stream_unittest.cc
@@ -54,20 +54,6 @@ TEST(FlexfecReceiveStreamTest, DoesNotProcessPacketWhenNoMediaSsrcGiven) {
       receive_stream.AddAndProcessReceivedPacket(packet, packet_length));
 }
 
-// TODO(brandtr): Remove when we support multistream protection.
-TEST(FlexfecReceiveStreamTest, CannotProtectMultipleMediaStreams) {
-  FlexfecReceiveStream::Config config;
-  config.flexfec_payload_type = 118;
-  config.flexfec_ssrc = 424223;
-  config.protected_media_ssrcs = {123, 456};
-  MockRecoveredPacketReceiver callback;
-  internal::FlexfecReceiveStream receive_stream(config, &callback);
-
-  ASSERT_EQ(1U, receive_stream.config().protected_media_ssrcs.size());
-  EXPECT_EQ(config.protected_media_ssrcs[0],
-            receive_stream.config().protected_media_ssrcs[0]);
-}
-
 // Create a FlexFEC packet that protects a single media packet and ensure
 // that the callback is called. Correctness of recovery is checked in the
 // FlexfecReceiver unit tests.