Delete member VideoReceiveStream::Config::Rtp::ulpfec.

video/end_to_end_tests.cc

 /*
  *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 #include <algorithm>
@@ skipping 678 matching lines @@
       send_config->encoder_settings.payload_type = kVideoSendPayloadType;
       VideoReceiveStream::Decoder decoder =
           test::CreateMatchingDecoder(send_config->encoder_settings);
       decoder_.reset(decoder.decoder);
       (*receive_configs)[0].decoders.clear();
       (*receive_configs)[0].decoders.push_back(decoder);
 
       // Enable ULPFEC over RED.
       send_config->rtp.ulpfec.red_payload_type = kRedPayloadType;
       send_config->rtp.ulpfec.ulpfec_payload_type = kUlpfecPayloadType;
-      (*receive_configs)[0].rtp.ulpfec.red_payload_type = kRedPayloadType;
-      (*receive_configs)[0].rtp.ulpfec.ulpfec_payload_type = kUlpfecPayloadType;
+      (*receive_configs)[0].rtp.red_payload_type = kRedPayloadType;
+      (*receive_configs)[0].rtp.ulpfec_payload_type = kUlpfecPayloadType;
 
       (*receive_configs)[0].renderer = this;
     }
 
     void PerformTest() override {
       EXPECT_TRUE(Wait())
           << "Timed out waiting for dropped frames to be rendered.";
     }
 
     rtc::CriticalSection crit_;
@@ skipping 135 matching lines @@
 
   void ModifyVideoConfigs(
       VideoSendStream::Config* send_config,
       std::vector<VideoReceiveStream::Config>* receive_configs,
       VideoEncoderConfig* encoder_config) override {
     (*receive_configs)[0].rtp.local_ssrc = kVideoLocalSsrc;
     (*receive_configs)[0].renderer = this;
 
     if (enable_nack_) {
       send_config->rtp.nack.rtp_history_ms = test::CallTest::kNackRtpHistoryMs;
       send_config->rtp.ulpfec.red_rtx_payload_type =

[brandtr, 2017/09/21 08:11:33]

RED should not be used in this test, so you can remove this line too?

[nisse-webrtc, 2017/09/21 08:37:46]

Done.

           test::CallTest::kRtxRedPayloadType;
       send_config->rtp.rtx.ssrcs.push_back(test::CallTest::kSendRtxSsrcs[0]);
       send_config->rtp.rtx.payload_type = test::CallTest::kSendRtxPayloadType;
 
       (*receive_configs)[0].rtp.nack.rtp_history_ms =
           test::CallTest::kNackRtpHistoryMs;
-      (*receive_configs)[0].rtp.ulpfec.red_rtx_payload_type =
-          test::CallTest::kRtxRedPayloadType;
-
       (*receive_configs)[0].rtp.rtx_ssrc = test::CallTest::kSendRtxSsrcs[0];
       (*receive_configs)[0]
           .rtp
           .rtx_associated_payload_types[test::CallTest::kSendRtxPayloadType] =
           test::CallTest::kVideoSendPayloadType;
     }
   }
 
   void OnFrameGeneratorCapturerCreated(
       test::FrameGeneratorCapturer* frame_generator_capturer) override {
@@ skipping 160 matching lines @@
       // Configure hybrid NACK/FEC.
       send_config->rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
       send_config->rtp.ulpfec.red_payload_type = kRedPayloadType;
       send_config->rtp.ulpfec.ulpfec_payload_type = kUlpfecPayloadType;
       // Set codec to VP8, otherwise NACK/FEC hybrid will be disabled.
       send_config->encoder_settings.encoder = encoder_.get();
       send_config->encoder_settings.payload_name = "VP8";
       send_config->encoder_settings.payload_type = kFakeVideoSendPayloadType;
 
       (*receive_configs)[0].rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
-      (*receive_configs)[0].rtp.ulpfec.red_payload_type = kRedPayloadType;
-      (*receive_configs)[0].rtp.ulpfec.ulpfec_payload_type = kUlpfecPayloadType;
+      (*receive_configs)[0].rtp.red_payload_type = kRedPayloadType;
+      (*receive_configs)[0].rtp.ulpfec_payload_type = kUlpfecPayloadType;
 
       (*receive_configs)[0].decoders.resize(1);
       (*receive_configs)[0].decoders[0].payload_type =
           send_config->encoder_settings.payload_type;
       (*receive_configs)[0].decoders[0].payload_name =
           send_config->encoder_settings.payload_name;
       (*receive_configs)[0].decoders[0].decoder = decoder_.get();
     }
 
     void PerformTest() override {
@@ skipping 106 matching lines @@
       (*receive_configs)[0].disable_prerenderer_smoothing = true;
       (*receive_configs)[0].renderer = this;
 
       (*receive_configs)[0].rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
 
       if (payload_type_ == kRedPayloadType) {
         send_config->rtp.ulpfec.ulpfec_payload_type = kUlpfecPayloadType;
         send_config->rtp.ulpfec.red_payload_type = kRedPayloadType;
         if (retransmission_ssrc_ == kSendRtxSsrcs[0])
           send_config->rtp.ulpfec.red_rtx_payload_type = kRtxRedPayloadType;
-        (*receive_configs)[0].rtp.ulpfec.ulpfec_payload_type =
+        (*receive_configs)[0].rtp.ulpfec_payload_type =
             send_config->rtp.ulpfec.ulpfec_payload_type;
-        (*receive_configs)[0].rtp.ulpfec.red_payload_type =
+        (*receive_configs)[0].rtp.red_payload_type =
             send_config->rtp.ulpfec.red_payload_type;
-        (*receive_configs)[0].rtp.ulpfec.red_rtx_payload_type =
-            send_config->rtp.ulpfec.red_rtx_payload_type;
       }
 
       if (retransmission_ssrc_ == kSendRtxSsrcs[0]) {
         send_config->rtp.rtx.ssrcs.push_back(kSendRtxSsrcs[0]);
         send_config->rtp.rtx.payload_type = kSendRtxPayloadType;
         (*receive_configs)[0].rtp.rtx_ssrc = kSendRtxSsrcs[0];
         (*receive_configs)[0]
             .rtp.rtx_associated_payload_types[(payload_type_ == kRedPayloadType)
                                                   ? kRtxRedPayloadType
                                                   : kSendRtxPayloadType] =
@@ skipping 10 matching lines @@
     void OnFrameGeneratorCapturerCreated(
         test::FrameGeneratorCapturer* frame_generator_capturer) override {
       frame_generator_capturer->SetFakeRotation(kVideoRotation_90);
     }
 
     void PerformTest() override {
       EXPECT_TRUE(Wait())
           << "Timed out while waiting for retransmission to render.";
     }
 
     int GetPayloadType(bool use_rtx, bool use_red) {

[nisse-webrtc, 2017/09/21 08:57:29]

I'm doing a search-and-replace then, changing to use_fec here too.

       if (use_red) {
         if (use_rtx)
           return kRtxRedPayloadType;
         return kRedPayloadType;
       }
       if (use_rtx)
         return kSendRtxPayloadType;
       return kFakeVideoSendPayloadType;
     }
 
@@ skipping 1472 matching lines @@
   RunBaseTest(&test);
 
   EXPECT_EQ(
       1, metrics::NumSamples("WebRTC.Video.UniqueNackRequestsSentInPercent"));
   EXPECT_EQ(1, metrics::NumSamples(
                    "WebRTC.Video.UniqueNackRequestsReceivedInPercent"));
   EXPECT_GT(metrics::MinSample("WebRTC.Video.NackPacketsSentPerMinute"), 0);
 }
 
 void EndToEndTest::VerifyHistogramStats(bool use_rtx,
                                         bool use_red,

[brandtr, 2017/09/21 08:11:33]

Would you mind renaming |use_red| to |use_fec| ? Because that's how the flag is
actually used.

[nisse-webrtc, 2017/09/21 08:37:46]

Just in this method and its local classes, or globally in this file?

[brandtr, 2017/09/21 08:45:22]

I think they are equivalent: I have already tried to remove all uses of "RED"
when what is actually meant is "RED+ULPFEC" in this file. But apparently I
missed this particular test.

                                         bool screenshare) {
   class StatsObserver : public test::EndToEndTest,
                         public rtc::VideoSinkInterface<VideoFrame> {
    public:
     StatsObserver(bool use_rtx, bool use_red, bool screenshare)
         : EndToEndTest(kLongTimeoutMs),
           use_rtx_(use_rtx),
           use_red_(use_red),
           screenshare_(screenshare),
           // This test uses NACK, so to send FEC we can't use a fake encoder.
@@ skipping 32 matching lines @@
       int64_t elapsed_sec = (now - start_runtime_ms_) / 1000;
       return elapsed_sec > metrics::kMinRunTimeInSeconds * 2;
     }
 
     bool MinNumberOfFramesReceived() const {
       const int kMinRequiredHistogramSamples = 200;
       rtc::CritScope lock(&crit_);
       return num_frames_received_ > kMinRequiredHistogramSamples;
     }
 
     void ModifyVideoConfigs(

[brandtr, 2017/09/21 08:11:33]

It doesn't look like this test sets ups RED/RTX correctly, when both are used.
Can you add the correct configs?

[nisse-webrtc, 2017/09/21 08:37:46]

I've given it a try, please have a look.

         VideoSendStream::Config* send_config,
         std::vector<VideoReceiveStream::Config>* receive_configs,
         VideoEncoderConfig* encoder_config) override {
       // NACK
       send_config->rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
       (*receive_configs)[0].rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
       (*receive_configs)[0].renderer = this;
       // FEC
       if (use_red_) {
         send_config->rtp.ulpfec.ulpfec_payload_type = kUlpfecPayloadType;
         send_config->rtp.ulpfec.red_payload_type = kRedPayloadType;
         send_config->encoder_settings.encoder = vp8_encoder_.get();
         send_config->encoder_settings.payload_name = "VP8";
         (*receive_configs)[0].decoders[0].payload_name = "VP8";
-        (*receive_configs)[0].rtp.ulpfec.red_payload_type = kRedPayloadType;
-        (*receive_configs)[0].rtp.ulpfec.ulpfec_payload_type =
-            kUlpfecPayloadType;
+        (*receive_configs)[0].rtp.red_payload_type = kRedPayloadType;
+        (*receive_configs)[0].rtp.ulpfec_payload_type = kUlpfecPayloadType;
       }
       // RTX
       if (use_rtx_) {
         send_config->rtp.rtx.ssrcs.push_back(kSendRtxSsrcs[0]);
         send_config->rtp.rtx.payload_type = kSendRtxPayloadType;
         (*receive_configs)[0].rtp.rtx_ssrc = kSendRtxSsrcs[0];
         (*receive_configs)[0]
             .rtp.rtx_associated_payload_types[kSendRtxPayloadType] =
             kFakeVideoSendPayloadType;
       }
@@ skipping 2047 matching lines @@
       default_receive_config.rtp.rtcp_xr.receiver_reference_time_report)
       << "RTCP XR settings require rtcp-xr to be negotiated.";
   EXPECT_EQ(0U, default_receive_config.rtp.rtx_ssrc)
       << "Enabling RTX requires ssrc-group: FID negotiation";
   EXPECT_TRUE(default_receive_config.rtp.rtx_associated_payload_types.empty())
       << "Enabling RTX requires rtpmap: rtx negotiation.";
   EXPECT_TRUE(default_receive_config.rtp.extensions.empty())
       << "Enabling RTP extensions require negotiation.";
 
   VerifyEmptyNackConfig(default_receive_config.rtp.nack);
-  VerifyEmptyUlpfecConfig(default_receive_config.rtp.ulpfec);
+  EXPECT_EQ(-1, default_receive_config.rtp.ulpfec_payload_type)
+      << "Enabling ULPFEC requires rtpmap: ulpfec negotiation.";
+  EXPECT_EQ(-1, default_receive_config.rtp.red_payload_type)
+      << "Enabling ULPFEC requires rtpmap: red negotiation.";
 }
 
 TEST_F(EndToEndTest, VerifyDefaultFlexfecReceiveConfigParameters) {
   test::NullTransport rtcp_send_transport;
   FlexfecReceiveStream::Config default_receive_config(&rtcp_send_transport);
   EXPECT_EQ(-1, default_receive_config.payload_type)
       << "Enabling FlexFEC requires rtpmap: flexfec negotiation.";
   EXPECT_EQ(0U, default_receive_config.remote_ssrc)
       << "Enabling FlexFEC requires ssrc-group: FEC-FR negotiation.";
   EXPECT_TRUE(default_receive_config.protected_media_ssrcs.empty())
@@ skipping 167 matching lines @@
     std::unique_ptr<VideoEncoder> encoder_;
     std::unique_ptr<VideoDecoder> decoder_;
     rtc::CriticalSection crit_;
     int recorded_frames_ RTC_GUARDED_BY(crit_);
   } test(this);
 
   RunBaseTest(&test);
 }
 
 }  // namespace webrtc