Create unit tests for 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
new file mode 100644
index 0000000000000000000000000000000000000000..fc9cabecfc78fc842da310b2d27b6ee3f6345eaa
--- /dev/null
+++ b/webrtc/call/rtp_demuxer_unittest.cc
@@ -0,0 +1,180 @@
+/*
+ *  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 <memory>
+
+#include "webrtc/call/rtp_demuxer.h"

[danilchap, 2017/05/24 13:45:43]

order includes alphabetically, except for the rtp_demuxer.h: include it before
any other header (i.e. before <memory>)

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

+#include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h"
+
+#include "webrtc/base/checks.h"
+#include "webrtc/test/gmock.h"
+#include "webrtc/test/gtest.h"
+
+// TODO(elad.alon): This is not video-specific, and so should not be

[elad.alon_webrtc.org, 2017/05/24 12:19:35]

Please advise. :-)

[holmer, 2017/05/24 13:10:23]

How is this currently a video engine test? Because it's included in a video test
executable? I think what we may want to do is to rename that executable or break
out common tests into a new exec. Definitely not part of this CL anyway. :)

[danilchap, 2017/05/24 13:45:43]

you put it into call_tests set, which is reasonable.
The fact that it is in turn included into video_engine tests is a different
problem and not subject of this CL.
So I would advise to keep as is.
May move the todo into build.gn file, since it is about organizing dependency,
nothing to do in this file.

+// in the video-engine tests.
+
+namespace webrtc {
+
+namespace {
+
+constexpr uint32_t kSsrcs[] = {101, 202, 303};

[danilchap, 2017/05/24 13:45:43]

may be prefer same order as recommended for classes, i.e. type then constant

https://google.github.io/styleguide/cppguide.html#Declaration_Order

+
+class MockRtpPacketSink : public RtpPacketSinkInterface {
+ public:
+  MOCK_METHOD1(OnRtpPacket, void(const RtpPacketReceived&));
+};
+
+MATCHER_P(RtpPacketReceivedSsrcMatcher, other, "") {
+  return arg.Ssrc() == other.Ssrc();
+}
+
+std::unique_ptr<RtpPacketReceived> GetRtpPacketReceived(uint32_t ssrc) {

[danilchap, 2017/05/24 13:45:43]

why return unique_ptr instead of copy of the packet?
RVO would likely make it as fast.

+  constexpr int8_t kPayloadType = 100;
+  constexpr uint16_t kSeqNum = 0x1234;
+  constexpr uint8_t kSeqNumFirstHalf = kSeqNum >> 8;
+  constexpr uint8_t kSeqNumSecondHalf = kSeqNum & 0xff;
+  // clang-format off
+  constexpr uint8_t kMinimumPacket[] = {
+      0x80, kPayloadType, kSeqNumFirstHalf, kSeqNumSecondHalf,
+      0x65, 0x43, 0x12, 0x78,
+      0x12, 0x34, 0x56, 0x78};
+  // clang-format on
+
+  std::unique_ptr<RtpPacketReceived> packet(new RtpPacketReceived());
+  EXPECT_TRUE(packet->Parse(kMinimumPacket, sizeof(kMinimumPacket)));

[danilchap, 2017/05/24 13:45:43]

you do not have to parse packet to create it:
packet->SetPayloadType(kPayloadType);
packet->SetSequenceNumber(kSeqNum);
would create the same packet as parsed (on the other side, since the only field
you use is ssrc, no need to set other fields)

+  packet->SetSsrc(ssrc);
+  return packet;
+}
+
+}  // namespace
+
+namespace test {

[danilchap, 2017/05/24 13:45:43]

might be better to keep using unnamed namespace.

+
+class RtpDemuxerTest : public ::testing::Test {
+ protected:
+  RtpDemuxerTest() = default;
+  virtual ~RtpDemuxerTest() = default;
+
+  void SetUp() {

[danilchap, 2017/05/24 13:45:43]

prefer constructor over SetUp when you can

+    for (size_t i = 0; i < kNumOfSinks; i++) {
+      demuxer.AddSink(kSsrcs[i], &sinks[i]);
+    }
+  }
+
+  void TearDown() {
+    for (size_t i = 0; i < kNumOfSinks; i++) {
+      EXPECT_EQ(demuxer.RemoveSink(&sinks[i]), 1u);
+    }
+  }
+
+  static constexpr size_t kNumOfSinks = 3;

[danilchap, 2017/05/24 13:45:43]

put constants before constructors.
Since you assume kNumOfSinks == arraysize(kSsrcs), make be put it near kSsrcs
array

+
+  RtpDemuxer demuxer;
+  MockRtpPacketSink sinks[kNumOfSinks];
+};
+
+TEST_F(RtpDemuxerTest, OnRtpPacketCalledOnCorrectSink) {
+  for (size_t i = 0; i < kNumOfSinks; i++) {
+    auto packet = GetRtpPacketReceived(kSsrcs[i]);
+    EXPECT_CALL(sinks[i], OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet)));

[danilchap, 2017/05/24 13:45:43]

OnRtpPacket(Property(&RtpPacketReceived::Ssrc, kSsrcs[i])) is not good enough?
using own match looks cleaner, but can you compare error messages when test
fail, which one will be more readable?

btw, why do you test that OnRtpPacket is called with correct packet at all? work
of the demuxer is to forward it to correct sink, so ::testing::_ might be good
enough.

+    demuxer.OnRtpPacket(*packet);
+    testing::Mock::VerifyAndClearExpectations(&sinks[i]);
+  }
+}
+
+TEST_F(RtpDemuxerTest, MultipleSinksMappedToSameSsrc) {
+  // |sinks| associated with different SSRCs each. Add a few additional sinks
+  // that are all associated with one new, distinct SSRC.
+  constexpr size_t kNumOfSameSsrcSinks = 3;
+  MockRtpPacketSink same_ssrc_sinks[kNumOfSameSsrcSinks];
+  constexpr size_t kSharedSsrc = 404;

[danilchap, 2017/05/24 13:45:43]

use uint32_t for ssrc

+  for (size_t i = 0; i < kNumOfSameSsrcSinks; i++) {
+    demuxer.AddSink(kSharedSsrc, &same_ssrc_sinks[i]);
+  }
+
+  // Reception of an RTP packet associated with the shared SSRC triggers the
+  // callback on all of the interfaces associated with it.
+  auto packet = GetRtpPacketReceived(kSharedSsrc);
+  for (size_t i = 0; i < kNumOfSameSsrcSinks; i++) {
+    EXPECT_CALL(same_ssrc_sinks[i],
+                OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet)));
+  }
+  demuxer.OnRtpPacket(*packet);
+
+  // Test-specific tear-down
+  for (size_t i = 0; i < kNumOfSameSsrcSinks; i++) {
+    EXPECT_EQ(demuxer.RemoveSink(&same_ssrc_sinks[i]), 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);
+  }
+
+  // The sink which is associated with multiple SSRCs gets the callback
+  // triggered for each of those SSRCs.
+  for (uint32_t ssrc : kSsrcsOfMultiSsrcSink) {
+    auto packet = GetRtpPacketReceived(ssrc);
+    EXPECT_CALL(multi_ssrc_sink,
+                OnRtpPacket(RtpPacketReceivedSsrcMatcher(*packet)));
+    demuxer.OnRtpPacket(*packet);
+    testing::Mock::VerifyAndClearExpectations(&multi_ssrc_sink);
+  }
+
+  // Test-specific tear-down
+  EXPECT_EQ(demuxer.RemoveSink(&multi_ssrc_sink),
+            sizeof(kSsrcsOfMultiSsrcSink) / sizeof(kSsrcsOfMultiSsrcSink[0]));

[danilchap, 2017/05/24 13:45:43]

there is arraysize macro in base/arraysize.h exactly for this

+}
+
+TEST_F(RtpDemuxerTest, SinkRemovalSanity) {
+  // |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),
+            sizeof(kSsrcsOfMultiSsrcSink) / sizeof(kSsrcsOfMultiSsrcSink[0]));
+
+  // The removed sink does not get callbacks triggered for any of the SSRCs
+  // with which it was previously associated.
+  for (uint32_t ssrc : kSsrcsOfMultiSsrcSink) {
+    auto packet = GetRtpPacketReceived(ssrc);
+    demuxer.OnRtpPacket(*packet);
+  }
+}
+
+#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)

[danilchap, 2017/05/24 13:45:43]

why exclude 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, SinksMustBeRemovedBeforeDestruction) {
+  std::unique_ptr<RtpDemuxer> bad_demuxer(new RtpDemuxer());
+  MockRtpPacketSink sink;
+  constexpr uint32_t ssrc = 111;
+  bad_demuxer->AddSink(ssrc, &sink);
+  EXPECT_DEATH(bad_demuxer.reset(), "");
+  EXPECT_EQ(bad_demuxer->RemoveSink(&sink), 1u);
+}
+#endif
+
+}  // namespace test
+}  // namespace webrtc