dexter has uploaded this change for review.
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, 88 insertions(+), 26 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/92/27392/1
diff --git a/src/libosmo-mgcp/mgcp_codec.c b/src/libosmo-mgcp/mgcp_codec.c
index 6b8d14a..0b806d9 100644
--- a/src/libosmo-mgcp/mgcp_codec.c
+++ b/src/libosmo-mgcp/mgcp_codec.c
@@ -364,9 +364,9 @@
return codec->param.amr_octet_aligned;
}
-/* 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)
+/* Compare two codecs, all parameters must match up, except for the payload type number. When comparing two codecs,
+ * we have the option to match the payload format as well. */
+static bool codecs_same(struct mgcp_rtp_codec *codec_a, struct mgcp_rtp_codec *codec_b, bool match_pload_fmt)
{
if (codec_a->rate != codec_b->rate)
return false;
@@ -378,7 +378,7 @@
return false;
if (strcmp(codec_a->subtype_name, codec_b->subtype_name))
return false;
- if (!strcmp(codec_a->subtype_name, "AMR")) {
+ if (match_pload_fmt && !strcmp(codec_a->subtype_name, "AMR")) {
if (mgcp_codec_amr_is_octet_aligned(codec_a) != mgcp_codec_amr_is_octet_aligned(codec_b))
return false;
}
@@ -417,16 +417,28 @@
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++) {
- if (codecs_same(codec_src, &rtp_dst->codecs[i])) {
+ if (codecs_same(codec_src, &rtp_dst->codecs[i], true)) {
codec_dst = &rtp_dst->codecs[i];
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. bwe->oe) */
+ if (!codec_dst) {
+ for (i = 0; i < codecs_assigned; i++) {
+ if (codecs_same(codec_src, &rtp_dst->codecs[i], false)) {
+ 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 9b4933c..ea45b22 100644
--- a/tests/mgcp/mgcp_test.c
+++ b/tests/mgcp/mgcp_test.c
@@ -1850,6 +1850,7 @@
{ .payload_type_map = {96, 97}, },
{ .payload_type_map = {97, 96}, },
{ .payload_type_map = {0, 0}, },
+ { .payload_type_map = {123, -EINVAL} },
{ .end = true },
},
},
@@ -1906,31 +1907,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, },
},
{
@@ -1938,8 +1971,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
@@ -1324,6 +1324,7 @@
- mgcp_codec_pt_translate(conn1, conn0, 96) -> 97
- mgcp_codec_pt_translate(conn0, conn1, 0) -> 0
- mgcp_codec_pt_translate(conn1, conn0, 0) -> 0
+ - mgcp_codec_pt_translate(conn0, conn1, 123) -> -22
#3: conn0 has no codecs
- add codecs on conn0:
(none)
@@ -1355,25 +1356,40 @@
- mgcp_codec_pt_translate(conn1, conn0, 121) -> 111
- mgcp_codec_pt_translate(conn0, conn1, 112) -> 122
- mgcp_codec_pt_translate(conn1, conn0, 122) -> 112
-#6: test AMR with missing octet-aligned settings (defaults to 0)
+ - mgcp_codec_pt_translate(conn0, conn1, 123) -> -22
+#6: test AMR with missing octet-aligned settings (oa <-> unset)
- add codecs on conn0:
0: 111 AMR/8000 octet-aligned=1 -> rc=0
- 1: 112 AMR/8000 octet-aligned=0 -> rc=0
- add codecs on conn1:
0: 122 AMR/8000 octet-aligned=unset -> rc=0
- - mgcp_codec_pt_translate(conn0, conn1, 111) -> -22
- - mgcp_codec_pt_translate(conn0, conn1, 112) -> 122
- - mgcp_codec_pt_translate(conn1, conn0, 122) -> 112
-#7: test AMR with NULL param (defaults to 0)
+ - mgcp_codec_pt_translate(conn0, conn1, 111) -> 122
+ - mgcp_codec_pt_translate(conn1, conn0, 122) -> 111
+ - mgcp_codec_pt_translate(conn0, conn1, 55) -> -22
+#7: test AMR with missing octet-aligned settings (bwe <-> unset)
- add codecs on conn0:
- 0: 111 AMR/8000 octet-aligned=1 -> rc=0
- 1: 112 AMR/8000 octet-aligned=0 -> rc=0
+ 0: 111 AMR/8000 octet-aligned=0 -> rc=0
+ - add codecs on conn1:
+ 0: 122 AMR/8000 octet-aligned=unset -> rc=0
+ - mgcp_codec_pt_translate(conn0, conn1, 111) -> 122
+ - mgcp_codec_pt_translate(conn1, conn0, 122) -> 111
+ - mgcp_codec_pt_translate(conn0, conn1, 55) -> -22
+#8: test AMR with NULL param (oa <-> null)
+ - add codecs on conn0:
+ 0: 112 AMR/8000 octet-aligned=1 -> rc=0
- add codecs on conn1:
0: 122 AMR/8000 -> rc=0
- - mgcp_codec_pt_translate(conn0, conn1, 111) -> -22
- mgcp_codec_pt_translate(conn0, conn1, 112) -> 122
- mgcp_codec_pt_translate(conn1, conn0, 122) -> 112
-#8: match FOO/8000/1 and FOO/8000 as identical, single channel is implicit
+ - mgcp_codec_pt_translate(conn0, conn1, 55) -> -22
+#9: test AMR with NULL param (bwe <-> null)
+ - add codecs on conn0:
+ 0: 112 AMR/8000 octet-aligned=0 -> rc=0
+ - add codecs on conn1:
+ 0: 122 AMR/8000 -> rc=0
+ - mgcp_codec_pt_translate(conn0, conn1, 112) -> 122
+ - mgcp_codec_pt_translate(conn1, conn0, 122) -> 112
+ - mgcp_codec_pt_translate(conn0, conn1, 55) -> -22
+#10: match FOO/8000/1 and FOO/8000 as identical, single channel is implicit
- add codecs on conn0:
0: 0 PCMU/8000/1 -> rc=0
1: 111 GSM-HR-08/8000/1 -> rc=0
@@ -1389,7 +1405,7 @@
- mgcp_codec_pt_translate(conn0, conn1, 111) -> 97
- mgcp_codec_pt_translate(conn1, conn0, 97) -> 111
- mgcp_codec_pt_translate(conn0, conn1, 123) -> -22
-#9: match FOO/8000/1 and FOO as identical, 8k and single channel are implicit
+#11: match FOO/8000/1 and FOO as identical, 8k and single channel are implicit
- add codecs on conn0:
0: 0 PCMU/8000/1 -> rc=0
1: 111 GSM-HR-08/8000/1 -> rc=0
@@ -1405,7 +1421,7 @@
- mgcp_codec_pt_translate(conn0, conn1, 111) -> 97
- mgcp_codec_pt_translate(conn1, conn0, 97) -> 111
- mgcp_codec_pt_translate(conn0, conn1, 123) -> -22
-#10: test whether channel number matching is waterproof
+#12: test whether channel number matching is waterproof
- add codecs on conn0:
0: 111 GSM-HR-08/8000 -> rc=0
1: 112 GSM-HR-08/8000/2 -> rc=-22
To view, visit change 27392. To unsubscribe, or for help writing mail filters, visit settings.