Code consolidation in BaseChannel and derived classes.
This is a bit of refactoring to clear the way for some more upcoming
changes and fix little oddities here and there that are basically
artifacts of many small incremental changes throughout the years.
* Remove the CryptoOptions member variable and instead only keep around
the filter for rtp header extensions.
* Remove several member methods that only forwarded calls to
media_channel() and effectively reduced readability.
* Consolidated quite a bit of code related to UpdateRemoteStreams_w
and the copy/pasted code in the Video/Voice classes around calling it.
* UpdateRemoteStreams_w now returns an error when it's encountered.
Before, an error would still be returned in those cases but all
operations were unnecessarily performed.
Bug: none
Change-Id: I85a37b9e8f00584aa794abef11abfe89dec5d0a6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/244098
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35637}
diff --git a/pc/channel.cc b/pc/channel.cc
index a164e0a..56bd771 100644
--- a/pc/channel.cc
+++ b/pc/channel.cc
@@ -100,9 +100,13 @@
template <class Codec>
void RtpSendParametersFromMediaDescription(
const MediaContentDescriptionImpl<Codec>* desc,
- const RtpHeaderExtensions& extensions,
- bool is_stream_active,
+ webrtc::RtpExtension::Filter extensions_filter,
RtpSendParameters<Codec>* send_params) {
+ RtpHeaderExtensions extensions =
+ webrtc::RtpExtension::DeduplicateHeaderExtensions(
+ desc->rtp_header_extensions(), extensions_filter);
+ const bool is_stream_active =
+ webrtc::RtpTransceiverDirectionHasRecv(desc->direction());
RtpParametersFromMediaDescription(desc, extensions, is_stream_active,
send_params);
send_params->max_bandwidth_bps = desc->bandwidth();
@@ -122,7 +126,10 @@
signaling_thread_(signaling_thread),
alive_(PendingTaskSafetyFlag::Create()),
srtp_required_(srtp_required),
- crypto_options_(crypto_options),
+ extensions_filter_(
+ crypto_options.srtp.enable_encrypted_rtp_header_extensions
+ ? webrtc::RtpExtension::kPreferEncryptedExtension
+ : webrtc::RtpExtension::kDiscardEncryptedExtension),
media_channel_(std::move(media_channel)),
demuxer_criteria_(content_name),
ssrc_generator_(ssrc_generator) {
@@ -539,18 +546,6 @@
RTC_LOG(LS_INFO) << "Channel not writable (" << ToString() << ")";
}
-bool BaseChannel::AddRecvStream_w(const StreamParams& sp) {
- return media_channel()->AddRecvStream(sp);
-}
-
-bool BaseChannel::RemoveRecvStream_w(uint32_t ssrc) {
- return media_channel()->RemoveRecvStream(ssrc);
-}
-
-void BaseChannel::ResetUnsignaledRecvStream_w() {
- media_channel()->ResetUnsignaledRecvStream();
-}
-
bool BaseChannel::SetPayloadTypeDemuxingEnabled_w(bool enabled) {
if (enabled == payload_type_demuxing_enabled_) {
return true;
@@ -653,22 +648,35 @@
return ret;
}
-bool BaseChannel::UpdateRemoteStreams_w(
- const std::vector<StreamParams>& streams,
- SdpType type,
- std::string& error_desc) {
+bool BaseChannel::UpdateRemoteStreams_w(const MediaContentDescription* content,
+ SdpType type,
+ std::string& error_desc) {
+ RTC_LOG_THREAD_BLOCK_COUNT();
+ bool needs_re_registration = false;
+ if (!webrtc::RtpTransceiverDirectionHasSend(content->direction())) {
+ RTC_DLOG(LS_VERBOSE) << "UpdateRemoteStreams_w: remote side will not send "
+ "- disable payload type demuxing for "
+ << ToString();
+ if (ClearHandledPayloadTypes()) {
+ needs_re_registration = payload_type_demuxing_enabled_;
+ }
+ }
+
+ const std::vector<StreamParams>& streams = content->streams();
+ const bool new_has_unsignaled_ssrcs = HasStreamWithNoSsrcs(streams);
+ const bool old_has_unsignaled_ssrcs = HasStreamWithNoSsrcs(remote_streams_);
+
// Check for streams that have been removed.
- bool ret = true;
for (const StreamParams& old_stream : remote_streams_) {
// If we no longer have an unsignaled stream, we would like to remove
// the unsignaled stream params that are cached.
- if (!old_stream.has_ssrcs() && !HasStreamWithNoSsrcs(streams)) {
- ResetUnsignaledRecvStream_w();
+ if (!old_stream.has_ssrcs() && !new_has_unsignaled_ssrcs) {
+ media_channel()->ResetUnsignaledRecvStream();
RTC_LOG(LS_INFO) << "Reset unsignaled remote stream for " << ToString()
<< ".";
} else if (old_stream.has_ssrcs() &&
!GetStreamBySsrc(streams, old_stream.first_ssrc())) {
- if (RemoveRecvStream_w(old_stream.first_ssrc())) {
+ if (media_channel()->RemoveRecvStream(old_stream.first_ssrc())) {
RTC_LOG(LS_INFO) << "Remove remote ssrc: " << old_stream.first_ssrc()
<< " from " << ToString() << ".";
} else {
@@ -676,19 +684,20 @@
"Failed to remove remote stream with ssrc %u from m-section with "
"mid='%s'.",
old_stream.first_ssrc(), content_name().c_str());
- ret = false;
+ return false;
}
}
}
- demuxer_criteria_.ssrcs().clear();
+
// Check for new streams.
+ webrtc::flat_set<uint32_t> ssrcs;
for (const StreamParams& new_stream : streams) {
// We allow a StreamParams with an empty list of SSRCs, in which case the
// MediaChannel will cache the parameters and use them for any unsignaled
// stream received later.
- if ((!new_stream.has_ssrcs() && !HasStreamWithNoSsrcs(remote_streams_)) ||
+ if ((!new_stream.has_ssrcs() && !old_has_unsignaled_ssrcs) ||
!GetStreamBySsrc(remote_streams_, new_stream.first_ssrc())) {
- if (AddRecvStream_w(new_stream)) {
+ if (media_channel()->AddRecvStream(new_stream)) {
RTC_LOG(LS_INFO) << "Add remote ssrc: "
<< (new_stream.has_ssrcs()
? std::to_string(new_stream.first_ssrc())
@@ -701,28 +710,41 @@
? std::to_string(new_stream.first_ssrc()).c_str()
: "unsignaled",
ToString().c_str());
- ret = false;
+ return false;
}
}
// Update the receiving SSRCs.
- demuxer_criteria_.ssrcs().insert(new_stream.ssrcs.begin(),
- new_stream.ssrcs.end());
+ ssrcs.insert(new_stream.ssrcs.begin(), new_stream.ssrcs.end());
}
- // Re-register the sink to update the receiving ssrcs.
- if (!RegisterRtpDemuxerSink_w()) {
- RTC_LOG(LS_ERROR) << "Failed to set up demuxing for " << ToString();
- ret = false;
+
+ if (demuxer_criteria_.ssrcs() != ssrcs) {
+ demuxer_criteria_.ssrcs() = std::move(ssrcs);
+ needs_re_registration = true;
}
+
+ RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(0);
+
+ // Re-register the sink to update after changing the demuxer criteria.
+ if (needs_re_registration && !RegisterRtpDemuxerSink_w()) {
+ error_desc = StringFormat("Failed to set up audio demuxing for mid='%s'.",
+ content_name().c_str());
+ return false;
+ }
+
remote_streams_ = streams;
- return ret;
+
+ set_remote_content_direction(content->direction());
+ UpdateMediaSendRecvState_w();
+
+ RTC_DCHECK_BLOCK_COUNT_NO_MORE_THAN(1);
+
+ return true;
}
RtpHeaderExtensions BaseChannel::GetDeduplicatedRtpHeaderExtensions(
const RtpHeaderExtensions& extensions) {
- return webrtc::RtpExtension::DeduplicateHeaderExtensions(
- extensions, crypto_options_.srtp.enable_encrypted_rtp_header_extensions
- ? webrtc::RtpExtension::kPreferEncryptedExtension
- : webrtc::RtpExtension::kDiscardEncryptedExtension);
+ return webrtc::RtpExtension::DeduplicateHeaderExtensions(extensions,
+ extensions_filter_);
}
void BaseChannel::MaybeAddHandledPayloadType(int payload_type) {
@@ -735,9 +757,11 @@
payload_types_.insert(static_cast<uint8_t>(payload_type));
}
-void BaseChannel::ClearHandledPayloadTypes() {
+bool BaseChannel::ClearHandledPayloadTypes() {
+ const bool was_empty = demuxer_criteria_.payload_types().empty();
demuxer_criteria_.payload_types().clear();
payload_types_.clear();
+ return !was_empty;
}
void BaseChannel::SignalSentPacket_n(const rtc::SentPacket& sent_packet) {
@@ -818,7 +842,6 @@
}
// Need to re-register the sink to update the handled payload.
if (!RegisterRtpDemuxerSink_w()) {
- RTC_LOG(LS_ERROR) << "Failed to set up audio demuxing for " << ToString();
error_desc = StringFormat("Failed to set up audio demuxing for mid='%s'.",
content_name().c_str());
return false;
@@ -847,15 +870,9 @@
TRACE_EVENT0("webrtc", "VoiceChannel::SetRemoteContent_w");
RTC_LOG(LS_INFO) << "Setting remote voice description for " << ToString();
- const AudioContentDescription* audio = content->as_audio();
-
- RtpHeaderExtensions rtp_header_extensions =
- GetDeduplicatedRtpHeaderExtensions(audio->rtp_header_extensions());
-
AudioSendParameters send_params = last_send_params_;
- RtpSendParametersFromMediaDescription(
- audio, rtp_header_extensions,
- webrtc::RtpTransceiverDirectionHasRecv(audio->direction()), &send_params);
+ RtpSendParametersFromMediaDescription(content->as_audio(),
+ extensions_filter(), &send_params);
send_params.mid = content_name();
bool parameters_applied = media_channel()->SetSendParameters(send_params);
@@ -868,32 +885,7 @@
}
last_send_params_ = send_params;
- if (!webrtc::RtpTransceiverDirectionHasSend(content->direction())) {
- RTC_DLOG(LS_VERBOSE) << "SetRemoteContent_w: remote side will not send - "
- "disable payload type demuxing for "
- << ToString();
- ClearHandledPayloadTypes();
- if (!RegisterRtpDemuxerSink_w()) {
- RTC_LOG(LS_ERROR) << "Failed to update audio demuxing for " << ToString();
- return false;
- }
- }
-
- // TODO(pthatcher): Move remote streams into AudioRecvParameters,
- // and only give it to the media channel once we have a local
- // description too (without a local description, we won't be able to
- // recv them anyway).
- if (!UpdateRemoteStreams_w(audio->streams(), type, error_desc)) {
- error_desc = StringFormat(
- "Failed to set remote audio description streams for m-section with "
- "mid='%s'.",
- content_name().c_str());
- return false;
- }
-
- set_remote_content_direction(content->direction());
- UpdateMediaSendRecvState_w();
- return true;
+ return UpdateRemoteStreams_w(content, type, error_desc);
}
VideoChannel::VideoChannel(rtc::Thread* worker_thread,
@@ -924,11 +916,7 @@
// Send outgoing data if we're the active call, we have the remote content,
// and we have had some form of connectivity.
bool send = IsReadyToSendMedia_w();
- if (!media_channel()->SetSend(send)) {
- RTC_LOG(LS_ERROR) << "Failed to SetSend on video channel: " + ToString();
- // TODO(gangji): Report error back to server.
- }
-
+ media_channel()->SetSend(send);
RTC_LOG(LS_INFO) << "Changing video state, send=" << send << " for "
<< ToString();
}
@@ -992,7 +980,6 @@
}
// Need to re-register the sink to update the handled payload.
if (!RegisterRtpDemuxerSink_w()) {
- RTC_LOG(LS_ERROR) << "Failed to set up video demuxing for " << ToString();
error_desc = StringFormat("Failed to set up video demuxing for mid='%s'.",
content_name().c_str());
return false;
@@ -1033,17 +1020,11 @@
const VideoContentDescription* video = content->as_video();
- RtpHeaderExtensions rtp_header_extensions =
- GetDeduplicatedRtpHeaderExtensions(video->rtp_header_extensions());
-
VideoSendParameters send_params = last_send_params_;
- RtpSendParametersFromMediaDescription(
- video, rtp_header_extensions,
- webrtc::RtpTransceiverDirectionHasRecv(video->direction()), &send_params);
- if (video->conference_mode()) {
- send_params.conference_mode = true;
- }
+ RtpSendParametersFromMediaDescription(video, extensions_filter(),
+ &send_params);
send_params.mid = content_name();
+ send_params.conference_mode = video->conference_mode();
VideoRecvParameters recv_params = last_recv_params_;
@@ -1085,31 +1066,7 @@
last_recv_params_ = recv_params;
}
- if (!webrtc::RtpTransceiverDirectionHasSend(content->direction())) {
- RTC_DLOG(LS_VERBOSE) << "SetRemoteContent_w: remote side will not send - "
- "disable payload type demuxing for "
- << ToString();
- ClearHandledPayloadTypes();
- if (!RegisterRtpDemuxerSink_w()) {
- RTC_LOG(LS_ERROR) << "Failed to update video demuxing for " << ToString();
- return false;
- }
- }
-
- // TODO(pthatcher): Move remote streams into VideoRecvParameters,
- // and only give it to the media channel once we have a local
- // description too (without a local description, we won't be able to
- // recv them anyway).
- if (!UpdateRemoteStreams_w(video->streams(), type, error_desc)) {
- error_desc = StringFormat(
- "Failed to set remote video description streams for m-section with "
- "mid='%s'.",
- content_name().c_str());
- return false;
- }
- set_remote_content_direction(content->direction());
- UpdateMediaSendRecvState_w();
- return true;
+ return UpdateRemoteStreams_w(content, type, error_desc);
}
} // namespace cricket
diff --git a/pc/channel.h b/pc/channel.h
index e0bf3ba..a94749c 100644
--- a/pc/channel.h
+++ b/pc/channel.h
@@ -213,6 +213,10 @@
return remote_content_direction_;
}
+ webrtc::RtpExtension::Filter extensions_filter() const {
+ return extensions_filter_;
+ }
+
bool enabled() const RTC_RUN_ON(worker_thread()) { return enabled_; }
rtc::Thread* signaling_thread() const { return signaling_thread_; }
@@ -252,13 +256,8 @@
void ChannelWritable_n() RTC_RUN_ON(network_thread());
void ChannelNotWritable_n() RTC_RUN_ON(network_thread());
- bool AddRecvStream_w(const StreamParams& sp) RTC_RUN_ON(worker_thread());
- bool RemoveRecvStream_w(uint32_t ssrc) RTC_RUN_ON(worker_thread());
- void ResetUnsignaledRecvStream_w() RTC_RUN_ON(worker_thread());
bool SetPayloadTypeDemuxingEnabled_w(bool enabled)
RTC_RUN_ON(worker_thread());
- bool AddSendStream_w(const StreamParams& sp) RTC_RUN_ON(worker_thread());
- bool RemoveSendStream_w(uint32_t ssrc) RTC_RUN_ON(worker_thread());
// Should be called whenever the conditions for
// IsReadyToReceiveMedia/IsReadyToSendMedia are satisfied (or unsatisfied).
@@ -269,7 +268,7 @@
webrtc::SdpType type,
std::string& error_desc)
RTC_RUN_ON(worker_thread());
- bool UpdateRemoteStreams_w(const std::vector<StreamParams>& streams,
+ bool UpdateRemoteStreams_w(const MediaContentDescription* content,
webrtc::SdpType type,
std::string& error_desc)
RTC_RUN_ON(worker_thread());
@@ -292,7 +291,9 @@
// enabled.
void MaybeAddHandledPayloadType(int payload_type) RTC_RUN_ON(worker_thread());
- void ClearHandledPayloadTypes() RTC_RUN_ON(worker_thread());
+ // Returns true iff the demuxer payload type criteria was non-empty before
+ // clearing.
+ bool ClearHandledPayloadTypes() RTC_RUN_ON(worker_thread());
void UpdateRtpHeaderExtensionMap(
const RtpHeaderExtensions& header_extensions);
@@ -327,24 +328,9 @@
bool was_ever_writable_ RTC_GUARDED_BY(worker_thread()) = false;
const bool srtp_required_ = true;
- // TODO(tommi): This field shouldn't be necessary. It's a copy of
- // PeerConnection::GetCryptoOptions(), which is const state. It's also only
- // used to filter header extensions when calling
- // `rtp_transport_->UpdateRtpHeaderExtensionMap()` when the local/remote
- // content description is updated. Since the transport is actually owned
- // by the transport controller that also gets updated whenever the content
- // description changes, it seems we have two paths into the transports, along
- // with several thread hops via various classes (such as the Channel classes)
- // that only serve as additional layers and store duplicate state. The Jsep*
- // family of classes already apply session description updates on the network
- // thread every time it changes.
- // For the Channel classes, we should be able to get rid of:
- // * crypto_options (and fewer construction parameters)_
- // * UpdateRtpHeaderExtensionMap
- // * GetFilteredRtpHeaderExtensions
- // * Blocking thread hop to the network thread for every call to set
- // local/remote content is updated.
- const webrtc::CryptoOptions crypto_options_;
+ // Set to either kPreferEncryptedExtension or kDiscardEncryptedExtension
+ // based on the supplied CryptoOptions.
+ const webrtc::RtpExtension::Filter extensions_filter_;
// MediaChannel related members that should be accessed from the worker
// thread.
