Add RSID-based demuxing to 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.
  */
 
 #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"

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

thanks to your poke this can be replaced with
/rtp_rtcp/include/rtp_header_extension_map.h"
which is (hopefully) cleaner.

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

Done.

+#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"
 
 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();
-}
-
-std::unique_ptr<RtpPacketReceived> CreateRtpPacketReceived(uint32_t ssrc) {
-  auto packet = rtc::MakeUnique<RtpPacketReceived>();
+MATCHER_P(SamePacketAs, other, "") {
+  return arg.Ssrc() == other.Ssrc() &&
+         arg.SequenceNumber() == other.SequenceNumber();
+}
+
+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;
+    rtp::Packet::ExtensionManager extension_manager;

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

may be RtpPacketReceived instead of rtp::Packet
(this would avoid complicating refactoring/renaming of the rtp::Packet - it
probably shouldn't live in it's own namespace)

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

Done.

+    extension_manager.Register<webrtc::RtpStreamId>(kRsidExtensionId);
+    packet = rtc::MakeUnique<RtpPacketReceived>(&extension_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);
-    }
-  }
-
-  RtpDemuxer demuxer;
-  MockRtpPacketSink sinks[arraysize(kSsrcs)];
-};
-
-TEST_F(RtpDemuxerTest, OnRtpPacketCalledOnCorrectSink) {
+TEST(RtpDemuxerTest, OnRtpPacketCalledOnCorrectSink) {

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

with new demuxer responsibilities may be describe what 'Correct' mean:
e.g. TEST(RtpDemuxerTest, DemuxRtpPacketsBySsrc)

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

Appended "BySsrc".

+  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(sinks); i++) {

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

may be, for in-test consistency, use arraysize(same variable)

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

Done.

-    auto packet = CreateRtpPacketReceived(kSsrcs[i]);
-    EXPECT_CALL(sinks[i], OnRtpPacket(SsrcSameAsIn(*packet)));
-    demuxer.OnRtpPacket(*packet);
-  }
-}
-
-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);
+    auto packet = CreateRtpPacketReceived(ssrcs[i]);
+    EXPECT_CALL(sinks[i], OnRtpPacket(SamePacketAs(*packet))).Times(1);
+    EXPECT_TRUE(demuxer.OnRtpPacket(*packet));
+  }
+
+  // Test tear-down
+  for (size_t i = 0; i < arraysize(ssrcs); i++) {

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

may be use c++11 for loop like in MultipleSinksMappedToSameSsrc test tear down.

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

Done.

+    EXPECT_EQ(demuxer.RemoveSink(&sinks[i]), 1u);
+  }
+}
+
+TEST(RtpDemuxerTest, PacketsDeliveredInRightOrder) {

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

I'm not sure what is the value of this test.
Packets delivered using callback (i.e. sink::OnRtpPacket called before
demuxer::OnRtpPacket returns).
That make this test too trivial in synchronous scenario.

If you pretend not to know demuxer implementation, then it isn't demuxer's
responsibility to keep packets order. (they might arrive from network in wrong
order anyway and it is next module responsibility to reorder packets by sequence
number)

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

Clarification - the code neither re-orders the packet according to their
sequence number, nor does it test that he packets are correctly ordered
according to sequence number. What it does is check that the order - whether
correct or wrong - remains unchanged. (Premature reordering by this module might
have adversely affected other modules down the pipeline that would have, for
example, attempted to gauge the network's quality.)

+  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]);
+  }
+
+  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));
+  }
+
+  InSequence sequence;
+  for (const auto& packet : packets) {
+    EXPECT_CALL(sinks[0], OnRtpPacket(SamePacketAs(*packet))).Times(1);
+  }
+
+  for (const auto& packet : packets) {
+    EXPECT_TRUE(demuxer.OnRtpPacket(*packet));
+  }
+
+  // Test tear-down
+  for (size_t i = 0; i < arraysize(ssrcs); i++) {
+    EXPECT_EQ(demuxer.RemoveSink(&sinks[i]), 1u);
+  }
+}
+
+TEST(RtpDemuxerTest, MultipleSinksMappedToSameSsrc) {

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

may be improve test name by describing what Demuxer should do in this scenario:
DeliverRtpPacketToMultipleSinksMappedToSameSsrc

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

IMHO, clear from context - the demuxer delivers packets to the correct sinks.

+  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 interfaces associated with it.
-  auto packet = CreateRtpPacketReceived(kSharedSsrc);
-  for (auto& sink : same_ssrc_sinks) {
-    EXPECT_CALL(sink, OnRtpPacket(SsrcSameAsIn(*packet)));
-  }
-  demuxer.OnRtpPacket(*packet);
-
-  // Test-specific tear-down
-  for (auto& sink : same_ssrc_sinks) {
+  // 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) {
     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);
+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
   // 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);
-  }
-
-  // Test-specific tear-down
-  EXPECT_EQ(demuxer.RemoveSink(&multi_ssrc_sink),
-            arraysize(kSsrcsOfMultiSsrcSink));
-}
-
-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);
-  }
-
-  // Remove the sink.
-  EXPECT_EQ(demuxer.RemoveSink(&multi_ssrc_sink),
-            arraysize(kSsrcsOfMultiSsrcSink));
-
-  // 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);
-  }
+    EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet)));
+    EXPECT_TRUE(demuxer.OnRtpPacket(*packet));
+  }
+
+  // Test tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&sink), arraysize(ssrcs));
+}
+
+TEST(RtpDemuxerTest, NoCallbackOnSsrcSinkRemovedBeforeFirstPacket) {
+  RtpDemuxer demuxer;
+
+  constexpr uint32_t ssrc = 404;
+  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_FALSE(demuxer.OnRtpPacket(*packet));
+}
+
+TEST(RtpDemuxerTest, NoCallbackOnSsrcSinkRemovedAfterFirstPacket) {
+  RtpDemuxer demuxer;
+
+  constexpr uint32_t ssrc = 404;
+  MockRtpPacketSink sink;
+  demuxer.AddSink(ssrc, &sink);
+
+  InSequence sequence;
+  uint16_t seq_num;
+  for (seq_num = 0; seq_num < 10; seq_num++) {
+    auto packet = CreateRtpPacketReceived(ssrc, seq_num);
+    EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
+    EXPECT_TRUE(demuxer.OnRtpPacket(*packet));
+  }
+
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
+
+  // The removed sink does not get callbacks.
+  auto packet = CreateRtpPacketReceived(ssrc, seq_num);
+  EXPECT_CALL(sink, OnRtpPacket(_)).Times(0);
+  EXPECT_FALSE(demuxer.OnRtpPacket(*packet));
+}
+
+TEST(RtpDemuxerTest, RepeatedSsrcAssociationsDoNotTriggerRepeatedCallbacks) {
+  RtpDemuxer demuxer;
+
+  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_TRUE(demuxer.OnRtpPacket(*packet));
+
+  // Test tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
+}
+
+TEST(RtpDemuxerTest, OnRtpPacketCalledForRsidSink) {
+  RtpDemuxer demuxer;
+
+  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));

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

may be change CreateRtpPacket to take int for sequence number and convert to
uint16_t inside the function to make use of it cleaner while bots happy.

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

I've adopted, but took size_t instead of int, because then we can really get rid
of all of the places where we had the cast.
I did not do the same for SSRC, btw, because it's needed less often.

+  }
+
+  // The first packet associates the RSID with the SSRC, thereby allowing the
+  // demuxer to correctly demux all of the packets.
+  InSequence sequence;
+  for (const auto& packet : packets) {
+    EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*packet))).Times(1);
+  }
+  for (const auto& packet : packets) {
+    EXPECT_TRUE(demuxer.OnRtpPacket(*packet));
+  }
+
+  // Test tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 1u);
+}
+
+TEST(RtpDemuxerTest, NoCallbackOnRsidSinkRemovedBeforeFirstPacket) {
+  RtpDemuxer demuxer;
+
+  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_FALSE(demuxer.OnRtpPacket(*packet));
+}
+
+TEST(RtpDemuxerTest, NoCallbackOnRsidSinkRemovedAfterFirstPacket) {
+  RtpDemuxer demuxer;
+
+  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_TRUE(demuxer.OnRtpPacket(*packet));
+  }
+
+  // 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_FALSE(demuxer.OnRtpPacket(*packet));
+}
+
+// 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(RtpDemuxerTest, FirstSsrcAssociatedWithAnRsidIsNotForgotten) {
+  RtpDemuxer demuxer;
+
+  // 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;

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

sometimes you use constant name style for ssrc constants, sometimes variable
name style.
Either is acceptable, but can you use only one style through the file.
https://google.github.io/styleguide/cppguide.html#Constant_Names

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

Done.

+
+  // First a packet with |rsid_a| is received, and |sink_a| is associated with
+  // its SSRC.
+  auto packet_a =

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

since packet 'b' doesn't map to sink 'b' may be use different numeration style
for sinks and packets:
sink_a, sink_b, but packet1, packet2, packet3

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

Please refer to this comment:
// Second, a packet with |rsid_b| is received. Its RSID is ignored.

+      CreateRtpPacketReceived(kSharedSsrc, static_cast<uint16_t>(10), &rsid_a);
+  EXPECT_CALL(sink_a, OnRtpPacket(SamePacketAs(*packet_a))).Times(1);
+  EXPECT_TRUE(demuxer.OnRtpPacket(*packet_a));
+
+  // 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_TRUE(demuxer.OnRtpPacket(*packet_b));
+
+  // 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_TRUE(demuxer.OnRtpPacket(*packet_c));
+
+  // 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(RtpDemuxerTest, MultipleRsidsOnSameSink) {
+  RtpDemuxer demuxer;
+
+  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);
+    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_TRUE(demuxer.OnRtpPacket(*packet));
+  }
+
+  // Test tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&sink), arraysize(rsids));
+}
+
+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);
+
+  InSequence sequence;
+
+  auto ssrc_packet = CreateRtpPacketReceived(standalone_ssrc, 11);
+  EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*ssrc_packet))).Times(1);
+  EXPECT_TRUE(demuxer.OnRtpPacket(*ssrc_packet));
+
+  auto rsid_packet = CreateRtpPacketReceived(rsid_ssrc, 22, &rsid);
+  EXPECT_CALL(sink, OnRtpPacket(SamePacketAs(*rsid_packet))).Times(1);
+  EXPECT_TRUE(demuxer.OnRtpPacket(*rsid_packet));
+
+  // Test tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&sink), 2u);
+}
+
+TEST(RtpDemuxerTest, AssociatingByRsidAndBySsrcCannotTriggerDoubleCall) {
+  RtpDemuxer demuxer;
+  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_TRUE(demuxer.OnRtpPacket(*packet));
+
+  // Test tear-down
+
+  // The RSID and the SSRC associations above have merged into one.

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

what is deletion count? is it number of times AddSink was called for the sink?
this test suggest it is not.
Does that 'deletion count' need to be well-defined?
May be document 'return positive number if at least one sink was deleted' and
EXPECT that in tests instead of exact number.
(and change to return bool in a follow-up CL).

If you want to expect exact number, then it need better documentation and few
dedicated tests.

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

I've previously discussed this problem with nisse@ and we've decided to keep the
deletion count as the number of removed associations the sink has which are
removed when RemoveSink is called. This is known to *not* always correspond to
the number of AddSink() calls for the sink, since some RSID associations might
be deleted in response to OnRtpPacket(). That was a known edge-case.

Following our (Danil and mine) offline discussion, I've changed this function to
return a boolean rather than the deletion count.

+  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(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;
+  demuxer.AddSink("a", &sink);
+  EXPECT_DEATH(demuxer.AddSink("a", &sink), "");
 }
 #endif
 
 }  // namespace
 }  // namespace webrtc