[MERGED] libosmo-netif[master]: osmux: Set Marker bit on osmux frame loss detected

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Pau Espin Pedrol gerrit-no-reply at lists.osmocom.org
Mon Apr 23 09:50:16 UTC 2018


Pau Espin Pedrol has submitted this change and it was merged.

Change subject: osmux: Set Marker bit on osmux frame loss detected
......................................................................


osmux: Set Marker bit on osmux frame loss detected

Until this patch, we didn't notify in any way to the RTP reader when an
Osmux frame was lost. Instead, we updated the seq&timestamp as if there
was no lost, and as a result the RTP reader would only see a steady
increase of delay every time an osmux frame was lost.

As the batch_factor for the lost packet is unknown, we cannot assume any
number of amr payloads lost, and thus we cannot simply increment seq and
timestamp for a specific amount. Instead, the only viable solution seems
to set the M marker bit in the first rtp packet generated after a
non-consecutive osmux frame is received.

The implementation may act differently with the first generated RTP
packet based on the first osmux seq number used for the stream. In case
0 it's used as first osmux seq number, M will be set depending on
request from original RTP packet having the M bit set. If it's not 0,
the first RTP packer will unconditionally have the M bit. That's not an
issue because it's anyway expect for receiver to sync on the first
packet.

Related: OS#3185

Change-Id: I2efed6d726a1b8e77e686c7a5fe1940d3f4901a7
---
M include/osmocom/netif/osmux.h
M src/osmux.c
M tests/osmux/osmux_test.c
M tests/osmux/osmux_test2.ok
4 files changed, 34 insertions(+), 12 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/include/osmocom/netif/osmux.h b/include/osmocom/netif/osmux.h
index e8e2c67..dfed66a 100644
--- a/include/osmocom/netif/osmux.h
+++ b/include/osmocom/netif/osmux.h
@@ -80,6 +80,7 @@
 	uint16_t rtp_seq;
 	uint32_t rtp_timestamp;
 	uint32_t rtp_ssrc;
+	uint8_t osmux_seq_ack; /* Latest received seq num */
 	struct osmo_timer_list	timer;
 	struct llist_head list;
 	void (*tx_cb)(struct msgb *msg, void *data); /* Used defined rtp tx callback */
diff --git a/src/osmux.c b/src/osmux.c
index a7339c6..872588d 100644
--- a/src/osmux.c
+++ b/src/osmux.c
@@ -151,8 +151,14 @@
 	rtph->timestamp = htonl(h->rtp_timestamp);
 	rtph->sequence = htons(h->rtp_seq);
 	rtph->ssrc = htonl(h->rtp_ssrc);
-	/* rtp packet with the marker bit is always warranted to be the first one */
-	rtph->marker = first_pkt && osmuxh->rtp_m;
+	/* rtp packet with the marker bit is always guaranteed to be the first
+	 * one. We want to notify with marker in 2 scenarios:
+	 * 1- Sender told us through osmux frame rtp_m.
+	 * 2- Sntermediate osmux frame lost (seq gap), otherwise rtp receiver only sees
+	 *    steady increase of delay
+	 */
+	rtph->marker = first_pkt &&
+			(osmuxh->rtp_m || (osmuxh->seq != h->osmux_seq_ack + 1));
 
 	msgb_put(out_msg, sizeof(struct rtp_hdr));
 
@@ -218,6 +224,10 @@
 #endif
 		llist_add_tail(&msg->list, list);
 	}
+
+	/* Update last seen seq number: */
+	h->osmux_seq_ack = osmuxh->seq;
+
 	return i;
 }
 
@@ -291,6 +301,10 @@
 
 		llist_add_tail(&msg->list, &h->list);
 	}
+
+	/* Update last seen seq number: */
+	h->osmux_seq_ack = osmuxh->seq;
+
 	/* In case list is still empty after parsing messages, no need to rearm */
 	if(was_empty && !llist_empty(&h->list))
 		osmux_xfrm_output_trigger(h);
diff --git a/tests/osmux/osmux_test.c b/tests/osmux/osmux_test.c
index 01ce2d1..631ade8 100644
--- a/tests/osmux/osmux_test.c
+++ b/tests/osmux/osmux_test.c
@@ -152,7 +152,8 @@
 			memcpy(msg->data, rtp_pkt, sizeof(rtp_pkt));
 			cpy_rtph = (struct rtp_hdr *) msgb_put(msg, sizeof(rtp_pkt));
 
-			if ((i+j) % 7 == 0) {
+			/* first condition guarantees that 1st packet per stream contains M bit set. */
+			if (i == 0 || (i+j) % 7 == 0) {
 				cpy_rtph->marker = 1;
 				mark_pkts++;
 			}
@@ -175,7 +176,7 @@
 	}
 
 	if (mark_pkts) {
-		fprintf(stdout, "RTP M bit (marker) mismatch! %d\n", mark_pkts);
+		fprintf(stdout, "osmux_test_marker: RTP M bit (marker) mismatch! %d\n", mark_pkts);
 		exit(EXIT_FAILURE);
 	}
 }
@@ -183,6 +184,7 @@
 static void osmux_test_loop(int ccid)
 {
 	struct rtp_hdr *rtph = (struct rtp_hdr *)rtp_pkt;
+	struct rtp_hdr *cpy_rtph;
 	struct msgb *msg;
 	int i, j, k = 0;
 	char buf[1024];
@@ -194,11 +196,16 @@
 			exit(EXIT_FAILURE);
 
 		memcpy(msg->data, rtp_pkt, sizeof(rtp_pkt));
-		msgb_put(msg, sizeof(rtp_pkt));
+		cpy_rtph = (struct rtp_hdr *) msgb_put(msg, sizeof(rtp_pkt));
 
 		seq = ntohs(rtph->sequence);
 		seq++;
 		rtph->sequence = htons(seq);
+		if (i < 3) {
+			/* Mark 1 rtp packet of each stream */
+			cpy_rtph->marker = 1;
+			mark_pkts++;
+		}
 
 		osmo_rtp_snprintf(buf, sizeof(buf), msg);
 		fprintf(stderr, "adding to ccid=%u %s\n", (i % 2) + ccid, buf);
@@ -239,7 +246,7 @@
 	}
 
 	if (mark_pkts) {
-		fprintf(stdout, "RTP M bit (marker) mismatch! %d\n", mark_pkts);
+		fprintf(stdout, "osmux_test_loop: RTP M bit (marker) mismatch! %d\n", mark_pkts);
 		exit(EXIT_FAILURE);
 	}
 }
diff --git a/tests/osmux/osmux_test2.ok b/tests/osmux/osmux_test2.ok
index 1e6f95a..53ab67d 100644
--- a/tests/osmux/osmux_test2.ok
+++ b/tests/osmux/osmux_test2.ok
@@ -1,6 +1,6 @@
 ===test_output_consecutive===
 sys={0.000000}, mono={0.000000}: clock_override_set
-sys={0.000000}, mono={0.000000}: dequeue: seq=50 ts=500 enqueued=5
+sys={0.000000}, mono={0.000000}: dequeue: seq=50 ts=500 M enqueued=5
 sys={0.000000}, mono={0.000000}: first dequed before first select
 sys={0.020000}, mono={0.020000}: clock_override_add
 sys={0.020000}, mono={0.020000}: second select, second dequed
@@ -32,7 +32,7 @@
 sys={0.200000}, mono={0.200000}: calling flush on empty list, should do nothing
 ===test_output_interleaved===
 sys={0.000000}, mono={0.000000}: clock_override_set
-sys={0.000000}, mono={0.000000}: dequeue: seq=50 ts=500 enqueued=5
+sys={0.000000}, mono={0.000000}: dequeue: seq=50 ts=500 M enqueued=5
 sys={0.040000}, mono={0.040000}: clock_override_add
 sys={0.040000}, mono={0.040000}: select, 3 dequed, 3 still queued
 sys={0.040000}, mono={0.040000}: dequeue: seq=51 ts=660 enqueued=4
@@ -51,7 +51,7 @@
 sys={0.140000}, mono={0.140000}: dequeue: seq=61 ts=2260 enqueued=0
 ===test_output_2together===
 sys={0.000000}, mono={0.000000}: clock_override_set
-sys={0.000000}, mono={0.000000}: dequeue: seq=50 ts=500 enqueued=5
+sys={0.000000}, mono={0.000000}: dequeue: seq=50 ts=500 M enqueued=5
 sys={0.000000}, mono={0.000000}: calling select in between 2 osmux recv
 sys={0.000000}, mono={0.000000}: calling select after receiving 2nd osmux. Dequeue 1st osmux frame and 1st rtp from 2nd osmux frame.
 sys={0.000000}, mono={0.000000}: dequeue: seq=51 ts=660 enqueued=10
@@ -70,7 +70,7 @@
 ===test_output_frame_lost===
 sys={0.000000}, mono={0.000000}: clock_override_set
 sys={0.000000}, mono={0.000000}: first osmux frame
-sys={0.000000}, mono={0.000000}: dequeue: seq=50 ts=500 enqueued=5
+sys={0.000000}, mono={0.000000}: dequeue: seq=50 ts=500 M enqueued=5
 sys={0.100000}, mono={0.100000}: clock_override_add
 sys={0.100000}, mono={0.100000}: dequeue: seq=51 ts=660 enqueued=4
 sys={0.100000}, mono={0.100000}: dequeue: seq=52 ts=820 enqueued=3
@@ -80,7 +80,7 @@
 sys={0.100000}, mono={0.100000}: one osmux frame is now lost (seq++)
 sys={0.220000}, mono={0.220000}: clock_override_add
 sys={0.220000}, mono={0.220000}: 3rd osmux frame arrives
-sys={0.220000}, mono={0.220000}: dequeue: seq=56 ts=1460 enqueued=5
+sys={0.220000}, mono={0.220000}: dequeue: seq=56 ts=1460 M enqueued=5
 sys={0.320000}, mono={0.320000}: clock_override_add
 sys={0.320000}, mono={0.320000}: dequeue: seq=57 ts=1620 enqueued=4
 sys={0.320000}, mono={0.320000}: dequeue: seq=58 ts=1780 enqueued=3
@@ -90,7 +90,7 @@
 ===test_output_flush===
 sys={0.000000}, mono={0.000000}: clock_override_set
 sys={0.000000}, mono={0.000000}: first osmux frame
-sys={0.000000}, mono={0.000000}: dequeue: seq=50 ts=500 enqueued=5
+sys={0.000000}, mono={0.000000}: dequeue: seq=50 ts=500 M enqueued=5
 sys={0.040000}, mono={0.040000}: clock_override_add
 sys={0.040000}, mono={0.040000}: dequeue: seq=51 ts=660 enqueued=4
 sys={0.040000}, mono={0.040000}: dequeue: seq=52 ts=820 enqueued=3

-- 
To view, visit https://gerrit.osmocom.org/7871
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I2efed6d726a1b8e77e686c7a5fe1940d3f4901a7
Gerrit-PatchSet: 4
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pablo Neira Ayuso <pablo at gnumonks.org>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>



More information about the gerrit-log mailing list