[S] Change in osmo-bts[master]: all models, HR1 codec: accept both TS101318 and RFC5993 formats

Attention is currently required from: laforge, pespin, dexter. falconia has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32968 ) Change subject: all models, HR1 codec: accept both TS101318 and RFC5993 formats ...................................................................... Patch Set 1: (3 comments) File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/32968/comment/a3452940_ea23b703 PS1, Line 1264: static bool rtppayload_validate_hr(struct msgb *msg) The same is already the case for rtppayload_validate_{fr,efr} - those functions call osmo_{fr,efr}_sid_preen(), and the latter functions apply a transformation to the bits: if the received frame was a deemed-valid SID with some bit errors, that SID is rejuvenated by clearing all those errored bits. Should we perhaps rename all these functions to rtppayload_preen_*() instead of validate? Please ack if I should make a preliminary patch renaming the two existing functions and then use the new name in the next spin of the present patch. https://gerrit.osmocom.org/c/osmo-bts/+/32968/comment/dece4921_02b17e31 PS1, Line 1270: case GSM_HR_BYTES_RTP_RFC5993: I concur with @laforge, and my initial thoughts on how to approach OS#5688 on the fundamental design level (before going down to code implementation level) were/are based on the same principle: it is unreasonable to anticipate new hypothetical DSP backends that would differ from what we have currently. @pespin: Prior to recent libosmocoding fixes, there was this issue: the channel encoding function used by osmo-bts-trx stupidly required 15-byte format even though it never actually looked at the ToC octet. That design bug is now fixed in libosmocoding in a backward-compatible way (that's what the Depends: line in the present patch refers to), and now that same function happily accepts 14-byte payloads. https://gerrit.osmocom.org/c/osmo-bts/+/32968/comment/d0e73f23_4d1f3678 PS1, Line 1279: memmove(msg->data, msg->data + 1, GSM_HR_BYTES); : msgb_get(msg, 1); Please read the long C comment on the 7 lines immediately preceding your highlight. The obvious msgb_pull() way was the first thing I tried, quite naturally, but it immediately brought the sysmoBTS unit to its knees! The visible symptoms were: as soon as that code path starts executing (as soon as RTP packets come in that require this one octet stripping), the MS on the test call would declare radio link failure, I seem to recall other MS also going into No Service because they stopped receiving PCH, and the vty telnet session was slowed to a crawl. I reason that the bad behavior is caused by misaligned accesses to 32-bit fields inside the l1sap header being added by l1sap_tch_rts_ind() - that header becomes misaligned once the msgb_pull() moves off the aligned position by 1 byte. I agree with you that the memmove way is inefficient - but the simple way doesn't work right now. Can we perhaps brainstorm some better way? -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32968 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I702e26c3ad5b9d8347e73c6cd23efa38a3a3407e Gerrit-Change-Number: 32968 Gerrit-PatchSet: 1 Gerrit-Owner: falconia <falcon(a)freecalypso.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Wed, 24 May 2023 17:37:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment