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

webrtc/call/rtp_demuxer_unittest.cc

 /*
  *  Copyright (c) 2017 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 63 matching lines @@
   const int rsid_extension_id = 6;
   RtpPacketReceived::ExtensionManager extension_manager;
   extension_manager.Register<RtpStreamId>(rsid_extension_id);
   auto packet = rtc::MakeUnique<RtpPacketReceived>(&extension_manager);
   packet->SetExtension<RtpStreamId>(rsid);
   packet->SetSsrc(ssrc);
   packet->SetSequenceNumber(static_cast<uint16_t>(sequence_number));
   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;
 
   constexpr uint32_t ssrcs[] = {101, 202, 303};
   MockRtpPacketSink sinks[arraysize(ssrcs)];
   for (size_t i = 0; i < arraysize(ssrcs); i++) {
     demuxer.AddSink(ssrcs[i], &sinks[i]);
   }
 
   for (size_t i = 0; i < arraysize(ssrcs); i++) {
@@ skipping 48 matching lines @@
   }
 
   for (const auto& packet : packets) {
     EXPECT_TRUE(demuxer.OnRtpPacket(*packet));
   }
 
   // Test tear-down
   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;
 
   constexpr uint32_t ssrcs[] = {404, 505, 606};
   MockRtpPacketSink sink;
   for (uint32_t ssrc : ssrcs) {
     demuxer.AddSink(ssrc, &sink);
   }
 
   // The sink which is associated with multiple SSRCs gets the callback
@@ skipping 37 matching lines @@
   }
 
   ASSERT_TRUE(demuxer.RemoveSink(&sink));
 
   // The removed sink does not get callbacks.
   auto packet = CreateRtpPacketReceived(ssrc, seq_num);
   EXPECT_CALL(sink, OnRtpPacket(_)).Times(0);  // Not called.
   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);
   EXPECT_TRUE(demuxer.OnRtpPacket(*packet));
 
   // Test tear-down
   demuxer.RemoveSink(&sink);
 }
 
 TEST(RtpDemuxerTest, RemoveSinkReturnsFalseForNeverAddedSink) {
@@ skipping 175 matching lines @@
     auto packet =
         CreateRtpPacketReceivedWithRsid(rsids[i], ssrc, sequence_number);
     EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
     EXPECT_TRUE(demuxer.OnRtpPacket(*packet));
   }
 
   // Test tear-down
   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;
 
   MockRtpPacketSink sink;
   constexpr uint32_t standalone_ssrc = 10101;
   constexpr uint32_t rsid_ssrc = 20202;
   const std::string rsid = "a";
 
   demuxer.AddSink(standalone_ssrc, &sink);
   demuxer.AddSink(rsid, &sink);
@@ skipping 24 matching lines @@
 
   constexpr uint16_t seq_num = 999;
   auto packet = CreateRtpPacketReceivedWithRsid(rsid, ssrc, seq_num);
   EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
   EXPECT_TRUE(demuxer.OnRtpPacket(*packet));
 
   // Test tear-down
   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);
     EXPECT_CALL(observer, OnRsidResolved(rsid, ssrc)).Times(1);
   }
 
   // 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);
   }
+  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) {
   RtpDemuxer demuxer;
 
   constexpr uint32_t ssrc = 111;
   const std::string rsid = "a";
   NiceMock<MockRtpPacketSink> sink;
   demuxer.AddSink(rsid, &sink);
 
   // Register several, then deregister only one, to show that not all of the
   // observers had been forgotten when one was removed.
@@ skipping 17 matching lines @@
   // Test tear-down
   demuxer.RemoveSink(&sink);
   demuxer.DeregisterRsidResolutionObserver(&observer_1);
   demuxer.DeregisterRsidResolutionObserver(&observer_3);
 }
 
 #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
 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), "");
 }
 
 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);
 }
 
 TEST(RtpDemuxerTest,
      DregisterationOfNeverRegisteredRsidResolutionObserverDisallowed) {
   RtpDemuxer demuxer;
   MockRsidResolutionObserver observer;
+
   EXPECT_DEATH(demuxer.DeregisterRsidResolutionObserver(&observer), "");
 }
 
 #endif
 
 }  // namespace
 }  // namespace webrtc