Once bug 902003 gives us a basic getStats skeleton (which as of this writing should include basic packetsSent/Received, bytesSent/Received stats and little else), we should add more stats. We should compile a list of most desirable stats in this bug. In lieu of that list, a good starting point may be here: http://lists.w3.org/Archives/Public/public-webrtc/2013May/0053.html

(In reply to Jan-Ivar Bruaroey [:jib] from comment #0) > Once bug 902003 gives us a basic getStats skeleton (which as of this writing > should include basic packetsSent/Received, bytesSent/Received stats and > little else), we should add more stats. Slight change, patch for packets/bytes-sent/received now goes here. > We should compile a list of most desirable stats in this bug. New best reference is here http://www.w3.org/2011/04/webrtc/wiki/Stats

Not a complete set of RTP stats, but a start. Still missing SSRC, jitter etc. Try - https://tbpl.mozilla.org/?tree=Try&rev=20ec7249f958

Try - https://tbpl.mozilla.org/?tree=Try&rev=20ec7249f958

Unbitrot.

Comment on attachment 8344167 [details] [diff] [review] Packets/Bytes-sent/received stats (2) Review of attachment 8344167 [details] [diff] [review]: ----------------------------------------------------------------- r+, but we should really consider moving those stats off of int to something more definite. really should be uint64_t - negative values are not sensical. That can be a followup bug, however, though feel free to change it now if there are no outside impacts. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h @@ +139,1 @@ > int rtcp_packets_received() const { return rtcp_packets_received_; } We should consider making these something other than 'int' ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +497,5 @@ > + break; > + } > + case MediaPipeline::RECEIVE: { > + RTCInboundRTPStreamStats s; > + s.mTimestamp.Construct(now); hoist the common code out of the switch (s, Construct(now), etc) @@ +503,5 @@ > + s.mType.Construct(RTCStatsType::Inboundrtp); > + s.mSsrc.Construct(NS_LITERAL_STRING("123457")); > + s.mPacketsReceived.Construct(mp.rtp_packets_received()); > + s.mBytesReceived.Construct(mp.rtp_bytes_received()); > + inbound->AppendElement(s); Move this to after the switch

(In reply to Randell Jesup [:jesup] from comment #6) > r+, but we should really consider moving those stats off of int to something > more definite. really should be uint64_t - negative values are not > sensical. Thanks for catching that. > ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h > @@ +139,1 @@ > > int rtcp_packets_received() const { return rtcp_packets_received_; } > > We should consider making these something other than 'int' Yes, I'll make packet count be uint32_t and byte count uint64_t like http://www.w3.org/2011/04/webrtc/wiki/Stats says. > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp > @@ +497,5 @@ > > + break; > > + } > > + case MediaPipeline::RECEIVE: { > > + RTCInboundRTPStreamStats s; > > + s.mTimestamp.Construct(now); > > hoist the common code out of the switch (s, Construct(now), etc) I tried, but since the types are case-specific, it was messy (? operator), and less robust: A common accident in near-duplicate code is mixing things like received vs. sent, and the way it is now the compiler catches this (no way to use wrong s by accident). I prefer that. > @@ +503,5 @@ > > + s.mType.Construct(RTCStatsType::Inboundrtp); > > + s.mSsrc.Construct(NS_LITERAL_STRING("123457")); > > + s.mPacketsReceived.Construct(mp.rtp_packets_received()); > > + s.mBytesReceived.Construct(mp.rtp_bytes_received()); > > + inbound->AppendElement(s); > > Move this to after the switch The target array (inbound vs. outbound) is case specific as well.

Hmm, I also broke the mediapipeline_unittest https://tbpl.mozilla.org/?tree=Try&rev=20ec7249f958 It's not immediately obvious to me, so I need to look at it. [==========] Running 2 tests from 1 test case. [----------] Global test environment set-up. [----------] 2 tests from MediaPipelineTest [ RUN ] MediaPipelineTest.TestAudioSendNoMux WARNING: no real random source present! /Users/Jan/moz/mozilla-central/media/webrtc/signaling/test/mediapipeline_unittest.cpp:328: Failure Expected: (p2_.GetAudioRtpCount()) >= (40), actual: 0 vs 40 Assertion failure: !stream_, at /Users/Jan/moz/mozilla-central/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:53 Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000 mozilla::MediaPipeline::~MediaPipeline (this=0x11116c900) at MediaPipeline.cpp:53 53 MOZ_ASSERT(!stream_); // Check that we have shut down already. (gdb) bt #0 mozilla::MediaPipeline::~MediaPipeline at MediaPipeline.cpp:53 #1 mozilla::MediaPipelineReceive::~MediaPipelineReceive at MediaPipeline.h:432 #2 mozilla::MediaPipelineReceiveAudio::~MediaPipelineReceiveAudio at MediaPipeline.h:460 #3 mozilla::MediaPipelineReceiveAudio::~MediaPipelineReceiveAudio at MediaPipeline.h:460 #4 mozilla::MediaPipelineReceiveAudio::~MediaPipelineReceiveAudio at MediaPipeline.h:460 #5 mozilla::MediaPipeline::Release at MediaPipeline.h:144 #6 mozilla::RefPtr<mozilla::MediaPipeline>::unref at RefPtr.h:203 #7 mozilla::RefPtr<mozilla::MediaPipeline>::~RefPtr at RefPtr.h:153 #8 mozilla::RefPtr<mozilla::MediaPipeline>::~RefPtr at RefPtr.h:153 #9 (anonymous namespace)::TestAgent::~TestAgent at media/webrtc/signaling/test/mediapipeline_unittest.cpp:105

It seems intermittent, and I can't understand how my changes could cause this to happen. If someone who knows the MediaPipeline code better can take a look I would really appreciate it!

The crash appears to be that if the ASSERT_GE() fails, it exits without calling p1_.Stop()/p2_.Stop(), and there is a shutdown issue it appears. Fancier setups where the destructors guarantee calling shutdown functions would sidestep this I thik The real problem appears to be that rtp_packets_received_ is 0 - and this patch does touch that code.

The cause appears to be failure to Unprotect() the RTP packets. You moved the "received" bumping to after Unprotect(), and we never get there. I popped this patch off, and tested in GDB without it, and it also won't Unprotect() the data. And if I move the increment of packets received to after Unprotect() (i.e. it doesn't get called, and the ASSERT_GE() fails) we get teh same shutdown crash. Please file a bug against the unit test for the Unprotect() problem, assign to ekr (he can reassign if he wishes), ditto one that if the test fails it crashes on exit. Add a disabling of the test for received packets in the pipeline test. (comment out the ASSERT_GE()). When the problem in the test is resolved, it can be re-enabled. You have r+ from me to disable the ASSERT_GE(), since it wasn't actually receiving packets successfully in the test

- Rectified side-effect that caused mediapipeline_unittest crash. - Made media-pipeline packet counter 32-bit and bytes counter 64-bit as suggested. I didn't dare switch the MediaPipeline code from signed to unsigned for fear of introducing bugs (IMHO unsigned is just a source of bugs anyhow these 64-bit days). Try - https://tbpl.mozilla.org/?tree=Try&rev=2ad1ee2c1976

If you could give it another look-over that would be great. Thanks for pointing me in the right direction.

(In reply to Randell Jesup [:jesup] from comment #11) > The cause appears to be failure to Unprotect() the RTP packets. You moved > the "received" bumping to after Unprotect(), and we never get there. Sorry I didn't see this comment before I posted. I found the same thing and I believe I've solved it by moving it back close to where it was before. > Please file a bug against the unit test for the Unprotect() problem, assign > to ekr (he can reassign if he wishes), ditto one that if the test fails it > crashes on exit. > > Add a disabling of the test for received packets in the pipeline test. > (comment out the ASSERT_GE()). When the problem in the test is resolved, it > can be re-enabled. You have r+ from me to disable the ASSERT_GE(), since it > wasn't actually receiving packets successfully in the test Please take a look at the patch and let me know if you think this is still necessary.

Comment on attachment 8344245 [details] [diff] [review] Packets/Bytes-sent/received stats (3) Review of attachment 8344245 [details] [diff] [review]: ----------------------------------------------------------------- r+ if you also disable the packets-received test in mediapipeline_unittests.cpp (per previous comment) Also file the followup bugs (doesn't have to be today), and a followup to convert to unsigned (it's still the right thing to do, but no hurry).

(In reply to Jan-Ivar Bruaroey [:jib] from comment #14) > (In reply to Randell Jesup [:jesup] from comment #11) > > The cause appears to be failure to Unprotect() the RTP packets. You moved > > the "received" bumping to after Unprotect(), and we never get there. > > Sorry I didn't see this comment before I posted. I found the same thing and > I believe I've solved it by moving it back close to where it was before. yes that should work (as in not fail), though it simply side-steps the failure in the unit tests. The test *should* fail since it didn't receive packets it could decrypt - receiving unusable packets is not success. I'd prefer to explicitly disable the test that isn't actually testing what it thinks it is, until it's fixed. Please disable the test, and move the call back to where you had it (after you test the result of Unprotect()).

I see what the problem is here: TransportLayerPrsock is coalescing the packets...

Done. I filed Bug 947663 for the unittest. Carrying forward r+ from jesup.

https://hg.mozilla.org/mozilla-central/rev/2b1230494e0b https://hg.mozilla.org/mozilla-central/rev/7b0b527637dd

Reading mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h#221 I realized all the rtp stats (currently) gathered are safe to read off main, so I moved gathering for rtp to main. Seemed simpler for now than prolonging life-cycles safely. We can always move it back. Please re-open this issue only if you decide to take this patch. Thanks. Try - https://tbpl.mozilla.org/?tree=Try&rev=82b2ddcca9cc

Oh and GetPipelineStats never fails. http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#474

Comment on attachment 8344502 [details] [diff] [review] Move gathering of RTP stats to main thread Review of attachment 8344502 [details] [diff] [review]: ----------------------------------------------------------------- I assume the code only calls GetPipelineStats on MainThread (from what I can see) ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +481,1 @@ > for (std::map<int, RefPtr<MediaPipeline> >::iterator it = mPipelines.begin(); This is safe from MainThread only, since mPipelines is modified on mainthread only. Please add a MainThread assert in place of the STS assert. Note that mPipelines is accessed (but not modified) on STS in shutdown (only).

Will do. Thanks. After speaking with Byron, I'll modify this to shove the pipelines into a vector and move the gathering back to STS, so he can build on top of it.

Try - https://tbpl.mozilla.org/?tree=Try&rev=3a11490f0072

Removed an unneeded typedef.

Comment on attachment 8345144 [details] [diff] [review] Collect pipelines on main, dispatch to STS for stats (2) Review of attachment 8345144 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +1256,2 @@ > internalStats, > + pipelines, How do you avoid the references you passed to STS being released there if timing is just wrong? (I.e. if the mainthread releases all other references to the pipeline while STS has a ref, then STS releases?) Two options: send the pipelines back to mainthread to drop the ref, or make sure pipelines can be safely released on any thread. (if it's already safe to release them on STS, great! If so, we may be very close to r+ depending on the other comment.) Releasing from any thread (if some parts need to be mainthread-released) can be done with a outer threadsafe object and an inner mainthread object, and if the outer is destroyed on a thread other than main, it ejects the inner in a runnable to MainThread to die (if on mainthread, it just kills the inner). The former solution is simpler generally (much). @@ +1869,5 @@ > + // TODO: Get SSRC > + // int channel = mp.Conduit()->GetChannel(); > + s.mSsrc.Construct(NS_LITERAL_STRING("123457")); > + s.mPacketsSent.Construct(mp.rtp_packets_sent()); > + s.mBytesSent.Construct(mp.rtp_bytes_sent()); Which thread can rtp_*_sent/received be safely touched? Is it STS (if so, good)? Are they the same? Or do you need to lock? Or (if they're mainthread, which they very likely aren't) you could gather first and then send to STS instead of the pipeline pointer. Likely receive is STS, and send might be STS (probably, and good) or the GIPS thread that calls SendPacket (though it could likely be moved to STS if that's the case. Please annotate the .h files with which threads they're safe to access from. Perhaps we should also add a thread assert for accessing the values.

Comment on attachment 8345144 [details] [diff] [review] Collect pipelines on main, dispatch to STS for stats (2) Review of attachment 8345144 [details] [diff] [review]: ----------------------------------------------------------------- This is roughly what we should be doing. As for the lifecycle of MediaPipeline, it looks like we run some cleanup code on main (namely ShutdownMedia_m), then some cleanup code on STS (namely, ShutdownTransport_s) before finally destroying back on the main thread. As long as we do a sanity-check on main before we call GetStats_s, I'm pretty sure that the back-and-forth GetStats stuff will finish before any back-and-forth teardown stuff finishes. We could future-proof by throwing in a ref pointer on the return dispatch, though. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +1214,5 @@ > +PushBackSelect(std::vector<RefPtr<MediaPipeline>>& aDst, > + const std::map<TrackID, RefPtr<mozilla::MediaPipeline>> & aSrc, > + TrackID aKey = 0) { > + auto it1 = aKey ? aSrc.find(aKey) : aSrc.begin(); > + auto it2 = aKey ? it1 : aSrc.end(); For readability, suggest it1 => begin and it2 => end. @@ +1215,5 @@ > + const std::map<TrackID, RefPtr<mozilla::MediaPipeline>> & aSrc, > + TrackID aKey = 0) { > + auto it1 = aKey ? aSrc.find(aKey) : aSrc.begin(); > + auto it2 = aKey ? it1 : aSrc.end(); > + if (it1 != aSrc.end()) { This only happens when it2 is also at the end, so the for loop should have us covered.

(In reply to Randell Jesup [:jesup] from comment #27) > Comment on attachment 8345144 [details] [diff] [review] > Collect pipelines on main, dispatch to STS for stats (2) > > Review of attachment 8345144 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp > @@ +1256,2 @@ > > internalStats, > > + pipelines, > > How do you avoid the references you passed to STS being released there if > timing is just wrong? (I.e. if the mainthread releases all other references > to the pipeline while STS has a ref, then STS releases?) > > Two options: send the pipelines back to mainthread to drop the ref, or make > sure pipelines can be safely released on any thread. (if it's already safe > to release them on STS, great! If so, we may be very close to r+ depending > on the other comment.) I see no ASSERT_ON_THREAD in ~MediaPipeline(). It seems to hit on main-thread from PeerConnectionMedia::SelfDestruct_m(), though lots of threads already touch MediaPipeline. > Releasing from any thread (if some parts need to be mainthread-released) can > be done with a outer threadsafe object and an inner mainthread object, and > if the outer is destroyed on a thread other than main, it ejects the inner > in a runnable to MainThread to die (if on mainthread, it just kills the > inner). Is there a rule for what needs to be mainthread-released? With our threading model wouldn't it be wise to ASSERT_ON_THREAD all of these cases? > The former solution is simpler generally (much). Yes that may be simpler than figuring out whether it is ok now. Though that potentially keeps pipelines alive even longer, and is the return dispatch guaranteed to make it back to main? When is the STS queue torn down? It might be better to piggyback on ShutdownMediaTransport_s(). > @@ +1869,5 @@ > > + // TODO: Get SSRC > > + // int channel = mp.Conduit()->GetChannel(); > > + s.mSsrc.Construct(NS_LITERAL_STRING("123457")); > > + s.mPacketsSent.Construct(mp.rtp_packets_sent()); > > + s.mBytesSent.Construct(mp.rtp_bytes_sent()); > > Which thread can rtp_*_sent/received be safely touched? Is it STS (if so, > good)? Are they the same? Or do you need to lock? Or (if they're > mainthread, which they very likely aren't) you could gather first and then > send to STS instead of the pipeline pointer. My last patch read them on main, which the MediaPipeline threading-cheatsheet http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.h#34 said was OK, but without a mutex this is dubious, and may not be true for all stats ever. So Byron and I agreed STS access is best. > Please annotate the .h files with which threads they're safe to access from. > Perhaps we should also add a thread assert for accessing the values.

- Returns pipelines back to main to be freed. - Fixed nits and iterator mistake. - Added ASSERT_ON_THREAD(main_thread_) to ~MediaPipeline() Try - https://tbpl.mozilla.org/?tree=Try&rev=0ee697f996b1

Comment on attachment 8345606 [details] [diff] [review] Collect pipelines on main, dispatch to STS for stats (3) Review of attachment 8345606 [details] [diff] [review]: ----------------------------------------------------------------- Seems about right. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +1217,5 @@ > + auto begin = aKey ? aSrc.find(aKey) : aSrc.begin(); > + for (auto it = begin, end = (aKey && begin != aSrc.end())? ++begin : aSrc.end(); > + it != end; ++it) { > + aDst.push_back(it->second); > + } This simple concept is frustratingly hard to express in a simple way. If it were me, I'd just do if/else here, since the actual work is just a push_back, but that's just taste.

(In reply to Byron Campen [:bwc] from comment #31) > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp > @@ +1217,5 @@ > > + auto begin = aKey ? aSrc.find(aKey) : aSrc.begin(); > > + for (auto it = begin, end = (aKey && begin != aSrc.end())? ++begin : aSrc.end(); > > + it != end; ++it) { > > + aDst.push_back(it->second); > > + } > > This simple concept is frustratingly hard to express in a simple way. If it > were me, I'd just do if/else here, since the actual work is just a > push_back, but that's just taste. I had something simpler in mind using std::next, but apparently, we don't support std::next. If-else looked complicated when I tried.

Thanks. The auto use in Comment 32 burned on Windows though: c:/builds/moz2_slave/try-w32-0000000000000000000000/build/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp(1218) : error C3538: in a declarator-list 'auto' must always deduce to the same type could be 'std::_Tree_const_iterator<_Mytree>' with [ _Mytree=std::_Tree_val<std::_Tmap_traits<mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>,std::less<base::Histogram::Sample>,std::allocator<std::pair<const mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>>>,false>> ] or 'std::_Tree_const_iterator<_Mytree>' with [ _Mytree=std::_Tree_val<std::_Tmap_traits<mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>,std::less<base::Histogram::Sample>,std::allocator<std::pair<const mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>>>,false>> ] I don't see a difference here, but hey. Will fix.

Compiles on Windows. Carrying forward r+ fron jesup. Try - https://tbpl.mozilla.org/?tree=Try&rev=00f625deb3d5

Only the second patch needs checking in. The first has landed already.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #33) > Thanks. The auto use in Comment 32 burned on Windows though: > c:/builds/moz2_slave/try-w32-0000000000000000000000/build/media/webrtc/ > signaling/src/peerconnection/PeerConnectionImpl.cpp(1218) : error C3538: in > a declarator-list 'auto' must always deduce to the same type > could be 'std::_Tree_const_iterator<_Mytree>' > with > [ > > _Mytree=std::_Tree_val<std::_Tmap_traits<mozilla::TrackID,mozilla:: > RefPtr<mozilla::MediaPipeline>,std::less<base::Histogram::Sample>,std:: > allocator<std::pair<const > mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>>>,false>> > ] > or 'std::_Tree_const_iterator<_Mytree>' > with > [ > > _Mytree=std::_Tree_val<std::_Tmap_traits<mozilla::TrackID,mozilla:: > RefPtr<mozilla::MediaPipeline>,std::less<base::Histogram::Sample>,std:: > allocator<std::pair<const > mozilla::TrackID,mozilla::RefPtr<mozilla::MediaPipeline>>>,false>> > ] > > I don't see a difference here, but hey. Will fix. A little late, but FYI this is a compiler bug, probably due to the ternary operator being used: http://connect.microsoft.com/VisualStudio/feedback/details/728741

Yeah I figured. I avoided it by moving one of them out of the declarator list.

Closing as fixed. See Bug 950855 for getStats mochitest.