Project

General

Profile

Actions

Bug #6040

closed

osmo-bts-trx needlessly applies ECU to its UL output

Added by falconia 11 months ago. Updated 10 months ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
osmo-bts-trx
Target version:
-
Start date:
05/18/2023
Due date:
% Done:

100%

Spec Reference:

Description

In a departure from all other OsmoBTS models, osmo-bts-trx seeks to apply a libosmocodec ECU to its UL output, with the ECU inserted into the UL path between GSM 05.03 channel decoding and RTP output via l1sap. More precisely, osmo-bts-trx attempts to apply such ECU to its UL in all speech modes, for both TCH/F and TCH/H, but because libosmocodec currently provides an ECU implementation only for FR1 codec, TCH/FS is the only channel mode that ends up being subjected to UL ECU application by osmo-bts-trx. This misfeature was added to osmo-bts-trx (and no other models) back in early 2018:

https://cgit.osmocom.org/osmo-bts/commit/?id=69d0d506775c82eb2bde66fe748100a94a3173a0

The problem, however, is that insertion of an ECU transform into the UL output path of a BTS runs counter to standard GSM architecture. In the standard architecture the BTS is supposed to transmit BFI (bad frame indication) frames in those frame positions where it got bad Rx (or where the frame was stolen for FACCH), and all bad frame handling functions (which include not only the ECU function, but also the comfort noise insertion function) are to be applied by the entity on the receiving end of the frame stream originating from the BTS. This architecture was originally designed with TDM transport in mind, but it carries over to RTP just as well: in RTP a BFI frame is either absence of packet transmission (an intentional gap in the RTP stream) or (with recently added "rtp continuous-streaming" option) a special timing-tick-only RTP packet with zero-length payload. If the entity on the receiving end of the RTP stream is another GSM call leg (TrFO), these BFIs propagate to call leg B DL and are ultimately handled by the spec-compliant Rx DTX handler in the MS; if the entity on the receiving end of the RTP stream is a network edge transcoder or some other process that decodes GSM 06.10 via libgsm, that process is responsible for calling a proper FR1 Rx DTX handler (e.g., Themyscira libgsmfrp) just before the call to gsm_decode(). (For all other GSM codecs the Rx DTX handler has been built into the main body of the standard decoder from the beginning; FR1 is the lone exception.)

It has been claimed that the discrepancy of osmo-bts-trx applying an ECU to its UL output while no other OsmoBTS model does likewise is due to sysmoBTS PHY (a proprietary black box DSP) applying its own version of ECU to its UL output. However, this claim can be easily proven false by observing the output from sysmoBTS PHY under BFI conditions. Testing with both FR1 and EFR codecs on a sysmoBTS (others don't matter for comparison with osmo-bts-trx ECU application, which is effective only for FR1), we can see the following behavior:

  • When a traffic frame in the UL is stolen for FACCH (this condition can be easily induced by pressing DTMF keys and thus creating FACCH activity during speech), the PHY only sends GsmL1_Sapi_FacchF (no GsmL1_Sapi_TchF, see the fix in #5974), thus there is no clearly no ECU output being emitted.
  • When the MS goes into a DTXu pause (enable 'dtx uplink' in OsmoBSC config and go silent on the test MS) and transmits nothing at all on the air, the PHY emits a GsmL1_Sapi_TchF with zero-length payload in every 20 ms window, indicating that it received nothing on the air. Once again, no ECU output.
  • With FR1 codec during DTXu pauses the output from the PHY includes occasional interspersed bogons: most GsmL1_Sapi_TchF indications carry zero-length payloads, indicating BFI, but every now and then there will be a GsmL1_Sapi_TchF carrying an FR1 codec frame presented as valid. However, these occasional frames are garbage, and if they aren't suppressed by the check in l1sap, the result will be highly unpleasant sounds replacing the expected comfort noise. These bogons are likely caused by the weak CRC-3 check in GSM 05.03 channel coding for FR1 (1/8 probability of indicating "good" when decoding radio noise), and they are suppressed by the check in l1sap (common for all BTS models and all speech modes) that suppresses frame output from the PHY and replaces it with BFI if the link quality is too low. In any case, the presence of these bogons in the output from sysmoBTS PHY once again confirms that this DSP is not applying any ECU-style transform to its UL output.

So why did osmo-bts-trx developers back in late 2017 go out of their way to implement an ECU (the original libosmocodec FR ECU was developed specifically for this purpose) and insert it into the UL output path of this BTS model? They clearly noticed poor audio quality with osmo-bts-trx compared to sysmoBTS, but unfortunately they came to wrong conclusions about the cause. In my opinion, the problem they were seeing (or hearing) was actually bug #6033: please look at the cgit link at the beginning of the present write-up, linking to the original commit that added ECU UL application to osmo-bts-trx, and note the "before" part of the diff. Prior to the introduction of ECU, the osmo-bts-trx model-specific code was emitting its BFI as a valid-looking FR1 codec frame with all 260 bits of payload set to 0 - but that BFI marker format was an osmo-bts-trx invention, not any kind of standard! When sent to a standard decoder on the receiving end of the RTP stream, these bogus BFI markers must have produced unpleasant sounds instead of spec-defined bad frame handling:

  • If the entity on the receiving end of the RTP stream was another GSM call leg going to a standard (non-Osmocom) GSM MS, the Rx DTX handler in that MS would interpret a frame of all zero bits as a SID (that's how the rules of GSM 06.31 section 6.1.1 will classify such a frame), and would invoke its comfort noise generator. However, because LARc parameters for this CN generation are bogus (a payload of all zero bits constitutes bogus LARc), the resulting "comfort" noise was likely not so comforting.
  • If the entity on the receiving end of the RTP stream was GAPK or some other tool that invokes an 06.10 decoder (gsm_decode() from libgsm) without invoking an Rx DTX handler first (against the spec, but the people involved likely thought they could get away with it if they had 'dtx uplink' disabled in OsmoBSC config), the all-zeros FR1 payload was fed directly to the 06.10 decoder, which is still a bogon - and the audio output was once again most likely unpleasant.

Applying an ECU in the UL output path of osmo-bts-trx masked this bug #6033: with the old ECU implemented in 2017-2018, bogus FR1 frames of all zero bits could still be emitted, once Xmaxc was reduced to 0, but if there is only one bad frame here and there, or a short burst of bad frames, short enough for Xmaxc reduction to not reach 0, then the ECU output never exhibits the bogon of #6033.

The correct solution is to first fix bug #6033 (remove the code that emits those bogons, and let l1sap emit the same BFI representation as all other BTS models), and then either remove UL ECU invokation from osmo-bts altogether, or at least make it optional per vty config. And if the latter option is chosen (vty config, rather than removing it altogether), then this ECU provision needs to be moved from osmo-bts-trx into the common part.


Related issues

Related to OsmoBTS - Bug #6033: TRX version unnecessarily invents its own BFI packet formatsResolvedfalconia05/16/2023

Actions
Actions #1

Updated by falconia 11 months ago

  • Related to Bug #6033: TRX version unnecessarily invents its own BFI packet formats added
Actions #2

Updated by falconia 10 months ago

  • Status changed from New to In Progress
  • % Done changed from 0 to 50

The simplest (and my preferred) fix for this bug is implemented on this feature branch:

https://cgit.osmocom.org/osmo-bts/log/?h=falconia/ecu-ectomy

I plan on covering this topic in the upcoming OsmoDevCall.

Actions #3

Updated by falconia 10 months ago

  • Status changed from In Progress to Resolved
  • % Done changed from 50 to 100
Actions #4

Updated by falconia 10 months ago

  • Status changed from Resolved to In Progress
  • % Done changed from 100 to 50

It looks like the bot that auto-closes Redmine tickets based on the "Closes:" line in git commits is wrong: it should only trigger when the bug-closing commit was merged to master, not on a developer's personal feature branch. Revert the status change made by the bot - my proposed ECU-ectomy patch is not in master yet, hence the present ticket is NOT yet ready to close.

Actions #5

Updated by laforge 10 months ago

On Tue, Jun 20, 2023 at 06:06:33PM +0000, falconia wrote:

It looks like the bot that auto-closes Redmine tickets based on the "Closes:" line in git commits is wrong:

it sadly is not a (external) bot, but built-in functionality of redmine itself,
see scan_comment_for_issue_ids and related code in
https://github.com/redmine/redmine/blob/828439338f35000eb5da567316ee4ba94f940f3d/app/models/changeset.rb#L124

A quick search at https://www.redmine.org/ looks like there is no related issue reported upstream so far.

it should only trigger when the bug-closing commit was merged to master, not on a developer's personal feature branch.

I'd agree, and I'm surprised this hasn't hit us earlier.

Actions #6

Updated by falconia 10 months ago

laforge - thanks for the heads-up. To avoid getting bitten by this bug in the future, I'll avoid the "Closes:" construct and just use "Related:" - it isn't that much extra work to close resolved tickets manually, and there is always a certain satisfaction in closing successfully fixed bugs and successfully implemented features. :)

Actions #7

Updated by falconia 10 months ago

  • Assignee set to falconia
  • % Done changed from 50 to 0

The present issue has been discussed in OsmoDevCall on 2023-06-21. The consensus reached was that complete removal of the ECU call from osmo-bts UL path is not currently possible because apparently some commercial osmo-bts-trx customers desire and depend on this BTS UL behavior, even if it goes against the spirit of 3GPP specs. (Spirit of the specs, rather than letter, as there is no official 3GPP spec for Abis over IP.) However, it was agreed that:

  • We should make this ECU optional;
  • The inconsistency between osmo-bts-trx and other models should be resolved by moving this made-optional ECU call from trx model code to the common layer;
  • The inconsistency caused by the l1sap link quality check sometimes defeating the ECU should be resolved by moving the ECU call after this link quality check.

None of this work has been done yet, hence I am resetting the progress on this ticket to zero. However, I am taking on this task, hence I am also making myself the assignee.

Actions #8

Updated by falconia 10 months ago

  • % Done changed from 0 to 50

A complete implementation of the consensus from 2023-06-21 has been submitted for code review:

https://gerrit.osmocom.org/c/osmo-bts/+/32110
https://gerrit.osmocom.org/c/osmo-bts/+/33470
https://gerrit.osmocom.org/c/osmo-bts/+/33471

Actions #9

Updated by falconia 10 months ago

  • Status changed from In Progress to Resolved
  • % Done changed from 50 to 100

The solution that ended up being implemented and merged is as follows:

  • The ECU is now fully optional, enabled or disabled with vty option "rtp internal-uplink-ecu".
  • When the ECU is enabled, it happens in the common layer and is thus independent of BTS model.
  • When enabled, ECU application happens after the link quality check, thus the output is fully consistent in terms of switching between ECU-active periods and DTXu pauses.

The mainline commits implementing this change are as follows:

https://cgit.osmocom.org/osmo-bts/commit/?id=f0f91fc66c9e3ca531d6cbc9d4c2a946da7a1e50
https://cgit.osmocom.org/osmo-bts/commit/?id=676e9e5804b545e2e1ec773d8c31a64804a27775
https://cgit.osmocom.org/osmo-bts/commit/?id=4c3b4e2868dba83f1450b2bf9d0e48b93a0e2d6f

Actions

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)