Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libwebrtc: replace MS_ASSERT with MS_ERROR #988

Merged
merged 3 commits into from
Jan 26, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,14 @@ bool DelayBasedBwe::LatestEstimate(std::vector<uint32_t>* 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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ absl::optional<DataRate> 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);

Expand All @@ -91,12 +94,14 @@ absl::optional<DataRate> 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 *
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,9 +251,15 @@ std::vector<ProbeClusterConfig> 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.
Expand Down
65 changes: 46 additions & 19 deletions worker/deps/libwebrtc/libwebrtc/modules/pacing/bitrate_prober.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#include <absl/memory/memory.h>
#include <algorithm>

// TODO: jeje
// TODO: Remove.
#define TODO_PRINT_PROBING_STATE() \
switch (probing_state_) \
{ \
Expand Down Expand Up @@ -80,7 +80,7 @@ BitrateProber::BitrateProber(const WebRtcKeyValueConfig& field_trials)
config_(&field_trials) {
SetEnabled(true);

// TODO: jeje
// TODO: Remove.
TODO_PRINT_PROBING_STATE();
}

Expand All @@ -95,7 +95,7 @@ void BitrateProber::SetEnabled(bool enable) {
MS_DEBUG_TAG(bwe, "Bandwidth probing disabled");
}

// TODO: jeje
// TODO: Remove.
TODO_PRINT_PROBING_STATE();
}

Expand All @@ -114,7 +114,7 @@ void BitrateProber::OnIncomingPacket(size_t packet_size) {
probing_state_ = ProbingState::kActive;
}

// TODO: jeje
// TODO: Remove.
TODO_PRINT_PROBING_STATE();
}

Expand All @@ -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() &&
Expand All @@ -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;
Expand All @@ -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.
Expand All @@ -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<PacedPacketInfo> 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;
}
Comment on lines +206 to +208
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how modern libwebrtc code looks.


return clusters_.front().pace_info;
}
Expand All @@ -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;
ibc marked this conversation as resolved.
Show resolved Hide resolved
}

return clusters_.front().pace_info.send_bitrate_bps * 2 *
config_.min_probe_delta->ms() / (8 * 1000);
Expand All @@ -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;
}
Expand All @@ -242,16 +263,22 @@ 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();
}
}

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;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmillan I'm gonna revert these changes because those were RTC_CHECK so assertion is needed.


// Compute the time delta from the cluster start to ensure probe bitrate stays
// close to the target bitrate. Result is in milliseconds.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class BitrateProber {
int TimeUntilNextProbe(int64_t now_ms);

// Information about the current probing cluster.
PacedPacketInfo CurrentCluster() const;
absl::optional<PacedPacketInfo> CurrentCluster() const;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from modern libwebrtc version.


// Returns the minimum number of bytes that the prober recommends for
// the next probe.
Expand Down
19 changes: 14 additions & 5 deletions worker/deps/libwebrtc/libwebrtc/modules/pacing/paced_sender.cc
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,21 @@ 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);
}

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);
Expand All @@ -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);

Expand Down Expand Up @@ -185,7 +194,7 @@ void PacedSender::Process() {
PacedPacketInfo pacing_info;
absl::optional<size_t> recommended_probe_size;

pacing_info = prober_.CurrentCluster();
pacing_info = prober_.CurrentCluster().value_or(PacedPacketInfo());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope this is ok. Used in libwebrtc modern version somewhere.

recommended_probe_size = prober_.RecommendedMinProbeSize();

size_t bytes_sent = 0;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,23 @@ 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;
Expand Down Expand Up @@ -94,7 +105,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<int>(current_timestamp_group_.size) -
static_cast<int>(prev_timestamp_group_.size);
Expand Down Expand Up @@ -161,10 +176,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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return missing here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no such a return in libwebrtc.

}

slope_ = slope_ + K[0] * residual;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down