Add RSID-based demuxing to 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 6aee11e7636d916a2c385628aebb85f689513e7d..5ad7cb4215f8030c3d5da5d971707a739021184a 100644
--- a/webrtc/call/rtp_demuxer_unittest.cc
+++ b/webrtc/call/rtp_demuxer_unittest.cc
@@ -11,10 +11,13 @@
 #include "webrtc/call/rtp_demuxer.h"
 
 #include <memory>
+#include <string>
 
 #include "webrtc/base/arraysize.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/ptr_util.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_header_extension.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h"
 #include "webrtc/test/gmock.h"
 #include "webrtc/test/gtest.h"
@@ -24,122 +27,385 @@ namespace webrtc {
 namespace {
 
 using ::testing::_;
+using ::testing::AtLeast;
+using ::testing::InSequence;
 
 class MockRtpPacketSink : public RtpPacketSinkInterface {
  public:
   MOCK_METHOD1(OnRtpPacket, void(const RtpPacketReceived&));
 };
 
-constexpr uint32_t kSsrcs[] = {101, 202, 303};
-
-MATCHER_P(SsrcSameAsIn, other, "") {
-  return arg.Ssrc() == other.Ssrc();
+MATCHER_P(SamePacketAs, other, "") {
+  return arg.Ssrc() == other.Ssrc() &&
+         arg.SequenceNumber() == other.SequenceNumber();
 }
 
-std::unique_ptr<RtpPacketReceived> CreateRtpPacketReceived(uint32_t ssrc) {
-  auto packet = rtc::MakeUnique<RtpPacketReceived>();
+std::unique_ptr<RtpPacketReceived> CreateRtpPacketReceived(
+    uint32_t ssrc,
+    uint16_t sequence_number = 0,
+    const std::string* rsid = nullptr) {
+  std::unique_ptr<RtpPacketReceived> packet;
+  if (rsid == nullptr) {
+    packet = rtc::MakeUnique<RtpPacketReceived>();
+  } else {
+    const int kRsidExtensionId = 6;
+    webrtc::RtpHeaderExtensionMap extensions;
+    extensions.Register<webrtc::RtpStreamId>(kRsidExtensionId);
+    rtp::Packet::ExtensionManager extensions_manager(extensions);

[danilchap, 2017/06/02 17:42:19]

why not just
RtpPacketReceieved::ExtensionManager extensions;
extensions.Register<webrtc::RtpStreamId>(kRsidExtensionId);
auto packet = rtc::MakeUnique<RtpPacketReceived>(&extenions);
if (rsid != nullptr) { // or if (!rsid.empty())
  packet->SetExtension<webrtc::RtpStreamId>(*rsid);
}

[eladalon, 2017/06/02 19:44:04]

1. I'd like to make it very clear when the packet has an RSID, and when it
doesn't. So not even the ExtensionManager when no RSID is desired.
2. Can't do !rsid.empty() because I also check for invalid RSIDs, and an
empty-string is also a possible invalid RSID.
3. Removed the RtpHeaderExtensionMap, as per your suggestion.

[danilchap, 2017/06/07 13:45:41]

Good reason!
Did you consider two functions to make it clearer?
CreateRtpPacket(ssrc, seq_no = 0)
CreateRtpPacketWithRsid(rsid, ssrc, seq_no = 0)

[eladalon, 2017/06/07 19:30:00]

Done.

+    packet = rtc::MakeUnique<RtpPacketReceived>(&extensions_manager);
+    packet->SetExtension<webrtc::RtpStreamId>(*rsid);
+  }
   packet->SetSsrc(ssrc);
+  packet->SetSequenceNumber(sequence_number);
   return packet;
 }
 
 class RtpDemuxerTest : public ::testing::Test {
  protected:
-  RtpDemuxerTest() {
-    for (size_t i = 0; i < arraysize(sinks); i++) {
-      demuxer.AddSink(kSsrcs[i], &sinks[i]);
-    }
-  }
-
-  ~RtpDemuxerTest() override {
-    for (auto& sink : sinks) {
-      EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
-    }
-  }
+  RtpDemuxerTest() = default;

[danilchap, 2017/06/02 17:42:19]

feel free to omit default constructor/destructor for class declared in .cc file

[eladalon, 2017/06/02 19:44:04]

Done.

+  ~RtpDemuxerTest() override = default;
 
   RtpDemuxer demuxer;

[danilchap, 2017/06/02 17:42:20]

fixture that small easy not to use at all:
just an extra line per test, but you demonstrate that RtpDemuxer doesn't need
any complicated setup.

[eladalon, 2017/06/02 19:44:04]

Done.

-  MockRtpPacketSink sinks[arraysize(kSsrcs)];
 };
 
 TEST_F(RtpDemuxerTest, OnRtpPacketCalledOnCorrectSink) {
+  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(sinks); i++) {
-    auto packet = CreateRtpPacketReceived(kSsrcs[i]);
-    EXPECT_CALL(sinks[i], OnRtpPacket(SsrcSameAsIn(*packet)));
-    demuxer.OnRtpPacket(*packet);
+    auto packet = CreateRtpPacketReceived(ssrcs[i]);
+    EXPECT_CALL(sinks[i], OnRtpPacket(SamePacketAs(*packet))).Times(1);

[danilchap, 2017/06/02 17:42:19]

.Times(1) can by omitted if you wish
https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.m...

[eladalon, 2017/06/02 19:44:04]

Some people see it as self-documentation. I am still on the fence. I'll keep it
for now.

+    EXPECT_EQ(demuxer.OnRtpPacket(*packet), true);

[danilchap, 2017/06/02 17:42:19]

EXPECT_TRUE

[eladalon, 2017/06/02 19:44:04]

Yes, that does look silly.Almost like "if (x == true)". Thanks for pointing it
out.

+  }
+
+  // Test tear-down

[danilchap, 2017/06/02 17:42:19]

end comments with a '.'

[eladalon, 2017/06/02 19:44:04]

I believe full stops belong at the end of a sentence, not of a title. 
"War and Peace" vs. "War and Peace."

+  for (size_t i = 0; i < arraysize(ssrcs); i++) {
+    EXPECT_EQ(demuxer.RemoveSink(&sinks[i]), 1u);
+  }
+}
+
+TEST_F(RtpDemuxerTest, PacketsDeliveredInRightOrder) {
+  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]);
+  }
+
+  std::unique_ptr<RtpPacketReceived> packets[5];
+  for (size_t i = 0; i < arraysize(packets); i++) {
+    packets[i] = CreateRtpPacketReceived(ssrcs[0], static_cast<uint16_t>(i));

[danilchap, 2017/06/02 17:42:20]

if you wish to be more explicit, then beeing explicit about parameter name might
be better than about parameter type:
CreateRtpPacketReceived(ssrc[0], /*sequence_number=*/i);

[eladalon, 2017/06/02 19:44:04]

I'm actually doing this to avoid compiler errors. Implicit narrowing conversions
stress one of the trybots out.

+  }
+
+  InSequence sequence;
+  for (auto& packet : packets) {

[danilchap, 2017/06/02 17:42:19]

may by const auto& since you do not change the value

[eladalon, 2017/06/02 19:44:04]

Done.

+    EXPECT_CALL(sinks[0], OnRtpPacket(SamePacketAs(*packet))).Times(1);
+  }
+
+  for (auto& packet : packets) {
+    EXPECT_EQ(demuxer.OnRtpPacket(*packet), true);
+  }
+
+  // Test tear-down
+  for (size_t i = 0; i < arraysize(ssrcs); i++) {
+    EXPECT_EQ(demuxer.RemoveSink(&sinks[i]), 1u);
   }
 }
 
 TEST_F(RtpDemuxerTest, MultipleSinksMappedToSameSsrc) {
-  // |sinks| associated with different SSRCs each. Add a few additional sinks
-  // that are all associated with one new, distinct SSRC.
-  MockRtpPacketSink same_ssrc_sinks[arraysize(sinks)];
-  constexpr uint32_t kSharedSsrc = 404;
-  for (auto& sink : same_ssrc_sinks) {
-    demuxer.AddSink(kSharedSsrc, &sink);
+  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 interfaces associated with it.
-  auto packet = CreateRtpPacketReceived(kSharedSsrc);
-  for (auto& sink : same_ssrc_sinks) {
-    EXPECT_CALL(sink, OnRtpPacket(SsrcSameAsIn(*packet)));
+  // callback on all of the sinks associated with it.
+  auto packet = CreateRtpPacketReceived(ssrc);
+  for (auto& sink : sinks) {
+    EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet)));
   }
-  demuxer.OnRtpPacket(*packet);
+  EXPECT_EQ(demuxer.OnRtpPacket(*packet), true);
 
-  // Test-specific tear-down
-  for (auto& sink : same_ssrc_sinks) {
+  // Test tear-down
+  for (auto& sink : sinks) {

[danilchap, 2017/06/02 17:42:20]

may be const auto&

[eladalon, 2017/06/02 19:44:04]

Done.

     EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
   }
 }
 
 TEST_F(RtpDemuxerTest, SinkMappedToMultipleSsrcs) {
-  // |sinks| associated with different SSRCs each. We set one of them to also
-  // be mapped to additional SSRCs.
-  constexpr uint32_t kSsrcsOfMultiSsrcSink[] = {404, 505, 606};
-  MockRtpPacketSink multi_ssrc_sink;
-  for (uint32_t ssrc : kSsrcsOfMultiSsrcSink) {
-    demuxer.AddSink(ssrc, &multi_ssrc_sink);
+  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
   // triggered for each of those SSRCs.
-  for (uint32_t ssrc : kSsrcsOfMultiSsrcSink) {
+  for (uint32_t ssrc : ssrcs) {
     auto packet = CreateRtpPacketReceived(ssrc);
-    EXPECT_CALL(multi_ssrc_sink, OnRtpPacket(SsrcSameAsIn(*packet)));
-    demuxer.OnRtpPacket(*packet);
+    EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet)));
+    EXPECT_EQ(demuxer.OnRtpPacket(*packet), true);
   }
 
-  // Test-specific tear-down
-  EXPECT_EQ(demuxer.RemoveSink(&multi_ssrc_sink),
-            arraysize(kSsrcsOfMultiSsrcSink));
+  // Test tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&sink), arraysize(ssrcs));
 }
 
-TEST_F(RtpDemuxerTest, OnRtpPacketNotCalledOnRemovedSinks) {
-  // |sinks| associated with different SSRCs each. We set one of them to also
-  // be mapped to additional SSRCs.
-  constexpr uint32_t kSsrcsOfMultiSsrcSink[] = {404, 505, 606};
-  MockRtpPacketSink multi_ssrc_sink;
-  for (uint32_t ssrc : kSsrcsOfMultiSsrcSink) {
-    demuxer.AddSink(ssrc, &multi_ssrc_sink);
+TEST_F(RtpDemuxerTest, NoCallbackOnSsrcSinkRemovedBeforeFirstPacket) {
+  constexpr uint32_t ssrc = 404;

[danilchap, 2017/06/02 17:42:19]

404 is very good number for test where ssrc will not be found :)

[eladalon, 2017/06/02 19:44:04]

:-)

+  MockRtpPacketSink sink;
+  demuxer.AddSink(ssrc, &sink);
+
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
+
+  // The removed sink does not get callbacks.
+  auto packet = CreateRtpPacketReceived(ssrc);
+  EXPECT_CALL(sink, OnRtpPacket(_)).Times(0);
+  EXPECT_EQ(demuxer.OnRtpPacket(*packet), false);

[danilchap, 2017/06/02 17:42:19]

EXPECT_FALSE

[eladalon, 2017/06/02 19:44:04]

Done.

+}
+
+TEST_F(RtpDemuxerTest, NoCallbackOnSsrcSinkRemovedAfterFirstPacket) {
+  constexpr uint32_t ssrc = 404;
+  MockRtpPacketSink sink;
+  demuxer.AddSink(ssrc, &sink);
+
+  InSequence sequence;
+  uint16_t seq_num;

[danilchap, 2017/06/02 17:42:19]

do you use the variable outside the for loop?

[eladalon, 2017/06/02 19:44:04]

Actually, I'd meant to do it for the last packet (the one after the sink's
removal). I've added that now; thanks.

(It's not really necessary, but something feels odd about sending a sequence of
RTP packets with specific, incrementing sequence numbers, then just forgetting
about it and sending one with an undetermined sequence number. One could ask
oneself if the test didn't perhaps only pass because of a bad sequence number.
This eliminates such concerns.)

+  for (seq_num = 0; seq_num < 10; seq_num++) {
+    auto packet = CreateRtpPacketReceived(ssrc, seq_num);
+    EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
+    EXPECT_EQ(demuxer.OnRtpPacket(*packet), true);
   }
 
-  // Remove the sink.
-  EXPECT_EQ(demuxer.RemoveSink(&multi_ssrc_sink),
-            arraysize(kSsrcsOfMultiSsrcSink));
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
 
-  // The removed sink does not get callbacks triggered for any of the SSRCs
-  // with which it was previously associated.
-  EXPECT_CALL(multi_ssrc_sink, OnRtpPacket(_)).Times(0);
-  for (uint32_t ssrc : kSsrcsOfMultiSsrcSink) {
-    auto packet = CreateRtpPacketReceived(ssrc);
-    demuxer.OnRtpPacket(*packet);
+  // The removed sink does not get callbacks.
+  auto packet = CreateRtpPacketReceived(ssrc);
+  EXPECT_CALL(sink, OnRtpPacket(_)).Times(0);
+  EXPECT_EQ(demuxer.OnRtpPacket(*packet), false);
+}
+
+TEST_F(RtpDemuxerTest, RepeatedSsrcAssociationsDoNotTriggerRepeatedCallbacks) {
+  constexpr uint32_t ssrc = 111;
+  MockRtpPacketSink sink;
+
+  demuxer.AddSink(ssrc, &sink);
+  demuxer.AddSink(ssrc, &sink);
+
+  auto packet = CreateRtpPacketReceived(ssrc);
+  EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
+  EXPECT_EQ(demuxer.OnRtpPacket(*packet), true);
+
+  // Test tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
+}
+
+TEST_F(RtpDemuxerTest, OnRtpPacketCalledForRsidSink) {
+  MockRtpPacketSink sink;
+  const std::string rsid = "a";
+  demuxer.AddSink(rsid, &sink);
+
+  // Create a sequence of RTP packets, where only the first one actually
+  // mentions the RSID.
+  std::unique_ptr<RtpPacketReceived> packets[5];
+  constexpr uint32_t rsid_ssrc = 111;
+  packets[0] =
+      CreateRtpPacketReceived(rsid_ssrc, static_cast<uint16_t>(0), &rsid);
+  for (size_t i = 1; i < arraysize(packets); i++) {
+    packets[i] = CreateRtpPacketReceived(rsid_ssrc, static_cast<uint16_t>(i));
+  }
+
+  // The first packet associates the RSID with the SSRC, thereby allowing the
+  // demuxer to correctly demux all of the packets.
+  InSequence sequence;
+  for (auto& packet : packets) {
+    EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
   }
+  for (auto& packet : packets) {
+    EXPECT_EQ(demuxer.OnRtpPacket(*packet), true);
+  }
+
+  // Test tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
+}
+
+TEST_F(RtpDemuxerTest, NoCallbackOnRsidSinkRemovedBeforeFirstPacket) {
+  MockRtpPacketSink sink;
+  const std::string rsid = "a";
+  demuxer.AddSink(rsid, &sink);
+
+  // Sink removed - it won't get triggers even if packets with its RSID arrive.
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
+
+  constexpr uint32_t ssrc = 111;
+  constexpr uint16_t seq_num = 222;
+  auto packet = CreateRtpPacketReceived(ssrc, seq_num, &rsid);
+  EXPECT_CALL(sink, OnRtpPacket(_)).Times(0);  // Not called.
+  EXPECT_EQ(demuxer.OnRtpPacket(*packet), false);
+}
+
+TEST_F(RtpDemuxerTest, NoCallbackOnRsidSinkRemovedAfterFirstPacket) {
+  MockRtpPacketSink sink;
+  const std::string rsid = "a";
+  demuxer.AddSink(rsid, &sink);
+
+  InSequence sequence;
+  constexpr uint32_t ssrc = 111;
+  uint16_t seq_num;
+  for (seq_num = 0; seq_num < 10; seq_num++) {
+    auto packet = CreateRtpPacketReceived(ssrc, seq_num, &rsid);
+    EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
+    EXPECT_EQ(demuxer.OnRtpPacket(*packet), true);
+  }
+
+  // Sink removed - it won't get triggers even if packets with its RSID arrive.
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
+
+  auto packet = CreateRtpPacketReceived(ssrc, seq_num, &rsid);
+  EXPECT_CALL(sink, OnRtpPacket(_)).Times(0);  // Not called.
+  EXPECT_EQ(demuxer.OnRtpPacket(*packet), false);
+}
+
+// The RSID to SSRC mapping should be one-to-one. If we end up receiving
+// two (or more) packets with the same SSRC, but different RSIDs, we guarantee
+// remembering the first one; no guarantees are made about further associations.
+TEST_F(RtpDemuxerTest, FirstSsrcAssociatedWithAnRsidIsNotForgotten) {
+  // Each sink has a distinct RSID.
+  MockRtpPacketSink sink_a;
+  const std::string rsid_a = "a";
+  demuxer.AddSink(rsid_a, &sink_a);
+
+  MockRtpPacketSink sink_b;
+  const std::string rsid_b = "b";
+  demuxer.AddSink(rsid_b, &sink_b);
+
+  InSequence sequence;  // Verify that the order of delivery is unchanged.
+
+  constexpr uint32_t kSharedSsrc = 100;
+
+  // First a packet with |rsid_a| is received, and |sink_a| is associated with
+  // its SSRC.
+  auto packet_a =
+      CreateRtpPacketReceived(kSharedSsrc, static_cast<uint16_t>(10), &rsid_a);
+  EXPECT_CALL(sink_a, OnRtpPacket(SamePacketAs(*packet_a))).Times(1);
+  EXPECT_EQ(demuxer.OnRtpPacket(*packet_a), true);
+
+  // Second, a packet with |rsid_b| is received. Its RSID is ignored.
+  auto packet_b =
+      CreateRtpPacketReceived(kSharedSsrc, static_cast<uint16_t>(20), &rsid_b);
+  EXPECT_CALL(sink_a, OnRtpPacket(SamePacketAs(*packet_b))).Times(1);
+  EXPECT_EQ(demuxer.OnRtpPacket(*packet_b), true);
+
+  // Known edge-case; adding a new RSID association makes us re-examine all
+  // SSRCs. |sink_b| may or may not be associated with the SSRC now; we make
+  // no promises on that. We do however still guarantee that |sink_a| still
+  // receives the new packets.
+  MockRtpPacketSink sink_ignored;
+  demuxer.AddSink("ignored", &sink_ignored);
+  auto packet_c =
+      CreateRtpPacketReceived(kSharedSsrc, static_cast<uint16_t>(30), &rsid_b);
+  EXPECT_CALL(sink_a, OnRtpPacket(SamePacketAs(*packet_c))).Times(1);
+  EXPECT_CALL(sink_b, OnRtpPacket(SamePacketAs(*packet_c))).Times(AtLeast(0));
+  EXPECT_EQ(demuxer.OnRtpPacket(*packet_c), true);
+
+  // Test tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&sink_a), 1u);
+  EXPECT_EQ(demuxer.RemoveSink(&sink_b), 1u);
+  EXPECT_EQ(demuxer.RemoveSink(&sink_ignored), 1u);
+}
+
+TEST_F(RtpDemuxerTest, MultipleRsidsOnSameSink) {
+  MockRtpPacketSink sink;
+  const std::string rsids[] = {"a", "b", "c"};
+
+  for (const std::string& rsid : rsids) {
+    demuxer.AddSink(rsid, &sink);
+  }
+
+  InSequence sequence;
+  for (size_t i = 0; i < arraysize(rsids); i++) {
+    // Assign different SSRCs and sequence numbers to all packets.
+    const uint32_t ssrc = 1000 + static_cast<uint32_t>(i);

[danilchap, 2017/06/02 17:42:19]

static_cast is probably an overkill here.
Do you think it is more readable with or without it?

[eladalon, 2017/06/02 19:44:04]

Not readability; trying to keep the trybots' various compilers calm and happy.

+    const uint16_t sequence_number = 50 + static_cast<uint32_t>(i);
+    auto packet = CreateRtpPacketReceived(ssrc, sequence_number, &rsids[i]);
+    EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
+    EXPECT_EQ(demuxer.OnRtpPacket(*packet), true);
+  }
+
+  // Test tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&sink), arraysize(rsids));
+}
+
+TEST_F(RtpDemuxerTest, SinkWithBothRsidAndSsrcAssociations) {
+  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);
+
+  InSequence sequence;
+
+  auto ssrc_packet = CreateRtpPacketReceived(standalone_ssrc, 11);
+  EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*ssrc_packet))).Times(1);
+  EXPECT_EQ(demuxer.OnRtpPacket(*ssrc_packet), true);
+
+  auto rsid_packet = CreateRtpPacketReceived(rsid_ssrc, 22, &rsid);
+  EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*rsid_packet))).Times(1);
+  EXPECT_EQ(demuxer.OnRtpPacket(*rsid_packet), true);
+
+  // Test tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 2u);
+}
+
+TEST_F(RtpDemuxerTest, AssociatingByRsidAndBySsrcCannotTriggerDoubleCall) {
+  MockRtpPacketSink sink;
+
+  constexpr uint32_t ssrc = 10101;
+  demuxer.AddSink(ssrc, &sink);
+
+  const std::string rsid = "a";
+  demuxer.AddSink(rsid, &sink);
+
+  constexpr uint16_t seq_num = 999;
+  auto packet = CreateRtpPacketReceived(ssrc, seq_num, &rsid);
+  EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
+  EXPECT_EQ(demuxer.OnRtpPacket(*packet), true);
+
+  // Test tear-down
+
+  // The RSID and the SSRC associations above have merged into one.
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
 }
 
 #if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
-TEST_F(RtpDemuxerTest, RepeatedAssociationsForbidden) {
-  // Set-up already associated sinks[0] with kSsrcs[0]. Repeating the
-  // association is an error.
-  EXPECT_DEATH(demuxer.AddSink(kSsrcs[0], &sinks[0]), "");
+TEST_F(RtpDemuxerTest, RsidMustBeNonEmpty) {
+  MockRtpPacketSink sink;
+  EXPECT_DEATH(demuxer.AddSink("", &sink), "");
+}
+
+TEST_F(RtpDemuxerTest, RsidMustBeAlphaNumeric) {
+  MockRtpPacketSink sink;
+  EXPECT_DEATH(demuxer.AddSink("a_3", &sink), "");
+}
+
+TEST_F(RtpDemuxerTest, RsidMustNotExceedMaximumLength) {
+  std::stringstream ss;
+  for (size_t i = 0; i < StreamId::kMaxSize + 1; i++) {
+    ss << "a";
+  }

[danilchap, 2017/06/02 17:42:19]

std::string long_rsid(StreamId::kMaxSize + 1, 'a');

[eladalon, 2017/06/02 19:44:04]

Done.

+  MockRtpPacketSink sink;
+  EXPECT_DEATH(demuxer.AddSink(ss.str(), &sink), "");
+}
+
+TEST_F(RtpDemuxerTest, RepeatedRsidAssociationsDisallowed) {
+  MockRtpPacketSink sink;
+  demuxer.AddSink("a", &sink);
+  EXPECT_DEATH(demuxer.AddSink("a", &sink), "");
 }
 #endif