[Asterisk-code-review] res_rtp_asterisk: Fix sequence number cycling and packet loss count. (...asterisk[16])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/c/asterisk/+/11365 )

Change subject: res_rtp_asterisk: Fix sequence number cycling and packet loss count.
......................................................................

res_rtp_asterisk: Fix sequence number cycling and packet loss count.

This change fixes two bugs which both resulted in the packet loss
count exceeding 65,000.

The first issue is that the sequence number check to determine if
cycling had occurred was using the wrong variable resulting in the
check never seeing that cycling has occurred, throwing off the
packet loss calculation. It now uses the correct variable.

The second issue is that the packet loss calculation assumed that
the received number of packets in an interval could never exceed
the expected number. In practice this isn't true due to delayed
or retransmitted packets. The expected will now be updated to
the received number if the received exceeds it.

ASTERISK-28379

Change-Id: If888ebc194ab69ac3194113a808c414b014ce0f6
---
M res/res_rtp_asterisk.c
1 file changed, 9 insertions(+), 1 deletion(-)

Approvals:
  George Joseph: Looks good to me, but someone else must approve
  Benjamin Keith Ford: Looks good to me, but someone else must approve
  Kevin Harwell: Looks good to me, approved
  Joshua Colp: Approved for Submit



diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index 023273a..c4df6a4 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -4126,6 +4126,14 @@
 	*lost_packets = expected_packets - rtp->rxcount;
 	expected_interval = expected_packets - rtp->rtcp->expected_prior;
 	received_interval = rtp->rxcount - rtp->rtcp->received_prior;
+	if (received_interval > expected_interval) {
+		/* If we receive some late packets it is possible for the packets
+		 * we received in this interval to exceed the number we expected.
+		 * We update the expected so that the packet loss calculations
+		 * show that no packets are lost.
+		 */
+		expected_interval = received_interval;
+	}
 	lost_interval = expected_interval - received_interval;
 	if (expected_interval == 0 || lost_interval <= 0) {
 		*fraction_lost = 0;
@@ -6801,7 +6809,7 @@
 			ast_log(LOG_WARNING, "scheduling RTCP transmission failed.\n");
 		}
 	}
-	if ((int)rtp->lastrxseqno - (int)seqno  > 100) /* if so it would indicate that the sender cycled; allow for misordering */
+	if ((int)prev_seqno - (int)seqno  > 100) /* if so it would indicate that the sender cycled; allow for misordering */
 		rtp->cycles += RTP_SEQ_MOD;
 
 	/* If we are directly bridged to another instance send the audio directly out,

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

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: If888ebc194ab69ac3194113a808c414b014ce0f6
Gerrit-Change-Number: 11365
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190515/dba04ab4/attachment.html>