Bug 1151139 - Simplify how we choose which streams to gather stats from. r=mt, a=1.4+
☠☠ backed out by 25ceaaf19c0a ☠ ☠
authorByron Campen [:bwc] <docfaraday@gmail.com>
Mon, 06 Apr 2015 11:52:28 -0700
changeset 188825 e379411f1404c51a966a7ec859baf85f02a48351
parent 188824 56820b564d9cf49f83cc96ea104d6754d3992743
child 188826 25ceaaf19c0a2453bf463a436b059d6c16c80e7a
push id864
push userryanvm@gmail.com
push dateThu, 23 Apr 2015 13:10:04 +0000
treeherdermozilla-b2g30_v1_4@e379411f1404 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmt, 1
bugs1151139
milestone30.0
Bug 1151139 - Simplify how we choose which streams to gather stats from. r=mt, a=1.4+
dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveAudio.html
dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideo.html
dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideoAudio.html
media/mtransport/nricectx.h
media/mtransport/nricemediastream.cpp
media/mtransport/nricemediastream.h
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
--- a/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveAudio.html
+++ b/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveAudio.html
@@ -12,15 +12,17 @@
 <script type="application/javascript">
   createHTML({
     bug: "850275",
     title: "Simple offer media constraint test with audio"
   });
 
   runNetworkTest(function() {
     var test = new PeerConnectionTest();
+    test.chain.remove('PC_LOCAL_CHECK_STATS');
+    test.chain.remove('PC_REMOTE_CHECK_STATS');
     test.setOfferConstraints({ mandatory: { OfferToReceiveAudio: true } });
     test.run();
   });
 </script>
 </pre>
 </body>
 </html>
--- a/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideo.html
+++ b/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideo.html
@@ -12,15 +12,17 @@
 <script type="application/javascript">
   createHTML({
     bug: "850275",
     title: "Simple offer media constraint test with video"
   });
 
   runNetworkTest(function() {
     var test = new PeerConnectionTest();
+    test.chain.remove('PC_LOCAL_CHECK_STATS');
+    test.chain.remove('PC_REMOTE_CHECK_STATS');
     test.setOfferConstraints({ mandatory: { OfferToReceiveVideo: true } });
     test.run();
   });
 </script>
 </pre>
 </body>
 </html>
--- a/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideoAudio.html
+++ b/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideoAudio.html
@@ -12,16 +12,18 @@
 <script type="application/javascript">
   createHTML({
     bug: "850275",
     title: "Simple offer media constraint test with video/audio"
   });
 
   runNetworkTest(function() {
     var test = new PeerConnectionTest();
+    test.chain.remove('PC_LOCAL_CHECK_STATS');
+    test.chain.remove('PC_REMOTE_CHECK_STATS');
     test.setOfferConstraints({ mandatory: {
       OfferToReceiveVideo: true,
       OfferToReceiveAudio: true
     }});
     test.run();
   });
 </script>
 </pre>
--- a/media/mtransport/nricectx.h
+++ b/media/mtransport/nricectx.h
@@ -191,16 +191,29 @@ class NrIceCtx {
 
   // Testing only.
   void destroy_peer_ctx();
 
   // Create a media stream
   RefPtr<NrIceMediaStream> CreateStream(const std::string& name,
                                                  int components);
 
+  RefPtr<NrIceMediaStream> GetStream(size_t index) {
+    if (index < streams_.size()) {
+      return streams_[index];
+    }
+    return nullptr;
+  }
+
+  // Some might be null
+  size_t GetStreamCount() const
+  {
+    return streams_.size();
+  }
+
   // The name of the ctx
   const std::string& name() const { return name_; }
 
   // Current state
   ConnectionState connection_state() const {
     return connection_state_;
   }
 
--- a/media/mtransport/nricemediastream.cpp
+++ b/media/mtransport/nricemediastream.cpp
@@ -204,16 +204,17 @@ nsresult NrIceMediaStream::ParseAttribut
                                                   &attributes_in[0] : nullptr,
                                                   attributes_in.size());
   if (r) {
     MOZ_MTLOG(ML_ERROR, "Couldn't parse attributes for stream "
               << name_ << "'");
     return NS_ERROR_FAILURE;
   }
 
+  has_parsed_attrs_ = true;
   return NS_OK;
 }
 
 // Parse trickle ICE candidate
 nsresult NrIceMediaStream::ParseTrickleCandidate(const std::string& candidate) {
   int r;
 
   MOZ_MTLOG(ML_DEBUG, "NrIceCtx(" << ctx_->name() << ")/STREAM(" <<
--- a/media/mtransport/nricemediastream.h
+++ b/media/mtransport/nricemediastream.h
@@ -144,16 +144,17 @@ class NrIceMediaStream {
   // queue, in priority order. |out_pairs| is cleared before being filled.
   nsresult GetCandidatePairs(std::vector<NrIceCandidatePair>* out_pairs) const;
 
   // Get the default candidate as host and port
   nsresult GetDefaultCandidate(int component, std::string *host, int *port);
 
   // Parse remote attributes
   nsresult ParseAttributes(std::vector<std::string>& candidates);
+  bool HasParsedAttributes() const { return has_parsed_attrs_; }
 
   // Parse trickle ICE candidate
   nsresult ParseTrickleCandidate(const std::string& candidate);
 
   // Disable a component
   nsresult DisableComponent(int component);
 
   // Get the candidate pair currently active. It's the
@@ -199,23 +200,25 @@ class NrIceMediaStream {
  private:
   NrIceMediaStream(NrIceCtx *ctx,  const std::string& name,
                    int components) :
       state_(ICE_CONNECTING),
       ctx_(ctx),
       name_(name),
       components_(components),
       stream_(nullptr),
-      opaque_(nullptr) {}
+      opaque_(nullptr),
+      has_parsed_attrs_(false) {}
 
   DISALLOW_COPY_ASSIGN(NrIceMediaStream);
 
   State state_;
   NrIceCtx *ctx_;
   const std::string name_;
   const int components_;
   nr_ice_media_stream *stream_;
   ScopedDeletePtr<NrIceOpaque> opaque_;
+  bool has_parsed_attrs_;
 };
 
 
 }  // close namespace
 #endif
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -144,17 +144,18 @@ PRLogModuleInfo *signalingLogInfo() {
   }
   return logModuleInfo;
 }
 
 
 namespace sipcc {
 
 #ifdef MOZILLA_INTERNAL_API
-RTCStatsQuery::RTCStatsQuery(bool internal) : internalStats(internal) {
+RTCStatsQuery::RTCStatsQuery(bool internal) : internalStats(internal),
+  grabAllLevels(false) {
 }
 
 RTCStatsQuery::~RTCStatsQuery() {
   MOZ_ASSERT(NS_IsMainThread());
 }
 
 #endif
 
@@ -1957,69 +1958,20 @@ PeerConnectionImpl::BuildStatsQuery_m(
   for (int i = 0, len = mMedia->RemoteStreamsLength(); i < len; i++) {
     PushBackSelect(query->pipelines,
                    mMedia->GetRemoteStream(i)->GetPipelines(),
                    trackId);
   }
 
   query->iceCtx = mMedia->ice_ctx();
 
-  // From the list of MediaPipelines, determine the set of NrIceMediaStreams
-  // we are interested in.
-  std::set<size_t> streamsGrabbed;
-  for (size_t p = 0; p < query->pipelines.Length(); ++p) {
-
-    size_t level = query->pipelines[p]->level();
-    MOZ_ASSERT(level);
-
-    // Don't grab the same stream twice, since that causes duplication
-    // of the ICE stats.
-    if (streamsGrabbed.count(level)) {
-      continue;
-    }
-
-    streamsGrabbed.insert(level);
-    // TODO(bcampen@mozilla.com): I may need to revisit this for bundle.
-    // (Bug 786234)
-    RefPtr<NrIceMediaStream> temp(mMedia->ice_media_stream(level - 1));
-    if (temp) {
-      query->streams.AppendElement(temp);
-    } else {
-       CSFLogError(logTag, "Failed to get NrIceMediaStream for level %zu "
-                           "in %s:  %s",
-                           static_cast<size_t>(level),
-                           __FUNCTION__,
-                           mHandle.c_str());
-       MOZ_CRASH();
-    }
+  if (!trackId) {
+    query->grabAllLevels = true;
   }
 
-  // If the selector is null, we want to get ICE stats for the DataChannel
-  if (!aSelector && mDataConnection) {
-    std::vector<uint16_t> streamIds;
-    mDataConnection->GetStreamIds(&streamIds);
-
-    for (auto s = streamIds.begin(); s!= streamIds.end(); ++s) {
-      MOZ_ASSERT(*s);
-
-      if (streamsGrabbed.count(*s) || *s == INVALID_STREAM) {
-        continue;
-      }
-
-      streamsGrabbed.insert(*s);
-
-      RefPtr<NrIceMediaStream> temp(mMedia->ice_media_stream(*s - 1));
-
-      // This will be null if DataChannel is not in use
-      RefPtr<TransportFlow> flow(mMedia->GetTransportFlow(*s, false));
-      if (temp && flow) {
-        query->streams.AppendElement(temp);
-      }
-    }
-  }
   return rv;
 }
 
 static void ToRTCIceCandidateStats(
     const std::vector<NrIceCandidate>& candidates,
     RTCStatsType candidateType,
     const nsString& componentId,
     DOMHighResTimeStamp now,
@@ -2048,16 +2000,19 @@ static void ToRTCIceCandidateStats(
   }
 }
 
 static void RecordIceStats_s(
     NrIceMediaStream& mediaStream,
     bool internalStats,
     DOMHighResTimeStamp now,
     RTCStatsReportInternal* report) {
+  if (!mediaStream.HasParsedAttributes()) {
+    return;
+  }
 
   NS_ConvertASCIItoUTF16 componentId(mediaStream.name().c_str());
   if (internalStats) {
     std::vector<NrIceCandidatePair> candPairs;
     nsresult res = mediaStream.GetCandidatePairs(&candPairs);
     if (NS_FAILED(res)) {
       CSFLogError(logTag, "%s: Error getting candidate pairs", __FUNCTION__);
       return;
@@ -2242,26 +2197,43 @@ PeerConnectionImpl::ExecuteStatsQuery_s(
             s.mMozJitterBufferDelay.Construct(jitterBufferDelay);
             s.mMozAvSyncDelay.Construct(avSyncDelta);
           }
         }
         query->report.mInboundRTPStreamStats.Value().AppendElement(s);
         break;
       }
     }
+
+    if (!query->grabAllLevels) {
+      // If we're grabbing all levels, that means we want datachannels too,
+      // which don't have pipelines.
+      if (query->iceCtx->GetStream(p - 1)) {
+        RecordIceStats_s(*query->iceCtx->GetStream(p - 1),
+                         query->internalStats,
+                         query->now,
+                         &(query->report));
+      }
+    }
   }
 
-  // Gather stats from ICE
-  for (size_t s = 0; s != query->streams.Length(); ++s) {
-    RecordIceStats_s(*query->streams[s],
-                     query->internalStats,
-                     query->now,
-                     &(query->report));
+  if (query->grabAllLevels) {
+    for (size_t i = 0; i < query->iceCtx->GetStreamCount(); ++i) {
+      if (query->iceCtx->GetStream(i)) {
+        RecordIceStats_s(*query->iceCtx->GetStream(i),
+                         query->internalStats,
+                         query->now,
+                         &(query->report));
+      }
+    }
   }
 
+  // NrIceCtx must be destroyed on STS, so it is not safe
+  // to dispatch it back to main.
+  query->iceCtx = nullptr;
   return NS_OK;
 }
 
 void PeerConnectionImpl::GetStatsForPCObserver_s(
     const std::string& pcHandle, // The Runnable holds the memory
     nsAutoPtr<RTCStatsQuery> query) {
 
   MOZ_ASSERT(query);
--- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ -168,17 +168,17 @@ class RTCStatsQuery {
     std::string error;
 
   private:
     friend class PeerConnectionImpl;
     std::string pcName;
     bool internalStats;
     nsTArray<mozilla::RefPtr<mozilla::MediaPipeline>> pipelines;
     mozilla::RefPtr<NrIceCtx> iceCtx;
-    nsTArray<mozilla::RefPtr<NrIceMediaStream>> streams;
+    bool grabAllLevels;
     DOMHighResTimeStamp now;
 };
 #endif // MOZILLA_INTERNAL_API
 
 // Enter an API call and check that the state is OK,
 // the PC isn't closed, etc.
 #define PC_AUTO_ENTER_API_CALL(assert_ice_ready) \
     do { \