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

@@ -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);
   }
 }

webrtc/api/datachannel.h

@@ -180,6 +180,8 @@ class DataChannel : public DataChannelInterface,
     return data_channel_type_;
   }
 
+  // Emitted when state transitions to kOpen.
+  sigslot::signal1<DataChannel*> 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.

webrtc/api/datachannel_unittest.cc

@@ -94,6 +94,24 @@ class SctpDataChannelTest : public testing::Test {
   rtc::scoped_refptr<DataChannel> 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()));
 }

webrtc/api/peerconnection.cc

@@ -2102,6 +2102,7 @@ rtc::scoped_refptr<DataChannel> PeerConnection::InternalCreateDataChannel(
                                   &PeerConnection::OnSctpDataChannelClosed);
   }
 
+  SignalDataChannelCreated(channel.get());
   return channel;
 }
 

webrtc/api/peerconnection.h

@@ -143,6 +143,8 @@ class PeerConnection : public PeerConnectionInterface,
 
   void Close() override;
 
+  sigslot::signal1<DataChannel*> SignalDataChannelCreated;
+
   // Virtual for unit tests.
   virtual const std::vector<rtc::scoped_refptr<DataChannel>>&
   sctp_data_channels() const {

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<rtc::scoped_refptr<DataChannel>>& data_channels =
-      pc_->sctp_data_channels();
-  for (const rtc::scoped_refptr<DataChannel>& 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<RTCPeerConnectionStats> stats(
     new RTCPeerConnectionStats("RTCPeerConnection", timestamp_us));
-  stats->data_channels_opened = data_channels_opened;
-  stats->data_channels_closed = static_cast<uint32_t>(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<uintptr_t>(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<uintptr_t>(channel)) !=
+      internal_record_.opened_data_channels.end()) {
+    ++internal_record_.data_channels_closed;
+  }
+}
+
 const char* CandidateTypeToRTCIceCandidateTypeForTesting(
     const std::string& type) {
   return CandidateTypeToRTCIceCandidateType(type);

webrtc/api/rtcstatscollector.h

@@ -13,14 +13,17 @@
 
 #include <map>
 #include <memory>
+#include <set>
 #include <vector>
 
+#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<RTCStatsCollector> Create(
       PeerConnection* pc,
@@ -118,6 +122,12 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface {
   std::map<std::string, CertificateStatsPair>
   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<const RTCStatsReport> 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<uintptr_t> opened_data_channels;
+  };
+  InternalRecord internal_record_;
 };
 
 const char* CandidateTypeToRTCIceCandidateTypeForTesting(

webrtc/api/rtcstatscollector_unittest.cc

@@ -1070,44 +1070,56 @@ TEST_F(RTCStatsCollectorTest, CollectRTCIceCandidatePairStats) {
 }
 
 TEST_F(RTCStatsCollectorTest, CollectRTCPeerConnectionStats) {
-  rtc::scoped_refptr<const RTCStatsReport> report = GetStatsReport();
-  EXPECT_EQ(report->GetStatsOfType<RTCPeerConnectionStats>().size(),
-            static_cast<size_t>(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<RTCPeerConnectionStats>();
-    EXPECT_EQ(*pcstats.data_channels_opened, static_cast<uint32_t>(0));
-    EXPECT_EQ(*pcstats.data_channels_closed, static_cast<uint32_t>(0));
+    rtc::scoped_refptr<const RTCStatsReport> 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<DataChannel> dummy_channel_a = DataChannel::Create(
+      nullptr, cricket::DCT_NONE, "DummyChannelA", InternalDataChannelInit());
+  test_->pc().SignalDataChannelCreated(dummy_channel_a.get());
+  rtc::scoped_refptr<DataChannel> 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<RTCPeerConnectionStats>().size(),
-            static_cast<size_t>(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<RTCPeerConnectionStats>();
-    EXPECT_EQ(*pcstats.data_channels_opened, static_cast<uint32_t>(1));
-    EXPECT_EQ(*pcstats.data_channels_closed, static_cast<uint32_t>(3));
+    collector_->ClearCachedStatsReport();
+    rtc::scoped_refptr<const RTCStatsReport> 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<const RTCStatsReport> 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>());
   }
 }