Removes locking in TransportFeedbackProxy.
The lock in TransportFeedbackProxy could cause a dead-lock if audio is
included in transport feedback messages, and necessitated a revert:
https://webrtc-review.googlesource.com/c/src/+/178100
This CL removes that lock and in fact the entire TransportFeedbackProxy
class, and instead sets the observer at construction time.
We therefore don't need to guard the observer pointer anymore.
For further context, see also internal bug b/153893626
Bug: webrtc:10809
Change-Id: I79b08d8d0e587a59736b383c3596a26836c33d2e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/178207
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Sam Zackrisson <saza@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31583}
diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc
index 42705aa..301e42d 100644
--- a/audio/audio_send_stream.cc
+++ b/audio/audio_send_stream.cc
@@ -116,18 +116,20 @@
bitrate_allocator,
event_log,
suspended_rtp_state,
- voe::CreateChannelSend(clock,
- task_queue_factory,
- module_process_thread,
- config.send_transport,
- rtcp_rtt_stats,
- event_log,
- config.frame_encryptor,
- config.crypto_options,
- config.rtp.extmap_allow_mixed,
- config.rtcp_report_interval_ms,
- config.rtp.ssrc,
- config.frame_transformer)) {}
+ voe::CreateChannelSend(
+ clock,
+ task_queue_factory,
+ module_process_thread,
+ config.send_transport,
+ rtcp_rtt_stats,
+ event_log,
+ config.frame_encryptor,
+ config.crypto_options,
+ config.rtp.extmap_allow_mixed,
+ config.rtcp_report_interval_ms,
+ config.rtp.ssrc,
+ config.frame_transformer,
+ rtp_transport->transport_feedback_observer())) {}
AudioSendStream::AudioSendStream(
Clock* clock,
@@ -506,10 +508,7 @@
}
void AudioSendStream::DeliverRtcp(const uint8_t* packet, size_t length) {
- // TODO(solenberg): Tests call this function on a network thread, libjingle
- // calls on the worker thread. We should move towards always using a network
- // thread. Then this check can be enabled.
- // RTC_DCHECK(!worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
channel_send_->ReceivedRTCPPacket(packet, length);
worker_queue_->PostTask([&]() {
// Poll if overhead has changed, which it can do if ack triggers us to stop
diff --git a/audio/channel_send.cc b/audio/channel_send.cc
index 16d1da6..2e7ca72c 100644
--- a/audio/channel_send.cc
+++ b/audio/channel_send.cc
@@ -55,7 +55,6 @@
constexpr int64_t kMinRetransmissionWindowMs = 30;
class RtpPacketSenderProxy;
-class TransportFeedbackProxy;
class TransportSequenceNumberProxy;
class VoERtcpObserver;
@@ -78,7 +77,8 @@
bool extmap_allow_mixed,
int rtcp_report_interval_ms,
uint32_t ssrc,
- rtc::scoped_refptr<FrameTransformerInterface> frame_transformer);
+ rtc::scoped_refptr<FrameTransformerInterface> frame_transformer,
+ TransportFeedbackObserver* feedback_observer);
~ChannelSend() override;
@@ -213,7 +213,7 @@
PacketRouter* packet_router_ RTC_GUARDED_BY(&worker_thread_checker_) =
nullptr;
- const std::unique_ptr<TransportFeedbackProxy> feedback_observer_proxy_;
+ TransportFeedbackObserver* const feedback_observer_;
const std::unique_ptr<RtpPacketSenderProxy> rtp_packet_pacer_proxy_;
const std::unique_ptr<RateLimiter> retransmission_rate_limiter_;
@@ -244,43 +244,6 @@
const int kTelephoneEventAttenuationdB = 10;
-class TransportFeedbackProxy : public TransportFeedbackObserver {
- public:
- TransportFeedbackProxy() : feedback_observer_(nullptr) {
- pacer_thread_.Detach();
- network_thread_.Detach();
- }
-
- void SetTransportFeedbackObserver(
- TransportFeedbackObserver* feedback_observer) {
- RTC_DCHECK(thread_checker_.IsCurrent());
- rtc::CritScope lock(&crit_);
- feedback_observer_ = feedback_observer;
- }
-
- // Implements TransportFeedbackObserver.
- void OnAddPacket(const RtpPacketSendInfo& packet_info) override {
- RTC_DCHECK(pacer_thread_.IsCurrent());
- rtc::CritScope lock(&crit_);
- if (feedback_observer_)
- feedback_observer_->OnAddPacket(packet_info);
- }
-
- void OnTransportFeedback(const rtcp::TransportFeedback& feedback) override {
- RTC_DCHECK(network_thread_.IsCurrent());
- rtc::CritScope lock(&crit_);
- if (feedback_observer_)
- feedback_observer_->OnTransportFeedback(feedback);
- }
-
- private:
- rtc::CriticalSection crit_;
- rtc::ThreadChecker thread_checker_;
- rtc::ThreadChecker pacer_thread_;
- rtc::ThreadChecker network_thread_;
- TransportFeedbackObserver* feedback_observer_ RTC_GUARDED_BY(&crit_);
-};
-
class RtpPacketSenderProxy : public RtpPacketSender {
public:
RtpPacketSenderProxy() : rtp_packet_pacer_(nullptr) {}
@@ -489,7 +452,8 @@
bool extmap_allow_mixed,
int rtcp_report_interval_ms,
uint32_t ssrc,
- rtc::scoped_refptr<FrameTransformerInterface> frame_transformer)
+ rtc::scoped_refptr<FrameTransformerInterface> frame_transformer,
+ TransportFeedbackObserver* feedback_observer)
: event_log_(rtc_event_log),
_timeStamp(0), // This is just an offset, RTP module will add it's own
// random offset
@@ -498,7 +462,7 @@
previous_frame_muted_(false),
_includeAudioLevelIndication(false),
rtcp_observer_(new VoERtcpObserver(this)),
- feedback_observer_proxy_(new TransportFeedbackProxy()),
+ feedback_observer_(feedback_observer),
rtp_packet_pacer_proxy_(new RtpPacketSenderProxy()),
retransmission_rate_limiter_(
new RateLimiter(clock, kMaxRetransmissionWindowMs)),
@@ -514,7 +478,7 @@
RtpRtcpInterface::Configuration configuration;
configuration.bandwidth_callback = rtcp_observer_.get();
- configuration.transport_feedback_callback = feedback_observer_proxy_.get();
+ configuration.transport_feedback_callback = feedback_observer_;
configuration.clock = (clock ? clock : Clock::GetRealTimeClock());
configuration.audio = true;
configuration.outgoing_transport = rtp_transport;
@@ -663,6 +627,8 @@
}
void ChannelSend::ReceivedRTCPPacket(const uint8_t* data, size_t length) {
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
+
// Deliver RTCP packet to RTP/RTCP module for parsing
rtp_rtcp_->IncomingRtcpPacket(data, length);
@@ -743,17 +709,12 @@
RtcpBandwidthObserver* bandwidth_observer) {
RTC_DCHECK_RUN_ON(&worker_thread_checker_);
RtpPacketSender* rtp_packet_pacer = transport->packet_sender();
- TransportFeedbackObserver* transport_feedback_observer =
- transport->transport_feedback_observer();
PacketRouter* packet_router = transport->packet_router();
RTC_DCHECK(rtp_packet_pacer);
- RTC_DCHECK(transport_feedback_observer);
RTC_DCHECK(packet_router);
RTC_DCHECK(!packet_router_);
rtcp_observer_->SetBandwidthObserver(bandwidth_observer);
- feedback_observer_proxy_->SetTransportFeedbackObserver(
- transport_feedback_observer);
rtp_packet_pacer_proxy_->SetPacketPacer(rtp_packet_pacer);
rtp_rtcp_->SetStorePacketsStatus(true, 600);
constexpr bool remb_candidate = false;
@@ -766,7 +727,6 @@
RTC_DCHECK(packet_router_);
rtp_rtcp_->SetStorePacketsStatus(false, 600);
rtcp_observer_->SetBandwidthObserver(nullptr);
- feedback_observer_proxy_->SetTransportFeedbackObserver(nullptr);
packet_router_->RemoveSendRtpModule(rtp_rtcp_.get());
packet_router_ = nullptr;
rtp_packet_pacer_proxy_->SetPacketPacer(nullptr);
@@ -985,12 +945,13 @@
bool extmap_allow_mixed,
int rtcp_report_interval_ms,
uint32_t ssrc,
- rtc::scoped_refptr<FrameTransformerInterface> frame_transformer) {
+ rtc::scoped_refptr<FrameTransformerInterface> frame_transformer,
+ TransportFeedbackObserver* feedback_observer) {
return std::make_unique<ChannelSend>(
clock, task_queue_factory, module_process_thread, rtp_transport,
rtcp_rtt_stats, rtc_event_log, frame_encryptor, crypto_options,
extmap_allow_mixed, rtcp_report_interval_ms, ssrc,
- std::move(frame_transformer));
+ std::move(frame_transformer), feedback_observer);
}
} // namespace voe
diff --git a/audio/channel_send.h b/audio/channel_send.h
index 56fea97..2e23ef5 100644
--- a/audio/channel_send.h
+++ b/audio/channel_send.h
@@ -135,7 +135,8 @@
bool extmap_allow_mixed,
int rtcp_report_interval_ms,
uint32_t ssrc,
- rtc::scoped_refptr<FrameTransformerInterface> frame_transformer);
+ rtc::scoped_refptr<FrameTransformerInterface> frame_transformer,
+ TransportFeedbackObserver* feedback_observer);
} // namespace voe
} // namespace webrtc
