Make more. Followup to Bug 908695.

Comment on attachment 8359877 [details] [diff] [review] SSRC stats Review of attachment 8359877 [details] [diff] [review]: ----------------------------------------------------------------- This is kind of the right idea, but the cast to down to WebRTC classes completely defeats the Conduit class hierarchy design. You need to refactor this so that PCImpl's calls look like "mp.Conduit()->GetRemoteSSRC(&remoteSsrc)". ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +232,5 @@ > CSFLogError(logTag, "%s Unable to initialize VoEVideoSync", __FUNCTION__); > return kMediaConduitSessionNotInited; > } > > + if( !(mPtrRTP = webrtc::VoERTP_RTCP::GetInterface(mVoiceEngine))) fix spacing: "if (!(..." @@ +234,5 @@ > } > > + if( !(mPtrRTP = webrtc::VoERTP_RTCP::GetInterface(mVoiceEngine))) > + { > + CSFLogError(logTag, "%s Unable to get audio RTCP interface ", __FUNCTION__); "RTP/RTCP interface" ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h @@ +207,5 @@ > MediaConduitErrorCode Init(WebrtcVideoConduit *other); > > int GetChannel() { return mChannel; } > webrtc::VideoEngine* GetVideoEngine() { return mVideoEngine; } > + int GetLocalSSRC(unsigned int* ssrc); bool? I don't thing we need to propagate ViE's 0/-1 response model up to our code. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +1968,5 @@ > + switch (mp.Conduit()->type()) { > + case MediaSessionConduit::VIDEO: { > + idstr = NS_LITERAL_STRING("video_"); > + WebrtcVideoConduit *conduit = > + static_cast<WebrtcVideoConduit*>(mp.Conduit()); We *really* shouldn't see Webrtc constructs here. The whole point of the class abstraction of the conduits is that we can have multiple, different implementations without having to retool all the code that uses them. What you really need to do is add virtual "Get{Local,Remote}SSRC" methods to MediaSessionConduit, and then plumb these down to Webrtc{Video,Audio}Conduit.

(In reply to Adam Roach [:abr] from comment #3) > This is kind of the right idea, but the cast to down to WebRTC classes > completely defeats the Conduit class hierarchy design. You need to refactor > this so that PCImpl's calls look like > "mp.Conduit()->GetRemoteSSRC(&remoteSsrc)". Will do. > ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp > @@ +232,5 @@ > > CSFLogError(logTag, "%s Unable to initialize VoEVideoSync", __FUNCTION__); > > return kMediaConduitSessionNotInited; > > } > > > > + if( !(mPtrRTP = webrtc::VoERTP_RTCP::GetInterface(mVoiceEngine))) > > fix spacing: "if (!(..." Right. Funny, I cut'n'pasted it from http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#222 > @@ +234,5 @@ > > } > > > > + if( !(mPtrRTP = webrtc::VoERTP_RTCP::GetInterface(mVoiceEngine))) > > + { > > + CSFLogError(logTag, "%s Unable to get audio RTCP interface ", __FUNCTION__); > > "RTP/RTCP interface" Want me to touch http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#224 as well? > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h > @@ +207,5 @@ > > MediaConduitErrorCode Init(WebrtcVideoConduit *other); > > > > int GetChannel() { return mChannel; } > > webrtc::VideoEngine* GetVideoEngine() { return mVideoEngine; } > > + int GetLocalSSRC(unsigned int* ssrc); > > bool? I don't thing we need to propagate ViE's 0/-1 response model up to our > code. Thank goodness. > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp > @@ +1968,5 @@ > > + switch (mp.Conduit()->type()) { > > + case MediaSessionConduit::VIDEO: { > > + idstr = NS_LITERAL_STRING("video_"); > > + WebrtcVideoConduit *conduit = > > + static_cast<WebrtcVideoConduit*>(mp.Conduit()); > > We *really* shouldn't see Webrtc constructs here. The whole point of the > class abstraction of the conduits is that we can have multiple, different > implementations without having to retool all the code that uses them. > > What you really need to do is add virtual "Get{Local,Remote}SSRC" methods to > MediaSessionConduit, and then plumb these down to Webrtc{Video,Audio}Conduit. I'll do that.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #4) > Want me to touch > http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/ > media-conduit/VideoConduit.cpp#224 as well? Since you'll be opening that file as well, it's probably worthwhile.

Feedback incorporated.

Displays jitter.

Comment on attachment 8360051 [details] [diff] [review] SSRC stats (2) Review of attachment 8360051 [details] [diff] [review]: ----------------------------------------------------------------- Thanks; much cleaner. r+ conditional on fixing the itoa potential collision. ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +232,5 @@ > CSFLogError(logTag, "%s Unable to initialize VoEVideoSync", __FUNCTION__); > return kMediaConduitSessionNotInited; > } > > + if(!(mPtrRTP = webrtc::VoERTP_RTCP::GetInterface(mVoiceEngine))) Space between "if" and parenthesis. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +1944,5 @@ > return NS_OK; > } > > #ifdef MOZILLA_INTERNAL_API > +static nsString itoa(unsigned int i) { I'm afraid that this will collide with the platform-provided "itoa" in some build environments. If you really think there's value in extracting this into its own function, please name it something less likely to cause conflicts.

Fixed nits. Carrying forward r+ from Adam. Try: https://tbpl.mozilla.org/?tree=Try&rev=6f4f1973f9c8

Incorporated verbal feedback from reviewer. - Refactored ontop of ssrc patch - Now returns correct jitter for audio (received rather than sent) - Renamed new methods with more appropriate names.

I think I've uncovered a bug: a new specstats.html is needed to get audio jitter reports. It appears we don't receive RTCP sender reports (w/audio jitter) unless a stream is added to BOTH peerConnections involved. I've filed Bug 960177 for this.

Hmm, not sure why it takes a second for audio jitter reports to appear when video jitter appears instantaneously. Seems suspicious.

Comment on attachment 8360530 [details] [diff] [review] Jitter stat (2) Review of attachment 8360530 [details] [diff] [review]: ----------------------------------------------------------------- Looks good except where I confused you (sorry!). I really wish the ViERTP_RTCP and VoERTP_RTCP interfaces didn't have capricious differences between them. Sigh. ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +153,5 @@ > + unsigned int timestamp; > + unsigned int playoutTimestamp; > + unsigned short fractionLost; > + > + return !mPtrRTP->GetRemoteRTCPData(mChannel, After digging into the API a bit more, I think I may have led you astray in our IRC conversation. I think your original formulation was actually correct, in that GetRTPStatistics() reports the jitter that we have calculated based on the timing of RTP packets arriving.

Reverted to use GetRTPStatistics() as noted.

Comment on attachment 8360664 [details] [diff] [review] Jitter stat (3) Review of attachment 8360664 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa6159af7e5 https://hg.mozilla.org/integration/mozilla-inbound/rev/abe44fd348b6

https://hg.mozilla.org/mozilla-central/rev/6fa6159af7e5 https://hg.mozilla.org/mozilla-central/rev/abe44fd348b6

Realized I was missing RTCP stats. Only for inbound audio at the moment.

A bit last minute, but I'll take the first review that lands me on 29. ;-) - Includes remoteId cross-linked RTCP information in getStats, per spec. - about:webrtc: now groups RTP + RTCP stats together. Try: https://tbpl.mozilla.org/?tree=Try&rev=c5408e448688

Removed commented-out code I left by accident.

Fixed mochitest/pc.js to not count remote inbound/outbound stats. Try: https://tbpl.mozilla.org/?tree=Try&rev=f7e158507e4c

Comment on attachment 8362339 [details] [diff] [review] RTCP stats in getStats and on the about:webrtc page (3) Review of attachment 8362339 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nit fix ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +178,5 @@ > + *timestamp = NTPtoDOMHighResTimeStamp(ntpHigh, ntpLow); > + *packetsReceived = (packetsSent >= cumulativeLost) ? > + (packetsSent - cumulativeLost) : 0; > + *bytesReceived = packetsSent ? > + (bytesSent32 / packetsSent * (*packetsReceived)) : 0; Will this will blow up if *packetsReceived is 0? Depends on operator precedence.... Add parens instead of relying on order-of-operation ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +184,5 @@ > + *timestamp = NTPtoDOMHighResTimeStamp(ntpHigh, ntpLow); > + *packetsReceived = (packetsSent >= cumulativeLost) ? > + (packetsSent - cumulativeLost) : 0; > + *bytesReceived = packetsSent ? > + (bytesSent32 / packetsSent * (*packetsReceived)) : 0; Ditto ::: media/webrtc/trunk/webrtc/video_engine/include/vie_rtp_rtcp.h @@ +266,5 @@ > const int video_channel, > + unsigned int& ntpHigh, > + unsigned int& ntpLow, > + unsigned int& bytes_sent, > + unsigned int& packets_sent, Note: please create an bug for upstreaming these changes to media/webrtc/trunk/webrtc, and create a bug at webrtc.org, and link to the webrtc_updates bug. Also double-check that this hasn't been changed in webrtc.org trunk

Updated with nits. Taking r+ from jesup. (In reply to Randell Jesup [:jesup] from comment #24) > > + *bytesReceived = packetsSent ? > > + (bytesSent32 / packetsSent * (*packetsReceived)) : 0; > > Will this will blow up if *packetsReceived is 0? Depends on operator > precedence.... Add parens instead of relying on order-of-operation It wont blow up, but I've added parens as suggested. > ::: media/webrtc/trunk/webrtc/video_engine/include/vie_rtp_rtcp.h > @@ +266,5 @@ > > const int video_channel, > > + unsigned int& ntpHigh, > > + unsigned int& ntpLow, > > + unsigned int& bytes_sent, > > + unsigned int& packets_sent, > > Note: please create an bug for upstreaming these changes to > media/webrtc/trunk/webrtc, and create a bug at webrtc.org, and link to the > webrtc_updates bug. Also double-check that this hasn't been changed in > webrtc.org trunk I should have checked there first. Turns out this has already been addressed upstream. https://code.google.com/p/webrtc/issues/detail?id=2589 Their code revamps the GetSent/ReceivedRtcpStatistics API beyond what I did, lifting out a new RtcpStatistics struct: https://code.google.com/p/webrtc/source/browse/trunk/webrtc/video_engine/vie_rtp_rtcp_impl.h?spec=svn5142&r=5142#92 We can still land and fix this next time we merge, or we can land and I can file a followup bug here to mirror the upstream API, or I can look into what it would take to mirror it more closely now, or I can hold off. Let me know your preference.

Wrong bug# in commit msg. Carrying forward r+ from jesup.

Try - https://tbpl.mozilla.org/?tree=Try&rev=25f9a71bba35

Let's land. Remind me to back you out locally before merging and you can prepare an updated patch.

OK will do.

For discussion, please ignore.