[M] Change in osmo-bts[master]: trx: remove model-specific BFI packet formats

falconia has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32788 ) Change subject: trx: remove model-specific BFI packet formats ...................................................................... trx: remove model-specific BFI packet formats As detailed in OS#6033, osmo-bts-trx was emitting its own invented packet formats to signal BFIs (bad frame indications), different from the vty-configured ("rtp continuous-streaming" or not) BFI signaling method implemented in l1sap and used by all other BTS models. In the case of EFR codec, the made-up BFI format previously used by osmo-bts-trx caused EFR operation to be broken: a spec-compliant EFR decoder on the receiving end of the RTP stream (a network transcoder or the MS on the other GSM call leg) would receive and decode garbage EFR frames of 244 zero bits instead of invoking the spec-defined bad frame handler, causing bad audio artifacts for the user. The same situation would also happen for FR1 codec, but the application of ECU masked this bug: with the ECU invoked in the UL output path, the BFI emitting code for FR1 never executed. In the case of AMR and HR1 codecs, the model-specific BFI packet format of osmo-bts-trx is currently harmless, but: * The extra code adds unnecessary complexity; * BFI packet formats are inconsistent between osmo-bts-trx and other OsmoBTS models; * BFI format is even inconsistent within osmo-bts-trx itself, as under certain conditions the common code in l1sap will override the UL payload from the BTS model and emit its own form of BFI instead. Fix all of the above by removing trx model-specific BFI formats for all codecs, and let l1sap handle all BFIs. Related: OS#6033 Change-Id: I8f9fb5b8c5b2cad4b92ac693c0040779f811981a --- M src/osmo-bts-trx/sched_lchan_tchf.c M src/osmo-bts-trx/sched_lchan_tchh.c 2 files changed, 51 insertions(+), 53 deletions(-) Approvals: laforge: Looks good to me, but someone else must approve fixeria: Looks good to me, approved pespin: Looks good to me, but someone else must approve Jenkins Builder: Verified diff --git a/src/osmo-bts-trx/sched_lchan_tchf.c b/src/osmo-bts-trx/sched_lchan_tchf.c index 6d6fb0b..43a3784 100644 --- a/src/osmo-bts-trx/sched_lchan_tchf.c +++ b/src/osmo-bts-trx/sched_lchan_tchf.c @@ -295,35 +295,11 @@ goto compose_l1sap; } - switch (tch_mode) { - case GSM48_CMODE_SPEECH_V1: /* FR */ - memset(tch_data, 0, GSM_FR_BYTES); - tch_data[0] = 0xd0; - rc = GSM_FR_BYTES; - break; - case GSM48_CMODE_SPEECH_EFR: /* EFR */ - memset(tch_data, 0, GSM_EFR_BYTES); - tch_data[0] = 0xc0; - rc = GSM_EFR_BYTES; - break; - case GSM48_CMODE_SPEECH_AMR: /* AMR */ - rc = osmo_amr_rtp_enc(tch_data, - chan_state->codec[chan_state->ul_cmr], - chan_state->codec[chan_state->ul_ft], - AMR_BAD); - if (rc < 2) { - LOGL1SB(DL1P, LOGL_ERROR, l1ts, bi, - "Failed to encode AMR_BAD frame (rc=%d), " - "not sending BFI\n", rc); - return -EINVAL; - } - memset(tch_data + sizeof(struct amr_hdr), 0, rc - sizeof(struct amr_hdr)); - break; - default: - LOGL1SB(DL1P, LOGL_ERROR, l1ts, bi, - "TCH mode %u invalid, please fix!\n", tch_mode); - return -EINVAL; - } + /* In order to signal BFI in our UL RTP output, we need + * to push an empty payload to l1sap. The upper layer + * will choose the correct RTP representation of this + * BFI based on model-independent vty config. */ + rc = 0; } } diff --git a/src/osmo-bts-trx/sched_lchan_tchh.c b/src/osmo-bts-trx/sched_lchan_tchh.c index 41965c4..5872c33 100644 --- a/src/osmo-bts-trx/sched_lchan_tchh.c +++ b/src/osmo-bts-trx/sched_lchan_tchh.c @@ -333,30 +333,11 @@ goto compose_l1sap; } - switch (tch_mode) { - case GSM48_CMODE_SPEECH_V1: /* HR */ - tch_data[0] = 0x70; /* F = 0, FT = 111 */ - memset(tch_data + 1, 0, 14); - rc = 15; - break; - case GSM48_CMODE_SPEECH_AMR: /* AMR */ - rc = osmo_amr_rtp_enc(tch_data, - chan_state->codec[chan_state->ul_cmr], - chan_state->codec[chan_state->ul_ft], - AMR_BAD); - if (rc < 2) { - LOGL1SB(DL1P, LOGL_ERROR, l1ts, bi, - "Failed to encode AMR_BAD frame (rc=%d), " - "not sending BFI\n", rc); - return -EINVAL; - } - memset(tch_data + sizeof(struct amr_hdr), 0, rc - sizeof(struct amr_hdr)); - break; - default: - LOGL1SB(DL1P, LOGL_ERROR, l1ts, bi, - "TCH mode %u invalid, please fix!\n", tch_mode); - return -EINVAL; - } + /* In order to signal BFI in our UL RTP output, we need + * to push an empty payload to l1sap. The upper layer + * will choose the correct RTP representation of this + * BFI based on model-independent vty config. */ + rc = 0; } } -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32788 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I8f9fb5b8c5b2cad4b92ac693c0040779f811981a Gerrit-Change-Number: 32788 Gerrit-PatchSet: 2 Gerrit-Owner: falconia &lt;falcon(a)freecalypso.org&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: falconia &lt;falcon(a)freecalypso.org&gt; Gerrit-Reviewer: fixeria &lt;axilirator(a)gmail.com&gt; Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: merged