GNOME Bugzilla – Bug 751444
RTP: Keep track of all estimated round trip times observed per each report block
Last modified: 2015-06-29 17:54:24 UTC
Created attachment 306004 [details] [review] enhance stats for RtpSources RTSession only stores the last round-trip-time calculated per RB so it is being continuously overwritten as long as a new ssrc is extracted from the GstRTCPPacket. Further more, rtt stats are stored in the proper RtpSource (the one wich we receive the report for), current implementation only stores it in the sender stats which is weird so that any RTCP packet might contain several RB belonging to different SSRCs. On the other hand, a new GstStructure is provided with the RTPSource stats that contains all observed SSRCs and their proper round-trip-time to them. These stats are helpful to provide internal information about WebRTC statistics such as defined in http://www.w3.org/TR/webrtc-stats/#idl-def-RTCOutboundRTPStreamStats
Review of attachment 306004 [details] [review]: ::: gst/rtpmanager/rtpsource.c @@ +1392,3 @@ + + g_hash_table_insert (src->stats.rtts, + GUINT_TO_POINTER (src_sender->ssrc), GUINT_TO_POINTER (A)); The ssrc should be removed from the stats once the ssrc is timed out or says BYE
I'll rework my patch to provide such functionality. Thanks for your feedback.
Created attachment 306081 [details] [review] enhance stats for RtpSources Also remove the ssrc from the stats once the ssrc is timed out or says BYE
Created attachment 306082 [details] [review] enhance stats for RtpSources This is the right one. I selected the wrong patch in my previous post.
Review of attachment 306082 [details] [review]: ::: gst/rtpmanager/rtpsource.c @@ +1362,3 @@ + curridx = src_sender->stats.curr_rr ^ 1; + curr = &src_sender->stats.rr[curridx]; You now still store the rtt in the sender source, not in the receiver source. Isn't the whole point of this to store the rtt in the receiver source (instead of the sender source as it is now), and additionally provide a list of all rtts in the sender source?
Ok, so the RB handling is completely weird! rtp_session_request_local_key_unit() assumes that it can get the rtt on the sender side from the media-receiver source. But rtp_session_process_rb() calls rtp_source_process_rb() with the media-sender source. So the media-receiver source on the sender side should never have a non-zero rtt. And rtp_source_create_stats() only looks at non-internal sources for adding the RB stats... so it will always add 0 there currently.
Actually this code already does the right thing, it's just confusing with all the different ssrcs and src variables there. rtp_session_process_rr() has "source" as the one that sent the RR, i.e. the media-receiver and passes that to rtp_session_process_rb(). So inside rtp_session_process_rb(), "source" is the media-receiver source. Inside there inside the loop, "ssrc" and "src" are the media-sender source (i.e. the one about which the RB is). rtp_source_process_rb() is then called with "source" (the media-receiver source), if "ssrc" and "src" are the internal source of this session (i.e. it's not some other media sender but really our session here). Similar in rtp_session_process_sr(), "source" is the sender of the SR. So all else being the same, on the receiver session the sender source will get updated with the SR's RBs but as we only collect stats of internal sources, nothing really will happen for this case. So what exactly are you trying to accomplish here with your patch? That on receiving a RR, the media-sender source will *also* get a list of all rtts? Or that on receiving a SR, the media-receiver source will get a list of all rtts of the other media-receivers?
Ok so I looked at the spec and what you need to do here is on the sender side to get the non-internal source, and get the stats from that. You get that for example with the on-ssrc-active signal (it's emitted whenever a non-internal source receives an RTCP packet).