SSRC and RSID may only refer to one sink each in RtpDemuxer

webrtc/call/rtp_demuxer_unittest.cc

Index: webrtc/call/rtp_demuxer_unittest.cc
diff --git a/webrtc/call/rtp_demuxer_unittest.cc b/webrtc/call/rtp_demuxer_unittest.cc
index 6d61f57894d9128b0e4f51df450f0913626f6d89..848dc78809109470ef3afa6c021d20d9c5671dff 100644
--- a/webrtc/call/rtp_demuxer_unittest.cc
+++ b/webrtc/call/rtp_demuxer_unittest.cc
@@ -81,6 +81,17 @@ std::unique_ptr<RtpPacketReceived> CreateRtpPacketReceivedWithRsid(
   return packet;
 }
 
+TEST(RtpDemuxerTest, CanAddSinkBySsrc) {
+  RtpDemuxer demuxer;
+  MockRtpPacketSink sink;
+  constexpr uint32_t ssrc = 1;
+
+  EXPECT_TRUE(demuxer.AddSink(ssrc, &sink));
+
+  // Test tear-down
+  demuxer.RemoveSink(&sink);
+}
+
 TEST(RtpDemuxerTest, OnRtpPacketCalledOnCorrectSinkBySsrc) {
   RtpDemuxer demuxer;
 
@@ -149,29 +160,6 @@ TEST(RtpDemuxerTest, PacketsDeliveredInRightOrder) {
   demuxer.RemoveSink(&sink);
 }
 
-TEST(RtpDemuxerTest, MultipleSinksMappedToSameSsrc) {
-  RtpDemuxer demuxer;
-
-  MockRtpPacketSink sinks[3];
-  constexpr uint32_t ssrc = 404;
-  for (auto& sink : sinks) {
-    demuxer.AddSink(ssrc, &sink);
-  }
-
-  // Reception of an RTP packet associated with the shared SSRC triggers the
-  // callback on all of the sinks associated with it.
-  auto packet = CreateRtpPacketReceived(ssrc);
-  for (auto& sink : sinks) {
-    EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet)));
-  }
-  EXPECT_TRUE(demuxer.OnRtpPacket(*packet));
-
-  // Test tear-down
-  for (const auto& sink : sinks) {
-    demuxer.RemoveSink(&sink);
-  }
-}
-
 TEST(RtpDemuxerTest, SinkMappedToMultipleSsrcs) {
   RtpDemuxer demuxer;
 
@@ -229,14 +217,61 @@ TEST(RtpDemuxerTest, NoCallbackOnSsrcSinkRemovedAfterFirstPacket) {
   EXPECT_FALSE(demuxer.OnRtpPacket(*packet));
 }
 
-TEST(RtpDemuxerTest, RepeatedSsrcAssociationsDoNotTriggerRepeatedCallbacks) {
+TEST(RtpDemuxerTest, AddSinkFailsIfCalledForTwoSinks) {
+  RtpDemuxer demuxer;
+  MockRtpPacketSink sink_a;
+  MockRtpPacketSink sink_b;
+  constexpr uint32_t ssrc = 1;
+  ASSERT_TRUE(demuxer.AddSink(ssrc, &sink_a));
+
+  EXPECT_FALSE(demuxer.AddSink(ssrc, &sink_b));
+
+  // Test tear-down
+  demuxer.RemoveSink(&sink_a);
+}
+
+// An SSRC may only be mapped to a single sink. However, since configuration
+// of this associations might come from the network, we need to fail gracefully.
+TEST(RtpDemuxerTest, OnlyOneSinkPerSsrcGetsOnRtpPacketTriggered) {
+  RtpDemuxer demuxer;
+
+  MockRtpPacketSink sinks[3];
+  constexpr uint32_t ssrc = 404;
+  for (size_t i = 0; i < arraysize(sinks); i++) {

[danilchap, 2017/06/30 16:45:31]

unwind loop probably easier to read:
ASSERT_TRUE(demuxer.AddSink(ssrc, &sinks[0]));
ASSERT_FALSE(demuxer.AddSink(ssrc, &sinks[1]));
ASSERT_FALSE(demuxer.AddSink(ssrc, &sinks[2]));

[eladalon, 2017/07/03 08:23:35]

Done. (The downside of this approach is that you can't as easily change the
number of sinks anymore.)

+    const bool is_first_add = (i == 0);
+    ASSERT_EQ(is_first_add, demuxer.AddSink(ssrc, &sinks[i]));
+  }
+
+  // The first sink associated with the SSRC remains active; other sinks
+  // were not really added, and so do not get OnRtpPacket() called.
+  auto packet = CreateRtpPacketReceived(ssrc);
+  EXPECT_CALL(sinks[0], OnRtpPacket(SamePacketAs(*packet)));
+  ASSERT_TRUE(demuxer.OnRtpPacket(*packet));
+
+  // Test tear-down
+  demuxer.RemoveSink(&sinks[0]);
+}
+
+TEST(RtpDemuxerTest, AddSinkFailsIfCalledTwiceEvenIfBothTimesAreForSameSink) {

[danilchap, 2017/06/30 16:45:31]

may be drop 'EvenIfBothTimesAre'

[eladalon, 2017/07/03 08:23:35]

Changed to AddSinkFailsIfCalledTwiceEvenIfSameSink; I'd like to keep some focus
on "this adds a twist to a previous test." (The previous test being
OnlyOneSinkPerSsrcGetsOnRtpPacketTriggered, but I don't want to rename that to
get them to look more similar, as I believe that names would be less
illustrative of the test's function.)

+  RtpDemuxer demuxer;
+  MockRtpPacketSink sink;
+  constexpr uint32_t ssrc = 1;
+  ASSERT_TRUE(demuxer.AddSink(ssrc, &sink));
+
+  EXPECT_FALSE(demuxer.AddSink(ssrc, &sink));
+
+  // Test tear-down
+  demuxer.RemoveSink(&sink);
+}
+
+TEST(RtpDemuxerTest, NoRepeatedCallbackOnRepeatedAddSinkForSameSink) {
   RtpDemuxer demuxer;
 
   constexpr uint32_t ssrc = 111;
   MockRtpPacketSink sink;
 
-  demuxer.AddSink(ssrc, &sink);
-  demuxer.AddSink(ssrc, &sink);
+  ASSERT_TRUE(demuxer.AddSink(ssrc, &sink));
+  ASSERT_FALSE(demuxer.AddSink(ssrc, &sink));
 
   auto packet = CreateRtpPacketReceived(ssrc);
   EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
@@ -432,31 +467,6 @@ TEST(RtpDemuxerTest, MultipleRsidsOnSameSink) {
   demuxer.RemoveSink(&sink);
 }
 
-TEST(RtpDemuxerTest, RsidUsedByMultipleSinks) {
-  RtpDemuxer demuxer;
-
-  MockRtpPacketSink sinks[3];
-  const std::string shared_rsid = "a";
-
-  for (MockRtpPacketSink& sink : sinks) {
-    demuxer.AddSink(shared_rsid, &sink);
-  }
-
-  constexpr uint32_t shared_ssrc = 888;
-  auto packet = CreateRtpPacketReceivedWithRsid(shared_rsid, shared_ssrc);
-
-  for (auto& sink : sinks) {
-    EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
-  }
-
-  EXPECT_TRUE(demuxer.OnRtpPacket(*packet));
-
-  // Test tear-down
-  for (MockRtpPacketSink& sink : sinks) {
-    demuxer.RemoveSink(&sink);
-  }
-}
-
 TEST(RtpDemuxerTest, SinkWithBothRsidAndSsrcAssociations) {
   RtpDemuxer demuxer;
 
@@ -501,12 +511,16 @@ TEST(RtpDemuxerTest, AssociatingByRsidAndBySsrcCannotTriggerDoubleCall) {
   demuxer.RemoveSink(&sink);
 }
 
-TEST(RtpDemuxerTest, RsidObserversInformedOfResolutions) {
+TEST(RtpDemuxerTest, RsidObserversInformedOfResolutionsOfTrackedRsids) {
   RtpDemuxer demuxer;
 
   constexpr uint32_t ssrc = 111;
   const std::string rsid = "a";
 
+  // Only RSIDs which the demuxer knows may be resolved.
+  NiceMock<MockRtpPacketSink> sink;
+  demuxer.AddSink(rsid, &sink);
+
   MockRsidResolutionObserver rsid_resolution_observers[3];
   for (auto& observer : rsid_resolution_observers) {
     demuxer.RegisterRsidResolutionObserver(&observer);
@@ -520,6 +534,112 @@ TEST(RtpDemuxerTest, RsidObserversInformedOfResolutions) {
   for (auto& observer : rsid_resolution_observers) {
     demuxer.DeregisterRsidResolutionObserver(&observer);
   }
+  demuxer.RemoveSink(&sink);
+}
+
+TEST(RtpDemuxerTest, RsidObserversNotInformedOfResolutionsOfUntrackedRsids) {
+  RtpDemuxer demuxer;
+
+  constexpr uint32_t ssrc = 111;
+  const std::string rsid = "a";
+
+  MockRsidResolutionObserver rsid_resolution_observers[3];
+  for (auto& observer : rsid_resolution_observers) {
+    demuxer.RegisterRsidResolutionObserver(&observer);
+    EXPECT_CALL(observer, OnRsidResolved(rsid, ssrc)).Times(0);
+  }
+
+  // The expected calls to OnRsidResolved() will be triggered by this.
+  demuxer.OnRtpPacket(*CreateRtpPacketReceivedWithRsid(rsid, ssrc));
+
+  // Test tear-down
+  for (auto& observer : rsid_resolution_observers) {
+    demuxer.DeregisterRsidResolutionObserver(&observer);
+  }
+}
+
+// If one sink is associated with SSRC x, and another sink with RSID y, we
+// should never observe RSID x being resolved to SSRC x, or else we'd end
+// up with one SSRC mapped to two sinks. However, if such faulty input
+// ever reaches us, we should handle it gracefully - not crash, and keep the
+// packets routed only to the SSRC sink.
+TEST(RtpDemuxerTest, PacketFittingBothRsidSinkAndSsrcSinkGivenOnlyToSsrcSink) {
+  RtpDemuxer demuxer;
+
+  constexpr uint32_t ssrc = 111;
+  MockRtpPacketSink ssrc_sink;
+  demuxer.AddSink(ssrc, &ssrc_sink);
+
+  const std::string rsid = "a";
+  MockRtpPacketSink rsid_sink;
+  demuxer.AddSink(rsid, &rsid_sink);
+
+  auto packet = CreateRtpPacketReceivedWithRsid(rsid, ssrc);
+  EXPECT_CALL(ssrc_sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
+  EXPECT_CALL(rsid_sink, OnRtpPacket(SamePacketAs(*packet))).Times(0);
+  demuxer.OnRtpPacket(*packet);
+
+  // Test tear-down
+  demuxer.RemoveSink(&ssrc_sink);
+  demuxer.RemoveSink(&rsid_sink);
+}
+
+TEST(RtpDemuxerTest,
+     PacketFittingBothRsidSinkAndSsrcSinkDoesNotTriggerResolutionCallbacks) {
+  RtpDemuxer demuxer;
+
+  constexpr uint32_t ssrc = 111;
+  NiceMock<MockRtpPacketSink> ssrc_sink;
+  demuxer.AddSink(ssrc, &ssrc_sink);
+
+  const std::string rsid = "a";
+  NiceMock<MockRtpPacketSink> rsid_sink;
+  demuxer.AddSink(rsid, &rsid_sink);
+
+  MockRsidResolutionObserver observer;
+  demuxer.RegisterRsidResolutionObserver(&observer);
+
+  auto packet = CreateRtpPacketReceivedWithRsid(rsid, ssrc);
+  EXPECT_CALL(observer, OnRsidResolved(_, _)).Times(0);
+  demuxer.OnRtpPacket(*packet);
+
+  // Test tear-down
+  demuxer.RemoveSink(&ssrc_sink);
+  demuxer.RemoveSink(&rsid_sink);
+}
+
+// We're not expecting RSIDs to be resolved to SSRCs which were previously
+// mapped to sinks, and make no guarantees except for graceful handling.
+TEST(RtpDemuxerTest, GracefullyHandleRsidBeingMappedToPrevouslyAssociatedSsrc) {
+  RtpDemuxer demuxer;
+
+  constexpr uint32_t ssrc = 111;
+  NiceMock<MockRtpPacketSink> ssrc_sink;
+  demuxer.AddSink(ssrc, &ssrc_sink);
+
+  const std::string rsid = "a";
+  MockRtpPacketSink rsid_sink;
+  demuxer.AddSink(rsid, &rsid_sink);
+
+  MockRsidResolutionObserver observer;
+  demuxer.RegisterRsidResolutionObserver(&observer);
+
+  // The SSRC was mapped to an SSRC sink, but was even active (packets flowed
+  // over it).
+  auto packet = CreateRtpPacketReceivedWithRsid(rsid, ssrc);
+  demuxer.OnRtpPacket(*packet);
+
+  // If the SSRC sink is ever removed, the RSID sink *might* receive indications
+  // of packets, and observers *might* be informed. Only graceful handling
+  // is guaranteed.
+  demuxer.RemoveSink(&ssrc_sink);
+  EXPECT_CALL(rsid_sink, OnRtpPacket(SamePacketAs(*packet))).Times(AtLeast(0));
+  EXPECT_CALL(observer, OnRsidResolved(rsid, ssrc)).Times(AtLeast(0));
+  demuxer.OnRtpPacket(*packet);
+
+  // Test tear-down
+  demuxer.DeregisterRsidResolutionObserver(&observer);
+  demuxer.RemoveSink(&rsid_sink);
 }
 
 TEST(RtpDemuxerTest, DeregisteredRsidObserversNotInformedOfResolutions) {
@@ -559,12 +679,14 @@ TEST(RtpDemuxerTest, DeregisteredRsidObserversNotInformedOfResolutions) {
 TEST(RtpDemuxerTest, RsidMustBeNonEmpty) {
   RtpDemuxer demuxer;
   MockRtpPacketSink sink;
+
   EXPECT_DEATH(demuxer.AddSink("", &sink), "");
 }
 
 TEST(RtpDemuxerTest, RsidMustBeAlphaNumeric) {
   RtpDemuxer demuxer;
   MockRtpPacketSink sink;
+
   EXPECT_DEATH(demuxer.AddSink("a_3", &sink), "");
 }
 
@@ -572,23 +694,45 @@ TEST(RtpDemuxerTest, RsidMustNotExceedMaximumLength) {
   RtpDemuxer demuxer;
   MockRtpPacketSink sink;
   std::string rsid(StreamId::kMaxSize + 1, 'a');
+
   EXPECT_DEATH(demuxer.AddSink(rsid, &sink), "");
 }
 
 TEST(RtpDemuxerTest, RepeatedRsidAssociationsDisallowed) {
   RtpDemuxer demuxer;
+  MockRtpPacketSink sink_a;
+  MockRtpPacketSink sink_b;
+
+  const std::string rsid = "a";
+  demuxer.AddSink(rsid, &sink_a);
+
+  EXPECT_DEATH(demuxer.AddSink(rsid, &sink_b), "");
+
+  // Test tear-down
+  demuxer.RemoveSink(&sink_a);
+}
+
+TEST(RtpDemuxerTest, RepeatedRsidAssociationsDisallowedEvenIfSameSink) {
+  RtpDemuxer demuxer;
   MockRtpPacketSink sink;
-  demuxer.AddSink("a", &sink);
-  EXPECT_DEATH(demuxer.AddSink("a", &sink), "");
+
+  const std::string rsid = "a";
+  demuxer.AddSink(rsid, &sink);
+
+  EXPECT_DEATH(demuxer.AddSink(rsid, &sink), "");
+
+  // Test tear-down
   demuxer.RemoveSink(&sink);
 }
 
-TEST(RtpDemuxerTest,
-     DoubleRegisterationOfNeverRegisteredRsidResolutionObserverDisallowed) {
+TEST(RtpDemuxerTest, DoubleRegisterationOfRsidResolutionObserverDisallowed) {
   RtpDemuxer demuxer;
   MockRsidResolutionObserver observer;
   demuxer.RegisterRsidResolutionObserver(&observer);
+
   EXPECT_DEATH(demuxer.RegisterRsidResolutionObserver(&observer), "");
+
+  // Test tear-down
   demuxer.DeregisterRsidResolutionObserver(&observer);
 }
 
@@ -596,6 +740,7 @@ TEST(RtpDemuxerTest,
      DregisterationOfNeverRegisteredRsidResolutionObserverDisallowed) {
   RtpDemuxer demuxer;
   MockRsidResolutionObserver observer;
+
   EXPECT_DEATH(demuxer.DeregisterRsidResolutionObserver(&observer), "");
 }