Attention is currently required from: dexter, falconia, fixeria, laforge.

Hello Jenkins Builder, dexter, fixeria, laforge,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/c/libosmo-abis/+/39624?usp=email

to look at the new patch set (#2).

The following approvals got outdated and were removed: Code-Review+1 by laforge, Verified+1 by Jenkins Builder

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.

* 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 that plan does NOT include No_Data input to osmo_rtp2trau().

* 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, but the bigger issue is consistency with FR/EFR: while it would be trivial to change the fill pattern to the recently added osmo_gsm620_silence_frame[], if we are removing support for NULL input to osmo_rtp2trau() in the case of FR and EFR, it would be pointless to support such operation just for HR.

Because fixing the case of NULL or No_Data input for {FR,EFR}->TRAU would entail more complexity and rather arbitrary decision-making in the library that better belongs in applications (see below), let's take the Alexandrian solution to this Gordian Knot: 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.

Further notes:

* If someone does wish to emit the idle speech frame of TS 48.060 section 5.4 on Abis DL, despite the observation that historical TRAUs never emit such frames and the uncertainty of how E1 BTS would handle such strangeness, they can use osmo_trau_frame_encode() directly, without going through osmo_rtp2trau() - trivial as there is no payload in need of conversion.

* In the realm of TRAU-UL output (TFO or emulation of E1 BTS), every time a BFI frame is emitted, _some_ payload bit pattern is required: there is no provision for No_Data frames in FR/EFR TRAU/TFO frame format. Selecting one particular fill bit pattern to be "special" in that the library fills it in when the application passed NULL or No_Data is philosophically wrong: no one bit pattern is any more special than any other in this situation. Historical E1 BTS most commonly use the buffer method: they maintain an internal buffer into which new channel-decoded frames are written, and during No_Data conditions (FACCH stealing), the previous buffer content is repeated, perhaps corrupted in some peculiar way. The same approach is recommended for applications that need to transform from RTP to TRAU-UL/TFO output - but this buffer clearly belongs in the application, not in the library.

* For HR codec, there is currently no fully working support for it in Osmocom - or more precisely, the combination of OsmoBSC+OsmoMGW has no working HR support with E1 BTS. Therefore, there are no backward compatibility concerns, and we still have the freedom to change the behavior of TRAU<->RTP library functions to make it more sensible.

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/2