diff --git a/webrtc/api/datachannel.cc b/webrtc/api/datachannel.cc index af694b7b1c..9812e9bb6a 100644 --- a/webrtc/api/datachannel.cc +++ b/webrtc/api/datachannel.cc @@ -480,7 +480,9 @@ void DataChannel::SetState(DataState state) { if (observer_) { observer_->OnStateChange(); } - if (state_ == kClosed) { + if (state_ == kOpen) { + SignalOpened(this); + } else if (state_ == kClosed) { SignalClosed(this); } } diff --git a/webrtc/api/datachannel.h b/webrtc/api/datachannel.h index 7d7f6c75df..9208ada5ca 100644 --- a/webrtc/api/datachannel.h +++ b/webrtc/api/datachannel.h @@ -180,6 +180,8 @@ class DataChannel : public DataChannelInterface, return data_channel_type_; } + // Emitted when state transitions to kOpen. + sigslot::signal1 SignalOpened; // Emitted when state transitions to kClosed. // In the case of SCTP channels, this signal can be used to tell when the // channel's sid is free. diff --git a/webrtc/api/datachannel_unittest.cc b/webrtc/api/datachannel_unittest.cc index 773e7523e6..a8c8361cb1 100644 --- a/webrtc/api/datachannel_unittest.cc +++ b/webrtc/api/datachannel_unittest.cc @@ -94,6 +94,24 @@ class SctpDataChannelTest : public testing::Test { rtc::scoped_refptr 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(DataChannel* data_channel) { + ++opened_count_; + } + + void OnSignalClosed(DataChannel* 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) { provider_->set_transport_available(true); @@ -122,14 +140,25 @@ TEST_F(SctpDataChannelTest, ConnectedAfterTransportBecomesAvailable) { // 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); 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); webrtc_data_channel_->Close(); EXPECT_EQ(webrtc::DataChannelInterface::kClosed, webrtc_data_channel_->state()); + EXPECT_EQ(state_signals_listener.opened_count(), 1); + EXPECT_EQ(state_signals_listener.closed_count(), 1); // Verifies that it's disconnected from the transport. EXPECT_FALSE(provider_->IsConnected(webrtc_data_channel_.get())); } diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc index 3fdbefb8a9..2f320d78ed 100644 --- a/webrtc/api/peerconnection.cc +++ b/webrtc/api/peerconnection.cc @@ -2102,6 +2102,7 @@ rtc::scoped_refptr PeerConnection::InternalCreateDataChannel( &PeerConnection::OnSctpDataChannelClosed); } + SignalDataChannelCreated(channel.get()); return channel; } diff --git a/webrtc/api/peerconnection.h b/webrtc/api/peerconnection.h index 740958528d..f444eb7294 100644 --- a/webrtc/api/peerconnection.h +++ b/webrtc/api/peerconnection.h @@ -143,6 +143,8 @@ class PeerConnection : public PeerConnectionInterface, void Close() override; + sigslot::signal1 SignalDataChannelCreated; + // Virtual for unit tests. virtual const std::vector>& sctp_data_channels() const { diff --git a/webrtc/api/rtcstatscollector.cc b/webrtc/api/rtcstatscollector.cc index d1145ebc53..fbc6dbc5d3 100644 --- a/webrtc/api/rtcstatscollector.cc +++ b/webrtc/api/rtcstatscollector.cc @@ -338,6 +338,8 @@ RTCStatsCollector::RTCStatsCollector(PeerConnection* pc, RTC_DCHECK(worker_thread_); RTC_DCHECK(network_thread_); RTC_DCHECK_GE(cache_lifetime_us_, 0); + pc_->SignalDataChannelCreated.connect( + this, &RTCStatsCollector::OnDataChannelCreated); } void RTCStatsCollector::GetStatsReport( @@ -581,23 +583,10 @@ void RTCStatsCollector::ProduceMediaStreamAndTrackStats_s( void RTCStatsCollector::ProducePeerConnectionStats_s( int64_t timestamp_us, RTCStatsReport* report) const { RTC_DCHECK(signaling_thread_->IsCurrent()); - // TODO(hbos): If data channels are removed from the peer connection this will - // yield incorrect counts. Address before closing crbug.com/636818. See - // https://w3c.github.io/webrtc-stats/webrtc-stats.html#pcstats-dict*. - uint32_t data_channels_opened = 0; - const std::vector>& data_channels = - pc_->sctp_data_channels(); - for (const rtc::scoped_refptr& data_channel : data_channels) { - if (data_channel->state() == DataChannelInterface::kOpen) - ++data_channels_opened; - } - // There is always just one |RTCPeerConnectionStats| so its |id| can be a - // constant. std::unique_ptr stats( new RTCPeerConnectionStats("RTCPeerConnection", timestamp_us)); - stats->data_channels_opened = data_channels_opened; - stats->data_channels_closed = static_cast(data_channels.size()) - - data_channels_opened; + stats->data_channels_opened = internal_record_.data_channels_opened; + stats->data_channels_closed = internal_record_.data_channels_closed; report->AddStats(std::move(stats)); } @@ -786,6 +775,30 @@ RTCStatsCollector::PrepareTransportCertificateStats_s( return transport_cert_stats; } +void RTCStatsCollector::OnDataChannelCreated(DataChannel* channel) { + channel->SignalOpened.connect(this, &RTCStatsCollector::OnDataChannelOpened); + channel->SignalClosed.connect(this, &RTCStatsCollector::OnDataChannelClosed); +} + +void RTCStatsCollector::OnDataChannelOpened(DataChannel* channel) { + RTC_DCHECK(signaling_thread_->IsCurrent()); + bool result = internal_record_.opened_data_channels.insert( + reinterpret_cast(channel)).second; + ++internal_record_.data_channels_opened; + RTC_DCHECK(result); +} + +void RTCStatsCollector::OnDataChannelClosed(DataChannel* channel) { + RTC_DCHECK(signaling_thread_->IsCurrent()); + // 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.find( + reinterpret_cast(channel)) != + internal_record_.opened_data_channels.end()) { + ++internal_record_.data_channels_closed; + } +} + const char* CandidateTypeToRTCIceCandidateTypeForTesting( const std::string& type) { return CandidateTypeToRTCIceCandidateType(type); diff --git a/webrtc/api/rtcstatscollector.h b/webrtc/api/rtcstatscollector.h index ae4ba19abc..08bab803e2 100644 --- a/webrtc/api/rtcstatscollector.h +++ b/webrtc/api/rtcstatscollector.h @@ -13,14 +13,17 @@ #include #include +#include #include +#include "webrtc/api/datachannel.h" #include "webrtc/api/datachannelinterface.h" #include "webrtc/api/stats/rtcstats_objects.h" #include "webrtc/api/stats/rtcstatsreport.h" #include "webrtc/base/asyncinvoker.h" #include "webrtc/base/refcount.h" #include "webrtc/base/scoped_ref_ptr.h" +#include "webrtc/base/sigslot.h" #include "webrtc/base/sslidentity.h" #include "webrtc/base/timeutils.h" @@ -49,7 +52,8 @@ class RTCStatsCollectorCallback : public virtual rtc::RefCountInterface { // 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 virtual rtc::RefCountInterface { +class RTCStatsCollector : public virtual rtc::RefCountInterface, + public sigslot::has_slots<> { public: static rtc::scoped_refptr Create( PeerConnection* pc, @@ -118,6 +122,12 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface { std::map PrepareTransportCertificateStats_s(const SessionStats& session_stats) const; + // Slots for signals (sigslot) that are wired up to |pc_|. + void OnDataChannelCreated(DataChannel* channel); + // Slots for signals (sigslot) that are wired up to |channel|. + void OnDataChannelOpened(DataChannel* channel); + void OnDataChannelClosed(DataChannel* channel); + PeerConnection* const pc_; rtc::Thread* const signaling_thread_; rtc::Thread* const worker_thread_; @@ -136,6 +146,25 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface { int64_t cache_timestamp_us_; int64_t cache_lifetime_us_; rtc::scoped_refptr cached_report_; + + // Data recorded and maintained by the stats collector during its lifetime. + // Some stats are produced from this record instead of other components. + struct InternalRecord { + InternalRecord() : data_channels_opened(0), + data_channels_closed(0) {} + + // The opened count goes up when a channel is fully opened and the closed + // count goes up if a previously opened channel has fully closed. The opened + // count does not go down when a channel closes, meaning (opened - closed) + // is the number of channels currently opened. A channel that is closed + // 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. + std::set opened_data_channels; + }; + InternalRecord internal_record_; }; const char* CandidateTypeToRTCIceCandidateTypeForTesting( diff --git a/webrtc/api/rtcstatscollector_unittest.cc b/webrtc/api/rtcstatscollector_unittest.cc index 73d70dc2a6..e032384238 100644 --- a/webrtc/api/rtcstatscollector_unittest.cc +++ b/webrtc/api/rtcstatscollector_unittest.cc @@ -1070,44 +1070,56 @@ TEST_F(RTCStatsCollectorTest, CollectRTCIceCandidatePairStats) { } TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) { - rtc::scoped_refptr report = GetStatsReport(); - EXPECT_EQ(report->GetStatsOfType().size(), - static_cast(1)) << "Expecting 1 RTCPeerConnectionStats."; - const RTCStats* stats = report->Get("RTCPeerConnection"); - EXPECT_TRUE(stats); { - // Expected stats with no data channels - const RTCPeerConnectionStats& pcstats = - stats->cast_to(); - EXPECT_EQ(*pcstats.data_channels_opened, static_cast(0)); - EXPECT_EQ(*pcstats.data_channels_closed, static_cast(0)); + rtc::scoped_refptr report = GetStatsReport(); + RTCPeerConnectionStats expected("RTCPeerConnection", + report->timestamp_us()); + expected.data_channels_opened = 0; + expected.data_channels_closed = 0; + EXPECT_TRUE(report->Get("RTCPeerConnection")); + EXPECT_EQ(expected, + report->Get("RTCPeerConnection")->cast_to< + RTCPeerConnectionStats>()); } - test_->data_channels().push_back( - new MockDataChannel(0, DataChannelInterface::kConnecting)); - test_->data_channels().push_back( - new MockDataChannel(1, DataChannelInterface::kOpen)); - test_->data_channels().push_back( - new MockDataChannel(2, DataChannelInterface::kClosing)); - test_->data_channels().push_back( - new MockDataChannel(3, DataChannelInterface::kClosed)); + rtc::scoped_refptr dummy_channel_a = DataChannel::Create( + nullptr, cricket::DCT_NONE, "DummyChannelA", InternalDataChannelInit()); + test_->pc().SignalDataChannelCreated(dummy_channel_a.get()); + rtc::scoped_refptr dummy_channel_b = DataChannel::Create( + nullptr, cricket::DCT_NONE, "DummyChannelB", InternalDataChannelInit()); + test_->pc().SignalDataChannelCreated(dummy_channel_b.get()); + + dummy_channel_a->SignalOpened(dummy_channel_a.get()); + // Closing a channel that is not opened should not affect the counts. + dummy_channel_b->SignalClosed(dummy_channel_b.get()); - collector_->ClearCachedStatsReport(); - report = GetStatsReport(); - EXPECT_EQ(report->GetStatsOfType().size(), - static_cast(1)) << "Expecting 1 RTCPeerConnectionStats."; - stats = report->Get("RTCPeerConnection"); - ASSERT_TRUE(stats); { - // Expected stats with the above four data channels - // TODO(hbos): When the |RTCPeerConnectionStats| is the number of data - // channels that have been opened and closed, not the numbers currently - // open/closed, we would expect opened >= closed and (opened - closed) to be - // the number currently open. crbug.com/636818. - const RTCPeerConnectionStats& pcstats = - stats->cast_to(); - EXPECT_EQ(*pcstats.data_channels_opened, static_cast(1)); - EXPECT_EQ(*pcstats.data_channels_closed, static_cast(3)); + collector_->ClearCachedStatsReport(); + rtc::scoped_refptr report = GetStatsReport(); + RTCPeerConnectionStats expected("RTCPeerConnection", + report->timestamp_us()); + expected.data_channels_opened = 1; + expected.data_channels_closed = 0; + EXPECT_TRUE(report->Get("RTCPeerConnection")); + EXPECT_EQ(expected, + report->Get("RTCPeerConnection")->cast_to< + RTCPeerConnectionStats>()); + } + + dummy_channel_b->SignalOpened(dummy_channel_b.get()); + dummy_channel_b->SignalClosed(dummy_channel_b.get()); + + { + collector_->ClearCachedStatsReport(); + rtc::scoped_refptr report = GetStatsReport(); + RTCPeerConnectionStats expected("RTCPeerConnection", + report->timestamp_us()); + expected.data_channels_opened = 2; + expected.data_channels_closed = 1; + EXPECT_TRUE(report->Get("RTCPeerConnection")); + EXPECT_EQ(expected, + report->Get("RTCPeerConnection")->cast_to< + RTCPeerConnectionStats>()); } }