From ee153c92fef781ac5d3bd2e5dba1de8a0badf54f Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Thu, 10 Oct 2019 16:43:46 +0200 Subject: [PATCH] Send rtcp::RemoteEstimate and rtcp::TransportFeedback in one packet Change-Id: I53912f4e82a9fd795f8886d6b2cdb313bde08c4d BUG: webrtc:10742 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/156380 Reviewed-by: Sebastian Jansson Reviewed-by: Danil Chapovalov Commit-Queue: Per Kjellander Cr-Commit-Position: refs/heads/master@{#29437} --- modules/pacing/packet_router.cc | 34 +- modules/pacing/packet_router.h | 11 +- modules/pacing/packet_router_unittest.cc | 21 +- modules/remote_bitrate_estimator/BUILD.gn | 1 + .../include/remote_bitrate_estimator.h | 10 +- .../remote_estimator_proxy.cc | 31 +- .../remote_estimator_proxy_unittest.cc | 403 +++++++++++------- 7 files changed, 302 insertions(+), 209 deletions(-) diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index 229cdb36b6..2946b5c139 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -13,11 +13,13 @@ #include #include #include +#include #include #include "absl/types/optional.h" #include "modules/rtp_rtcp/include/rtp_rtcp.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "modules/rtp_rtcp/source/rtcp_packet.h" #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "rtc_base/atomic_ops.h" #include "rtc_base/checks.h" @@ -274,33 +276,25 @@ bool PacketRouter::SendRemb(int64_t bitrate_bps, return true; } -bool PacketRouter::SendTransportFeedback(rtcp::TransportFeedback* packet) { +bool PacketRouter::SendCombinedRtcpPacket( + std::vector> packets) { rtc::CritScope cs(&modules_crit_); + // Prefer send modules. for (auto* rtp_module : rtp_send_modules_) { - packet->SetSenderSsrc(rtp_module->SSRC()); - if (rtp_module->SendFeedbackPacket(*packet)) { - return true; + if (rtp_module->RTCP() == RtcpMode::kOff) { + continue; } + rtp_module->SendCombinedRtcpPacket(std::move(packets)); + return true; } - for (auto* rtcp_sender : rtcp_feedback_senders_) { - packet->SetSenderSsrc(rtcp_sender->SSRC()); - if (rtcp_sender->SendFeedbackPacket(*packet)) { - return true; - } - } - return false; -} -void PacketRouter::SendNetworkStateEstimatePacket( - rtcp::RemoteEstimate* packet) { - rtc::CritScope cs(&modules_crit_); - for (auto* rtcp_sender : rtcp_feedback_senders_) { - packet->SetSenderSsrc(rtcp_sender->SSRC()); - if (rtcp_sender->SendNetworkStateEstimatePacket(*packet)) { - break; - } + if (rtcp_feedback_senders_.empty()) { + return false; } + auto* rtcp_sender = rtcp_feedback_senders_[0]; + rtcp_sender->SendCombinedRtcpPacket(std::move(packets)); + return true; } void PacketRouter::AddRembModuleCandidate( diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index be535fec60..85aa003696 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -22,6 +22,7 @@ #include "api/transport/network_types.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "modules/rtp_rtcp/source/rtcp_packet.h" #include "modules/rtp_rtcp/source/rtp_packet_to_send.h" #include "rtc_base/constructor_magic.h" #include "rtc_base/critical_section.h" @@ -30,9 +31,6 @@ namespace webrtc { class RtpRtcp; -namespace rtcp { -class TransportFeedback; -} // namespace rtcp // PacketRouter keeps track of rtp send modules to support the pacer. // In addition, it handles feedback messages, which are sent on a send @@ -76,10 +74,9 @@ class PacketRouter : public RemoteBitrateObserver, // Send REMB feedback. bool SendRemb(int64_t bitrate_bps, const std::vector& ssrcs); - // Send transport feedback packet to send-side. - bool SendTransportFeedback(rtcp::TransportFeedback* packet) override; - // Send RemoteEstimate packet to send-side. - void SendNetworkStateEstimatePacket(rtcp::RemoteEstimate* packet) override; + // Sends |packets| in one or more IP packets. + bool SendCombinedRtcpPacket( + std::vector> packets) override; private: RtpRtcp* FindRtpModule(uint32_t ssrc) diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc index 22de6179ab..3fd9882207 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -89,9 +89,10 @@ TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_SendRemb) { } TEST_F(PacketRouterTest, Sanity_NoModuleRegistered_SendTransportFeedback) { - rtcp::TransportFeedback feedback; + std::vector> feedback; + feedback.push_back(std::make_unique()); - EXPECT_FALSE(packet_router_.SendTransportFeedback(&feedback)); + EXPECT_FALSE(packet_router_.SendCombinedRtcpPacket(std::move(feedback))); } TEST_F(PacketRouterTest, GeneratePaddingPicksCorrectModule) { @@ -242,15 +243,21 @@ TEST_F(PacketRouterTest, SendTransportFeedback) { NiceMock rtp_1; NiceMock rtp_2; + ON_CALL(rtp_1, RTCP()).WillByDefault(Return(RtcpMode::kCompound)); + ON_CALL(rtp_2, RTCP()).WillByDefault(Return(RtcpMode::kCompound)); + 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); + std::vector> feedback; + feedback.push_back(std::make_unique()); + EXPECT_CALL(rtp_1, SendCombinedRtcpPacket).Times(1); + packet_router_.SendCombinedRtcpPacket(std::move(feedback)); packet_router_.RemoveSendRtpModule(&rtp_1); - EXPECT_CALL(rtp_2, SendFeedbackPacket(_)).Times(1).WillOnce(Return(true)); - packet_router_.SendTransportFeedback(&feedback); + EXPECT_CALL(rtp_2, SendCombinedRtcpPacket).Times(1); + std::vector> new_feedback; + new_feedback.push_back(std::make_unique()); + packet_router_.SendCombinedRtcpPacket(std::move(new_feedback)); packet_router_.RemoveReceiveRtpModule(&rtp_2); } diff --git a/modules/remote_bitrate_estimator/BUILD.gn b/modules/remote_bitrate_estimator/BUILD.gn index cec58a1811..aa046d5ae4 100644 --- a/modules/remote_bitrate_estimator/BUILD.gn +++ b/modules/remote_bitrate_estimator/BUILD.gn @@ -115,6 +115,7 @@ if (rtc_include_tests) { "../..:webrtc_common", "../../api/transport:field_trial_based_config", "../../api/transport:mock_network_control", + "../../api/transport:network_control", "../../rtc_base", "../../rtc_base:checks", "../../rtc_base:rtc_base_approved", diff --git a/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h b/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h index a6bba70db1..c60c030e8d 100644 --- a/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h +++ b/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h @@ -14,16 +14,15 @@ #define MODULES_REMOTE_BITRATE_ESTIMATOR_INCLUDE_REMOTE_BITRATE_ESTIMATOR_H_ #include +#include #include #include "modules/include/module.h" #include "modules/include/module_common_types.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "modules/rtp_rtcp/source/rtcp_packet.h" namespace webrtc { -namespace rtcp { -class TransportFeedback; -} // namespace rtcp class Clock; @@ -42,8 +41,9 @@ class RemoteBitrateObserver { class TransportFeedbackSenderInterface { public: virtual ~TransportFeedbackSenderInterface() = default; - virtual bool SendTransportFeedback(rtcp::TransportFeedback* packet) = 0; - virtual void SendNetworkStateEstimatePacket(rtcp::RemoteEstimate* packet) = 0; + + virtual bool SendCombinedRtcpPacket( + std::vector> packets) = 0; }; // TODO(holmer): Remove when all implementations have been updated. diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc index e9b61004be..f66b37046a 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc @@ -12,6 +12,8 @@ #include #include +#include +#include #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "rtc_base/checks.h" @@ -187,13 +189,13 @@ void RemoteEstimatorProxy::SendPeriodicFeedbacks() { if (!periodic_window_start_seq_) return; + std::unique_ptr remote_estimate; if (network_state_estimator_) { absl::optional state_estimate = network_state_estimator_->GetCurrentEstimate(); if (state_estimate) { - rtcp::RemoteEstimate rtcp_estimate; - rtcp_estimate.SetEstimate(state_estimate.value()); - feedback_sender_->SendNetworkStateEstimatePacket(&rtcp_estimate); + remote_estimate = std::make_unique(); + remote_estimate->SetEstimate(state_estimate.value()); } } @@ -202,13 +204,20 @@ void RemoteEstimatorProxy::SendPeriodicFeedbacks() { begin_iterator != packet_arrival_times_.cend(); begin_iterator = packet_arrival_times_.lower_bound(*periodic_window_start_seq_)) { - rtcp::TransportFeedback feedback_packet; + auto feedback_packet = std::make_unique(); periodic_window_start_seq_ = BuildFeedbackPacket( feedback_packet_count_++, media_ssrc_, *periodic_window_start_seq_, - begin_iterator, packet_arrival_times_.cend(), &feedback_packet); + begin_iterator, packet_arrival_times_.cend(), feedback_packet.get()); RTC_DCHECK(feedback_sender_ != nullptr); - feedback_sender_->SendTransportFeedback(&feedback_packet); + + std::vector> packets; + if (remote_estimate) { + packets.push_back(std::move(remote_estimate)); + } + packets.push_back(std::move(feedback_packet)); + + feedback_sender_->SendCombinedRtcpPacket(std::move(packets)); // Note: Don't erase items from packet_arrival_times_ after sending, in case // they need to be re-sent after a reordering. Removal will be handled // by OnPacketArrival once packets are too old. @@ -221,7 +230,9 @@ void RemoteEstimatorProxy::SendFeedbackOnRequest( if (feedback_request.sequence_count == 0) { return; } - rtcp::TransportFeedback feedback_packet(feedback_request.include_timestamps); + + auto feedback_packet = std::make_unique( + feedback_request.include_timestamps); int64_t first_sequence_number = sequence_number - feedback_request.sequence_count + 1; @@ -231,13 +242,15 @@ void RemoteEstimatorProxy::SendFeedbackOnRequest( BuildFeedbackPacket(feedback_packet_count_++, media_ssrc_, first_sequence_number, begin_iterator, end_iterator, - &feedback_packet); + feedback_packet.get()); // Clear up to the first packet that is included in this feedback packet. packet_arrival_times_.erase(packet_arrival_times_.begin(), begin_iterator); RTC_DCHECK(feedback_sender_ != nullptr); - feedback_sender_->SendTransportFeedback(&feedback_packet); + std::vector> packets; + packets.push_back(std::move(feedback_packet)); + feedback_sender_->SendCombinedRtcpPacket(std::move(packets)); } int64_t RemoteEstimatorProxy::BuildFeedbackPacket( diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc index 30e6ef4102..2d2d8af52c 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc @@ -10,7 +10,11 @@ #include "modules/remote_bitrate_estimator/remote_estimator_proxy.h" +#include +#include + #include "api/transport/field_trial_based_config.h" +#include "api/transport/network_types.h" #include "api/transport/test/mock_network_control.h" #include "modules/pacing/packet_router.h" #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" @@ -22,6 +26,7 @@ using ::testing::_; using ::testing::ElementsAre; using ::testing::Invoke; using ::testing::Return; +using ::testing::SizeIs; namespace webrtc { namespace { @@ -60,10 +65,9 @@ std::vector TimestampsMs( class MockTransportFeedbackSender : public TransportFeedbackSenderInterface { public: - MOCK_METHOD1(SendTransportFeedback, - bool(rtcp::TransportFeedback* feedback_packet)); - MOCK_METHOD1(SendNetworkStateEstimatePacket, - void(rtcp::RemoteEstimate* packet)); + MOCK_METHOD1( + SendCombinedRtcpPacket, + bool(std::vector> feedback_packets)); }; class RemoteEstimatorProxyTest : public ::testing::Test { @@ -116,15 +120,21 @@ class RemoteEstimatorProxyTest : public ::testing::Test { TEST_F(RemoteEstimatorProxyTest, SendsSinglePacketFeedback) { IncomingPacket(kBaseSeq, kBaseTimeMs); - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), ElementsAre(kBaseSeq)); - EXPECT_THAT(TimestampsMs(*feedback_packet), ElementsAre(kBaseTimeMs)); - return true; - })); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs)); + return true; + })); Process(); } @@ -133,15 +143,21 @@ TEST_F(RemoteEstimatorProxyTest, DuplicatedPackets) { IncomingPacket(kBaseSeq, kBaseTimeMs); IncomingPacket(kBaseSeq, kBaseTimeMs + 1000); - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), ElementsAre(kBaseSeq)); - EXPECT_THAT(TimestampsMs(*feedback_packet), ElementsAre(kBaseTimeMs)); - return true; - })); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs)); + return true; + })); Process(); } @@ -150,23 +166,27 @@ TEST_F(RemoteEstimatorProxyTest, FeedbackWithMissingStart) { // First feedback. IncomingPacket(kBaseSeq, kBaseTimeMs); IncomingPacket(kBaseSeq + 1, kBaseTimeMs + 1000); - EXPECT_CALL(router_, SendTransportFeedback(_)).WillOnce(Return(true)); + EXPECT_CALL(router_, SendCombinedRtcpPacket).WillOnce(Return(true)); Process(); // Second feedback starts with a missing packet (DROP kBaseSeq + 2). IncomingPacket(kBaseSeq + 3, kBaseTimeMs + 3000); - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq + 2, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq + 2, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), - ElementsAre(kBaseSeq + 3)); - EXPECT_THAT(TimestampsMs(*feedback_packet), - ElementsAre(kBaseTimeMs + 3000)); - return true; - })); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq + 3)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + 3000)); + return true; + })); Process(); } @@ -176,18 +196,22 @@ TEST_F(RemoteEstimatorProxyTest, SendsFeedbackWithVaryingDeltas) { IncomingPacket(kBaseSeq + 1, kBaseTimeMs + kMaxSmallDeltaMs); IncomingPacket(kBaseSeq + 2, kBaseTimeMs + (2 * kMaxSmallDeltaMs) + 1); - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), - ElementsAre(kBaseSeq, kBaseSeq + 1, kBaseSeq + 2)); - EXPECT_THAT(TimestampsMs(*feedback_packet), - ElementsAre(kBaseTimeMs, kBaseTimeMs + kMaxSmallDeltaMs, - kBaseTimeMs + (2 * kMaxSmallDeltaMs) + 1)); - return true; - })); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq, kBaseSeq + 1, kBaseSeq + 2)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs, kBaseTimeMs + kMaxSmallDeltaMs, + kBaseTimeMs + (2 * kMaxSmallDeltaMs) + 1)); + return true; + })); Process(); } @@ -199,25 +223,35 @@ TEST_F(RemoteEstimatorProxyTest, SendsFragmentedFeedback) { IncomingPacket(kBaseSeq, kBaseTimeMs); IncomingPacket(kBaseSeq + 1, kBaseTimeMs + kTooLargeDelta); - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), ElementsAre(kBaseSeq)); - EXPECT_THAT(TimestampsMs(*feedback_packet), ElementsAre(kBaseTimeMs)); - return true; - })) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq + 1, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs)); + return true; + })) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq + 1, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), - ElementsAre(kBaseSeq + 1)); - EXPECT_THAT(TimestampsMs(*feedback_packet), - ElementsAre(kBaseTimeMs + kTooLargeDelta)); - return true; - })); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq + 1)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + kTooLargeDelta)); + return true; + })); Process(); } @@ -228,15 +262,19 @@ TEST_F(RemoteEstimatorProxyTest, HandlesReorderingAndWrap) { IncomingPacket(kBaseSeq, kBaseTimeMs); IncomingPacket(kLargeSeq, kBaseTimeMs + kDeltaMs); - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([&](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kLargeSeq, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [&](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kLargeSeq, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(TimestampsMs(*feedback_packet), - ElementsAre(kBaseTimeMs + kDeltaMs, kBaseTimeMs)); - return true; - })); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + kDeltaMs, kBaseTimeMs)); + return true; + })); Process(); } @@ -254,17 +292,21 @@ TEST_F(RemoteEstimatorProxyTest, HandlesMalformedSequenceNumbers) { } // Only expect feedback for the last two packets. - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([&](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq + 20000 + 9, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), - ElementsAre(kBaseSeq + 20009, kBaseSeq + 40009)); - EXPECT_THAT(TimestampsMs(*feedback_packet), - ElementsAre(kBaseTimeMs + 28 * kDeltaMs, - kBaseTimeMs + 29 * kDeltaMs)); - return true; - })); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [&](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq + 20000 + 9, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq + 20009, kBaseSeq + 40009)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + 28 * kDeltaMs, + kBaseTimeMs + 29 * kDeltaMs)); + return true; + })); Process(); } @@ -281,16 +323,20 @@ TEST_F(RemoteEstimatorProxyTest, HandlesBackwardsWrappingSequenceNumbers) { } // Only expect feedback for the first two packets. - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([&](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq + 40000, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), - ElementsAre(kBaseSeq + 40000, kBaseSeq)); - EXPECT_THAT(TimestampsMs(*feedback_packet), - ElementsAre(kBaseTimeMs + kDeltaMs, kBaseTimeMs)); - return true; - })); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [&](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq + 40000, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq + 40000, kBaseSeq)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + kDeltaMs, kBaseTimeMs)); + return true; + })); Process(); } @@ -299,33 +345,41 @@ TEST_F(RemoteEstimatorProxyTest, ResendsTimestampsOnReordering) { IncomingPacket(kBaseSeq, kBaseTimeMs); IncomingPacket(kBaseSeq + 2, kBaseTimeMs + 2); - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), - ElementsAre(kBaseSeq, kBaseSeq + 2)); - EXPECT_THAT(TimestampsMs(*feedback_packet), - ElementsAre(kBaseTimeMs, kBaseTimeMs + 2)); - return true; - })); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq, kBaseSeq + 2)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs, kBaseTimeMs + 2)); + return true; + })); Process(); IncomingPacket(kBaseSeq + 1, kBaseTimeMs + 1); - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq + 1, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq + 1, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), - ElementsAre(kBaseSeq + 1, kBaseSeq + 2)); - EXPECT_THAT(TimestampsMs(*feedback_packet), - ElementsAre(kBaseTimeMs + 1, kBaseTimeMs + 2)); - return true; - })); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq + 1, kBaseSeq + 2)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + 1, kBaseTimeMs + 2)); + return true; + })); Process(); } @@ -335,21 +389,29 @@ TEST_F(RemoteEstimatorProxyTest, RemovesTimestampsOutOfScope) { IncomingPacket(kBaseSeq + 2, kBaseTimeMs); - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq + 2, feedback_packet->GetBaseSequence()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq + 2, feedback_packet->GetBaseSequence()); - EXPECT_THAT(TimestampsMs(*feedback_packet), ElementsAre(kBaseTimeMs)); - return true; - })); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs)); + return true; + })); Process(); IncomingPacket(kBaseSeq + 3, kTimeoutTimeMs); // kBaseSeq + 2 times out here. - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce( - Invoke([kTimeoutTimeMs](rtcp::TransportFeedback* feedback_packet) { + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [&](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); EXPECT_EQ(kBaseSeq + 3, feedback_packet->GetBaseSequence()); EXPECT_THAT(TimestampsMs(*feedback_packet), @@ -364,9 +426,12 @@ TEST_F(RemoteEstimatorProxyTest, RemovesTimestampsOutOfScope) { IncomingPacket(kBaseSeq, kBaseTimeMs - 1); IncomingPacket(kBaseSeq + 1, kTimeoutTimeMs - 1); - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce( - Invoke([kTimeoutTimeMs](rtcp::TransportFeedback* feedback_packet) { + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [&](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); EXPECT_THAT(SequenceNumbers(*feedback_packet), @@ -430,7 +495,7 @@ TEST_F(RemoteEstimatorProxyOnRequestTest, TimeUntilNextProcessIsHigh) { TEST_F(RemoteEstimatorProxyOnRequestTest, ProcessDoesNotSendFeedback) { proxy_.SetSendPeriodicFeedback(false); IncomingPacket(kBaseSeq, kBaseTimeMs); - EXPECT_CALL(router_, SendTransportFeedback(_)).Times(0); + EXPECT_CALL(router_, SendCombinedRtcpPacket).Times(0); Process(); } @@ -440,17 +505,21 @@ TEST_F(RemoteEstimatorProxyOnRequestTest, RequestSinglePacketFeedback) { IncomingPacket(kBaseSeq + 1, kBaseTimeMs + kMaxSmallDeltaMs); IncomingPacket(kBaseSeq + 2, kBaseTimeMs + 2 * kMaxSmallDeltaMs); - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq + 3, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq + 3, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), - ElementsAre(kBaseSeq + 3)); - EXPECT_THAT(TimestampsMs(*feedback_packet), - ElementsAre(kBaseTimeMs + 3 * kMaxSmallDeltaMs)); - return true; - })); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq + 3)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + 3 * kMaxSmallDeltaMs)); + return true; + })); constexpr FeedbackRequest kSinglePacketFeedbackRequest = { /*include_timestamps=*/true, /*sequence_count=*/1}; @@ -465,22 +534,26 @@ TEST_F(RemoteEstimatorProxyOnRequestTest, RequestLastFivePacketFeedback) { IncomingPacket(kBaseSeq + i, kBaseTimeMs + i * kMaxSmallDeltaMs); } - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq + 6, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq + 6, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), - ElementsAre(kBaseSeq + 6, kBaseSeq + 7, kBaseSeq + 8, - kBaseSeq + 9, kBaseSeq + 10)); - EXPECT_THAT(TimestampsMs(*feedback_packet), - ElementsAre(kBaseTimeMs + 6 * kMaxSmallDeltaMs, - kBaseTimeMs + 7 * kMaxSmallDeltaMs, - kBaseTimeMs + 8 * kMaxSmallDeltaMs, - kBaseTimeMs + 9 * kMaxSmallDeltaMs, - kBaseTimeMs + 10 * kMaxSmallDeltaMs)); - return true; - })); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq + 6, kBaseSeq + 7, kBaseSeq + 8, + kBaseSeq + 9, kBaseSeq + 10)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + 6 * kMaxSmallDeltaMs, + kBaseTimeMs + 7 * kMaxSmallDeltaMs, + kBaseTimeMs + 8 * kMaxSmallDeltaMs, + kBaseTimeMs + 9 * kMaxSmallDeltaMs, + kBaseTimeMs + 10 * kMaxSmallDeltaMs)); + return true; + })); constexpr FeedbackRequest kFivePacketsFeedbackRequest = { /*include_timestamps=*/true, /*sequence_count=*/5}; @@ -497,19 +570,23 @@ TEST_F(RemoteEstimatorProxyOnRequestTest, IncomingPacket(kBaseSeq + i, kBaseTimeMs + i * kMaxSmallDeltaMs); } - EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq + 6, feedback_packet->GetBaseSequence()); - EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce(Invoke( + [](std::vector> feedback_packets) { + rtcp::TransportFeedback* feedback_packet = + static_cast( + feedback_packets[0].get()); + EXPECT_EQ(kBaseSeq + 6, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(SequenceNumbers(*feedback_packet), - ElementsAre(kBaseSeq + 6, kBaseSeq + 8, kBaseSeq + 10)); - EXPECT_THAT(TimestampsMs(*feedback_packet), - ElementsAre(kBaseTimeMs + 6 * kMaxSmallDeltaMs, - kBaseTimeMs + 8 * kMaxSmallDeltaMs, - kBaseTimeMs + 10 * kMaxSmallDeltaMs)); - return true; - })); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq + 6, kBaseSeq + 8, kBaseSeq + 10)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + 6 * kMaxSmallDeltaMs, + kBaseTimeMs + 8 * kMaxSmallDeltaMs, + kBaseTimeMs + 10 * kMaxSmallDeltaMs)); + return true; + })); constexpr FeedbackRequest kFivePacketsFeedbackRequest = { /*include_timestamps=*/true, /*sequence_count=*/5}; @@ -578,11 +655,15 @@ TEST_F(RemoteEstimatorProxyTest, SendTransportFeedbackAndNetworkStateUpdate) { kBaseTimeMs, kDefaultPacketSize, CreateHeader(kBaseSeq, absl::nullopt, AbsoluteSendTime::MsTo24Bits(kBaseTimeMs - 1))); - - EXPECT_CALL(router_, SendTransportFeedback(_)); EXPECT_CALL(network_state_estimator_, GetCurrentEstimate()) .WillOnce(Return(NetworkStateEstimate())); - EXPECT_CALL(router_, SendNetworkStateEstimatePacket(_)); + EXPECT_CALL(router_, SendCombinedRtcpPacket) + .WillOnce( + [](std::vector> feedback_packets) { + EXPECT_THAT(feedback_packets, SizeIs(2)); + return true; + }); + Process(); }