[worker] Must replace MS_ASSERT() with a non aborting warn/error log

[ibc]

So it happens that we replaced all those RTC_DCHECK() in libwebrtc with MS_ASSERT(). However, when in Release mode, RTC_DCHECK() in libwebrtc doesn't not abort the process (see discussion in #357 (comment)).

In mediasoup v3 branch we have all these MS_ASSERT() in libwebrtc dep:

$ ack MS_ASSERT

libwebrtc/system_wrappers/source/field_trial.cc
113:    MS_ASSERT(

libwebrtc/call/rtp_transport_controller_send.cc
75:  MS_ASSERT(bitrate_config.start_bitrate_bps > 0, "start bitrate must be > 0");
90:  MS_ASSERT(observer_ != nullptr, "no observer");
139:    MS_ASSERT(observer_ == nullptr, "observer already set");
265:  MS_ASSERT(!controller_, "controller already set");
266:  MS_ASSERT(!control_handler_, "controller handler already set");
283:  MS_ASSERT(controller_, "controller not set");

libwebrtc/modules/pacing/bitrate_prober.cc
126:  MS_ASSERT(probing_state_ != ProbingState::kDisabled, "probing disabled");
127:  MS_ASSERT(bitrate_bps > 0, "bitrate must be > 0");
144:  MS_ASSERT(cluster.pace_info.probe_cluster_min_bytes >= 0, "cluster min bytes must be >= 0");
197:  MS_ASSERT(!clusters_.empty(), "clusters is empty");
198:  MS_ASSERT(probing_state_ == ProbingState::kActive, "probing not active");
208:  MS_ASSERT(!clusters_.empty(), "clusters is empty");
217:  MS_ASSERT(probing_state_ == ProbingState::kActive, "probing not active");
218:  MS_ASSERT(bytes > 0, "bytes must be > 0");
224:      MS_ASSERT(cluster->time_started_ms == -1, "cluster started time must not be -1");
253:  MS_ASSERT(cluster.pace_info.send_bitrate_bps > 0, "cluster.pace_info.send_bitrate_bps must be > 0");
254:  MS_ASSERT(cluster.time_started_ms > 0, "cluster.time_started_ms must be > 0");

libwebrtc/modules/pacing/paced_sender.cc
102:  MS_ASSERT(packet_counter_ == 0, "packet counter must be 0");
110:  MS_ASSERT(pacing_rate_bps > 0, "pacing rate must be > 0");
124:  MS_ASSERT(pacing_bitrate_kbps_ > 0, "SetPacingRates() must be called before InsertPacket()");

libwebrtc/modules/remote_bitrate_estimator/overuse_estimator.cc
111:  MS_ASSERT(positive_semi_definite, "positive_semi_definite is not true");

libwebrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc
239:  MS_ASSERT(send_time_24bits < (1ul << 24), "invalid sendTime24bits value");

libwebrtc/modules/remote_bitrate_estimator/inter_arrival.cc
41:  MS_ASSERT(timestamp_delta != nullptr, "timestamp_delta is null");
42:  MS_ASSERT(arrival_time_delta_ms != nullptr, "arrival_time_delta_ms is null");
43:  MS_ASSERT(packet_size_delta != nullptr, "packet_size_delta is null");
92:      MS_ASSERT(*arrival_time_delta_ms >= 0, "arrival_time_delta_ms is < 0");
159:  MS_ASSERT(

libwebrtc/modules/congestion_controller/goog_cc/probe_bitrate_estimator.cc
67:  MS_ASSERT(cluster_id != PacedPacketInfo::kNotAProbe, "cluster_id == kNotAProbe");
94:  MS_ASSERT(
97:  MS_ASSERT(

libwebrtc/modules/congestion_controller/goog_cc/probe_controller.cc
254:  MS_ASSERT(network_available_, "network not available");
255:  MS_ASSERT(state_ == State::kInit, "state_ must be State::kInit");
256:  MS_ASSERT(start_bitrate_bps_ > 0, "start_bitrate_bps_ must be > 0");

libwebrtc/modules/congestion_controller/goog_cc/delay_based_bwe.cc
255:  MS_ASSERT(ssrcs, "ssrcs must be != null");
256:  MS_ASSERT(bitrate, "bitrate must be != null");

So we should replace all these MS_ASSERT() with a MS_WARN_TAB(bwe) (or similar warn/error log) plus a return xxxx;.

NOTE: This should also be done in sarumjanuch:bwe_backport branch in ongoing PR #922

[ibc]

@jmillan @ggarber @vpalmisano I'd like to know how RTC_DCHECK() is supposed to behave in Release mode when a return value is required by the function/method, such as here:

Does it mean that, when in Release mode, it just logs something and code below keep running? with invalid values?

[sarumjanuch]

I would assume, that some ASSERT's must be left, as otherwise it will lead to something terrible, like this ones:
139: MS_ASSERT(observer_ == nullptr, "observer already set");
265: MS_ASSERT(!controller_, "controller already set");
266: MS_ASSERT(!control_handler_, "controller handler already set");
283: MS_ASSERT(controller_, "controller not set");

[ibc]

I would assume, that some ASSERT's must be left, as otherwise it will lead to something terrible, like this ones: 139: MS_ASSERT(observer_ == nullptr, "observer already set"); 265: MS_ASSERT(!controller_, "controller already set"); 266: MS_ASSERT(!control_handler_, "controller handler already set"); 283: MS_ASSERT(controller_, "controller not set");

Sure, those are not being modified in my ongoing PR.

[ibc]

PR done #988