falconia has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmo-abis/+/39624?usp=email ) Change subject: rtp2trau: remove broken acceptance of 0-length payloads ...................................................................... rtp2trau: remove broken acceptance of 0-length payloads Since the beginning of osmo_rtp2trau() in 2020, this API accepted 0-length RTP payloads for FR, HR and EFR, for both TRAU-DL and TRAU-UL output. However, this "support" never worked correctly: * For FR & EFR in DL direction, previous patch 1d42fcd4ea40 attempted to emit the idle speech frame of TS 48.060 section 5.4, following sections 6.5.2 and 6.5.3 of the same spec. However, the actual combination of osmo_rtp2trau() followed by osmo_trau_frame_encode() resulted in an invalid FR/EFR TRAU-DL frame being emitted, rather than the intended special idle speech frame. It is also very uncertain if real E1 BTS would handle that idle speech frame particularly well even if we did emit one correctly - it is a strange "feature" of the spec that never occurs in real operation of a TRAU, neither in free-running speech encoder state nor in TFO. * For FR & EFR in UL direction, the same previous patch would turn 0-length payload input (or TW-TS-001 No_Data input with later patches) into BFI with a peculiar fill pattern. The fill pattern was all-1s in the case of FR or all-1s plus bad CRC in the case of EFR. (The 5 CRCs of EFR TRAU frame format just happen to come out bad when all Dn bits are set to 1 - not intentional CRC inversion.) TRAU-UL output is not currently used anywhere in Osmocom; there is a plan to implement TFO in ThemWi software using Osmocom libraries, but this type of fixed bit fill is not a good choice for TFO frame output going to foreign GSM network operators. * For HR speech to TRAU-DL conversion, the effect of 0-length payload input was emission of a TRAU-DL frame that looks valid, but has all 112 bits of payload set to 0, with correct CRC. All-zeros is a poor choice of LPC parameter fill for dummy GSM-HR codec frames; if an application wishes to implement dummy frame fill, it should control its own fill frame pattern, rather than expect osmo_rtp2trau() to provide good idle fill. Solution: simply remove all bogo-support for 0-length RTP payload input to osmo_rtp2trau() in the case of speech codecs, and return -EINVAL just like for all other types of invalid RTP input. Effect on OsmoMGW-E1: the only time when OsmoMGW could pass a 0-length payload to osmo_rtp2trau() would be if the RTP stream comes from OsmoBTS (not from another leg on OsmoMGW-E1) with vty option 'rtp continuous-streaming' enabled. Prior to the present patch, the resulting effect would be bogus output on Abis DL; after this patch, the MGW behavior will be the same as with no RTP input - the MGW will insert fixed fill frames at the application level, going directly to I.460 mux. Change-Id: Ia41ea2eab1ded094cadcd91dbd33de8800af11ee --- M src/trau/trau_rtp_conv.c 1 file changed, 30 insertions(+), 108 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-abis refs/changes/24/39624/1 diff --git a/src/trau/trau_rtp_conv.c b/src/trau/trau_rtp_conv.c index 86d2b77..8d2631b 100644 --- a/src/trau/trau_rtp_conv.c +++ b/src/trau/trau_rtp_conv.c @@ -580,18 +580,11 @@ data_len--; } - /* now we must have either standard FR or no-data input */ - switch (data_len) { - case GSM_FR_BYTES: - if ((data[0] & 0xF0) != 0xD0) - return -EINVAL; - break; - case 0: - bfi = true; - break; - default: + /* with or without TW-TS-001 TEH, we require 260-bit GSM-FR payload */ + if (data_len != GSM_FR_BYTES) return -EINVAL; - } + if ((data[0] & 0xF0) != 0xD0) + return -EINVAL; tf->type = OSMO_TRAU16_FT_FR; @@ -606,34 +599,20 @@ tf->c_bits[3] = 1; tf->c_bits[4] = 0; } else { /* DL */ - if (data_len == 0) { - /* C1 .. C5: idle speech */ - tf->c_bits[0] = 0; - tf->c_bits[1] = 1; - tf->c_bits[2] = 1; - tf->c_bits[3] = 1; - tf->c_bits[4] = 0; - } else { - /* C1 .. C5: FR DL */ - tf->c_bits[0] = 1; - tf->c_bits[1] = 1; - tf->c_bits[2] = 1; - tf->c_bits[3] = 0; - tf->c_bits[4] = 0; - } + /* C1 .. C5: FR DL */ + tf->c_bits[0] = 1; + tf->c_bits[1] = 1; + tf->c_bits[2] = 1; + tf->c_bits[3] = 0; + tf->c_bits[4] = 0; } memset(&tf->c_bits[5], 0, 6); /* C6 .. C11: Time Alignment */ if (tf->dir == OSMO_TRAU_DIR_UL) { tf->c_bits[11] = bfi; /* C12: BFI */ - if (data_len == 0) { - tf->c_bits[12] = 0; /* C13: SID=0 */ - tf->c_bits[13] = 0; /* C14: SID=0 */ - } else { - /* SID classification per GSM 06.31 section 6.1.1 */ - sidc = osmo_fr_sid_classify(data); - tf->c_bits[12] = (sidc >> 1) & 1; /* C13: msb */ - tf->c_bits[13] = (sidc >> 0) & 1; /* C14: lsb */ - } + /* SID classification per GSM 06.31 section 6.1.1 */ + sidc = osmo_fr_sid_classify(data); + tf->c_bits[12] = (sidc >> 1) & 1; /* C13: msb */ + tf->c_bits[13] = (sidc >> 0) & 1; /* C14: lsb */ tf->c_bits[14] = taf; /* C15: TAF (SACCH or not) */ tf->c_bits[15] = 1; /* C16: spare */ tf->c_bits[16] = dtxd; /* C17: DTXd applied or not */ @@ -648,12 +627,6 @@ memset(&tf->c_bits[17], 1, 4); /* C18 .. C21: spare */ memset(&tf->t_bits[0], 1, 4); - if (!data_len) { - /* idle speech frame if DL, BFI speech frame if UL */ - memset(&tf->d_bits[0], 1, 260); - return 0; - } - /* reassemble d-bits */ i = 0; /* counts bits */ j = 4; /* counts input bits */ @@ -690,9 +663,6 @@ data++; data_len--; break; - case 0: - /* accept no-data input */ - break; default: return -EINVAL; } @@ -719,10 +689,7 @@ memset(tf->c_bits+17, 1, 4); /* C18..C21: spare */ memset(&tf->t_bits[0], 1, 4); tf->ufi = 1; /* spare bit in TRAU-DL */ - if (data_len) - osmo_pbit2ubit(tf->d_bits, data, GSM_HR_BYTES * 8); - else - memset(tf->d_bits, 0, GSM_HR_BYTES * 8); + osmo_pbit2ubit(tf->d_bits, data, GSM_HR_BYTES * 8); /* CRC is *not* computed by TRAU frame encoder - we have to do it */ osmo_crc8gen_set_bits(&gsm0860_efr_crc3, tf->d_bits, 44, tf->crc_bits); @@ -739,9 +706,6 @@ data++; data_len--; break; - case 0: - /* accept no-data input */ - break; default: return -EINVAL; } @@ -776,10 +740,7 @@ memset(&tf->t_bits[0], 1, 2); - if (data_len) - osmo_pbit2ubit(tf->d_bits, data, GSM_HR_BYTES * 8); - else - memset(tf->d_bits, 0, GSM_HR_BYTES * 8); + osmo_pbit2ubit(tf->d_bits, data, GSM_HR_BYTES * 8); /* CRC is *not* computed by TRAU frame encoder - we have to do it */ osmo_crc8gen_set_bits(&gsm0860_efr_crc3, tf->d_bits, 44, tf->crc_bits); @@ -927,52 +888,30 @@ data_len--; } - /* now we must have either standard EFR or no-data input */ - switch (data_len) { - case GSM_EFR_BYTES: - if ((data[0] & 0xF0) != 0xC0) - return -EINVAL; - break; - case 0: - bfi = true; - break; - default: + /* with or without TW-TS-001 TEH, we require 244-bit GSM-EFR payload */ + if (data_len != GSM_EFR_BYTES) return -EINVAL; - } + if ((data[0] & 0xF0) != 0xC0) + return -EINVAL; tf->type = OSMO_TRAU16_FT_EFR; /* FR Data Bits according to TS 48.060 Section 5.5.1.1.2 */ /* set c-bits and t-bits */ - if (data_len == 0 && tf->dir == OSMO_TRAU_DIR_DL) { - /* C1 .. C5: idle speech */ - tf->c_bits[0] = 0; - tf->c_bits[1] = 1; - tf->c_bits[2] = 1; - tf->c_bits[3] = 1; - tf->c_bits[4] = 0; - } else { - /* C1 .. C5: EFR */ - tf->c_bits[0] = 1; - tf->c_bits[1] = 1; - tf->c_bits[2] = 0; - tf->c_bits[3] = 1; - tf->c_bits[4] = 0; - } - + /* C1 .. C5: EFR */ + tf->c_bits[0] = 1; + tf->c_bits[1] = 1; + tf->c_bits[2] = 0; + tf->c_bits[3] = 1; + tf->c_bits[4] = 0; memset(&tf->c_bits[5], 0, 6); /* C6 .. C11: Time Alignment */ if (tf->dir == OSMO_TRAU_DIR_UL) { tf->c_bits[11] = bfi; /* C12: BFI */ - if (data_len == 0) { - tf->c_bits[12] = 0; /* C13: SID=0 */ - tf->c_bits[13] = 0; /* C14: SID=0 */ - } else { - /* SID classification per GSM 06.81 section 6.1.1 */ - sidc = osmo_efr_sid_classify(data); - tf->c_bits[12] = (sidc >> 1) & 1; /* C13: msb */ - tf->c_bits[13] = (sidc >> 0) & 1; /* C14: lsb */ - } + /* SID classification per GSM 06.81 section 6.1.1 */ + sidc = osmo_efr_sid_classify(data); + tf->c_bits[12] = (sidc >> 1) & 1; /* C13: msb */ + tf->c_bits[13] = (sidc >> 0) & 1; /* C14: lsb */ tf->c_bits[14] = taf; /* C15: TAF (SACCH or not) */ tf->c_bits[15] = 1; /* C16: spare */ tf->c_bits[16] = dtxd; /* C17: DTXd applied or not */ @@ -988,12 +927,6 @@ memset(&tf->c_bits[17], 1, 4); /* C18 .. C21: spare */ memset(&tf->t_bits[0], 1, 4); - if (data_len == 0) { - /* idle speech frame if DL, BFI speech frame if UL */ - memset(&tf->d_bits[0], 1, 260); - return 0; - } - /* reassemble d-bits */ tf->d_bits[0] = 1; for (i = 1, j = 4; i < 39; i++, j++) @@ -1645,17 +1578,6 @@ * frames for call leg B, is to apply the TFO transform of TS 28.062 section * C.3.2.1.1, then feed the output of that transform to the present function. * - * - The provision whereby a zero-length RTP payload is not treated as an error - * like other invalid RTP inputs, but is converted to an idle speech frame - * in TRAU-DL output should be considered a bogon. This condition never - * occurs when the just-mentioned TFO transform is applied immediately prior - * to TRAU-DL output, nor does it ever occur in the original GSM architecture - * where the TRAU either free-runs a speech encoder or applies the same TFO - * transform - hence it is unlikely to be handled well by real E1 BTSes. - * Furthermore, this code path in libosmotrau is currently broken (the - * intended idle speech frame gets turned into a "regular" but invalid FR/EFR - * speech frame) and should be considered for removal. - * * Considerations for TRAU-UL output: * * - For FR and EFR codecs, the only correct RTP format for conversion to -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/39624?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: libosmo-abis Gerrit-Branch: master Gerrit-Change-Id: Ia41ea2eab1ded094cadcd91dbd33de8800af11ee Gerrit-Change-Number: 39624 Gerrit-PatchSet: 1 Gerrit-Owner: falconia &lt;falcon(a)freecalypso.org&gt;