[Asterisk-code-review] res rtp asterisk: RTT miscalculation in RTCP (asterisk[14])
Joshua Colp
asteriskteam at digium.com
Wed Nov 23 12:10:37 CST 2016
Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/4490 )
Change subject: res_rtp_asterisk: RTT miscalculation in RTCP
......................................................................
res_rtp_asterisk: RTT miscalculation in RTCP
When retrieving RTCP stats for PJSIP channels, RTT values are unreliable.
RTT calculation is correct, but the data representation isn't. RTT is
represented by a 32-bit fixed-point number with the integer part in the
first 16 bits and the fractional part in the last 16 bits. In order to
get the RTT value, the fractional part is miscalculated, there is an
unnecessary 16 bit shift that causes overflow. Besides this there is
another mistake, when transforming the integer value to the fixed point
fractional part via bitwise operation, that loses precision.
* RTT fractional part is no longer shifted, avoiding overflow.
* RTT fractional part is transformed to its fixed-point value more
precisely.
* Fixed timeval2ntp() and ntp2timeval() second fraction conversions.
* Fixed NTP timestamp report logging. The usec was inexplicably
multiplied by 4096.
ASTERISK-26566 #close
Reported by Hector Royo Concepcion
Change-Id: Ie09bdabfee75afb3f1b8ddfd963e5219ada3b96f
---
M main/rtp_engine.c
M res/res_rtp_asterisk.c
2 files changed, 42 insertions(+), 9 deletions(-)
Approvals:
Richard Mudgett: Looks good to me, but someone else must approve
Anonymous Coward #1000019: Verified
Joshua Colp: Looks good to me, approved
diff --git a/main/rtp_engine.c b/main/rtp_engine.c
index 1b72af1..9175a28 100644
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -2495,7 +2495,7 @@
if (type == AST_RTP_RTCP_SR) {
ast_str_append(&packet_string, 0, "SentNTP: %lu.%06lu\r\n",
(unsigned long)payload->report->sender_information.ntp_timestamp.tv_sec,
- (unsigned long)payload->report->sender_information.ntp_timestamp.tv_usec * 4096);
+ (unsigned long)payload->report->sender_information.ntp_timestamp.tv_usec);
ast_str_append(&packet_string, 0, "SentRTP: %u\r\n",
payload->report->sender_information.rtp_timestamp);
ast_str_append(&packet_string, 0, "SentPackets: %u\r\n",
diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index 7fd5524..588247b 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -3086,7 +3086,26 @@
unsigned int sec, usec, frac;
sec = tv.tv_sec + 2208988800u; /* Sec between 1900 and 1970 */
usec = tv.tv_usec;
- frac = (usec << 12) + (usec << 8) - ((usec * 3650) >> 6);
+ /*
+ * Convert usec to 0.32 bit fixed point without overflow.
+ *
+ * = usec * 2^32 / 10^6
+ * = usec * 2^32 / (2^6 * 5^6)
+ * = usec * 2^26 / 5^6
+ *
+ * The usec value needs 20 bits to represent 999999 usec. So
+ * splitting the 2^26 to get the most precision using 32 bit
+ * values gives:
+ *
+ * = ((usec * 2^12) / 5^6) * 2^14
+ *
+ * Splitting the division into two stages preserves all the
+ * available significant bits of usec over doing the division
+ * all at once.
+ *
+ * = ((((usec * 2^12) / 5^3) * 2^7) / 5^3) * 2^7
+ */
+ frac = ((((usec << 12) / 125) << 7) / 125) << 7;
*msw = sec;
*lsw = frac;
}
@@ -3094,7 +3113,8 @@
static void ntp2timeval(unsigned int msw, unsigned int lsw, struct timeval *tv)
{
tv->tv_sec = msw - 2208988800u;
- tv->tv_usec = ((lsw << 6) / 3650) - (lsw >> 12) - (lsw >> 8);
+ /* Reverse the sequence in timeval2ntp() */
+ tv->tv_usec = ((((lsw >> 7) * 125) >> 7) * 125) >> 12;
}
static void calculate_lost_packet_statistics(struct ast_rtp *rtp,
@@ -3279,9 +3299,9 @@
ast_sockaddr_stringify(&remote_address), ice ? " (via ICE)" : "");
ast_verbose(" Our SSRC: %u\n", rtcp_report->ssrc);
if (sr) {
- ast_verbose(" Sent(NTP): %u.%010u\n",
+ ast_verbose(" Sent(NTP): %u.%06u\n",
(unsigned int)rtcp_report->sender_information.ntp_timestamp.tv_sec,
- (unsigned int)rtcp_report->sender_information.ntp_timestamp.tv_usec * 4096);
+ (unsigned int)rtcp_report->sender_information.ntp_timestamp.tv_usec);
ast_verbose(" Sent(RTP): %u\n", rtcp_report->sender_information.rtp_timestamp);
ast_verbose(" Sent packets: %u\n", rtcp_report->sender_information.packet_count);
ast_verbose(" Sent octets: %u\n", rtcp_report->sender_information.octet_count);
@@ -4018,9 +4038,22 @@
lsr_a = ((msw & 0x0000ffff) << 16) | ((lsw & 0xffff0000) >> 16);
rtt = lsr_a - lsr - dlsr;
rtt_msw = (rtt & 0xffff0000) >> 16;
- rtt_lsw = (rtt & 0x0000ffff) << 16;
+ rtt_lsw = (rtt & 0x0000ffff);
rtt_tv.tv_sec = rtt_msw;
- rtt_tv.tv_usec = ((rtt_lsw << 6) / 3650) - (rtt_lsw >> 12) - (rtt_lsw >> 8);
+ /*
+ * Convert 16.16 fixed point rtt_lsw to usec without
+ * overflow.
+ *
+ * = rtt_lsw * 10^6 / 2^16
+ * = rtt_lsw * (2^6 * 5^6) / 2^16
+ * = rtt_lsw * 5^6 / 2^10
+ *
+ * The rtt_lsw value is in 16.16 fixed point format and 5^6
+ * requires 14 bits to represent. We have enough space to
+ * directly do the conversion because there is no integer
+ * component in rtt_lsw.
+ */
+ rtt_tv.tv_usec = (rtt_lsw * 15625) >> 10;
rtp->rtcp->rtt = (double)rtt_tv.tv_sec + ((double)rtt_tv.tv_usec / 1000000);
if (lsr_a - dlsr < lsr) {
return 1;
@@ -4216,9 +4249,9 @@
&rtcp_report->sender_information.ntp_timestamp);
rtcp_report->sender_information.rtp_timestamp = ntohl(rtcpheader[i + 2]);
if (rtcp_debug_test_addr(&addr)) {
- ast_verbose("NTP timestamp: %u.%010u\n",
+ ast_verbose("NTP timestamp: %u.%06u\n",
(unsigned int)rtcp_report->sender_information.ntp_timestamp.tv_sec,
- (unsigned int)rtcp_report->sender_information.ntp_timestamp.tv_usec * 4096);
+ (unsigned int)rtcp_report->sender_information.ntp_timestamp.tv_usec);
ast_verbose("RTP timestamp: %u\n", rtcp_report->sender_information.rtp_timestamp);
ast_verbose("SPC: %u\tSOC: %u\n",
rtcp_report->sender_information.packet_count,
--
To view, visit https://gerrit.asterisk.org/4490
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie09bdabfee75afb3f1b8ddfd963e5219ada3b96f
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Hector Royo Concepcion <hectorroyo92 at gmail.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
More information about the asterisk-code-review
mailing list