Let FlexfecReceiveStreamImpl send RTCP RRs.

webrtc/video/end_to_end_tests.cc

Index: webrtc/video/end_to_end_tests.cc
diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc
index 2c71e3951785cee43b0b5faa0165afd5c90d59d2..6c3266a7bccc6ac3058e079e99bf49bcfba3130e 100644
--- a/webrtc/video/end_to_end_tests.cc
+++ b/webrtc/video/end_to_end_tests.cc
@@ -704,84 +704,127 @@ TEST_P(EndToEndTest, CanReceiveUlpfec) {
   RunBaseTest(&test);
 }
 
-TEST_P(EndToEndTest, CanReceiveFlexfec) {
-  class FlexfecRenderObserver : public test::EndToEndTest,
-                                public rtc::VideoSinkInterface<VideoFrame> {
-   public:
-    FlexfecRenderObserver()
-        : EndToEndTest(kDefaultTimeoutMs), random_(0xcafef00d1) {}
+class FlexfecRenderObserver : public test::EndToEndTest,
+                              public rtc::VideoSinkInterface<VideoFrame> {
+ public:
+  static constexpr uint32_t kVideoLocalSsrc = 123;
+  static constexpr uint32_t kFlexfecLocalSsrc = 456;
 
-    size_t GetNumFlexfecStreams() const override { return 1; }
+  explicit FlexfecRenderObserver(bool expect_flexfec_rtcp)
+      : test::EndToEndTest(test::CallTest::kDefaultTimeoutMs),
+        expect_flexfec_rtcp_(expect_flexfec_rtcp),
+        received_flexfec_rtcp_(false),
+        random_(0xcafef00d1) {}
 
-   private:
-    Action OnSendRtp(const uint8_t* packet, size_t length) override {
-      rtc::CritScope lock(&crit_);
-      RTPHeader header;
-      EXPECT_TRUE(parser_->Parse(packet, length, &header));
+  size_t GetNumFlexfecStreams() const override { return 1; }
 
-      uint8_t payload_type = header.payloadType;
-      if (payload_type != kFakeVideoSendPayloadType) {
-        EXPECT_EQ(kFlexfecPayloadType, payload_type);
-      }
+ private:
+  Action OnSendRtp(const uint8_t* packet, size_t length) override {
+    rtc::CritScope lock(&crit_);
+    RTPHeader header;
+    EXPECT_TRUE(parser_->Parse(packet, length, &header));
 
-      // Is this a retransmitted media packet? From the perspective of FEC, this
-      // packet is then no longer dropped, so remove it from the list of
-      // dropped packets.
-      if (payload_type == kFakeVideoSendPayloadType) {
-        auto seq_num_it = dropped_sequence_numbers_.find(header.sequenceNumber);
-        if (seq_num_it != dropped_sequence_numbers_.end()) {
-          dropped_sequence_numbers_.erase(seq_num_it);
-          auto ts_it = dropped_timestamps_.find(header.timestamp);
-          EXPECT_NE(ts_it, dropped_timestamps_.end());
-          dropped_timestamps_.erase(ts_it);
+    uint8_t payload_type = header.payloadType;
+    if (payload_type != test::CallTest::kFakeVideoSendPayloadType) {
+      EXPECT_EQ(test::CallTest::kFlexfecPayloadType, payload_type);
+    }
 
-          return SEND_PACKET;
-        }
-      }
+    // Is this a retransmitted media packet? From the perspective of FEC, this
+    // packet is then no longer dropped, so remove it from the list of
+    // dropped packets.
+    if (payload_type == test::CallTest::kFakeVideoSendPayloadType) {
+      auto seq_num_it = dropped_sequence_numbers_.find(header.sequenceNumber);
+      if (seq_num_it != dropped_sequence_numbers_.end()) {
+        dropped_sequence_numbers_.erase(seq_num_it);
+        auto ts_it = dropped_timestamps_.find(header.timestamp);
+        EXPECT_NE(ts_it, dropped_timestamps_.end());
+        dropped_timestamps_.erase(ts_it);
 
-      // Simulate 5% packet loss. Record what media packets, and corresponding
-      // timestamps, that were dropped.
-      if (random_.Rand(1, 100) <= 5) {
-        if (payload_type == kFakeVideoSendPayloadType) {
-          dropped_sequence_numbers_.insert(header.sequenceNumber);
-          dropped_timestamps_.insert(header.timestamp);
-        }
+        return SEND_PACKET;
+      }
+    }
 
-        return DROP_PACKET;
+    // Simulate 5% packet loss. Record what media packets, and corresponding
+    // timestamps, that were dropped.
+    if (random_.Rand(1, 100) <= 5) {
+      if (payload_type == test::CallTest::kFakeVideoSendPayloadType) {
+        dropped_sequence_numbers_.insert(header.sequenceNumber);
+        dropped_timestamps_.insert(header.timestamp);
       }
 
-      return SEND_PACKET;
+      return DROP_PACKET;
     }
 
-    void OnFrame(const VideoFrame& video_frame) override {
-      rtc::CritScope lock(&crit_);
-      // Rendering frame with timestamp of packet that was dropped -> FEC
-      // protection worked.
-      auto it = dropped_timestamps_.find(video_frame.timestamp());
-      if (it != dropped_timestamps_.end())
-        observation_complete_.Set();
-    }
+    return SEND_PACKET;
+  }
 
-    void ModifyVideoConfigs(
-        VideoSendStream::Config* send_config,
-        std::vector<VideoReceiveStream::Config>* receive_configs,
-        VideoEncoderConfig* encoder_config) override {
-      (*receive_configs)[0].renderer = this;
+  Action OnReceiveRtcp(const uint8_t* data, size_t length) override {
+    test::RtcpPacketParser parser;
+
+    parser.Parse(data, length);
+    if (parser.sender_ssrc() == kFlexfecLocalSsrc) {
+      received_flexfec_rtcp_ = true;

[danilchap, 2017/01/11 12:13:07]

probably set to true only if (!report_blocks.empty())

[brandtr, 2017/01/11 12:25:36]

Yes, this is better, as it requires the EXPECTs below to always run at least
once.

+
+      EXPECT_EQ(1, parser.receiver_report()->num_packets());
+      const std::vector<rtcp::ReportBlock>& report_blocks =
+          parser.receiver_report()->report_blocks();
+      if (!report_blocks.empty()) {

[brandtr, 2017/01/11 10:47:52]

This check is needed because the newly created RTP module spits out one or two
empty RTCP packets, before sending proper RTCP RRs for FlexFEC.

These empty RTCP packets seem to be sent even if no RTP statistics are provided
to the RTP module. Is this correct/reasonable behaviour of the RTP module?

[danilchap, 2017/01/11 12:13:07]

should be correct, not so sure if reasonable.

quote from rfc3550 section 6.4.2: "An empty RR packet (RC = 0) MUST be put at
the head of a compound RTCP packet when there is no data transmission or
reception to report."

[brandtr, 2017/01/11 12:25:36]

Acknowledged.

+        EXPECT_EQ(1U, report_blocks.size());
+        EXPECT_EQ(test::CallTest::kFlexfecSendSsrc,
+                  report_blocks[0].source_ssrc());
+      }
     }
 
-    void PerformTest() override {
-      EXPECT_TRUE(Wait())
-          << "Timed out waiting for dropped frames to be rendered.";
+    return SEND_PACKET;
+  }
+
+  void OnFrame(const VideoFrame& video_frame) override {
+    rtc::CritScope lock(&crit_);
+    // Rendering frame with timestamp of packet that was dropped -> FEC
+    // protection worked.
+    auto it = dropped_timestamps_.find(video_frame.timestamp());
+    if (it != dropped_timestamps_.end()) {
+      if (!expect_flexfec_rtcp_ || received_flexfec_rtcp_) {
+        observation_complete_.Set();
+      }
     }
+  }
 
-    rtc::CriticalSection crit_;
-    std::set<uint32_t> dropped_sequence_numbers_ GUARDED_BY(crit_);
-    // Since several packets can have the same timestamp a multiset is used
-    // instead of a set.
-    std::multiset<uint32_t> dropped_timestamps_ GUARDED_BY(crit_);
-    Random random_;
-  } test;
+  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;
+  }
+
+  void ModifyFlexfecConfigs(
+      std::vector<FlexfecReceiveStream::Config>* receive_configs) override {
+    (*receive_configs)[0].local_ssrc = kFlexfecLocalSsrc;
+  }
+
+  void PerformTest() override {
+    EXPECT_TRUE(Wait())
+        << "Timed out waiting for dropped frames to be rendered.";
+  }
+
+  rtc::CriticalSection crit_;
+  std::set<uint32_t> dropped_sequence_numbers_ GUARDED_BY(crit_);
+  // Since several packets can have the same timestamp a multiset is used
+  // instead of a set.
+  std::multiset<uint32_t> dropped_timestamps_ GUARDED_BY(crit_);
+  bool expect_flexfec_rtcp_;
+  bool received_flexfec_rtcp_;
+  Random random_;
+};
+
+TEST_P(EndToEndTest, ReceivesFlexfec) {
+  FlexfecRenderObserver test(false);
+  RunBaseTest(&test);
+}
 
+TEST_P(EndToEndTest, ReceivesFlexfecAndSendsCorrespondingRtcp) {
+  FlexfecRenderObserver test(true);
   RunBaseTest(&test);
 }