laforge has submitted this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/27392 ) Change subject: mgcp_codec: fix oa/bwe comparison in mgcp_codec_pt_translate() ...................................................................... mgcp_codec: fix oa/bwe comparison in mgcp_codec_pt_translate() The function mgcp_codec_pt_translate is very strict when comparing the codecs to each other to find a matching payload type number to be used on the egress side. This poses a problem when one side uses AMR in bandwith-efficient, while the other side uses AMR in octet-aligned payload type format. To the pt translate function the difference in the payload format will appear as if the codec were different and eventually the payload type number cannot be translated. since osmo-mgw offers conversion between the payload type format it would be no problem to ignore the payload type format when making the translation decision. The only exception here would be if one side would announce AMR two times, the first time with octet-aligned and the second time with bandwith-efficient format. Then we would have to use the payload type number from the exact match. (and skip any formatconversion) To archive such an optimized decision we will first go through the codec lists and perform an exact match. If we don't get a match we go through the codec lists a second time, but this time we ignore the payload format. Change-Id: Ifbd201a2749009a4644a29bd77e1d0fc0c124a9d Related: OS#5461 --- M src/libosmo-mgcp/mgcp_codec.c M tests/mgcp/mgcp_test.c M tests/mgcp/mgcp_test.ok 3 files changed, 119 insertions(+), 14 deletions(-) Approvals: Jenkins Builder: Verified pespin: Looks good to me, but someone else must approve laforge: Looks good to me, approved diff --git a/src/libosmo-mgcp/mgcp_codec.c b/src/libosmo-mgcp/mgcp_codec.c index c287b7e..ccb5d77 100644 --- a/src/libosmo-mgcp/mgcp_codec.c +++ b/src/libosmo-mgcp/mgcp_codec.c @@ -376,10 +376,15 @@ return codec->param.amr_octet_aligned; } -/* Compare two codecs, all parameters must match up, except for the payload type - * number. */ +/* Compare two codecs, all parameters must match up */ static bool codecs_same(struct mgcp_rtp_codec *codec_a, struct mgcp_rtp_codec *codec_b) { + /* All codec properties must match up, except the payload type number. Even though standardisd payload numbers + * exist for certain situations, the call agent may still assign them freely. Hence we must not insist on equal + * payload type numbers. Also the audio_name is not checked since it is already parsed into subtype_name, rate, + * and channels, which are checked. */ + if (strcmp(codec_a->subtype_name, codec_b->subtype_name)) + return false; if (codec_a->rate != codec_b->rate) return false; if (codec_a->channels != codec_b->channels) @@ -388,9 +393,11 @@ return false; if (codec_a->frame_duration_den != codec_b->frame_duration_den) return false; - if (strcmp(codec_a->subtype_name, codec_b->subtype_name)) - return false; - if (!strcmp(codec_a->subtype_name, "AMR")) { + + /* AMR payload may be formatted in two different payload formats, it is still the same codec but since the + * formatting of the payload is different, conversation is required, so we must treat it as a different + * codec here. */ + if (strcmp(codec_a->subtype_name, "AMR") == 0) { if (mgcp_codec_amr_is_octet_aligned(codec_a) != mgcp_codec_amr_is_octet_aligned(codec_b)) return false; } @@ -398,6 +405,26 @@ return true; } +/* Compare two codecs, all parameters must match up, except parameters related to payload formatting (not checked). */ +static bool codecs_convertible(struct mgcp_rtp_codec *codec_a, struct mgcp_rtp_codec *codec_b) +{ + /* OsmoMGW currently has no ability to transcode from one codec to another. However OsmoMGW is still able to + * translate between different payload formats as long as the encoded voice data itself does not change. + * Therefore we must insist on equal codecs but still allow different payload formatting. */ + if (strcmp(codec_a->subtype_name, codec_b->subtype_name)) + return false; + if (codec_a->rate != codec_b->rate) + return false; + if (codec_a->channels != codec_b->channels) + return false; + if (codec_a->frame_duration_num != codec_b->frame_duration_num) + return false; + if (codec_a->frame_duration_den != codec_b->frame_duration_den) + return false; + + return true; +} + /*! Translate a given payload type number that belongs to the packet of a * source connection to the equivalent payload type number that matches the * configuration of a destination connection. @@ -429,8 +456,8 @@ if (!codec_src) return -EINVAL; - /* Use the codec information from the source and try to find the - * equivalent of it on the destination side */ + /* Use the codec information from the source and try to find the equivalent of it on the destination side. In + * the first run we will look for an exact match. */ codecs_assigned = rtp_dst->codecs_assigned; OSMO_ASSERT(codecs_assigned <= MGCP_MAX_CODECS); for (i = 0; i < codecs_assigned; i++) { @@ -439,6 +466,18 @@ break; } } + + /* In case we weren't able to find an exact match, we will try to find a match that is the same codec, but the + * payload format may be different. This alternative will require a frame format conversion (i.e. AMR bwe->oe) */ + if (!codec_dst) { + for (i = 0; i < codecs_assigned; i++) { + if (codecs_convertible(codec_src, &rtp_dst->codecs[i])) { + codec_dst = &rtp_dst->codecs[i]; + break; + } + } + } + if (!codec_dst) return -EINVAL; diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c index 444e07a..8f081a9 100644 --- a/tests/mgcp/mgcp_test.c +++ b/tests/mgcp/mgcp_test.c @@ -1851,6 +1851,7 @@ { .payload_type_map = {96, 97}, }, { .payload_type_map = {97, 96}, }, { .payload_type_map = {0, 0}, }, + { .payload_type_map = {123, -EINVAL} }, { .end = true }, }, }, @@ -1907,31 +1908,63 @@ .expect = { { .payload_type_map = {111, 121}, }, { .payload_type_map = {112, 122} }, + { .payload_type_map = {123, -EINVAL} }, { .end = true }, }, }, { - .descr = "test AMR with missing octet-aligned settings (defaults to 0)", + .descr = "test AMR with missing octet-aligned settings (oa <-> unset)", .codecs = { { { 111, "AMR/8000", &amr_param_octet_aligned_true, }, - { 112, "AMR/8000", &amr_param_octet_aligned_false, }, }, { { 122, "AMR/8000", &amr_param_octet_aligned_unset, }, }, }, .expect = { - { .payload_type_map = {111, -EINVAL}, }, - { .payload_type_map = {112, 122} }, + { .payload_type_map = {111, 122}, }, + { .payload_type_map = {55, -EINVAL}, }, { .end = true }, }, }, { - .descr = "test AMR with NULL param (defaults to 0)", + .descr = "test AMR with missing octet-aligned settings (bwe <-> unset)", .codecs = { { - { 111, "AMR/8000", &amr_param_octet_aligned_true, }, + { 111, "AMR/8000", &amr_param_octet_aligned_false, }, + }, + { + { 122, "AMR/8000", &amr_param_octet_aligned_unset, }, + }, + }, + .expect = { + { .payload_type_map = {111, 122}, }, + { .payload_type_map = {55, -EINVAL}, }, + { .end = true }, + }, + }, + { + .descr = "test AMR with NULL param (oa <-> null)", + .codecs = { + { + { 112, "AMR/8000", &amr_param_octet_aligned_true, }, + }, + { + { 122, "AMR/8000", NULL, }, + }, + }, + .expect = { + /* Note: Both 111, anbd 112 will translate to 122. The translation from 112 */ + { .payload_type_map = {112, 122} }, + { .payload_type_map = {55, -EINVAL}, }, + { .end = true }, + }, + }, + { + .descr = "test AMR with NULL param (bwe <-> null)", + .codecs = { + { { 112, "AMR/8000", &amr_param_octet_aligned_false, }, }, { @@ -1939,8 +1972,9 @@ }, }, .expect = { - { .payload_type_map = {111, -EINVAL}, }, + /* Note: Both 111, anbd 112 will translate to 122. The translation from 112 */ { .payload_type_map = {112, 122} }, + { .payload_type_map = {55, -EINVAL}, }, { .end = true }, }, }, diff --git a/tests/mgcp/mgcp_test.ok b/tests/mgcp/mgcp_test.ok index 94fada3..8b3b3d1 100644 --- a/tests/mgcp/mgcp_test.ok +++ b/tests/mgcp/mgcp_test.ok Binary files differ -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/27392 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Ifbd201a2749009a4644a29bd77e1d0fc0c124a9d Gerrit-Change-Number: 27392 Gerrit-PatchSet: 5 Gerrit-Owner: dexter &lt;pmaier(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: msuraev &lt;msuraev(a)sysmocom.de&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: merged