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..f74466576355c757e9610a237ed211064a350c0a 100644
--- a/webrtc/video/end_to_end_tests.cc
+++ b/webrtc/video/end_to_end_tests.cc
@@ -704,84 +704,125 @@ 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:
+  explicit FlexfecRenderObserver(bool expect_flexfec_rtcp)
+      : test::EndToEndTest(test::CallTest::kDefaultTimeoutMs),
+        expect_flexfec_rtcp_(expect_flexfec_rtcp),
+        received_flexfec_rtcp_(false),
+        random_(0xcafef00d1) {}
 
-    size_t GetNumFlexfecStreams() const override { return 1; }
+  size_t GetNumFlexfecStreams() const override { return 1; }
 
-   private:
-    Action OnSendRtp(const uint8_t* packet, size_t length) override {
-      rtc::CritScope lock(&crit_);
-      RTPHeader header;
-      EXPECT_TRUE(parser_->Parse(packet, length, &header));
+  static constexpr uint32_t kVideoLocalSsrc = 123;

[danilchap, 2017/01/10 13:48:17]

constants above constructors

[brandtr, 2017/01/11 09:55:24]

Done.

+  static constexpr uint32_t kFlexfecLocalSsrc = 456;
 
-      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;
+  }
+
+  Action OnReceiveRtcp(const uint8_t* data, size_t length) override {

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

This method is new.

+    if (length < 32U) {
+      // No report block, or truncated report block.
+      return SEND_PACKET;
     }
 
-    void ModifyVideoConfigs(
-        VideoSendStream::Config* send_config,
-        std::vector<VideoReceiveStream::Config>* receive_configs,
-        VideoEncoderConfig* encoder_config) override {
-      (*receive_configs)[0].renderer = this;
+    uint32_t rtcp_sender_ssrc = ByteReader<uint32_t>::ReadBigEndian(&data[4]);

[danilchap, 2017/01/10 13:48:17]

avoid reimplmenting (simplifed) rtcp parser:
in test code you can use RtcpPacketParser from test/rtcp_packet_parser.h

it has sender_ssrc() accessor

[brandtr, 2017/01/11 09:55:24]

Done. See below.

+    if (rtcp_sender_ssrc == kFlexfecLocalSsrc) {
+      received_flexfec_rtcp_ = true;
+
+      uint8_t payload_type = data[1];
+      EXPECT_EQ(201U, payload_type);  // RTCP RR.
+      uint32_t reported_ssrc = ByteReader<uint32_t>::ReadBigEndian(&data[8]);

[danilchap, 2017/01/10 13:48:17]

test_parser->receiver_report()->report_blocks().front().source_ssrc()
with some natural expectation above to have nice error messages if something
will be wrong.

[brandtr, 2017/01/11 09:55:24]

Hmm, OK. I did have a look at that class before writing this code, but it looked
to me that it was only about counting stuff. But now when I look more carefully,
I see how the inheritance is set up.

Done.

+      EXPECT_EQ(test::CallTest::kFlexfecSendSsrc, reported_ssrc);
     }
+    return SEND_PACKET;
+  }
 
-    void PerformTest() override {
-      EXPECT_TRUE(Wait())
-          << "Timed out waiting for dropped frames to be rendered.";
+  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);
 }