Add PacketRouterTest.Sanity_NoModuleRegistered_*

webrtc/modules/pacing/packet_router_unittest.cc

 /*
  *  Copyright (c) 2015 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 19 matching lines @@
 
 class PacketRouterTest : public ::testing::Test {
  public:
   PacketRouterTest() : packet_router_(new PacketRouter()) {}
  protected:
   static const int kProbeMinProbes = 5;
   static const int kProbeMinBytes = 1000;
   const std::unique_ptr<PacketRouter> packet_router_;
 };
 
+TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_TimeToSendPacket) {
+  PacketRouter packet_router;
+
+  constexpr uint16_t ssrc = 1234;
+  constexpr uint16_t sequence_number = 17;
+  constexpr uint64_t timestamp = 7890;
+  constexpr bool retransmission = false;
+  const PacedPacketInfo paced_info(1, kProbeMinProbes, kProbeMinBytes);
+
+  // TODO(eladalon): !!! Discuss with reviewers - it's interesting that this
+  // returns true even when no modules are found that match the SSRC.

[danilchap, 2017/07/28 12:25:48]

looking for documentation for the return value of the TimeToSendPacket... it is
declared in PacedSender::PacketSender:
"Returns false if packet cannot be sent."
which implies true when packet can.
That matches your discovery how return value is used.

So yes, return true when there are no module to send packet with looks like a
bug.

Though there is another problem: packet that failed to be sent
(PacedSender::Process) stays in queue, but packet that has no matching sender -
shouldn't.

So may be better to extend documentation for the TimeToSendPacket: "Returns
false if packet cannot be send now, retry later".
If sender for the packet is gone, updating budget might be wrong, but not a big
deal - those packets should be few.

[eladalon, 2017/07/29 11:27:10]

I'll use this CL to introduce the sanity tests, and keep a (non-!!!) TODO here
for it. I'll push a CL based on top of this one that will fix this issue.

We have two things left to discuss, then:
1. Whether to fix.
2. How to fix.

About #1, I think it's worth it, because there are several cases where this
would happen:
1.1. The aforementioned case of no module with the right SSRC (probably should
never happen).
1.2. When (!RTPSender::SendingMedia())
1.3. Packet somehow missing from packet history (queued for too long?).

As for the fix (#2), how about returning both success/failure as well as the
number of sent bytes? Success/failure can be treated as it is now, but for
updating budgets, only the number of sent bytes would be used.

Alternatively, we could use -1 for error and anything non-negative for
success-and-sent-bytes.

+  EXPECT_TRUE(packet_router.TimeToSendPacket(ssrc, sequence_number, timestamp,
+                                             retransmission, paced_info));
+}
+
 TEST_F(PacketRouterTest, TimeToSendPacket) {
   NiceMock<MockRtpRtcp> rtp_1;
   NiceMock<MockRtpRtcp> rtp_2;
   packet_router_->AddSendRtpModule(&rtp_1);
   packet_router_->AddSendRtpModule(&rtp_2);
 
   const uint16_t kSsrc1 = 1234;
   uint16_t sequence_number = 17;
   uint64_t timestamp = 7890;
   bool retransmission = false;
@@ skipping 420 matching lines @@
 
   // Lower the estimate to trigger a new packet REMB packet.
   EXPECT_CALL(rtp, SetREMBData(bitrate_estimate - 100, ssrcs)).Times(1);
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate - 100);
 
   EXPECT_CALL(rtp, SetREMBStatus(false)).Times(1);
   packet_router.RemoveReceiveRtpModule(&rtp);
 }
 
 }  // namespace webrtc