November 2022 - gerrit-log - lists.osmocom.org

Change in libosmo-netif[master]: tests/osmux: Test big seqnum holes (>batch_factor) in incoming RTP st...

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30191 ) Change subject: tests/osmux: Test big seqnum holes (>batch_factor) in incoming RTP stream ...................................................................... tests/osmux: Test big seqnum holes (>batch_factor) in incoming RTP stream This test shows that there's 2 bugs in the osmux_input code: * It should be transmitting 2 osmux frames instead of 1 * Once it is fixed to transmitt 2 osmux frames, it should set the M bit of the 2nd generated osmux header after the seqnum jump in order to announce a jump in the stream to the peer receiving osmux. The bugs are fixed in follow-up patches. Related: SYS#6161 Change-Id: I521c2e97a739e8a824b16f06ec2a578333388247 --- M tests/osmux/osmux_input_test.c M tests/osmux/osmux_input_test.ok 2 files changed, 230 insertions(+), 1 deletion(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve osmith: Looks good to me, but someone else must approve pespin: Looks good to me, approved diff --git a/tests/osmux/osmux_input_test.c b/tests/osmux/osmux_input_test.c index aaab557..a728f6b 100644 --- a/tests/osmux/osmux_input_test.c +++ b/tests/osmux/osmux_input_test.c @@ -491,7 +491,7 @@ talloc_free(h_input); } -/* Test some scenario where some RTP packets are lost */ +/* Test some scenario where one RTP packet is lost */ static void test_rtp_pkt_gap_osmux_deliver_cb(struct msgb *batch_msg, void *data) { struct osmux_hdr *osmuxh; @@ -574,6 +574,122 @@ talloc_free(h_input); } +/* Test some scenario where some RTP packets (>batch_factor) are lost */ +static void test_rtp_pkt_gap_bigger_than_batch_factor_osmux_deliver_cb(struct msgb *batch_msg, void *data) +{ + struct osmux_hdr *osmuxh; + char buf[2048]; + unsigned *osmux_transmitted = (unsigned *)data; + (*osmux_transmitted)++; + + osmux_snprintf(buf, sizeof(buf), batch_msg); + clock_debug("OSMUX message %u (len=%d): %s\n", *osmux_transmitted, batch_msg->len, buf); + + /* We expect 1 batch: */ + osmuxh = osmux_xfrm_output_pull(batch_msg); + /* Check seqnum is the one configured beforehand: */ + OSMO_ASSERT(osmuxh->seq == (123 + *osmux_transmitted - 1)); + /* Check four AMR payloads appear: */ + OSMO_ASSERT(osmuxh->ctr == 3); + + /* Second osmux frame: is it expected to have the Marker bit set, since + * there was an incoming RTP jump between this resulting osmux frame and + * the previous one. */ + if (*osmux_transmitted == 2) + OSMO_ASSERT(osmuxh->rtp_m == 0); /* FIXME: This should be 1! Fix osmux_input.c code! */ + + osmuxh = osmux_xfrm_output_pull(batch_msg); + OSMO_ASSERT(osmuxh == NULL); + + msgb_free(batch_msg); +} +static void test_rtp_pkt_gap_bigger_than_batch_factor(uint16_t rtp_start_seqnum) +{ + struct msgb *msg; + int rc; + const uint8_t cid = 33; + unsigned osmux_transmitted = 0; + struct osmux_in_handle *h_input; + int i; + + printf("===%s(%" PRIu16 ")===\n", __func__, rtp_start_seqnum); + + clock_override_enable(true); + clock_override_set(0, 0); + rtp_init(rtp_start_seqnum, 18000); + + h_input = osmux_xfrm_input_alloc(tall_ctx); + osmux_xfrm_input_set_initial_seqnum(h_input, 123); + osmux_xfrm_input_set_batch_factor(h_input, 4); + osmux_xfrm_input_set_deliver_cb(h_input, + test_rtp_pkt_gap_bigger_than_batch_factor_osmux_deliver_cb, + &osmux_transmitted); + osmux_xfrm_input_open_circuit(h_input, cid, false); + + /* First RTP frame at t=0 */ + clock_debug("1st RTP packet is received"); + msg = rtp_next(); + rtp_append_amr(msg, AMR_FT_2); + rc = osmux_xfrm_input(h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + clock_debug("10 packets are lost"); + for (i = 0; i < 10; i++) { + clock_override_add(0, TIME_RTP_PKT_MS*1000); + msg = rtp_next(); + msgb_free(msg); + } + + clock_debug("12th RTP packet is received"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + msg = rtp_next(); + rtp_append_amr(msg, AMR_FT_2); + /* The recreation of lost pkts fills the batch and hence triggers transmission of osmux frame: */ + rc = osmux_xfrm_input(h_input, msg, cid); + OSMO_ASSERT(rc == 1); + /* We call deliver as requested by API: */ + osmux_xfrm_input_deliver(h_input); + OSMO_ASSERT(osmux_transmitted == 1); + /* We try to feed the RTP packet again as per API. This time 0 is + * received, pkt is stored in osmux layer. This is because we don't + * recreate entire batches with fake data. Instead, Osmux should use the + * Marker bit to announce the peer there was a (potentially huge) jump + * in seqnums. */ + rc = osmux_xfrm_input(h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + /* We continue filling the current batch so it gets transmitted: */ + clock_debug("12th+1 RTP packet is received"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + msg = rtp_next(); + rtp_append_amr(msg, AMR_FT_2); + rc = osmux_xfrm_input(h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + clock_debug("12th+2 RTP packet is received"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + msg = rtp_next(); + rtp_append_amr(msg, AMR_FT_2); + rc = osmux_xfrm_input(h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + clock_debug("12th+3 RTP packet is received"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + msg = rtp_next(); + rtp_append_amr(msg, AMR_FT_2); + rc = osmux_xfrm_input(h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + clock_debug("osmux batch transmitted"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + osmo_select_main(0); + OSMO_ASSERT(osmux_transmitted == 1); /* FIXME: this is a bug, should be 2! */ + + clock_debug("Closing circuit"); + osmux_xfrm_input_close_circuit(h_input, cid); + talloc_free(h_input); +} + int main(int argc, char **argv) { @@ -600,6 +716,11 @@ test_rtp_pkt_gap(65533); test_rtp_pkt_gap(65534); test_rtp_pkt_gap(65535); + test_rtp_pkt_gap_bigger_than_batch_factor(60); + /* Test several wraparound scenarios: */ + test_rtp_pkt_gap_bigger_than_batch_factor(65533); + test_rtp_pkt_gap_bigger_than_batch_factor(65534); + test_rtp_pkt_gap_bigger_than_batch_factor(65535); fprintf(stdout, "OK: Test passed\n"); return EXIT_SUCCESS; diff --git a/tests/osmux/osmux_input_test.ok b/tests/osmux/osmux_input_test.ok index 652a6c8..006d3f0 100644 --- a/tests/osmux/osmux_input_test.ok +++ b/tests/osmux/osmux_input_test.ok @@ -93,4 +93,112 @@ sys={0.080000}, mono={0.080000}: OSMUX message (len=64): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] sys={0.080000}, mono={0.080000}: Closing circuit +===test_rtp_pkt_gap_bigger_than_batch_factor(60)=== +sys={0.000000}, mono={0.000000}: clock_override_set +sys={0.000000}, mono={0.000000}: 1st RTP packet is received +sys={0.000000}, mono={0.000000}: 10 packets are lost +sys={0.020000}, mono={0.020000}: clock_override_add +sys={0.040000}, mono={0.040000}: clock_override_add +sys={0.060000}, mono={0.060000}: clock_override_add +sys={0.080000}, mono={0.080000}: clock_override_add +sys={0.100000}, mono={0.100000}: clock_override_add +sys={0.120000}, mono={0.120000}: clock_override_add +sys={0.140000}, mono={0.140000}: clock_override_add +sys={0.160000}, mono={0.160000}: clock_override_add +sys={0.180000}, mono={0.180000}: clock_override_add +sys={0.200000}, mono={0.200000}: clock_override_add +sys={0.200000}, mono={0.200000}: 12th RTP packet is received +sys={0.220000}, mono={0.220000}: clock_override_add +sys={0.220000}, mono={0.220000}: OSMUX message 1 (len=64): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + +sys={0.220000}, mono={0.220000}: 12th+1 RTP packet is received +sys={0.240000}, mono={0.240000}: clock_override_add +sys={0.240000}, mono={0.240000}: 12th+2 RTP packet is received +sys={0.260000}, mono={0.260000}: clock_override_add +sys={0.260000}, mono={0.260000}: 12th+3 RTP packet is received +sys={0.280000}, mono={0.280000}: clock_override_add +sys={0.280000}, mono={0.280000}: osmux batch transmitted +sys={0.300000}, mono={0.300000}: clock_override_add +sys={0.300000}, mono={0.300000}: Closing circuit +===test_rtp_pkt_gap_bigger_than_batch_factor(65533)=== +sys={0.000000}, mono={0.000000}: clock_override_set +sys={0.000000}, mono={0.000000}: 1st RTP packet is received +sys={0.000000}, mono={0.000000}: 10 packets are lost +sys={0.020000}, mono={0.020000}: clock_override_add +sys={0.040000}, mono={0.040000}: clock_override_add +sys={0.060000}, mono={0.060000}: clock_override_add +sys={0.080000}, mono={0.080000}: clock_override_add +sys={0.100000}, mono={0.100000}: clock_override_add +sys={0.120000}, mono={0.120000}: clock_override_add +sys={0.140000}, mono={0.140000}: clock_override_add +sys={0.160000}, mono={0.160000}: clock_override_add +sys={0.180000}, mono={0.180000}: clock_override_add +sys={0.200000}, mono={0.200000}: clock_override_add +sys={0.200000}, mono={0.200000}: 12th RTP packet is received +sys={0.220000}, mono={0.220000}: clock_override_add +sys={0.220000}, mono={0.220000}: OSMUX message 1 (len=64): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + +sys={0.220000}, mono={0.220000}: 12th+1 RTP packet is received +sys={0.240000}, mono={0.240000}: clock_override_add +sys={0.240000}, mono={0.240000}: 12th+2 RTP packet is received +sys={0.260000}, mono={0.260000}: clock_override_add +sys={0.260000}, mono={0.260000}: 12th+3 RTP packet is received +sys={0.280000}, mono={0.280000}: clock_override_add +sys={0.280000}, mono={0.280000}: osmux batch transmitted +sys={0.300000}, mono={0.300000}: clock_override_add +sys={0.300000}, mono={0.300000}: Closing circuit +===test_rtp_pkt_gap_bigger_than_batch_factor(65534)=== +sys={0.000000}, mono={0.000000}: clock_override_set +sys={0.000000}, mono={0.000000}: 1st RTP packet is received +sys={0.000000}, mono={0.000000}: 10 packets are lost +sys={0.020000}, mono={0.020000}: clock_override_add +sys={0.040000}, mono={0.040000}: clock_override_add +sys={0.060000}, mono={0.060000}: clock_override_add +sys={0.080000}, mono={0.080000}: clock_override_add +sys={0.100000}, mono={0.100000}: clock_override_add +sys={0.120000}, mono={0.120000}: clock_override_add +sys={0.140000}, mono={0.140000}: clock_override_add +sys={0.160000}, mono={0.160000}: clock_override_add +sys={0.180000}, mono={0.180000}: clock_override_add +sys={0.200000}, mono={0.200000}: clock_override_add +sys={0.200000}, mono={0.200000}: 12th RTP packet is received +sys={0.220000}, mono={0.220000}: clock_override_add +sys={0.220000}, mono={0.220000}: OSMUX message 1 (len=64): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + +sys={0.220000}, mono={0.220000}: 12th+1 RTP packet is received +sys={0.240000}, mono={0.240000}: clock_override_add +sys={0.240000}, mono={0.240000}: 12th+2 RTP packet is received +sys={0.260000}, mono={0.260000}: clock_override_add +sys={0.260000}, mono={0.260000}: 12th+3 RTP packet is received +sys={0.280000}, mono={0.280000}: clock_override_add +sys={0.280000}, mono={0.280000}: osmux batch transmitted +sys={0.300000}, mono={0.300000}: clock_override_add +sys={0.300000}, mono={0.300000}: Closing circuit +===test_rtp_pkt_gap_bigger_than_batch_factor(65535)=== +sys={0.000000}, mono={0.000000}: clock_override_set +sys={0.000000}, mono={0.000000}: 1st RTP packet is received +sys={0.000000}, mono={0.000000}: 10 packets are lost +sys={0.020000}, mono={0.020000}: clock_override_add +sys={0.040000}, mono={0.040000}: clock_override_add +sys={0.060000}, mono={0.060000}: clock_override_add +sys={0.080000}, mono={0.080000}: clock_override_add +sys={0.100000}, mono={0.100000}: clock_override_add +sys={0.120000}, mono={0.120000}: clock_override_add +sys={0.140000}, mono={0.140000}: clock_override_add +sys={0.160000}, mono={0.160000}: clock_override_add +sys={0.180000}, mono={0.180000}: clock_override_add +sys={0.200000}, mono={0.200000}: clock_override_add +sys={0.200000}, mono={0.200000}: 12th RTP packet is received +sys={0.220000}, mono={0.220000}: clock_override_add +sys={0.220000}, mono={0.220000}: OSMUX message 1 (len=64): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + +sys={0.220000}, mono={0.220000}: 12th+1 RTP packet is received +sys={0.240000}, mono={0.240000}: clock_override_add +sys={0.240000}, mono={0.240000}: 12th+2 RTP packet is received +sys={0.260000}, mono={0.260000}: clock_override_add +sys={0.260000}, mono={0.260000}: 12th+3 RTP packet is received +sys={0.280000}, mono={0.280000}: clock_override_add +sys={0.280000}, mono={0.280000}: osmux batch transmitted +sys={0.300000}, mono={0.300000}: clock_override_add +sys={0.300000}, mono={0.300000}: Closing circuit OK: Test passed -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30191 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I521c2e97a739e8a824b16f06ec2a578333388247 Gerrit-Change-Number: 30191 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 7 months

1
0
0 0

Change in libosmo-netif[master]: tests/osmux: Test replay of one lost RTP packet when generating osmux...

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30173 ) Change subject: tests/osmux: Test replay of one lost RTP packet when generating osmux batches ...................................................................... tests/osmux: Test replay of one lost RTP packet when generating osmux batches Change-Id: I913c2ccfc3ad4e7ed801344d64e33e166a0817cf --- M tests/osmux/osmux_input_test.c M tests/osmux/osmux_input_test.ok 2 files changed, 98 insertions(+), 0 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve osmith: Looks good to me, but someone else must approve pespin: Looks good to me, approved diff --git a/tests/osmux/osmux_input_test.c b/tests/osmux/osmux_input_test.c index 7a547d9..66335cd 100644 --- a/tests/osmux/osmux_input_test.c +++ b/tests/osmux/osmux_input_test.c @@ -491,6 +491,89 @@ talloc_free(h_input); } +/* Test some scenario where some RTP packets are lost */ +static void test_rtp_pkt_gap_osmux_deliver_cb(struct msgb *batch_msg, void *data) +{ + struct osmux_hdr *osmuxh; + char buf[2048]; + bool *osmux_transmitted = (bool *)data; + + osmux_snprintf(buf, sizeof(buf), batch_msg); + clock_debug("OSMUX message (len=%d): %s\n", batch_msg->len, buf); + + /* We expect 1 batch: */ + osmuxh = osmux_xfrm_output_pull(batch_msg); + /* Check seqnum is the one configured beforehand: */ + OSMO_ASSERT(osmuxh->seq == 123); + /* Check four AMR payloads appear: */ + OSMO_ASSERT(osmuxh->ctr == 3); + + osmuxh = osmux_xfrm_output_pull(batch_msg); + OSMO_ASSERT(osmuxh == NULL); + + msgb_free(batch_msg); + + *osmux_transmitted = true; +} +static void test_rtp_pkt_gap(void) +{ + struct msgb *msg; + int rc; + const uint8_t cid = 33; + bool osmux_transmitted = false; + struct osmux_in_handle *h_input; + + printf("===%s===\n", __func__); + + clock_override_enable(true); + clock_override_set(0, 0); + rtp_init(60, 18000); + + h_input = osmux_xfrm_input_alloc(tall_ctx); + osmux_xfrm_input_set_initial_seqnum(h_input, 123); + osmux_xfrm_input_set_batch_factor(h_input, 4); + osmux_xfrm_input_set_deliver_cb(h_input, + test_rtp_pkt_gap_osmux_deliver_cb, + &osmux_transmitted); + osmux_xfrm_input_open_circuit(h_input, cid, false); + + /* First RTP frame at t=0 */ + clock_debug("1st RTP packet is received"); + msg = rtp_next(); + rtp_append_amr(msg, AMR_FT_2); + rc = osmux_xfrm_input(h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + clock_debug("2nd RTP packet is lost"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + msg = rtp_next(); + msgb_free(msg); + + clock_debug("3rd RTP packet is received"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + msg = rtp_next(); + rtp_append_amr(msg, AMR_FT_2); + rc = osmux_xfrm_input(h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + clock_debug("4th RTP packet is received"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + msg = rtp_next(); + rtp_append_amr(msg, AMR_FT_2); + rc = osmux_xfrm_input(h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + /* t=80, osmux batch is scheduled to be transmitted: */ + clock_debug("osmux batch transmitted"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + osmo_select_main(0); + OSMO_ASSERT(osmux_transmitted == true); + + clock_debug("Closing circuit"); + osmux_xfrm_input_close_circuit(h_input, cid); + talloc_free(h_input); +} + int main(int argc, char **argv) { @@ -512,6 +595,7 @@ test_last_amr_cmr_f_q_used(); test_initial_osmux_seqnum(); test_rtp_dup(); + test_rtp_pkt_gap(); fprintf(stdout, "OK: Test passed\n"); return EXIT_SUCCESS; diff --git a/tests/osmux/osmux_input_test.ok b/tests/osmux/osmux_input_test.ok index b5b2f00..d119b62 100644 --- a/tests/osmux/osmux_input_test.ok +++ b/tests/osmux/osmux_input_test.ok @@ -37,4 +37,18 @@ sys={0.040000}, mono={0.040000}: OSMUX message (len=19): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=0 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 12 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] sys={0.040000}, mono={0.040000}: Closing circuit +===test_rtp_pkt_gap=== +sys={0.000000}, mono={0.000000}: clock_override_set +sys={0.000000}, mono={0.000000}: 1st RTP packet is received +sys={0.000000}, mono={0.000000}: 2nd RTP packet is lost +sys={0.020000}, mono={0.020000}: clock_override_add +sys={0.020000}, mono={0.020000}: 3rd RTP packet is received +sys={0.040000}, mono={0.040000}: clock_override_add +sys={0.040000}, mono={0.040000}: 4th RTP packet is received +sys={0.060000}, mono={0.060000}: clock_override_add +sys={0.060000}, mono={0.060000}: osmux batch transmitted +sys={0.080000}, mono={0.080000}: clock_override_add +sys={0.080000}, mono={0.080000}: OSMUX message (len=64): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + +sys={0.080000}, mono={0.080000}: Closing circuit OK: Test passed -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30173 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I913c2ccfc3ad4e7ed801344d64e33e166a0817cf Gerrit-Change-Number: 30173 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 7 months

1
0
0 0

Change in libosmo-netif[master]: tests/osmux: Test incoming RTP stream with seqnum jumps during wrapar...

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30184 ) Change subject: tests/osmux: Test incoming RTP stream with seqnum jumps during wraparound ...................................................................... tests/osmux: Test incoming RTP stream with seqnum jumps during wraparound Related: SYS#6161 Change-Id: Idd3a609452767f9c480c7c5ba9fc2a3e0a499968 --- M tests/osmux/osmux_input_test.c M tests/osmux/osmux_input_test.ok 2 files changed, 51 insertions(+), 5 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve pespin: Looks good to me, approved osmith: Looks good to me, but someone else must approve diff --git a/tests/osmux/osmux_input_test.c b/tests/osmux/osmux_input_test.c index 66335cd..aaab557 100644 --- a/tests/osmux/osmux_input_test.c +++ b/tests/osmux/osmux_input_test.c @@ -515,7 +515,7 @@ *osmux_transmitted = true; } -static void test_rtp_pkt_gap(void) +static void test_rtp_pkt_gap(uint16_t rtp_start_seqnum) { struct msgb *msg; int rc; @@ -523,11 +523,11 @@ bool osmux_transmitted = false; struct osmux_in_handle *h_input; - printf("===%s===\n", __func__); + printf("===%s(%" PRIu16 ")===\n", __func__, rtp_start_seqnum); clock_override_enable(true); clock_override_set(0, 0); - rtp_init(60, 18000); + rtp_init(rtp_start_seqnum, 18000); h_input = osmux_xfrm_input_alloc(tall_ctx); osmux_xfrm_input_set_initial_seqnum(h_input, 123); @@ -595,7 +595,11 @@ test_last_amr_cmr_f_q_used(); test_initial_osmux_seqnum(); test_rtp_dup(); - test_rtp_pkt_gap(); + test_rtp_pkt_gap(60); + /* Test several wraparound scenarios: */ + test_rtp_pkt_gap(65533); + test_rtp_pkt_gap(65534); + test_rtp_pkt_gap(65535); fprintf(stdout, "OK: Test passed\n"); return EXIT_SUCCESS; diff --git a/tests/osmux/osmux_input_test.ok b/tests/osmux/osmux_input_test.ok index d119b62..652a6c8 100644 --- a/tests/osmux/osmux_input_test.ok +++ b/tests/osmux/osmux_input_test.ok @@ -37,7 +37,49 @@ sys={0.040000}, mono={0.040000}: OSMUX message (len=19): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=0 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 12 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] sys={0.040000}, mono={0.040000}: Closing circuit -===test_rtp_pkt_gap=== +===test_rtp_pkt_gap(60)=== +sys={0.000000}, mono={0.000000}: clock_override_set +sys={0.000000}, mono={0.000000}: 1st RTP packet is received +sys={0.000000}, mono={0.000000}: 2nd RTP packet is lost +sys={0.020000}, mono={0.020000}: clock_override_add +sys={0.020000}, mono={0.020000}: 3rd RTP packet is received +sys={0.040000}, mono={0.040000}: clock_override_add +sys={0.040000}, mono={0.040000}: 4th RTP packet is received +sys={0.060000}, mono={0.060000}: clock_override_add +sys={0.060000}, mono={0.060000}: osmux batch transmitted +sys={0.080000}, mono={0.080000}: clock_override_add +sys={0.080000}, mono={0.080000}: OSMUX message (len=64): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + +sys={0.080000}, mono={0.080000}: Closing circuit +===test_rtp_pkt_gap(65533)=== +sys={0.000000}, mono={0.000000}: clock_override_set +sys={0.000000}, mono={0.000000}: 1st RTP packet is received +sys={0.000000}, mono={0.000000}: 2nd RTP packet is lost +sys={0.020000}, mono={0.020000}: clock_override_add +sys={0.020000}, mono={0.020000}: 3rd RTP packet is received +sys={0.040000}, mono={0.040000}: clock_override_add +sys={0.040000}, mono={0.040000}: 4th RTP packet is received +sys={0.060000}, mono={0.060000}: clock_override_add +sys={0.060000}, mono={0.060000}: osmux batch transmitted +sys={0.080000}, mono={0.080000}: clock_override_add +sys={0.080000}, mono={0.080000}: OSMUX message (len=64): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + +sys={0.080000}, mono={0.080000}: Closing circuit +===test_rtp_pkt_gap(65534)=== +sys={0.000000}, mono={0.000000}: clock_override_set +sys={0.000000}, mono={0.000000}: 1st RTP packet is received +sys={0.000000}, mono={0.000000}: 2nd RTP packet is lost +sys={0.020000}, mono={0.020000}: clock_override_add +sys={0.020000}, mono={0.020000}: 3rd RTP packet is received +sys={0.040000}, mono={0.040000}: clock_override_add +sys={0.040000}, mono={0.040000}: 4th RTP packet is received +sys={0.060000}, mono={0.060000}: clock_override_add +sys={0.060000}, mono={0.060000}: osmux batch transmitted +sys={0.080000}, mono={0.080000}: clock_override_add +sys={0.080000}, mono={0.080000}: OSMUX message (len=64): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=3 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + +sys={0.080000}, mono={0.080000}: Closing circuit +===test_rtp_pkt_gap(65535)=== sys={0.000000}, mono={0.000000}: clock_override_set sys={0.000000}, mono={0.000000}: 1st RTP packet is received sys={0.000000}, mono={0.000000}: 2nd RTP packet is lost -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30184 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: Idd3a609452767f9c480c7c5ba9fc2a3e0a499968 Gerrit-Change-Number: 30184 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 7 months

1
0
0 0

Change in libosmo-netif[master]: osmux: Drop noop OR during assignment

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30190 ) Change subject: osmux: Drop noop OR during assignment ...................................................................... osmux: Drop noop OR during assignment osmuxh->rtp_m is zeroed at that point, it makes no sense to bit-OR at that point. Change-Id: I09c920035b1aa1af139a2a63f7cb4a3390bb9d36 --- M src/osmux_input.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/osmux_input.c b/src/osmux_input.c index 866588f..a50a179 100644 --- a/src/osmux_input.c +++ b/src/osmux_input.c @@ -154,7 +154,7 @@ sizeof(struct osmux_hdr)); osmuxh->ft = OSMUX_FT_VOICE_AMR; osmuxh->ctr = 0; - osmuxh->rtp_m = osmuxh->rtp_m || state->rtph->marker; + osmuxh->rtp_m = state->rtph->marker; osmuxh->seq = state->circuit->seq++; osmuxh->circuit_id = state->circuit->ccid; osmuxh->amr_ft = state->amrh->ft; -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30190 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I09c920035b1aa1af139a2a63f7cb4a3390bb9d36 Gerrit-Change-Number: 30190 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 7 months

1
0
0 0

Change in libosmo-netif[master]: osmux: Add data[0] field to osmux_hdr

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30172 ) Change subject: osmux: Add data[0] field to osmux_hdr ...................................................................... osmux: Add data[0] field to osmux_hdr This allows easy access to osmux_hdr payload. Change-Id: I27750c38f12d3d84dbaac5beff166d78ffa6c45b --- M TODO-RELEASE M include/osmocom/netif/osmux.h 2 files changed, 2 insertions(+), 0 deletions(-) Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve laforge: Looks good to me, approved diff --git a/TODO-RELEASE b/TODO-RELEASE index 5629a9c..55371fd 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -9,4 +9,5 @@ #library what description / commit summary line libosmo-netif osmux new osmux_xfrm_output_* APIs libosmo-netif osmux new osmux_xfrm_input_* APIs +libosmo-netif osmux new struct osmux_hdr->data[0] field libosmo-netif stream new APIs osmo_stream_{cli,srv}_clear_tx_queue() \ No newline at end of file diff --git a/include/osmocom/netif/osmux.h b/include/osmocom/netif/osmux.h index f7a5fd5..b6014d5 100644 --- a/include/osmocom/netif/osmux.h +++ b/include/osmocom/netif/osmux.h @@ -53,6 +53,7 @@ /* auto-generated from the little endian part above (libosmocore/contrib/struct_endianess.py) */ uint8_t amr_ft:4, amr_cmr:4; #endif + uint8_t data[0]; } __attribute__((packed)); /* one to handle all existing RTP flows */ -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30172 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I27750c38f12d3d84dbaac5beff166d78ffa6c45b Gerrit-Change-Number: 30172 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 7 months

1
0
0 0

Change in libosmo-netif[master]: osmux: Drop marker bit in forged RTP packets to fill gaps

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30170 ) Change subject: osmux: Drop marker bit in forged RTP packets to fill gaps ...................................................................... osmux: Drop marker bit in forged RTP packets to fill gaps If the last RTP we received from the user is a Marker bit (which is expected to be the first in the batch), then there's no need to keep marking the forged RTP packets to fill holes also as Marker bit. Change-Id: I5cef6185afbfce748473a096e8ebabd9c9628e12 --- M src/osmux_input.c 1 file changed, 2 insertions(+), 1 deletion(-) Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve laforge: Looks good to me, approved diff --git a/src/osmux_input.c b/src/osmux_input.c index 3a85b45..866588f 100644 --- a/src/osmux_input.c +++ b/src/osmux_input.c @@ -401,7 +401,8 @@ /* The original RTP message has been already sanity checked. */ rtph = osmo_rtp_get_hdr(clone); - + /* Faking a follow up RTP pkt here, so no Marker bit: */ + rtph->marker = false; /* Adjust sequence number and timestamp */ rtph->sequence = htons(ntohs(rtph->sequence) + i); rtph->timestamp = htonl(ntohl(rtph->timestamp) + -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30170 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I5cef6185afbfce748473a096e8ebabd9c9628e12 Gerrit-Change-Number: 30170 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 7 months

1
0
0 0

Change in libosmo-netif[master]: osmux: recreate lost RTP pkts before handling newest one

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30169 ) Change subject: osmux: recreate lost RTP pkts before handling newest one ...................................................................... osmux: recreate lost RTP pkts before handling newest one In the event we need to fill in the batch with intermediate lost RTP packets, we must do so before handling the last one. Change-Id: Ib5dd7b1fa6213c96ece803b781a7ef1cf102a1d4 --- M src/osmux_input.c 1 file changed, 5 insertions(+), 5 deletions(-) Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve laforge: Looks good to me, approved diff --git a/src/osmux_input.c b/src/osmux_input.c index bdc7fd3..3a85b45 100644 --- a/src/osmux_input.c +++ b/src/osmux_input.c @@ -479,6 +479,11 @@ } } + /* Handle RTP packet loss scenario */ + rc = osmux_replay_lost_packets(link, req); + if (rc != 0) + return rc; + /* Init of talkspurt (RTP M marker bit) needs to be in the first AMR slot * of the OSMUX packet, enforce sending previous batch if required: */ @@ -499,11 +504,6 @@ if (needed_bytes > link->remaining_bytes) return 1; - /* Handle RTP packet loss scenario */ - rc = osmux_replay_lost_packets(link, req); - if (rc != 0) - return rc; - /* This batch is full, force batch delivery */ rc = osmux_circuit_enqueue(link, req->circuit, req->msg); if (rc != 0) -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30169 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: Ib5dd7b1fa6213c96ece803b781a7ef1cf102a1d4 Gerrit-Change-Number: 30169 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 7 months

1
0
0 0

Change in libosmo-netif[master]: osmux: dup in RTP pkt: check before applying queue flush due to Marke...

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30167 ) Change subject: osmux: dup in RTP pkt: check before applying queue flush due to Marker bit ...................................................................... osmux: dup in RTP pkt: check before applying queue flush due to Marker bit We need to filter duplicate messages before checking the Mark bit and returning 1 to tell the user to deliver the current batchset content. Otherwise, a repeated RTP packet with an M bit would trigger delivery, which is wrong. Change-Id: I4a01b0935f112d650d8fc161b3389eda6c8e75ec --- M src/osmux_input.c 1 file changed, 6 insertions(+), 6 deletions(-) Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve laforge: Looks good to me, approved diff --git a/src/osmux_input.c b/src/osmux_input.c index 657abaf..10598d2 100644 --- a/src/osmux_input.c +++ b/src/osmux_input.c @@ -452,12 +452,6 @@ link->ndummy--; } - /* Init of talkspurt (RTP M marker bit) needs to be in the first AMR slot - * of the OSMUX packet, enforce sending previous batch if required: - */ - if (req->rtph->marker && req->circuit->nmsgs != 0) - return 1; - /* Extra validation: check if this message already exists, should not * happen but make sure we don't propagate duplicated messages. */ @@ -473,6 +467,12 @@ } } + /* Init of talkspurt (RTP M marker bit) needs to be in the first AMR slot + * of the OSMUX packet, enforce sending previous batch if required: + */ + if (req->rtph->marker && req->circuit->nmsgs != 0) + return 1; + /* First check if there is room for this message in the batch */ /* First in batch comes after the batch header: */ if (req->circuit->nmsgs == 0) -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30167 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I4a01b0935f112d650d8fc161b3389eda6c8e75ec Gerrit-Change-Number: 30167 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 7 months

1
0
0 0

Change in libosmo-netif[master]: osmux: dup in RTP pkt: Replace potentially internally forged pkt with...

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30168 ) Change subject: osmux: dup in RTP pkt: Replace potentially internally forged pkt with incoming one ...................................................................... osmux: dup in RTP pkt: Replace potentially internally forged pkt with incoming one When RTP packet provided by user to osmux layer, it may contain a seq gap, and osmux will refill the packets to avoid losing that information on the other side when it receives the batch. If out of order UDP packets are received, it can happen that we first detect a gap and later on we receive the previous RTP packet, which we is then detected as duplicate because it was previously forged to fill the hole. In that case, let's better keep the incoming packet instead of the potentially internall forged one which doesn't contain the real AMR payload. Related: SYS#6161 Change-Id: I82e11ef3dcd20ffea33c94ed83abcedf0f186871 --- M src/osmux_input.c M tests/osmux/osmux_input_test.c M tests/osmux/osmux_input_test.ok 3 files changed, 106 insertions(+), 6 deletions(-) Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve laforge: Looks good to me, approved diff --git a/src/osmux_input.c b/src/osmux_input.c index 10598d2..bdc7fd3 100644 --- a/src/osmux_input.c +++ b/src/osmux_input.c @@ -442,7 +442,7 @@ static int osmux_link_add(struct osmux_link *link, const struct osmux_in_req *req) { - struct msgb *cur; + struct msgb *cur, *next; int rc; unsigned int needed_bytes = 0; @@ -455,15 +455,27 @@ /* Extra validation: check if this message already exists, should not * happen but make sure we don't propagate duplicated messages. */ - llist_for_each_entry(cur, &req->circuit->msg_list, list) { + llist_for_each_entry_safe(cur, next, &req->circuit->msg_list, list) { struct rtp_hdr *rtph2 = osmo_rtp_get_hdr(cur); OSMO_ASSERT(rtph2); - /* Already exists message with this sequence, skip */ + /* Already exists message with this sequence. Let's copy over + * the new RTP, since there's the chance that the existing one may + * be a forged copy we did when we detected a hole. */ if (rtph2->sequence == req->rtph->sequence) { - LOGP(DLMUX, LOGL_ERROR, "RTP pkt with seq=%u already exists, skip it\n", - ntohs(req->rtph->sequence)); - return -1; + if (msgb_length(cur) != msgb_length(req->msg)) { + /* Different packet size, AMR FT may have changed. Let's avoid changing it to + * break accounted size to be written (would need new osmux_hdr, etc.) */ + LOGP(DLMUX, LOGL_NOTICE, "RTP pkt with seq=%u and different len %u != %u already exists, skip it\n", + ntohs(req->rtph->sequence), msgb_length(cur), msgb_length(req->msg)); + return -1; + } + LOGP(DLMUX, LOGL_INFO, "RTP pkt with seq=%u already exists, replace it\n", + ntohs(req->rtph->sequence)); + __llist_add(&req->msg->list, &cur->list, cur->list.next); + llist_del(&cur->list); + msgb_free(cur); + return 0; } } diff --git a/tests/osmux/osmux_input_test.c b/tests/osmux/osmux_input_test.c index 8645929..7a547d9 100644 --- a/tests/osmux/osmux_input_test.c +++ b/tests/osmux/osmux_input_test.c @@ -413,6 +413,84 @@ talloc_free(h_input); } +static void test_rtp_dup_osmux_deliver_cb(struct msgb *batch_msg, void *data) +{ + struct osmux_hdr *osmuxh; + char buf[2048]; + uint8_t *osmux_pl; + bool *osmux_transmitted = (bool *)data; + + osmux_snprintf(buf, sizeof(buf), batch_msg); + clock_debug("OSMUX message (len=%d): %s\n", batch_msg->len, buf); + + /* We expect 1 batch: */ + osmuxh = osmux_xfrm_output_pull(batch_msg); + /* Check seqnum is the one configured beforehand: */ + OSMO_ASSERT(osmuxh->seq == 123); + osmux_pl = (uint8_t *)osmuxh + sizeof(*osmuxh); + OSMO_ASSERT(osmux_pl[0] == 0x12); + + osmuxh = osmux_xfrm_output_pull(batch_msg); + OSMO_ASSERT(osmuxh == NULL); + + msgb_free(batch_msg); + + *osmux_transmitted = true; +} +/* Test user pushes duplicated RTP (dup seqnum) to osmux: */ +static void test_rtp_dup(void) +{ + struct msgb *msg, *msg_dup; + struct amr_hdr *amrh; + int rc; + const uint8_t cid = 33; + bool osmux_transmitted = false; + struct osmux_in_handle *h_input; + + printf("===%s===\n", __func__); + + + + clock_override_enable(true); + clock_override_set(0, 0); + rtp_init(0, 0); + + h_input = osmux_xfrm_input_alloc(tall_ctx); + osmux_xfrm_input_set_initial_seqnum(h_input, 123); + osmux_xfrm_input_set_batch_factor(h_input, 2); + osmux_xfrm_input_set_deliver_cb(h_input, + test_rtp_dup_osmux_deliver_cb, + &osmux_transmitted); + osmux_xfrm_input_open_circuit(h_input, cid, false); + + /* First RTP frame at t=0 */ + msg = rtp_next(); + msg_dup = msgb_copy(msg, "dup"); + rtp_append_amr(msg, AMR_FT_2); + rc = osmux_xfrm_input(h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + clock_debug("Submit 2nd RTP packet, seqnum dup"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + amrh = rtp_append_amr(msg_dup, AMR_FT_2); + amrh->data[0] = 0x12; /* Change AMR payload to check it is updated. */ + rc = osmux_xfrm_input(h_input, msg_dup, cid); + OSMO_ASSERT(rc == 0); + OSMO_ASSERT(osmux_transmitted == false); + + /* t=60, osmux batch is scheduled to be transmitted: */ + clock_debug("Submit 3rd RTP packet, triggers osmux batch"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + msg = rtp_next(); + rtp_append_amr(msg, AMR_FT_2); + osmo_select_main(0); + OSMO_ASSERT(osmux_transmitted == true); + + clock_debug("Closing circuit"); + osmux_xfrm_input_close_circuit(h_input, cid); + talloc_free(h_input); +} + int main(int argc, char **argv) { @@ -433,6 +511,7 @@ test_amr_ft_change_middle_batch(); test_last_amr_cmr_f_q_used(); test_initial_osmux_seqnum(); + test_rtp_dup(); fprintf(stdout, "OK: Test passed\n"); return EXIT_SUCCESS; diff --git a/tests/osmux/osmux_input_test.ok b/tests/osmux/osmux_input_test.ok index 60daf26..b5b2f00 100644 --- a/tests/osmux/osmux_input_test.ok +++ b/tests/osmux/osmux_input_test.ok @@ -28,4 +28,13 @@ sys={0.020000}, mono={0.020000}: OSMUX message (len=19): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=0 amr_f=1 amr_q=1 amr_ft=02 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] sys={0.020000}, mono={0.020000}: Closing circuit +===test_rtp_dup=== +sys={0.000000}, mono={0.000000}: clock_override_set +sys={0.000000}, mono={0.000000}: Submit 2nd RTP packet, seqnum dup +sys={0.020000}, mono={0.020000}: clock_override_add +sys={0.020000}, mono={0.020000}: Submit 3rd RTP packet, triggers osmux batch +sys={0.040000}, mono={0.040000}: clock_override_add +sys={0.040000}, mono={0.040000}: OSMUX message (len=19): OSMUX seq=123 ccid=033 ft=1 rtp_m=0 ctr=0 amr_f=0 amr_q=1 amr_ft=02 amr_cmr=00 [ 12 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ] + +sys={0.040000}, mono={0.040000}: Closing circuit OK: Test passed -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30168 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I82e11ef3dcd20ffea33c94ed83abcedf0f186871 Gerrit-Change-Number: 30168 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 7 months

1
0
0 0

Change in libosmo-netif[master]: osmux: Use internal struct to cache parsing state of rtp pkt from user

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/30166 ) Change subject: osmux: Use internal struct to cache parsing state of rtp pkt from user ...................................................................... osmux: Use internal struct to cache parsing state of rtp pkt from user This allows incrementally gathering all the information once and only once. While doing so, this patch tends to move the decoding/parsing of the incoming RTP packet further towards the start of the code, hence detecing errors in the input data early in the process and avoiding touching internal state in this case. Change-Id: I55e0d2772e7054c0d734f5918c6938a5c8a6e781 --- M src/osmux_input.c 1 file changed, 64 insertions(+), 56 deletions(-) Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve laforge: Looks good to me, but someone else must approve pespin: Looks good to me, approved diff --git a/src/osmux_input.c b/src/osmux_input.c index fb246a5..657abaf 100644 --- a/src/osmux_input.c +++ b/src/osmux_input.c @@ -87,6 +87,16 @@ uint8_t seq; }; +/* Used internally to temporarily cache all parsed content of an RTP pkt from user to be transmitted as Osmux */ +struct osmux_in_req { + struct osmux_circuit *circuit; + struct msgb *msg; + struct rtp_hdr *rtph; + uint32_t rtp_payload_len; + struct amr_hdr *amrh; + int amr_payload_len; +}; + /* returns: 1 if batch is full, 0 if batch still not full, negative on error. */ static int osmux_circuit_enqueue(struct osmux_link *link, struct osmux_circuit *circuit, struct msgb *msg) { @@ -344,8 +354,7 @@ } /* returns: 1 if batch is full, 0 if batch still not full, negative on error. */ -static int osmux_replay_lost_packets(struct osmux_link *link, struct osmux_circuit *circuit, - struct rtp_hdr *cur_rtph) +static int osmux_replay_lost_packets(struct osmux_link *link, const struct osmux_in_req *req) { int16_t diff; struct msgb *last; @@ -354,16 +363,16 @@ int i, rc; /* Have we seen any RTP packet in this batch before? */ - if (llist_empty(&circuit->msg_list)) + if (llist_empty(&req->circuit->msg_list)) return 0; /* Get last RTP packet seen in this batch */ - last = llist_entry(circuit->msg_list.prev, struct msgb, list); + last = llist_entry(req->circuit->msg_list.prev, struct msgb, list); last_rtph = osmo_rtp_get_hdr(last); if (last_rtph == NULL) return -1; last_seq = ntohs(last_rtph->sequence); - cur_seq = ntohs(cur_rtph->sequence); + cur_seq = ntohs(req->rtph->sequence); diff = cur_seq - last_seq; /* If diff between last RTP packet seen and this one is > 1, @@ -399,7 +408,7 @@ DELTA_RTP_TIMESTAMP); /* No more room in this batch, skip padding with more clones */ - rc = osmux_circuit_enqueue(link, circuit, clone); + rc = osmux_circuit_enqueue(link, req->circuit, clone); if (rc != 0) { msgb_free(clone); return rc; @@ -431,78 +440,60 @@ /* returns: 1 if batch is full, 0 if batch still not full, negative on error. */ static int -osmux_link_add(struct osmux_link *link, struct msgb *msg, - struct rtp_hdr *rtph, int ccid) +osmux_link_add(struct osmux_link *link, const struct osmux_in_req *req) { - int bytes = 0, amr_payload_len; - struct osmux_circuit *circuit; struct msgb *cur; int rc; - struct amr_hdr *amrh; - uint32_t amr_len; - - circuit = osmux_link_find_circuit(link, ccid); - if (!circuit) - return -1; + unsigned int needed_bytes = 0; /* We've seen the first RTP message, disable dummy padding */ - if (circuit->dummy) { - circuit->dummy = 0; + if (req->circuit->dummy) { + req->circuit->dummy = 0; link->ndummy--; } - amrh = osmo_rtp_get_payload(rtph, msg, &amr_len); - if (amrh == NULL) - return -1; - amr_payload_len = osmux_rtp_amr_payload_len(amrh, amr_len); - if (amr_payload_len < 0) { - LOGP(DLMUX, LOGL_NOTICE, "AMR payload invalid\n"); - return -1; - } - /* Init of talkspurt (RTP M marker bit) needs to be in the first AMR slot * of the OSMUX packet, enforce sending previous batch if required: */ - if (rtph->marker && circuit->nmsgs != 0) + if (req->rtph->marker && req->circuit->nmsgs != 0) return 1; /* Extra validation: check if this message already exists, should not * happen but make sure we don't propagate duplicated messages. */ - llist_for_each_entry(cur, &circuit->msg_list, list) { + llist_for_each_entry(cur, &req->circuit->msg_list, list) { struct rtp_hdr *rtph2 = osmo_rtp_get_hdr(cur); - if (rtph2 == NULL) - return -1; + OSMO_ASSERT(rtph2); /* Already exists message with this sequence, skip */ - if (rtph2->sequence == rtph->sequence) { + if (rtph2->sequence == req->rtph->sequence) { LOGP(DLMUX, LOGL_ERROR, "RTP pkt with seq=%u already exists, skip it\n", - ntohs(rtph->sequence)); + ntohs(req->rtph->sequence)); return -1; } } /* First check if there is room for this message in the batch */ /* First in batch comes after the batch header: */ - if (circuit->nmsgs == 0) - bytes += sizeof(struct osmux_hdr); + if (req->circuit->nmsgs == 0) + needed_bytes += sizeof(struct osmux_hdr); /* If AMR FT changes in the middle of current batch a new header is * required to adapt to size change: */ - else if (osmux_circuit_get_last_stored_amr_ft(circuit) != amrh->ft) - bytes += sizeof(struct osmux_hdr); - bytes += amr_payload_len; + else if (osmux_circuit_get_last_stored_amr_ft(req->circuit) != req->amrh->ft) + needed_bytes += sizeof(struct osmux_hdr); + needed_bytes += req->amr_payload_len; /* No room, sorry. You'll have to retry */ - if (bytes > link->remaining_bytes) + if (needed_bytes > link->remaining_bytes) return 1; /* Handle RTP packet loss scenario */ - rc = osmux_replay_lost_packets(link, circuit, rtph); + rc = osmux_replay_lost_packets(link, req); if (rc != 0) return rc; /* This batch is full, force batch delivery */ - rc = osmux_circuit_enqueue(link, circuit, msg); + rc = osmux_circuit_enqueue(link, req->circuit, req->msg); if (rc != 0) return rc; @@ -512,7 +503,7 @@ #endif /* Update remaining room in this batch */ - link->remaining_bytes -= bytes; + link->remaining_bytes -= needed_bytes; if (link->nmsgs == 0) { #ifdef DEBUG_MSG @@ -542,8 +533,22 @@ int osmux_xfrm_input(struct osmux_in_handle *h, struct msgb *msg, int ccid) { int ret; - struct rtp_hdr *rtph; struct osmux_link *link = (struct osmux_link *)h->internal_data; + struct osmux_in_req req = { + .msg = msg, + .rtph = osmo_rtp_get_hdr(msg), + .circuit = osmux_link_find_circuit(link, ccid), + }; + + if (!req.circuit) { + LOGP(DLMUX, LOGL_INFO, "Couldn't find circuit CID=%u\n", ccid); + goto err_free; + } + + if (!req.rtph) { + LOGP(DLMUX, LOGL_NOTICE, "msg not containing an RTP header\n"); + goto err_free; + } /* Ignore too big RTP/RTCP messages, most likely forged. Sanity check * to avoid a possible forever loop in the caller. @@ -551,18 +556,10 @@ if (msg->len > h->batch_size - sizeof(struct osmux_hdr)) { LOGP(DLMUX, LOGL_NOTICE, "RTP payload too big (%u) for configured batch size (%u)\n", msg->len, h->batch_size); - msgb_free(msg); - return -1; + goto err_free; } - rtph = osmo_rtp_get_hdr(msg); - if (rtph == NULL) { - LOGP(DLMUX, LOGL_NOTICE, "msg not containing an RTP header\n"); - msgb_free(msg); - return -1; - } - - switch (rtph->payload_type) { + switch (req.rtph->payload_type) { case RTP_PT_RTCP: LOGP(DLMUX, LOGL_INFO, "Dropping RTCP packet\n"); msgb_free(msg); @@ -572,17 +569,24 @@ * although we use static ones. Assume that we always * receive AMR traffic. */ + req.amrh = osmo_rtp_get_payload(req.rtph, req.msg, &req.rtp_payload_len); + if (req.amrh == NULL) + goto err_free; + req.amr_payload_len = osmux_rtp_amr_payload_len(req.amrh, req.rtp_payload_len); + if (req.amr_payload_len < 0) { + LOGP(DLMUX, LOGL_NOTICE, "AMR payload invalid\n"); + goto err_free; + } /* Add this RTP to the OSMUX batch */ - ret = osmux_link_add(link, msg, rtph, ccid); + ret = osmux_link_add(link, &req); if (ret < 0) { /* Cannot put this message into the batch. * Malformed, duplicated, OOM. Drop it and tell * the upper layer that we have digest it. */ LOGP(DLMUX, LOGL_DEBUG, "Dropping RTP packet instead of adding to batch\n"); - msgb_free(msg); - return ret; + goto err_free; } h->stats.input_rtp_msgs++; @@ -590,6 +594,10 @@ break; } return ret; + +err_free: + msgb_free(msg); + return -1; } static int osmux_xfrm_input_talloc_destructor(struct osmux_in_handle *h) -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/30166 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I55e0d2772e7054c0d734f5918c6938a5c8a6e781 Gerrit-Change-Number: 30166 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 7 months

1
0
0 0

2025

2024

2023

2022

gerrit-log November 2022