Delete VieRemb class, move functionality to PacketRouter.

webrtc/modules/pacing/packet_router_unittest.cc

Index: webrtc/modules/pacing/packet_router_unittest.cc
diff --git a/webrtc/modules/pacing/packet_router_unittest.cc b/webrtc/modules/pacing/packet_router_unittest.cc
index be28bf1dbe65bb705036d96a25a5eb6a7058832b..d91e34727e95d5deca76cb36fd26265a827bd0eb 100644
--- a/webrtc/modules/pacing/packet_router_unittest.cc
+++ b/webrtc/modules/pacing/packet_router_unittest.cc
@@ -12,6 +12,7 @@
 #include <memory>
 
 #include "webrtc/base/checks.h"
+#include "webrtc/base/fakeclock.h"
 #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"
@@ -242,7 +243,7 @@ TEST_F(PacketRouterTest, AllocateSequenceNumbers) {
   }
 }
 
-TEST_F(PacketRouterTest, SendFeedback) {
+TEST_F(PacketRouterTest, SendTransportFeedback) {
   MockRtpRtcp rtp_1;
   MockRtpRtcp rtp_2;
   packet_router_->AddSendRtpModule(&rtp_1);
@@ -250,10 +251,265 @@ TEST_F(PacketRouterTest, SendFeedback) {
 
   rtcp::TransportFeedback feedback;
   EXPECT_CALL(rtp_1, SendFeedbackPacket(_)).Times(1);
-  packet_router_->SendFeedback(&feedback);
+  packet_router_->SendTransportFeedback(&feedback);
   packet_router_->RemoveSendRtpModule(&rtp_1);
   EXPECT_CALL(rtp_2, SendFeedbackPacket(_)).Times(1);
-  packet_router_->SendFeedback(&feedback);
+  packet_router_->SendTransportFeedback(&feedback);
   packet_router_->RemoveReceiveRtpModule(&rtp_2);
 }
+
+// TODO(nisse): This test is a bit pointless. Do we really want to

[danilchap, 2017/04/11 12:34:14]

Is there a use case where same module used both as sender and receiver (audio?)
If not - probably better to remove this test - you do not want to advocate using
same module both as sender and receiver.
If yes - mention the use case, probably with todo to remove this test when use
case will be gone.

May be this test will make sense if different modules will be used as send and
receive (i.e. 'PreferSenderModuleToSendRemb'. in that test it would also make
sense to add receive module 1st and send module 2nd to test remb handover).

[nisse-webrtc, 2017/04/11 14:30:14]

Reworked to test handover.

+// support the same module being registered twice? The REMBStatus
+// logic probably won't do the right thing in this case.

[stefan-webrtc, 2017/04/11 09:28:36]

Can this even be done in the code right now? I thought send-side and
receive-side were separated enough that this wouldn't be the case?

[nisse-webrtc, 2017/04/11 10:57:41]

I don't think there's any code that calls AddSendRtpModule and
AddReceiveRtpModule with the same module. Except this test.

+TEST(PacketRouterRembTest, OneModuleTestForSendingRemb) {
+  rtc::ScopedFakeClock clock;
+  MockRtpRtcp rtp;
+  PacketRouter packet_router;
+
+  EXPECT_CALL(rtp, SetREMBStatus(true)).Times(1);
+  packet_router.AddSendRtpModule(&rtp);
+  packet_router.AddReceiveRtpModule(&rtp);
+
+  const uint32_t bitrate_estimate = 456;
+  uint32_t ssrc = 1234;
+  std::vector<uint32_t> ssrcs(&ssrc, &ssrc + 1);
+
+  EXPECT_CALL(rtp, REMB()).WillRepeatedly(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 bitrate to send another 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);
+  packet_router.RemoveSendRtpModule(&rtp);
+}
+
+TEST(PacketRouterRembTest, LowerEstimateToSendRemb) {
+  rtc::ScopedFakeClock clock;
+  MockRtpRtcp rtp;
+  PacketRouter packet_router;
+
+  EXPECT_CALL(rtp, SetREMBStatus(true)).Times(1);
+  packet_router.AddSendRtpModule(&rtp);
+
+  uint32_t bitrate_estimate = 456;
+  uint32_t ssrc = 1234;
+  std::vector<uint32_t> ssrcs(&ssrc, &ssrc + 1);

[danilchap, 2017/04/11 12:34:14]

std::vector<uint32_t> ssrcs = {1234};
likely cleaner.

[nisse-webrtc, 2017/04/11 14:30:14]

Done, all occurences.

+
+  EXPECT_CALL(rtp, REMB()).WillRepeatedly(Return(true));

[danilchap, 2017/04/11 12:34:14]

do you mean ON_CALL here? (i.e. you do not want to test how many times REMB()
was called)

[nisse-webrtc, 2017/04/11 14:30:14]

Done. All replaced with ON_CALL(...).WillByDefault(Return(...))

+  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);
+  packet_router.RemoveSendRtpModule(&rtp);
+}
+
+TEST(PacketRouterRembTest, UseReceiveModule) {

[danilchap, 2017/04/11 12:34:14]

may be name test 'UseReceiveModuleWhenNoSendModules'
how is it differerent to the last TEST in this file: 'NoSendingRtpModule'?

[nisse-webrtc, 2017/04/11 14:30:14]

No interesting difference, it just uses a bit stricter EXPECT_CALLs. I'll delete
this, and copy the expectations to the other test.

(I copied tests from vie_remb_unittest.cc quite mindlessly...)

+  rtc::ScopedFakeClock clock;
+  MockRtpRtcp rtp;
+  PacketRouter packet_router;
+
+  EXPECT_CALL(rtp, SetREMBStatus(true)).Times(1);
+  packet_router.AddReceiveRtpModule(&rtp);
+
+  uint32_t bitrate_estimate = 456;
+  uint32_t ssrc = 1234;
+  std::vector<uint32_t> ssrcs(&ssrc, &ssrc + 1);
+
+  EXPECT_CALL(rtp, REMB()).WillRepeatedly(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);
+  packet_router.RemoveReceiveRtpModule(&rtp);
+}
+
+TEST(PacketRouterRembTest, VerifyIncreasingAndDecreasing) {
+  rtc::ScopedFakeClock clock;
+  MockRtpRtcp rtp_0;
+  MockRtpRtcp rtp_1;  // TODO(nisse): How is this relevant to this test?

[danilchap, 2017/04/11 12:34:14]

May be add test 'PreferSenderModuleToSendRemb' where modules are called
sender_rtp/receiver_rtp, 
then it will make sense to delete rtp_1 in this and below tests.

[nisse-webrtc, 2017/04/11 14:30:14]

Deleting rtp_1. There are now two tests  using rtp_send and rtp_recv.

+  PacketRouter packet_router;
+  packet_router.AddSendRtpModule(&rtp_0);
+  packet_router.AddReceiveRtpModule(&rtp_1);
+
+  uint32_t bitrate_estimate[] = {456, 789};
+  uint32_t ssrc[] = {1234, 5678};
+  std::vector<uint32_t> ssrcs(ssrc, ssrc + sizeof(ssrc) / sizeof(ssrc[0]));
+
+  EXPECT_CALL(rtp_0, REMB()).WillRepeatedly(Return(true));
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate[0]);
+
+  // Call OnReceiveBitrateChanged twice to get a first estimate.
+  EXPECT_CALL(rtp_0, 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_0, SetREMBData(bitrate_estimate[1], ssrcs)).Times(1);
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate[1]);
+
+  packet_router.RemoveSendRtpModule(&rtp_0);
+  packet_router.RemoveReceiveRtpModule(&rtp_1);
+}
+
+TEST(PacketRouterRembTest, NoRembForIncreasedBitrate) {
+  rtc::ScopedFakeClock clock;
+  MockRtpRtcp rtp_0;
+  MockRtpRtcp rtp_1;  // TODO(nisse): How is this relevant to this test?
+  PacketRouter packet_router;
+  packet_router.AddSendRtpModule(&rtp_0);
+  packet_router.AddReceiveRtpModule(&rtp_1);
+
+  uint32_t bitrate_estimate = 456;
+  uint32_t ssrc[] = {1234, 5678};
+  std::vector<uint32_t> ssrcs(ssrc, ssrc + sizeof(ssrc) / sizeof(ssrc[0]));
+
+  EXPECT_CALL(rtp_0, REMB()).WillRepeatedly(Return(true));
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Call OnReceiveBitrateChanged twice to get a first estimate.
+  EXPECT_CALL(rtp_0, 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_0, 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_0, SetREMBData(_, _)).Times(0);
+  int lower_estimate = bitrate_estimate * 98 / 100;
+  packet_router.OnReceiveBitrateChanged(ssrcs, lower_estimate);
+
+  packet_router.RemoveSendRtpModule(&rtp_0);
+  packet_router.RemoveReceiveRtpModule(&rtp_1);
+}
+
+TEST(PacketRouterRembTest, ChangeSendRtpModule) {
+  rtc::ScopedFakeClock clock;
+  MockRtpRtcp rtp_0;
+  MockRtpRtcp rtp_1;
+  PacketRouter packet_router;
+  packet_router.AddSendRtpModule(&rtp_0);
+  packet_router.AddReceiveRtpModule(&rtp_1);
+
+  uint32_t bitrate_estimate = 456;
+  uint32_t ssrc[] = {1234, 5678};
+  std::vector<uint32_t> ssrcs(ssrc, ssrc + sizeof(ssrc) / sizeof(ssrc[0]));
+
+  EXPECT_CALL(rtp_0, REMB()).WillRepeatedly(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_0, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Decrease estimate to trigger a REMB.
+  bitrate_estimate = bitrate_estimate - 100;
+  EXPECT_CALL(rtp_0, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Remove the sending module -> should get remb on the second module.
+  packet_router.RemoveSendRtpModule(&rtp_0);
+
+  EXPECT_CALL(rtp_0, REMB()).WillRepeatedly(Return(false));
+  EXPECT_CALL(rtp_1, REMB()).WillRepeatedly(Return(true));
+
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  bitrate_estimate = bitrate_estimate - 100;
+  EXPECT_CALL(rtp_1, SetREMBData(bitrate_estimate, ssrcs)).Times(1);
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  packet_router.RemoveReceiveRtpModule(&rtp_1);
+}
+
+TEST(PacketRouterRembTest, OnlyOneRembForDoubleProcess) {

[stefan-webrtc, 2017/04/11 09:28:36]

Process has no meaning below. Should it say OnReceiveBitrateChanged?

[nisse-webrtc, 2017/04/11 14:30:14]

Renamed to OnlyOneRembForRepeatedOnReceiveBitrateChanged.

+  rtc::ScopedFakeClock clock;
+  MockRtpRtcp rtp;
+  PacketRouter packet_router;
+  packet_router.AddSendRtpModule(&rtp);
+
+  uint32_t bitrate_estimate = 456;
+  uint32_t ssrc = 1234;
+  std::vector<uint32_t> ssrcs(&ssrc, &ssrc + 1);
+
+  EXPECT_CALL(rtp, REMB()).WillRepeatedly(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);
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Lower the estimate, should 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);
+
+  // Call OnReceiveBitrateChanged again, this should not trigger a new callback.
+  EXPECT_CALL(rtp, SetREMBData(_, _)).Times(0);
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+  packet_router.RemoveSendRtpModule(&rtp);
+}
+
+// Only register receiving modules and make sure we fallback to trigger a REMB
+// packet on this one.
+TEST(PacketRouterRembTest, NoSendingRtpModule) {
+  rtc::ScopedFakeClock clock;
+  MockRtpRtcp rtp;
+  PacketRouter packet_router;
+
+  packet_router.AddReceiveRtpModule(&rtp);
+
+  uint32_t bitrate_estimate = 456;
+  uint32_t ssrc = 1234;
+  std::vector<uint32_t> ssrcs(&ssrc, &ssrc + 1);
+
+  EXPECT_CALL(rtp, REMB()).WillRepeatedly(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);
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  // Lower the estimate to trigger a new packet REMB packet.
+  bitrate_estimate = bitrate_estimate - 100;
+  EXPECT_CALL(rtp, SetREMBData(_, _)).Times(1);
+  packet_router.OnReceiveBitrateChanged(ssrcs, bitrate_estimate);
+
+  packet_router.RemoveReceiveRtpModule(&rtp);
+}
+
 }  // namespace webrtc