From 5f61ae17ec82d288a3fe4892ec999c0e20c486c0 Mon Sep 17 00:00:00 2001 From: "Byron Campen [:bwc]" Date: Mon, 6 Apr 2015 11:52:28 -0700 Subject: [PATCH] Bug 1151139 - Simplify how we choose which streams to gather stats from. r=mt, a=abillings --- ...t_peerConnection_offerRequiresReceiveAudio.html | 2 + ...t_peerConnection_offerRequiresReceiveVideo.html | 2 + ...rConnection_offerRequiresReceiveVideoAudio.html | 2 + media/mtransport/nricectx.h | 13 +++++ media/mtransport/nricemediastream.cpp | 1 + media/mtransport/nricemediastream.h | 5 +- .../src/peerconnection/PeerConnectionImpl.cpp | 66 ++++++++++------------ .../src/peerconnection/PeerConnectionImpl.h | 2 +- 8 files changed, 54 insertions(+), 39 deletions(-) diff --git a/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveAudio.html b/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveAudio.html index 69d7e49..d68c078 100644 --- a/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveAudio.html +++ b/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveAudio.html @@ -17,6 +17,8 @@ runTest(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(); }); diff --git a/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideo.html b/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideo.html index 5f1d0e5..0ecb0b7 100644 --- a/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideo.html +++ b/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideo.html @@ -17,6 +17,8 @@ runTest(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(); }); diff --git a/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideoAudio.html b/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideoAudio.html index c3dea10..78eb0d4 100644 --- a/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideoAudio.html +++ b/dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideoAudio.html @@ -17,6 +17,8 @@ runTest(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 diff --git a/media/mtransport/nricectx.h b/media/mtransport/nricectx.h index d1209a7..7350666 100644 --- a/media/mtransport/nricectx.h +++ b/media/mtransport/nricectx.h @@ -196,6 +196,19 @@ class NrIceCtx { RefPtr CreateStream(const std::string& name, int components); + RefPtr 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_; } diff --git a/media/mtransport/nricemediastream.cpp b/media/mtransport/nricemediastream.cpp index 9e96cb5..d2b6429 100644 --- a/media/mtransport/nricemediastream.cpp +++ b/media/mtransport/nricemediastream.cpp @@ -209,6 +209,7 @@ nsresult NrIceMediaStream::ParseAttributes(std::vector& return NS_ERROR_FAILURE; } + has_parsed_attrs_ = true; return NS_OK; } diff --git a/media/mtransport/nricemediastream.h b/media/mtransport/nricemediastream.h index aba5fc3..2494ecf 100644 --- a/media/mtransport/nricemediastream.h +++ b/media/mtransport/nricemediastream.h @@ -149,6 +149,7 @@ class NrIceMediaStream { // Parse remote attributes nsresult ParseAttributes(std::vector& candidates); + bool HasParsedAttributes() const { return has_parsed_attrs_; } // Parse trickle ICE candidate nsresult ParseTrickleCandidate(const std::string& candidate); @@ -204,7 +205,8 @@ class NrIceMediaStream { name_(name), components_(components), stream_(nullptr), - opaque_(nullptr) {} + opaque_(nullptr), + has_parsed_attrs_(false) {} DISALLOW_COPY_ASSIGN(NrIceMediaStream); @@ -214,6 +216,7 @@ class NrIceMediaStream { const int components_; nr_ice_media_stream *stream_; ScopedDeletePtr opaque_; + bool has_parsed_attrs_; }; diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp index ebcc17d..c70e3e4 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ -149,7 +149,8 @@ PRLogModuleInfo *signalingLogInfo() { namespace sipcc { #ifdef MOZILLA_INTERNAL_API -RTCStatsQuery::RTCStatsQuery(bool internal) : internalStats(internal) { +RTCStatsQuery::RTCStatsQuery(bool internal) : internalStats(internal), + grabAllLevels(false) { } RTCStatsQuery::~RTCStatsQuery() { @@ -2037,32 +2038,8 @@ PeerConnectionImpl::BuildStatsQuery_m( query->iceCtx = mMedia->ice_ctx(); - // From the list of MediaPipelines, determine the set of NrIceMediaStreams - // we are interested in. - std::set levelsToGrab; - if (trackId) { - for (size_t p = 0; p < query->pipelines.Length(); ++p) { - size_t level = query->pipelines[p]->level(); - MOZ_ASSERT(level); - levelsToGrab.insert(level); - } - } else { - // We want to grab all streams, so ignore the pipelines (this also ends up - // grabbing DataChannel streams, which is what we want) - for (size_t s = 0; s < mMedia->num_ice_media_streams(); ++s) { - levelsToGrab.insert(s + 1); // mIceStreams is 0-indexed - } - } - - for (auto s = levelsToGrab.begin(); s != levelsToGrab.end(); ++s) { - // TODO(bcampen@mozilla.com): I may need to revisit this for bundle. - // (Bug 786234) - RefPtr temp(mMedia->ice_media_stream(*s - 1)); - RefPtr flow(mMedia->GetTransportFlow(*s, false)); - // flow can be null for unused levels, such as unused DataChannels - if (temp && flow) { - query->streams.AppendElement(temp); - } + if (!trackId) { + query->grabAllLevels = true; } return rv; @@ -2103,6 +2080,9 @@ static void RecordIceStats_s( bool internalStats, DOMHighResTimeStamp now, RTCStatsReportInternal* report) { + if (!mediaStream.HasParsedAttributes()) { + return; + } NS_ConvertASCIItoUTF16 componentId(mediaStream.name().c_str()); if (internalStats) { @@ -2292,20 +2272,32 @@ PeerConnectionImpl::ExecuteStatsQuery_s(RTCStatsQuery *query) { 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 and NrIceMediaStream must be destroyed on STS, so it is not safe - // to dispatch them back to main. - // We clear streams first to maintain destruction order - query->streams.Clear(); + // NrIceCtx must be destroyed on STS, so it is not safe + // to dispatch it back to main. query->iceCtx = nullptr; return NS_OK; } diff --git a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h index 847085c..497230a 100644 --- a/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h +++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h @@ -174,7 +174,7 @@ class RTCStatsQuery { bool internalStats; nsTArray> pipelines; mozilla::RefPtr iceCtx; - nsTArray> streams; + bool grabAllLevels; DOMHighResTimeStamp now; }; #endif // MOZILLA_INTERNAL_API -- 2.2.1