Add PacketRouterTest.Sanity_NoModuleRegistered_*

webrtc/modules/pacing/packet_router_unittest.cc

Index: webrtc/modules/pacing/packet_router_unittest.cc
diff --git a/webrtc/modules/pacing/packet_router_unittest.cc b/webrtc/modules/pacing/packet_router_unittest.cc
index e7f08ca9b5b51684e898a34ab611bcf9519dac3d..47a2b18fa9f6d9558260b2c155aadf07b27c48ad 100644
--- a/webrtc/modules/pacing/packet_router_unittest.cc
+++ b/webrtc/modules/pacing/packet_router_unittest.cc
@@ -37,6 +37,21 @@ class PacketRouterTest : public ::testing::Test {
   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;