Explicitly inform PacketRouter which RTP-RTCP modules are REMB-candidates

webrtc/modules/pacing/packet_router_unittest.cc

 /*
  *  Copyright (c) 2015 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 <list>
 #include <memory>
 
 #include "webrtc/modules/pacing/packet_router.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h"
 #include "webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
 #include "webrtc/rtc_base/checks.h"
 #include "webrtc/rtc_base/fakeclock.h"
 #include "webrtc/test/gmock.h"
 #include "webrtc/test/gtest.h"
 
 using ::testing::_;
 using ::testing::AnyNumber;
 using ::testing::Field;
+using ::testing::Invoke;
 using ::testing::NiceMock;
 using ::testing::Return;
 
 namespace webrtc {
 
+// TODO(eladalon): Restructure and/or replace the existing monolithic tests
+// (only some of the test are monolithic) according to the new
+// guidelines - small tests for one thing at a time.
+// (I'm not removing any tests during CL, so as to demonstrate no regressions.)
+
+void ExpectSetREMBStatusAndSetRembAccordingly(MockRtpRtcp* mock_rtp_rtcp,
+                                              bool remb_state) {
+  EXPECT_CALL(*mock_rtp_rtcp, SetREMBStatus(remb_state))

[danilchap, 2017/07/28 11:35:04]

EXPECT_CALL in helper complicates reading error message when it fails - error
message will contain line number of the helper instead of line number where
expectation was set.

[eladalon, 2017/07/28 15:47:11]

Removed in favor of MockRtpRtcpRemb (under a different name) and explicitly
checking REMB after the function under test is called.

+      .Times(1)
+      .WillOnce(Invoke([mock_rtp_rtcp](bool state) {
+        ON_CALL(*mock_rtp_rtcp, REMB()).WillByDefault(Return(state));
+      }));
+}
+
+// Same as the Expect* version, but without the expectation. Note that this
+// does not mix well with Expect*.
+void OnSetREMBStatusAndSetRembAccordingly(MockRtpRtcp* mock_rtp_rtcp) {

[danilchap, 2017/07/28 11:35:04]

probably cleaner to use different Mock instead of free functions:
class MockRtpRtcpRemb : public MockRtpRtcp {
 public:
  MockRtpRtcpRemb() {
    ON_CALL(*this, SetREMBStatus(_)).WillByDefault(SaveArg<0>(&remb_));
    ON_CALL(*this, REMB()).WillByDefault(ReturnPointee(&remb_));
  }

 private:
  bool remb_ = false;
};

TEST(...) {
  // both EXPECT_CALL(module, SetRembStatus(false)) before call
  // and EXPECT_FALSE(module->REMB()) after the call with check what you need.
  // personally would prefer the latter: expect module end-up in correct remb
state rather than how it was changed to it.
  // or, like you already doing, test with EXPECT_CALL(, SetREMBData) removing
mentioning GetRembStatus/SetRembStatus from individual tests.
}

For more details:
https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#...

[eladalon, 2017/07/28 15:47:11]

MockRtpRtcpRemb - Good suggestions, I'll use that.
About the rest - discussed offline.

+  ON_CALL(*mock_rtp_rtcp, SetREMBStatus(_))
+      .WillByDefault(Invoke([mock_rtp_rtcp](bool state) {
+        ON_CALL(*mock_rtp_rtcp, REMB()).WillByDefault(Return(state));
+      }));
+}
+
 class PacketRouterTest : public ::testing::Test {
  public:
   PacketRouterTest() : packet_router_(new PacketRouter()) {}
  protected:
-  static const int kProbeMinProbes = 5;
-  static const int kProbeMinBytes = 1000;
+  static constexpr int kProbeMinProbes = 5;
+  static constexpr int kProbeMinBytes = 1000;
   const std::unique_ptr<PacketRouter> packet_router_;
 };
 
+TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_TimeToSendPacket) {

[danilchap, 2017/07/28 11:35:04]

this questionable sanity checks looks unlrelated to remb changes.
(Do you know a use case that rely on this edge behaviour?)
May be move discussion about them into dedicated CL

[eladalon, 2017/07/28 15:47:11]

I'll move to its own CL (https://codereview.webrtc.org/2986093003).
As for questionability, I'd say that at the very least we want to make sure we
don't crash - hence the "sanity". Whether it should return false, return true,
or RTC_DCHECK (then the sanity test would be a death-test) is indeed up for
discussion.

+  PacketRouter packet_router;
+
+  constexpr uint16_t ssrc = 1234;
+  constexpr uint16_t sequence_number = 17;
+  constexpr uint64_t timestamp = 7890;
+  constexpr bool retransmission = false;
+  const PacedPacketInfo paced_info(1, kProbeMinProbes, kProbeMinBytes);
+
+  // TODO(eladalon): !!! Discuss with reviewers - it's interesting that this
+  // returns true even when no modules are found that match the SSRC.

[danilchap, 2017/07/27 20:00:20]

probably it mean that module for the packet was just removed.
i.e. it is right thing to drop the packet.
TimeToSendPacket probably should return void.

[eladalon, 2017/07/27 21:30:53]

PacedSender::SendPacket uses that, so even if we end up removing it, that's work
for a separate CL.
I have the TODO there because it looks suspicious. If we look at that
aforementioned SendPacket(), we can see the following (where |success| is the
return value of TimeToSendPacket):
  if (success) {
    if (packet.priority != kHighPriority) {
      // Update media bytes sent.
      UpdateBudgetWithBytesSent(packet.bytes);
    }
It seems suspicious to me that a packet that did not end up being sent anywhere
could lead to an updating of a budget, and merits further investigation. Then
again, it might also be that this is all highly theoretical, because the SSRC
not matching might not be possible. Wdyt about filing a bug for this and
removing this TODO from this CL?

+  EXPECT_TRUE(packet_router.TimeToSendPacket(ssrc, sequence_number, timestamp,
+                                             retransmission, paced_info));
+}
+
+TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_TimeToSendPadding) {
+  PacketRouter packet_router;
+
+  constexpr size_t bytes = 300;
+  const PacedPacketInfo paced_info(1, kProbeMinProbes, kProbeMinBytes);
+
+  EXPECT_EQ(packet_router.TimeToSendPadding(bytes, paced_info), 0u);
+}
+
+TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_OnReceiveBitrateChanged) {
+  PacketRouter packet_router;
+
+  const std::vector<uint32_t> ssrcs = {1, 2, 3};
+  constexpr uint32_t bitrate_bps = 10000;
+
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_bps);
+}
+
+TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_SendRemb) {
+  PacketRouter packet_router;
+
+  const std::vector<uint32_t> ssrcs = {1, 2, 3};
+  constexpr uint32_t bitrate_bps = 10000;
+
+  EXPECT_FALSE(packet_router.SendRemb(bitrate_bps, ssrcs));
+}
+
+TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_SendTransportFeedback) {
+  PacketRouter packet_router;
+
+  rtcp::TransportFeedback feedback;
+
+  EXPECT_FALSE(packet_router_->SendTransportFeedback(&feedback));
+}
+
 TEST_F(PacketRouterTest, TimeToSendPacket) {
   NiceMock<MockRtpRtcp> rtp_1;
   NiceMock<MockRtpRtcp> rtp_2;
-  packet_router_->AddSendRtpModule(&rtp_1);
-  packet_router_->AddSendRtpModule(&rtp_2);
+  packet_router_->AddSendRtpModule(&rtp_1, false);
+  packet_router_->AddSendRtpModule(&rtp_2, false);
 
   const uint16_t kSsrc1 = 1234;
   uint16_t sequence_number = 17;
   uint64_t timestamp = 7890;
   bool retransmission = false;
 
   // Send on the first module by letting rtp_1 be sending with correct ssrc.
   EXPECT_CALL(rtp_1, SendingMedia()).Times(1).WillOnce(Return(true));
   EXPECT_CALL(rtp_1, SSRC()).Times(1).WillOnce(Return(kSsrc1));
   EXPECT_CALL(rtp_1, TimeToSendPacket(
@@ skipping 63 matching lines @@
   const uint16_t kSsrc1 = 1234;
   const uint16_t kSsrc2 = 4567;
 
   NiceMock<MockRtpRtcp> rtp_1;
   EXPECT_CALL(rtp_1, RtxSendStatus()).WillOnce(Return(kRtxOff));
   EXPECT_CALL(rtp_1, SSRC()).WillRepeatedly(Return(kSsrc1));
   NiceMock<MockRtpRtcp> rtp_2;
   // rtp_2 will be prioritized for padding.
   EXPECT_CALL(rtp_2, RtxSendStatus()).WillOnce(Return(kRtxRedundantPayloads));
   EXPECT_CALL(rtp_2, SSRC()).WillRepeatedly(Return(kSsrc2));
-  packet_router_->AddSendRtpModule(&rtp_1);
-  packet_router_->AddSendRtpModule(&rtp_2);
+  packet_router_->AddSendRtpModule(&rtp_1, false);
+  packet_router_->AddSendRtpModule(&rtp_2, false);
 
   // Default configuration, sending padding on all modules sending media,
   // ordered by priority (based on rtx mode).
   const size_t requested_padding_bytes = 1000;
   const size_t sent_padding_bytes = 890;
   EXPECT_CALL(rtp_2, SendingMedia()).Times(1).WillOnce(Return(true));
   EXPECT_CALL(rtp_2, HasBweExtensions()).Times(1).WillOnce(Return(true));
   EXPECT_CALL(rtp_2,
               TimeToSendPadding(requested_padding_bytes,
                                 Field(&PacedPacketInfo::probe_cluster_id, 111)))
@@ skipping 63 matching lines @@
             packet_router_->TimeToSendPadding(
                 requested_padding_bytes,
                 PacedPacketInfo(PacedPacketInfo::kNotAProbe, kProbeMinBytes,
                                 kProbeMinBytes)));
 
   packet_router_->RemoveSendRtpModule(&rtp_2);
 }
 
 TEST_F(PacketRouterTest, SenderOnlyFunctionsRespectSendingMedia) {
   NiceMock<MockRtpRtcp> rtp;
-  packet_router_->AddSendRtpModule(&rtp);
+  packet_router_->AddSendRtpModule(&rtp, false);
   static const uint16_t kSsrc = 1234;
   EXPECT_CALL(rtp, SSRC()).WillRepeatedly(Return(kSsrc));
   EXPECT_CALL(rtp, SendingMedia()).WillRepeatedly(Return(false));
 
   // Verify that TimeToSendPacket does not end up in a receiver.
   EXPECT_CALL(rtp, TimeToSendPacket(_, _, _, _, _)).Times(0);
   EXPECT_TRUE(packet_router_->TimeToSendPacket(
       kSsrc, 1, 1, false, PacedPacketInfo(PacedPacketInfo::kNotAProbe,
                                           kProbeMinBytes, kProbeMinBytes)));
   // Verify that TimeToSendPadding does not end up in a receiver.
@@ skipping 15 matching lines @@
   for (size_t i = 0; i < kNumPackets; ++i) {
     uint16_t seq = packet_router_->AllocateSequenceNumber();
     uint32_t expected_unwrapped_seq = static_cast<uint32_t>(kStartSeq) + i;
     EXPECT_EQ(static_cast<uint16_t>(expected_unwrapped_seq & 0xFFFF), seq);
   }
 }
 
 TEST_F(PacketRouterTest, SendTransportFeedback) {
   NiceMock<MockRtpRtcp> rtp_1;
   NiceMock<MockRtpRtcp> rtp_2;
-  packet_router_->AddSendRtpModule(&rtp_1);
-  packet_router_->AddReceiveRtpModule(&rtp_2);
+  packet_router_->AddSendRtpModule(&rtp_1, false);
+  packet_router_->AddReceiveRtpModule(&rtp_2, false);
 
   rtcp::TransportFeedback feedback;
   EXPECT_CALL(rtp_1, SendFeedbackPacket(_)).Times(1).WillOnce(Return(true));
   packet_router_->SendTransportFeedback(&feedback);
   packet_router_->RemoveSendRtpModule(&rtp_1);
   EXPECT_CALL(rtp_2, SendFeedbackPacket(_)).Times(1).WillOnce(Return(true));
   packet_router_->SendTransportFeedback(&feedback);
   packet_router_->RemoveReceiveRtpModule(&rtp_2);
 }
 
+#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
+TEST_F(PacketRouterTest, DoubleRegistrationOfSendModuleDisallowed) {
+  PacketRouter packet_router;
+
+  NiceMock<MockRtpRtcp> module;
+  OnSetREMBStatusAndSetRembAccordingly(&module);
+
+  constexpr bool remb_candidate = false;  // Value irrelevant.
+  packet_router.AddSendRtpModule(&module, remb_candidate);
+  EXPECT_DEATH(packet_router.AddSendRtpModule(&module, remb_candidate), "");
+
+  // Test tear-down
+  packet_router.RemoveSendRtpModule(&module);
+}
+
+TEST_F(PacketRouterTest, DoubleRegistrationOfReceiveModuleDisallowed) {
+  PacketRouter packet_router;
+
+  NiceMock<MockRtpRtcp> module;
+  OnSetREMBStatusAndSetRembAccordingly(&module);
+
+  constexpr bool remb_candidate = false;  // Value irrelevant.
+  packet_router.AddReceiveRtpModule(&module, remb_candidate);
+  EXPECT_DEATH(packet_router.AddReceiveRtpModule(&module, remb_candidate), "");
+
+  // Test tear-down
+  packet_router.RemoveReceiveRtpModule(&module);
+}
+
+TEST_F(PacketRouterTest, RemovalOfNeverAddedSendModuleDisallowed) {
+  PacketRouter packet_router;
+
+  NiceMock<MockRtpRtcp> module;
+  OnSetREMBStatusAndSetRembAccordingly(&module);
+
+  EXPECT_DEATH(packet_router.RemoveSendRtpModule(&module), "");
+}
+
+TEST_F(PacketRouterTest, RemovalOfNeverAddedReceiveModuleDisallowed) {
+  PacketRouter packet_router;
+
+  NiceMock<MockRtpRtcp> module;
+  OnSetREMBStatusAndSetRembAccordingly(&module);
+
+  EXPECT_DEATH(packet_router.RemoveReceiveRtpModule(&module), "");
+}
+#endif  // RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
+
+// TODO(eladalon): Remove this test; it should be covered by:
+// 1. SendCandidatePreferredOverReceiveCandidate_SendModuleAddedFirst
+// 2. SendCandidatePreferredOverReceiveCandidate_ReceiveModuleAddedFirst
+// 3. LowerEstimateToSendRemb
+// (Not removing in this CL to prove it doesn't break this test.)
 TEST(PacketRouterRembTest, PreferSendModuleOverReceiveModule) {
   rtc::ScopedFakeClock clock;
   NiceMock<MockRtpRtcp> rtp_recv;
   NiceMock<MockRtpRtcp> rtp_send;
   PacketRouter packet_router;
 
-  EXPECT_CALL(rtp_recv, SetREMBStatus(true)).Times(1);
-  packet_router.AddReceiveRtpModule(&rtp_recv);
+  ExpectSetREMBStatusAndSetRembAccordingly(&rtp_recv, true);
+  packet_router.AddReceiveRtpModule(&rtp_recv, true);
 
   const uint32_t bitrate_estimate = 456;
   const std::vector<uint32_t> ssrcs = {1234};
 
-  ON_CALL(rtp_recv, REMB()).WillByDefault(Return(true));
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
   // Call OnReceiveBitrateChanged twice to get a first estimate.
   clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
   EXPECT_CALL(rtp_recv, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
   // Add a send module, which should be preferred over the receive module.
-  EXPECT_CALL(rtp_recv, SetREMBStatus(false)).Times(1);
-  EXPECT_CALL(rtp_send, SetREMBStatus(true)).Times(1);
-  packet_router.AddSendRtpModule(&rtp_send);
-  ON_CALL(rtp_recv, REMB()).WillByDefault(Return(false));
-  ON_CALL(rtp_send, REMB()).WillByDefault(Return(true));
+  ExpectSetREMBStatusAndSetRembAccordingly(&rtp_recv, false);
+  ExpectSetREMBStatusAndSetRembAccordingly(&rtp_send, true);
+  packet_router.AddSendRtpModule(&rtp_send, true);
 
   // Lower bitrate to send another REMB packet.
   EXPECT_CALL(rtp_send, SetREMBData(bitrate_estimate - 100, ssrcs)).Times(1);
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate - 100);
 
-  EXPECT_CALL(rtp_send, SetREMBStatus(false)).Times(1);
-  EXPECT_CALL(rtp_recv, SetREMBStatus(true)).Times(1);
+  ExpectSetREMBStatusAndSetRembAccordingly(&rtp_send, false);
+  ExpectSetREMBStatusAndSetRembAccordingly(&rtp_recv, true);
   packet_router.RemoveSendRtpModule(&rtp_send);
-  EXPECT_CALL(rtp_recv, SetREMBStatus(false)).Times(1);
+
+  ExpectSetREMBStatusAndSetRembAccordingly(&rtp_recv, false);
   packet_router.RemoveReceiveRtpModule(&rtp_recv);
 }
 
 TEST(PacketRouterRembTest, LowerEstimateToSendRemb) {
   rtc::ScopedFakeClock clock;
   NiceMock<MockRtpRtcp> rtp;
   PacketRouter packet_router;
 
-  EXPECT_CALL(rtp, SetREMBStatus(true)).Times(1);
-  packet_router.AddSendRtpModule(&rtp);
+  ExpectSetREMBStatusAndSetRembAccordingly(&rtp, true);
+  packet_router.AddSendRtpModule(&rtp, true);
 
   uint32_t bitrate_estimate = 456;
   const std::vector<uint32_t> ssrcs = {1234};
 
-  ON_CALL(rtp, REMB()).WillByDefault(Return(true));
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
   // Call OnReceiveBitrateChanged twice to get a first estimate.
   clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
   EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
   // Lower the estimate with more than 3% to trigger a call to SetREMBData right
   // away.
   bitrate_estimate = bitrate_estimate - 100;
   EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
-  EXPECT_CALL(rtp, SetREMBStatus(false)).Times(1);
+  ExpectSetREMBStatusAndSetRembAccordingly(&rtp, false);
   packet_router.RemoveSendRtpModule(&rtp);
 }
 
 TEST(PacketRouterRembTest, VerifyIncreasingAndDecreasing) {
   rtc::ScopedFakeClock clock;
   NiceMock<MockRtpRtcp> rtp;
   PacketRouter packet_router;
-  packet_router.AddSendRtpModule(&rtp);
+  packet_router.AddSendRtpModule(&rtp, true);
 
   uint32_t bitrate_estimate[] = {456, 789};
   std::vector<uint32_t> ssrcs = {1234, 5678};
 
   ON_CALL(rtp, REMB()).WillByDefault(Return(true));
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate[0]);
 
   // Call OnReceiveBitrateChanged twice to get a first estimate.
   EXPECT_CALL(rtp, SetREMBData(bitrate_estimate[0], ssrcs)).Times(1);
   clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate[0]);
 
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate[1] + 100);
 
   // Lower the estimate to trigger a callback.
   EXPECT_CALL(rtp, SetREMBData(bitrate_estimate[1], ssrcs)).Times(1);
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate[1]);
 
   packet_router.RemoveSendRtpModule(&rtp);
 }
 
 TEST(PacketRouterRembTest, NoRembForIncreasedBitrate) {
   rtc::ScopedFakeClock clock;
   NiceMock<MockRtpRtcp> rtp;
   PacketRouter packet_router;
-  packet_router.AddSendRtpModule(&rtp);
+  packet_router.AddSendRtpModule(&rtp, true);
 
   uint32_t bitrate_estimate = 456;
   std::vector<uint32_t> ssrcs = {1234, 5678};
 
   ON_CALL(rtp, REMB()).WillByDefault(Return(true));
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
   // Call OnReceiveBitrateChanged twice to get a first estimate.
   EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
   clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
   // Increased estimate shouldn't trigger a callback right away.
   EXPECT_CALL(rtp, SetREMBData(_, _)).Times(0);
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate + 1);
 
   // Decreasing the estimate less than 3% shouldn't trigger a new callback.
   EXPECT_CALL(rtp, SetREMBData(_, _)).Times(0);
   int lower_estimate = bitrate_estimate * 98 / 100;
   packet_router.OnReceiveBitrateChanged(ssrcs, lower_estimate);
 
   packet_router.RemoveSendRtpModule(&rtp);
 }
 
 TEST(PacketRouterRembTest, ChangeSendRtpModule) {
   rtc::ScopedFakeClock clock;
   NiceMock<MockRtpRtcp> rtp_send;
   NiceMock<MockRtpRtcp> rtp_recv;
   PacketRouter packet_router;
-  packet_router.AddSendRtpModule(&rtp_send);
-  packet_router.AddReceiveRtpModule(&rtp_recv);
+  packet_router.AddSendRtpModule(&rtp_send, true);
+  packet_router.AddReceiveRtpModule(&rtp_recv, true);
 
   uint32_t bitrate_estimate = 456;
   std::vector<uint32_t> ssrcs = {1234, 5678};
 
   ON_CALL(rtp_send, REMB()).WillByDefault(Return(true));
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
   // Call OnReceiveBitrateChanged twice to get a first estimate.
   clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
   EXPECT_CALL(rtp_send, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
@@ skipping 16 matching lines @@
   EXPECT_CALL(rtp_recv, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
   packet_router.RemoveReceiveRtpModule(&rtp_recv);
 }
 
 TEST(PacketRouterRembTest, OnlyOneRembForRepeatedOnReceiveBitrateChanged) {
   rtc::ScopedFakeClock clock;
   NiceMock<MockRtpRtcp> rtp;
   PacketRouter packet_router;
-  packet_router.AddSendRtpModule(&rtp);
+  packet_router.AddSendRtpModule(&rtp, true);
 
   uint32_t bitrate_estimate = 456;
   const std::vector<uint32_t> ssrcs = {1234};
 
   ON_CALL(rtp, REMB()).WillByDefault(Return(true));
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
   // Call OnReceiveBitrateChanged twice to get a first estimate.
   clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
   EXPECT_CALL(rtp, SetREMBData(_, _)).Times(1);
@@ skipping 11 matching lines @@
 }
 
 // Only register receiving modules and make sure we fallback to trigger a REMB
 // packet on this one.
 TEST(PacketRouterRembTest, NoSendingRtpModule) {
   rtc::ScopedFakeClock clock;
   NiceMock<MockRtpRtcp> rtp;
   PacketRouter packet_router;
 
   EXPECT_CALL(rtp, SetREMBStatus(true)).Times(1);
-  packet_router.AddReceiveRtpModule(&rtp);
+  packet_router.AddReceiveRtpModule(&rtp, true);
 
   uint32_t bitrate_estimate = 456;
   const std::vector<uint32_t> ssrcs = {1234};
 
   ON_CALL(rtp, REMB()).WillByDefault(Return(true));
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
   // Call OnReceiveBitrateChanged twice to get a first estimate.
   clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
   EXPECT_CALL(rtp, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
 
   // Lower the estimate to trigger a new packet REMB packet.
   EXPECT_CALL(rtp, SetREMBData(bitrate_estimate - 100, ssrcs)).Times(1);
   packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate - 100);
 
   EXPECT_CALL(rtp, SetREMBStatus(false)).Times(1);
   packet_router.RemoveReceiveRtpModule(&rtp);
 }
 
+TEST(PacketRouterRembTest, NonCandidateSendRtpModuleNotUsedForRemb) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  NiceMock<MockRtpRtcp> module;
+
+  constexpr bool remb_candidate = false;
+  ON_CALL(module, REMB()).WillByDefault(Return(false));
+  EXPECT_CALL(module, SetREMBStatus(_)).Times(0);
+  packet_router.AddSendRtpModule(&module, remb_candidate);
+
+  constexpr uint32_t bitrate_estimate = 456;
+  const std::vector<uint32_t> ssrcs = {1234};
+  EXPECT_CALL(module, SetREMBData(_, _)).Times(0);
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));

[danilchap, 2017/07/28 11:35:04]

probably better to advance time after OnReceiveBitrateChanged:
remb sending normally happen within a second _after_ bitrate is changed.

[eladalon, 2017/07/28 15:47:11]

What effect would moving the clock have on the packet_router if does after the
last manipulation of packet_router? (This is a single-threaded test.)

[eladalon, 2017/07/28 15:47:12]

Done.

[danilchap, 2017/07/31 13:18:03]

Hm, true, it shouldn't. until clock will learn to run pending tasks during
AdvanceTime.
Let me reread tests keeping that in mind...

+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Test tear-down
+  packet_router.RemoveSendRtpModule(&module);
+}
+
+TEST(PacketRouterRembTest, CandidateSendRtpModuleUsedForRemb) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  NiceMock<MockRtpRtcp> module;
+
+  ExpectSetREMBStatusAndSetRembAccordingly(&module, true);
+  constexpr bool remb_candidate = true;
+  packet_router.AddSendRtpModule(&module, remb_candidate);
+
+  constexpr uint32_t bitrate_estimate = 456;
+  const std::vector<uint32_t> ssrcs = {1234};
+  EXPECT_CALL(module, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));

[danilchap, 2017/07/28 11:35:04]

either do not advance time at all (to stress remb is sent immidiately) or
advance it after new bitrate estimate. 

[eladalon, 2017/07/28 15:47:11]

OnReceiveBitrateChanged() won't have the expected effect if the clock is not
advanced beforehand. This might be seen as edge behavior, because
last_remb_time_ms_ is 0 and the fake clock is also 0. I could change
last_remb_time_ms_ to be an optional, but that doesn't seem to me to be
reasonable; we probably don't really want to send an REMB on the very first call
to PacketRouter::OnReceiveBitrateChanged(), because we probably don't have a
valid estimation yet.

Maybe moving AdvanceTime() to the beginning of the test would make things
clearer? I am not sure about this. Wdyt?

+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Test tear-down
+  ExpectSetREMBStatusAndSetRembAccordingly(&module, false);
+  packet_router.RemoveSendRtpModule(&module);
+}
+
+TEST(PacketRouterRembTest, NonCandidateReceiveRtpModuleNotUsedForRemb) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  NiceMock<MockRtpRtcp> module;
+
+  ON_CALL(module, REMB()).WillByDefault(Return(false));
+  EXPECT_CALL(module, SetREMBStatus(_)).Times(0);
+  constexpr bool remb_candidate = false;
+  packet_router.AddReceiveRtpModule(&module, remb_candidate);
+
+  constexpr uint32_t bitrate_estimate = 456;
+  const std::vector<uint32_t> ssrcs = {1234};
+  EXPECT_CALL(module, SetREMBData(_, _)).Times(0);
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Test tear-down
+  packet_router.RemoveReceiveRtpModule(&module);
+}
+
+TEST(PacketRouterRembTest, CandidateReceiveRtpModuleUsedForRemb) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  NiceMock<MockRtpRtcp> module;
+
+  ExpectSetREMBStatusAndSetRembAccordingly(&module, true);
+  constexpr bool remb_candidate = true;
+  packet_router.AddReceiveRtpModule(&module, remb_candidate);
+
+  constexpr uint32_t bitrate_estimate = 456;
+  const std::vector<uint32_t> ssrcs = {1234};
+  EXPECT_CALL(module, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Test tear-down
+  ExpectSetREMBStatusAndSetRembAccordingly(&module, false);
+  packet_router.RemoveReceiveRtpModule(&module);
+}
+
+TEST(PacketRouterRembTest,
+     SendCandidatePreferredOverReceiveCandidate_SendModuleAddedFirst) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  NiceMock<MockRtpRtcp> send_module;
+  NiceMock<MockRtpRtcp> receive_module;
+
+  constexpr bool remb_candidate = true;
+
+  // Send module added - activated.
+  ExpectSetREMBStatusAndSetRembAccordingly(&send_module, true);
+  packet_router.AddSendRtpModule(&send_module, remb_candidate);
+
+  // Receive module added - the send module remains the active one.
+  ON_CALL(receive_module, REMB()).WillByDefault(Return(false));
+  EXPECT_CALL(receive_module, SetREMBStatus(true)).Times(0);
+  packet_router.AddReceiveRtpModule(&receive_module, remb_candidate);
+
+  constexpr uint32_t bitrate_estimate = 456;
+  const std::vector<uint32_t> ssrcs = {1234};
+  EXPECT_CALL(send_module, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
+  EXPECT_CALL(receive_module, SetREMBData(_, _)).Times(0);
+
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Test tear-down
+  packet_router.RemoveReceiveRtpModule(&receive_module);
+  ExpectSetREMBStatusAndSetRembAccordingly(&send_module, false);
+  packet_router.RemoveSendRtpModule(&send_module);
+}
+
+TEST(PacketRouterRembTest,
+     SendCandidatePreferredOverReceiveCandidate_ReceiveModuleAddedFirst) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  NiceMock<MockRtpRtcp> send_module;
+  NiceMock<MockRtpRtcp> receive_module;
+
+  constexpr bool remb_candidate = true;
+
+  // Receive module added - activated.
+  ExpectSetREMBStatusAndSetRembAccordingly(&receive_module, true);
+  packet_router.AddReceiveRtpModule(&receive_module, remb_candidate);
+
+  // Send module added - replaces receive module as active.
+  ExpectSetREMBStatusAndSetRembAccordingly(&receive_module, false);
+  ExpectSetREMBStatusAndSetRembAccordingly(&send_module, true);
+  packet_router.AddSendRtpModule(&send_module, remb_candidate);
+
+  constexpr uint32_t bitrate_estimate = 456;
+  const std::vector<uint32_t> ssrcs = {1234};
+  EXPECT_CALL(send_module, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
+  EXPECT_CALL(receive_module, SetREMBData(_, _)).Times(0);
+
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Test tear-down
+  packet_router.RemoveReceiveRtpModule(&receive_module);
+  ExpectSetREMBStatusAndSetRembAccordingly(&send_module, false);
+  packet_router.RemoveSendRtpModule(&send_module);
+}
+
+TEST(PacketRouterRembTest, ReceiveModuleTakesOverWhenLastSendModuleRemoved) {
+  rtc::ScopedFakeClock clock;
+  PacketRouter packet_router;
+  NiceMock<MockRtpRtcp> send_module;
+  NiceMock<MockRtpRtcp> receive_module;
+
+  constexpr bool remb_candidate = true;
+
+  // Send module added - activated.
+  ExpectSetREMBStatusAndSetRembAccordingly(&send_module, true);
+  packet_router.AddSendRtpModule(&send_module, remb_candidate);
+
+  // Receive module added - the send module remains the active one.
+  ON_CALL(receive_module, REMB()).WillByDefault(Return(false));
+  EXPECT_CALL(receive_module, SetREMBStatus(true)).Times(0);
+  packet_router.AddReceiveRtpModule(&receive_module, remb_candidate);
+
+  // Send module removed - receive module becomes active.
+  ExpectSetREMBStatusAndSetRembAccordingly(&send_module, false);
+  ExpectSetREMBStatusAndSetRembAccordingly(&receive_module, true);
+  packet_router.RemoveSendRtpModule(&send_module);
+
+  constexpr uint32_t bitrate_estimate = 456;
+  const std::vector<uint32_t> ssrcs = {1234};
+  EXPECT_CALL(send_module, SetREMBData(_, _)).Times(0);
+  EXPECT_CALL(receive_module, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
+
+  clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1000));
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Test tear-down
+  ExpectSetREMBStatusAndSetRembAccordingly(&receive_module, false);
+  packet_router.RemoveReceiveRtpModule(&receive_module);
+}
+
+// TODO(eladalon): Tests for a module which is both receive as well as send?

[danilchap, 2017/07/27 20:00:20]

do not think we have those modules. At least for video.

[eladalon, 2017/07/27 21:30:54]

Acknowledged; will remove this TODO with the next revision (which I'll upload
after I have your feedback on the other comment).

+
 }  // namespace webrtc