Guard send_codec variable against receive channel access
Also fix one instance where access was done wrongly.
This makes certain that the split between MediaChannel types is respected
for this variable (prior to splitting the actual C++ types).
Bug: webrtc:13931
Change-Id: I8cf48ff5eddef35fda75533bb9c5075083c4ab16
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/305220
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40065}
diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc
index 8a3ead1..ee868de 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -797,7 +797,7 @@
if (negotiated_codecs_ != negotiated_codecs) {
if (negotiated_codecs.empty()) {
changed_params->send_codec = absl::nullopt;
- } else if (send_codec_ != negotiated_codecs.front()) {
+ } else if (send_codec() != negotiated_codecs.front()) {
changed_params->send_codec = negotiated_codecs.front();
}
changed_params->negotiated_codecs = std::move(negotiated_codecs);
@@ -903,7 +903,7 @@
new_codec_setting.codec.params[kv.first] = kv.second;
}
- if (send_codec_ == new_codec_setting) {
+ if (send_codec() == new_codec_setting) {
// Already using this codec, no switch required.
return;
}
@@ -931,7 +931,7 @@
negotiated_codecs_ = *changed_params.negotiated_codecs;
if (changed_params.send_codec)
- send_codec_ = changed_params.send_codec;
+ send_codec() = changed_params.send_codec;
if (changed_params.extmap_allow_mixed) {
SetExtmapAllowMixed(*changed_params.extmap_allow_mixed);
@@ -951,10 +951,10 @@
bitrate_config_.max_bitrate_bps = -1;
}
- if (send_codec_) {
+ if (send_codec()) {
// TODO(holmer): Changing the codec parameters shouldn't necessarily mean
// that we change the min/max of bandwidth estimation. Reevaluate this.
- bitrate_config_ = GetBitrateConfigForCodec(send_codec_->codec);
+ bitrate_config_ = GetBitrateConfigForCodec(send_codec()->codec);
if (!changed_params.send_codec) {
// If the codec isn't changing, set the start bitrate to -1 which means
// "unchanged" so that BWE isn't affected.
@@ -985,12 +985,12 @@
if (role() == MediaChannel::Role::kBoth) {
if (changed_params.send_codec || changed_params.rtcp_mode) {
// Update receive feedback parameters from new codec or RTCP mode.
- if (send_codec_) {
+ if (send_codec()) {
SetReceiverFeedbackParameters(
- HasLntf(send_codec_->codec), HasNack(send_codec_->codec),
+ HasLntf(send_codec()->codec), HasNack(send_codec()->codec),
send_params_.rtcp.reduced_size ? webrtc::RtcpMode::kReducedSize
: webrtc::RtcpMode::kCompound,
- send_codec_->rtx_time);
+ send_codec()->rtx_time);
}
}
} else {
@@ -1042,9 +1042,9 @@
// Need to add the common list of codecs to the send stream-specific
// RTP parameters.
for (const VideoCodec& codec : send_params_.codecs) {
- if (send_codec_ && send_codec_->codec.id == codec.id) {
+ if (send_codec() && send_codec()->codec.id == codec.id) {
// Put the current send codec to the front of the codecs list.
- RTC_DCHECK_EQ(codec.name, send_codec_->codec.name);
+ RTC_DCHECK_EQ(codec.name, send_codec()->codec.name);
rtp_params.codecs.insert(rtp_params.codecs.begin(),
codec.ToCodecParameters());
} else {
@@ -1286,11 +1286,11 @@
bool WebRtcVideoChannel::GetSendCodec(VideoCodec* codec) {
RTC_DCHECK_RUN_ON(&thread_checker_);
- if (!send_codec_) {
+ if (!send_codec()) {
RTC_LOG(LS_VERBOSE) << "GetSendCodec: No send codec set.";
return false;
}
- *codec = send_codec_->codec;
+ *codec = send_codec()->codec;
return true;
}
@@ -1312,7 +1312,7 @@
RTC_DCHECK_RUN_ON(&thread_checker_);
TRACE_EVENT0("webrtc", "WebRtcVideoChannel::SetSend");
RTC_LOG(LS_VERBOSE) << "SetSend: " << (send ? "true" : "false");
- if (send && !send_codec_) {
+ if (send && !send_codec()) {
RTC_DLOG(LS_ERROR) << "SetSend(true) called before setting codec.";
return false;
}
@@ -1408,7 +1408,7 @@
WebRtcVideoSendStream* stream = new WebRtcVideoSendStream(
call_, sp, std::move(config), default_send_options_,
video_config_.enable_cpu_adaptation, bitrate_config_.max_bitrate_bps,
- send_codec_, send_rtp_extensions_, send_params_);
+ send_codec(), send_rtp_extensions_, send_params_);
uint32_t ssrc = sp.first_ssrc();
RTC_DCHECK(ssrc != 0);
@@ -1558,16 +1558,17 @@
config->rtp.rtcp_mode = send_params_.rtcp.reduced_size
? webrtc::RtcpMode::kReducedSize
: webrtc::RtcpMode::kCompound;
+ // rtx-time (RFC 4588) is a declarative attribute similar to rtcp-rsize and
+ // determined by the sender / send codec.
+ if (send_codec() && send_codec()->rtx_time) {
+ config->rtp.nack.rtp_history_ms = *send_codec()->rtx_time;
+ }
} else {
- // The mode is determined by a call to the configuration function.
+ // The mode and rtx time is determined by a call to the configuration
+ // function.
config->rtp.rtcp_mode = rtp_config_.rtcp_mode;
}
- // rtx-time (RFC 4588) is a declarative attribute similar to rtcp-rsize and
- // determined by the sender / send codec.
- if (send_codec_ && send_codec_->rtx_time) {
- config->rtp.nack.rtp_history_ms = *send_codec_->rtx_time;
- }
sp.GetFidSsrc(ssrc, &config->rtp.rtx_ssrc);
// TODO(brandtr): Generalize when we add support for multistream protection.
@@ -1765,7 +1766,7 @@
void WebRtcVideoChannel::FillSendCodecStats(
VideoMediaSendInfo* video_media_info) {
RTC_DCHECK_RUN_ON(&thread_checker_);
- if (!send_codec_) {
+ if (!send_codec()) {
return;
}
// Note: since RTP stats don't account for RTX and FEC separately (see
@@ -1773,7 +1774,7 @@
// we can omit the codec information for those here and only insert the
// primary codec that is being used to send here.
video_media_info->send_codecs.insert(std::make_pair(
- send_codec_->codec.id, send_codec_->codec.ToCodecParameters()));
+ send_codec()->codec.id, send_codec()->codec.ToCodecParameters()));
}
void WebRtcVideoChannel::FillReceiveCodecStats(
diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h
index 382b139..d3c088c 100644
--- a/media/engine/webrtc_video_engine.h
+++ b/media/engine/webrtc_video_engine.h
@@ -247,24 +247,24 @@
bool SendCodecHasLntf() const override {
RTC_DCHECK_RUN_ON(&thread_checker_);
- if (!send_codec_) {
+ if (!send_codec()) {
return false;
}
- return HasLntf(send_codec_->codec);
+ return HasLntf(send_codec()->codec);
}
bool SendCodecHasNack() const override {
RTC_DCHECK_RUN_ON(&thread_checker_);
- if (!send_codec_) {
+ if (!send_codec()) {
return false;
}
- return HasNack(send_codec_->codec);
+ return HasNack(send_codec()->codec);
}
absl::optional<int> SendCodecRtxTime() const override {
RTC_DCHECK_RUN_ON(&thread_checker_);
- if (!send_codec_) {
+ if (!send_codec()) {
return absl::nullopt;
}
- return send_codec_->rtx_time;
+ return send_codec()->rtx_time;
}
void SetReceiverFeedbackParameters(bool lntf_enabled,
bool nack_enabled,
@@ -616,6 +616,21 @@
void FillReceiveCodecStats(VideoMediaReceiveInfo* video_media_info)
RTC_EXCLUSIVE_LOCKS_REQUIRED(thread_checker_);
+ // Accessor function for send_codec_. Introduced in order to ensure
+ // that a receive channel does not touch the send codec directly.
+ // Can go away once these are different classes.
+ // TODO(bugs.webrtc.org/13931): Remove this function
+ absl::optional<VideoCodecSettings>& send_codec() {
+ RTC_DCHECK(role() == MediaChannel::Role::kSend ||
+ role() == MediaChannel::Role::kBoth);
+ return send_codec_;
+ }
+ const absl::optional<VideoCodecSettings>& send_codec() const {
+ RTC_DCHECK(role() == MediaChannel::Role::kSend ||
+ role() == MediaChannel::Role::kBoth);
+ return send_codec_;
+ }
+
webrtc::TaskQueueBase* const worker_thread_;
webrtc::ScopedTaskSafety task_safety_;
RTC_NO_UNIQUE_ADDRESS webrtc::SequenceChecker network_thread_checker_{
