[Asterisk-code-review] res_rtp_asterisk: Don't count 0 as a minimum lost packets (asterisk[master])

[George Joseph]

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

Change subject: res_rtp_asterisk: Don't count 0 as a minimum lost packets
......................................................................

res_rtp_asterisk: Don't count 0 as a minimum lost packets

The calculated minimum lost packets represents the lowest number of
lost packets missed during an RTCP report interval. Zero of course
is the lowest, but the idea is that this value contain the lowest
number of lost packets once some have been missed.

This patch checks to make sure the number of lost packets over an
interval is not zero before checking and setting the minimum value.

Also, this patch updates the rtp lost packet test to check for
packet loss over several reports vs one.

Change-Id: I07d6e21cec61e289c2326138d6bcbcb3c3d5e008
---
M res/res_rtp_asterisk.c
M tests/test_res_rtp.c
2 files changed, 38 insertions(+), 4 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 16c3be0..4fd803b 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -4472,7 +4472,7 @@
 	if (rtp->rtcp->rxlost_count == 0) {
 		rtp->rtcp->minrxlost = rtp->rtcp->rxlost;
 	}
-	if (lost_interval < rtp->rtcp->minrxlost) {
+	if (lost_interval && lost_interval < rtp->rtcp->minrxlost) {
 		rtp->rtcp->minrxlost = rtp->rtcp->rxlost;
 	}
 	if (lost_interval > rtp->rtcp->maxrxlost) {
diff --git a/tests/test_res_rtp.c b/tests/test_res_rtp.c
index 08c9971..1d36116 100644
--- a/tests/test_res_rtp.c
+++ b/tests/test_res_rtp.c
@@ -269,7 +269,7 @@
 	RAII_VAR(struct ast_rtp_instance *, instance2, NULL, ast_rtp_instance_destroy);
 	RAII_VAR(struct ast_sched_context *, test_sched, NULL, ast_sched_context_destroy_wrapper);
 	struct ast_rtp_instance_stats stats = { 0, };
-	enum ast_rtp_instance_stat stat = AST_RTP_INSTANCE_STAT_RXPLOSS;
+	enum ast_rtp_instance_stat stat = AST_RTP_INSTANCE_STAT_ALL;
 
 	switch (cmd) {
 	case TEST_INIT:
@@ -303,8 +303,42 @@
 
 	/* Check RTCP stats to see if we got the expected packet loss count */
 	ast_rtp_instance_get_stats(instance2, &stats, stat);
-	ast_test_validate(test, stats.rxploss == 5,
-		"Condition of 5 lost packets was not met");
+	ast_test_validate(test, stats.rxploss == 5 && stats.local_minrxploss == 5 &&
+		stats.local_maxrxploss == 5, "Condition of 5 lost packets was not met");
+
+	/* Drop 3 before writing 5 more */
+	test_write_and_read_frames(instance1, instance2, 1023, 5);
+
+	ast_rtp_instance_queue_report(instance1);
+	test_write_frames(instance2, 1001, 1);
+	ast_rtp_instance_get_stats(instance2, &stats, stat);
+
+	/* Should now be missing 8 total packets with a change in min */
+	ast_test_validate(test, stats.rxploss == 8 && stats.local_minrxploss == 3 &&
+		stats.local_maxrxploss == 5);
+
+	/* Write 5 more with no gaps */
+	test_write_and_read_frames(instance1, instance2, 1028, 5);
+
+	ast_rtp_instance_queue_report(instance1);
+	test_write_frames(instance2, 1002, 1);
+	ast_rtp_instance_get_stats(instance2, &stats, stat);
+
+	/* Should still only be missing 8 total packets */
+	ast_test_validate(test, stats.rxploss == 8 && stats.local_minrxploss == 3 &&
+		stats.local_maxrxploss == 5);
+
+	/* Now drop 1, write another 5, drop 8, and then write 5 */
+	test_write_and_read_frames(instance1, instance2, 1034, 5);
+	test_write_and_read_frames(instance1, instance2, 1047, 5);
+
+	ast_rtp_instance_queue_report(instance1);
+	test_write_frames(instance2, 1003, 1);
+	ast_rtp_instance_get_stats(instance2, &stats, stat);
+
+	/* Now it should be missing 17 total packets, with a change in max */
+	ast_test_validate(test, stats.rxploss == 17 && stats.local_minrxploss == 3 &&
+		stats.local_maxrxploss == 9);
 
 	return AST_TEST_PASS;
 }

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I07d6e21cec61e289c2326138d6bcbcb3c3d5e008
Gerrit-Change-Number: 15711
Gerrit-PatchSet: 3
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Friendly Automation
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/20210401/8a798740/attachment.html>