-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
- 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.
if (clusters_.empty() || probing_state_ != ProbingState::kActive) { | ||
return absl::nullopt; | ||
} |
There was a problem hiding this comment.
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.
@@ -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; |
There was a problem hiding this comment.
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.
@@ -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()); |
There was a problem hiding this comment.
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.
Pull reviewers statsStats of the last 365 days for mediasoup:
|
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return missing here?
There was a problem hiding this comment.
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.
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; | ||
} |
There was a problem hiding this comment.
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.
MS_ERROR
instead ofMS_WARN_TAG(bwe)
to make these errors as visible as possible.absl::optional
somewhere in code diff.RTC_DCHECK
in libwebrtc does NOT abort so we must only show error logs and, depending on each case, do early return or not. If not, it means that code below keeps running with inconsistent data. I've tried to return earlier as much as possible but it's not always possible. In fact,RTC_CHECK
*does abort even in Release mode. However,RTC_DCHECK
just logs it.