From 82ebe02491d066697717ae386f886b752729e013 Mon Sep 17 00:00:00 2001 From: hbos Date: Mon, 14 Nov 2016 01:41:09 -0800 Subject: [PATCH] Correct stats for RTCPeerConnectionStats.dataChannels[Opened/Closed]. DataChannel.SignalOpened and unittests added. PeerConnection.SignalDataChannelCreated added and wired up to RTCStatsCollector.OnDataChannelCreated on RTCStatsCollector construction. RTCStatsCollector.OnSignalOpened/Closed added and wired up on OnDataChannelCreated. rtcstatscollector_unittest.cc updated, faking that channels are opened and closed. I did not want to use DataChannelObserver because it is used for more than state changes and there can only be one observer (unless code is updated). Since DataChannel already had a SignalClosed it made sense to add a SignalOpened. Having OnSignalBlah in RTCStatsCollector is new in this CL but will likely be needed to correctly handle RTPMediaStreamTracks being added and detached independently of getStats. This CL establishes this pattern. (An integration test will be needed for this and all the other stats to make sure everything is wired up correctly and test outside of a mock/fake environment, but this is not news.) BUG=chromium:636818, chromium:627816 Review-Url: https://codereview.webrtc.org/2472113002 Cr-Commit-Position: refs/heads/master@{#15059} --- webrtc/api/datachannel.cc | 4 +- webrtc/api/datachannel.h | 2 + webrtc/api/datachannel_unittest.cc | 29 +++++++++ webrtc/api/peerconnection.cc | 1 + webrtc/api/peerconnection.h | 2 + webrtc/api/rtcstatscollector.cc | 43 ++++++++----- webrtc/api/rtcstatscollector.h | 31 +++++++++- webrtc/api/rtcstatscollector_unittest.cc | 78 ++++++++++++++---------- 8 files changed, 140 insertions(+), 50 deletions(-) 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>()); } }