From 38018ba67d5c62ff1171760f4d4899dacd3e7a7f Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 12 Jun 2017 16:29:45 +0200 Subject: [PATCH] Merge BitrateControllerImpl::RtcpBandwidthObserverImpl into BitrateControllerImpl This allows to protect ssrc_to_last_received_extended_high_seq_num_ member and make calls to OnReceivedRtcpReceiverReport thread-safe without introducing new critical section. Bug: webrtc:7735 Change-Id: Iee23bb780d07b0f906f1f8eeddde2b74cc0a2b89 Reviewed-on: https://chromium-review.googlesource.com/518130 Reviewed-by: Stefan Holmer Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#18540} --- .../bitrate_controller_impl.cc | 104 +++++++++--------- .../bitrate_controller_impl.h | 11 +- .../include/bitrate_controller.h | 6 +- 3 files changed, 60 insertions(+), 61 deletions(-) diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc index 2d76cc99d9..757ffb5f3b 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc +++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc @@ -12,7 +12,6 @@ #include "webrtc/modules/bitrate_controller/bitrate_controller_impl.h" #include -#include #include #include "webrtc/base/checks.h" @@ -28,64 +27,20 @@ class BitrateControllerImpl::RtcpBandwidthObserverImpl explicit RtcpBandwidthObserverImpl(BitrateControllerImpl* owner) : owner_(owner) { } - virtual ~RtcpBandwidthObserverImpl() { - } + ~RtcpBandwidthObserverImpl() override = default; // Received RTCP REMB or TMMBR. void OnReceivedEstimatedBitrate(uint32_t bitrate) override { - owner_->OnReceiverEstimatedBitrate(bitrate); + owner_->OnReceivedEstimatedBitrate(bitrate); } // Received RTCP receiver block. void OnReceivedRtcpReceiverReport(const ReportBlockList& report_blocks, int64_t rtt, int64_t now_ms) override { - if (report_blocks.empty()) - return; - - int fraction_lost_aggregate = 0; - int total_number_of_packets = 0; - - // Compute the a weighted average of the fraction loss from all report - // blocks. - for (const RTCPReportBlock& report_block : report_blocks) { - std::map::iterator seq_num_it = - ssrc_to_last_received_extended_high_seq_num_.find( - report_block.sourceSSRC); - - int number_of_packets = 0; - if (seq_num_it != ssrc_to_last_received_extended_high_seq_num_.end()) { - number_of_packets = - report_block.extendedHighSeqNum - seq_num_it->second; - } - - fraction_lost_aggregate += number_of_packets * report_block.fractionLost; - total_number_of_packets += number_of_packets; - - // Update last received for this SSRC. - ssrc_to_last_received_extended_high_seq_num_[report_block.sourceSSRC] = - report_block.extendedHighSeqNum; - } - if (total_number_of_packets < 0) { - LOG(LS_WARNING) << "Received report block where extended high sequence " - "number goes backwards, ignoring."; - return; - } - if (total_number_of_packets == 0) - fraction_lost_aggregate = 0; - else - fraction_lost_aggregate = (fraction_lost_aggregate + - total_number_of_packets / 2) / total_number_of_packets; - if (fraction_lost_aggregate > 255) - return; - - RTC_DCHECK_GE(total_number_of_packets, 0); - - owner_->OnReceivedRtcpReceiverReport(fraction_lost_aggregate, rtt, - total_number_of_packets, now_ms); + owner_->OnReceivedRtcpReceiverReport(report_blocks, rtt, now_ms); } private: - std::map ssrc_to_last_received_extended_high_seq_num_; - BitrateControllerImpl* owner_; + BitrateControllerImpl* const owner_; }; BitrateController* BitrateController::CreateBitrateController( @@ -175,7 +130,7 @@ void BitrateControllerImpl::SetReservedBitrate(uint32_t reserved_bitrate_bps) { } // This is called upon reception of REMB or TMMBR. -void BitrateControllerImpl::OnReceiverEstimatedBitrate(uint32_t bitrate) { +void BitrateControllerImpl::OnReceivedEstimatedBitrate(uint32_t bitrate) { { rtc::CritScope cs(&critsect_); bandwidth_estimation_.UpdateReceiverEstimate(clock_->TimeInMilliseconds(), @@ -220,14 +175,55 @@ void BitrateControllerImpl::Process() { } void BitrateControllerImpl::OnReceivedRtcpReceiverReport( - uint8_t fraction_loss, + const ReportBlockList& report_blocks, int64_t rtt, - int number_of_packets, int64_t now_ms) { + if (report_blocks.empty()) + return; + { rtc::CritScope cs(&critsect_); - bandwidth_estimation_.UpdateReceiverBlock(fraction_loss, rtt, - number_of_packets, now_ms); + int fraction_lost_aggregate = 0; + int total_number_of_packets = 0; + + // Compute the a weighted average of the fraction loss from all report + // blocks. + for (const RTCPReportBlock& report_block : report_blocks) { + std::map::iterator seq_num_it = + ssrc_to_last_received_extended_high_seq_num_.find( + report_block.sourceSSRC); + + int number_of_packets = 0; + if (seq_num_it != ssrc_to_last_received_extended_high_seq_num_.end()) { + number_of_packets = + report_block.extendedHighSeqNum - seq_num_it->second; + } + + fraction_lost_aggregate += number_of_packets * report_block.fractionLost; + total_number_of_packets += number_of_packets; + + // Update last received for this SSRC. + ssrc_to_last_received_extended_high_seq_num_[report_block.sourceSSRC] = + report_block.extendedHighSeqNum; + } + if (total_number_of_packets < 0) { + LOG(LS_WARNING) << "Received report block where extended high sequence " + "number goes backwards, ignoring."; + return; + } + if (total_number_of_packets == 0) + fraction_lost_aggregate = 0; + else + fraction_lost_aggregate = + (fraction_lost_aggregate + total_number_of_packets / 2) / + total_number_of_packets; + if (fraction_lost_aggregate > 255) + return; + + RTC_DCHECK_GE(total_number_of_packets, 0); + + bandwidth_estimation_.UpdateReceiverBlock(fraction_lost_aggregate, rtt, + total_number_of_packets, now_ms); } MaybeTriggerOnNetworkChanged(); } diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h b/webrtc/modules/bitrate_controller/bitrate_controller_impl.h index 1a53e5d374..ff271531b4 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.h +++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.h @@ -18,6 +18,7 @@ #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h" #include +#include #include #include @@ -70,12 +71,12 @@ class BitrateControllerImpl : public BitrateController { class RtcpBandwidthObserverImpl; // Called by BitrateObserver's direct from the RTCP module. - void OnReceiverEstimatedBitrate(uint32_t bitrate); + // Implements RtcpBandwidthObserver. + void OnReceivedEstimatedBitrate(uint32_t bitrate) override; - void OnReceivedRtcpReceiverReport(uint8_t fraction_loss, + void OnReceivedRtcpReceiverReport(const ReportBlockList& report_blocks, int64_t rtt, - int number_of_packets, - int64_t now_ms); + int64_t now_ms) override; // Deprecated void MaybeTriggerOnNetworkChanged(); @@ -91,6 +92,8 @@ class BitrateControllerImpl : public BitrateController { RtcEventLog* const event_log_; rtc::CriticalSection critsect_; + std::map ssrc_to_last_received_extended_high_seq_num_ + GUARDED_BY(critsect_); SendSideBandwidthEstimation bandwidth_estimation_ GUARDED_BY(critsect_); uint32_t reserved_bitrate_bps_ GUARDED_BY(critsect_); diff --git a/webrtc/modules/bitrate_controller/include/bitrate_controller.h b/webrtc/modules/bitrate_controller/include/bitrate_controller.h index 194c99be55..c6695e9311 100644 --- a/webrtc/modules/bitrate_controller/include/bitrate_controller.h +++ b/webrtc/modules/bitrate_controller/include/bitrate_controller.h @@ -15,8 +15,6 @@ #ifndef WEBRTC_MODULES_BITRATE_CONTROLLER_INCLUDE_BITRATE_CONTROLLER_H_ #define WEBRTC_MODULES_BITRATE_CONTROLLER_INCLUDE_BITRATE_CONTROLLER_H_ -#include - #include "webrtc/modules/congestion_controller/delay_based_bwe.h" #include "webrtc/modules/include/module.h" #include "webrtc/modules/pacing/paced_sender.h" @@ -42,7 +40,8 @@ class BitrateObserver { virtual ~BitrateObserver() {} }; -class BitrateController : public Module { +class BitrateController : public Module, + public RtcpBandwidthObserver { // This class collects feedback from all streams sent to a peer (via // RTCPBandwidthObservers). It does one aggregated send side bandwidth // estimation and divide the available bitrate between all its registered @@ -62,6 +61,7 @@ class BitrateController : public Module { virtual ~BitrateController() {} + // Creates RtcpBandwidthObserver caller responsible to delete. virtual RtcpBandwidthObserver* CreateRtcpBandwidthObserver() = 0; // Deprecated