May 2023 - gerrit-log - lists.osmocom.org

[S] Change in osmo-hnbgw[master]: vty_go_parent(): remove legacy cruft

by pespin

Attention is currently required from: neels. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/32914 ) Change subject: vty_go_parent(): remove legacy cruft ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/32914 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hnbgw Gerrit-Branch: master Gerrit-Change-Id: Id3ecc53885436294d2217c52130910872dde550e Gerrit-Change-Number: 32914 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Comment-Date: Tue, 23 May 2023 08:25:07 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 1 month

1
0
0 0

[S] Change in osmo-hnbgw[master]: unbloat: drop context_map_check_released()

by pespin

Attention is currently required from: neels. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/32913 ) Change subject: unbloat: drop context_map_check_released() ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/32913 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hnbgw Gerrit-Branch: master Gerrit-Change-Id: I535780de0d3b09893ba89d66804e5e36b26db049 Gerrit-Change-Number: 32913 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Comment-Date: Tue, 23 May 2023 08:24:47 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 1 month

1
0
0 0

[S] Change in osmo-hnbgw[master]: remove obsolete context_map_check_released() call

by pespin

Attention is currently required from: neels. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/32912 ) Change subject: remove obsolete context_map_check_released() call ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/32912 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hnbgw Gerrit-Branch: master Gerrit-Change-Id: I4bf9a9de7861296ec38c3a37de0254ad9a41ef1b Gerrit-Change-Number: 32912 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Comment-Date: Tue, 23 May 2023 08:23:36 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 1 month

1
0
0 0

[S] Change in libosmocore[master]: gsm620: make osmo_hr_check_sid compatible with RFC 5993

by pespin

Attention is currently required from: falconia, fixeria, dexter. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/32740 ) Change subject: gsm620: make osmo_hr_check_sid compatible with RFC 5993 ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32740 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I25728299b757fbc87dd1b3f5adaec9b8b240c5d1 Gerrit-Change-Number: 32740 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: falconia <falcon(a)freecalypso.org> Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: falconia <falcon(a)freecalypso.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Tue, 23 May 2023 08:22:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

2 years, 1 month

1
0
0 0

[S] Change in libosmocore[master]: gsm620: make osmo_hr_check_sid compatible with RFC 5993

by dexter

Attention is currently required from: falconia, fixeria, pespin. dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/32740 ) Change subject: gsm620: make osmo_hr_check_sid compatible with RFC 5993 ...................................................................... Patch Set 2: (2 comments) Commit Message: https://gerrit.osmocom.org/c/libosmocore/+/32740/comment/e685a4c8_4ef0857b PS1, Line 7: gsm630: make osmo_hr_check_sid compatible with RFC 5993 > I assume you meant gsm620 here. Done Patchset: PS1: > @fixeria - I concur. […] I was thinking about normalizing the to one format at the beginning but I quickly realized that this would mean that we would often end up with two unnecessary conversions. This is something I wanted to avoid. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32740 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I25728299b757fbc87dd1b3f5adaec9b8b240c5d1 Gerrit-Change-Number: 32740 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: falconia <falcon(a)freecalypso.org> Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: falconia <falcon(a)freecalypso.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 23 May 2023 07:57:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: falconia <falcon(a)freecalypso.org> Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 1 month

1
0
0 0

[S] Change in libosmocore[master]: gsm620: make osmo_hr_check_sid compatible with RFC 5993

by dexter

Attention is currently required from: falconia, fixeria, pespin. Hello Jenkins Builder, falconia, pespin, fixeria, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/32740 to look at the new patch set (#2). Change subject: gsm620: make osmo_hr_check_sid compatible with RFC 5993 ...................................................................... gsm620: make osmo_hr_check_sid compatible with RFC 5993 RFC 5993 specifies a one byte TOC at the beginning of the RTP payload, while TS 101 318 does not have such a TOC byte. Since both formats are used in the field, let's make sure that osmo_hr_check_sid will work with both formats. Related: OS#5688 Related: OS#5996 Change-Id: I25728299b757fbc87dd1b3f5adaec9b8b240c5d1 --- M src/codec/gsm620.c 1 file changed, 28 insertions(+), 4 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/40/32740/2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32740 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I25728299b757fbc87dd1b3f5adaec9b8b240c5d1 Gerrit-Change-Number: 32740 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: falconia <falcon(a)freecalypso.org> Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: falconia <falcon(a)freecalypso.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

2 years, 1 month

1
0
0 0

[S] Change in osmo-mgw[master]: mgcp_network: do not deliver RTP packets with unpatched PT

by dexter

dexter has submitted this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32290 ) ( 6 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: mgcp_network: do not deliver RTP packets with unpatched PT ...................................................................... mgcp_network: do not deliver RTP packets with unpatched PT When a call leg is set up, then the call agent (e.g. BSC, MSC, etc.) will also negotiate a codec along with a payload type number. When sending RTP packets, each RTP packet must also contain the negotiated payload type number. To prevent the emission of RTP packets with an incorrect payload type number, ensure that no packet is sent when mgcp_patch_pt() fails. Change-Id: I013a24c1e0f853557257368cfab9192d4611aafa Related: OS#5461 --- M src/libosmo-mgcp/mgcp_network.c 1 file changed, 20 insertions(+), 12 deletions(-) Approvals: pespin: Looks good to me, but someone else must approve laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/libosmo-mgcp/mgcp_network.c b/src/libosmo-mgcp/mgcp_network.c index 6b16907..33cd8af 100644 --- a/src/libosmo-mgcp/mgcp_network.c +++ b/src/libosmo-mgcp/mgcp_network.c @@ -1164,18 +1164,9 @@ * IuUP -> AMR: calls this function, skip patching if conn_src is IuUP. * {AMR or IuUP} -> IuUP: calls mgcp_udp_send() directly, skipping this function: No need to examine dst. */ if (is_rtp && !mgcp_conn_rtp_is_iuup(conn_src)) { - rc = mgcp_patch_pt(conn_dst, msg); - if (rc < 0) { - /* FIXME: It is legal that the payload type on the egress connection is - * different from the payload type that has been negotiated on the - * ingress connection. Essentially the codecs are the same so we can - * match them and patch the payload type. However, if we can not find - * the codec pendant (everything ist equal except the PT), we are of - * course unable to patch the payload type. A situation like this - * should not occur if transcoding is consequently avoided. Until - * we have transcoding support in osmo-mgw we can not resolve this. */ - LOGPENDP(endp, DRTP, LOGL_DEBUG, - "can not patch PT because no suitable egress codec was found.\n"); + if (mgcp_patch_pt(conn_dst, msg) < 0) { + LOGPENDP(endp, DRTP, LOGL_NOTICE, "unable to patch payload type RTP packet, discarding...\n"); + return -EINVAL; } } -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32290 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I013a24c1e0f853557257368cfab9192d4611aafa Gerrit-Change-Number: 32290 Gerrit-PatchSet: 8 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 1 month

1
0
0 0

[L] Change in osmo-mgw[master]: mgcp_codec: fix codec decision

by dexter

dexter has submitted this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32218 ) Change subject: mgcp_codec: fix codec decision ...................................................................... mgcp_codec: fix codec decision Unfortunately OsmoMGW was never really tested with multiple different codecs on either side of the connection. While OsmoMSC and OsmoBSC only assign exactly one codec on each side this has never been a problem, however it might become a problem when a call agent assigns multiple codecs on one side. This has been observed in a setup where OsmoMGW had one leg towards an external call agent. Also due to recent upgrades to the TTCN3 tests we are now able to simulate different codecs on both sides to pinpoint issues. Testing has shown that OsmoMGW has difficulties with multiple codecs. The reason for this is that the function that makes the codec decision was not fully finished. So let's finish the codec decision function and let's also use that decision when patching the payload type of outgoing RTP packets. Related: OS#5461 Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5 --- M include/osmocom/mgcp/mgcp_codec.h M include/osmocom/mgcp/mgcp_trunk.h M src/libosmo-mgcp/mgcp_codec.c M src/libosmo-mgcp/mgcp_network.c M src/libosmo-mgcp/mgcp_protocol.c M src/libosmo-mgcp/mgcp_vty.c M tests/mgcp/mgcp_test.c M tests/mgcp/mgcp_test.ok 8 files changed, 256 insertions(+), 232 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve pespin: Looks good to me, approved diff --git a/include/osmocom/mgcp/mgcp_codec.h b/include/osmocom/mgcp/mgcp_codec.h index 1f29b08..cfc8ecf 100644 --- a/include/osmocom/mgcp/mgcp_codec.h +++ b/include/osmocom/mgcp/mgcp_codec.h @@ -13,8 +13,7 @@ void mgcp_codec_summary(struct mgcp_conn_rtp *conn); void mgcp_codec_reset_all(struct mgcp_conn_rtp *conn); int mgcp_codec_add(struct mgcp_conn_rtp *conn, int payload_type, const char *audio_name, const struct mgcp_codec_param *param); -int mgcp_codec_decide(struct mgcp_conn_rtp *conn); -struct mgcp_rtp_codec *mgcp_codec_find_convertible(struct mgcp_conn_rtp *conn, struct mgcp_rtp_codec *codec); +int mgcp_codec_decide(struct mgcp_conn_rtp *conn_src, struct mgcp_conn_rtp *conn_dst); const struct mgcp_rtp_codec *mgcp_codec_pt_find_by_subtype_name(struct mgcp_conn_rtp *conn, const char *subtype_name, unsigned int match_nr); bool mgcp_codec_amr_align_mode_is_indicated(const struct mgcp_rtp_codec *codec); diff --git a/include/osmocom/mgcp/mgcp_trunk.h b/include/osmocom/mgcp/mgcp_trunk.h index 33c3d5b..e608161 100644 --- a/include/osmocom/mgcp/mgcp_trunk.h +++ b/include/osmocom/mgcp/mgcp_trunk.h @@ -31,8 +31,6 @@ int audio_send_ptime; int audio_send_name; - int no_audio_transcoding; - int omit_rtcp; int keepalive_interval; diff --git a/src/libosmo-mgcp/mgcp_codec.c b/src/libosmo-mgcp/mgcp_codec.c index 7fada78..905795c 100644 --- a/src/libosmo-mgcp/mgcp_codec.c +++ b/src/libosmo-mgcp/mgcp_codec.c @@ -344,11 +344,30 @@ return true; } -/*! For a given codec, find a convertible codec in the given connection. - * \param[in] conn connection to search for a convertible codec - * \param[in] codec for which a convertible codec shall be found. - * \returns codec on success, -NULL on failure. */ -struct mgcp_rtp_codec *mgcp_codec_find_convertible(struct mgcp_conn_rtp *conn, struct mgcp_rtp_codec *codec) +struct mgcp_rtp_codec *mgcp_codec_find_same(struct mgcp_conn_rtp *conn, struct mgcp_rtp_codec *codec) +{ + struct mgcp_rtp_end *rtp_end; + unsigned int i; + unsigned int codecs_assigned; + + rtp_end = &conn->end; + + /* 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_end->codecs_assigned; + OSMO_ASSERT(codecs_assigned <= MGCP_MAX_CODECS); + for (i = 0; i < codecs_assigned; i++) { + if (codecs_same(codec, &rtp_end->codecs[i])) { + return &rtp_end->codecs[i]; + break; + } + } + + return NULL; +} + +/* For a given codec, find a convertible codec in the given connection. */ +static struct mgcp_rtp_codec *codec_find_convertible(struct mgcp_conn_rtp *conn, struct mgcp_rtp_codec *codec) { struct mgcp_rtp_end *rtp_end; unsigned int i; @@ -359,93 +378,72 @@ /* 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_end->codecs_assigned; - OSMO_ASSERT(codecs_assigned <= MGCP_MAX_CODECS); - for (i = 0; i < codecs_assigned; i++) { - if (codecs_same(codec, &rtp_end->codecs[i])) { - codec_convertible = &rtp_end->codecs[i]; - break; - } - } + codec_convertible = mgcp_codec_find_same(conn, codec); + if (codec_convertible) + return codec_convertible; /* 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_convertible) { - for (i = 0; i < codecs_assigned; i++) { - if (codecs_convertible(codec, &rtp_end->codecs[i])) { - codec_convertible = &rtp_end->codecs[i]; - break; - } + codecs_assigned = rtp_end->codecs_assigned; + OSMO_ASSERT(codecs_assigned <= MGCP_MAX_CODECS); + for (i = 0; i < codecs_assigned; i++) { + if (codecs_convertible(codec, &rtp_end->codecs[i])) { + codec_convertible = &rtp_end->codecs[i]; + break; } } return codec_convertible; } -/* Check if the given codec is applicable on the specified endpoint - * Helper function for mgcp_codec_decide() */ -static bool is_codec_compatible(const struct mgcp_endpoint *endp, const struct mgcp_rtp_codec *codec) -{ - /* A codec name must be set, if not, this might mean that the codec - * (payload type) that was assigned is unknown to us so we must stop - * here. */ - if (!strlen(codec->subtype_name)) - return false; - - /* FIXME: implement meaningful checks to make sure that the given codec - * is compatible with the given endpoint */ - - return true; -} - -/*! Decide for one suitable codec - * \param[in] conn related rtp-connection. +/*! Decide for one suitable codec on both of the given connections. In case a destination connection is not available, + * a tentative decision is made. + * \param[inout] conn_src related rtp-connection. + * \param[inout] conn_dst related destination rtp-connection (NULL if not present). * \returns 0 on success, -EINVAL on failure. */ -int mgcp_codec_decide(struct mgcp_conn_rtp *conn) +int mgcp_codec_decide(struct mgcp_conn_rtp *conn_src, struct mgcp_conn_rtp *conn_dst) { - struct mgcp_rtp_end *rtp; unsigned int i; - struct mgcp_endpoint *endp; - bool codec_assigned = false; - endp = conn->conn->endp; - rtp = &conn->end; - - /* This function works on the results the SDP/LCO parser has extracted - * from the MGCP message. The goal is to select a suitable codec for - * the given connection. When transcoding is available, the first codec - * from the codec list is taken without further checking. When - * transcoding is not available, then the choice must be made more - * carefully. Each codec in the list is checked until one is found that - * is rated compatible. The rating is done by the helper function - * is_codec_compatible(), which does the actual checking. */ - for (i = 0; i < rtp->codecs_assigned; i++) { - /* When no transcoding is available, avoid codecs that would - * require transcoding. */ - if (endp->trunk->no_audio_transcoding && !is_codec_compatible(endp, &rtp->codecs[i])) { - LOGP(DLMGCP, LOGL_NOTICE, "transcoding not available, skipping codec: %d/%s\n", - rtp->codecs[i].payload_type, rtp->codecs[i].subtype_name); - continue; - } - - rtp->codec = &rtp->codecs[i]; - codec_assigned = true; - break; + /* In case no destination connection is available (yet), or in case the destination connection exists but has + * no codecs assigned, we are forced to make a simple tentative decision: + * We just use the first codec of the source connection (conn_src) */ + OSMO_ASSERT(conn_src->end.codecs_assigned <= MGCP_MAX_CODECS); + if (!conn_dst || conn_dst->end.codecs_assigned == 0) { + if (conn_src->end.codecs_assigned >= 1) { + conn_src->end.codec = &conn_src->end.codecs[0]; + return 0; + } else + return -EINVAL; } - /* FIXME: To the reviewes: This is problematic. I do not get why we - * need to reset the packet_duration_ms depending on the codec - * selection. I thought it were all 20ms? Is this to address some - * cornercase. (This piece of code was in the code path before, - * together with the note: "TODO/XXX: Store this per codec and derive - * it on use" */ - if (codec_assigned) { - if (rtp->maximum_packet_time >= 0 - && rtp->maximum_packet_time * rtp->codec->frame_duration_den > - rtp->codec->frame_duration_num * 1500) - rtp->packet_duration_ms = 0; + /* Compare all codecs of the source connection (conn_src) to the codecs of the destination connection (conn_dst). In case + * of a match set this codec on both connections. This would be an ideal selection since no codec conversion would be + * required. */ + for (i = 0; i < conn_src->end.codecs_assigned; i++) { + struct mgcp_rtp_codec *codec_conn_dst = mgcp_codec_find_same(conn_dst, &conn_src->end.codecs[i]); + if (codec_conn_dst) { + /* We found the a codec that is exactly the same (same codec, same payload format etc.) on both + * sides. We now set this codec on both connections. */ + conn_dst->end.codec = codec_conn_dst; + conn_src->end.codec = mgcp_codec_find_same(conn_src, codec_conn_dst); + OSMO_ASSERT(conn_src->end.codec); + return 0; + } + } - return 0; + /* In case we could not find a codec that is exactly the same, let's at least try to find a codec that we are able + * to convert. */ + for (i = 0; i < conn_src->end.codecs_assigned; i++) { + struct mgcp_rtp_codec *codec_conn_dst = codec_find_convertible(conn_dst, &conn_src->end.codecs[i]); + if (codec_conn_dst) { + /* We found the a codec that we are able to convert on both sides. We now set this codec on both + * connections. */ + conn_dst->end.codec = codec_conn_dst; + conn_src->end.codec = codec_find_convertible(conn_src, codec_conn_dst); + OSMO_ASSERT(conn_src->end.codec); + return 0; + } } return -EINVAL; diff --git a/src/libosmo-mgcp/mgcp_network.c b/src/libosmo-mgcp/mgcp_network.c index ea1a699..6b16907 100644 --- a/src/libosmo-mgcp/mgcp_network.c +++ b/src/libosmo-mgcp/mgcp_network.c @@ -492,32 +492,24 @@ /* There may be different payload type numbers negotiated for two connections. * Patch the payload type of an RTP packet so that it uses the payload type - * that is valid for the destination connection (conn_dst) */ -static int mgcp_patch_pt(struct mgcp_conn_rtp *conn_src, struct mgcp_conn_rtp *conn_dst, struct msgb *msg) + * of the codec that is set for the destination connection (conn_dst) */ +static int mgcp_patch_pt(struct mgcp_conn_rtp *conn_dst, struct msgb *msg) { struct rtp_hdr *rtp_hdr; - struct mgcp_rtp_codec *codec_src; - struct mgcp_rtp_codec *codec_dst; if (msgb_length(msg) < sizeof(struct rtp_hdr)) { - LOG_CONN_RTP(conn_src, LOGL_ERROR, "RTP packet too short (%u < %zu)\n", + LOG_CONN_RTP(conn_dst, LOGL_NOTICE, "RTP packet too short (%u < %zu)\n", msgb_length(msg), sizeof(struct rtp_hdr)); return -EINVAL; } rtp_hdr = (struct rtp_hdr *)msgb_data(msg); - /* Find the codec information that is used on the source side */ - codec_src = mgcp_codec_from_pt(conn_src, rtp_hdr->payload_type); - if (!codec_src) + if (!conn_dst->end.codec) { + LOG_CONN_RTP(conn_dst, LOGL_NOTICE, "no codec set on destination connection!\n"); return -EINVAL; + } + rtp_hdr->payload_type = (uint8_t) conn_dst->end.codec->payload_type; - /* Lookup a suitable codec in the destination connection. (The codec must be of the same type or at least - * convertible) */ - codec_dst = mgcp_codec_find_convertible(conn_dst, codec_src); - if (!codec_dst) - return -EINVAL; - - rtp_hdr->payload_type = (uint8_t) codec_dst->payload_type; return 0; } @@ -1172,7 +1164,7 @@ * IuUP -> AMR: calls this function, skip patching if conn_src is IuUP. * {AMR or IuUP} -> IuUP: calls mgcp_udp_send() directly, skipping this function: No need to examine dst. */ if (is_rtp && !mgcp_conn_rtp_is_iuup(conn_src)) { - rc = mgcp_patch_pt(conn_src, conn_dst, msg); + rc = mgcp_patch_pt(conn_dst, msg); if (rc < 0) { /* FIXME: It is legal that the payload type on the egress connection is * different from the payload type that has been negotiated on the diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c index b697a94..1fa345b 100644 --- a/src/libosmo-mgcp/mgcp_protocol.c +++ b/src/libosmo-mgcp/mgcp_protocol.c @@ -761,6 +761,9 @@ struct mgcp_request_data *rq, int have_sdp, bool crcx) { struct mgcp_endpoint *endp = rq->endp; + struct mgcp_conn *conn_dst; + struct mgcp_conn_rtp *conn_dst_rtp; + int rc; char *cmd; @@ -802,8 +805,15 @@ goto error; } + /* Try to find an destination RTP connection that we can include in the codec decision. */ + conn_dst = mgcp_find_dst_conn(conn->conn); + if (conn_dst && conn_dst->type == MGCP_CONN_TYPE_RTP) + conn_dst_rtp = &conn_dst->u.rtp; + else + conn_dst_rtp = NULL; + /* Make codec decision */ - if (mgcp_codec_decide(conn) != 0) + if (mgcp_codec_decide(conn, conn_dst_rtp) != 0) goto error; return 0; diff --git a/src/libosmo-mgcp/mgcp_vty.c b/src/libosmo-mgcp/mgcp_vty.c index 5c19c14..784894c 100644 --- a/src/libosmo-mgcp/mgcp_vty.c +++ b/src/libosmo-mgcp/mgcp_vty.c @@ -120,8 +120,6 @@ trunk->audio_send_name ? "" : "no ", VTY_NEWLINE); vty_out(vty, " number endpoints %u%s", trunk->v.vty_number_endpoints, VTY_NEWLINE); - vty_out(vty, " %sallow-transcoding%s", - trunk->no_audio_transcoding ? "no " : "", VTY_NEWLINE); if (strlen(g_cfg->call_agent_addr)) vty_out(vty, " call-agent ip %s%s", g_cfg->call_agent_addr, VTY_NEWLINE); @@ -701,25 +699,19 @@ return CMD_SUCCESS; } -DEFUN_USRATTR(cfg_mgcp_allow_transcoding, - cfg_mgcp_allow_transcoding_cmd, - X(MGW_CMD_ATTR_NEWCONN), - "allow-transcoding", "Allow transcoding\n") +DEFUN_DEPRECATED(cfg_mgcp_allow_transcoding, + cfg_mgcp_allow_transcoding_cmd, + "allow-transcoding", "Allow transcoding\n") { - struct mgcp_trunk *trunk = mgcp_trunk_by_num(g_cfg, MGCP_TRUNK_VIRTUAL, MGCP_VIRT_TRUNK_ID); - OSMO_ASSERT(trunk); - trunk->no_audio_transcoding = 0; + vty_out(vty, "%% Deprecated 'allow-transcoding' config no longer has any effect%s", VTY_NEWLINE); return CMD_SUCCESS; } -DEFUN_USRATTR(cfg_mgcp_no_allow_transcoding, - cfg_mgcp_no_allow_transcoding_cmd, - X(MGW_CMD_ATTR_NEWCONN), - "no allow-transcoding", NO_STR "Allow transcoding\n") +DEFUN_DEPRECATED(cfg_mgcp_no_allow_transcoding, + cfg_mgcp_no_allow_transcoding_cmd, + "no allow-transcoding", NO_STR "Allow transcoding\n") { - struct mgcp_trunk *trunk = mgcp_trunk_by_num(g_cfg, MGCP_TRUNK_VIRTUAL, MGCP_VIRT_TRUNK_ID); - OSMO_ASSERT(trunk); - trunk->no_audio_transcoding = 1; + vty_out(vty, "%% Deprecated 'no allow-transcoding' config no longer has any effect%s", VTY_NEWLINE); return CMD_SUCCESS; } @@ -1073,8 +1065,6 @@ if (trunk->audio_fmtp_extra) vty_out(vty, " sdp audio fmtp-extra %s%s", trunk->audio_fmtp_extra, VTY_NEWLINE); - vty_out(vty, " %sallow-transcoding%s", - trunk->no_audio_transcoding ? "no " : "", VTY_NEWLINE); } return CMD_SUCCESS; @@ -1326,23 +1316,19 @@ return CMD_SUCCESS; } -DEFUN_USRATTR(cfg_trunk_allow_transcoding, - cfg_trunk_allow_transcoding_cmd, - X(MGW_CMD_ATTR_NEWCONN), - "allow-transcoding", "Allow transcoding\n") +DEFUN_DEPRECATED(cfg_trunk_allow_transcoding, + cfg_trunk_allow_transcoding_cmd, + "allow-transcoding", "Allow transcoding\n") { - struct mgcp_trunk *trunk = vty->index; - trunk->no_audio_transcoding = 0; + vty_out(vty, "%% Deprecated 'allow-transcoding' config no longer has any effect%s", VTY_NEWLINE); return CMD_SUCCESS; } -DEFUN_USRATTR(cfg_trunk_no_allow_transcoding, - cfg_trunk_no_allow_transcoding_cmd, - X(MGW_CMD_ATTR_NEWCONN), - "no allow-transcoding", NO_STR "Allow transcoding\n") +DEFUN_DEPRECATED(cfg_trunk_no_allow_transcoding, + cfg_trunk_no_allow_transcoding_cmd, + "no allow-transcoding", NO_STR "Allow transcoding\n") { - struct mgcp_trunk *trunk = vty->index; - trunk->no_audio_transcoding = 1; + vty_out(vty, "%% Deprecated 'no allow-transcoding' config no longer has any effect%s", VTY_NEWLINE); return CMD_SUCCESS; } diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c index bc2dae0..2374e91 100644 --- a/tests/mgcp/mgcp_test.c +++ b/tests/mgcp/mgcp_test.c @@ -1467,7 +1467,6 @@ /* Allocate 5@mgw and let osmo-mgw pick a codec from the list */ last_endpoint[0] = '\0'; inp = create_msg(CRCX_MULT_GSM_EXACT, NULL); - trunk->no_audio_transcoding = 1; resp = mgcp_handle_message(cfg, inp); OSMO_ASSERT(get_conn_id_from_response(resp->data, conn_id, sizeof(conn_id)) == 0); @@ -1511,7 +1510,6 @@ last_endpoint[0] = '\0'; inp = create_msg(CRCX_MULT_GSM_EXACT, NULL); - trunk->no_audio_transcoding = 0; resp = mgcp_handle_message(cfg, inp); OSMO_ASSERT(get_conn_id_from_response(resp->data, conn_id, sizeof(conn_id)) == 0); @@ -1769,26 +1767,25 @@ .amr_octet_aligned_present = false, }; -struct testcase_mgcp_codec_find_convertible_codec { +struct testcase_mgcp_codec_decide_codec { int payload_type; const char *audio_name; const struct mgcp_codec_param *param; int expect_rc; }; -struct testcase_mgcp_codec_find_convertible_expect { - bool end; +struct testcase_mgcp_codec_decide_expect { int payload_type_map[2]; }; -struct testcase_mgcp_codec_find_convertible { +struct testcase_mgcp_codec_decide { const char *descr; /* two conns on an endpoint, each with N configured codecs */ - struct testcase_mgcp_codec_find_convertible_codec codecs[2][10]; - struct testcase_mgcp_codec_find_convertible_expect expect[32]; + struct testcase_mgcp_codec_decide_codec codecs[2][10]; + struct testcase_mgcp_codec_decide_expect expect[2]; }; -static const struct testcase_mgcp_codec_find_convertible test_mgcp_codec_find_convertible_cases[] = { +static const struct testcase_mgcp_codec_decide test_mgcp_codec_find_convertible_cases[] = { { .descr = "same order, but differing payload type numbers", .codecs = { @@ -1805,10 +1802,7 @@ }, .expect = { { .payload_type_map = {112, 96}, }, - { .payload_type_map = {0, 0}, }, - { .payload_type_map = {111, 97} }, - { .payload_type_map = {123, -EINVAL} }, - { .end = true }, + { .payload_type_map = {112, 96}, }, }, }, { @@ -1826,11 +1820,8 @@ }, }, .expect = { - { .payload_type_map = {112, 96}, }, { .payload_type_map = {0, 0}, }, - { .payload_type_map = {111, 97} }, - { .payload_type_map = {123, -EINVAL} }, - { .end = true }, + { .payload_type_map = {111, 97}, }, }, }, { @@ -1848,11 +1839,8 @@ }, }, .expect = { - { .payload_type_map = {96, 97}, }, - { .payload_type_map = {97, 96}, }, { .payload_type_map = {0, 0}, }, - { .payload_type_map = {123, -EINVAL} }, - { .end = true }, + { .payload_type_map = {96, 97}, }, }, }, { @@ -1868,10 +1856,8 @@ }, }, .expect = { - { .payload_type_map = {112, -EINVAL}, }, - { .payload_type_map = {0, -EINVAL}, }, - { .payload_type_map = {111, -EINVAL} }, - { .end = true }, + { .payload_type_map = {-EINVAL, -EINVAL}, }, + { .payload_type_map = {-EINVAL, 96}, }, }, }, { @@ -1888,13 +1874,11 @@ }, .expect = { { .payload_type_map = {112, -EINVAL}, }, - { .payload_type_map = {0, -EINVAL}, }, - { .payload_type_map = {111, -EINVAL} }, - { .end = true }, + { .payload_type_map = {-EINVAL, -EINVAL}, }, }, }, { - .descr = "test AMR with differing octet-aligned settings", + .descr = "test AMR with differing octet-aligned settings (both <-> both)", .codecs = { { { 111, "AMR/8000", &amr_param_octet_aligned_true, }, @@ -1908,8 +1892,38 @@ .expect = { { .payload_type_map = {111, 121}, }, { .payload_type_map = {112, 122} }, - { .payload_type_map = {123, -EINVAL} }, - { .end = true }, + }, + }, + { + .descr = "test AMR with differing octet-aligned settings (oa <-> both)", + .codecs = { + { + { 111, "AMR/8000", &amr_param_octet_aligned_true, }, + }, + { + { 122, "AMR/8000", &amr_param_octet_aligned_false, }, + { 121, "AMR/8000", &amr_param_octet_aligned_true, }, + }, + }, + .expect = { + { .payload_type_map = {111, 121}, }, + { .payload_type_map = {111, 121}, }, + }, + }, + { + .descr = "test AMR with differing octet-aligned settings (bwe <-> both)", + .codecs = { + { + { 112, "AMR/8000", &amr_param_octet_aligned_false, }, + }, + { + { 122, "AMR/8000", &amr_param_octet_aligned_false, }, + { 121, "AMR/8000", &amr_param_octet_aligned_true, }, + }, + }, + .expect = { + { .payload_type_map = {112, 122}, }, + { .payload_type_map = {112, 122}, }, }, }, { @@ -1924,8 +1938,7 @@ }, .expect = { { .payload_type_map = {111, 122}, }, - { .payload_type_map = {55, -EINVAL}, }, - { .end = true }, + { .payload_type_map = {111, 122}, }, }, }, { @@ -1940,8 +1953,7 @@ }, .expect = { { .payload_type_map = {111, 122}, }, - { .payload_type_map = {55, -EINVAL}, }, - { .end = true }, + { .payload_type_map = {111, 122}, }, }, }, { @@ -1957,8 +1969,7 @@ .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 }, + { .payload_type_map = {112, 122}, }, }, }, { @@ -1974,8 +1985,7 @@ .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 }, + { .payload_type_map = {112, 122}, }, }, }, { @@ -1993,11 +2003,8 @@ }, }, .expect = { - { .payload_type_map = {112, 96}, }, { .payload_type_map = {0, 0}, }, - { .payload_type_map = {111, 97} }, - { .payload_type_map = {123, -EINVAL} }, - { .end = true }, + { .payload_type_map = {111, 97}, }, }, }, { @@ -2015,11 +2022,8 @@ }, }, .expect = { - { .payload_type_map = {112, 96}, }, { .payload_type_map = {0, 0}, }, - { .payload_type_map = {111, 97} }, - { .payload_type_map = {123, -EINVAL} }, - { .end = true }, + { .payload_type_map = {111, 97}, }, }, }, { @@ -2037,70 +2041,97 @@ }, .expect = { { .payload_type_map = {111, 121}, }, - { .payload_type_map = {112, -EINVAL} }, - { .payload_type_map = {113, -EINVAL} }, - { .end = true }, + { .payload_type_map = {111, 121} }, }, }, }; -static int pt_translate(struct mgcp_conn_rtp *conn, unsigned int index_src, unsigned int index_dst, int payload_type) +static bool codec_decision(struct mgcp_conn_rtp *conn, unsigned int index_conn_src, unsigned int index_conn_dst, + const struct testcase_mgcp_codec_decide_expect *expect) { - struct mgcp_rtp_codec *codec_src = NULL; - struct mgcp_rtp_codec *codec_dst = NULL; - struct mgcp_conn_rtp *conn_src = &conn[index_src]; - struct mgcp_conn_rtp *conn_dst = &conn[index_dst]; + bool ok = true; + int payload_type_conn_src; + int payload_type_conn_dst; - /* Find the codec information that is used on the source side */ - codec_src = mgcp_codec_from_pt(conn_src, payload_type); - if (!codec_src) { - printf(" - mgcp_codec_from_pt(conn%u, %d) -> NO RESULT\n", index_src, payload_type); - return -EINVAL; - } - printf(" - mgcp_codec_from_pt(conn%u, %d) -> %s\n", index_src, payload_type, codec_src->subtype_name); + printf(" - mgcp_codec_decide(&conn[%u], &conn[%u]):\n", index_conn_src, index_conn_dst); + if (mgcp_codec_decide(&conn[index_conn_src], &conn[index_conn_dst]) != 0) { + if (expect->payload_type_map[index_conn_src] == -EINVAL + && expect->payload_type_map[index_conn_dst] == -EINVAL) + printf(" codec decision failed (expected)!\n"); + else { + printf(" ERROR: codec decision failed!\n"); + ok = false; + } + } else { + printf(" Codec decision result:\n"); + if (conn[index_conn_src].end.codec) { + payload_type_conn_src = conn[index_conn_src].end.codec->payload_type; + printf(" conn[%u]: codec:%s, pt:%d\n", + index_conn_src, conn[index_conn_src].end.codec->subtype_name, payload_type_conn_src); + } else { + payload_type_conn_src = -EINVAL; + printf(" conn[%u]: codec:none, pt:none\n", index_conn_src); + } - codec_dst = mgcp_codec_find_convertible(conn_dst, codec_src); - if (!codec_dst) { - printf(" - mgcp_codec_find_convertible(conn%u, %s) -> NO RESULT\n", index_dst, codec_src->subtype_name); - return -EINVAL; + if (conn[index_conn_dst].end.codec) { + payload_type_conn_dst = conn[index_conn_dst].end.codec->payload_type; + printf(" conn[%u]: codec:%s, pt:%d\n", + index_conn_dst, conn[index_conn_dst].end.codec->subtype_name, + payload_type_conn_dst); + } else { + payload_type_conn_dst = -EINVAL; + printf(" conn[%u]: codec:none, pt:none\n", index_conn_dst); + } + + if (payload_type_conn_src != expect->payload_type_map[index_conn_src]) { + printf(" ERROR: conn[%u] unexpected codec decision, expected pt=%d, got pt=%d\n", + index_conn_src, expect->payload_type_map[index_conn_src], payload_type_conn_src); + ok = false; + } + + if (payload_type_conn_dst != expect->payload_type_map[index_conn_dst]) { + printf(" ERROR: conn[%u] unexpected codec decision, expected pt=%d, got pt=%d\n", + index_conn_dst, expect->payload_type_map[index_conn_dst], + payload_type_conn_dst); + ok = false; + } } - printf(" - mgcp_codec_find_convertible(conn%u, %s) -> %s -> %u\n", - index_dst, codec_src->subtype_name, codec_dst->subtype_name, codec_dst->payload_type); - return codec_dst->payload_type; + + return ok; } -static void test_mgcp_codec_find_convertible(void) +static void test_mgcp_codec_decide(void) { int i; bool ok = true; printf("\nTesting mgcp_codec_find_convertible()\n"); for (i = 0; i < ARRAY_SIZE(test_mgcp_codec_find_convertible_cases); i++) { - const struct testcase_mgcp_codec_find_convertible *t = &test_mgcp_codec_find_convertible_cases[i]; - struct mgcp_conn_rtp conn[2] = {}; + const struct testcase_mgcp_codec_decide *t = &test_mgcp_codec_find_convertible_cases[i]; + struct mgcp_conn_rtp conn[2] = { }; int rc; int conn_i; int c; printf("#%d: %s\n", i, t->descr); + /* Build testvector (add codecs to conn, set properties etc... */ for (conn_i = 0; conn_i < 2; conn_i++) { printf(" - add codecs on conn%d:\n", conn_i); for (c = 0; c < ARRAY_SIZE(t->codecs[conn_i]); c++) { - const struct testcase_mgcp_codec_find_convertible_codec *codec = &t->codecs[conn_i][c]; + const struct testcase_mgcp_codec_decide_codec *codec = &t->codecs[conn_i][c]; if (!codec->audio_name) break; - rc = mgcp_codec_add(&conn[conn_i], codec->payload_type, codec->audio_name, codec->param); + rc = mgcp_codec_add(&conn[conn_i], codec->payload_type, codec->audio_name, + codec->param); printf(" %2d: %3d %s%s -> rc=%d\n", c, codec->payload_type, codec->audio_name, codec->param ? - (codec->param->amr_octet_aligned_present? - (codec->param->amr_octet_aligned ? - " octet-aligned=1" : " octet-aligned=0") - : " octet-aligned=unset") - : "", - rc); + (codec->param->amr_octet_aligned_present ? + (codec->param->amr_octet_aligned ? " octet-aligned=1" : " octet-aligned=0") + : " octet-aligned=unset") + : "", rc); if (rc != codec->expect_rc) { printf(" ERROR: expected rc=%d\n", codec->expect_rc); ok = false; @@ -2110,32 +2141,17 @@ printf(" (none)\n"); } - for (c = 0; c < ARRAY_SIZE(t->expect); c++) { - const struct testcase_mgcp_codec_find_convertible_expect *expect = &t->expect[c]; - int result; + /* Run codec decision and check expectation */ + if (!codec_decision(conn, 0, 1, &t->expect[0])) + ok = false; - if (expect->end) - break; + if (!codec_decision(conn, 1, 0, &t->expect[1])) + ok = false; - result = pt_translate(conn, 0, 1, expect->payload_type_map[0]); - if (result != expect->payload_type_map[1]) { - printf(" ERROR: expected -> %d\n", expect->payload_type_map[1]); - ok = false; - } - - /* If the expected result is an error, don't do reverse map test */ - if (expect->payload_type_map[1] < 0) - continue; - - result = pt_translate(conn, 1, 0, expect->payload_type_map[1]); - if (result != expect->payload_type_map[0]) { - printf(" ERROR: expected -> %d\n", expect->payload_type_map[0]); - ok = false; - } - } - - for (conn_i = 0; conn_i < 2; conn_i++) - mgcp_codec_reset_all(&conn[conn_i]); + if (ok) + printf(" ===> SUCCESS: codec decision as expected!\n"); + else + printf(" ===> FAIL: unexpected codec decision!\n"); } OSMO_ASSERT(ok); @@ -2295,7 +2311,7 @@ test_osmux_cid(); test_get_lco_identifier(); test_check_local_cx_options(ctx); - test_mgcp_codec_find_convertible(); + test_mgcp_codec_decide(); test_conn_id_matching(); test_e1_trunk_nr_from_epname(); test_mgcp_is_rtp_dummy_payload(); diff --git a/tests/mgcp/mgcp_test.ok b/tests/mgcp/mgcp_test.ok index 4ea9a34..08df5d4 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/+/32218 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I6c3291f825488e5d8ce136aeb18450156794aeb5 Gerrit-Change-Number: 32218 Gerrit-PatchSet: 13 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 1 month

1
0
0 0

[M] Change in osmo-mgw[master]: mgcp_codec: move mgcp_codec_decide down

by dexter

dexter has submitted this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/32506 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: mgcp_codec: move mgcp_codec_decide down ...................................................................... mgcp_codec: move mgcp_codec_decide down In a follow up patch we intend to fix mgcp_codec_decide, since the fixed version of mgcp_codec_decide will use some functions below its current position let's move it down before fixing it to make reviewing the changes easier. Related: OS#5461 Change-Id: I2f2538ff912eae4d80d3b74b766e18c4da94d6b6 --- M src/libosmo-mgcp/mgcp_codec.c 1 file changed, 96 insertions(+), 81 deletions(-) Approvals: pespin: Looks good to me, but someone else must approve msuraev: Looks good to me, but someone else must approve laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/libosmo-mgcp/mgcp_codec.c b/src/libosmo-mgcp/mgcp_codec.c index 003c4c4..7fada78 100644 --- a/src/libosmo-mgcp/mgcp_codec.c +++ b/src/libosmo-mgcp/mgcp_codec.c @@ -276,87 +276,6 @@ return -EINVAL; } -/* Check if the given codec is applicable on the specified endpoint - * Helper function for mgcp_codec_decide() */ -static bool is_codec_compatible(const struct mgcp_endpoint *endp, const struct mgcp_rtp_codec *codec) -{ - /* A codec name must be set, if not, this might mean that the codec - * (payload type) that was assigned is unknown to us so we must stop - * here. */ - if (!strlen(codec->subtype_name)) - return false; - - /* FIXME: implement meaningful checks to make sure that the given codec - * is compatible with the given endpoint */ - - return true; -} - -/*! Decide for one suitable codec - * \param[in] conn related rtp-connection. - * \returns 0 on success, -EINVAL on failure. */ -int mgcp_codec_decide(struct mgcp_conn_rtp *conn) -{ - struct mgcp_rtp_end *rtp; - unsigned int i; - struct mgcp_endpoint *endp; - bool codec_assigned = false; - - endp = conn->conn->endp; - rtp = &conn->end; - - /* This function works on the results the SDP/LCO parser has extracted - * from the MGCP message. The goal is to select a suitable codec for - * the given connection. When transcoding is available, the first codec - * from the codec list is taken without further checking. When - * transcoding is not available, then the choice must be made more - * carefully. Each codec in the list is checked until one is found that - * is rated compatible. The rating is done by the helper function - * is_codec_compatible(), which does the actual checking. */ - for (i = 0; i < rtp->codecs_assigned; i++) { - /* When no transcoding is available, avoid codecs that would - * require transcoding. */ - if (endp->trunk->no_audio_transcoding && !is_codec_compatible(endp, &rtp->codecs[i])) { - LOGP(DLMGCP, LOGL_NOTICE, "transcoding not available, skipping codec: %d/%s\n", - rtp->codecs[i].payload_type, rtp->codecs[i].subtype_name); - continue; - } - - rtp->codec = &rtp->codecs[i]; - codec_assigned = true; - break; - } - - /* FIXME: To the reviewes: This is problematic. I do not get why we - * need to reset the packet_duration_ms depending on the codec - * selection. I thought it were all 20ms? Is this to address some - * cornercase. (This piece of code was in the code path before, - * together with the note: "TODO/XXX: Store this per codec and derive - * it on use" */ - if (codec_assigned) { - if (rtp->maximum_packet_time >= 0 - && rtp->maximum_packet_time * rtp->codec->frame_duration_den > - rtp->codec->frame_duration_num * 1500) - rtp->packet_duration_ms = 0; - - return 0; - } - - return -EINVAL; -} - -/* Check if the codec has a specific AMR mode (octet-aligned or bandwith-efficient) set. */ -bool mgcp_codec_amr_align_mode_is_indicated(const struct mgcp_rtp_codec *codec) -{ - if (codec->param_present == false) - return false; - if (!codec->param.amr_octet_aligned_present) - return false; - if (strcmp(codec->subtype_name, "AMR") != 0) - return false; - return true; -} - /* Return true if octet-aligned is set in the given codec. Default to octet-aligned=0, i.e. bandwidth-efficient mode. * See RFC4867 "RTP Payload Format for AMR and AMR-WB" sections "8.1. AMR Media Type Registration" and "8.2. AMR-WB * Media Type Registration": @@ -463,6 +382,87 @@ return codec_convertible; } +/* Check if the given codec is applicable on the specified endpoint + * Helper function for mgcp_codec_decide() */ +static bool is_codec_compatible(const struct mgcp_endpoint *endp, const struct mgcp_rtp_codec *codec) +{ + /* A codec name must be set, if not, this might mean that the codec + * (payload type) that was assigned is unknown to us so we must stop + * here. */ + if (!strlen(codec->subtype_name)) + return false; + + /* FIXME: implement meaningful checks to make sure that the given codec + * is compatible with the given endpoint */ + + return true; +} + +/*! Decide for one suitable codec + * \param[in] conn related rtp-connection. + * \returns 0 on success, -EINVAL on failure. */ +int mgcp_codec_decide(struct mgcp_conn_rtp *conn) +{ + struct mgcp_rtp_end *rtp; + unsigned int i; + struct mgcp_endpoint *endp; + bool codec_assigned = false; + + endp = conn->conn->endp; + rtp = &conn->end; + + /* This function works on the results the SDP/LCO parser has extracted + * from the MGCP message. The goal is to select a suitable codec for + * the given connection. When transcoding is available, the first codec + * from the codec list is taken without further checking. When + * transcoding is not available, then the choice must be made more + * carefully. Each codec in the list is checked until one is found that + * is rated compatible. The rating is done by the helper function + * is_codec_compatible(), which does the actual checking. */ + for (i = 0; i < rtp->codecs_assigned; i++) { + /* When no transcoding is available, avoid codecs that would + * require transcoding. */ + if (endp->trunk->no_audio_transcoding && !is_codec_compatible(endp, &rtp->codecs[i])) { + LOGP(DLMGCP, LOGL_NOTICE, "transcoding not available, skipping codec: %d/%s\n", + rtp->codecs[i].payload_type, rtp->codecs[i].subtype_name); + continue; + } + + rtp->codec = &rtp->codecs[i]; + codec_assigned = true; + break; + } + + /* FIXME: To the reviewes: This is problematic. I do not get why we + * need to reset the packet_duration_ms depending on the codec + * selection. I thought it were all 20ms? Is this to address some + * cornercase. (This piece of code was in the code path before, + * together with the note: "TODO/XXX: Store this per codec and derive + * it on use" */ + if (codec_assigned) { + if (rtp->maximum_packet_time >= 0 + && rtp->maximum_packet_time * rtp->codec->frame_duration_den > + rtp->codec->frame_duration_num * 1500) + rtp->packet_duration_ms = 0; + + return 0; + } + + return -EINVAL; +} + +/* Check if the codec has a specific AMR mode (octet-aligned or bandwith-efficient) set. */ +bool mgcp_codec_amr_align_mode_is_indicated(const struct mgcp_rtp_codec *codec) +{ + if (codec->param_present == false) + return false; + if (!codec->param.amr_octet_aligned_present) + return false; + if (strcmp(codec->subtype_name, "AMR") != 0) + return false; + return true; +} + /* Find the payload type number configured for a specific codec by SDP. * For example, IuUP gets assigned a payload type number, and the endpoint needs to translate that to the number * assigned to "AMR" on the other conn (by a=rtpmap:N). -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/32506 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I2f2538ff912eae4d80d3b74b766e18c4da94d6b6 Gerrit-Change-Number: 32506 Gerrit-PatchSet: 3 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

2 years, 1 month

1
0
0 0

[S] Change in osmocom-bb[master]: trxcon/l1sched: drop Tx prims for not supported TCH modes

by fixeria

fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmocom-bb/+/32917 ) Change subject: trxcon/l1sched: drop Tx prims for not supported TCH modes ...................................................................... trxcon/l1sched: drop Tx prims for not supported TCH modes Otherwise we'll be attempting to transmit the same prim again and again. Change-Id: I2f137a0b931e8e7d05a5d903023c50065f57bfa6 Related: OS#4396 --- M src/host/trxcon/src/sched_lchan_tchf.c M src/host/trxcon/src/sched_lchan_tchh.c 2 files changed, 20 insertions(+), 4 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/17/32917/1 diff --git a/src/host/trxcon/src/sched_lchan_tchf.c b/src/host/trxcon/src/sched_lchan_tchf.c index fca2108..1a00a93 100644 --- a/src/host/trxcon/src/sched_lchan_tchf.c +++ b/src/host/trxcon/src/sched_lchan_tchf.c @@ -303,8 +303,10 @@ break; } default: - LOGP_LCHAND(lchan, LOGL_ERROR, "Invalid TCH mode: %u\n", lchan->tch_mode); - return -EINVAL; + LOGP_LCHAND(lchan, LOGL_ERROR, + "TCH mode %s is unknown or not supported\n", + gsm48_chan_mode_name(lchan->tch_mode)); + goto free_bad_msg; } if (rc) { diff --git a/src/host/trxcon/src/sched_lchan_tchh.c b/src/host/trxcon/src/sched_lchan_tchh.c index e3e4cde..303d59e 100644 --- a/src/host/trxcon/src/sched_lchan_tchh.c +++ b/src/host/trxcon/src/sched_lchan_tchh.c @@ -523,8 +523,10 @@ break; } default: - LOGP_LCHAND(lchan, LOGL_ERROR, "Invalid TCH mode: %u\n", lchan->tch_mode); - return -EINVAL; + LOGP_LCHAND(lchan, LOGL_ERROR, + "TCH mode %s is unknown or not supported\n", + gsm48_chan_mode_name(lchan->tch_mode)); + goto free_bad_msg; } if (rc) { -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/32917 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: I2f137a0b931e8e7d05a5d903023c50065f57bfa6 Gerrit-Change-Number: 32917 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: newchange

2 years, 1 month

1
0
0 0

2025

2024

2023

2022

gerrit-log May 2023