pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29505 )
Change subject: tests/osmux: Always run with fake time
......................................................................
tests/osmux: Always run with fake time
Let's not make the tests more difficult to maintain and extend by
allowing them to run in real clock time, there's no real need for it.
Change-Id: I549a4722d63d700b54b146260131a68e656b843e
---
M tests/osmux/osmux_test.c
1 file changed, 1 insertion(+), 36 deletions(-)
Approvals:
Jenkins Builder: Verified
laforge: Looks good to me, approved
fixeria: Looks good to me, but someone else must approve
diff --git a/tests/osmux/osmux_test.c b/tests/osmux/osmux_test.c
index 54e9368..3284a15 100644
--- a/tests/osmux/osmux_test.c
+++ b/tests/osmux/osmux_test.c
@@ -1,6 +1,7 @@
/*
* (C) 2013 by Pablo Neira Ayuso <pablo(a)gnumonks.org>
* (C) 2013 by On Waves ehf <http://www.on-waves.com>
+ * (C) 2022 by sysmocom - s.f.m.c. GmbH <info(a)sysmocom.de>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -8,8 +9,6 @@
* (at your option) any later version.
*/
-#define OSMUX_TEST_USE_TIMING 0
-
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
@@ -17,9 +16,6 @@
#include <string.h>
#include <signal.h>
#include <arpa/inet.h>
-#if OSMUX_TEST_USE_TIMING
-#include <sys/time.h>
-#endif
#include <osmocom/core/select.h>
#include <osmocom/core/application.h>
@@ -59,9 +55,6 @@
static int rtp_pkts;
static int mark_pkts;
-#if OSMUX_TEST_USE_TIMING
-static struct timeval last;
-#endif
#define clock_debug(fmt, args...) \
do { \
@@ -92,20 +85,6 @@
{
struct rtp_hdr *rtph = (struct rtp_hdr *)msg->data;
char buf[4096];
-#if OSMUX_TEST_USE_TIMING
- struct timeval now, diff;
-
- osmo_gettimeofday(&now, NULL);
- timersub(&now, &last, &diff);
- last = now;
-
- if (diff.tv_usec > 2*17000) {
- clock_debug("delivery of reconstructed RTP lagged"
- " (diff.tv_usec=%u > 2*17000)\n",
- (unsigned int)diff.tv_usec);
- exit(EXIT_FAILURE);
- }
-#endif
osmo_rtp_snprintf(buf, sizeof(buf), msg);
clock_debug("extracted packet: %s\n", buf);
@@ -185,15 +164,11 @@
osmux_xfrm_input_deliver(&h_input);
}
}
-#if !OSMUX_TEST_USE_TIMING
clock_override_add(0, PKT_TIME_USEC);
-#endif
}
while (rtp_pkts) {
-#if !OSMUX_TEST_USE_TIMING
clock_override_add(1, 0);
-#endif
osmo_select_main(0);
}
@@ -242,10 +217,6 @@
osmux_xfrm_input(&h_input, msg, (i % 2) + ccid);
if (i % 4 == 0) {
-#if OSMUX_TEST_USE_TIMING
- osmo_gettimeofday(&last, NULL);
-#endif
-
/* After four RTP messages, squash them into the OSMUX
* batch and call the routine to deliver it.
*/
@@ -258,9 +229,7 @@
*/
for (j = 0; j < k-2; j++) {
osmo_select_main(0);
-#if !OSMUX_TEST_USE_TIMING
clock_override_add(0, PKT_TIME_USEC);
-#endif
}
k = 0;
@@ -282,11 +251,9 @@
exit(EXIT_FAILURE);
}
-#if !OSMUX_TEST_USE_TIMING
/* This test uses fake time to speedup the run, unless we want to manually
* test time specific stuff */
clock_override_enable(true);
-#endif
/* This test doesn't use it, but osmux requires it internally. */
void *tall_ctx = talloc_named_const(NULL, 1, "Root context");
@@ -309,7 +276,6 @@
/* If the test takes longer than 10 seconds, abort it */
alarm(10);
-#if !OSMUX_TEST_USE_TIMING
/* Check if marker bit features work correctly */
osmux_xfrm_input_init(&h_input);
for (i = 0; i < 4; i++)
@@ -318,7 +284,6 @@
for (i = 0; i < 4; i++)
osmux_xfrm_input_close_circuit(&h_input, i);
osmux_xfrm_input_fini(&h_input);
-#endif
osmux_xfrm_input_init(&h_input);
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29505
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I549a4722d63d700b54b146260131a68e656b843e
Gerrit-Change-Number: 29505
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29504 )
Change subject: tests/osmux_test2: Document unit tests
......................................................................
tests/osmux_test2: Document unit tests
Change-Id: I56f27af52b6a8d798879c70d68a9a3b9e512867d
---
M tests/osmux/osmux_test2.c
1 file changed, 16 insertions(+), 1 deletion(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, approved
laforge: Looks good to me, approved
diff --git a/tests/osmux/osmux_test2.c b/tests/osmux/osmux_test2.c
index 7a66920..46065fe 100644
--- a/tests/osmux/osmux_test2.c
+++ b/tests/osmux/osmux_test2.c
@@ -155,6 +155,7 @@
OSMO_ASSERT(_rc == _osmuxh->ctr+1); \
}
+/* Test some regular scenario where frames arrive at exactly the time they should. */
static void test_output_consecutive(void)
{
struct osmux_out_handle *h_output;
@@ -222,6 +223,12 @@
talloc_free(h_output);
}
+/* Test that receiving new osmux frame triggers flushing of RTP pakcets
+ * generated from previous one, to avoid steady growing delay in scheduling due to
+ * jitter of osmux packets received. Specifically test case where the 2 Osmux
+ * packets arrive with a small delay of system time in between them, aka the 1st
+ * Osmux frame has had some of its AMR payloads already forwarded as RTP to the
+ * upper layers. */
static void test_output_interleaved(void)
{
struct osmux_out_handle *h_output;
@@ -264,6 +271,11 @@
talloc_free(h_output);
}
+/* Test that receiving new osmux frame triggers flushing of RTP pakcets
+ * generated from previous one, to avoid steady growing delay in scheduling due to
+ * jitter of osmux packets received. Specifically test case where the 2 Osmux
+ * packets arrive (almost) exactly at the same time, so no internal acton is
+ * triggered between receival of those. */
static void test_output_2together(void)
{
struct osmux_out_handle *h_output;
@@ -302,7 +314,8 @@
talloc_free(h_output);
}
-/* FIXME: this test shows generated rtp stream doesn't have osmux lost frames into account! */
+/* Generated rtp stream gets first RTP pkt marked with M bit after osmux frame
+ * lost is detected (hence a gap in sequence) */
static void test_output_frame_lost(void)
{
struct osmux_out_handle *h_output;
@@ -340,6 +353,8 @@
talloc_free(h_output);
}
+/* Test all packets are transmitted immediately when osmux_xfrm_output_flush()
+ * is called. */
static void test_output_flush(void)
{
struct osmux_out_handle *h_output;
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29504
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I56f27af52b6a8d798879c70d68a9a3b9e512867d
Gerrit-Change-Number: 29504
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29501 )
Change subject: osmux: Early return on error or batch full during osmux_replay_lost_packets()
......................................................................
osmux: Early return on error or batch full during osmux_replay_lost_packets()
If there's an error while replaying lost packets, return the error to
the caller.
If the batch is found full, early return indicating so, there's no need
to continue flow calling osmux_batch_enqueue() in osmux_batch_add()
again.
Change-Id: I62f494d2b2e7903a6683f6dea00781bb721a3d41
---
M src/osmux.c
1 file changed, 13 insertions(+), 7 deletions(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, but someone else must approve
laforge: Looks good to me, approved
diff --git a/src/osmux.c b/src/osmux.c
index bcf6e47..2a5dcc9 100644
--- a/src/osmux.c
+++ b/src/osmux.c
@@ -560,23 +560,24 @@
return amr_payload_len;
}
-static void osmux_replay_lost_packets(struct osmux_circuit *circuit,
+/* returns: 1 if batch is full, 0 if batch still not full, negative on error. */
+static int osmux_replay_lost_packets(struct osmux_circuit *circuit,
struct rtp_hdr *cur_rtph, int batch_factor)
{
int16_t diff;
struct msgb *last;
struct rtp_hdr *rtph;
- int i;
+ int i, rc;
/* Have we seen any RTP packet in this batch before? */
if (llist_empty(&circuit->msg_list))
- return;
+ return 0;
/* Get last RTP packet seen in this batch */
last = llist_entry(circuit->msg_list.prev, struct msgb, list);
rtph = osmo_rtp_get_hdr(last);
if (rtph == NULL)
- return;
+ return -1;
diff = ntohs(cur_rtph->sequence) - ntohs(rtph->sequence);
@@ -584,6 +585,7 @@
if (diff > 16)
diff = 16;
+ rc = 0;
/* If diff between last RTP packet seen and this one is > 1,
* then we lost several RTP packets, let's replay them.
*/
@@ -607,13 +609,15 @@
DELTA_RTP_TIMESTAMP);
/* No more room in this batch, skip padding with more clones */
- if (osmux_batch_enqueue(clone, circuit, batch_factor) != 0) {
+ rc = osmux_batch_enqueue(clone, circuit, batch_factor);
+ if (rc != 0) {
msgb_free(clone);
- break;
+ return rc;
}
LOGP(DLMUX, LOGL_ERROR, "adding cloned RTP\n");
}
+ return rc;
}
static struct osmux_circuit *
@@ -727,7 +731,9 @@
return 1;
/* Handle RTP packet loss scenario */
- osmux_replay_lost_packets(circuit, rtph, batch_factor);
+ rc = osmux_replay_lost_packets(circuit, rtph, batch_factor);
+ if (rc != 0)
+ return rc;
/* This batch is full, force batch delivery */
rc = osmux_batch_enqueue(msg, circuit, batch_factor);
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29501
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I62f494d2b2e7903a6683f6dea00781bb721a3d41
Gerrit-Change-Number: 29501
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29502 )
Change subject: osmux: assert no batch factor greater than 8 is used
......................................................................
osmux: assert no batch factor greater than 8 is used
Change-Id: Ie17a8174bc220d091cb7ff880363d22179b4f621
---
M src/osmux.c
1 file changed, 2 insertions(+), 1 deletion(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, but someone else must approve
laforge: Looks good to me, approved
diff --git a/src/osmux.c b/src/osmux.c
index 2a5dcc9..9ac0a7e 100644
--- a/src/osmux.c
+++ b/src/osmux.c
@@ -318,7 +318,8 @@
/* Validate amount of messages per batch. The counter field of the
* osmux header is just 3 bits long, so make sure it doesn't overflow.
*/
- if (circuit->nmsgs >= batch_factor || circuit->nmsgs >= 8) {
+ OSMO_ASSERT(batch_factor <= 8);
+ if (circuit->nmsgs >= batch_factor) {
struct rtp_hdr *rtph;
rtph = osmo_rtp_get_hdr(msg);
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29502
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Ie17a8174bc220d091cb7ff880363d22179b4f621
Gerrit-Change-Number: 29502
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29500 )
Change subject: osmux: Unify return codes of osmux_batch_add() and osmux_batch_enqueue()
......................................................................
osmux: Unify return codes of osmux_batch_add() and osmux_batch_enqueue()
This way it's way easier to return the error to the caller, and the code
is easier to read.
Change-Id: Ib78c9b82b85c74be2c5e94dae7c212175244d5fa
---
M src/osmux.c
1 file changed, 8 insertions(+), 4 deletions(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, but someone else must approve
laforge: Looks good to me, approved
diff --git a/src/osmux.c b/src/osmux.c
index 801372b..bcf6e47 100644
--- a/src/osmux.c
+++ b/src/osmux.c
@@ -311,6 +311,7 @@
int dummy;
};
+/* returns: 1 if batch is full, 0 if batch still not full, negative on error. */
static int osmux_batch_enqueue(struct msgb *msg, struct osmux_circuit *circuit,
uint8_t batch_factor)
{
@@ -325,7 +326,7 @@
return -1;
LOGP(DLMUX, LOGL_DEBUG, "Batch is full for RTP sssrc=%u\n", rtph->ssrc);
- return -1;
+ return 1;
}
llist_add_tail(&msg->list, &circuit->msg_list);
@@ -606,7 +607,7 @@
DELTA_RTP_TIMESTAMP);
/* No more room in this batch, skip padding with more clones */
- if (osmux_batch_enqueue(clone, circuit, batch_factor) < 0) {
+ if (osmux_batch_enqueue(clone, circuit, batch_factor) != 0) {
msgb_free(clone);
break;
}
@@ -668,6 +669,7 @@
talloc_free(circuit);
}
+/* returns: 1 if batch is full, 0 if batch still not full, negative on error. */
static int
osmux_batch_add(struct osmux_batch *batch, uint32_t batch_factor, struct msgb *msg,
struct rtp_hdr *rtph, int ccid)
@@ -675,6 +677,7 @@
int bytes = 0, amr_payload_len;
struct osmux_circuit *circuit;
struct msgb *cur;
+ int rc;
circuit = osmux_batch_find_circuit(batch, ccid);
if (!circuit)
@@ -727,8 +730,9 @@
osmux_replay_lost_packets(circuit, rtph, batch_factor);
/* This batch is full, force batch delivery */
- if (osmux_batch_enqueue(msg, circuit, batch_factor) < 0)
- return 1;
+ rc = osmux_batch_enqueue(msg, circuit, batch_factor);
+ if (rc != 0)
+ return rc;
#ifdef DEBUG_MSG
LOGP(DLMUX, LOGL_DEBUG, "adding msg with ssrc=%u to batch\n",
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29500
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Ib78c9b82b85c74be2c5e94dae7c212175244d5fa
Gerrit-Change-Number: 29500
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29499 )
Change subject: cosmetic: osmux: Fix typo in comment
......................................................................
cosmetic: osmux: Fix typo in comment
Change-Id: Ieeaa5543e56a824752413dadf161329f5ea0e4e7
---
M src/osmux.c
1 file changed, 2 insertions(+), 2 deletions(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, approved
laforge: Looks good to me, approved
diff --git a/src/osmux.c b/src/osmux.c
index bd05f76..801372b 100644
--- a/src/osmux.c
+++ b/src/osmux.c
@@ -567,7 +567,7 @@
struct rtp_hdr *rtph;
int i;
- /* Have we see any RTP packet in this batch before? */
+ /* Have we seen any RTP packet in this batch before? */
if (llist_empty(&circuit->msg_list))
return;
@@ -597,7 +597,7 @@
memcpy(clone->data, last->data, last->len);
msgb_put(clone, last->len);
- /* The original RTP message has been already sanity check. */
+ /* The original RTP message has been already sanity checked. */
rtph = osmo_rtp_get_hdr(clone);
/* Adjust sequence number and timestamp */
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29499
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Ieeaa5543e56a824752413dadf161329f5ea0e4e7
Gerrit-Change-Number: 29499
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged