From 1de4b62955cb20f7287b3291efdec5c4f3ee91d7 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Wed, 13 Dec 2017 13:35:10 +0100 Subject: [PATCH] Change RtpRtcp::SetRemb signature to match RtcpTransceiver::SetRemb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit in particular change bitrate type to int64_t to follow style guide. With an extra interface it will allow to add both RtpRtcp module and RtcpTransceiver as feedback sender to PacketRouter Bug: webrtc:8239 Change-Id: I9ea265686d7cd2d709f0b42e8a983ebe1790a6ba Reviewed-on: https://webrtc-review.googlesource.com/32302 Commit-Queue: Danil Chapovalov Reviewed-by: Philip Eliasson Reviewed-by: Erik Språng Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/master@{#21250} --- modules/pacing/packet_router.cc | 22 +++++++++++-------- modules/pacing/packet_router.h | 11 +++++----- modules/pacing/packet_router_unittest.cc | 6 ++--- modules/rtp_rtcp/include/rtp_rtcp.h | 3 +-- modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 3 +-- modules/rtp_rtcp/source/rtcp_sender.cc | 7 +++--- modules/rtp_rtcp/source/rtcp_sender.h | 4 ++-- modules/rtp_rtcp/source/rtcp_transceiver.cc | 5 +++-- modules/rtp_rtcp/source/rtcp_transceiver.h | 2 +- .../rtp_rtcp/source/rtcp_transceiver_impl.cc | 2 +- .../rtp_rtcp/source/rtcp_transceiver_impl.h | 2 +- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 6 ++--- modules/rtp_rtcp/source/rtp_rtcp_impl.h | 3 +-- 13 files changed, 39 insertions(+), 37 deletions(-) diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index 8b3ffc19a6..ba1359a556 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -153,7 +153,10 @@ uint16_t PacketRouter::AllocateSequenceNumber() { void PacketRouter::OnReceiveBitrateChanged(const std::vector& ssrcs, uint32_t bitrate_bps) { // % threshold for if we should send a new REMB asap. - const uint32_t kSendThresholdPercent = 97; + const int64_t kSendThresholdPercent = 97; + // TODO(danilchap): Remove receive_bitrate_bps variable and the cast + // when OnReceiveBitrateChanged takes bitrate as int64_t. + int64_t receive_bitrate_bps = static_cast(bitrate_bps); int64_t now_ms = rtc::TimeMillis(); { @@ -162,8 +165,8 @@ void PacketRouter::OnReceiveBitrateChanged(const std::vector& ssrcs, // If we already have an estimate, check if the new total estimate is below // kSendThresholdPercent of the previous estimate. if (last_send_bitrate_bps_ > 0) { - uint32_t new_remb_bitrate_bps = - last_send_bitrate_bps_ - bitrate_bps_ + bitrate_bps; + int64_t new_remb_bitrate_bps = + last_send_bitrate_bps_ - bitrate_bps_ + receive_bitrate_bps; if (new_remb_bitrate_bps < kSendThresholdPercent * last_send_bitrate_bps_ / 100) { @@ -172,7 +175,7 @@ void PacketRouter::OnReceiveBitrateChanged(const std::vector& ssrcs, last_remb_time_ms_ = now_ms - kRembSendIntervalMs; } } - bitrate_bps_ = bitrate_bps; + bitrate_bps_ = receive_bitrate_bps; if (now_ms - last_remb_time_ms_ < kRembSendIntervalMs) { return; @@ -180,14 +183,15 @@ void PacketRouter::OnReceiveBitrateChanged(const std::vector& ssrcs, // NOTE: Updated if we intend to send the data; we might not have // a module to actually send it. last_remb_time_ms_ = now_ms; - last_send_bitrate_bps_ = bitrate_bps; + last_send_bitrate_bps_ = receive_bitrate_bps; // Cap the value to send in remb with configured value. - bitrate_bps = std::min(bitrate_bps, max_bitrate_bps_); + receive_bitrate_bps = std::min(receive_bitrate_bps, max_bitrate_bps_); } - SendRemb(bitrate_bps, ssrcs); + SendRemb(receive_bitrate_bps, ssrcs); } -void PacketRouter::SetMaxDesiredReceiveBitrate(uint32_t bitrate_bps) { +void PacketRouter::SetMaxDesiredReceiveBitrate(int64_t bitrate_bps) { + RTC_DCHECK_GE(bitrate_bps, 0); { rtc::CritScope lock(&remb_crit_); max_bitrate_bps_ = bitrate_bps; @@ -201,7 +205,7 @@ void PacketRouter::SetMaxDesiredReceiveBitrate(uint32_t bitrate_bps) { SendRemb(bitrate_bps, /*ssrcs=*/{}); } -bool PacketRouter::SendRemb(uint32_t bitrate_bps, +bool PacketRouter::SendRemb(int64_t bitrate_bps, const std::vector& ssrcs) { rtc::CritScope lock(&modules_crit_); diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 929bd51a92..77d46d7717 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -72,11 +72,10 @@ class PacketRouter : public PacedSender::PacketSender, // Ensures remote party notified of the receive bitrate limit no larger than // |bitrate_bps|. - void SetMaxDesiredReceiveBitrate(uint32_t bitrate_bps); + void SetMaxDesiredReceiveBitrate(int64_t bitrate_bps); // Send REMB feedback. - virtual bool SendRemb(uint32_t bitrate_bps, - const std::vector& ssrcs); + bool SendRemb(int64_t bitrate_bps, const std::vector& ssrcs); // Send transport feedback packet to send-side. bool SendTransportFeedback(rtcp::TransportFeedback* packet) override; @@ -99,10 +98,10 @@ class PacketRouter : public PacedSender::PacketSender, rtc::CriticalSection remb_crit_; // The last time a REMB was sent. int64_t last_remb_time_ms_ RTC_GUARDED_BY(remb_crit_); - uint32_t last_send_bitrate_bps_ RTC_GUARDED_BY(remb_crit_); + int64_t last_send_bitrate_bps_ RTC_GUARDED_BY(remb_crit_); // The last bitrate update. - uint32_t bitrate_bps_ RTC_GUARDED_BY(remb_crit_); - uint32_t max_bitrate_bps_ RTC_GUARDED_BY(remb_crit_); + int64_t bitrate_bps_ RTC_GUARDED_BY(remb_crit_); + int64_t max_bitrate_bps_ RTC_GUARDED_BY(remb_crit_); // Candidates for the REMB module can be RTP sender/receiver modules, with // the sender modules taking precedence. diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc index 54c8f12c05..9acd080dd5 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -515,7 +515,7 @@ TEST(PacketRouterRembTest, SetMaxDesiredReceiveBitrateLimitsSetRemb) { constexpr bool remb_candidate = true; packet_router.AddSendRtpModule(&remb_sender, remb_candidate); - const uint32_t cap_bitrate = 100000; + const int64_t cap_bitrate = 100000; EXPECT_CALL(remb_sender, SetRemb(Le(cap_bitrate), _)).Times(AtLeast(1)); EXPECT_CALL(remb_sender, SetRemb(Gt(cap_bitrate), _)).Times(0); @@ -538,8 +538,8 @@ TEST(PacketRouterRembTest, constexpr bool remb_candidate = true; packet_router.AddSendRtpModule(&remb_sender, remb_candidate); - const uint32_t measured_bitrate_bps = 150000; - const uint32_t cap_bitrate_bps = measured_bitrate_bps - 5000; + const int64_t measured_bitrate_bps = 150000; + const int64_t cap_bitrate_bps = measured_bitrate_bps - 5000; const std::vector ssrcs = {1234}; EXPECT_CALL(remb_sender, SetRemb(measured_bitrate_bps, _)); packet_router.OnReceiveBitrateChanged(ssrcs, measured_bitrate_bps); diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index ef5f62a77f..6e146229e4 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -341,8 +341,7 @@ class RtpRtcp : public Module { // (REMB) Receiver Estimated Max Bitrate. // Schedules sending REMB on next and following sender/receiver reports. - virtual void SetRemb(uint32_t bitrate_bps, - const std::vector& ssrcs) = 0; + virtual void SetRemb(int64_t bitrate_bps, std::vector ssrcs) = 0; // Stops sending REMB on next and following sender/receiver reports. virtual void UnsetRemb() = 0; diff --git a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 10461ebdd6..05eea81cd5 100644 --- a/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -157,8 +157,7 @@ class MockRtpRtcp : public RtpRtcp { MOCK_METHOD1(SetRTCPVoIPMetrics, int32_t(const RTCPVoIPMetric* voip_metric)); MOCK_METHOD1(SetRtcpXrRrtrStatus, void(bool enable)); MOCK_CONST_METHOD0(RtcpXrRrtrStatus, bool()); - MOCK_METHOD2(SetRemb, - void(uint32_t bitrate, const std::vector& ssrcs)); + MOCK_METHOD2(SetRemb, void(int64_t bitrate, std::vector ssrcs)); MOCK_METHOD0(UnsetRemb, void()); MOCK_CONST_METHOD0(TMMBR, bool()); MOCK_METHOD1(SetTMMBRStatus, void(bool enable)); diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index c75768d850..bd1a5d4b62 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -230,10 +230,11 @@ int32_t RTCPSender::SetSendingStatus(const FeedbackState& feedback_state, return 0; } -void RTCPSender::SetRemb(uint32_t bitrate, const std::vector& ssrcs) { +void RTCPSender::SetRemb(int64_t bitrate_bps, std::vector ssrcs) { + RTC_CHECK_GE(bitrate_bps, 0); rtc::CritScope lock(&critical_section_rtcp_sender_); - remb_bitrate_ = bitrate; - remb_ssrcs_ = ssrcs; + remb_bitrate_ = bitrate_bps; + remb_ssrcs_ = std::move(ssrcs); SetFlag(kRtcpRemb, /*is_volatile=*/false); // Send a REMB immediately if we have a new REMB. The frequency of REMBs is diff --git a/modules/rtp_rtcp/source/rtcp_sender.h b/modules/rtp_rtcp/source/rtcp_sender.h index 3896559219..e56f66ce62 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -120,7 +120,7 @@ class RTCPSender { int32_t nackSize = 0, const uint16_t* nackList = 0); - void SetRemb(uint32_t bitrate, const std::vector& ssrcs); + void SetRemb(int64_t bitrate_bps, std::vector ssrcs); void UnsetRemb(); @@ -221,7 +221,7 @@ class RTCPSender { uint8_t sequence_number_fir_ RTC_GUARDED_BY(critical_section_rtcp_sender_); // REMB - uint32_t remb_bitrate_ RTC_GUARDED_BY(critical_section_rtcp_sender_); + int64_t remb_bitrate_ RTC_GUARDED_BY(critical_section_rtcp_sender_); std::vector remb_ssrcs_ RTC_GUARDED_BY(critical_section_rtcp_sender_); diff --git a/modules/rtp_rtcp/source/rtcp_transceiver.cc b/modules/rtp_rtcp/source/rtcp_transceiver.cc index a5dd8dc941..52b2a683ce 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver.cc @@ -67,7 +67,8 @@ void RtcpTransceiver::SendCompoundPacket() { }); } -void RtcpTransceiver::SetRemb(int bitrate_bps, std::vector ssrcs) { +void RtcpTransceiver::SetRemb(int64_t bitrate_bps, + std::vector ssrcs) { // TODO(danilchap): Replace with lambda with move capture when available. struct SetRembClosure { void operator()() { @@ -76,7 +77,7 @@ void RtcpTransceiver::SetRemb(int bitrate_bps, std::vector ssrcs) { } rtc::WeakPtr ptr; - int bitrate_bps; + int64_t bitrate_bps; std::vector ssrcs; }; task_queue_->PostTask(SetRembClosure{ptr_, bitrate_bps, std::move(ssrcs)}); diff --git a/modules/rtp_rtcp/source/rtcp_transceiver.h b/modules/rtp_rtcp/source/rtcp_transceiver.h index 5388d18776..09840e1721 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver.h +++ b/modules/rtp_rtcp/source/rtcp_transceiver.h @@ -40,7 +40,7 @@ class RtcpTransceiver { // (REMB) Receiver Estimated Max Bitrate. // Includes REMB in following compound packets. - void SetRemb(int bitrate_bps, std::vector ssrcs); + void SetRemb(int64_t bitrate_bps, std::vector ssrcs); // Stops sending REMB in following compound packets. void UnsetRemb(); diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc index 6a2caab06c..e8de54aa7d 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.cc @@ -134,7 +134,7 @@ void RtcpTransceiverImpl::SendCompoundPacket() { ReschedulePeriodicCompoundPackets(); } -void RtcpTransceiverImpl::SetRemb(int bitrate_bps, +void RtcpTransceiverImpl::SetRemb(int64_t bitrate_bps, std::vector ssrcs) { RTC_DCHECK_GE(bitrate_bps, 0); remb_.emplace(); diff --git a/modules/rtp_rtcp/source/rtcp_transceiver_impl.h b/modules/rtp_rtcp/source/rtcp_transceiver_impl.h index c6bb90bdd0..83e6087780 100644 --- a/modules/rtp_rtcp/source/rtcp_transceiver_impl.h +++ b/modules/rtp_rtcp/source/rtcp_transceiver_impl.h @@ -47,7 +47,7 @@ class RtcpTransceiverImpl { void SendCompoundPacket(); - void SetRemb(int bitrate_bps, std::vector ssrcs); + void SetRemb(int64_t bitrate_bps, std::vector ssrcs); void UnsetRemb(); void SendNack(uint32_t ssrc, std::vector sequence_numbers); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index d649fd67f3..ae658a481a 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -629,9 +629,9 @@ int32_t ModuleRtpRtcpImpl::RemoteRTCPStat( } // (REMB) Receiver Estimated Max Bitrate. -void ModuleRtpRtcpImpl::SetRemb(uint32_t bitrate_bps, - const std::vector& ssrcs) { - rtcp_sender_.SetRemb(bitrate_bps, ssrcs); +void ModuleRtpRtcpImpl::SetRemb(int64_t bitrate_bps, + std::vector ssrcs) { + rtcp_sender_.SetRemb(bitrate_bps, std::move(ssrcs)); } void ModuleRtpRtcpImpl::UnsetRemb() { diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index ebe942b916..0bb9edee5b 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -192,8 +192,7 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { std::vector* receive_blocks) const override; // (REMB) Receiver Estimated Max Bitrate. - void SetRemb(uint32_t bitrate_bps, - const std::vector& ssrcs) override; + void SetRemb(int64_t bitrate_bps, std::vector ssrcs) override; void UnsetRemb() override; // (TMMBR) Temporary Max Media Bit Rate.