Closed Bug 947665 Opened 6 years ago Closed 6 years ago

Still more webRTC stats

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: jib, Assigned: jib)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 13 obsolete files)

11.08 KB, patch
jib
: review+
Details | Diff | Splinter Review
6.15 KB, patch
abr
: review+
Details | Diff | Splinter Review
14.69 KB, text/html
Details
48.03 KB, patch
jib
: review+
Details | Diff | Splinter Review
20.34 KB, text/html
Details
Make more. Followup to Bug 908695.
Attached patch SSRC stats (obsolete) — Splinter Review
Attachment #8359877 - Flags: review?(adam)
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.
Attachment #8359877 - Flags: review?(adam) → review-
(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.
Attached patch SSRC stats (2) (obsolete) — Splinter Review
Feedback incorporated.
Attachment #8359877 - Attachment is obsolete: true
Attachment #8360051 - Flags: review?(adam)
Attached patch Jitter stat (obsolete) — Splinter Review
Attachment #8360064 - Flags: review?(adam)
Attached file specstats.html - stats test page (2) (obsolete) —
Displays jitter.
Attachment #8359878 - Attachment is obsolete: true
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.
Attachment #8360051 - Flags: review?(adam) → review+
Fixed nits. Carrying forward r+ from Adam.

Try: https://tbpl.mozilla.org/?tree=Try&rev=6f4f1973f9c8
Attachment #8360051 - Attachment is obsolete: true
Attachment #8360412 - Flags: review+
Attachment #8360412 - Attachment description: SSRC stats (2) r=abr → SSRC stats (3) r=abr
Attached patch Jitter stat (2) (obsolete) — Splinter Review
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.
Attachment #8360064 - Attachment is obsolete: true
Attachment #8360064 - Flags: review?(adam)
Attachment #8360530 - Flags: review?(adam)
Attached file specstats.html - stats test page (3) (obsolete) —
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.
Attachment #8360065 - Attachment is obsolete: true
Hmm, not sure why it takes a second for audio jitter reports to appear when video jitter appears instantaneously. Seems suspicious.
Keywords: checkin-needed
Whiteboard: [leave-open]
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.
Attached patch Jitter stat (3)Splinter Review
Reverted to use GetRTPStatistics() as noted.
Attachment #8360530 - Attachment is obsolete: true
Attachment #8360530 - Flags: review?(adam)
Attachment #8360664 - Flags: review?(adam)
Comment on attachment 8360664 [details] [diff] [review]
Jitter stat (3)

Review of attachment 8360664 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks.
Attachment #8360664 - Flags: review?(adam) → review+
Attachment #8360065 - Attachment is obsolete: false
Attachment #8360538 - Attachment is obsolete: true
Attached patch RTCP stats (WIP) (obsolete) — Splinter Review
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
Attachment #8361869 - Attachment is obsolete: true
Attachment #8362331 - Flags: review?(rjesup)
Attachment #8362331 - Flags: review?(adam)
Attachment #8360065 - Attachment is obsolete: true
Whiteboard: [leave-open]
Removed commented-out code I left by accident.
Attachment #8362331 - Attachment is obsolete: true
Attachment #8362331 - Flags: review?(rjesup)
Attachment #8362331 - Flags: review?(adam)
Attachment #8362335 - Flags: review?(rjesup)
Attachment #8362335 - Flags: review?(adam)
Fixed mochitest/pc.js to not count remote inbound/outbound stats.

Try: https://tbpl.mozilla.org/?tree=Try&rev=f7e158507e4c
Attachment #8362335 - Attachment is obsolete: true
Attachment #8362335 - Flags: review?(rjesup)
Attachment #8362335 - Flags: review?(adam)
Attachment #8362339 - Flags: review?(rjesup)
Attachment #8362339 - Flags: review?(adam)
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
Attachment #8362339 - Flags: review?(rjesup) → review+
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.
Attachment #8362339 - Attachment is obsolete: true
Attachment #8362339 - Flags: review?(adam)
Attachment #8363039 - Flags: review+
Flags: needinfo?(rjesup)
Attachment #8363039 - Attachment description: RTCP stats in getStats and on the about:webrtc page (4) → RTCP stats in getStats and on the about:webrtc page (4) r=jesup
Wrong bug# in commit msg. Carrying forward r+ from jesup.
Attachment #8363039 - Attachment is obsolete: true
Attachment #8363114 - Flags: review+
Let's land.  Remind me to back you out locally before merging and you can prepare an updated patch.
Flags: needinfo?(rjesup)
OK will do.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b21f20e9f9e8
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
For discussion, please ignore.
Attachment #8397943 - Attachment description: chromestats.html - for comparison (not spec) → chromestats.html - modified to use spec-constraints
You need to log in before you can comment on or make changes to this bug.