Switch from pointer to ID for OnSctpDataChannelStateChanged
* The pointer isn't needed for this notification. Arguably using
the internal id is more consistent with the stats code.
* Using the int makes it safer down the line to post the operation
from the network thread to the signaling thread rather than post
an object reference.
Bug: none
Change-Id: I1e9eb31d8386dca3feaa90ee3267ea98eb3e81ec
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299144
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39696}
diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
index c6139a9..c8e46c7 100644
--- a/pc/data_channel_controller.cc
+++ b/pc/data_channel_controller.cc
@@ -69,7 +69,7 @@
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 4ae7642..5697624 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -2115,11 +2115,11 @@
}
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 d1b197c..5174711 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -317,7 +317,7 @@
absl::optional<rtc::SSLRole> 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 2ddb57b..abf5825 100644
--- a/pc/peer_connection_internal.h
+++ b/pc/peer_connection_internal.h
@@ -177,8 +177,11 @@
// 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 1d8cf3c..429e9d0 100644
--- a/pc/rtc_stats_collector.cc
+++ b/pc/rtc_stats_collector.cc
@@ -2493,17 +2493,18 @@
}
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 7c7c177..ac0453f 100644
--- a/pc/rtc_stats_collector.h
+++ b/pc/rtc_stats_collector.h
@@ -91,7 +91,7 @@
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 @@
// 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<void*> 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<int> opened_data_channels;
};
InternalRecord internal_record_;
};
diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc
index 5021d19..304272c 100644
--- a/pc/rtc_stats_collector_unittest.cc
+++ b/pc/rtc_stats_collector_unittest.cc
@@ -2132,10 +2132,10 @@
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<const RTCStatsReport> report =
@@ -2148,9 +2148,9 @@
}
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<const RTCStatsReport> report =
@@ -2165,7 +2165,7 @@
// 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<const RTCStatsReport> report =
@@ -2178,9 +2178,9 @@
}
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<const RTCStatsReport> report =
diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h
index 8d95cc4..184603d 100644
--- a/pc/sctp_data_channel.h
+++ b/pc/sctp_data_channel.h
@@ -211,6 +211,11 @@
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 81f8903..4647567 100644
--- a/pc/test/mock_peer_connection_internal.h
+++ b/pc/test/mock_peer_connection_internal.h
@@ -319,7 +319,7 @@
MOCK_METHOD(void, NoteDataAddedEvent, (), (override));
MOCK_METHOD(void,
OnSctpDataChannelStateChanged,
- (DataChannelInterface * channel, DataChannelInterface::DataState),
+ (int channel_id, DataChannelInterface::DataState),
(override));
};
