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..39ee86b69b3a6890bcd52e9906f7ee79cf1e484f 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++;
+  receiver_.OnRtpPacket(ParsePacket(**media_it));
 }
 
 // Here we are implicitly assuming packet masks that are suitable for
@@ -464,6 +471,58 @@ TEST_F(FlexfecReceiverTest, RecoversWithMediaPacketsOutOfOrder) {
   }
 }
 
+// Recovered media packet 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:
+    LoopbackRecoveredPacketReceiver()
+        : receiver_(nullptr), did_receive_call_back_(false) {}
+
+    void SetReceiver(FlexfecReceiver* receiver) { receiver_ = receiver; }
+
+    bool DidReceiveCallback() { return did_receive_call_back_; }
+
+    // Implements RecoveredPacketReceiver.
+    void OnRecoveredPacket(const uint8_t* packet, size_t length) {

[nisse-webrtc, 2017/05/22 10:39:05]

Can you add the recursion counter here? Something like

if (count_ > 10) {
  deep_recursion_ = true;
  return;
}
++count_;
receiver_->OnRtpPacket(parsed_packet);
--count;

together with an 

EXPECT_FALSE(loopback_recovered_packet_receiver.deep_recursion_);

at the end of the test.

[brandtr, 2017/05/22 11:52:39]

Yes, that makes a lot of sense! I forgot that I had access to the recursion
chain here... :)

+      RtpPacketReceived parsed_packet;
+      EXPECT_TRUE(parsed_packet.Parse(packet, length));
+      RTC_DCHECK(receiver_);
+      receiver_->OnRtpPacket(parsed_packet);
+      did_receive_call_back_ = true;
+    }
+
+   private:
+    FlexfecReceiver* receiver_;
+    bool did_receive_call_back_;
+  } 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());
+
+  // No extra callback, and thus no infinite loop.

[nisse-webrtc, 2017/05/22 08:39:20]

If this test fails (by undoing the fix in the other cl), it will fail by running
out of stack space and crash, right? Would it make sense to add some recursion
counter, and fail with a more user-friendly message?

[brandtr, 2017/05/22 09:10:09]

Yes, I this test crashes by undoing your change.

Is there a way to throw a nice error message here without adding a recursion
counter inside the FlexfecReceiver? If not, I'm not sure if it is worth it?

The logic to check that duplicate packets are not accepted and that recovered
packets are only sent to the callback once already exists in
ForwardErrorCorrection and FlexfecReceiver, respectively. (The latter was fixed
by you.)

[danilchap, 2017/05/22 09:17:28]

did you try EXPECT_DEATH ? (not sure it would in this case)

[danilchap, 2017/05/22 09:18:22]

nvm... it is for opposite case.

[nisse-webrtc, 2017/05/22 10:39:05]

I agree it's worth doing only if it can be done by some hacking in the test code
only.

+}
+
 TEST_F(FlexfecReceiverTest, CalculatesNumberOfPackets) {
   const size_t kNumMediaPackets = 2;
   const size_t kNumFecPackets = 1;