Change in ...osmo-mgw[master]: ptmap: implicitly match '/8000' and '/8000/1'
neels has submitted this change and it was merged. (
https://gerrit.osmocom.org/c/osmo-mgw/+/15137 )
Change subject: ptmap: implicitly match '/8000' and '/8000/1'
..
ptmap: implicitly match '/8000' and '/8000/1'
In codecs_same(), do not compare the complete audio_name. The parts of it are
already checked individually:
- subtype_name ("AMR"),
- rate ("8000"; defaults to 8000 if omitted) and
- channels ("1"; defaults to 1 if omitted)
So by also checking the complete audio_name, we brushed over the match of
implicit "/8000" and "/8000/1", which otherwise works out fine.
As a result, translating payload type numbers in RTP headers now also works if
one conn of an endpoint set an rtpmap with "AMR/8000" and the other conn set
"AMR/8000/1".
It seems to me that most PBX out there generate ptmaps omitting the "/1", so
fixing this should make us more interoperable with third party SDP.
See IETF RFC4566 section 6. SDP Attributes:
For audio streams, indicates the number
of audio channels. This parameter is OPTIONAL and may be
omitted if the number of channels is one, provided that no
additional parameters are needed.
Also allowing to omit the "/8000" is a mere side effect of this patch.
Omitting the rate does not seem to be specified in an RFC, but is logical for
audio codecs defined to require exactly 8000 set as rate (most GSM codecs).
Add tests in mgcp_test.c.
Change-Id: Iab00bf9a55b1847f85999077114b37e70fb677c2
---
M src/libosmo-mgcp/mgcp_codec.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
3 files changed, 108 insertions(+), 2 deletions(-)
Approvals:
Jenkins Builder: Verified
neels: Looks good to me, approved
pespin: Looks good to me, but someone else must approve
diff --git a/src/libosmo-mgcp/mgcp_codec.c b/src/libosmo-mgcp/mgcp_codec.c
index 5d7f840..7c8f6d5 100644
--- a/src/libosmo-mgcp/mgcp_codec.c
+++ b/src/libosmo-mgcp/mgcp_codec.c
@@ -379,8 +379,6 @@
return false;
if (codec_a->frame_duration_den != codec_b->frame_duration_den)
return false;
- if (strcmp(codec_a->audio_name, codec_b->audio_name))
- return false;
if (strcmp(codec_a->subtype_name, codec_b->subtype_name))
return false;
if (!strcmp(codec_a->subtype_name, "AMR")) {
diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c
index 5ebe475..e5dec2a 100644
--- a/tests/mgcp/mgcp_test.c
+++ b/tests/mgcp/mgcp_test.c
@@ -1900,6 +1900,70 @@
{ .end = true },
},
},
+ {
+ .descr = "match FOO/8000/1 and FOO/8000 as identical, single
channel is implicit",
+ .codecs = {
+ {
+ { 0, "PCMU/8000/1", NULL, },
+ { 111, "GSM-HR-08/8000/1", NULL, },
+ { 112, "AMR/8000/1",
_param_octet_aligned_true, },
+ },
+ {
+ { 97, "GSM-HR-08/8000", NULL, },
+ { 0, "PCMU/8000", NULL, },
+ { 96, "AMR/8000",
_param_octet_aligned_true, },
+ },
+ },
+ .expect = {
+ { .payload_type_map = {112, 96}, },
+ { .payload_type_map = {0, 0}, },
+ { .payload_type_map = {111, 97} },
+ { .payload_type_map = {123, -EINVAL} },
+ { .end = true },
+ },
+ },
+ {
+ .descr = "match FOO/8000/1 and FOO as identical, 8k and single
channel are implicit",
+ .codecs = {
+ {
+ { 0, "PCMU/8000/1", NULL, },
+ { 111, "GSM-HR-08/8000/1", NULL, },
+ { 112, "AMR/8000/1",
_param_octet_aligned_true, },
+ },
+ {
+ { 97, "GSM-HR-08", NULL, },
+ { 0, "PCMU", NULL, },
+ { 96, "AMR", _param_octet_aligned_true, },
+ },
+ },
+ .expect = {
+ { .payload_type_map = {112, 96}, },
+ { .payload_type_map = {0, 0}, },
+ { .payload_type_map = {111, 97} },
+ { .payload_type_map = {123, -EINVAL} },
+ { .end = true },
+ },
+ },
+ {
+ .descr = "test whether channel number matching is waterproof",
+ .codecs = {
+ {
+ { 111, "GSM-HR-08/8000", },
+ { 112, "GSM-HR-08/8000/2", .expect_rc = -22},
+ { 113, "GSM-HR-08/8000/3", .expect_rc = -22},
+
Change in ...osmo-mgw[master]: ptmap: implicitly match '/8000' and '/8000/1'
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/15137 ) Change subject: ptmap: implicitly match '/8000' and '/8000/1' .. Patch Set 4: Code-Review+2 combining votes -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/15137 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Iab00bf9a55b1847f85999077114b37e70fb677c2 Gerrit-Change-Number: 15137 Gerrit-PatchSet: 4 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 28 Aug 2019 02:53:09 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: ptmap: implicitly match '/8000' and '/8000/1'
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/15137 ) Change subject: ptmap: implicitly match '/8000' and '/8000/1' .. Patch Set 3: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/15137 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Iab00bf9a55b1847f85999077114b37e70fb677c2 Gerrit-Change-Number: 15137 Gerrit-PatchSet: 3 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 13 Aug 2019 09:36:50 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: ptmap: implicitly match '/8000' and '/8000/1'
Hello pespin, laforge, Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-mgw/+/15137
to look at the new patch set (#3).
Change subject: ptmap: implicitly match '/8000' and '/8000/1'
..
ptmap: implicitly match '/8000' and '/8000/1'
In codecs_same(), do not compare the complete audio_name. The parts of it are
already checked individually:
- subtype_name ("AMR"),
- rate ("8000"; defaults to 8000 if omitted) and
- channels ("1"; defaults to 1 if omitted)
So by also checking the complete audio_name, we brushed over the match of
implicit "/8000" and "/8000/1", which otherwise works out fine.
As a result, translating payload type numbers in RTP headers now also works if
one conn of an endpoint set an rtpmap with "AMR/8000" and the other conn set
"AMR/8000/1".
It seems to me that most PBX out there generate ptmaps omitting the "/1", so
fixing this should make us more interoperable with third party SDP.
See IETF RFC4566 section 6. SDP Attributes:
For audio streams, indicates the number
of audio channels. This parameter is OPTIONAL and may be
omitted if the number of channels is one, provided that no
additional parameters are needed.
Also allowing to omit the "/8000" is a mere side effect of this patch.
Omitting the rate does not seem to be specified in an RFC, but is logical for
audio codecs defined to require exactly 8000 set as rate (most GSM codecs).
Add tests in mgcp_test.c.
Change-Id: Iab00bf9a55b1847f85999077114b37e70fb677c2
---
M src/libosmo-mgcp/mgcp_codec.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
3 files changed, 108 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/37/15137/3
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/15137
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: Iab00bf9a55b1847f85999077114b37e70fb677c2
Gerrit-Change-Number: 15137
Gerrit-PatchSet: 3
Gerrit-Owner: neels
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge
Gerrit-Reviewer: pespin
Gerrit-MessageType: newpatchset
Change in ...osmo-mgw[master]: ptmap: implicitly match '/8000' and '/8000/1'
Hello pespin, laforge, Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-mgw/+/15137
to look at the new patch set (#2).
Change subject: ptmap: implicitly match '/8000' and '/8000/1'
..
ptmap: implicitly match '/8000' and '/8000/1'
In codecs_cmp(), do not compare the complete audio_name. The parts of it are
already checked individually:
- subtype_name ("AMR"),
- rate ("8000"; defaults to 8000 if omitted) and
- channels ("1"; defaults to 1 if omitted)
So by also checking the complete audio_name, we brushed over the match of
implicit "/8000" and "/8000/1", which otherwise works out fine.
As a result, translating payload type numbers in RTP headers now also works if
one conn of an endpoint set an rtpmap with "AMR/8000" and the other conn set
"AMR/8000/1".
It seems to me that most PBX out there generate ptmaps omitting the "/1", so
fixing this should make us more interoperable with third party SDP.
See IETF RFC4566 section 6. SDP Attributes:
For audio streams, indicates the number
of audio channels. This parameter is OPTIONAL and may be
omitted if the number of channels is one, provided that no
additional parameters are needed.
Also allowing to omit the "/8000" is a mere side effect of this patch.
Omitting the rate does not seem to be specified in an RFC, but is logical for
audio codecs defined to require exactly 8000 set as rate (most GSM codecs).
Add tests in mgcp_test.c.
Change-Id: Iab00bf9a55b1847f85999077114b37e70fb677c2
---
M src/libosmo-mgcp/mgcp_codec.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
3 files changed, 108 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/37/15137/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/15137
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: Iab00bf9a55b1847f85999077114b37e70fb677c2
Gerrit-Change-Number: 15137
Gerrit-PatchSet: 2
Gerrit-Owner: neels
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge
Gerrit-Reviewer: pespin
Gerrit-MessageType: newpatchset
Change in ...osmo-mgw[master]: ptmap: implicitly match '/8000' and '/8000/1'
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/15137 ) Change subject: ptmap: implicitly match '/8000' and '/8000/1' .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/15137 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Iab00bf9a55b1847f85999077114b37e70fb677c2 Gerrit-Change-Number: 15137 Gerrit-PatchSet: 1 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-Comment-Date: Fri, 09 Aug 2019 11:14:49 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: ptmap: implicitly match '/8000' and '/8000/1'
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/15137 ) Change subject: ptmap: implicitly match '/8000' and '/8000/1' .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/15137 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Iab00bf9a55b1847f85999077114b37e70fb677c2 Gerrit-Change-Number: 15137 Gerrit-PatchSet: 1 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Comment-Date: Fri, 09 Aug 2019 08:22:20 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-mgw[master]: ptmap: implicitly match '/8000' and '/8000/1'
neels has uploaded this change for review. (
https://gerrit.osmocom.org/c/osmo-mgw/+/15137
Change subject: ptmap: implicitly match '/8000' and '/8000/1'
..
ptmap: implicitly match '/8000' and '/8000/1'
In codecs_cmp(), do not compare the complete audio_name. The parts of it are
already checked individually:
- subtype_name ("AMR"),
- rate ("8000"; defaults to 8000 if omitted) and
- channels ("1"; defaults to 1 if omitted)
So by also checking the complete audio_name, we brushed over the match of
implicit "/8000" and "/8000/1", which otherwise works out fine.
As a result, translating payload type numbers in RTP headers now also works if
one conn of an endpoint set an rtpmap with "AMR/8000" and the other conn set
"AMR/8000/1".
It seems to me that most PBX out there generate ptmaps omitting the "/1", so
fixing this should make us more interoperable with third party SDP.
See IETF RFC4566 section 6. SDP Attributes:
For audio streams, indicates the number
of audio channels. This parameter is OPTIONAL and may be
omitted if the number of channels is one, provided that no
additional parameters are needed.
Also allowing to omit the "/8000" is a mere side effect of this patch.
Omitting the rate does not seem to be specified in an RFC, but is logical for
audio codecs defined to require exactly 8000 set as rate (most GSM codecs).
Add tests in mgcp_test.c.
Change-Id: Iab00bf9a55b1847f85999077114b37e70fb677c2
---
M src/libosmo-mgcp/mgcp_codec.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
3 files changed, 92 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/37/15137/1
diff --git a/src/libosmo-mgcp/mgcp_codec.c b/src/libosmo-mgcp/mgcp_codec.c
index ead1412..a947bb3 100644
--- a/src/libosmo-mgcp/mgcp_codec.c
+++ b/src/libosmo-mgcp/mgcp_codec.c
@@ -380,8 +380,6 @@
return false;
if (codec_a->frame_duration_den != codec_b->frame_duration_den)
return false;
- if (strcmp(codec_a->audio_name, codec_b->audio_name))
- return false;
if (strcmp(codec_a->subtype_name, codec_b->subtype_name))
return false;
if (!strcmp(codec_a->subtype_name, "AMR")) {
diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c
index 5d4267f..8a37ef0 100644
--- a/tests/mgcp/mgcp_test.c
+++ b/tests/mgcp/mgcp_test.c
@@ -1900,6 +1900,70 @@
{ .end = true },
},
},
+ {
+ .descr = "match FOO/8000/1 and FOO/8000 as identical, single
channel is implicit",
+ .codecs = {
+ {
+ { 0, "PCMU/8000/1", NULL, },
+ { 111, "GSM-HR-08/8000/1", NULL, },
+ { 112, "AMR/8000/1",
_param_octet_aligned_true, },
+ },
+ {
+ { 97, "GSM-HR-08/8000", NULL, },
+ { 0, "PCMU/8000", NULL, },
+ { 96, "AMR/8000",
_param_octet_aligned_true, },
+ },
+ },
+ .expect = {
+ { .payload_type_map = {112, 96}, },
+ { .payload_type_map = {0, 0}, },
+ { .payload_type_map = {111, 97} },
+ { .payload_type_map = {123, -EINVAL} },
+ { .end = true },
+ },
+ },
+ {
+ .descr = "match FOO/8000/1 and FOO as identical, 8k and single
channel are implicit",
+ .codecs = {
+ {
+ { 0, "PCMU/8000/1", NULL, },
+ { 111, "GSM-HR-08/8000/1", NULL, },
+ { 112, "AMR/8000/1",
_param_octet_aligned_true, },
+ },
+ {
+ { 97, "GSM-HR-08", NULL, },
+ { 0, "PCMU", NULL, },
+ { 96, "AMR", _param_octet_aligned_true, },
+ },
+ },
+ .expect = {
+ { .payload_type_map = {112, 96}, },
+ { .payload_type_map = {0, 0}, },
+ { .payload_type_map = {111, 97} },
+ { .payload_type_map = {123, -EINVAL} },
+ { .end = true },
+ },
+ },
+ {
+ .descr = "test whether channel number matching is waterproof",
+ .codecs = {
+ {
+ { 111, "GSM-HR-08/8000", },
+ { 112, "GSM-HR-08/8000/2", .expect_rc = -22},
+ { 113, "GSM-HR-08/8000/3", .expect_rc = -22},
+ },
+ {
+
