Bug #6027
closedFR ECU implementation is very poor
100%
Description
The current implementation of ECU for the FR codec in libosmocodec exhibits the following serious defects:
- If right after osmo_ecu_init() the application calls the frame_out() method, without first feeding in a good frame with frame_in(), the output from the ECU will be a bogus RTP payload with the signature nibble set to 0 instead of D. This scenario will occur all the time in real world usage, when initial reception on a freshly activated channel picks up radio noise rather than valid codec frames.
- The muting procedure specified in TS 46.011 is implemented incorrectly. When block magnitude Xmaxc reaches 0, the ECU is supposed to emit silence frames, and the specific silence frame to be emitted is listed in Table 1 at the end of the spec. However, the implemented ECU emits a frame of all zero bits, which is wrong - a frame of all zero bits is a SID frame, meaning a request for the DTX handler on the receiving end to initiate comfort noise generation, not a silence frame per GSM 06.11 or its 3GPP successor.
- If a received good frame passed to frame_in() is a SID frame, that SID gets cached and never expires - if the application keeps calling frame_out(), receiving only radio noise, that cached SID will continue to be output forever. Per GSM 06.31 and 06.11 specs, this cached SID should be treated as expired after 960 ms have passed without any new valid frames being received, and comfort noise muting (or SID muting, in the case of a "pure" ECU without a directly coupled CN generator) should be initiated.
- The case of ECU input being an invalid SID, or a valid SID with one errored bit in the SID field, is not handled at all.
Correct FR codec ECU functionality, coupled with comfort noise generation, is implemented in Themyscira libgsmfrp version 1.0.1:
https://www.freecalypso.org/hg/gsm-codec-lib/
Fortunately, it is also possible to implement a more correct ECU for FR that would fit into the generic ECU abstraction layer in libosmocodec - a patch will be coming soon.
Updated by falconia about 1 year ago
- Status changed from New to Feedback
- % Done changed from 0 to 80
The new ECU implementation has been submitted to Gerrit:
https://gerrit.osmocom.org/c/libosmocore/+/32685
It still has some remaining shortcomings and limitations, as detailed in code comments, but these remaining flaws stem from the libosmocodec ECU model itself, primarily from the poor design decision of making the ECU by itself (without the rest of Rx DTX handler) into a modular component, which runs counter to the standard GSM architecture where the modular component boundary is drawn around the slightly larger Rx DTX handler, rather than around its ECU function. Making the ECU in libosmocodec as close to correct as it can be within the limitations of this flawed architectural design can be regarded as a harm reduction measure - but at the same time an effort needs to be made to phase out the use of this misdesigned ECU API altogether:
- The invokation of ECU in the UL path in osmo-bts-trx is wrong, as noted in the comments on <https://gerrit.osmocom.org/c/osmo-bts/+/32110>. We probably can't remove it without upsetting users who like having it, but it needs to be made optional, and maybe some day we can even flip the default.
- gapk has a provision for invoking the FR ECU (using the old API, not the abstracted one) before invoking the 06.10 decoder from libgsm. However, this design is flawed: by design the ECU (old or new) does not include a comfort noise generator, i.e., incoming SID frames will pass through unchanged, and classic libgsm has no support for SID - it is a pure GSM 06.10 implementation without any DTX functions. Any SID frames in the input will thus be misdecoded, likely producing poor audio quality for the user. The correct solution would be to provide an option in gapk to make use of Themyscira libgsmfrp (a proper and complete Rx DTX hander for GSM-FR), and call this preprocessor (instead of the ECU) before libgsm decoding function. gapk already uses classic libgsm as an external dependency, why can't it use libgsmfrp in the same way?
Updated by falconia about 1 year ago
The link to the other Gerrit patch (the one for osmo-bts, not the libosmocodec one) got bungled in the previous comment; here is the correct link:
Updated by falconia about 1 year ago
- Status changed from Feedback to Resolved
- % Done changed from 80 to 100
For programs that invoke the ECU via the abstracted API, the old broken implementation has been swapped out for a better one:
https://cgit.osmocom.org/libosmocore/commit/?id=5eb356be99d0b3ee75b0643d81e66ca9274b001f
The present ticket can now be closed as resolved; invokation of the ECU from the UL output path in osmo-bts-trx is still completely unnecessary, but with the new ECU implementation it no longer causes harm.
