Add FlexfecReceiver unit test for infinite recovery loop.

webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc

 /*
  *  Copyright (c) 2016 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.
  */
 
@@ skipping 283 matching lines @@
   auto fec_it = fec_packets.begin();
   std::unique_ptr<Packet> packet_with_rtp_header =
       packet_generator_.BuildFlexfecPacket(**fec_it);
   media_it++;
   EXPECT_CALL(recovered_packet_receiver_,
               OnRecoveredPacket(_, (*media_it)->length))
       .With(
           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
 // this type of 50% correlated loss. If we are changing our precomputed
 // packet masks, this test might need to be updated.
 TEST_F(FlexfecReceiverTest, RecoversFrom50PercentLoss) {
   const size_t kNumFecPackets = 5;
   const size_t kNumFrames = 2 * kNumFecPackets;
   const size_t kNumMediaPacketsPerFrame = 1;
 
@@ skipping 139 matching lines @@
   // Add FEC packets.
   auto fec_it = fec_packets.begin();
   std::unique_ptr<Packet> packet_with_rtp_header;
   while (fec_it != fec_packets.end()) {
     packet_with_rtp_header = packet_generator_.BuildFlexfecPacket(**fec_it);
     receiver_.OnRtpPacket(ParsePacket(*packet_with_rtp_header));
     ++fec_it;
   }
 }
 
+// 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;
 
   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();
@@ skipping 11 matching lines @@
   receiver_.OnRtpPacket(ParsePacket(*packet_with_rtp_header));
 
   // Check stats calculations.
   FecPacketCounter packet_counter = receiver_.GetPacketCounter();
   EXPECT_EQ(2U, packet_counter.num_packets);
   EXPECT_EQ(1U, packet_counter.num_fec_packets);
   EXPECT_EQ(1U, packet_counter.num_recovered_packets);
 }
 
 }  // namespace webrtc