neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-mgw/+/34899?usp=email ) Change subject: eliminate duality of codecs[] and ptmap[] ...................................................................... eliminate duality of codecs[] and ptmap[] codecs[] is an array of enum osmo_mgcp_codecs. ptmap[] is an array of { enum osmo_mgcp_codecs, unsigned int ptmap }. MGCP lists first a bunch of payload type numbers and then specifies them again for details, like the numbers 112, 96, 3 in this example: m=audio <port> RTP/AVP 112 96 3 a=rtpmap:112 AMR/8000 a=rtpmap:96 VND.3GPP.IUFP/16000 a=rtpmap:3 GSM-FR/8000 So far we keep these lists in two separate arrays, in both struct mgcp_response and struct mgcp_msg: - codecs[], codecs_len stores the 'm=audio' list - ptmap[], ptmap_len stores the 'a=rtpmap' list These are semantically identical, and keeping two instances leads to numerous checks, conversions and dear hopes of correct ordering. So let's keep only one list with both codec and payload type number in it. The 'm=audio' list establishes the order of the pt numbers, and the 'a=rtpmap' list adds codec information to the established entries. Change-Id: I798e02c6663376d3d52f4a74fc4b32411ce95bed --- M include/osmocom/mgcp_client/mgcp_client.h M include/osmocom/mgcp_client/mgcp_client_fsm.h M src/libosmo-mgcp-client/mgcp_client.c M src/libosmo-mgcp-client/mgcp_client_fsm.c M tests/mgcp_client/mgcp_client_test.c 5 files changed, 107 insertions(+), 63 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/99/34899/1 diff --git a/include/osmocom/mgcp_client/mgcp_client.h b/include/osmocom/mgcp_client/mgcp_client.h index 9a0611a..31f4ae7 100644 --- a/include/osmocom/mgcp_client/mgcp_client.h +++ b/include/osmocom/mgcp_client/mgcp_client.h @@ -95,8 +95,6 @@ uint16_t audio_port; char audio_ip[INET6_ADDRSTRLEN]; unsigned int ptime; - enum mgcp_codecs codecs[MGCP_MAX_CODECS]; - unsigned int codecs_len; struct ptmap ptmap[MGCP_MAX_CODECS]; unsigned int ptmap_len; }; @@ -129,8 +127,6 @@ char *audio_ip; enum mgcp_connection_mode conn_mode; unsigned int ptime; - enum mgcp_codecs codecs[MGCP_MAX_CODECS]; - unsigned int codecs_len; struct ptmap ptmap[MGCP_MAX_CODECS]; unsigned int ptmap_len; uint32_t x_osmo_ign; diff --git a/include/osmocom/mgcp_client/mgcp_client_fsm.h b/include/osmocom/mgcp_client/mgcp_client_fsm.h index ade4f49..25f0732 100644 --- a/include/osmocom/mgcp_client/mgcp_client_fsm.h +++ b/include/osmocom/mgcp_client/mgcp_client_fsm.h @@ -29,12 +29,6 @@ /*! RTP packetization interval (optional) */ unsigned int ptime; - /*! RTP codec list (optional) */ - enum mgcp_codecs codecs[MGCP_MAX_CODECS]; - - /*! Number of codecs in RTP codec list (optional) */ - unsigned int codecs_len; - /*! RTP payload type map (optional, only needed when payload types are * used that differ from what IANA/3GPP defines) */ struct ptmap ptmap[MGCP_MAX_CODECS]; diff --git a/src/libosmo-mgcp-client/mgcp_client.c b/src/libosmo-mgcp-client/mgcp_client.c index 95c7a4b..a66b499 100644 --- a/src/libosmo-mgcp-client/mgcp_client.c +++ b/src/libosmo-mgcp-client/mgcp_client.c @@ -302,7 +302,7 @@ char *pt_str; char *pt_end; unsigned long int pt; - unsigned int count = 0; + unsigned int ptmap_len; unsigned int i; /* Extract port information */ @@ -316,10 +316,15 @@ if (!line) goto exit; + /* Clear any previous entries before writing over r->ptmap */ + r->ptmap_len = 0; + /* Keep a local ptmap_len to show only the full list after parsing succeeded in whole. */ + ptmap_len = 0; + pt_str = strtok(line, " "); while (1) { /* Do not allow excessive payload types */ - if (count >= ARRAY_SIZE(r->ptmap)) + if (ptmap_len >= ARRAY_SIZE(r->ptmap)) goto response_parse_failure_pt; pt_str = strtok(NULL, " "); @@ -335,8 +340,8 @@ goto response_parse_failure_pt; /* Do not allow duplicate payload types */ - for (i = 0; i < count; i++) - if (r->codecs[i] == pt) + for (i = 0; i < ptmap_len; i++) + if (r->ptmap[i].pt == pt) goto response_parse_failure_pt; /* Note: The payload type we store may not necessarly match @@ -345,11 +350,13 @@ * match enum mgcp_codecs, we will go through afterwards and * remap the affected entries with the inrofmation we learn * from rtpmap */ - r->codecs[count] = pt; - count++; + r->ptmap[ptmap_len].codec = -1; + r->ptmap[ptmap_len].pt = pt; + ptmap_len++; } - r->codecs_len = count; + /* Parsing succeeded, publish all entries. */ + r->ptmap_len = ptmap_len; exit: return 0; @@ -369,6 +376,7 @@ static int mgcp_parse_audio_ptime_rtpmap(struct mgcp_response *r, const char *line) { unsigned int pt; + unsigned int i; char codec_resp[64]; int rc; @@ -397,9 +405,36 @@ "Failed to parse SDP parameter, can't parse codec in rtpmap: %s\n", osmo_quote_str(line, -1)); return -EINVAL; } - r->ptmap[r->ptmap_len].pt = pt; - r->ptmap[r->ptmap_len].codec = rc; - r->ptmap_len++; + /* Earlier, a line like "m=audio 16002 RTP/AVP 98 112 3" established the desired order of payloads, now + * enrich it with actual codec information provided by "a=rtpmap:..." entries. + * For each, find the entry with the right pt number and add the info there. */ + + + for (i = 0; i < r->ptmap_len; i++) { + if (r->ptmap[i].pt != pt) + continue; + r->ptmap[i].codec = rc; + break; + } + + if (i >= r->ptmap_len) { + /* No entry was found. This is an error in the MGCP protocol, but let's just add another entry + * anyway, to not make it look like it was never there. */ + LOGP(DLMGCP, LOGL_ERROR, + "error in MGCP message: 'a=rtpmap:%u' has no matching entry in 'm=audio ... %u'\n", + pt, pt); + if (r->ptmap_len <= ARRAY_SIZE(r->ptmap)) { + LOGP(DLMGCP, LOGL_ERROR, + "cannot parse all codecs: can only store up to %zu rtpmap entries.\n", + ARRAY_SIZE(r->ptmap)); + return -ENOSPC; + } + r->ptmap[r->ptmap_len] = (struct ptmap){ + .pt = pt, + .codec = rc, + }; + r->ptmap_len++; + } } return 0; @@ -508,7 +543,6 @@ int rc; char *data; char *data_ptr; - int i; /* Since this functions performs a destructive parsing, we create a * local copy of the body data */ @@ -553,10 +587,6 @@ } } - /* See also note in mgcp_parse_audio_port_pt() */ - for (i = 0; i < r->codecs_len; i++) - r->codecs[i] = map_pt_to_codec(r->ptmap, r->ptmap_len, r->codecs[i]); - rc = 0; exit: talloc_free(data); @@ -1233,7 +1263,6 @@ { unsigned int i; const char *codec; - unsigned int pt; #define MSGB_PRINTF_OR_RET(FMT, ARGS...) do { \ if (msgb_printf(msg, FMT, ##ARGS) != 0) { \ @@ -1247,11 +1276,10 @@ if (mgcp_msg->ptime) MSGB_PRINTF_OR_RET(" p:%u,", mgcp_msg->ptime); - if (mgcp_msg->codecs_len) { + if (mgcp_msg->ptmap_len) { MSGB_PRINTF_OR_RET(" a:"); - for (i = 0; i < mgcp_msg->codecs_len; i++) { - pt = mgcp_msg->codecs[i]; - codec = get_value_string_or_null(osmo_mgcpc_codec_names, pt); + for (i = 0; i < mgcp_msg->ptmap_len; i++) { + codec = get_value_string_or_null(osmo_mgcpc_codec_names, mgcp_msg->ptmap[i].codec); /* Note: Use codec descriptors from enum mgcp_codecs * in mgcp_client only! */ @@ -1259,7 +1287,7 @@ return -EINVAL; MSGB_PRINTF_OR_RET("%s", extract_codec_name(codec)); - if (i < mgcp_msg->codecs_len - 1) + if (i < mgcp_msg->ptmap_len - 1) MSGB_PRINTF_OR_RET(";"); } MSGB_PRINTF_OR_RET(","); @@ -1335,20 +1363,20 @@ MSGB_PRINTF_OR_RET("t=0 0\r\n"); MSGB_PRINTF_OR_RET("m=audio %u RTP/AVP", mgcp_msg->audio_port); - for (i = 0; i < mgcp_msg->codecs_len; i++) { - pt = map_codec_to_pt(mgcp_msg->ptmap, mgcp_msg->ptmap_len, mgcp_msg->codecs[i]); - MSGB_PRINTF_OR_RET(" %u", pt); + for (i = 0; i < mgcp_msg->ptmap_len; i++) { + MSGB_PRINTF_OR_RET(" %u", mgcp_msg->ptmap[i].pt); } MSGB_PRINTF_OR_RET("\r\n"); /* Add optional codec parameters (fmtp) */ if (mgcp_msg->param_present) { - for (i = 0; i < mgcp_msg->codecs_len; i++) { + for (i = 0; i < mgcp_msg->ptmap_len; i++) { /* The following is only applicable for AMR */ - if (mgcp_msg->codecs[i] != CODEC_AMR_8000_1 && mgcp_msg->codecs[i] != CODEC_AMRWB_16000_1) + if (mgcp_msg->ptmap[i].codec != CODEC_AMR_8000_1 + && mgcp_msg->ptmap[i].codec != CODEC_AMRWB_16000_1) continue; - pt = map_codec_to_pt(mgcp_msg->ptmap, mgcp_msg->ptmap_len, mgcp_msg->codecs[i]); + pt = mgcp_msg->ptmap[i].pt; if (mgcp_msg->param.amr_octet_aligned_present && mgcp_msg->param.amr_octet_aligned) MSGB_PRINTF_OR_RET("a=fmtp:%u octet-align=1\r\n", pt); else if (mgcp_msg->param.amr_octet_aligned_present && !mgcp_msg->param.amr_octet_aligned) @@ -1356,14 +1384,14 @@ } } - for (i = 0; i < mgcp_msg->codecs_len; i++) { - pt = map_codec_to_pt(mgcp_msg->ptmap, mgcp_msg->ptmap_len, mgcp_msg->codecs[i]); + for (i = 0; i < mgcp_msg->ptmap_len; i++) { + pt = mgcp_msg->ptmap[i].pt; /* Note: Only dynamic payload type from the range 96-127 * require to be explained further via rtpmap. All others * are implcitly definedby the number in m=audio */ if (pt >= 96 && pt <= 127) { - codec = get_value_string_or_null(osmo_mgcpc_codec_names, mgcp_msg->codecs[i]); + codec = get_value_string_or_null(osmo_mgcpc_codec_names, mgcp_msg->ptmap[i].codec); /* Note: Use codec descriptors from enum mgcp_codecs * in mgcp_client only! */ diff --git a/src/libosmo-mgcp-client/mgcp_client_fsm.c b/src/libosmo-mgcp-client/mgcp_client_fsm.c index 660f0ad..775a3a8 100644 --- a/src/libosmo-mgcp-client/mgcp_client_fsm.c +++ b/src/libosmo-mgcp-client/mgcp_client_fsm.c @@ -114,12 +114,10 @@ .call_id = info->call_id, .conn_mode = MGCP_CONN_RECV_ONLY, .ptime = info->ptime, - .codecs_len = info->codecs_len, .ptmap_len = info->ptmap_len, .param_present = info->param_present }; osmo_strlcpy(mgcp_msg->endpoint, info->endpoint, MGCP_ENDPOINT_MAXLEN); - memcpy(mgcp_msg->codecs, info->codecs, sizeof(mgcp_msg->codecs)); memcpy(mgcp_msg->ptmap, info->ptmap, sizeof(mgcp_msg->ptmap)); memcpy(&mgcp_msg->param, &info->param, sizeof(mgcp_msg->param)); @@ -163,12 +161,10 @@ .audio_ip = mgcp_ctx->conn_peer_local.addr, .audio_port = mgcp_ctx->conn_peer_local.port, .ptime = mgcp_ctx->conn_peer_local.ptime, - .codecs_len = mgcp_ctx->conn_peer_local.codecs_len, .ptmap_len = mgcp_ctx->conn_peer_local.ptmap_len, .param_present = mgcp_ctx->conn_peer_local.param_present }; osmo_strlcpy(mgcp_msg.endpoint, mgcp_ctx->conn_peer_remote.endpoint, MGCP_ENDPOINT_MAXLEN); - memcpy(mgcp_msg.codecs, mgcp_ctx->conn_peer_local.codecs, sizeof(mgcp_msg.codecs)); memcpy(mgcp_msg.ptmap, mgcp_ctx->conn_peer_local.ptmap, sizeof(mgcp_msg.ptmap)); memcpy(&mgcp_msg.param, &mgcp_ctx->conn_peer_local.param, sizeof(mgcp_ctx->conn_peer_local.param)); diff --git a/tests/mgcp_client/mgcp_client_test.c b/tests/mgcp_client/mgcp_client_test.c index 37c5f6c..79047b6 100644 --- a/tests/mgcp_client/mgcp_client_test.c +++ b/tests/mgcp_client/mgcp_client_test.c @@ -107,9 +107,6 @@ printf(" audio_port = %u\n", response->audio_port); printf(" audio_ip = %s\n", response->audio_ip); printf(" ptime = %u\n", response->ptime); - printf(" codecs_len = %u\n", response->codecs_len); - for(i=0;i<response->codecs_len;i++) - printf(" codecs[%u] = %u\n", i, response->codecs[i]); printf(" ptmap_len = %u\n", response->ptmap_len); for(i=0;i<response->ptmap_len;i++) { printf(" ptmap[%u].codec = %u\n", i, response->ptmap[i].codec); @@ -149,12 +146,11 @@ .conn_id = "11", .conn_mode = MGCP_CONN_RECV_SEND, .ptime = 20, - .codecs[0] = CODEC_GSM_8000_1, - .codecs[1] = CODEC_AMR_8000_1, - .codecs[2] = CODEC_GSMEFR_8000_1, - .codecs_len = 1, - .ptmap[0].codec = CODEC_GSMEFR_8000_1, - .ptmap[0].pt = 96, + .ptmap = { + { .codec = CODEC_GSM_8000_1, .pt = CODEC_GSM_8000_1 }, + { .codec = CODEC_AMR_8000_1, .pt = CODEC_AMR_8000_1 }, + { .codec = CODEC_GSMEFR_8000_1, .pt = 96 }, + }, .ptmap_len = 1, .x_osmo_ign = MGCP_X_OSMO_IGN_CALLID, .x_osmo_osmux_cid = -1, /* wildcard */ @@ -179,9 +175,9 @@ mgcp_msg.presence = (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID | MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE); - mgcp_msg.codecs_len = 2; + mgcp_msg.ptmap_len = 2; msg = mgcp_msg_gen(mgcp, &mgcp_msg); - mgcp_msg.codecs_len = 1; + mgcp_msg.ptmap_len = 1; printf("%s\n", (char *)msg->data); printf("Generated CRCX message (three codecs, one with custom pt):\n"); @@ -189,9 +185,9 @@ mgcp_msg.presence = (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID | MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE); - mgcp_msg.codecs_len = 3; + mgcp_msg.ptmap_len = 3; msg = mgcp_msg_gen(mgcp, &mgcp_msg); - mgcp_msg.codecs_len = 1; + mgcp_msg.ptmap_len = 1; printf("%s\n", (char *)msg->data); printf("Generated MDCX message:\n"); @@ -209,9 +205,9 @@ (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID | MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE | MGCP_MSG_PRESENCE_AUDIO_IP | MGCP_MSG_PRESENCE_AUDIO_PORT); - mgcp_msg.codecs_len = 2; + mgcp_msg.ptmap_len = 2; msg = mgcp_msg_gen(mgcp, &mgcp_msg); - mgcp_msg.codecs_len = 1; + mgcp_msg.ptmap_len = 1; printf("%s\n", (char *)msg->data); printf("Generated MDCX message (three codecs, one with custom pt):\n"); @@ -220,9 +216,9 @@ (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID | MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE | MGCP_MSG_PRESENCE_AUDIO_IP | MGCP_MSG_PRESENCE_AUDIO_PORT); - mgcp_msg.codecs_len = 3; + mgcp_msg.ptmap_len = 3; msg = mgcp_msg_gen(mgcp, &mgcp_msg); - mgcp_msg.codecs_len = 1; + mgcp_msg.ptmap_len = 1; printf("%s\n", (char *)msg->data); printf("Generated DLCX message:\n"); @@ -330,8 +326,10 @@ .presence = (MGCP_MSG_PRESENCE_ENDPOINT | MGCP_MSG_PRESENCE_CALL_ID | MGCP_MSG_PRESENCE_CONN_ID | MGCP_MSG_PRESENCE_CONN_MODE), .ptime = 20, - .codecs[0] = CODEC_AMR_8000_1, - .codecs_len = 1 + .ptmap = { + { .codec = CODEC_AMR_8000_1, .pt = CODEC_AMR_8000_1 }, + }, + .ptmap_len = 1 }; printf("\n%s():\n", __func__); -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/34899?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I798e02c6663376d3d52f4a74fc4b32411ce95bed Gerrit-Change-Number: 34899 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr@sysmocom.de> Gerrit-MessageType: newchange