From bbc926d62c3336d7e372c1bd146734db60f515e0 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 21 Mar 2023 20:46:39 +0100 Subject: [PATCH] Cleanup construction parameters for remote bitrate estimator helpers Values of these parameters are always the same and thus can be hardcoded Bug: None Change-Id: Ie19a1c6305d503ad2c92af503006a72b7981e178 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298622 Auto-Submit: Danil Chapovalov Commit-Queue: Per Kjellander Reviewed-by: Per Kjellander Cr-Commit-Position: refs/heads/main@{#39637} --- .../remote_bitrate_estimator/inter_arrival.cc | 7 +--- .../remote_bitrate_estimator/inter_arrival.h | 4 +- .../inter_arrival_unittest.cc | 9 ++-- .../overuse_detector_unittest.cc | 20 ++++----- .../overuse_estimator.cc | 25 +++-------- .../overuse_estimator.h | 41 ++++++------------- .../remote_bitrate_estimator_abs_send_time.cc | 5 +-- .../remote_bitrate_estimator_single_stream.cc | 11 ++--- 8 files changed, 37 insertions(+), 85 deletions(-) diff --git a/modules/remote_bitrate_estimator/inter_arrival.cc b/modules/remote_bitrate_estimator/inter_arrival.cc index 41c092bf91..06ff580b92 100644 --- a/modules/remote_bitrate_estimator/inter_arrival.cc +++ b/modules/remote_bitrate_estimator/inter_arrival.cc @@ -19,13 +19,11 @@ static const int kBurstDeltaThresholdMs = 5; static const int kMaxBurstDurationMs = 100; InterArrival::InterArrival(uint32_t timestamp_group_length_ticks, - double timestamp_to_ms_coeff, - bool enable_burst_grouping) + double timestamp_to_ms_coeff) : kTimestampGroupLengthTicks(timestamp_group_length_ticks), current_timestamp_group_(), prev_timestamp_group_(), timestamp_to_ms_coeff_(timestamp_to_ms_coeff), - burst_grouping_(enable_burst_grouping), num_consecutive_reordered_packets_(0) {} bool InterArrival::ComputeDeltas(uint32_t timestamp, @@ -136,9 +134,6 @@ bool InterArrival::NewTimestampGroup(int64_t arrival_time_ms, bool InterArrival::BelongsToBurst(int64_t arrival_time_ms, uint32_t timestamp) const { - if (!burst_grouping_) { - return false; - } RTC_DCHECK_GE(current_timestamp_group_.complete_time_ms, 0); int64_t arrival_time_delta_ms = arrival_time_ms - current_timestamp_group_.complete_time_ms; diff --git a/modules/remote_bitrate_estimator/inter_arrival.h b/modules/remote_bitrate_estimator/inter_arrival.h index d31a8b63c3..c395af2f87 100644 --- a/modules/remote_bitrate_estimator/inter_arrival.h +++ b/modules/remote_bitrate_estimator/inter_arrival.h @@ -30,8 +30,7 @@ class InterArrival { // most timestamp_group_length_ticks older than the first timestamp in that // group. InterArrival(uint32_t timestamp_group_length_ticks, - double timestamp_to_ms_coeff, - bool enable_burst_grouping); + double timestamp_to_ms_coeff); InterArrival() = delete; InterArrival(const InterArrival&) = delete; @@ -87,7 +86,6 @@ class InterArrival { TimestampGroup current_timestamp_group_; TimestampGroup prev_timestamp_group_; double timestamp_to_ms_coeff_; - bool burst_grouping_; int num_consecutive_reordered_packets_; }; } // namespace webrtc diff --git a/modules/remote_bitrate_estimator/inter_arrival_unittest.cc b/modules/remote_bitrate_estimator/inter_arrival_unittest.cc index 72a772ed21..581963e96d 100644 --- a/modules/remote_bitrate_estimator/inter_arrival_unittest.cc +++ b/modules/remote_bitrate_estimator/inter_arrival_unittest.cc @@ -33,12 +33,11 @@ const double kAstToMs = 1000.0 / static_cast(1 << kInterArrivalShift); class InterArrivalTest : public ::testing::Test { protected: virtual void SetUp() { - inter_arrival_.reset( - new InterArrival(kTimestampGroupLengthUs / 1000, 1.0, true)); + inter_arrival_.reset(new InterArrival(kTimestampGroupLengthUs / 1000, 1.0)); inter_arrival_rtp_.reset(new InterArrival( - MakeRtpTimestamp(kTimestampGroupLengthUs), kRtpTimestampToMs, true)); - inter_arrival_ast_.reset(new InterArrival( - MakeAbsSendTime(kTimestampGroupLengthUs), kAstToMs, true)); + MakeRtpTimestamp(kTimestampGroupLengthUs), kRtpTimestampToMs)); + inter_arrival_ast_.reset( + new InterArrival(MakeAbsSendTime(kTimestampGroupLengthUs), kAstToMs)); } // Test that neither inter_arrival instance complete the timestamp group from diff --git a/modules/remote_bitrate_estimator/overuse_detector_unittest.cc b/modules/remote_bitrate_estimator/overuse_detector_unittest.cc index e91d4f0d22..82786521d7 100644 --- a/modules/remote_bitrate_estimator/overuse_detector_unittest.cc +++ b/modules/remote_bitrate_estimator/overuse_detector_unittest.cc @@ -35,8 +35,7 @@ class OveruseDetectorTest : public ::testing::Test { receive_time_ms_(0), rtp_timestamp_(10 * 90), overuse_detector_(), - overuse_estimator_(new OveruseEstimator(options_)), - inter_arrival_(new InterArrival(5 * 90, kRtpTimestampToMs, true)), + inter_arrival_(5 * 90, kRtpTimestampToMs), random_(123456789) {} protected: @@ -99,15 +98,15 @@ class OveruseDetectorTest : public ::testing::Test { uint32_t timestamp_delta; int64_t time_delta; int size_delta; - if (inter_arrival_->ComputeDeltas( + if (inter_arrival_.ComputeDeltas( rtp_timestamp, receive_time_ms, receive_time_ms, packet_size, ×tamp_delta, &time_delta, &size_delta)) { double timestamp_delta_ms = timestamp_delta / 90.0; - overuse_estimator_->Update(time_delta, timestamp_delta_ms, size_delta, - overuse_detector_->State(), receive_time_ms); - overuse_detector_->Detect( - overuse_estimator_->offset(), timestamp_delta_ms, - overuse_estimator_->num_of_deltas(), receive_time_ms); + overuse_estimator_.Update(time_delta, timestamp_delta_ms, size_delta, + overuse_detector_->State(), receive_time_ms); + overuse_detector_->Detect(overuse_estimator_.offset(), timestamp_delta_ms, + overuse_estimator_.num_of_deltas(), + receive_time_ms); } } @@ -115,10 +114,9 @@ class OveruseDetectorTest : public ::testing::Test { int64_t now_ms_; int64_t receive_time_ms_; uint32_t rtp_timestamp_; - OverUseDetectorOptions options_; std::unique_ptr overuse_detector_; - std::unique_ptr overuse_estimator_; - std::unique_ptr inter_arrival_; + OveruseEstimator overuse_estimator_; + InterArrival inter_arrival_; Random random_; }; diff --git a/modules/remote_bitrate_estimator/overuse_estimator.cc b/modules/remote_bitrate_estimator/overuse_estimator.cc index 684143fcce..5ca21d9629 100644 --- a/modules/remote_bitrate_estimator/overuse_estimator.cc +++ b/modules/remote_bitrate_estimator/overuse_estimator.cc @@ -20,29 +20,14 @@ #include "rtc_base/logging.h" namespace webrtc { +namespace { -enum { kMinFramePeriodHistoryLength = 60 }; -enum { kDeltaCounterMax = 1000 }; +constexpr int kMinFramePeriodHistoryLength = 60; +constexpr int kDeltaCounterMax = 1000; -OveruseEstimator::OveruseEstimator(const OverUseDetectorOptions& options) - : options_(options), - num_of_deltas_(0), - slope_(options_.initial_slope), - offset_(options_.initial_offset), - prev_offset_(options_.initial_offset), - E_(), - process_noise_(), - avg_noise_(options_.initial_avg_noise), - var_noise_(options_.initial_var_noise), - ts_delta_hist_() { - memcpy(E_, options_.initial_e, sizeof(E_)); - memcpy(process_noise_, options_.initial_process_noise, - sizeof(process_noise_)); -} +} // namespace -OveruseEstimator::~OveruseEstimator() { - ts_delta_hist_.clear(); -} +OveruseEstimator::OveruseEstimator() = default; void OveruseEstimator::Update(int64_t t_delta, double ts_delta, diff --git a/modules/remote_bitrate_estimator/overuse_estimator.h b/modules/remote_bitrate_estimator/overuse_estimator.h index c021f00da7..d75ae04b58 100644 --- a/modules/remote_bitrate_estimator/overuse_estimator.h +++ b/modules/remote_bitrate_estimator/overuse_estimator.h @@ -18,29 +18,15 @@ namespace webrtc { -// Bandwidth over-use detector options. These are used to drive -// experimentation with bandwidth estimation parameters. -// TODO(terelius): This is only used in overuse_estimator.cc, and only in the -// default constructed state. Can we move the relevant variables into that -// class and delete this? -struct OverUseDetectorOptions { - OverUseDetectorOptions() = default; - double initial_slope = 8.0 / 512.0; - double initial_offset = 0; - double initial_e[2][2] = {{100.0, 0.0}, {0.0, 1e-1}}; - double initial_process_noise[2] = {1e-13, 1e-3}; - double initial_avg_noise = 0.0; - double initial_var_noise = 50.0; -}; - class OveruseEstimator { public: - explicit OveruseEstimator(const OverUseDetectorOptions& options); - ~OveruseEstimator(); + OveruseEstimator(); OveruseEstimator(const OveruseEstimator&) = delete; OveruseEstimator& operator=(const OveruseEstimator&) = delete; + ~OveruseEstimator() = default; + // Update the estimator with a new sample. The deltas should represent deltas // between timestamp groups as defined by the InterArrival class. // `current_hypothesis` should be the hypothesis of the over-use detector at @@ -59,23 +45,20 @@ class OveruseEstimator { // Returns the number of deltas which the current over-use estimator state is // based on. - unsigned int num_of_deltas() const { return num_of_deltas_; } + int num_of_deltas() const { return num_of_deltas_; } private: double UpdateMinFramePeriod(double ts_delta); void UpdateNoiseEstimate(double residual, double ts_delta, bool stable_state); - // Must be first member variable. Cannot be const because we need to be - // copyable. - OverUseDetectorOptions options_; - uint16_t num_of_deltas_; - double slope_; - double offset_; - double prev_offset_; - double E_[2][2]; - double process_noise_[2]; - double avg_noise_; - double var_noise_; + int num_of_deltas_ = 0; + double slope_ = 8.0 / 512.0; + double offset_ = 0; + double prev_offset_ = 0; + double E_[2][2] = {{100.0, 0.0}, {0.0, 1e-1}}; + double process_noise_[2] = {1e-13, 1e-3}; + double avg_noise_ = 0.0; + double var_noise_ = 50.0; std::deque ts_delta_hist_; }; } // namespace webrtc diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc index 17a8315bd4..a07348c766 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc @@ -362,9 +362,8 @@ void RemoteBitrateEstimatorAbsSendTime::TimeoutStreams(Timestamp now) { if (ssrcs_.empty()) { // We can't update the estimate if we don't have any active streams. inter_arrival_ = std::make_unique( - (kTimestampGroupLengthMs << kInterArrivalShift) / 1000, kTimestampToMs, - true); - estimator_ = std::make_unique(OverUseDetectorOptions()); + (kTimestampGroupLengthMs << kInterArrivalShift) / 1000, kTimestampToMs); + estimator_ = std::make_unique(); // We deliberately don't reset the first_packet_time_ms_ here for now since // we only probe for bandwidth in the beginning of a call right now. } diff --git a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc index 049734969a..bc06d4e4b5 100644 --- a/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc +++ b/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc @@ -42,15 +42,11 @@ static const double kTimestampToMs = 1.0 / 90.0; struct RemoteBitrateEstimatorSingleStream::Detector { explicit Detector(int64_t last_packet_time_ms, - const OverUseDetectorOptions& options, - bool enable_burst_grouping, const FieldTrialsView* key_value_config) : last_packet_time_ms(last_packet_time_ms), - inter_arrival(90 * kTimestampGroupLengthMs, - kTimestampToMs, - enable_burst_grouping), - estimator(options), + inter_arrival(90 * kTimestampGroupLengthMs, kTimestampToMs), detector(key_value_config) {} + int64_t last_packet_time_ms; InterArrival inter_arrival; OveruseEstimator estimator; @@ -104,8 +100,7 @@ void RemoteBitrateEstimatorSingleStream::IncomingPacket( // group. std::pair insert_result = overuse_detectors_.insert( - std::make_pair(ssrc, new Detector(now_ms, OverUseDetectorOptions(), - true, &field_trials_))); + std::make_pair(ssrc, new Detector(now_ms, &field_trials_))); it = insert_result.first; } Detector* estimator = it->second;