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 58 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;
+  ASSERT_TRUE(demuxer.AddSink(ssrc, &sinks[0]));
+  ASSERT_FALSE(demuxer.AddSink(ssrc, &sinks[1]));
+  ASSERT_FALSE(demuxer.AddSink(ssrc, &sinks[2]));

[Steve Anton, 2017/08/02 20:35:24]

nit: I don't think it's really necessary to check that sinks[2] fails to be
added, since that case should be covered by the previous line.

[eladalon, 2017/08/03 11:14:25]

I've used an ASSERT here, which marks that "though this is not the thing being
tested, the test cannot go on if this doesn't hold true." The actual test for
this, using EXPECT, is done at AddSinkFailsIfCalledForTwoSinks. If someone
modifies code so that this breaks, we want AddSinkFailsIfCalledForTwoSinks to
record the failure, and OnlyOneSinkPerSsrcGetsOnRtpPacketTriggered shows an
assertion failure rather than passing.

+
+  // 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)));

[Steve Anton, 2017/08/02 20:35:24]

nit: Perhaps also expect that sinks[1], sinks[2] receive no calls to
OnRtpPacket?

[eladalon, 2017/08/03 11:14:25]

Done.

+  ASSERT_TRUE(demuxer.OnRtpPacket(*packet));
+
+  // Test tear-down
+  demuxer.RemoveSink(&sinks[0]);
+}
+
+TEST(RtpDemuxerTest, AddSinkFailsIfCalledTwiceEvenIfSameSink) {
+  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