| Summary: | ASTERISK-14745: rtt should be stored as double in struct ast_rtp_instance_stats | ||||
| Reporter: | klaus3000 (klaus3000) | Labels: | |||
| Date Opened: | 2009-09-01 09:36:39 | Date Closed: | 2010-06-07 14:52:42 | ||
| Priority: | Minor | Regression? | No | ||
| Status: | Closed/Complete | Components: | Core/RTP | ||
| Versions: | Frequency of Occurrence | ||||
| Related Issues: |
| ||||
| Environment: | Attachments: | ||||
| Description: | Hi! AFAIS member "rtt" in struct ast_rtp_instance_stats stored the RTT in seconds, using an usigned int. As RTTs usually are below 0.5 (at least on earth and for VoIP useable links) this member is useless as it is always 0. Shouldn't that be changed to double? | ||||
| Comments: | By: Tilghman Lesher (tilghman) 2009-09-01 18:06:26 Given that the value is never actually set, how it is stored is kind of irrelevant, don't you think? I think it's actually meant to be stored in number of milliseconds, but given that it's never actually set, it's not really determined. file: should the field be removed? By: Joshua C. Colp (jcolp) 2009-09-01 18:17:35 No, the field should not be removed. It is set by res_rtp_asterisk when statistics are requested. As for changing it I'm pretty sure we treat that as milliseconds. By: Tilghman Lesher (tilghman) 2009-09-01 18:53:40 Given the comment from the RTP engine designer, this change appears to be no longer necessary or desireable. By: klaus3000 (klaus3000) 2009-09-02 04:22:01 Have you taken a look at the code? In res/res_rtp_asterisk.c in function ast_rtp_get_stat(...): AST_RTP_STAT_SET(AST_RTP_INSTANCE_STAT_RTT, AST_RTP_INSTANCE_STAT_COMBINED_RTT, stats->rtt, rtp->rtcp->rtt); ast_rtcp->rtt (double) is copied to ast_rtp_instance_stats->rtt (unsigend int). ast_rtcp->rtt is set in ast_rtcp_read(): rtt = rtt / 1000.; rttsec = rtt / 1000.; rtp->rtcp->rtt = rttsec; This sound pretty obvious that ast_rtcp->rtt stores the rtt in seconds using a double. This double is copied into an int which is of course more or less always 0. My suggestion: change struct ast_rtp_instance_stats->rtt from uint to double By: klaus3000 (klaus3000) 2009-09-02 04:58:05 Some more statistics issues: 1. in include/asterisk/rtp_engine.h, macro AST_RTP_STAT_SET: #define AST_RTP_STAT_SET(current_stat, combined, placement, value) \ if (stat == current_stat || stat == AST_RTP_INSTANCE_STAT_ALL || (combined >= 0 && combined == current_stat)) { \ placement = value; \ if (stat == current_stat) { \ return 0; \ } \ } The last condition (combined >= 0 && combined == current_stat) IMO makes no sense - I think this is a bug and should be: (combined >= 0 && combined == stat) 2. in main/rtp_engine.c function ast_rtp_instance_get_quality: In case AST_RTP_INSTANCE_STAT_FIELD_QUALITY, "stat" is set to AST_RTP_INSTANCE_STAT_ALL, which means that ALL stats or copied from the RTP engine to the "stats" strcuture, but only a few of them are actually copied into the buffer. Why not make this really usefull and copy ALL stats into the buffer? This would make it possible to retrieve ALL stats with just a single query. btw: I would be willing to write the patches .... PS: Probably this belongs not to this thread, but in include/asterisk/rtp_engine.h i found: * Properties allow behavior of the RTP engine and RTP engine core to be * changed. For example, there is a property named AST_RTP_PROPERTY_NAT which is * used to tell the RTP engine to enable symmetric RTP if it supports it. It is * not required for an RTP engine to support all properties. I wonder why things are not just called what they are? IMO AST_RTP_PROPERTY_SYMMETRICRTP would be much more meaningful than AST_RTP_PROPERTY_NAT By: Leif Madsen (lmadsen) 2010-03-24 11:49:08 oej: Any comments on this? We'd like to get some consensus on this to determine if a patch should be created, and what that patch should be doing. By: Paul Belanger (pabelanger) 2010-05-12 12:48:50 ping: Any updates? By: Tilghman Lesher (tilghman) 2010-06-02 14:23:30 klaus: You wrote: The last condition (combined >= 0 && combined == current_stat) IMO makes no sense - I think this is a bug and should be: (combined >= 0 && combined == stat) Could you explain your logic? Why do you think it makes no sense? By: Digium Subversion (svnbot) 2010-06-07 14:52:39 Repository: asterisk Revision: 268773 U trunk/channels/chan_sip.c U trunk/channels/sip/dialplan_functions.c U trunk/include/asterisk/rtp_engine.h U trunk/main/rtp_engine.c ------------------------------------------------------------------------ r268773 | tilghman | 2010-06-07 14:52:38 -0500 (Mon, 07 Jun 2010) | 5 lines Seems strange (and the code backs up) that if the max and min of a statistic is expressed as a double, the last value would not also need to be a double. (closes issue ASTERISK-14745) Reported by: klaus3000 ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=268773 | ||||