From 89ae5551cc8a8ac34ed0b6719b356600dc9e1ebb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Tue, 24 Jan 2023 12:49:38 +0100 Subject: [PATCH 1/3] libwebrtc: replace MS_ASSERT with MS_ERROR - Fixes #986 - Let's use `MS_ERROR` instead of `MS_WARN_TAG(bwe)` to make these errors as visible as possible. - Sometimes use more modern libwebrtc code such as the usage of `absl::optional` somewhere in code diff. - Original `RTC_DCHECK` is supposed to not abort in libwebrtc Release mode, which means that code below keeps running with inconsistent data. I've tried to return eariler as much as possible but it's not always possible. --- .../goog_cc/delay_based_bwe.cc | 10 ++- .../goog_cc/probe_bitrate_estimator.cc | 19 ++++-- .../goog_cc/probe_controller.cc | 12 +++- .../modules/pacing/bitrate_prober.cc | 65 +++++++++++++------ .../libwebrtc/modules/pacing/bitrate_prober.h | 2 +- .../libwebrtc/modules/pacing/paced_sender.cc | 19 ++++-- .../remote_bitrate_estimator/inter_arrival.cc | 38 ++++++++--- .../overuse_estimator.cc | 5 +- .../remote_bitrate_estimator_abs_send_time.cc | 6 +- 9 files changed, 126 insertions(+), 50 deletions(-) diff --git a/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/delay_based_bwe.cc b/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/delay_based_bwe.cc index 6cdf95a48b..57f4afa146 100644 --- a/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/delay_based_bwe.cc +++ b/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/delay_based_bwe.cc @@ -252,8 +252,14 @@ bool DelayBasedBwe::LatestEstimate(std::vector* ssrcs, // thread. //RTC_DCHECK(ssrcs); //RTC_DCHECK(bitrate); - MS_ASSERT(ssrcs, "ssrcs must be != null"); - MS_ASSERT(bitrate, "bitrate must be != null"); + if (!ssrcs) { + MS_ERROR("ssrcs must be != null"); + return false; + } + if (!bitrate) { + MS_ERROR("bitrate must be != null"); + return false; + } if (!rate_control_.ValidEstimate()) return false; diff --git a/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/probe_bitrate_estimator.cc b/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/probe_bitrate_estimator.cc index 6022dbdd99..99b6d2dea0 100644 --- a/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/probe_bitrate_estimator.cc +++ b/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/probe_bitrate_estimator.cc @@ -64,7 +64,10 @@ absl::optional ProbeBitrateEstimator::HandleProbeAndEstimateBitrate( int cluster_id = packet_feedback.sent_packet.pacing_info.probe_cluster_id; //RTC_DCHECK_NE(cluster_id, PacedPacketInfo::kNotAProbe); - MS_ASSERT(cluster_id != PacedPacketInfo::kNotAProbe, "cluster_id == kNotAProbe"); + if (cluster_id == PacedPacketInfo::kNotAProbe) { + MS_ERROR("cluster_id == kNotAProbe"); + return absl::nullopt; + } EraseOldClusters(packet_feedback.receive_time); @@ -91,12 +94,14 @@ absl::optional ProbeBitrateEstimator::HandleProbeAndEstimateBitrate( // packet_feedback.sent_packet.pacing_info.probe_cluster_min_probes, 0); // RTC_DCHECK_GT(packet_feedback.sent_packet.pacing_info.probe_cluster_min_bytes, // 0); - MS_ASSERT( - packet_feedback.sent_packet.pacing_info.probe_cluster_min_probes > 0, - "probe_cluster_min_probes must be > 0"); - MS_ASSERT( - packet_feedback.sent_packet.pacing_info.probe_cluster_min_bytes > 0, - "probe_cluster_min_bytes must be > 0"); + if (packet_feedback.sent_packet.pacing_info.probe_cluster_min_probes <= 0) { + MS_ERROR("probe_cluster_min_probes must be > 0"); + return absl::nullopt; + } + if (packet_feedback.sent_packet.pacing_info.probe_cluster_min_bytes <= 0) { + MS_ERROR("probe_cluster_min_bytes must be > 0"); + return absl::nullopt; + } int min_probes = packet_feedback.sent_packet.pacing_info.probe_cluster_min_probes * diff --git a/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/probe_controller.cc b/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/probe_controller.cc index cc745e0c23..b5e8abae36 100644 --- a/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/probe_controller.cc +++ b/worker/deps/libwebrtc/libwebrtc/modules/congestion_controller/goog_cc/probe_controller.cc @@ -251,9 +251,15 @@ std::vector ProbeController::InitiateExponentialProbing( //RTC_DCHECK(network_available_); //RTC_DCHECK(state_ == State::kInit); //RTC_DCHECK_GT(start_bitrate_bps_, 0); - MS_ASSERT(network_available_, "network not available"); - MS_ASSERT(state_ == State::kInit, "state_ must be State::kInit"); - MS_ASSERT(start_bitrate_bps_ > 0, "start_bitrate_bps_ must be > 0"); + if (!network_available_) { + MS_ERROR("network not available"); + } + if (state_ != State::kInit) { + MS_ERROR("state_ must be State::kInit"); + } + if (start_bitrate_bps_ <= 0) { + MS_ERROR("start_bitrate_bps_ must be > 0"); + } // When probing at 1.8 Mbps ( 6x 300), this represents a threshold of // 1.2 Mbps to continue probing. diff --git a/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.cc b/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.cc index 1d3ce9806f..40dcf3d070 100644 --- a/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.cc +++ b/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.cc @@ -18,7 +18,7 @@ #include #include -// TODO: jeje +// TODO: Remove. #define TODO_PRINT_PROBING_STATE() \ switch (probing_state_) \ { \ @@ -80,7 +80,7 @@ BitrateProber::BitrateProber(const WebRtcKeyValueConfig& field_trials) config_(&field_trials) { SetEnabled(true); - // TODO: jeje + // TODO: Remove. TODO_PRINT_PROBING_STATE(); } @@ -95,7 +95,7 @@ void BitrateProber::SetEnabled(bool enable) { MS_DEBUG_TAG(bwe, "Bandwidth probing disabled"); } - // TODO: jeje + // TODO: Remove. TODO_PRINT_PROBING_STATE(); } @@ -114,7 +114,7 @@ void BitrateProber::OnIncomingPacket(size_t packet_size) { probing_state_ = ProbingState::kActive; } - // TODO: jeje + // TODO: Remove. TODO_PRINT_PROBING_STATE(); } @@ -123,8 +123,14 @@ void BitrateProber::CreateProbeCluster(int bitrate_bps, int cluster_id) { // RTC_DCHECK(probing_state_ != ProbingState::kDisabled); // RTC_DCHECK_GT(bitrate_bps, 0); - MS_ASSERT(probing_state_ != ProbingState::kDisabled, "probing disabled"); - MS_ASSERT(bitrate_bps > 0, "bitrate must be > 0"); + if (probing_state_ == ProbingState::kDisabled) { + MS_ERROR("probing disabled"); + return; + } + if (bitrate_bps <= 0) { + MS_ERROR("bitrate must be > 0"); + return; + } total_probe_count_++; while (!clusters_.empty() && @@ -141,7 +147,10 @@ void BitrateProber::CreateProbeCluster(int bitrate_bps, config_.min_probe_duration->ms() / 8000); // RTC_DCHECK_GE(cluster.pace_info.probe_cluster_min_bytes, 0); - MS_ASSERT(cluster.pace_info.probe_cluster_min_bytes >= 0, "cluster min bytes must be >= 0"); + if (cluster.pace_info.probe_cluster_min_bytes < 0) { + MS_ERROR("cluster min bytes must be >= 0"); + return; + } cluster.pace_info.send_bitrate_bps = bitrate_bps; cluster.pace_info.probe_cluster_id = cluster_id; @@ -165,12 +174,12 @@ void BitrateProber::CreateProbeCluster(int bitrate_bps, probing_state_ = ProbingState::kActive; } - // TODO: jeje + // TODO: Remove. TODO_PRINT_PROBING_STATE(); } int BitrateProber::TimeUntilNextProbe(int64_t now_ms) { - // TODO: jeje + // TODO: Remove. TODO_PRINT_PROBING_STATE(); // Probing is not active or probing is already complete. @@ -191,11 +200,12 @@ int BitrateProber::TimeUntilNextProbe(int64_t now_ms) { return std::max(time_until_probe_ms, 0); } -PacedPacketInfo BitrateProber::CurrentCluster() const { +absl::optional BitrateProber::CurrentCluster() const { // RTC_DCHECK(!clusters_.empty()); // RTC_DCHECK(probing_state_ == ProbingState::kActive); - MS_ASSERT(!clusters_.empty(), "clusters is empty"); - MS_ASSERT(probing_state_ == ProbingState::kActive, "probing not active"); + if (clusters_.empty() || probing_state_ != ProbingState::kActive) { + return absl::nullopt; + } return clusters_.front().pace_info; } @@ -205,7 +215,9 @@ PacedPacketInfo BitrateProber::CurrentCluster() const { // feasible. size_t BitrateProber::RecommendedMinProbeSize() const { // RTC_DCHECK(!clusters_.empty()); - MS_ASSERT(!clusters_.empty(), "clusters is empty"); + if (clusters_.empty()) { + return 0; + } return clusters_.front().pace_info.send_bitrate_bps * 2 * config_.min_probe_delta->ms() / (8 * 1000); @@ -214,14 +226,23 @@ size_t BitrateProber::RecommendedMinProbeSize() const { void BitrateProber::ProbeSent(int64_t now_ms, size_t bytes) { // RTC_DCHECK(probing_state_ == ProbingState::kActive); // RTC_DCHECK_GT(bytes, 0); - MS_ASSERT(probing_state_ == ProbingState::kActive, "probing not active"); - MS_ASSERT(bytes > 0, "bytes must be > 0"); + if (probing_state_ != ProbingState::kActive) { + MS_ERROR("probing not active"); + return; + } + if (bytes <= 0) { + MS_ERROR("bytes must be > 0"); + return; + } if (!clusters_.empty()) { ProbeCluster* cluster = &clusters_.front(); if (cluster->sent_probes == 0) { // RTC_DCHECK_EQ(cluster->time_started_ms, -1); - MS_ASSERT(cluster->time_started_ms == -1, "cluster started time must not be -1"); + if (cluster->time_started_ms != -1) { + MS_ERROR("cluster started time must be -1"); + return; + } cluster->time_started_ms = now_ms; } @@ -242,7 +263,7 @@ void BitrateProber::ProbeSent(int64_t now_ms, size_t bytes) { if (clusters_.empty()) probing_state_ = ProbingState::kSuspended; - // TODO: jeje + // TODO: Remove. TODO_PRINT_PROBING_STATE(); } } @@ -250,8 +271,14 @@ void BitrateProber::ProbeSent(int64_t now_ms, size_t bytes) { int64_t BitrateProber::GetNextProbeTime(const ProbeCluster& cluster) { // RTC_CHECK_GT(cluster.pace_info.send_bitrate_bps, 0); // RTC_CHECK_GE(cluster.time_started_ms, 0); - MS_ASSERT(cluster.pace_info.send_bitrate_bps > 0, "cluster.pace_info.send_bitrate_bps must be > 0"); - MS_ASSERT(cluster.time_started_ms > 0, "cluster.time_started_ms must be > 0"); + if (cluster.pace_info.send_bitrate_bps <= 0) { + MS_ERROR("cluster.pace_info.send_bitrate_bps must be > 0"); + return 0; + } + if (cluster.time_started_ms <= 0) { + MS_ERROR("cluster.time_started_ms must be > 0"); + return 0; + } // Compute the time delta from the cluster start to ensure probe bitrate stays // close to the target bitrate. Result is in milliseconds. diff --git a/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.h b/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.h index ed42c531d8..3810f82958 100644 --- a/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.h +++ b/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.h @@ -66,7 +66,7 @@ class BitrateProber { int TimeUntilNextProbe(int64_t now_ms); // Information about the current probing cluster. - PacedPacketInfo CurrentCluster() const; + absl::optional CurrentCluster() const; // Returns the minimum number of bytes that the prober recommends for // the next probe. diff --git a/worker/deps/libwebrtc/libwebrtc/modules/pacing/paced_sender.cc b/worker/deps/libwebrtc/libwebrtc/modules/pacing/paced_sender.cc index 1c638428b2..9c69384b19 100644 --- a/worker/deps/libwebrtc/libwebrtc/modules/pacing/paced_sender.cc +++ b/worker/deps/libwebrtc/libwebrtc/modules/pacing/paced_sender.cc @@ -99,7 +99,10 @@ bool PacedSender::Congested() const { void PacedSender::SetProbingEnabled(bool enabled) { // RTC_CHECK_EQ(0, packet_counter_); - MS_ASSERT(packet_counter_ == 0, "packet counter must be 0"); + if (packet_counter_ != 0) { + MS_ERROR("packet counter must be 0"); + return; + } prober_.SetEnabled(enabled); } @@ -107,7 +110,10 @@ void PacedSender::SetProbingEnabled(bool enabled) { void PacedSender::SetPacingRates(uint32_t pacing_rate_bps, uint32_t padding_rate_bps) { // RTC_DCHECK(pacing_rate_bps > 0); - MS_ASSERT(pacing_rate_bps > 0, "pacing rate must be > 0"); + if (pacing_rate_bps == 0) { + MS_ERROR("pacing rate must be > 0"); + return; + } pacing_bitrate_kbps_ = pacing_rate_bps / 1000; padding_budget_.set_target_rate_kbps(padding_rate_bps / 1000); @@ -121,7 +127,10 @@ void PacedSender::SetPacingRates(uint32_t pacing_rate_bps, void PacedSender::InsertPacket(size_t bytes) { // RTC_DCHECK(pacing_bitrate_kbps_ > 0) // << "SetPacingRate must be called before InsertPacket."; - MS_ASSERT(pacing_bitrate_kbps_ > 0, "SetPacingRates() must be called before InsertPacket()"); + if (pacing_bitrate_kbps_ <= 0) { + MS_ERROR("SetPacingRates() must be called before InsertPacket()"); + return; + } prober_.OnIncomingPacket(bytes); @@ -185,7 +194,7 @@ void PacedSender::Process() { PacedPacketInfo pacing_info; absl::optional recommended_probe_size; - pacing_info = prober_.CurrentCluster(); + pacing_info = prober_.CurrentCluster().value_or(PacedPacketInfo()); recommended_probe_size = prober_.RecommendedMinProbeSize(); size_t bytes_sent = 0; @@ -266,7 +275,7 @@ PacedPacketInfo PacedSender::GetPacingInfo() { PacedPacketInfo pacing_info; if (prober_.IsProbing()) { - pacing_info = prober_.CurrentCluster(); + pacing_info = prober_.CurrentCluster().value_or(PacedPacketInfo());; } return pacing_info; diff --git a/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/inter_arrival.cc b/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/inter_arrival.cc index 6bb751f373..ddb080948d 100644 --- a/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/inter_arrival.cc +++ b/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/inter_arrival.cc @@ -38,12 +38,28 @@ bool InterArrival::ComputeDeltas(uint32_t timestamp, uint32_t* timestamp_delta, // send_delta. int64_t* arrival_time_delta_ms, // recv_delta. int* packet_size_delta) { - MS_ASSERT(timestamp_delta != nullptr, "timestamp_delta is null"); - MS_ASSERT(arrival_time_delta_ms != nullptr, "arrival_time_delta_ms is null"); - MS_ASSERT(packet_size_delta != nullptr, "packet_size_delta is null"); + if (timestamp_delta == nullptr) { + MS_ERROR("timestamp_delta is null"); + + return false; + } + + if (arrival_time_delta_ms == nullptr) { + MS_ERROR("arrival_time_delta_ms is null"); + + return false; + } + + if (packet_size_delta == nullptr) { + MS_ERROR("packet_size_delta is null"); + + return false; + } + // Ignore packets with invalid arrival time. if (arrival_time_ms < 0) { MS_WARN_TAG(bwe, "invalid arrival time %" PRIi64, arrival_time_ms); + return false; } bool calculated_deltas = false; @@ -94,7 +110,11 @@ bool InterArrival::ComputeDeltas(uint32_t timestamp, num_consecutive_reordered_packets_ = 0; } - MS_ASSERT(*arrival_time_delta_ms >= 0, "arrival_time_delta_ms is < 0"); + if (*arrival_time_delta_ms < 0) { + MS_ERROR("arrival_time_delta_ms is < 0"); + + return false; + } *packet_size_delta = static_cast(current_timestamp_group_.size) - static_cast(prev_timestamp_group_.size); @@ -161,10 +181,12 @@ bool InterArrival::BelongsToBurst(int64_t arrival_time_ms, return false; } - MS_ASSERT( - current_timestamp_group_.complete_time_ms >= 0, - "current_timestamp_group_.complete_time_ms < 0 [current_timestamp_group_.complete_time_ms:%" PRIi64 "]", - current_timestamp_group_.complete_time_ms); + if (current_timestamp_group_.complete_time_ms < 0) { + MS_ERROR("current_timestamp_group_.complete_time_ms < 0 [current_timestamp_group_.complete_time_ms:%" PRIi64 "]", + current_timestamp_group_.complete_time_ms); + + return false; + } int64_t arrival_time_delta_ms = arrival_time_ms - current_timestamp_group_.complete_time_ms; diff --git a/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/overuse_estimator.cc b/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/overuse_estimator.cc index 81cc876380..c0639b2942 100644 --- a/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/overuse_estimator.cc +++ b/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/overuse_estimator.cc @@ -108,11 +108,8 @@ void OveruseEstimator::Update(int64_t t_delta, E_[0][0] + E_[1][1] >= 0 && E_[0][0] * E_[1][1] - E_[0][1] * E_[1][0] >= 0 && E_[0][0] >= 0; - MS_ASSERT(positive_semi_definite, "positive_semi_definite is not true"); - if (!positive_semi_definite) { - MS_ERROR("The over-use estimator's covariance matrix is no longer " - "semi-definite."); + MS_ERROR("the over-use estimator's covariance matrix is no longer semi-definite"); } slope_ = slope_ + K[0] * residual; diff --git a/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc index 4d87021cdd..30da04df75 100644 --- a/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc +++ b/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc @@ -236,7 +236,11 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo( uint32_t send_time_24bits, size_t payload_size, uint32_t ssrc) { - MS_ASSERT(send_time_24bits < (1ul << 24), "invalid sendTime24bits value"); + // RTC_CHECK(send_time_24bits < (1ul << 24)); + if (send_time_24bits >= (1ul << 24)) { + MS_ERROR("invalid sendTime24bits value"); + return; + } if (!uma_recorded_) { uma_recorded_ = true; From 91a9bd565a155784e76d77646e2d32a491097e12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Tue, 24 Jan 2023 12:55:14 +0100 Subject: [PATCH 2/3] cosmetic --- .../modules/remote_bitrate_estimator/inter_arrival.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/inter_arrival.cc b/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/inter_arrival.cc index ddb080948d..0e339dc8b0 100644 --- a/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/inter_arrival.cc +++ b/worker/deps/libwebrtc/libwebrtc/modules/remote_bitrate_estimator/inter_arrival.cc @@ -40,19 +40,14 @@ bool InterArrival::ComputeDeltas(uint32_t timestamp, int* packet_size_delta) { if (timestamp_delta == nullptr) { MS_ERROR("timestamp_delta is null"); - return false; } - if (arrival_time_delta_ms == nullptr) { MS_ERROR("arrival_time_delta_ms is null"); - return false; } - if (packet_size_delta == nullptr) { MS_ERROR("packet_size_delta is null"); - return false; } From e98cc76d470426a83d6b944230b7f242a8cf603b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?I=C3=B1aki=20Baz=20Castillo?= Date: Tue, 24 Jan 2023 17:31:02 +0100 Subject: [PATCH 3/3] revert some checks --- .../libwebrtc/modules/pacing/bitrate_prober.cc | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.cc b/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.cc index 40dcf3d070..208b0d3975 100644 --- a/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.cc +++ b/worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.cc @@ -271,14 +271,8 @@ void BitrateProber::ProbeSent(int64_t now_ms, size_t bytes) { int64_t BitrateProber::GetNextProbeTime(const ProbeCluster& cluster) { // RTC_CHECK_GT(cluster.pace_info.send_bitrate_bps, 0); // RTC_CHECK_GE(cluster.time_started_ms, 0); - if (cluster.pace_info.send_bitrate_bps <= 0) { - MS_ERROR("cluster.pace_info.send_bitrate_bps must be > 0"); - return 0; - } - if (cluster.time_started_ms <= 0) { - MS_ERROR("cluster.time_started_ms must be > 0"); - return 0; - } + MS_ASSERT(cluster.pace_info.send_bitrate_bps > 0, "cluster.pace_info.send_bitrate_bps must be > 0"); + MS_ASSERT(cluster.time_started_ms > 0, "cluster.time_started_ms must be > 0"); // Compute the time delta from the cluster start to ensure probe bitrate stays // close to the target bitrate. Result is in milliseconds.