Bug #5461
closedFix regression translating AMR Payload Type between connections
100%
Description
This patch [1] in practice reverted a previous patch [2] which was submitted a for a reason.
The reason is that we want to match the exact AMR Payload Type in first place, in order to avoid AMR transcoding OA<->BE if not needed.
This can happen for instance if connection A in the endpoint announces through SDP that it supports only let's say:- PT=100 AMR octet_aligned=1
- PT=102 AMR octet_aligned=0
- PT=103 AMR octet_aligned=1
After [1] is merged, the code, when forwarding RTP connA->connB, will select the PT of first AMR codec found in conn regardless of the octet_aligned, that is PT=102, which means it will end up later on having to transocde OA->BE. The ideal thing to do in this situation would be to pick PT=103, so that transcoding can be avoided when sending packets on that direction, hence freeing CPU use.
In order to do so, it's quite easy, we just need to modify a bit patch [1] to incorporate the idea of [2]. So first we do a lookup as in [2], checking exact match, and if not found, do as in [1] (no AMR octet_aligned match).
This can be generalized as "first try a match not requiring transcoding, and if not found, try a match which requires transcoding".
It can be implemented with something like this:
diff --git a/src/libosmo-mgcp/mgcp_codec.c b/src/libosmo-mgcp/mgcp_codec.c index 38aa0a79f..f6dfbd66b 100644 --- a/src/libosmo-mgcp/mgcp_codec.c +++ b/src/libosmo-mgcp/mgcp_codec.c @@ -366,7 +366,7 @@ bool mgcp_codec_amr_is_octet_aligned(const struct mgcp_rtp_codec *codec) /* Compare two codecs, all parameters must match up, except for the payload type * number. */ -static bool codecs_same(struct mgcp_rtp_codec *codec_a, struct mgcp_rtp_codec *codec_b) +static bool codecs_same(struct mgcp_rtp_codec *codec_a, struct mgcp_rtp_codec *codec_b, bool allow_transcoding) { if (codec_a->rate != codec_b->rate) return false; @@ -378,10 +378,10 @@ static bool codecs_same(struct mgcp_rtp_codec *codec_a, struct mgcp_rtp_codec *c return false; if (strcmp(codec_a->subtype_name, codec_b->subtype_name)) return false; - - /* Note: AMR allows to set the RTP payload format to octet-aligned or bandwith-efficient (octet-aligned=0) - * via SDP. This difference concerns payload format only, but not the actual codec. It is not a difference - * within the meaning of this function. */ + if (!strcmp(codec_a->subtype_name, "AMR")) { + if (mgcp_codec_amr_is_octet_aligned(codec_a) != mgcp_codec_amr_is_octet_aligned(codec_b)) + return allow_transcoding; /* we support transcoding OA<->BE */ + } return true; } @@ -421,16 +421,19 @@ int mgcp_codec_pt_translate(struct mgcp_conn_rtp *conn_src, struct mgcp_conn_rtp * equivalent of it on the destination side */ codecs_assigned = rtp_dst->codecs_assigned; OSMO_ASSERT(codecs_assigned <= MGCP_MAX_CODECS); + /* First attempt exact match, without transcoding: */ for (i = 0; i < codecs_assigned; i++) { - if (codecs_same(codec_src, &rtp_dst->codecs[i])) { - codec_dst = &rtp_dst->codecs[i]; - break; - } + if (codecs_same(codec_src, &rtp_dst->codecs[i], false); + return rtp_dst->codecs[i]->payload_type; + } + /* No exact match, try again with transcoding enabled: */ + for (i = 0; i < codecs_assigned; i++) { + if (codecs_same(codec_src, &rtp_dst->codecs[i], true); + return rtp_dst->codecs[i]->payload_type; } - if (!codec_dst) - return -EINVAL; - return codec_dst->payload_type; + /* No matching codec found: */ + return -EINVAL; }
We should add TTCN3/unit tests validating scenarios like the one described above, and make sure we do the intelligent thing.
[1] https://gerrit.osmocom.org/c/osmo-mgw/+/27223
[2] https://gerrit.osmocom.org/c/osmo-mgw/+/15136