We need to add the RTCRtpContributingSourceStats report block. We can start with the timestamp and contributorSsrc fields. RTCRtpContributingSourceStats has not quite settled in the spec yet, so this may be a bit of a moving target.

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133894/#review136792 ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:149 (Diff revision 1) > + ContributingSourceInformation(const uint32_t aCsrc, > + const DOMHighResTimeStamp aTime); > + ~ContributingSourceInformation() {}; > + // Creates a webidl representation suitable for adding to a report > + // @param aStreamId the associated RTCInboundRTPStreamStats.id > + nsAutoPtr<dom::RTCRTPContributingSourceStats> Zapped ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:156 (Diff revision 1) > + void SetTimestamp(const DOMHighResTimeStamp aTime) { mTimestamp = aTime; } > + bool Expired(const DOMHighResTimeStamp aExpiry) const { > + return mTimestamp < aExpiry; > + } > + private: > + // TODO @@NG find the source of this constant and double check it! **TODO** ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:165 (Diff revision 1) > + }; > + > + // Gets the gathered contributing source information for the last 10 seconds > + // @return the contributing source information or an empty vector > + // if there is none > + // Thread Safe, TODO @@NG is the mutex necessary or is it overkill? **TODO** ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2380 (Diff revision 1) > > +DOMHighResTimeStamp > +MediaPipeline::ContributingSourceInformation::GetExpiryFromTime( > + const DOMHighResTimeStamp aTime) { > + // DOMHighResTimeStamp is a unit measured in ms > + // TODO @@NG double check that it is a 10 second window **TODO** ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2388 (Diff revision 1) > + > +MediaPipeline::ContributingSourceInformation::ContributingSourceInformation( > + const uint32_t aCsrc, const DOMHighResTimeStamp aTime): > + mCsrc(aCsrc), mTimestamp(aTime) {} > + > +nsAutoPtr<dom::RTCRTPContributingSourceStats> Zapped

Had trouble finding the spec link. This is still a PR...

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review137058 Nice! r- for bad reverse for-loop though, and various questions + nits. ::: dom/media/RTCStatsReport.jsm:15 (Diff revision 1) > let report = {}; > + appendStats(dict.rtpContributingSourceStats, report); Any reason to put this first? It affects JS-observable enumeration order, which isn't specified in the spec anywhere AFAIK. But our existing order appears to somewhat resembles the order of the RTCStatsType enum [1] (except for codecStats but it's empty). So by that logic I'd expect rtpContributingSourceStats after outboundRTPStreamStats. [1] https://rawgit.com/taylor-b/webrtc-stats/0c695512c9894e9364d0ed95ddcc71dbf3d41953/webrtc-stats.html#dom-rtcstatstype ::: dom/webidl/RTCStatsReport.webidl:99 (Diff revision 1) > +dictionary RTCRTPContributingSourceStats : RTCStats { > + unsigned long contributorSsrc; // CSRC > + DOMString rtpStreamStatsId; // Corresponding RTCInboundRTPStreamStats s/rtpStreamStatsId/inboundRtpStreamId/ according to the PR [1] which says: dictionary RTCRTPContributingSourceStats : RTCStats { unsigned long contributorSsrc; DOMString inboundRtpStreamId; unsigned long packetsContributedTo; double audioLevel; }; Also packetsContributedTo seems essential to calculating the RTCStats.timestamp. Are we planning on leaving out audioLevel for now? Just checking. Lastly, in spite of existing practice, we should probably stop adding comments to the webidl, except to point out discrepancies from the spec. This is fairly well-defined stuff after all. [1] https://rawgit.com/taylor-b/webrtc-stats/0c695512c9894e9364d0ed95ddcc71dbf3d41953/webrtc-stats.html#dom-rtcrtpcontributingsourcestats ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:144 (Diff revision 1) > + ContributingSourceInformation(const uint32_t aCsrc, > + const DOMHighResTimeStamp aTime); Nit: indent second arg to line up with first. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:149 (Diff revision 1) > + ContributingSourceInformation(const uint32_t aCsrc, > + const DOMHighResTimeStamp aTime); > + ~ContributingSourceInformation() {}; > + // Creates a webidl representation suitable for adding to a report > + // @param aStreamId the associated RTCInboundRTPStreamStats.id > + nsAutoPtr<dom::RTCRTPContributingSourceStats> Avoid nsAutoPtr with its unsafe move semantics. Use UniquePtr and Move(). Note other comments may remove the need for this. Also, trailing space ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:157 (Diff revision 1) > + bool Expired(const DOMHighResTimeStamp aExpiry) const { > + return mTimestamp < aExpiry; > + } > + private: > + // TODO @@NG find the source of this constant and double check it! > + enum { EXPIRY_TIME_MILISECONDS = 10 * 1000 }; Why not static const? Two Ls in milliseconds. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:166 (Diff revision 1) > + nsAutoPtr<std::vector<ContributingSourceInformation>> > + GetContribtutingSourceInformation() const; UniquePtr. Also, Mozilla normally prefers nsTArray over vector [1], though I see there's a precedent for vector here, and in media/webrtc/signaling/* in general, so just fyi. [1] https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code#C_and_Mozilla_standard_libraries ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:308 (Diff revision 1) > int64_t rtp_bytes_sent_; > int64_t rtp_bytes_received_; > > std::vector<uint32_t> ssrcs_received_; > > + std::map<uint32_t, ContributingSourceInformation> mCsrcInfo; Naming style in this module suggests csrc_info_ Also, a comment on thread access seems prudent ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:318 (Diff revision 1) > > // Written on Init, all following accesses are on the STS thread. > nsAutoPtr<MediaPipelineFilter> filter_; > nsAutoPtr<webrtc::RtpHeaderParser> rtp_parser_; > > + mutable Mutex mCsrcInfoGuard; csrc_info_guard_ ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:765 (Diff revision 1) > +nsAutoPtr<std::vector<MediaPipeline::ContributingSourceInformation>> > +MediaPipeline::GetContribtutingSourceInformation() const { Or: auto MediaPipeline::GetContribtutingSourceInformation() const -> nsAutoPtr<std::vector<ContributingSourceInformation>> { ? (I may have gotten the location of const wrong) ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:769 (Diff revision 1) > + DOMHighResTimeStamp expiry = > + ContributingSourceInformation::GetExpiryFromTime( Nit: ContributingSourceInformation:: seems verbose here ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:769 (Diff revision 1) > + DOMHighResTimeStamp expiry = > + ContributingSourceInformation::GetExpiryFromTime( > + webrtc::Clock::GetRealTimeClock()->TimeInMilliseconds()); > + { > + MutexAutoLock lock(mCsrcInfoGuard); > + for (auto info: mCsrcInfo) { > + if (!info.second.Expired(expiry)) { > + infos->push_back(info.second); Would this be simpler to reason about if we didn't hide the expiry logic inside the GetExpiryTime() and Expired()? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:774 (Diff revision 1) > + DOMHighResTimeStamp expiry = > + ContributingSourceInformation::GetExpiryFromTime( > + webrtc::Clock::GetRealTimeClock()->TimeInMilliseconds()); > + { > + MutexAutoLock lock(mCsrcInfoGuard); > + for (auto info: mCsrcInfo) { Nit: space before : ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1079 (Diff revision 1) > ssrcs_received_.end()) { > ssrcs_received_.push_back(header.ssrc); > } > > + { > + MutexAutoLock lock(mCsrcInfoGuard); Yeah a mutex here in RtpPacketReceived() looks expensive and out of place. I'd look at implementing packetsContributedTo first - where the spec seems to have the most direct advice [1] - and model it on how ssrcs_received_ and increment_rtp_packets_received() appear to be handled today. I'm not seeing any locks there. [1] https://rawgit.com/taylor-b/webrtc-stats/0c695512c9894e9364d0ed95ddcc71dbf3d41953/webrtc-stats.html#dom-rtcrtpcontributingsourcestats-packetscontributedto ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1086 (Diff revision 1) > + webrtc::Clock::GetRealTimeClock()->TimeInMilliseconds(); > + > + // Remove expired information > + if (!mCsrcInfo.empty()) { > + auto expiry = ContributingSourceInformation::GetExpiryFromTime(now); > + MOZ_ASSERT(expiry > 0); Why this assert? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1087 (Diff revision 1) > + std::vector<uint32_t> expired; > + for(auto p : mCsrcInfo) { > + if (p.second.Expired(expiry)) { > + expired.push_back(p.first); > + } > + } > + for(auto csrc : expired) { > + mCsrcInfo.erase(csrc); > + } Note, since c++11 erase() returns the next iterator, so you can erase in the same loop [1], instead of this construct. [1] http://stackoverflow.com/a/263958/918910 ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1088 (Diff revision 1) > + // Remove expired information > + if (!mCsrcInfo.empty()) { > + auto expiry = ContributingSourceInformation::GetExpiryFromTime(now); > + MOZ_ASSERT(expiry > 0); > + std::vector<uint32_t> expired; > + for(auto p : mCsrcInfo) { for space ( Applies throughout. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1100 (Diff revision 1) > + for(auto i = header.numCSRCs - 1; > + i >= 0 && i < webrtc::kRtpCsrcSize; --i) { header.numCSRCs is uint_8 which means i will ALWAYS be >= 0 ! Relying on 255 being larger than webrtc::kRtpCsrcSize seems like a poor way to break the loop. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2388 (Diff revision 1) > + > +MediaPipeline::ContributingSourceInformation::ContributingSourceInformation( > + const uint32_t aCsrc, const DOMHighResTimeStamp aTime): > + mCsrc(aCsrc), mTimestamp(aTime) {} > + > +nsAutoPtr<dom::RTCRTPContributingSourceStats> trailing space ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3799 (Diff revision 1) > + // Fill in Contributing Source statistics > + auto csrcInfos = mp.GetContribtutingSourceInformation(); > + for(auto csrcInfo: *csrcInfos) { GetContribtutingSourceInformation() seems to do an unnecessary copy. Why not check expiry in this outer loop instead? Also, we're on the STS thread here, so is the mutex even necessary? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3802 (Diff revision 1) > + query->report->mRtpContributingSourceStats.Value().AppendElement( > + *csrcInfo.CreateWebidlInstance(localId), fallible); Instead of creating RTCRTPContributingSourceStats on the heap temporarily, I think we should follow the pattern established here of creating it on the stack and passing it in to be filled out. E.g. RTCRTPContributingSourceStats s; csrcInfo.GetStats(s); query->report->mRtpContributingSourceStats.Value().AppendElement(s, fallible); Seems more efficient.

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review137058 > s/rtpStreamStatsId/inboundRtpStreamId/ according to the PR [1] which says: > > dictionary RTCRTPContributingSourceStats : RTCStats { > unsigned long contributorSsrc; > DOMString inboundRtpStreamId; > unsigned long packetsContributedTo; > double audioLevel; > }; > > Also packetsContributedTo seems essential to calculating the RTCStats.timestamp. > > Are we planning on leaving out audioLevel for now? Just checking. > > Lastly, in spite of existing practice, we should probably stop adding comments to the webidl, except to point out discrepancies from the spec. This is fairly well-defined stuff after all. > > [1] https://rawgit.com/taylor-b/webrtc-stats/0c695512c9894e9364d0ed95ddcc71dbf3d41953/webrtc-stats.html#dom-rtcrtpcontributingsourcestats Yes, in this patch we are intentionally leaving out audioLevel. As for comments in the webidl, sounds good. Though, because all we have is auto generated code for most stats structs (there is no hand crafted c++ construct that represents or wraps the structs), there isn't an obvious place for comments to land. I guess PeerConnectionImpl for now ... "Also packetsContributedTo seems essential to calculating the RTCStats.timestamp.", I don't think this is correct. It is just a counter of packets that have landed in the window. RTCStats.timestamp is set to the time in which the packet was received, which seems to match up with the spec: "... the time at which the information arrived at the local endpoint". > Would this be simpler to reason about if we didn't hide the expiry logic inside the GetExpiryTime() and Expired()? I will add an additional comment to make it clearer. > Note, since c++11 erase() returns the next iterator, so you can erase in the same loop [1], instead of this construct. > > [1] http://stackoverflow.com/a/263958/918910 Absolutely fanstastic! > Instead of creating RTCRTPContributingSourceStats on the heap temporarily, I think we should follow the pattern established here of creating it on the stack and passing it in to be filled out. E.g. > > RTCRTPContributingSourceStats s; > csrcInfo.GetStats(s); > query->report->mRtpContributingSourceStats.Value().AppendElement(s, fallible); > > Seems more efficient. I am not sure how much of difference that would make. Under the hood all of these stats are a bunch of Maybe\<Foo\>s, so their guts are all scattered on the heap.

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review137058 > Yes, in this patch we are intentionally leaving out audioLevel. > > As for comments in the webidl, sounds good. Though, because all we have is auto generated code for most stats structs (there is no hand crafted c++ construct that represents or wraps the structs), there isn't an obvious place for comments to land. I guess PeerConnectionImpl for now ... > > "Also packetsContributedTo seems essential to calculating the RTCStats.timestamp.", I don't think this is correct. It is just a counter of packets that have landed in the window. RTCStats.timestamp is set to the time in which the packet was received, which seems to match up with the spec: "... the time at which the information arrived at the local endpoint". I was referring to this part: "the most recent time an RTP packet the source contributed to was received AND counted by packetsContributedTo." - Does our logic avoid taking the timestamp from unrelated newer packets? > I am not sure how much of difference that would make. Under the hood all of these stats are a bunch of Maybe\<Foo\>s, so their guts are all scattered on the heap. getStats is apparently a hot path in performance runs (padenot found it), so I vote for not doing extraneous allocations if we can avoid it.

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review137058 > I was referring to this part: "the most recent time an RTP packet the source contributed to was received AND counted by packetsContributedTo." - Does our logic avoid taking the timestamp from unrelated newer packets? When a new packet comes in, it only updates the timestamp on CSCRCs that are found in the packet. > getStats is apparently a hot path in performance runs (padenot found it), so I vote for not doing extraneous allocations if we can avoid it. I did end up removing the allocation.

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review139040 Looks good. Just nits. Don't forget DOM review. ::: dom/webidl/RTCStatsReport.webidl:101 (Diff revisions 1 - 4) > sequence<DOMString> trackIds; // Note: stats object ids, not track.id > }; > > dictionary RTCRTPContributingSourceStats : RTCStats { > - unsigned long contributorSsrc; // CSRC > - DOMString rtpStreamStatsId; // Corresponding RTCInboundRTPStreamStats > + unsigned long contributorSsrc; > + DOMString rtpStreamStatsId; You forgot s/rtpStreamStatsId/inboundRtpStreamId/ ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:146 (Diff revisions 1 - 4) > // Creates a webidl representation suitable for adding to a report > + // @param aWebidlObj the webidl binding object to popluate > // @param aStreamId the associated RTCInboundRTPStreamStats.id > - nsAutoPtr<dom::RTCRTPContributingSourceStats> > - CreateWebidlInstance(const nsString &aStreamId) const; > + void > + InitializeWebidlInstance(dom::RTCRTPContributingSourceStats& aWebidlObj, > + const nsString &aStreamId) const; Nit: Comment is wrong now. s/Creates/Gets/ Also, this reads like a getter to me, like GetContributingSourceStats below. Maybe consistent names? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:159 (Diff revisions 1 - 4) > + // given expiration time. > bool Expired(const DOMHighResTimeStamp aExpiry) const { > return mTimestamp < aExpiry; > } > private: > - // TODO @@NG find the source of this constant and double check it! > + static const double constexpr EXPIRY_TIME_MILLISECONDS = 10 * 1000; constexpr is not needed. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:168 (Diff revisions 1 - 4) > - nsAutoPtr<std::vector<ContributingSourceInformation>> > - GetContribtutingSourceInformation() const; > + GetContributingSourceStats( > + FallibleTArray<dom::RTCRTPContributingSourceStats>& aArr, > + const nsString& aInboundStreamId) const; Nit: It's common for the out-param to come last, e.g. [1] [1] https://dxr.mozilla.org/mozilla-central/rev/a748acbebbde373a88868dc02910fb2bc5e6a023/accessible/base/nsEventShell.h#59-60 ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:79 (Diff revisions 1 - 4) > namespace mozilla { > extern mozilla::LogModule* AudioLogModule(); > > +static DOMHighResTimeStamp GetNow() { > + return webrtc::Clock::GetRealTimeClock()->TimeInMilliseconds(); > +} Beware of using generic-sounding names in the mozilla global namespace. Our unified builds compile all code inside a module as a single .cpp file for speed, which effectively obliterates file scope, causing high probability of name collisions, with the fun linker side-effects that can produce. You might want to consider putting it inside MediaPipeline:: I know, it stinks. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:775 (Diff revisions 1 - 4) > - nsAutoPtr<std::vector<ContributingSourceInformation>> infos( > - new std::vector<ContributingSourceInformation>()); > - DOMHighResTimeStamp expiry = > - ContributingSourceInformation::GetExpiryFromTime( > - webrtc::Clock::GetRealTimeClock()->TimeInMilliseconds()); > - { > + FallibleTArray<dom::RTCRTPContributingSourceStats>& aArr, > + const nsString& aInboundRtpStreamId) const > +{ > + // Get the expiry from now > + DOMHighResTimeStamp expiry = RtpCSRCStats::GetExpiryFromTime(GetNow()); > + for (auto info: csrc_stats_) { space before : ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1083 (Diff revisions 1 - 4) > + // Make sure to only get the time once, and only if we need it > + Maybe<DOMHighResTimeStamp> now; > + // Remove expired RtpCSRCStats > + if (!csrc_stats_.empty()) { > + auto expiry = RtpCSRCStats::GetExpiryFromTime(now.valueOrFrom(&GetNow)); You're not actually caching anything. now.valueOrFrom(&GetNow) calls GetNow() each time and returns the result; It never sets now. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2376 (Diff revisions 1 - 4) > -MediaPipeline::ContributingSourceInformation::ContributingSourceInformation( > +MediaPipeline::RtpCSRCStats::RtpCSRCStats( > const uint32_t aCsrc, const DOMHighResTimeStamp aTime): > mCsrc(aCsrc), mTimestamp(aTime) {} Please follow style in file and/or style guide [1] MediaPipeline::RtpCSRCStats::RtpCSRCStats(const uint32_t aCsrc, const DOMHighResTimeStamp aTime) : mCsrc(aCsrc) , mTimestamp(aTime) {} [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review137058 > I did end up removing the allocation. Good. Turns out Maybe<Foo> does not actually use the heap [1] [1] https://dxr.mozilla.org/mozilla-central/rev/a748acbebbde373a88868dc02910fb2bc5e6a023/mfbt/Maybe.h#53-58

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review139040 > constexpr is not needed. This is only true for integral and enum types [1] (yes really), and because it is a double, the constexpr is neeed. [1] http://en.cppreference.com/w/cpp/language/static section: "Constant static members" > Beware of using generic-sounding names in the mozilla global namespace. > > Our unified builds compile all code inside a module as a single .cpp file for speed, which effectively obliterates file scope, causing high probability of name collisions, with the fun linker side-effects that can produce. > > You might want to consider putting it inside MediaPipeline:: > I know, it stinks. Ugh.

DOM review notes: * Spec: https://rawgit.com/w3c/webrtc-stats/0c695512c9894e9364d0ed95ddcc71dbf3d41953/webrtc-stats.html#dom-rtcrtpcontributingsourcestats * Spec PR: https://github.com/w3c/webrtc-stats/pull/194 * not implementing packetsContributedTo or audioLevel at this time

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review140230 r+ for the .webidl

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review140122 I feel bad changing an r+ to r-... Can we add new patches instead for new code? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:124 (Diff revisions 4 - 5) > + // Used only for testing; adds RTP header extension for RTP Stream Id with > + // the given id. > + void AddRIDExtension_m(size_t extension_id); > + void AddRIDExtension_s(size_t extension_id); > + // Used only for testing; installs a MediaPipelineFilter that filters > + // everything but the given RID > + void AddRIDFilter_m(const std::string& rid); > + void AddRIDFilter_s(const std::string& rid); mozReview has lousy workflows for adding new code to already r+ed patches. Can we add this testing harness part in a separate patch along with test that uses it? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:143 (Diff revisions 4 - 5) > public: > + // Gets the current time as a DOMHighResTimeStamp > + static DOMHighResTimeStamp GetNow(); Can this be private to keep it close to file scope? ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:152 (Diff revisions 4 - 5) > // this reference time would normally be the time of calling. > // This value can then be used to check if a RtpCSRCStats > // has expired via Expired(...) > static DOMHighResTimeStamp > GetExpiryFromTime(const DOMHighResTimeStamp aTime); > - > + trailing space ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:176 (Diff revisions 4 - 5) > uint32_t mCsrc; > DOMHighResTimeStamp mTimestamp; > }; > > // Gets the gathered contributing source stats for the last expiration period. > + // @param aId the stream id to use for populating inboundRtpStreamId field trailing space ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:744 (Diff revisions 4 - 5) > + RUN_ON_THREAD(sts_thread_, > + WrapRunnable(this, I think we need RefPtr<MediaPipeline>(this) here to keep this alive. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:761 (Diff revisions 4 - 5) > + RUN_ON_THREAD(sts_thread_, > + WrapRunnable(this, I think we need RefPtr<MediaPipeline>(this) here to keep this alive. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:804 (Diff revisions 4 - 5) > - DOMHighResTimeStamp expiry = RtpCSRCStats::GetExpiryFromTime(GetNow()); > - for (auto info: csrc_stats_) { > + DOMHighResTimeStamp expiry = RtpCSRCStats::GetExpiryFromTime( > + RtpCSRCStats::GetNow()); Nit: Redundant RtpCSRCStats:: prefix ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1117 (Diff revisions 4 - 5) > + auto getTimestamp = [&now]() -> DOMHighResTimeStamp { > + if (!now) { > + now.emplace(RtpCSRCStats::GetNow()); > + } > + return now.value(); > + }; I find this overly complicated for just trying to avoid calling the same method twice, especially in the name of performance, as defining an interim functor doesn't seem like zero cost. I think we'd be better off solving this with a plain old boolean. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2352 (Diff revisions 4 - 5) > -nsresult > -PeerConnectionImpl::SelectSsrc(MediaStreamTrack& aRecvTrack, > +RefPtr<MediaPipeline> > +PeerConnectionImpl::GetMediaPipelineForTrack(MediaStreamTrack& aRecvTrack) This introduces an unnecessary refcount compared to the old version. Maybe return a raw pointer instead? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2374 (Diff revisions 4 - 5) > + > +nsresult > +PeerConnectionImpl::SelectSsrc(MediaStreamTrack& aRecvTrack, > + unsigned short aSsrcIndex) > +{ > + RefPtr<MediaPipeline> pipeline = GetMediaPipelineForTrack(aRecvTrack); Refcount seems unnecessary. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2385 (Diff revisions 4 - 5) > + > +nsresult > +PeerConnectionImpl::AddRIDExtension(MediaStreamTrack& aRecvTrack, > + unsigned short aExtensionId) > +{ > + RefPtr<MediaPipeline> pipeline = GetMediaPipelineForTrack(aRecvTrack); ditto ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2396 (Diff revisions 4 - 5) > + > +nsresult > +PeerConnectionImpl::AddRIDFilter(MediaStreamTrack& aRecvTrack, > + const nsAString& aRid) > +{ > + RefPtr<MediaPipeline> pipeline = GetMediaPipelineForTrack(aRecvTrack); ditto

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review140122 > mozReview has lousy workflows for adding new code to already r+ed patches. > > Can we add this testing harness part in a separate patch along with test that uses it? My mistake, not part of this patch, just rebase cruft. > I think we need > > RefPtr<MediaPipeline>(this) > > here to keep this alive. Not in this patch. > I think we need > > RefPtr<MediaPipeline>(this) > > here to keep this alive. Not in this patch. My mistake.

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review140122 > Refcount seems unnecessary. Wrong patch. > ditto Wrong patch.

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review140122 > This introduces an unnecessary refcount compared to the old version. Maybe return a raw pointer instead? wrong patch.

So basically most of that was a review mistake on my part. I ended up reviewing code caught up in mozreview's interdiff bug. My bad.

Comment on attachment 8861878 [details] Bug 1359775 - Part 1 - add RTCRtpContributingSourceStats; https://reviewboard.mozilla.org/r/133896/#review146202

Pushed by na-g@nostrum.com: https://hg.mozilla.org/integration/autoland/rev/aaf62f94cb34 Part 1 - add RTCRtpContributingSourceStats;r=jib,smaug