Summary:	ASTERISK-10189: [patch] RTP statistics returned by ast_rtp_get_quality reflects the last RTCP packet not a call overall
Reporter:	Gaspar Zoltan (gasparz)	Labels:
Date Opened:	2007-08-29 06:22:43	Date Closed:	2008-06-05 11:33:28
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/NewFeature
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) audioqos-trunk.diff ( 1) audiortpqos-1_4_11.patch ( 2) audiortpqos-1.4.17.patch ( 3) chan_sip_c.diff ( 4) chan_sip.c ( 5) rtp_c.diff ( 6) rtp_h.diff ( 7) rtp.c ( 8) rtp.h ( 9) rtpqos-14-r119891.diff (10) rtpqos-trunk-r119891.diff
Description:	The result of the function ast_rtp_get_quality is the value set by the LAST RTCP frame. This value is inserted to the RTPAUDIOQOS channel variable and should be a some kind of quality metric of the call. So if we take only one sample: one (the last) RTCP frame we won't get a relevant value for a parameter. The min/max values calculated (ex: maxrtt,minrtt and all the others) are unused and would be very usefull to aproximate call quality. An average value could be easily computed for the parameters (ex: rtt) and would give a mutch better picture of RTP quality. Ex: Last rtt value: 80ms Min rtt value: 30ms Max rtt value: 500ms (perhaps congestion) Avg rtt value: 35ms My proposal is to create a new function:ast_rtp_get_quality_ext that would return a detailed parsable string with the 4 values for each parameter. This could be saved in another channel variable RTPAUDIOQOSEXT. This way we would be backwards compatible with the current version. Another less elegant way is to modify the curent function and add the informations (min/max/avg) for each parameter at the end of the string returned by ast_rtp_get_quality.
Comments:	By: Jared Smith (jsmith) 2007-08-29 13:41:55 As I mentioned to the submitter on IRC, I like the word TOTAL instead of EXT.  Also, I'd like to see the per-call statistics show not only the average, but the mean and standard deviation. Also, gasparz mentioned that he's actively working on a patch for this bug, so let's not immediately close it as a "feature request without a patch".  Let's give him a short period to come up with the patch. By: Gaspar Zoltan (gasparz) 2007-08-31 07:28:56 Here are the new files (updated the 1.4.10.1). Please take a look at the source code and give feedback. I used an optimised version of the standard deviation formula given by: http://www.cs.umd.edu/~austinjp/constSD.pdf I made only one sqrt at the end. If the code is ok I can make a patch against trunk too. By: snuffy (snuffy) 2007-09-02 23:35:57 Can't see what u've changed, you should upload a diff of the file instead of whole new copy that way its easy to see the changes By: Gaspar Zoltan (gasparz) 2007-09-03 00:05:02 I uploaded the files, hope this helps. By: Marcelo M. Sosa Lugones (marsosa) 2007-09-27 14:10:48 I'm having a small problem with this patch... the contents of the AUDIORTPQOSTOTAL variable are too long, more than AST_MAX_USER_FIELD chars, so it gets truncated. By: Marcelo M. Sosa Lugones (marsosa) 2007-09-27 15:39:43 I've added a new patch for 1.4.11 (i know, i know, it is not trunk) that solves my problem of too large lines. I've splitted the AUDIORTPQOSTOTAL variable into AUDIORTPQOSJITTER, AUDIORTPQOSLOSS and AUDIORTPQOSRTT. It seems to be a useful feature, is it possible to go into trunk? what else is needed besides the patch against trunk? By: snuffy (snuffy) 2007-12-02 20:31:35.000-0600 I have uploaded a new patch that should cleanly apply to trunk. By: Gaspar Zoltan (gasparz) 2008-01-29 04:56:37.000-0600 I tried to use this with moderate success. The problems I encountered: - almost nobody is sending RTCP packets - the statistic is only for the inbound call leg So when the other end and asterisk are not sending RTCP packets the final result should be the statistics from the asterisk. I think this is the way it works now but I have to double check what are the functions (are they only called when receiving or sending RTCP) where the stats are calculated. - the statistic is only calculated for the inbound call leg: When using dial there are 2 call legs: the inbound and the outbound. If the called party hangs up we don't get the RTP statistic, because it sets a channel variable in the outbound channel that gets hanged up. For this I propose to set another set of variables to the "other" channel. I'll play a little bit with this these days and let you know how it worked out. Please feel provide feedback on these ideas, opinions. Thanks By: Gaspar Zoltan (gasparz) 2008-03-17 03:25:31 The last uploaded file contains the following fixes: - statistics when no RTCP packet is sent or received - statistics from both channels in every case, not just the outbound By: David Chappell (chappell) 2008-05-17 22:08:26 Tried latest patch.  If the bridged channel is an IAX2 channel, Asterisk crashes on hangup. By: Gaspar Zoltan (gasparz) 2008-05-23 03:48:21 As far as I know iax2 doesn't uses RTP for the voice traffic and this might cause the crash. I have to further investigate and try to fix this bug. I never tried it for IAX2, thanks for testing. By: Jared Smith (jsmith) 2008-05-30 13:53:08 That is correct.  The IAX2 channel driver *does not* use RTP, so it shouldn't have anything to do with RTCP. By: Sergey Tamkovich (sergee) 2008-06-03 03:33:22 This patch have a bug, it would crash your asterisk if one of your call-legs doesn't use RTP. Backtrace: #0  handle_request_bye (p=0x8205eb8, req=0x82061c4) at chan_sip.c:14904 #1  0xb670756e in handle_request (p=0x8205eb8, req=0xb66acfbc, sin=0xb66ae2e0, recount=0xb66ae2f0, nounlock=0xb66ae2f4) at chan_sip.c:15498 #2  0xb67094da in sipsock_read (id=0x81b6cd0, fd=24, events=1, ignore=0x0) at chan_sip.c:15638 #3  0x080ab610 in ast_io_wait (ioc=0x81b38e0, howlong=1000) at io.c:279 #4  0xb66f07f8 in do_monitor (data=0x0) at chan_sip.c:15851 ASTERISK-1  0x080fc57b in dummy_start (data=0x81b69f0) at utils.c:895 ASTERISK-2  0xb7f0834b in start_thread () from /lib/libpthread.so.0 ASTERISK-3  0xb71e465e in clone () from /lib/libc.so.6 Dialplan: exten => 555,1,Answer(); exten => 555,n,MusicOnHold() exten => 444,1,DIAL(IAX2/127.0.0.1/555) SIP phone calls 444 - asterisk segfaults on hangup. By: Sergey Tamkovich (sergee) 2008-06-03 03:53:26 This patch is tryin to access ast_bridged_channel(p->owner)->tech_pvt->rtp however only 5 pvt-s have rtp member, it are: gtalk_pvt, oh323_pvt, sip_pvt, mgcp_subchannel, skinny_subchannel. Also this patch doesn't conform to CODING GUIDELINES and it disables p2p bridging entirely for SIP calls. Before trying to access rtp member, we should check if our channel is belong to the list of rtp channels. I'll try to upgrade this patch today. By: Sergey Tamkovich (sergee) 2008-06-03 05:37:25 new patches available (against trunk and branches/1.4). 1. Segfault fixed. 2. p2p bridging uncommented. 3. Code formating updated to correspond with coding-guidelines. By: Digium Subversion (svnbot) 2008-06-05 11:17:50 Repository: asterisk Revision: 120635 U   trunk/channels/chan_sip.c U   trunk/funcs/func_channel.c U   trunk/include/asterisk/rtp.h U   trunk/main/rtp.c ------------------------------------------------------------------------ r120635 | bbryant | 2008-06-05 11:17:48 -0500 (Thu, 05 Jun 2008) | 14 lines This patch adds more detailed statistics for RTP channels, and provides an API call to access it, including maximums, minimums, standard deviatinos, and normal deviations. Currently this is implemented for chan_sip, but could be added to the func_channel_read callbacks for the CHANNEL function for any channel that uses RTP. (closes issue ASTERISK-10189) Reported by: gasparz Patches:      chan_sip_c.diff uploaded by gasparz (license 219)      rtp_c.diff uploaded by gasparz (license 219)      rtp_h.diff uploaded by gasparz (license 219)      audioqos-trunk.diff uploaded by snuffy (license 35)      rtpqos-trunk-r119891.diff uploaded by sergee (license 138) Tested by: jsmith, gasparz, snuffy, marsosa, chappell, sergee ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=120635 By: Digium Subversion (svnbot) 2008-06-05 11:19:22 Repository: asterisk Revision: 120643 _U  branches/1.6.0/ ------------------------------------------------------------------------ r120643 | bbryant | 2008-06-05 11:19:22 -0500 (Thu, 05 Jun 2008) | 21 lines Blocked revisions 120635 via svnmerge ........ r120635 | bbryant | 2008-06-05 11:24:19 -0500 (Thu, 05 Jun 2008) | 14 lines This patch adds more detailed statistics for RTP channels, and provides an API call to access it, including maximums, minimums, standard deviatinos, and normal deviations. Currently this is implemented for chan_sip, but could be added to the func_channel_read callbacks for the CHANNEL function for any channel that uses RTP. (closes issue ASTERISK-10189) Reported by: gasparz Patches:      chan_sip_c.diff uploaded by gasparz (license 219)      rtp_c.diff uploaded by gasparz (license 219)      rtp_h.diff uploaded by gasparz (license 219)      audioqos-trunk.diff uploaded by snuffy (license 35)      rtpqos-trunk-r119891.diff uploaded by sergee (license 138) Tested by: jsmith, gasparz, snuffy, marsosa, chappell, sergee ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=120643 By: Jared Smith (jsmith) 2008-06-05 11:33:28 I just want to give a big thank you to everyone who helped out with this... I'm really looking forward to playing with this in Asterisk 1.6.1 (or whenever it's put into a release branch.)