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

Change in libosmo-netif[master]: tests/osmux: Properly flush and free out_handle in osmux_test

by fixeria

Attention is currently required from: pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29519 ) Change subject: tests/osmux: Properly flush and free out_handle in osmux_test ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29519 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: Ia86e4324e21ccc4bd4b138fa5b2b748df23b0aed Gerrit-Change-Number: 29519 Gerrit-PatchSet: 3 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-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 29 Sep 2022 10:02:07 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 8 months

1
0
0 0

Change in libosmo-netif[master]: osmux: Fix osmux seqnum incremented globally instead of per circuit

by fixeria

Attention is currently required from: pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29518 ) Change subject: osmux: Fix osmux seqnum incremented globally instead of per circuit ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29518 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I562de6a871d8a35475c314ca107c2a12b55d6683 Gerrit-Change-Number: 29518 Gerrit-PatchSet: 3 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-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 29 Sep 2022 10:01:49 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 8 months

1
0
0 0

Change in libosmo-netif[master]: tests/osmux: Properly flush and free out_handle in osmux_test

by pespin

Attention is currently required from: pespin. Hello Jenkins Builder, laforge, fixeria, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-netif/+/29519 to look at the new patch set (#3). Change subject: tests/osmux: Properly flush and free out_handle in osmux_test ...................................................................... tests/osmux: Properly flush and free out_handle in osmux_test Change-Id: Ia86e4324e21ccc4bd4b138fa5b2b748df23b0aed --- M tests/osmux/osmux_test.c M tests/osmux/osmux_test.ok 2 files changed, 15 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/19/29519/3 -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29519 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: Ia86e4324e21ccc4bd4b138fa5b2b748df23b0aed Gerrit-Change-Number: 29519 Gerrit-PatchSet: 3 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-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

2 years, 8 months

1
0
0 0

Change in libosmo-netif[master]: osmux: Fix osmux seqnum incremented globally instead of per circuit

by pespin

Attention is currently required from: fixeria. Hello Jenkins Builder, laforge, fixeria, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-netif/+/29518 to look at the new patch set (#3). Change subject: osmux: Fix osmux seqnum incremented globally instead of per circuit ...................................................................... osmux: Fix osmux seqnum incremented globally instead of per circuit There's no real use in having a global seqnum. The seqnum, as specified by the osmux protocol specification, relates to that of the RTP seq+timestamp, hence it is per circuit. Having it per circuit allows detecting gaps and lost batches on the receiver side, applying the Marker bit on the re-assembled RTP packets. As a resut of the fix, tests/osmux/osmux_test part validating Marker bit started failing. It failed because it was wrongly written to start with, since it used only one osmux_out_handle for the 4 CIDs in use, which was wrong, since each CID must have its own osmux_out_handle as state is kept there per circuit. Related: SYS#5987 Change-Id: I562de6a871d8a35475c314ca107c2a12b55d6683 --- M src/osmux.c M tests/osmux/osmux_test.c M tests/osmux/osmux_test.ok 3 files changed, 589 insertions(+), 587 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/18/29518/3 -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29518 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I562de6a871d8a35475c314ca107c2a12b55d6683 Gerrit-Change-Number: 29518 Gerrit-PatchSet: 3 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-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: newpatchset

2 years, 8 months

1
0
0 0

Change in libosmo-netif[master]: osmux: Fix osmux seqnum incremented globally instead of per circuit

by pespin

Attention is currently required from: fixeria. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29518 ) Change subject: osmux: Fix osmux seqnum incremented globally instead of per circuit ...................................................................... Patch Set 2: (1 comment) File tests/osmux/osmux_test.c: https://gerrit.osmocom.org/c/libosmo-netif/+/29518/comment/7594a264_9ce32071 PS2, Line 269: 4 > ARRAY_SIZE(h_output) Ack -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29518 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I562de6a871d8a35475c314ca107c2a12b55d6683 Gerrit-Change-Number: 29518 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-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Thu, 29 Sep 2022 09:48:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 8 months

1
0
0 0

Change in libosmo-netif[master]: osmux: Mark unused osmux_in_handle field as deprecated

by pespin

Attention is currently required from: fixeria. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29517 ) Change subject: osmux: Mark unused osmux_in_handle field as deprecated ...................................................................... Patch Set 2: Code-Review-1 (1 comment) File include/osmocom/netif/osmux.h: https://gerrit.osmocom.org/c/libosmo-netif/+/29517/comment/9673c3fe_0c4b3b3b PS2, Line 60: /* Unused, DEPRECATED */ > Better use OSMO_DEPRECATED("Unused"). […] This is actually set by old users like osmo-bsc-nat. I'll move this commit to the end and actually make use of the value in libosmo-netif to set the initial value (needs to be done after another patch in the set which makes osmux seq properly go per-circuit). -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29517 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: Ia26fcba5d7364a5744b2d64d0542a2b3880eee34 Gerrit-Change-Number: 29517 Gerrit-PatchSet: 2 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-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Thu, 29 Sep 2022 09:46:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 8 months

1
0
0 0

Change in libosmo-netif[master]: cosmetic: osmux: Properly separate expressions with whitespace

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29515 ) Change subject: cosmetic: osmux: Properly separate expressions with whitespace ...................................................................... cosmetic: osmux: Properly separate expressions with whitespace Change-Id: I524899148da767516c7c1e4bc47b9d3a7b726356 --- M src/osmux.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved fixeria: Looks good to me, approved diff --git a/src/osmux.c b/src/osmux.c index 419645c..9816abb 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -243,7 +243,7 @@ osmo_timer_schedule(&h->timer, 0, 0); } - for (i=0; i<osmuxh->ctr+1; i++) { + for (i = 0; i < osmuxh->ctr + 1; i++) { struct rtp_hdr *rtph; msg = osmux_rebuild_rtp(h, osmuxh, -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29515 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I524899148da767516c7c1e4bc47b9d3a7b726356 Gerrit-Change-Number: 29515 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

2 years, 8 months

1
0
0 0

Change in libosmo-netif[master]: cosmetic: osmux: Fix typo in comment

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29516 ) Change subject: cosmetic: osmux: Fix typo in comment ...................................................................... cosmetic: osmux: Fix typo in comment Change-Id: I05aa1941f9cc8f3aa5ba873cf134767b56b56b69 --- M src/osmux.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved fixeria: Looks good to me, but someone else must approve diff --git a/src/osmux.c b/src/osmux.c index 9816abb..628b763 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -152,7 +152,7 @@ /* 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 + * 2- Intermediate osmux frame lost (seq gap), otherwise rtp receiver only sees * steady increase of delay */ rtph->marker = first_pkt && -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29516 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I05aa1941f9cc8f3aa5ba873cf134767b56b56b69 Gerrit-Change-Number: 29516 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

2 years, 8 months

1
0
0 0

Change in libosmo-netif[master]: osmux: Fix AMR F,Q,CMR fields not properly encoded in osmux header

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29514 ) Change subject: osmux: Fix AMR F,Q,CMR fields not properly encoded in osmux header ...................................................................... osmux: Fix AMR F,Q,CMR fields not properly encoded in osmux header The value of the first RTP packet in the batch was being encoded, instead of the last one (as documented in the Osmux protocol specification). Related: SYS#5987 Change-Id: I06f3d07a05181d938c22bbd0da7172b5dad6659a --- M src/osmux.c M tests/osmux/osmux_input_test.c M tests/osmux/osmux_input_test.ok 3 files changed, 115 insertions(+), 8 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve fixeria: Looks good to me, approved diff --git a/src/osmux.c b/src/osmux.c index 07d2b2e..419645c 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -364,20 +364,16 @@ static int osmux_batch_put(struct osmux_batch *batch, struct osmux_input_state *state) { - struct osmux_hdr *osmuxh; - if (state->add_osmux_hdr) { - osmuxh = (struct osmux_hdr *)state->out_msg->tail; + struct osmux_hdr *osmuxh; + osmuxh = (struct osmux_hdr *)msgb_put(state->out_msg, + sizeof(struct osmux_hdr)); osmuxh->ft = OSMUX_FT_VOICE_AMR; osmuxh->ctr = 0; osmuxh->rtp_m = osmuxh->rtp_m || state->rtph->marker; - osmuxh->amr_f = state->amrh->f; - osmuxh->amr_q= state->amrh->q; osmuxh->seq = batch->seq++; osmuxh->circuit_id = state->ccid; - osmuxh->amr_cmr = state->amrh->cmr; osmuxh->amr_ft = state->amrh->ft; - msgb_put(state->out_msg, sizeof(struct osmux_hdr)); /* annotate current osmux header */ batch->osmuxh = osmuxh; @@ -390,6 +386,10 @@ } batch->osmuxh->ctr++; } + /* For fields below, we only use the last included in batch and ignore any previous: */ + batch->osmuxh->amr_cmr = state->amrh->cmr; + batch->osmuxh->amr_f = state->amrh->f; + batch->osmuxh->amr_q = state->amrh->q; memcpy(state->out_msg->tail, osmo_amr_get_payload(state->amrh), state->amr_payload_len); diff --git a/tests/osmux/osmux_input_test.c b/tests/osmux/osmux_input_test.c index 3b7774c..ff5379a 100644 --- a/tests/osmux/osmux_input_test.c +++ b/tests/osmux/osmux_input_test.c @@ -85,7 +85,7 @@ return rtp_new(rtp_next_seq, rtp_next_ts, 0); } -static void rtp_append_amr(struct msgb *msg, uint8_t ft) +static struct amr_hdr *rtp_append_amr(struct msgb *msg, uint8_t ft) { struct amr_hdr *amrh; struct rtp_hdr *rtph = (struct rtp_hdr *)msg->data; @@ -98,6 +98,7 @@ amrh->f = 0; amrh->ft = ft; msgb_put(msg, osmo_amr_bytes(amrh->ft)); + return amrh; } static void sigalarm_handler(int foo) @@ -251,6 +252,100 @@ osmux_xfrm_input_fini(&h_input); } +static void test_last_amr_cmr_f_q_used_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); + OSMO_ASSERT(osmuxh->ft == OSMUX_FT_VOICE_AMR); + /* Check CMR and Q values are the ones from the last message: */ + OSMO_ASSERT(osmuxh->amr_f == 0); + OSMO_ASSERT(osmuxh->amr_q == 0); + OSMO_ASSERT(osmuxh->amr_cmr == 2); + + osmuxh = osmux_xfrm_output_pull(batch_msg); + OSMO_ASSERT(osmuxh == NULL); + + msgb_free(batch_msg); + + *osmux_transmitted = true; +} +/* Test that fields CMR, F and Q of the last RTP packet in the batch are the + * ones set in the osmux batch header. */ +static void test_last_amr_cmr_f_q_used(void) +{ + struct msgb *msg; + int rc; + const uint8_t cid = 32; + bool osmux_transmitted = false; + struct amr_hdr *amrh; + + printf("===%s===\n", __func__); + + + + clock_override_enable(true); + clock_override_set(0, 0); + rtp_init(0, 0); + + struct osmux_in_handle h_input = { + .osmux_seq = 0, /* sequence number to start OSmux message from */ + .batch_factor = 3, /* batch up to 3 RTP messages */ + .deliver = test_last_amr_cmr_f_q_used_osmux_deliver_cb, + .data = &osmux_transmitted, + }; + + osmux_xfrm_input_init(&h_input); + osmux_xfrm_input_open_circuit(&h_input, cid, false); + + /* First RTP frame at t=0 */ + msg = rtp_next(); + amrh = rtp_append_amr(msg, AMR_FT_2); + amrh->f = 1; + amrh->q = 1; + amrh->cmr = 0; + rc = osmux_xfrm_input(&h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + /* Second RTP frame at t=20, CMR changes 0->1 */ + clock_debug("Submit 2nd RTP packet, CMR changes"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + msg = rtp_next(); + amrh = rtp_append_amr(msg, AMR_FT_2); + amrh->f = 1; + amrh->q = 1; + amrh->cmr = 1; + rc = osmux_xfrm_input(&h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + /* Third RTP frame at t=40, q changes 1->0, CMR changes 1->2: */ + clock_debug("Submit 3rd RTP packet with Q and CMR changes"); + clock_override_add(0, TIME_RTP_PKT_MS*1000); + msg = rtp_next(); + amrh = rtp_append_amr(msg, AMR_FT_2); + amrh->f = 0; + amrh->q = 0; + amrh->cmr = 2; + rc = osmux_xfrm_input(&h_input, msg, cid); + OSMO_ASSERT(rc == 0); + + /* t=60, osmux batch is scheduled to be transmitted: */ + clock_override_add(0, TIME_RTP_PKT_MS*1000); + clock_debug("Osmux frame should now be transmitted"); + osmo_select_main(0); + OSMO_ASSERT(osmux_transmitted == true); + + clock_debug("Closing circuit"); + osmux_xfrm_input_close_circuit(&h_input, cid); + osmux_xfrm_input_fini(&h_input); +} + int main(int argc, char **argv) { @@ -269,6 +364,7 @@ alarm(10); test_amr_ft_change_middle_batch(); + test_last_amr_cmr_f_q_used(); 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 9aca52e..0464fd4 100644 --- a/tests/osmux/osmux_input_test.ok +++ b/tests/osmux/osmux_input_test.ok @@ -10,4 +10,15 @@ sys={0.080000}, mono={0.080000}: OSMUX message (len=81): OSMUX seq=000 ccid=030 ft=1 rtp_m=0 ctr=1 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 ], OSMUX seq=001 ccid=030 ft=1 rtp_m=0 ctr=0 amr_f=0 amr_q=1 amr_ft=06 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 ], OSMUX seq=002 ccid=030 ft=1 rtp_m=0 ctr=0 amr_f=0 amr_q=1 amr_ft=01 amr_cmr=00 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 ] sys={0.080000}, mono={0.080000}: Closing circuit +===test_last_amr_cmr_f_q_used=== +sys={0.000000}, mono={0.000000}: clock_override_set +sys={0.000000}, mono={0.000000}: Submit 2nd RTP packet, CMR changes +sys={0.020000}, mono={0.020000}: clock_override_add +sys={0.020000}, mono={0.020000}: Submit 3rd RTP packet with Q and CMR changes +sys={0.040000}, mono={0.040000}: clock_override_add +sys={0.060000}, mono={0.060000}: clock_override_add +sys={0.060000}, mono={0.060000}: Osmux frame should now be transmitted +sys={0.060000}, mono={0.060000}: OSMUX message (len=49): OSMUX seq=000 ccid=032 ft=1 rtp_m=0 ctr=2 amr_f=0 amr_q=0 amr_ft=02 amr_cmr=02 [ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 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.060000}, mono={0.060000}: Closing circuit OK: Test passed -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29514 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I06f3d07a05181d938c22bbd0da7172b5dad6659a Gerrit-Change-Number: 29514 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

2 years, 8 months

1
0
0 0

Change in libosmo-netif[master]: osmux: Proper encoding of osmux frames when when AMR FT changes

by pespin

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/29512 ) Change subject: osmux: Proper encoding of osmux frames when when AMR FT changes ...................................................................... osmux: Proper encoding of osmux frames when when AMR FT changes The AMR FT (and hence payload content and size) may well change over the lifetime of an RTP stream. Since all AMR payloads in an osmux batch share the same FT (its joined payload is calculated based on osmuxh->ctr*amr_bytes((osmuxh->amr_ft)), if a new RTP/AMR with a different FT arrives we cannot simply append it to the current batch. Instead, the current batch must be considered done and a new batch with anew osmuxh header must be prepared for the resulting osmux frame to send over UDP. Before this patch, when a packet with a different AMR FT was submitted for batching, it would be added in the same batch and decoding would fail since the sizes of the batches would be wrong. Related: SYS#5987 Change-Id: I25eb6ee54c898f575cc71ccfd6d190fe51d56dbe --- M src/osmux.c 1 file changed, 52 insertions(+), 29 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve fixeria: Looks good to me, but someone else must approve pespin: Looks good to me, approved diff --git a/src/osmux.c b/src/osmux.c index ac3cd06..07d2b2e 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -398,23 +398,6 @@ return 0; } -static int osmux_xfrm_encode_amr(struct osmux_batch *batch, - struct osmux_input_state *state) -{ - uint32_t amr_len; - - state->amrh = osmo_rtp_get_payload(state->rtph, state->msg, &amr_len); - if (state->amrh == NULL) - return -1; - - state->amr_payload_len = amr_len - sizeof(struct amr_hdr); - - if (osmux_batch_put(batch, state) < 0) - return -1; - - return 0; -} - static void osmux_encode_dummy(struct osmux_batch *batch, uint8_t batch_factor, struct osmux_input_state *state) { @@ -457,6 +440,7 @@ llist_for_each_entry(circuit, &batch->circuit_list, head) { struct msgb *cur, *tmp; int ctr = 0; + int prev_amr_ft; if (circuit->dummy) { struct osmux_input_state state = { @@ -473,6 +457,7 @@ .out_msg = batch_msg, .ccid = circuit->ccid, }; + uint32_t amr_len; #ifdef DEBUG_MSG char buf[4096]; @@ -482,20 +467,31 @@ #endif state.rtph = osmo_rtp_get_hdr(cur); - if (state.rtph == NULL) + if (!state.rtph) return NULL; + state.amrh = osmo_rtp_get_payload(state.rtph, state.msg, &amr_len); + if (!state.amrh) + return NULL; + state.amr_payload_len = amr_len - sizeof(struct amr_hdr); if (ctr == 0) { #ifdef DEBUG_MSG - LOGP(DLMUX, LOGL_DEBUG, "add osmux header\n"); + LOGP(DLMUX, LOGL_DEBUG, "Add osmux header (First in batch)\n"); +#endif + state.add_osmux_hdr = 1; + } else if (prev_amr_ft != state.amrh->ft) { + /* If AMR FT changed, we have to generate an extra batch osmux header: */ +#ifdef DEBUG_MSG + LOGP(DLMUX, LOGL_DEBUG, "Add osmux header (New AMR FT)\n"); #endif state.add_osmux_hdr = 1; } - osmux_xfrm_encode_amr(batch, &state); + osmux_batch_put(batch, &state); osmux_batch_dequeue(cur, circuit); - msgb_free(cur); + prev_amr_ft = state.amrh->ft; ctr++; + msgb_free(cur); batch->nmsgs--; } } @@ -536,16 +532,10 @@ osmux_xfrm_input_deliver(h); } -static int osmux_rtp_amr_payload_len(struct msgb *msg, struct rtp_hdr *rtph) +static int osmux_rtp_amr_payload_len(struct amr_hdr *amrh, uint32_t amr_len) { - struct amr_hdr *amrh; - unsigned int amr_len; int amr_payload_len; - amrh = osmo_rtp_get_payload(rtph, msg, &amr_len); - if (amrh == NULL) - return -1; - if (!osmo_amr_ft_valid(amrh->ft)) return -1; @@ -561,6 +551,28 @@ return amr_payload_len; } +/* Last stored AMR FT to be added in the current osmux batch. -1 if none stored yet */ +static int osmux_circuit_get_last_stored_amr_ft(struct osmux_circuit *circuit) +{ + struct msgb *last; + struct rtp_hdr *rtph; + struct amr_hdr *amrh; + uint32_t amr_len; + /* Have we seen any RTP packet in this batch before? */ + if (llist_empty(&circuit->msg_list)) + return -1; + OSMO_ASSERT(circuit->nmsgs > 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); + amrh = osmo_rtp_get_payload(rtph, last, &amr_len); + if (amrh == NULL) + return -1; + return amrh->ft; + +} + /* 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) @@ -683,6 +695,8 @@ struct osmux_circuit *circuit; struct msgb *cur; int rc; + struct amr_hdr *amrh; + uint32_t amr_len; circuit = osmux_batch_find_circuit(batch, ccid); if (!circuit) @@ -693,7 +707,11 @@ circuit->dummy = 0; batch->ndummy--; } - amr_payload_len = osmux_rtp_amr_payload_len(msg, rtph); + + 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; @@ -723,8 +741,13 @@ } /* 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 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; /* No room, sorry. You'll have to retry */ -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/29512 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: I25eb6ee54c898f575cc71ccfd6d190fe51d56dbe Gerrit-Change-Number: 29512 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

2 years, 8 months

1
0
0 0

2025

2024

2023

2022

gerrit-log September 2022