diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index c6139a9a8e..c8e46c7c1c 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -69,7 +69,7 @@ void DataChannelController::OnChannelStateChanged( if (state == DataChannelInterface::DataState::kClosed) OnSctpDataChannelClosed(channel); - pc_->OnSctpDataChannelStateChanged(channel, state); + pc_->OnSctpDataChannelStateChanged(channel->internal_id(), state); } void DataChannelController::OnDataReceived( diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 4ae7642fce..5697624cb4 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -2115,11 +2115,11 @@ void PeerConnection::ResetSctpDataMid() { } void PeerConnection::OnSctpDataChannelStateChanged( - DataChannelInterface* channel, + int channel_id, DataChannelInterface::DataState state) { RTC_DCHECK_RUN_ON(signaling_thread()); if (stats_collector_) - stats_collector_->OnSctpDataChannelStateChanged(channel, state); + stats_collector_->OnSctpDataChannelStateChanged(channel_id, state); } PeerConnection::InitializePortAllocatorResult diff --git a/pc/peer_connection.h b/pc/peer_connection.h index d1b197c069..51747114f5 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -317,7 +317,7 @@ class PeerConnection : public PeerConnectionInternal, absl::optional GetSctpSslRole_n() override; void OnSctpDataChannelStateChanged( - DataChannelInterface* channel, + int channel_id, 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 2ddb57b50c..abf5825bd6 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -177,8 +177,11 @@ class PeerConnectionInternal : public PeerConnectionInterface, // Functions needed by DataChannelController virtual void NoteDataAddedEvent() {} // Handler for sctp data channel state changes. + // The `channel_id` is the same unique identifier as used in + // `DataChannelStats::internal_id and + // `RTCDataChannelStats::data_channel_identifier`. virtual void OnSctpDataChannelStateChanged( - DataChannelInterface* channel, + int channel_id, DataChannelInterface::DataState state) {} }; diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index 1d8cf3ca6d..429e9d0268 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc @@ -2493,17 +2493,18 @@ void RTCStatsCollector::PrepareTransceiverStatsInfosAndCallStats_s_w_n() { } void RTCStatsCollector::OnSctpDataChannelStateChanged( - DataChannelInterface* channel, + int channel_id, DataChannelInterface::DataState state) { RTC_DCHECK_RUN_ON(signaling_thread_); if (state == DataChannelInterface::DataState::kOpen) { - bool result = internal_record_.opened_data_channels.insert(channel).second; + bool result = + internal_record_.opened_data_channels.insert(channel_id).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(channel)) { + if (internal_record_.opened_data_channels.erase(channel_id)) { ++internal_record_.data_channels_closed; } } diff --git a/pc/rtc_stats_collector.h b/pc/rtc_stats_collector.h index 7c7c177ebe..ac0453fb7c 100644 --- a/pc/rtc_stats_collector.h +++ b/pc/rtc_stats_collector.h @@ -91,7 +91,7 @@ class RTCStatsCollector : public rtc::RefCountInterface { void WaitForPendingRequest(); // Called by the PeerConnection instance when data channel states change. - void OnSctpDataChannelStateChanged(DataChannelInterface* channel, + void OnSctpDataChannelStateChanged(int channel_id, DataChannelInterface::DataState state); protected: @@ -321,12 +321,9 @@ class RTCStatsCollector : public rtc::RefCountInterface { // before reaching the open state does not affect these counters. uint32_t data_channels_opened; uint32_t data_channels_closed; - // Identifies by address channels that have been opened, which remain in the - // set until they have been fully closed. - // NOTE: There is no reference held here or any other type of guarantee that - // these pointers remain valid. So they MUST NOT be followed; hence, void* - // is used and not the explicit type. - webrtc::flat_set opened_data_channels; + // Identifies channels that have been opened, whose internal id is stored in + // the set until they have been fully closed. + webrtc::flat_set opened_data_channels; }; InternalRecord internal_record_; }; diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 5021d19e2b..304272c6cc 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -2132,10 +2132,10 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) { rtc::Thread::Current(), rtc::Thread::Current()); stats_->stats_collector()->OnSctpDataChannelStateChanged( - dummy_channel_a.get(), DataChannelInterface::DataState::kOpen); + dummy_channel_a->internal_id(), DataChannelInterface::DataState::kOpen); // Closing a channel that is not opened should not affect the counts. stats_->stats_collector()->OnSctpDataChannelStateChanged( - dummy_channel_b.get(), DataChannelInterface::DataState::kClosed); + dummy_channel_b->internal_id(), DataChannelInterface::DataState::kClosed); { rtc::scoped_refptr report = @@ -2148,9 +2148,9 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) { } stats_->stats_collector()->OnSctpDataChannelStateChanged( - dummy_channel_b.get(), DataChannelInterface::DataState::kOpen); + dummy_channel_b->internal_id(), DataChannelInterface::DataState::kOpen); stats_->stats_collector()->OnSctpDataChannelStateChanged( - dummy_channel_b.get(), DataChannelInterface::DataState::kClosed); + dummy_channel_b->internal_id(), DataChannelInterface::DataState::kClosed); { rtc::scoped_refptr report = @@ -2165,7 +2165,7 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) { // 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. stats_->stats_collector()->OnSctpDataChannelStateChanged( - dummy_channel_b.get(), DataChannelInterface::DataState::kOpen); + dummy_channel_b->internal_id(), DataChannelInterface::DataState::kOpen); { rtc::scoped_refptr report = @@ -2178,9 +2178,9 @@ TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) { } stats_->stats_collector()->OnSctpDataChannelStateChanged( - dummy_channel_a.get(), DataChannelInterface::DataState::kClosed); + dummy_channel_a->internal_id(), DataChannelInterface::DataState::kClosed); stats_->stats_collector()->OnSctpDataChannelStateChanged( - dummy_channel_b.get(), DataChannelInterface::DataState::kClosed); + dummy_channel_b->internal_id(), DataChannelInterface::DataState::kClosed); { rtc::scoped_refptr report = diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h index 8d95cc4cc8..184603d63f 100644 --- a/pc/sctp_data_channel.h +++ b/pc/sctp_data_channel.h @@ -211,6 +211,11 @@ class SctpDataChannel : public DataChannelInterface { DataChannelStats GetStats() const; + // Returns a unique identifier that's guaranteed to always be available, + // doesn't change throughout SctpDataChannel's lifetime and is used for + // stats purposes (see also `GetStats()`). + int internal_id() const { return internal_id_; } + const StreamId& sid() const { return id_; } // Reset the allocator for internal ID values for testing, so that diff --git a/pc/test/mock_peer_connection_internal.h b/pc/test/mock_peer_connection_internal.h index 81f8903bd5..4647567559 100644 --- a/pc/test/mock_peer_connection_internal.h +++ b/pc/test/mock_peer_connection_internal.h @@ -319,7 +319,7 @@ class MockPeerConnectionInternal : public PeerConnectionInternal { MOCK_METHOD(void, NoteDataAddedEvent, (), (override)); MOCK_METHOD(void, OnSctpDataChannelStateChanged, - (DataChannelInterface * channel, DataChannelInterface::DataState), + (int channel_id, DataChannelInterface::DataState), (override)); };