Add FlexfecReceiver unit test for infinite recovery loop.

webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc

Index: webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc
diff --git a/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc
index 508521da5dc487bdf1a3c3a5f0d9e4e0a72ba757..29c2fdf9fe8762ae953ddf6c32d473a53405d067 100644
--- a/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc
@@ -301,10 +301,17 @@ TEST_F(FlexfecReceiverTest, DoesNotCallbackTwice) {
           Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length)));
   receiver_.OnRtpPacket(ParsePacket(*packet_with_rtp_header));
 
-  // Receive FEC packet again.
+  // Receive the FEC packet again, but do not call back.
   receiver_.OnRtpPacket(ParsePacket(*packet_with_rtp_header));
 
-  // Do not call back again.
+  // Receive the first media packet again, but do not call back.
+  media_it = media_packets.begin();
+  receiver_.OnRtpPacket(ParsePacket(**media_it));
+
+  // Receive the second media packet again (the one recovered above),
+  // but do not call back again.
+  media_it++;

[danilchap, 2017/05/23 13:07:15]

nit: ++media_it;

[brandtr, 2017/05/23 13:19:17]

The style of this file is the other way :/

+  receiver_.OnRtpPacket(ParsePacket(**media_it));
 }
 
 // Here we are implicitly assuming packet masks that are suitable for
@@ -464,6 +471,72 @@ TEST_F(FlexfecReceiverTest, RecoversWithMediaPacketsOutOfOrder) {
   }
 }
 
+// Recovered media packets may be fed back into the FlexfecReceiver by the
+// callback. This test ensures the idempotency of such a situation.
+TEST_F(FlexfecReceiverTest, RecoveryCallbackDoesNotLoopInfinitely) {
+  class LoopbackRecoveredPacketReceiver : public RecoveredPacketReceiver {
+   public:
+    const int kMaxRecursionDepth = 10;
+
+    LoopbackRecoveredPacketReceiver()
+        : receiver_(nullptr),
+          did_receive_call_back_(false),
+          recursion_depth_(0),
+          deep_recursion_(false) {}
+
+    void SetReceiver(FlexfecReceiver* receiver) { receiver_ = receiver; }
+    bool DidReceiveCallback() const { return did_receive_call_back_; }
+    bool DeepRecursion() const { return deep_recursion_; }
+
+    // Implements RecoveredPacketReceiver.
+    void OnRecoveredPacket(const uint8_t* packet, size_t length) {
+      RtpPacketReceived parsed_packet;
+      EXPECT_TRUE(parsed_packet.Parse(packet, length));
+
+      did_receive_call_back_ = true;
+
+      if (recursion_depth_ > kMaxRecursionDepth) {

[danilchap, 2017/05/23 13:07:14]

alternative approach:
ASSERT_LT(recursion_depth_, kMaxRecursionDepth);
that will return too prevening stack overflow, but will point to this line and
save from extra member and accessor

[brandtr, 2017/05/23 13:19:17]

Good idea! Done.

[nisse-webrtc, 2017/05/23 14:05:25]

ASSERT_* does some magic which works as intended only when used in the top-level
test function.

It might work, but in a unexpected way, because (1) ASSERT_* is more or less is
the same as EXPECT_* + a return statement on failure, and (2) the return type of
this function happens to be void. I'm not sure it's a good idea to rely on that.
If you really want to, add a comment to not mislead readers to think that
ASSERT_* outside of top-level works in general; there's already too much
confusion around this.

[brandtr, 2017/05/23 15:02:47]

Better avoid confusion and use the old approach then.

Do ASSERTs only work at the top-level because the tests have return type void?

+        deep_recursion_ = true;
+        return;
+      }
+      ++recursion_depth_;
+      RTC_DCHECK(receiver_);
+      receiver_->OnRtpPacket(parsed_packet);
+      --recursion_depth_;
+    }
+
+   private:
+    FlexfecReceiver* receiver_;
+    bool did_receive_call_back_;
+    int recursion_depth_;
+    bool deep_recursion_;
+  } loopback_recovered_packet_receiver;
+
+  // Feed recovered packets back into |receiver|.
+  FlexfecReceiver receiver(kFlexfecSsrc, kMediaSsrc,
+                           &loopback_recovered_packet_receiver);
+  loopback_recovered_packet_receiver.SetReceiver(&receiver);
+
+  const size_t kNumMediaPackets = 2;
+  const size_t kNumFecPackets = 1;
+
+  PacketList media_packets;
+  PacketizeFrame(kNumMediaPackets, 0, &media_packets);
+  std::list<Packet*> fec_packets = EncodeFec(media_packets, kNumFecPackets);
+
+  // Receive first media packet but drop second.
+  auto media_it = media_packets.begin();
+  receiver.OnRtpPacket(ParsePacket(**media_it));
+
+  // Receive FEC packet and verify that a packet was recovered.
+  auto fec_it = fec_packets.begin();
+  std::unique_ptr<Packet> packet_with_rtp_header =
+      packet_generator_.BuildFlexfecPacket(**fec_it);
+  receiver.OnRtpPacket(ParsePacket(*packet_with_rtp_header));
+  EXPECT_TRUE(loopback_recovered_packet_receiver.DidReceiveCallback());
+  EXPECT_FALSE(loopback_recovered_packet_receiver.DeepRecursion());
+}
+
 TEST_F(FlexfecReceiverTest, CalculatesNumberOfPackets) {
   const size_t kNumMediaPackets = 2;
   const size_t kNumFecPackets = 1;