[Asterisk-code-review] res_rtp_asterisk: Don't use double math to generate timestamps (asterisk[18])

[George Joseph]

George Joseph has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/19832 )

Change subject: res_rtp_asterisk: Don't use double math to generate timestamps
......................................................................

res_rtp_asterisk: Don't use double math to generate timestamps

Rounding issues with double math were causing rtp timestamp
slips in outgoing packets.  We're now back to integer math
and are getting no more slips.

ASTERISK-30391

Change-Id: I6ba992b49ffdf9ebea074581dfa784a188c661a4
---
M res/res_rtp_asterisk.c
1 file changed, 41 insertions(+), 6 deletions(-)

Approvals:
  Joshua Colp: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved; Approved for Submit




diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index da0a3b9..bd4c1f7 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -442,6 +442,7 @@
 	int send_payload;
 	int send_duration;
 	unsigned int flags;
+	struct timeval rxcore;
 	struct timeval txcore;
 
 	struct timeval dtmfmute;
@@ -5496,10 +5497,10 @@
 {
 	int rate = ast_rtp_get_rate(rtp->f.subclass.format);
 
-	double estimated_elapsed;
 	double jitter = 0.0;
 	double prev_jitter = 0.0;
 	struct timeval now;
+	struct timeval tmp;
 	double rxnow;
 	double arrival_sec;
 	unsigned int arrival;
@@ -5512,9 +5513,22 @@
 		rtp->rxstart = ast_tv2double(&now);
 		rtp->remote_seed_rx_rtp_ts = rx_rtp_ts;
 
-		/* Round to 0.1ms for nice, pretty timestamps */
-		rtp->rxstart = floor( rtp->rxstart * 10000.0 ) / 10000.0;
-		*tv = ast_double2tv(rtp->rxstart);
+		/*
+		 * "tv" is placed in the received frame's
+		 * "delivered" field and when this frame is
+		 * sent out again on the other side, it's
+		 * used to calculate the timestamp on the
+		 * outgoing RTP packets.
+		 *
+		 * NOTE: We need to do integer math here
+		 * because double math rounding issues can
+		 * generate incorrect timestamps.
+		 */
+		rtp->rxcore = now;
+		tmp = ast_samp2tv(rx_rtp_ts, rate);
+		rtp->rxcore = ast_tvsub(rtp->rxcore, tmp);
+		rtp->rxcore.tv_usec -= rtp->rxcore.tv_usec % 100;
+		*tv = ast_tvadd(rtp->rxcore, tmp);
 
 		ast_debug_rtcp(3, "%s: "
 			"Seed ts: %u current time: %f\n",
@@ -5526,8 +5540,14 @@
 		return;
 	}
 
-	estimated_elapsed = ast_samp2sec(rx_rtp_ts - rtp->remote_seed_rx_rtp_ts, rate);
-	*tv = ast_double2tv(rtp->rxstart + estimated_elapsed);
+	tmp = ast_samp2tv(rx_rtp_ts, rate);
+	/* See the comment about "tv" above. Even if
+	 * we don't use this received packet for jitter
+	 * calculations, we still need to set tv so the
+	 * timestamp will be correct when this packet is
+	 * sent out again.
+	 */
+	*tv = ast_tvadd(rtp->rxcore, tmp);
 
 	/*
 	 * The first few packets are generally unstable so let's

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/19832
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: I6ba992b49ffdf9ebea074581dfa784a188c661a4
Gerrit-Change-Number: 19832
Gerrit-PatchSet: 2
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20230113/b42df77d/attachment-0001.html>