Project

General

Profile

Actions

Bug #5461

closed

Fix regression translating AMR Payload Type between connections

Added by pespin over 2 years ago. Updated about 1 year ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
02/18/2022
Due date:
% Done:

100%

Spec Reference:

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
And connection B, the other connection in the same endpoint, announces it supports both in SDP, let's say:
  • 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

Actions #1

Updated by neels over 2 years ago

Sounds good to me, thanks for the explanation, Pau!

Actions #2

Updated by dexter over 2 years ago

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

To ensure that we do not run again in the bwe<->oa conversion problem we have seen recently I have increased the test coverage in the TTCN3 tests. When I revert my previous patch, then TC_amr_oa_bwe_rtp_conversion fails, which shows that it works.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/27384 MGCP_Test: ensure PT translation works when converting AMR bwe/oa

I have now changed the mgcp_codec_pt_translate() and the related unit tests. The behavior is only slightly changed, but I don't think that this is of any practical relevance.

https://gerrit.osmocom.org/c/osmo-mgw/+/27391 Revert "mgcp_codec: do not differentiate between oa and bwe when comparing ...
https://gerrit.osmocom.org/c/osmo-mgw/+/27392 mgcp_codec: fix oa/bwe comparison in mgcp_codec_pt_translate()

We probably might investigate though what really happens when one side has for example octet-aligned and the other side has both payload formats assigned. I doubt that this will work with the current code. For that kind of case we will need a TTCN3 test and probably make the choice of the egress codec a little more dynamic. Apart from that I think this fix is restoring the intended behavior as it was.

When I am understanding the issue correctly, then the concern was that a PBX on the SIP side may assign two AMR modes with two different PT numbers for compatibility reasons.

Actions #3

Updated by dexter over 2 years ago

So a concrete scenario would be:

On the osmo-cn side use e.g. PT 112 for AMR in oa.

On the SIP side we use PT 113 for AMR in oa and PT 114 for AMR in bwe.

==> The MGW will notice that 112<->113 is requiring no format conversion, it will send out the packets to the SIP side under PT 113, while sending no packets under PT 114 at all. Any bwe packet that it receives under PT 114 will be converted to oe and forwarded to the osmo-cn side under PT 112. Any packet that osmo-mgw receives under PT 113 will just forwarded unchanged to the osmo-cn side under PT 112 as it is already oa formatted.

Actions #4

Updated by pespin over 2 years ago

dexter wrote:

When I am understanding the issue correctly, then the concern was that a PBX on the SIP side may assign two AMR modes with two different PT numbers for compatibility reasons.

Yes correct, that's the scenario I had in mind.

So a concrete scenario would be:

On the osmo-cn side use e.g. PT 112 for AMR in oa.

You probably mean the osmo-bss side here, since the one handling SIP is the MSC (which is the CN).

On the SIP side we use PT 113 for AMR in oa and PT 114 for AMR in bwe.

==> The MGW will notice that 112<->113 is requiring no format conversion, it will send out the packets to the SIP side under PT 113, while sending no packets under PT 114 at all. Any bwe packet that it receives under PT 114 will be converted to oe and forwarded to the osmo-cn side under PT 112. Any packet that osmo-mgw receives under PT 113 will just forwarded unchanged to the osmo-cn side under PT 112 as it is already oa formatted.

Yes, that's the ideal solution IMHO.

I think it should be failrly easy to write a TTCN3 test validating that exact scenario, and then we make sure we have no regressions there, which may be difficult to spot.

Actions #5

Updated by dexter over 1 year ago

I have rebased the two pending patches:

https://gerrit.osmocom.org/c/osmo-mgw/+/27391 Revert "mgcp_codec: do not differentiate between oa and bwe when comparing ...
https://gerrit.osmocom.org/c/osmo-mgw/+/27392 mgcp_codec: fix oa/bwe comparison in mgcp_codec_pt_translate()

Both patches must be merged at the same time since the first one is just a revert to a previous patch.

The next task here would be to create a TTCN3 test that simulates the behavior discussed above.

Actions #6

Updated by dexter about 1 year ago

I have now fixed the review issues and also refactored the code mgcp_codec.c a bit, see patch: https://gerrit.osmocom.org/c/osmo-mgw/+/32083 mgcp_codec: refactor payload type converstion

Actions #7

Updated by dexter about 1 year ago

I am almost done with extending the RTP Emulation and the MGCP_Test so that it supports multiple codecs with multiple pairs of RX/TX octet strings. Some bits are missing but once it is done we can verify if the octet-align/banwith-efficient conversation works as intended when both modes are allowed on one side.

Actions #8

Updated by dexter about 1 year ago

The patch that extends the RTP-Emulation and the MCGP/SDP generation is now up for review: https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/32153

Actions #9

Updated by dexter about 1 year ago

  • % Done changed from 50 to 80

I have now added a few more testcases that specifically address the problem when AMR is assigned twice with octet_aligned=0 and octet_aligned=1:

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/32220 MGCP_Test: add new testcases for oa/bwe conversion

The added tests showed that there are problems with the codec decision. This is now also fixed: https://gerrit.osmocom.org/c/osmo-mgw/+/32218

Actions #10

Updated by dexter about 1 year ago

The patchset for osmo-mgw seems to have no open issues, but still requires some more review:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218
https://gerrit.osmocom.org/c/osmo-mgw/+/32290

The corresponding patchset for the MGCP TTCN3 tests is almost done. There is only one patch that does not have enough review points yet:
https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/32153
(as far as I can see everything is ok here)

Actions #11

Updated by dexter about 1 year ago

  • % Done changed from 80 to 90

The following patches are currently still in review:

https://gerrit.osmocom.org/c/osmo-mgw/+/32506 mgcp_codec: move mgcp_codec_decide down
https://gerrit.osmocom.org/c/osmo-mgw/+/32218 mgcp_codec: fix codec decision
https://gerrit.osmocom.org/c/osmo-mgw/+/32290 mgcp_network: do not deliver RTP packets with unpatched PT

I noticed that the related TTCN3 patch is ready to submit but I think its better to wait until the mgw patches are ready as well.

Actions #12

Updated by dexter about 1 year ago

I have now merged the part of the TTCN3 patchset that introduces support for multiple RTP payloads since those patches do not change the testsuite itself. This probably makes more sense so anyone can use the multiple RTP payload feature now and we also may avoid merge conflicts.

The following patch is not yet merged since it adds testcase specific to the codec decision fixes in osmo-mgw (see above), which do not have enough review points yet.
https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/32220 MGCP_Test: add new testcases for oa/bwe conversion

Actions #13

Updated by dexter about 1 year ago

The following patch had review issues, which are fixed by now. It is also the only patch in this patch set that is not ready yet:
https://gerrit.osmocom.org/c/osmo-mgw/+/32218 mgcp_codec: fix codec decision

Actions #14

Updated by dexter about 1 year ago

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

This can now be resolved. Unfortunately the patchset caused problems with MGCP_Test.TC_two_crcx_mdcx_and_iuup_rtp and MGCP_Test.TC_two_crcx_mdcx_and_iuup_rtp_rfci_unordered because IuFP was not taken into account. This is now fixed and the two affected TTCN3 tests pass again.

Actions

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)