Remove sigslot from PeerConnectionInternal and RTCStatsCollector.
It turns out that there were several sigslot instances across data
channel, pc and stats classes that in practice only served as means
to update two counters in RTCStatsCollector. There's already a
notification path that's suitable.
This also fixes a case where the PC instance sat in the middle
of notifications from datachannels to the datachannel controller.
Bug: webrtc:11943
Change-Id: Ic60b76021584019f82085f6651230fe2fe82d465
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/295781
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39456}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 41b2e7e..0a2c9ba 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -1063,7 +1063,6 @@
"../rtc_base:threading",
"../rtc_base:timeutils",
"../rtc_base/synchronization:mutex",
- "../rtc_base/third_party/sigslot:sigslot",
]
absl_deps = [
"//third_party/abseil-cpp/absl/functional:bind_front",
diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
index 36f11fe..947efe5 100644
--- a/pc/data_channel_controller.cc
+++ b/pc/data_channel_controller.cc
@@ -103,6 +103,16 @@
return (data_channel_transport() && data_channel_transport_ready_to_send_);
}
+void DataChannelController::OnChannelStateChanged(
+ SctpDataChannel* channel,
+ DataChannelInterface::DataState state) {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ if (state == DataChannelInterface::DataState::kClosed)
+ OnSctpDataChannelClosed(channel);
+
+ pc_->OnSctpDataChannelStateChanged(channel, state);
+}
+
void DataChannelController::OnDataReceived(
int channel_id,
DataMessageType type,
@@ -298,9 +308,6 @@
return nullptr;
}
sctp_data_channels_.push_back(channel);
- channel->SignalClosed.connect(
- pc_, &PeerConnectionInternal::OnSctpDataChannelClosed);
- SignalSctpDataChannelCreated_(channel.get());
return channel;
}
@@ -381,8 +388,8 @@
const rtc::CopyOnWriteBuffer& payload,
cricket::SendDataResult* result) {
// TODO(bugs.webrtc.org/11547): Expect method to be called on the network
- // thread instead. Remove the BlockingCall() below and move assocated state to
- // the network thread.
+ // thread instead. Remove the BlockingCall() below and move associated state
+ // to the network thread.
RTC_DCHECK_RUN_ON(signaling_thread());
RTC_DCHECK(data_channel_transport());
diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h
index cec79038..f2b6425 100644
--- a/pc/data_channel_controller.h
+++ b/pc/data_channel_controller.h
@@ -57,6 +57,8 @@
void AddSctpDataStream(int sid) override;
void RemoveSctpDataStream(int sid) override;
bool ReadyToSendData() const override;
+ void OnChannelStateChanged(SctpDataChannel* channel,
+ DataChannelInterface::DataState state) override;
// Implements DataChannelSink.
void OnDataReceived(int channel_id,
@@ -99,10 +101,6 @@
DataChannelTransportInterface* data_channel_transport() const;
void set_data_channel_transport(DataChannelTransportInterface* transport);
- sigslot::signal1<SctpDataChannel*>& SignalSctpDataChannelCreated() {
- RTC_DCHECK_RUN_ON(signaling_thread());
- return SignalSctpDataChannelCreated_;
- }
// Called when the transport for the data channels is closed or destroyed.
void OnTransportChannelClosed(RTCError error);
@@ -169,9 +167,6 @@
sigslot::signal1<int> SignalDataChannelTransportChannelClosed_s
RTC_GUARDED_BY(signaling_thread());
- sigslot::signal1<SctpDataChannel*> SignalSctpDataChannelCreated_
- RTC_GUARDED_BY(signaling_thread());
-
// Owning PeerConnection.
PeerConnectionInternal* const pc_;
// The weak pointers must be dereferenced and invalidated on the signalling
diff --git a/pc/data_channel_unittest.cc b/pc/data_channel_unittest.cc
index 7b3e865..5ed6d52 100644
--- a/pc/data_channel_unittest.cc
+++ b/pc/data_channel_unittest.cc
@@ -27,7 +27,6 @@
#include "rtc_base/copy_on_write_buffer.h"
#include "rtc_base/gunit.h"
#include "rtc_base/ssl_stream_adapter.h"
-#include "rtc_base/third_party/sigslot/sigslot.h"
#include "rtc_base/thread.h"
#include "test/gtest.h"
@@ -107,20 +106,6 @@
rtc::scoped_refptr<SctpDataChannel> webrtc_data_channel_;
};
-class StateSignalsListener : public sigslot::has_slots<> {
- public:
- int opened_count() const { return opened_count_; }
- int closed_count() const { return closed_count_; }
-
- void OnSignalOpened(DataChannelInterface* data_channel) { ++opened_count_; }
-
- void OnSignalClosed(DataChannelInterface* data_channel) { ++closed_count_; }
-
- private:
- int opened_count_ = 0;
- int closed_count_ = 0;
-};
-
// Verifies that the data channel is connected to the transport after creation.
TEST_F(SctpDataChannelTest, ConnectedToTransportOnCreated) {
controller_->set_transport_available(true);
@@ -150,26 +135,22 @@
// Tests the state of the data channel.
TEST_F(SctpDataChannelTest, StateTransition) {
- StateSignalsListener state_signals_listener;
- webrtc_data_channel_->SignalOpened.connect(
- &state_signals_listener, &StateSignalsListener::OnSignalOpened);
- webrtc_data_channel_->SignalClosed.connect(
- &state_signals_listener, &StateSignalsListener::OnSignalClosed);
EXPECT_EQ(webrtc::DataChannelInterface::kConnecting,
webrtc_data_channel_->state());
- EXPECT_EQ(state_signals_listener.opened_count(), 0);
- EXPECT_EQ(state_signals_listener.closed_count(), 0);
+ EXPECT_EQ(controller_->channels_opened(), 0);
+ EXPECT_EQ(controller_->channels_closed(), 0);
SetChannelReady();
EXPECT_EQ(webrtc::DataChannelInterface::kOpen, webrtc_data_channel_->state());
- EXPECT_EQ(state_signals_listener.opened_count(), 1);
- EXPECT_EQ(state_signals_listener.closed_count(), 0);
+ EXPECT_EQ(controller_->channels_opened(), 1);
+ EXPECT_EQ(controller_->channels_closed(), 0);
+
webrtc_data_channel_->Close();
EXPECT_EQ(webrtc::DataChannelInterface::kClosed,
webrtc_data_channel_->state());
EXPECT_TRUE(webrtc_data_channel_->error().ok());
- EXPECT_EQ(state_signals_listener.opened_count(), 1);
- EXPECT_EQ(state_signals_listener.closed_count(), 1);
+ EXPECT_EQ(controller_->channels_opened(), 1);
+ EXPECT_EQ(controller_->channels_closed(), 1);
// Verifies that it's disconnected from the transport.
EXPECT_FALSE(controller_->IsConnected(webrtc_data_channel_.get()));
}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 88b99bb..ca234f7 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -2113,11 +2113,12 @@
SetSctpTransportName("");
}
-void PeerConnection::OnSctpDataChannelClosed(DataChannelInterface* channel) {
- // Since data_channel_controller doesn't do signals, this
- // signal is relayed here.
- data_channel_controller_.OnSctpDataChannelClosed(
- static_cast<SctpDataChannel*>(channel));
+void PeerConnection::OnSctpDataChannelStateChanged(
+ DataChannelInterface* channel,
+ DataChannelInterface::DataState state) {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ if (stats_collector_)
+ stats_collector_->OnSctpDataChannelStateChanged(channel, state);
}
PeerConnection::InitializePortAllocatorResult
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 850142b..3298203 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -283,11 +283,6 @@
return rtp_manager()->transceivers()->List();
}
- sigslot::signal1<SctpDataChannel*>& SignalSctpDataChannelCreated() override {
- RTC_DCHECK_RUN_ON(signaling_thread());
- return data_channel_controller_.SignalSctpDataChannelCreated();
- }
-
std::vector<DataChannelStats> GetDataChannelStats() const override;
absl::optional<std::string> sctp_transport_name() const override;
@@ -320,8 +315,10 @@
}
// Get current SSL role used by SCTP's underlying transport.
bool GetSctpSslRole(rtc::SSLRole* role) override;
- // Handler for the "channel closed" signal
- void OnSctpDataChannelClosed(DataChannelInterface* channel) override;
+
+ void OnSctpDataChannelStateChanged(
+ DataChannelInterface* channel,
+ DataChannelInterface::DataState state) override;
bool ShouldFireNegotiationNeededEvent(uint32_t event_id) override;
diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h
index 1085ff9..262d2d5 100644
--- a/pc/peer_connection_internal.h
+++ b/pc/peer_connection_internal.h
@@ -130,8 +130,7 @@
// Functions defined in this class are called by other objects,
// but not by SdpOfferAnswerHandler.
class PeerConnectionInternal : public PeerConnectionInterface,
- public PeerConnectionSdpMethods,
- public sigslot::has_slots<> {
+ public PeerConnectionSdpMethods {
public:
virtual rtc::Thread* network_thread() const = 0;
virtual rtc::Thread* worker_thread() const = 0;
@@ -143,9 +142,6 @@
rtc::scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>>>
GetTransceiversInternal() const = 0;
- virtual sigslot::signal1<SctpDataChannel*>&
- SignalSctpDataChannelCreated() = 0;
-
// Call on the network thread to fetch stats for all the data channels.
// TODO(tommi): Make pure virtual after downstream updates.
virtual std::vector<DataChannelStats> GetDataChannelStats() const {
@@ -180,8 +176,10 @@
rtc::SSLRole* role) = 0;
// Functions needed by DataChannelController
virtual void NoteDataAddedEvent() {}
- // Handler for the "channel closed" signal
- virtual void OnSctpDataChannelClosed(DataChannelInterface* channel) {}
+ // Handler for sctp data channel state changes.
+ virtual void OnSctpDataChannelStateChanged(
+ DataChannelInterface* channel,
+ DataChannelInterface::DataState state) {}
};
} // namespace webrtc
diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc
index dcf812e..e443b15 100644
--- a/pc/rtc_stats_collector.cc
+++ b/pc/rtc_stats_collector.cc
@@ -1390,8 +1390,6 @@
RTC_DCHECK(worker_thread_);
RTC_DCHECK(network_thread_);
RTC_DCHECK_GE(cache_lifetime_us_, 0);
- pc_->SignalSctpDataChannelCreated().connect(
- this, &RTCStatsCollector::OnSctpDataChannelCreated);
}
RTCStatsCollector::~RTCStatsCollector() {
@@ -2493,27 +2491,23 @@
}
}
-void RTCStatsCollector::OnSctpDataChannelCreated(SctpDataChannel* channel) {
- channel->SignalOpened.connect(this, &RTCStatsCollector::OnDataChannelOpened);
- channel->SignalClosed.connect(this, &RTCStatsCollector::OnDataChannelClosed);
-}
-
-void RTCStatsCollector::OnDataChannelOpened(DataChannelInterface* channel) {
+void RTCStatsCollector::OnSctpDataChannelStateChanged(
+ DataChannelInterface* channel,
+ DataChannelInterface::DataState state) {
RTC_DCHECK_RUN_ON(signaling_thread_);
- bool result = internal_record_.opened_data_channels
- .insert(reinterpret_cast<uintptr_t>(channel))
- .second;
- ++internal_record_.data_channels_opened;
- RTC_DCHECK(result);
-}
-
-void RTCStatsCollector::OnDataChannelClosed(DataChannelInterface* channel) {
- RTC_DCHECK_RUN_ON(signaling_thread_);
- // Only channels that have been fully opened (and have increased the
- // `data_channels_opened_` counter) increase the closed counter.
- if (internal_record_.opened_data_channels.erase(
- reinterpret_cast<uintptr_t>(channel))) {
- ++internal_record_.data_channels_closed;
+ if (state == DataChannelInterface::DataState::kOpen) {
+ bool result = internal_record_.opened_data_channels
+ .insert(reinterpret_cast<uintptr_t>(channel))
+ .second;
+ RTC_DCHECK(result);
+ ++internal_record_.data_channels_opened;
+ } else if (state == DataChannelInterface::DataState::kClosed) {
+ // Only channels that have been fully opened (and have increased the
+ // `data_channels_opened_` counter) increase the closed counter.
+ if (internal_record_.opened_data_channels.erase(
+ reinterpret_cast<uintptr_t>(channel))) {
+ ++internal_record_.data_channels_closed;
+ }
}
}
diff --git a/pc/rtc_stats_collector.h b/pc/rtc_stats_collector.h
index 89f6e98..6c36d22 100644
--- a/pc/rtc_stats_collector.h
+++ b/pc/rtc_stats_collector.h
@@ -44,7 +44,6 @@
#include "rtc_base/ssl_certificate.h"
#include "rtc_base/ssl_identity.h"
#include "rtc_base/synchronization/mutex.h"
-#include "rtc_base/third_party/sigslot/sigslot.h"
#include "rtc_base/thread.h"
#include "rtc_base/time_utils.h"
@@ -57,8 +56,7 @@
// Stats are gathered on the signaling, worker and network threads
// asynchronously. The callback is invoked on the signaling thread. Resulting
// reports are cached for `cache_lifetime_` ms.
-class RTCStatsCollector : public rtc::RefCountInterface,
- public sigslot::has_slots<> {
+class RTCStatsCollector : public rtc::RefCountInterface {
public:
static rtc::scoped_refptr<RTCStatsCollector> Create(
PeerConnectionInternal* pc,
@@ -92,6 +90,10 @@
// completed. Must be called on the signaling thread.
void WaitForPendingRequest();
+ // Called by the PeerConnection instance when data channel states change.
+ void OnSctpDataChannelStateChanged(DataChannelInterface* channel,
+ DataChannelInterface::DataState state);
+
protected:
RTCStatsCollector(PeerConnectionInternal* pc, int64_t cache_lifetime_us);
~RTCStatsCollector();
@@ -255,12 +257,6 @@
rtc::scoped_refptr<RtpSenderInternal> sender_selector,
rtc::scoped_refptr<RtpReceiverInternal> receiver_selector);
- // Slots for signals (sigslot) that are wired up to `pc_`.
- void OnSctpDataChannelCreated(SctpDataChannel* channel);
- // Slots for signals (sigslot) that are wired up to `channel`.
- void OnDataChannelOpened(DataChannelInterface* channel);
- void OnDataChannelClosed(DataChannelInterface* channel);
-
PeerConnectionInternal* const pc_;
rtc::Thread* const signaling_thread_;
rtc::Thread* const worker_thread_;
diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc
index 2a03c3b..efc9a6e 100644
--- a/pc/rtc_stats_collector_unittest.cc
+++ b/pc/rtc_stats_collector_unittest.cc
@@ -2111,15 +2111,15 @@
rtc::scoped_refptr<SctpDataChannel> dummy_channel_a = SctpDataChannel::Create(
&controller, "DummyChannelA", InternalDataChannelInit(),
rtc::Thread::Current(), rtc::Thread::Current());
- pc_->SignalSctpDataChannelCreated()(dummy_channel_a.get());
rtc::scoped_refptr<SctpDataChannel> dummy_channel_b = SctpDataChannel::Create(
&controller, "DummyChannelB", InternalDataChannelInit(),
rtc::Thread::Current(), rtc::Thread::Current());
- pc_->SignalSctpDataChannelCreated()(dummy_channel_b.get());
- dummy_channel_a->SignalOpened(dummy_channel_a.get());
+ stats_->stats_collector()->OnSctpDataChannelStateChanged(
+ dummy_channel_a.get(), DataChannelInterface::DataState::kOpen);
// Closing a channel that is not opened should not affect the counts.
- dummy_channel_b->SignalClosed(dummy_channel_b.get());
+ stats_->stats_collector()->OnSctpDataChannelStateChanged(
+ dummy_channel_b.get(), DataChannelInterface::DataState::kClosed);
{
rtc::scoped_refptr<const RTCStatsReport> report =
@@ -2131,8 +2131,10 @@
EXPECT_EQ(expected, report->Get("P")->cast_to<RTCPeerConnectionStats>());
}
- dummy_channel_b->SignalOpened(dummy_channel_b.get());
- dummy_channel_b->SignalClosed(dummy_channel_b.get());
+ stats_->stats_collector()->OnSctpDataChannelStateChanged(
+ dummy_channel_b.get(), DataChannelInterface::DataState::kOpen);
+ stats_->stats_collector()->OnSctpDataChannelStateChanged(
+ dummy_channel_b.get(), DataChannelInterface::DataState::kClosed);
{
rtc::scoped_refptr<const RTCStatsReport> report =
@@ -2146,7 +2148,8 @@
// Re-opening a data channel (or opening a new data channel that is re-using
// the same address in memory) should increase the opened count.
- dummy_channel_b->SignalOpened(dummy_channel_b.get());
+ stats_->stats_collector()->OnSctpDataChannelStateChanged(
+ dummy_channel_b.get(), DataChannelInterface::DataState::kOpen);
{
rtc::scoped_refptr<const RTCStatsReport> report =
@@ -2158,8 +2161,10 @@
EXPECT_EQ(expected, report->Get("P")->cast_to<RTCPeerConnectionStats>());
}
- dummy_channel_a->SignalClosed(dummy_channel_a.get());
- dummy_channel_b->SignalClosed(dummy_channel_b.get());
+ stats_->stats_collector()->OnSctpDataChannelStateChanged(
+ dummy_channel_a.get(), DataChannelInterface::DataState::kClosed);
+ stats_->stats_collector()->OnSctpDataChannelStateChanged(
+ dummy_channel_b.get(), DataChannelInterface::DataState::kClosed);
{
rtc::scoped_refptr<const RTCStatsReport> report =
diff --git a/pc/sctp_data_channel.cc b/pc/sctp_data_channel.cc
index 0591bcc..d8efb56 100644
--- a/pc/sctp_data_channel.cc
+++ b/pc/sctp_data_channel.cc
@@ -564,11 +564,9 @@
if (observer_) {
observer_->OnStateChange();
}
- if (state_ == kOpen) {
- SignalOpened(this);
- } else if (state_ == kClosed) {
- SignalClosed(this);
- }
+
+ if (!controller_detached_)
+ controller_->OnChannelStateChanged(this, state_);
}
void SctpDataChannel::DisconnectFromTransport() {
diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h
index a8442c5..909f6bc 100644
--- a/pc/sctp_data_channel.h
+++ b/pc/sctp_data_channel.h
@@ -55,6 +55,9 @@
virtual void RemoveSctpDataStream(int sid) = 0;
// Returns true if the transport channel is ready to send data.
virtual bool ReadyToSendData() const = 0;
+ // Notifies the controller of state changes.
+ virtual void OnChannelStateChanged(SctpDataChannel* data_channel,
+ DataChannelInterface::DataState state) = 0;
protected:
virtual ~SctpDataChannelControllerInterface() {}
@@ -214,12 +217,6 @@
DataChannelStats GetStats() const;
- // Emitted when state transitions to kOpen.
- sigslot::signal1<DataChannelInterface*> SignalOpened;
- // Emitted when state transitions to kClosed.
- // This signal can be used to tell when the channel's sid is free.
- sigslot::signal1<DataChannelInterface*> SignalClosed;
-
// Reset the allocator for internal ID values for testing, so that
// the internal IDs generated are predictable. Test only.
static void ResetInternalIdAllocatorForTesting(int new_value);
diff --git a/pc/test/fake_data_channel_controller.h b/pc/test/fake_data_channel_controller.h
index bdab7d2..158362d 100644
--- a/pc/test/fake_data_channel_controller.h
+++ b/pc/test/fake_data_channel_controller.h
@@ -53,7 +53,7 @@
if (!transport_available_) {
return false;
}
- RTC_LOG(LS_INFO) << "DataChannel connected " << data_channel;
+ RTC_LOG(LS_VERBOSE) << "DataChannel connected " << data_channel;
connected_channels_.insert(data_channel);
return true;
}
@@ -61,7 +61,7 @@
void DisconnectDataChannel(webrtc::SctpDataChannel* data_channel) override {
RTC_CHECK(connected_channels_.find(data_channel) !=
connected_channels_.end());
- RTC_LOG(LS_INFO) << "DataChannel disconnected " << data_channel;
+ RTC_LOG(LS_VERBOSE) << "DataChannel disconnected " << data_channel;
connected_channels_.erase(data_channel);
}
@@ -90,6 +90,16 @@
bool ReadyToSendData() const override { return ready_to_send_; }
+ void OnChannelStateChanged(
+ webrtc::SctpDataChannel* data_channel,
+ webrtc::DataChannelInterface::DataState state) override {
+ if (state == webrtc::DataChannelInterface::DataState::kOpen) {
+ ++channels_opened_;
+ } else if (state == webrtc::DataChannelInterface::DataState::kClosed) {
+ ++channels_closed_;
+ }
+ }
+
// Set true to emulate the SCTP stream being blocked by congestion control.
void set_send_blocked(bool blocked) {
send_blocked_ = blocked;
@@ -146,6 +156,9 @@
return recv_ssrcs_.find(stream) != recv_ssrcs_.end();
}
+ int channels_opened() const { return channels_opened_; }
+ int channels_closed() const { return channels_closed_; }
+
private:
int last_sid_;
webrtc::SendDataParams last_send_data_params_;
@@ -153,6 +166,8 @@
bool transport_available_;
bool ready_to_send_;
bool transport_error_;
+ int channels_closed_ = 0;
+ int channels_opened_ = 0;
std::set<webrtc::SctpDataChannel*> connected_channels_;
std::set<uint32_t> send_ssrcs_;
std::set<uint32_t> recv_ssrcs_;
diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h
index 18b8824..5a1859e 100644
--- a/pc/test/fake_peer_connection_base.h
+++ b/pc/test/fake_peer_connection_base.h
@@ -255,10 +255,6 @@
return {};
}
- sigslot::signal1<SctpDataChannel*>& SignalSctpDataChannelCreated() override {
- return SignalSctpDataChannelCreated_;
- }
-
absl::optional<std::string> sctp_transport_name() const override {
return absl::nullopt;
}
@@ -371,7 +367,6 @@
protected:
webrtc::test::ScopedKeyValueConfig field_trials_;
- sigslot::signal1<SctpDataChannel*> SignalSctpDataChannelCreated_;
};
} // namespace webrtc
diff --git a/pc/test/mock_peer_connection_internal.h b/pc/test/mock_peer_connection_internal.h
index 23ecc93..a47d0b7 100644
--- a/pc/test/mock_peer_connection_internal.h
+++ b/pc/test/mock_peer_connection_internal.h
@@ -282,10 +282,6 @@
GetTransceiversInternal,
(),
(const, override));
- MOCK_METHOD(sigslot::signal1<SctpDataChannel*>&,
- SignalSctpDataChannelCreated,
- (),
- (override));
MOCK_METHOD(std::vector<DataChannelStats>,
GetDataChannelStats,
(),
@@ -322,8 +318,8 @@
(override));
MOCK_METHOD(void, NoteDataAddedEvent, (), (override));
MOCK_METHOD(void,
- OnSctpDataChannelClosed,
- (DataChannelInterface*),
+ OnSctpDataChannelStateChanged,
+ (DataChannelInterface * channel, DataChannelInterface::DataState),
(override));
};
