Correct stats for RTCPeerConnectionStats.dataChannels[Opened/Closed].

webrtc/api/rtcstatscollector.cc

 /*
  *  Copyright 2016 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 213 matching lines @@
 
 RTCStatsCollector::RTCStatsCollector(PeerConnection* pc,
                                      int64_t cache_lifetime_us)
     : pc_(pc),
       signaling_thread_(pc->session()->signaling_thread()),
       worker_thread_(pc->session()->worker_thread()),
       network_thread_(pc->session()->network_thread()),
       num_pending_partial_reports_(0),
       partial_report_timestamp_us_(0),
       cache_timestamp_us_(0),
-      cache_lifetime_us_(cache_lifetime_us) {
+      cache_lifetime_us_(cache_lifetime_us),
+      data_channels_opened_(0),
+      data_channels_closed_(0) {
   RTC_DCHECK(pc_);
   RTC_DCHECK(signaling_thread_);
   RTC_DCHECK(worker_thread_);
   RTC_DCHECK(network_thread_);
   RTC_DCHECK_GE(cache_lifetime_us_, 0);
 }
 
 void RTCStatsCollector::GetStatsReport(
     rtc::scoped_refptr<RTCStatsCollectorCallback> callback) {
   RTC_DCHECK(signaling_thread_->IsCurrent());
@@ skipping 218 matching lines @@
 
         report->AddStats(std::move(candidate_pair_stats));
       }
     }
   }
 }
 
 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 = data_channels_opened_;
+  stats->data_channels_closed = data_channels_closed_;
   report->AddStats(std::move(stats));
 }
 
 void RTCStatsCollector::ProduceRTPStreamStats_s(
     int64_t timestamp_us, const SessionStats& session_stats,
     RTCStatsReport* report) const {
   RTC_DCHECK(signaling_thread_->IsCurrent());
 
   // Audio
   if (pc_->session()->voice_channel()) {
@@ skipping 168 matching lines @@
     if (remote_certificate) {
       certificate_stats_pair.remote = remote_certificate->GetStats();
     }
     transport_cert_stats.insert(
         std::make_pair(transport_stats.second.transport_name,
                        std::move(certificate_stats_pair)));
   }
   return transport_cert_stats;
 }
 
+void RTCStatsCollector::OnDataChannelOpened(DataChannel* channel) {
+  RTC_DCHECK(signaling_thread_->IsCurrent());
+  if (opened_data_channels_.insert(channel).second) {
+    ++data_channels_opened_;

[hta-webrtc, 2016/11/03 13:49:58]

Isn't this equvalent to

bool result = opened_data_channels_.insert(...
RTC_CHECK(result)

?

[hbos, 2016/11/03 15:02:02]

Done.

+  } else {
+    RTC_NOTREACHED();
+  }
+}
+
+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 (opened_data_channels_.find(channel) != opened_data_channels_.end()) {
+    ++data_channels_closed_;

[hta-webrtc, 2016/11/03 13:49:58]

Is it possible to get into the state where a channel signals closed but not
open?

You might want to add a DCHECK to be sure - if it doesn't trigger ever, you can
remove the set of opened channels.

[hbos, 2016/11/03 15:02:02]

From looking at the code I think this would happen if the channel's Close() is
called while it is in the connecting state, and I think it is perfectly legal to
do so and thus I should not DCHECK. That said, I have not spent the time to set
up a scenario where this happens.

[Taylor Brandstetter, 2016/11/09 00:32:48]

Yes, this is definitely legal. Any kind of underlying SCTP error would cause
this.

+  }
+}
+
+void RTCStatsCollector::OnDataChannelOpenedForTesting(DataChannel* channel) {
+  OnDataChannelOpened(channel);
+}
+
+void RTCStatsCollector::OnDataChannelClosedForTesting(DataChannel* channel) {
+  OnDataChannelClosed(channel);
+}
+
 const char* CandidateTypeToRTCIceCandidateTypeForTesting(
     const std::string& type) {
   return CandidateTypeToRTCIceCandidateType(type);
 }
 
 const char* DataStateToRTCDataChannelStateForTesting(
     DataChannelInterface::DataState state) {
   return DataStateToRTCDataChannelState(state);
 }
 
 }  // namespace webrtc