Change in osmo-mgw[master]: mgw: clean up codec negotiation (sdp)

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Harald Welte gerrit-no-reply at lists.osmocom.org
Sat Jun 23 11:39:44 UTC 2018


Harald Welte has submitted this change and it was merged. ( https://gerrit.osmocom.org/9648 )

Change subject: mgw: clean up codec negotiation (sdp)
......................................................................

mgw: clean up codec negotiation (sdp)

The codec negotiation via SDP is currently in a neglected state. Also
osmo-mgw does some kind of codec decision wile the SDP is parsed, the
result is information for one codec, even when there are multiple codecs
negotiated. This is problematic because we loose all information about
alternate codecs while we parse. This should be untangled and the
information should be presevered. Also we are not really capable
picking a default. Wehen we do not supply any codec information (not
even LCO), then we should pick a sane default codec.

- separate the codec decision from the sdp parser and concentrate
  codec related code in a separate c file
- add support for multiple codecs in one SDP negotiation
- do not initalize "magic" codec defaults during conn allocation
- do not allow invalid payload types, especially not 255. When
  someone tries to select an invalid payload type, do not fail
  hard, just pick a sane default.
- handle the codec decision in protocol.c, pick a sane default
  codec when no (valid) codec has been negotiated (no LCO, no SDP)

Change-Id: If730d022ba6bdb217ad4e20b3fbbd1114dbb4b8f
Closes: OS#2658
Related: OS#3114
Related: OS#2728
---
M include/osmocom/mgcp/Makefile.am
A include/osmocom/mgcp/mgcp_codec.h
M include/osmocom/mgcp/mgcp_common.h
M include/osmocom/mgcp/mgcp_internal.h
M include/osmocom/mgcp/mgcp_sdp.h
M src/libosmo-mgcp/Makefile.am
A src/libosmo-mgcp/mgcp_codec.c
M src/libosmo-mgcp/mgcp_conn.c
M src/libosmo-mgcp/mgcp_network.c
M src/libosmo-mgcp/mgcp_protocol.c
M src/libosmo-mgcp/mgcp_sdp.c
M src/libosmo-mgcp/mgcp_vty.c
M tests/mgcp/mgcp_test.c
M tests/mgcp/mgcp_test.ok
14 files changed, 655 insertions(+), 247 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/include/osmocom/mgcp/Makefile.am b/include/osmocom/mgcp/Makefile.am
index 7e297e4..65ca670 100644
--- a/include/osmocom/mgcp/Makefile.am
+++ b/include/osmocom/mgcp/Makefile.am
@@ -5,5 +5,6 @@
 	mgcp_stat.h \
 	mgcp_endp.h \
 	mgcp_sdp.h \
+	mgcp_codec.h \
 	debug.h \
 	$(NULL)
diff --git a/include/osmocom/mgcp/mgcp_codec.h b/include/osmocom/mgcp/mgcp_codec.h
new file mode 100644
index 0000000..f8d5e70
--- /dev/null
+++ b/include/osmocom/mgcp/mgcp_codec.h
@@ -0,0 +1,6 @@
+#pragma once
+
+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);
+int mgcp_codec_decide(struct mgcp_conn_rtp *conn);
diff --git a/include/osmocom/mgcp/mgcp_common.h b/include/osmocom/mgcp/mgcp_common.h
index d23339f..eb6564f 100644
--- a/include/osmocom/mgcp/mgcp_common.h
+++ b/include/osmocom/mgcp/mgcp_common.h
@@ -82,4 +82,8 @@
 /* A prefix to denote the virtual trunk (RTP on both ends) */
 #define MGCP_ENDPOINT_PREFIX_VIRTUAL_TRUNK "rtpbridge/"
 
+/* Maximal number of payload types / codecs that can be negotiated via SDP at
+ * at once. */
+#define MGCP_MAX_CODECS 10
+
 #endif
diff --git a/include/osmocom/mgcp/mgcp_internal.h b/include/osmocom/mgcp/mgcp_internal.h
index bff7da0..b564905 100644
--- a/include/osmocom/mgcp/mgcp_internal.h
+++ b/include/osmocom/mgcp/mgcp_internal.h
@@ -114,12 +114,19 @@
 	/* in network byte order */
 	int rtp_port, rtcp_port;
 
-	/* audio codec information */
-	struct mgcp_rtp_codec codec;
+	/* currently selected audio codec */
+	struct mgcp_rtp_codec *codec;
+
+	/* array with assigned audio codecs to choose from (SDP) */
+	struct mgcp_rtp_codec codecs[MGCP_MAX_CODECS];
+
+	/* number of assigned audio codecs (SDP) */
+	unsigned int codecs_assigned;
 
 	/* per endpoint data */
 	int  frames_per_packet;
 	uint32_t packet_duration_ms;
+	int maximum_packet_time; /* -1: not set */
 	char *fmtp_extra;
 	/* are we transmitting packets (1) or dropping (0) outbound packets */
 	int output_enabled;
diff --git a/include/osmocom/mgcp/mgcp_sdp.h b/include/osmocom/mgcp/mgcp_sdp.h
index 240f8cf..950092e 100644
--- a/include/osmocom/mgcp/mgcp_sdp.h
+++ b/include/osmocom/mgcp/mgcp_sdp.h
@@ -26,9 +26,6 @@
 			struct mgcp_conn_rtp *conn,
 			struct mgcp_parse_data *p);
 
-int mgcp_set_audio_info(void *ctx, struct mgcp_rtp_codec *codec,
-			int payload_type, const char *audio_name);
-
 int mgcp_write_response_sdp(const struct mgcp_endpoint *endp,
 			    const struct mgcp_conn_rtp *conn, struct msgb *sdp,
 			    const char *addr);
diff --git a/src/libosmo-mgcp/Makefile.am b/src/libosmo-mgcp/Makefile.am
index a98c391..587bdd4 100644
--- a/src/libosmo-mgcp/Makefile.am
+++ b/src/libosmo-mgcp/Makefile.am
@@ -35,6 +35,7 @@
 	mgcp_vty.c \
 	mgcp_osmux.c \
 	mgcp_sdp.c \
+	mgcp_codec.c \
 	mgcp_msg.c \
 	mgcp_conn.c \
 	mgcp_stat.c \
diff --git a/src/libosmo-mgcp/mgcp_codec.c b/src/libosmo-mgcp/mgcp_codec.c
new file mode 100644
index 0000000..2ce90dd
--- /dev/null
+++ b/src/libosmo-mgcp/mgcp_codec.c
@@ -0,0 +1,343 @@
+/*
+ * (C) 2009-2015 by Holger Hans Peter Freyther <zecke at selfish.org>
+ * (C) 2009-2014 by On-Waves
+ * All Rights Reserved
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+#include <osmocom/mgcp/mgcp_internal.h>
+#include <osmocom/mgcp/mgcp_endp.h>
+#include <errno.h>
+
+/* Helper function to dump codec information of a specified codec to a printable
+ * string, used by dump_codec_summary() */
+static char *dump_codec(struct mgcp_rtp_codec *codec)
+{
+	static char str[256];
+	char *pt_str;
+
+	if (codec->payload_type > 76)
+		pt_str = "DYNAMIC";
+	else if (codec->payload_type > 72)
+		pt_str = "RESERVED <!>";
+	else if (codec->payload_type != PTYPE_UNDEFINED)
+		pt_str = codec->subtype_name;
+	else
+		pt_str = "INVALID <!>";
+
+	snprintf(str, sizeof(str), "(pt:%i=%s, audio:%s subt=%s, rate=%u, ch=%i, t=%u/%u)", codec->payload_type, pt_str,
+		 codec->audio_name, codec->subtype_name, codec->rate, codec->channels, codec->frame_duration_num,
+		 codec->frame_duration_den);
+	return str;
+}
+
+/*! Dump a summary of all negotiated codecs to debug log
+ *  \param[in] conn related rtp-connection. */
+void mgcp_codec_summary(struct mgcp_conn_rtp *conn)
+{
+	struct mgcp_rtp_end *rtp;
+	unsigned int i;
+	struct mgcp_rtp_codec *codec;
+	struct mgcp_endpoint *endp;
+
+	rtp = &conn->end;
+	endp = conn->conn->endp;
+
+	if (rtp->codecs_assigned == 0) {
+		LOGP(DLMGCP, LOGL_ERROR, "endpoint:0x%x conn:%s no codecs available\n", ENDPOINT_NUMBER(endp),
+		     mgcp_conn_dump(conn->conn));
+		return;
+	}
+
+	/* Store parsed codec information */
+	for (i = 0; i < rtp->codecs_assigned; i++) {
+		codec = &rtp->codecs[i];
+
+		LOGP(DLMGCP, LOGL_DEBUG, "endpoint:0x%x conn:%s codecs[%u]:%s", ENDPOINT_NUMBER(endp),
+		     mgcp_conn_dump(conn->conn), i, dump_codec(codec));
+
+		if (codec == rtp->codec)
+			LOGPC(DLMGCP, LOGL_DEBUG, " [selected]");
+
+		LOGPC(DLMGCP, LOGL_DEBUG, "\n");
+	}
+}
+
+/* Initalize or reset codec information with default data. */
+void codec_init(struct mgcp_rtp_codec *codec)
+{
+	if (codec->subtype_name)
+		talloc_free(codec->subtype_name);
+	if (codec->audio_name)
+		talloc_free(codec->audio_name);
+	memset(codec, 0, sizeof(*codec));
+	codec->payload_type = -1;
+	codec->frame_duration_num = DEFAULT_RTP_AUDIO_FRAME_DUR_NUM;
+	codec->frame_duration_den = DEFAULT_RTP_AUDIO_FRAME_DUR_DEN;
+	codec->rate = DEFAULT_RTP_AUDIO_DEFAULT_RATE;
+	codec->channels = DEFAULT_RTP_AUDIO_DEFAULT_CHANNELS;
+}
+
+/*! Initalize or reset codec information with default data.
+ *  \param[out] conn related rtp-connection. */
+void mgcp_codec_reset_all(struct mgcp_conn_rtp *conn)
+{
+	memset(conn->end.codecs, 0, sizeof(conn->end.codecs));
+	conn->end.codecs_assigned = 0;
+	conn->end.codec = NULL;
+}
+
+/* Set members of struct mgcp_rtp_codec, extrapolate in missing information */
+static int codec_set(void *ctx, struct mgcp_rtp_codec *codec,
+		     int payload_type, const char *audio_name, unsigned int pt_offset)
+{
+	int rate;
+	int channels;
+	char audio_codec[64];
+
+	/* Initalize the codec struct with some default data to begin with */
+	codec_init(codec);
+
+	if (payload_type != PTYPE_UNDEFINED) {
+		/* Make sure we do not get any reserved or undefined type numbers */
+		/* See also: https://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml */
+		if (payload_type == 1 || payload_type == 2 || payload_type == 19)
+			goto error;
+		if (payload_type >= 72 && payload_type <= 76)
+			goto error;
+		if (payload_type >= 127)
+			goto error;
+
+		codec->payload_type = payload_type;
+	}
+
+	/* When no audio name is given, we are forced to use the payload
+	 * type to generate the audio name. This is only possible for
+	 * non dynamic payload types, which are statically defined */
+	if (!audio_name) {
+		switch (payload_type) {
+		case 0:
+			audio_name = talloc_strdup(ctx, "PCMU/8000/1");
+			break;
+		case 3:
+			audio_name = talloc_strdup(ctx, "GSM/8000/1");
+			break;
+		case 8:
+			audio_name = talloc_strdup(ctx, "PCMA/8000/1");
+			break;
+		case 18:
+			audio_name = talloc_strdup(ctx, "G729/8000/1");
+			break;
+		default:
+			/* The given payload type is not known to us, or it
+			 * it is a dynamic payload type for which we do not
+			 * know the audio name. We must give up here */
+			goto error;
+		}
+	}
+
+	/* Now we extract the codec subtype name, rate and channels. The latter
+	 * two are optional. If they are not present we use the safe defaults
+	 * above. */
+	if (strlen(audio_name) > sizeof(audio_codec))
+		goto error;
+	channels = DEFAULT_RTP_AUDIO_DEFAULT_CHANNELS;
+	rate = DEFAULT_RTP_AUDIO_DEFAULT_RATE;
+	if (sscanf(audio_name, "%63[^/]/%d/%d", audio_codec, &rate, &channels) < 1)
+		goto error;
+
+	/* Note: We only accept configurations with one audio channel! */
+	if (channels != 1)
+		goto error;
+
+	codec->rate = rate;
+	codec->channels = channels;
+	codec->subtype_name = talloc_strdup(ctx, audio_codec);
+	codec->audio_name = talloc_strdup(ctx, audio_name);
+	codec->payload_type = payload_type;
+
+	if (!strcmp(audio_codec, "G729")) {
+		codec->frame_duration_num = 10;
+		codec->frame_duration_den = 1000;
+	} else {
+		codec->frame_duration_num = DEFAULT_RTP_AUDIO_FRAME_DUR_NUM;
+		codec->frame_duration_den = DEFAULT_RTP_AUDIO_FRAME_DUR_DEN;
+	}
+
+	/* Derive the payload type if it is unknown */
+	if (codec->payload_type == PTYPE_UNDEFINED) {
+
+		/* For the known codecs from the static range we restore
+		 * the IANA or 3GPP assigned payload type number */
+		if (codec->rate == 8000 && codec->channels == 1) {
+			/* See also: https://www.iana.org/assignments/rtp-parameters/rtp-parameters.xhtml */
+			if (!strcmp(codec->subtype_name, "GSM"))
+				codec->payload_type = 3;
+			else if (!strcmp(codec->subtype_name, "PCMA"))
+				codec->payload_type = 8;
+			else if (!strcmp(codec->subtype_name, "PCMU"))
+				codec->payload_type = 0;
+			else if (!strcmp(codec->subtype_name, "G729"))
+				codec->payload_type = 18;
+
+			/* See also: 3GPP TS 48.103, chapter 5.4.2.2 RTP Payload
+			 * Note: These are not fixed payload types as the IANA
+			 * defined once, they still remain dymanic payload
+			 * types, but with a payload type number preference. */
+			else if (!strcmp(codec->subtype_name, "GSM-EFR"))
+				codec->payload_type = 110;
+			else if (!strcmp(codec->subtype_name, "GSM-HR-08"))
+				codec->payload_type = 111;
+			else if (!strcmp(codec->subtype_name, "AMR"))
+				codec->payload_type = 112;
+			else if (!strcmp(codec->subtype_name, "AMR-WB"))
+				codec->payload_type = 113;
+		}
+
+		/* If we could not determine a payload type we assume that
+		 * we are dealing with a codec from the dynamic range. We
+		 * choose a fixed identifier from 96-109. (Note: normally,
+		 * the dynamic payload type rante is from 96-127, but from
+		 * 110 onwards 3gpp defines prefered codec types, which are
+		 * also fixed, see above)  */
+		if (codec->payload_type < 0) {
+			codec->payload_type = 96 + pt_offset;
+			if (codec->payload_type > 109)
+				goto error;
+		}
+	}
+
+	return 0;
+error:
+	/* Make sure we leave a clean codec entry on error. */
+	codec_init(codec);
+	memset(codec, 0, sizeof(*codec));
+	return -EINVAL;
+}
+
+/*! Add codec configuration depending on payload type and/or codec name. This
+ *  function uses the input parameters to extrapolate the full codec information.
+ *  \param[out] codec configuration (caller provided memory).
+ *  \param[out] conn related rtp-connection.
+ *  \param[in] payload_type codec type id (e.g. 3 for GSM, -1 when undefined).
+ *  \param[in] audio_name audio codec name (e.g. "GSM/8000/1").
+ *  \returns 0 on success, -EINVAL on failure. */
+int mgcp_codec_add(struct mgcp_conn_rtp *conn, int payload_type, const char *audio_name)
+{
+	int rc;
+
+	/* The amount of codecs we can store is limited, make sure we do not
+	 * overrun this limit. */
+	if (conn->end.codecs_assigned >= MGCP_MAX_CODECS)
+		return -EINVAL;
+
+	rc = codec_set(conn->conn, &conn->end.codecs[conn->end.codecs_assigned], payload_type, audio_name,
+		       conn->end.codecs_assigned);
+	if (rc != 0)
+		return -EINVAL;
+
+	conn->end.codecs_assigned++;
+
+	return 0;
+}
+
+/* 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)
+{
+	char codec_name[64];
+
+	/* 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 (!codec->subtype_name)
+		return false;
+
+	/* We now extract the codec_name (letters before the /, e.g. "GSM"
+	 * from the audio name that is stored in the trunk configuration.
+	 * We do not compare to the full audio_name because we expect that
+	 * "GSM", "GSM/8000" and "GSM/8000/1" are all compatible when the
+	 * audio name of the codec is set to "GSM" */
+	if (sscanf(endp->tcfg->audio_name, "%63[^/]/%*d/%*d", codec_name) < 1)
+		return false;
+
+	/* Finally we check if the subtype_name we have generated from the
+	 * audio_name in the trunc struct patches the codec_name of the
+	 * given codec */
+	if (strcasecmp(codec_name, codec->subtype_name) == 0)
+		return true;
+
+	/* FIXME: It is questinable that the method to pick a compatible
+	 * codec can work properly. Since this useses tcfg->audio_name, as
+	 * a reference, which is set to "AMR/8000" permanently.
+	 * tcfg->audio_name must be updated by the first connection that
+	 * has been made on an endpoint, so that the second connection
+	 * can make a meaningful decision here */
+
+	return false;
+}
+
+/*! 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->tcfg->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;
+}
diff --git a/src/libosmo-mgcp/mgcp_conn.c b/src/libosmo-mgcp/mgcp_conn.c
index 4926768..e49559c 100644
--- a/src/libosmo-mgcp/mgcp_conn.c
+++ b/src/libosmo-mgcp/mgcp_conn.c
@@ -25,6 +25,8 @@
 #include <osmocom/mgcp/mgcp_internal.h>
 #include <osmocom/mgcp/mgcp_common.h>
 #include <osmocom/mgcp/mgcp_endp.h>
+#include <osmocom/mgcp/mgcp_sdp.h>
+#include <osmocom/mgcp/mgcp_codec.h>
 #include <osmocom/gsm/gsm_utils.h>
 #include <osmocom/core/rate_ctr.h>
 #include <ctype.h>
@@ -88,22 +90,6 @@
 	return -1;
 }
 
-/* Reset codec state and free memory */
-static void mgcp_rtp_codec_init(struct mgcp_rtp_codec *codec)
-{
-	/* see also mgcp_sdp.c, mgcp_set_audio_info() */
-	talloc_free(codec->subtype_name);
-	talloc_free(codec->audio_name);
-
-	codec->payload_type = -1;
-	codec->subtype_name = NULL;
-	codec->audio_name = NULL;
-	codec->frame_duration_num = DEFAULT_RTP_AUDIO_FRAME_DUR_NUM;
-	codec->frame_duration_den = DEFAULT_RTP_AUDIO_FRAME_DUR_DEN;
-	codec->rate = DEFAULT_RTP_AUDIO_DEFAULT_RATE;
-	codec->channels = DEFAULT_RTP_AUDIO_DEFAULT_CHANNELS;
-}
-
 /* Initialize rtp connection struct with default values */
 static void mgcp_rtp_conn_init(struct mgcp_conn_rtp *conn_rtp, struct mgcp_conn *conn)
 {
@@ -130,13 +116,15 @@
 	end->frames_per_packet = 0;	/* unknown */
 	end->packet_duration_ms = DEFAULT_RTP_AUDIO_PACKET_DURATION_MS;
 	end->output_enabled = 0;
-
-	mgcp_rtp_codec_init(&end->codec);
+	end->maximum_packet_time = -1;
 
 	conn_rtp->rate_ctr_group = rate_ctr_group_alloc(conn, &rate_ctr_group_desc, rate_ctr_index);
 	conn_rtp->state.in_stream.err_ts_ctr = &conn_rtp->rate_ctr_group->ctr[IN_STREAM_ERR_TSTMP_CTR];
 	conn_rtp->state.out_stream.err_ts_ctr = &conn_rtp->rate_ctr_group->ctr[OUT_STREAM_ERR_TSTMP_CTR];
 	rate_ctr_index++;
+
+	/* Make sure codec table is reset */
+	mgcp_codec_reset_all(conn_rtp);
 }
 
 /* Cleanup rtp connection struct */
diff --git a/src/libosmo-mgcp/mgcp_network.c b/src/libosmo-mgcp/mgcp_network.c
index 2da37b4..b47b76c 100644
--- a/src/libosmo-mgcp/mgcp_network.c
+++ b/src/libosmo-mgcp/mgcp_network.c
@@ -310,7 +310,7 @@
 			     ENDPOINT_NUMBER(endp), tsdelta,
 			     inet_ntoa(addr->sin_addr), ntohs(addr->sin_port));
 		} else {
-			tsdelta = rtp_end->codec.rate * 20 / 1000;
+			tsdelta = rtp_end->codec->rate * 20 / 1000;
 			LOGP(DRTP, LOGL_NOTICE,
 			     "Fixed packet duration and last timestamp delta "
 			     "are not available on 0x%x, "
@@ -421,8 +421,8 @@
 	     "endpoint:0x%x conn:%s using format defaults\n",
 	     ENDPOINT_NUMBER(endp), mgcp_conn_dump(conn->conn));
 
-	*payload_type = conn->end.codec.payload_type;
-	*audio_name = conn->end.codec.audio_name;
+	*payload_type = conn->end.codec->payload_type;
+	*audio_name = conn->end.codec->audio_name;
 	*fmtp_extra = conn->end.fmtp_extra;
 }
 
@@ -490,7 +490,7 @@
 	uint16_t seq;
 	uint32_t timestamp, ssrc;
 	struct rtp_hdr *rtp_hdr;
-	int payload = rtp_end->codec.payload_type;
+	int payload = rtp_end->codec->payload_type;
 
 	if (len < sizeof(*rtp_hdr))
 		return;
@@ -498,7 +498,7 @@
 	rtp_hdr = (struct rtp_hdr *)data;
 	seq = ntohs(rtp_hdr->sequence);
 	timestamp = ntohl(rtp_hdr->timestamp);
-	arrival_time = get_current_ts(rtp_end->codec.rate);
+	arrival_time = get_current_ts(rtp_end->codec->rate);
 	ssrc = ntohl(rtp_hdr->ssrc);
 	transit = arrival_time - timestamp;
 
@@ -524,7 +524,7 @@
 		     inet_ntoa(addr->sin_addr), ntohs(addr->sin_port));
 		if (state->packet_duration == 0) {
 			state->packet_duration =
-			    rtp_end->codec.rate * 20 / 1000;
+			    rtp_end->codec->rate * 20 / 1000;
 			LOGP(DRTP, LOGL_NOTICE,
 			     "endpoint:0x%x fixed packet duration is not available, "
 			     "using fixed 20ms instead: %d from %s:%d\n",
diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c
index 21f6cf3..eea67a6 100644
--- a/src/libosmo-mgcp/mgcp_protocol.c
+++ b/src/libosmo-mgcp/mgcp_protocol.c
@@ -40,6 +40,7 @@
 #include <osmocom/mgcp/mgcp_msg.h>
 #include <osmocom/mgcp/mgcp_endp.h>
 #include <osmocom/mgcp/mgcp_sdp.h>
+#include <osmocom/mgcp/mgcp_codec.h>
 
 struct mgcp_request {
 	char *name;
@@ -534,12 +535,15 @@
 
 	talloc_free(lco->string);
 	lco->string = talloc_strdup(ctx, options);
-	
+
 	p_opt = strstr(lco->string, "p:");
 	if (p_opt && sscanf(p_opt, "p:%d-%d",
 			    &lco->pkt_period_min, &lco->pkt_period_max) == 1)
 		lco->pkt_period_max = lco->pkt_period_min;
 
+	/* FIXME: LCO also supports the negotiation of more then one codec.
+	 * (e.g. a:PCMU;G726-32) But this implementation only supports a single
+	 * codec only. */
 	a_opt = strstr(lco->string, "a:");
 	if (a_opt && sscanf(a_opt, "a:%8[^,]", codec) == 1) {
 		talloc_free(lco->codec);
@@ -585,15 +589,15 @@
 	/* Get the number of frames per channel and packet */
 	if (rtp->frames_per_packet)
 		f = rtp->frames_per_packet;
-	else if (rtp->packet_duration_ms && rtp->codec.frame_duration_num) {
-		int den = 1000 * rtp->codec.frame_duration_num;
-		f = (rtp->packet_duration_ms * rtp->codec.frame_duration_den +
+	else if (rtp->packet_duration_ms && rtp->codec->frame_duration_num) {
+		int den = 1000 * rtp->codec->frame_duration_num;
+		f = (rtp->packet_duration_ms * rtp->codec->frame_duration_den +
 		     den / 2)
 		    / den;
 	}
 
-	return rtp->codec.rate * f * rtp->codec.frame_duration_num /
-	    rtp->codec.frame_duration_den;
+	return rtp->codec->rate * f * rtp->codec->frame_duration_num /
+	    rtp->codec->frame_duration_den;
 }
 
 static int mgcp_osmux_setup(struct mgcp_endpoint *endp, const char *line)
@@ -609,6 +613,68 @@
 	return mgcp_parse_osmux_cid(line);
 }
 
+/* Process codec information contained in CRCX/MDCX */
+static int handle_codec_info(struct mgcp_conn_rtp *conn,
+			     struct mgcp_parse_data *p, int have_sdp, bool crcx)
+{
+	struct mgcp_endpoint *endp = p->endp;
+	int rc;
+	char *cmd;
+
+	if (crcx)
+		cmd = "CRCX";
+	else
+		cmd = "MDCX";
+
+	/* Collect codec information */
+	if (have_sdp) {
+		/* If we have SDP, we ignore the local connection options and
+		 * use only the SDP information. */
+		mgcp_codec_reset_all(conn);
+		rc = mgcp_parse_sdp_data(endp, conn, p);
+		if (rc != 0) {
+			LOGP(DLMGCP, LOGL_ERROR,
+			     "%s: endpoint:%x sdp not parseable\n", cmd,
+			     ENDPOINT_NUMBER(endp));
+
+			/* See also RFC 3661: Protocol error */
+			return 510;
+		}
+	} else if (endp->local_options.codec) {
+		/* When no SDP is available, we use the codec information from
+		 * the local connection options (if present) */
+		mgcp_codec_reset_all(conn);
+		rc = mgcp_codec_add(conn, PTYPE_UNDEFINED, endp->local_options.codec);
+		if (rc != 0)
+			goto error;
+	}
+
+	/* Make sure we always set a sane default codec */
+	if (conn->end.codecs_assigned == 0) {
+		/* When SDP and/or LCO did not supply any codec information,
+		 * than it makes sense to pick a sane default: (payload-type 0,
+		 * PCMU), see also: OS#2658 */
+		mgcp_codec_reset_all(conn);
+		rc = mgcp_codec_add(conn, 0, NULL);
+		if (rc != 0)
+			goto error;
+	}
+
+	/* Make codec decision */
+	if (mgcp_codec_decide(conn) != 0)
+		goto error;
+
+	return 0;
+
+error:
+	LOGP(DLMGCP, LOGL_ERROR,
+	     "%s: endpoint:0x%x codec negotiation failure\n", cmd,
+	     ENDPOINT_NUMBER(endp));
+
+	/* See also RFC 3661: Codec negotiation failure */
+	return 534;
+}
+
 /* CRCX command handler, processes the received command */
 static struct msgb *handle_create_con(struct mgcp_parse_data *p)
 {
@@ -726,17 +792,6 @@
 	 * connection ids) */
 	endp->callid = talloc_strdup(tcfg->endpoints, callid);
 
-	/* Extract audio codec information */
-	rc = set_local_cx_options(endp->tcfg->endpoints, &endp->local_options,
-				  local_options);
-	if (rc != 0) {
-		LOGP(DLMGCP, LOGL_ERROR,
-		     "CRCX: endpoint:%x inavlid local connection options!\n",
-		     ENDPOINT_NUMBER(endp));
-		error_code = rc;
-		goto error2;
-	}
-
 	snprintf(conn_name, sizeof(conn_name), "%s", callid);
 	_conn = mgcp_conn_alloc(NULL, endp, MGCP_CONN_TYPE_RTP, conn_name);
 	if (!_conn) {
@@ -767,12 +822,27 @@
 		goto error2;
 	}
 
-	/* set up RTP media parameters */
-	if (have_sdp)
-		mgcp_parse_sdp_data(endp, conn, p);
-	else if (endp->local_options.codec)
-		mgcp_set_audio_info(p->cfg, &conn->end.codec,
-				    PTYPE_UNDEFINED, endp->local_options.codec);
+	/* Set local connection options, if present */
+	if (local_options) {
+		rc = set_local_cx_options(endp->tcfg->endpoints,
+					  &endp->local_options, local_options);
+		if (rc != 0) {
+			LOGP(DLMGCP, LOGL_ERROR,
+			     "CRCX: endpoint:%x inavlid local connection options!\n",
+			     ENDPOINT_NUMBER(endp));
+			error_code = rc;
+			goto error2;
+		}
+	}
+
+	/* Handle codec information and decide for a suitable codec */
+	rc = handle_codec_info(conn, p, have_sdp, true);
+	mgcp_codec_summary(conn);
+	if (rc) {
+		error_code = rc;
+		goto error2;
+	}
+
 	conn->end.fmtp_extra = talloc_strdup(tcfg->endpoints,
 					     tcfg->audio_fmtp_extra);
 
@@ -852,6 +922,10 @@
 	return create_err_response(endp, error_code, "CRCX", p->trans);
 }
 
+
+
+
+
 /* MDCX command handler, processes the received command */
 static struct msgb *handle_modify_con(struct mgcp_parse_data *p)
 {
@@ -943,23 +1017,27 @@
 	} else
 			conn->conn->mode = conn->conn->mode_orig;
 
-	if (have_sdp)
-		mgcp_parse_sdp_data(endp, conn, p);
+	/* Set local connection options, if present */
+	if (local_options) {
+		rc = set_local_cx_options(endp->tcfg->endpoints,
+					  &endp->local_options, local_options);
+		if (rc != 0) {
+			LOGP(DLMGCP, LOGL_ERROR,
+			     "MDCX: endpoint:%x inavlid local connection options!\n",
+			     ENDPOINT_NUMBER(endp));
+			error_code = rc;
+			goto error3;
+		}
+	}
 
-	rc = set_local_cx_options(endp->tcfg->endpoints, &endp->local_options,
-				  local_options);
-	if (rc != 0) {
-		LOGP(DLMGCP, LOGL_ERROR,
-		     "MDCX: endpoint:%x inavlid local connection options!\n",
-		     ENDPOINT_NUMBER(endp));
+	/* Handle codec information and decide for a suitable codec */
+	rc = handle_codec_info(conn, p, have_sdp, false);
+	mgcp_codec_summary(conn);
+	if (rc) {
 		error_code = rc;
 		goto error3;
 	}
 
-	if (!have_sdp && endp->local_options.codec)
-		mgcp_set_audio_info(p->cfg, &conn->end.codec,
-				    PTYPE_UNDEFINED, endp->local_options.codec);
-
 	/* check connection mode setting */
 	if (conn->conn->mode != MGCP_CONN_LOOPBACK
 	    && conn->conn->mode != MGCP_CONN_RECV_ONLY
@@ -971,6 +1049,7 @@
 		goto error3;
 	}
 
+
 	if (setup_rtp_processing(endp, conn) != 0)
 		goto error3;
 
diff --git a/src/libosmo-mgcp/mgcp_sdp.c b/src/libosmo-mgcp/mgcp_sdp.c
index 5cc34ea..102c8c3 100644
--- a/src/libosmo-mgcp/mgcp_sdp.c
+++ b/src/libosmo-mgcp/mgcp_sdp.c
@@ -25,9 +25,13 @@
 #include <osmocom/mgcp/mgcp_internal.h>
 #include <osmocom/mgcp/mgcp_msg.h>
 #include <osmocom/mgcp/mgcp_endp.h>
+#include <osmocom/mgcp/mgcp_codec.h>
 
 #include <errno.h>
 
+/* A struct to store intermediate parsing results. The function
+ * mgcp_parse_sdp_data() is using it as temporary storage for parsing the SDP
+ * codec information. */
 struct sdp_rtp_map {
 	/* the type */
 	int payload_type;
@@ -40,89 +44,8 @@
 	int channels;
 };
 
-/*! Set codec configuration depending on payload type and codec name.
- *  \param[in] ctx talloc context.
- *  \param[out] codec configuration (caller provided memory).
- *  \param[in] payload_type codec type id (e.g. 3 for GSM, -1 when undefined).
- *  \param[in] audio_name audio codec name (e.g. "GSM/8000/1").
- *  \returns 0 on success, -1 on failure. */
-int mgcp_set_audio_info(void *ctx, struct mgcp_rtp_codec *codec,
-			int payload_type, const char *audio_name)
-{
-	int rate = codec->rate;
-	int channels = codec->channels;
-	char audio_codec[64];
-
-	talloc_free(codec->subtype_name);
-	codec->subtype_name = NULL;
-	talloc_free(codec->audio_name);
-	codec->audio_name = NULL;
-
-	if (payload_type != PTYPE_UNDEFINED)
-		codec->payload_type = payload_type;
-
-	if (!audio_name) {
-		switch (payload_type) {
-		case 0:
-			audio_name = "PCMU/8000/1";
-			break;
-		case 3:
-			audio_name = "GSM/8000/1";
-			break;
-		case 8:
-			audio_name = "PCMA/8000/1";
-			break;
-		case 18:
-			audio_name = "G729/8000/1";
-			break;
-		default:
-			/* Payload type is unknown, don't change rate and
-			 * channels. */
-			/* TODO: return value? */
-			return 0;
-		}
-	}
-
-	if (sscanf(audio_name, "%63[^/]/%d/%d",
-		   audio_codec, &rate, &channels) < 1)
-		return -EINVAL;
-
-	codec->rate = rate;
-	codec->channels = channels;
-	codec->subtype_name = talloc_strdup(ctx, audio_codec);
-	codec->audio_name = talloc_strdup(ctx, audio_name);
-
-	if (!strcmp(audio_codec, "G729")) {
-		codec->frame_duration_num = 10;
-		codec->frame_duration_den = 1000;
-	} else {
-		codec->frame_duration_num = DEFAULT_RTP_AUDIO_FRAME_DUR_NUM;
-		codec->frame_duration_den = DEFAULT_RTP_AUDIO_FRAME_DUR_DEN;
-	}
-
-	if (payload_type < 0) {
-		payload_type = 96;
-		if (rate == 8000 && channels == 1) {
-			if (!strcmp(audio_codec, "GSM"))
-				payload_type = 3;
-			else if (!strcmp(audio_codec, "PCMA"))
-				payload_type = 8;
-			else if (!strcmp(audio_codec, "PCMU"))
-				payload_type = 0;
-			else if (!strcmp(audio_codec, "G729"))
-				payload_type = 18;
-		}
-
-		codec->payload_type = payload_type;
-	}
-
-	if (channels != 1)
-		LOGP(DLMGCP, LOGL_NOTICE,
-		     "Channels != 1 in SDP: '%s'\n", audio_name);
-
-	return 0;
-}
-
+/* Helper function to extrapolate missing codec parameters in a codec mao from
+ * an already filled in payload_type, called from: mgcp_parse_sdp_data() */
 static void codecs_initialize(void *ctx, struct sdp_rtp_map *codecs, int used)
 {
 	int i;
@@ -149,10 +72,16 @@
 			codecs[i].rate = 8000;
 			codecs[i].channels = 1;
 			break;
+		default:
+			codecs[i].codec_name = NULL;
+			codecs[i].rate = 0;
+			codecs[i].channels = 0;
 		}
 	}
 }
 
+/* Helper function to update codec map information with additional data from
+ * SDP, called from: mgcp_parse_sdp_data() */
 static void codecs_update(void *ctx, struct sdp_rtp_map *codecs, int used,
 			  int payload, const char *audio_name)
 {
@@ -162,8 +91,13 @@
 		char audio_codec[64];
 		int rate = -1;
 		int channels = -1;
+
+		/* Note: We can only update payload codecs that already exist
+		 * in our codec list. If we get an unexpected payload type,
+		 * we just drop it */
 		if (codecs[i].payload_type != payload)
 			continue;
+
 		if (sscanf(audio_name, "%63[^/]/%d/%d",
 			   audio_codec, &rate, &channels) < 1) {
 			LOGP(DLMGCP, LOGL_ERROR, "Failed to parse '%s'\n",
@@ -182,43 +116,72 @@
 	     audio_name);
 }
 
-/* Check if the codec matches what is set up in the trunk config */
-static int is_codec_compatible(const struct mgcp_endpoint *endp,
-			       const struct sdp_rtp_map *codec)
+/* Extract payload types from SDP, also check for duplicates */
+static int pt_from_sdp(void *ctx, struct sdp_rtp_map *codecs,
+		       unsigned int codecs_len, char *sdp)
 {
-	char *codec_str;
-	char audio_codec[64];
+	char *str;
+	char *str_ptr;
+	char *pt_str;
+	unsigned int pt;
+	unsigned int count = 0;
+	unsigned int i;
 
-	if (!codec->codec_name)
-		return 0;
+	str = talloc_zero_size(ctx, strlen(sdp) + 1);
+	str_ptr = str;
+	strcpy(str_ptr, sdp);
 
-	/* GSM, GSM/8000 and GSM/8000/1 should all be compatible...
-	 * let's go by name first. */
-	codec_str = endp->tcfg->audio_name;
-	if (sscanf(codec_str, "%63[^/]/%*d/%*d", audio_codec) < 1)
-		return 0;
+	str_ptr = strstr(str_ptr, "RTP/AVP ");
+	if (!str_ptr)
+		goto exit;
 
-	return strcasecmp(audio_codec, codec->codec_name) == 0;
+	pt_str = strtok(str_ptr, " ");
+	if (!pt_str)
+		goto exit;
+
+	while (1) {
+		/* Do not allow excessive payload types */
+		if (count > codecs_len)
+			goto error;
+
+		pt_str = strtok(NULL, " ");
+		if (!pt_str)
+			break;
+
+		pt = atoi(pt_str);
+
+		/* Do not allow duplicate payload types */
+		for (i = 0; i < count; i++)
+			if (codecs[i].payload_type == pt)
+				goto error;
+
+		codecs[count].payload_type = pt;
+		count++;
+	}
+
+exit:
+	talloc_free(str);
+	return count;
+error:
+	talloc_free(str);
+	return -EINVAL;
 }
 
 /*! Analyze SDP input string.
  *  \param[in] endp trunk endpoint.
  *  \param[out] conn associated rtp connection.
  *  \param[out] caller provided memory to store the parsing results.
- *  \returns 1 when a codec is assigned, 0 when no codec is assigned
  *
  *  Note: In conn (conn->end) the function returns the packet duration,
- *  rtp port, rtcp port and the assigned codec. */
+ *  rtp port, rtcp port and the codec information.
+ *  \returns 0 on success, -1 on failure. */
 int mgcp_parse_sdp_data(const struct mgcp_endpoint *endp,
-			struct mgcp_conn_rtp *conn,
-			struct mgcp_parse_data *p)
+			struct mgcp_conn_rtp *conn, struct mgcp_parse_data *p)
 {
-	struct sdp_rtp_map codecs[10];
-	int codecs_used = 0;
+	struct sdp_rtp_map codecs[MGCP_MAX_CODECS];
+	unsigned int codecs_used = 0;
 	char *line;
-	int maxptime = -1;
-	int i;
-	bool codec_assigned = false;
+	unsigned int i;
 	void *tmp_ctx = talloc_new(NULL);
 	struct mgcp_rtp_end *rtp;
 
@@ -255,34 +218,21 @@
 					rtp->packet_duration_ms = 0;
 				else
 					rtp->packet_duration_ms = ptime;
-			} else if (sscanf(line, "a=maxptime:%d", &ptime2)
-				   == 1) {
-				maxptime = ptime2;
+			} else if (sscanf(line, "a=maxptime:%d", &ptime2) == 1) {
+				rtp->maximum_packet_time = ptime2;
 			}
 			break;
 		case 'm':
-			rc = sscanf(line,
-				    "m=audio %d RTP/AVP %d %d %d %d %d %d %d %d %d %d",
-				    &port, &codecs[0].payload_type,
-				    &codecs[1].payload_type,
-				    &codecs[2].payload_type,
-				    &codecs[3].payload_type,
-				    &codecs[4].payload_type,
-				    &codecs[5].payload_type,
-				    &codecs[6].payload_type,
-				    &codecs[7].payload_type,
-				    &codecs[8].payload_type,
-				    &codecs[9].payload_type);
-			if (rc >= 2) {
+			rc = sscanf(line, "m=audio %d RTP/AVP", &port);
+			if (rc == 1) {
 				rtp->rtp_port = htons(port);
 				rtp->rtcp_port = htons(port + 1);
-				codecs_used = rc - 1;
-
-				/* So far we have only set the payload type in
-				 * the codec struct. Now we fill up the
-				 * remaining fields of the codec description */
-				codecs_initialize(tmp_ctx, codecs, codecs_used);
 			}
+
+			rc = pt_from_sdp(conn->conn, codecs,
+					 ARRAY_SIZE(codecs), line);
+			if (rc > 0)
+				codecs_used = rc;
 			break;
 		case 'c':
 
@@ -303,47 +253,36 @@
 			break;
 		}
 	}
+	OSMO_ASSERT(codecs_used <= MGCP_MAX_CODECS);
 
-	/* Now select a suitable codec */
+	/* So far we have only set the payload type in the codec struct. Now we
+	 * fill up the remaining fields of the codec description with some default
+	 * information */
+	codecs_initialize(tmp_ctx, codecs, codecs_used);
+
+	/* Store parsed codec information */
 	for (i = 0; i < codecs_used; i++) {
-
-		/* When no transcoding is available, avoid codecs that would
-		 * require transcoding. */
-		if (endp->tcfg->no_audio_transcoding &&
-		    !is_codec_compatible(endp, &codecs[i])) {
-			LOGP(DLMGCP, LOGL_NOTICE, "Skipping codec %s\n",
-			     codecs[i].codec_name);
-			continue;
-		}
-
-		mgcp_set_audio_info(p->cfg, &rtp->codec,
-				    codecs[i].payload_type, codecs[i].map_line);
-
-		codec_assigned = true;
-		break;
-	}
-
-	if (codec_assigned) {
-		/* TODO/XXX: Store this per codec and derive it on use */
-		if (maxptime >= 0 && maxptime * rtp->codec.frame_duration_den >
-		    rtp->codec.frame_duration_num * 1500) {
-			/* more than 1 frame */
-			rtp->packet_duration_ms = 0;
-		}
-
-		LOGP(DLMGCP, LOGL_NOTICE,
-		     "Got media info via SDP: port %d, payload %d (%s), "
-		     "duration %d, addr %s\n",
-		     ntohs(rtp->rtp_port), rtp->codec.payload_type,
-		     rtp->codec.subtype_name ? rtp->
-		     codec.subtype_name : "unknown", rtp->packet_duration_ms,
-		     inet_ntoa(rtp->addr));
+		rc = mgcp_codec_add(conn, codecs[i].payload_type, codecs[i].map_line);
+		if (rc < 0)
+			LOGP(DLMGCP, LOGL_NOTICE, "endpoint:0x%x, failed to add codec\n", ENDPOINT_NUMBER(p->endp));
 	}
 
 	talloc_free(tmp_ctx);
 
-	if (codec_assigned)
-		return 1;
+	LOGP(DLMGCP, LOGL_NOTICE,
+	     "Got media info via SDP: port:%d, addr:%s, duration:%d, payload-types:",
+	     ntohs(rtp->rtp_port), inet_ntoa(rtp->addr),
+	     rtp->packet_duration_ms);
+	if (codecs_used == 0)
+		LOGPC(DLMGCP, LOGL_NOTICE, "none");
+	for (i = 0; i < codecs_used; i++) {
+		LOGPC(DLMGCP, LOGL_NOTICE, "%d=%s",
+		      rtp->codecs[i].payload_type,
+		      rtp->codecs[i].subtype_name ? rtp-> codecs[i].subtype_name : "unknown");
+		LOGPC(DLMGCP, LOGL_NOTICE, " ");
+	}
+	LOGPC(DLMGCP, LOGL_NOTICE, "\n");
+
 	return 0;
 }
 
@@ -389,7 +328,9 @@
 		if (rc < 0)
 			goto buffer_too_small;
 
-		if (audio_name && endp->tcfg->audio_send_name) {
+		/* FIXME: Check if the payload type is from the static range,
+		 * if yes, omitthe a=rtpmap since it is unnecessary */
+		if (audio_name && endp->tcfg->audio_send_name && (payload_type >= 96 && payload_type <= 127)) {
 			rc = msgb_printf(sdp, "a=rtpmap:%d %s\r\n",
 					 payload_type, audio_name);
 
diff --git a/src/libosmo-mgcp/mgcp_vty.c b/src/libosmo-mgcp/mgcp_vty.c
index 3c1b596..a7a1feb 100644
--- a/src/libosmo-mgcp/mgcp_vty.c
+++ b/src/libosmo-mgcp/mgcp_vty.c
@@ -157,7 +157,7 @@
 static void dump_rtp_end(struct vty *vty, struct mgcp_rtp_state *state,
 			 struct mgcp_rtp_end *end)
 {
-	struct mgcp_rtp_codec *codec = &end->codec;
+	struct mgcp_rtp_codec *codec = end->codec;
 
 	vty_out(vty,
 		"   Timestamp Errs: %lu->%lu%s"
diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c
index 3fc8bc0..1d2cf4a 100644
--- a/tests/mgcp/mgcp_test.c
+++ b/tests/mgcp/mgcp_test.c
@@ -26,6 +26,8 @@
 #include <osmocom/mgcp/mgcp_stat.h>
 #include <osmocom/mgcp/mgcp_msg.h>
 #include <osmocom/mgcp/mgcp_endp.h>
+#include <osmocom/mgcp/mgcp_sdp.h>
+#include <osmocom/mgcp/mgcp_codec.h>
 
 #include <osmocom/core/application.h>
 #include <osmocom/core/talloc.h>
@@ -156,8 +158,8 @@
 	"s=-\r\n" \
 	"c=IN IP4 0.0.0.0\r\n" \
 	"t=0 0\r\n" \
-	"m=audio 16002 RTP/AVP 96\r\n" \
-	"a=rtpmap:96 AMR\r\n" \
+	"m=audio 16002 RTP/AVP 112\r\n" \
+	"a=rtpmap:112 AMR\r\n" \
 	"a=ptime:40\r\n"
 
 #define MDCX4_PT1 \
@@ -404,7 +406,7 @@
 	"v=0\r\n" \
 	"o=- 1439038275 1439038275 IN IP4 192.168.181.247\r\n" \
 	"s=-\r\nc=IN IP4 192.168.181.247\r\n" \
-	"t=0 0\r\nm=audio 29084 RTP/AVP 255 0 8 3 18 4 96 97 101\r\n" \
+	"t=0 0\r\nm=audio 29084 RTP/AVP 0 8 3 18 4 96 97 101\r\n" \
 	"a=rtpmap:0 PCMU/8000\r\n" \
 	"a=rtpmap:8 PCMA/8000\r\n" \
 	"a=rtpmap:3 gsm/8000\r\n" \
@@ -425,7 +427,24 @@
 	"I: %s\r\n" \
 	"\r\n" \
 	"c=IN IP4 8.8.8.8\r\n" \
-	"m=audio 16434 RTP/AVP 255\r\n"
+	"m=audio 16434 RTP/AVP 3\r\n"
+
+#define CRCX_NO_LCO_NO_SDP \
+	"CRCX 2 6 at mgw MGCP 1.0\r\n" \
+	"M: recvonly\r\n" \
+	"C: 2\r\n"
+
+#define CRCX_NO_LCO_NO_SDP_RET \
+	"200 2 OK\r\n" \
+	"I: %s\r\n" \
+	"\r\n" \
+	"v=0\r\n" \
+	"o=- %s 23 IN IP4 0.0.0.0\r\n" \
+	"s=-\r\n" \
+	"c=IN IP4 0.0.0.0\r\n" \
+	"t=0 0\r\n" \
+	"m=audio 16008 RTP/AVP 0\r\n" \
+	"a=ptime:20\r\n"
 
 struct mgcp_test {
 	const char *name;
@@ -462,6 +481,7 @@
 	{"MDCX3", MDCX3, MDCX3_FMTP_RET, PTYPE_NONE,.extra_fmtp =
 	 "a=fmtp:126 0/1/2"},
 	{"DLCX", DLCX, DLCX_RET, PTYPE_IGNORE,.extra_fmtp = "a=fmtp:126 0/1/2"},
+	{"CRCX", CRCX_NO_LCO_NO_SDP, CRCX_NO_LCO_NO_SDP_RET, 97},
 };
 
 static const struct mgcp_test retransmit[] = {
@@ -764,14 +784,14 @@
 			fprintf(stderr, "endpoint %d: "
 				"payload type %d (expected %d)\n",
 				last_endpoint,
-				conn->end.codec.payload_type, t->ptype);
+				conn->end.codec->payload_type, t->ptype);
 
 			if (t->ptype != PTYPE_IGNORE)
-				OSMO_ASSERT(conn->end.codec.payload_type ==
+				OSMO_ASSERT(conn->end.codec->payload_type ==
 					    t->ptype);
 
 			/* Reset them again for next test */
-			conn->end.codec.payload_type = PTYPE_NONE;
+			conn->end.codec->payload_type = PTYPE_NONE;
 		}
 	}
 
@@ -1167,7 +1187,8 @@
 
 	rtp = &conn->end;
 
-	rtp->codec.payload_type = 98;
+	OSMO_ASSERT(mgcp_codec_add(conn, PTYPE_UNDEFINED, "AMR/8000/1") == 0);
+	rtp->codec = &rtp->codecs[0];
 
 	for (i = 0; i < ARRAY_SIZE(test_rtp_packets1); ++i) {
 		struct rtp_packet_info *info = test_rtp_packets1 + i;
@@ -1243,7 +1264,7 @@
 	endp = &cfg->trunk.endpoints[last_endpoint];
 	conn = mgcp_conn_get_rtp(endp, conn_id);
 	OSMO_ASSERT(conn);
-	OSMO_ASSERT(conn->end.codec.payload_type == 18);
+	OSMO_ASSERT(conn->end.codec->payload_type == 18);
 
 	/* Allocate 2 at mgw with three codecs, last one ignored */
 	last_endpoint = -1;
@@ -1258,9 +1279,14 @@
 	endp = &cfg->trunk.endpoints[last_endpoint];
 	conn = mgcp_conn_get_rtp(endp, conn_id);
 	OSMO_ASSERT(conn);
-	OSMO_ASSERT(conn->end.codec.payload_type == 18);
+	OSMO_ASSERT(conn->end.codec->payload_type == 18);
 
-	/* Allocate 3 at mgw with no codecs, check for PT == -1 */
+	/* Allocate 3 at mgw with no codecs, check for PT == 0 */
+	/* Note: It usually makes no sense to leave the payload type list
+	 * out. However RFC 2327 does not clearly forbid this case and
+	 * it makes and since we already decided in OS#2658 that a missing
+	 * LCO should pick a sane default codec, it makes sense to expect
+	 * the same behaviour if SDP lacks proper payload type information */
 	last_endpoint = -1;
 	inp = create_msg(CRCX_MULT_3, NULL);
 	resp = mgcp_handle_message(cfg, inp);
@@ -1273,7 +1299,7 @@
 	endp = &cfg->trunk.endpoints[last_endpoint];
 	conn = mgcp_conn_get_rtp(endp, conn_id);
 	OSMO_ASSERT(conn);
-	OSMO_ASSERT(conn->end.codec.payload_type == -1);
+	OSMO_ASSERT(conn->end.codec->payload_type == 0);
 
 	/* Allocate 4 at mgw with a single codec */
 	last_endpoint = -1;
@@ -1288,7 +1314,7 @@
 	endp = &cfg->trunk.endpoints[last_endpoint];
 	conn = mgcp_conn_get_rtp(endp, conn_id);
 	OSMO_ASSERT(conn);
-	OSMO_ASSERT(conn->end.codec.payload_type == 18);
+	OSMO_ASSERT(conn->end.codec->payload_type == 18);
 
 	/* Allocate 5 at mgw at select GSM.. */
 	last_endpoint = -1;
@@ -1306,7 +1332,7 @@
 	endp = &cfg->trunk.endpoints[last_endpoint];
 	conn = mgcp_conn_get_rtp(endp, conn_id);
 	OSMO_ASSERT(conn);
-	OSMO_ASSERT(conn->end.codec.payload_type == 3);
+	OSMO_ASSERT(conn->end.codec->payload_type == 3);
 
 	inp = create_msg(MDCX_NAT_DUMMY, conn_id);
 	last_endpoint = -1;
@@ -1317,7 +1343,7 @@
 	endp = &cfg->trunk.endpoints[last_endpoint];
 	conn = mgcp_conn_get_rtp(endp, conn_id);
 	OSMO_ASSERT(conn);
-	OSMO_ASSERT(conn->end.codec.payload_type == 3);
+	OSMO_ASSERT(conn->end.codec->payload_type == 3);
 	OSMO_ASSERT(conn->end.rtp_port == htons(16434));
 	memset(&addr, 0, sizeof(addr));
 	inet_aton("8.8.8.8", &addr);
@@ -1347,7 +1373,7 @@
 	endp = &cfg->trunk.endpoints[last_endpoint];
 	conn = mgcp_conn_get_rtp(endp, conn_id);
 	OSMO_ASSERT(conn);
-	OSMO_ASSERT(conn->end.codec.payload_type == 255);
+	OSMO_ASSERT(conn->end.codec->payload_type == 0);
 
 	talloc_free(cfg);
 }
diff --git a/tests/mgcp/mgcp_test.ok b/tests/mgcp/mgcp_test.ok
index e293533..9838f4d 100644
--- a/tests/mgcp/mgcp_test.ok
+++ b/tests/mgcp/mgcp_test.ok
@@ -408,6 +408,21 @@
 Testing CRCX
 creating message from statically defined input:
 ---------8<---------
+CRCX 2 6 at mgw MGCP 1.0
+M: recvonly
+C: 2
+
+---------8<---------
+checking response:
+using message with patched conn_id for comparison
+Response matches our expectations.
+(response does not contain a connection id)
+Dummy packets: 2
+
+================================================
+Testing CRCX
+creating message from statically defined input:
+---------8<---------
 CRCX 2 1 at mgw MGCP 1.0
 M: recvonly
 C: 2
@@ -1031,7 +1046,7 @@
 s=-
 c=IN IP4 192.168.181.247
 t=0 0
-m=audio 29084 RTP/AVP 255 0 8 3 18 4 96 97 101
+m=audio 29084 RTP/AVP 0 8 3 18 4 96 97 101
 a=rtpmap:0 PCMU/8000
 a=rtpmap:8 PCMA/8000
 a=rtpmap:3 gsm/8000
@@ -1054,7 +1069,7 @@
 I: %s
 
 c=IN IP4 8.8.8.8
-m=audio 16434 RTP/AVP 255
+m=audio 16434 RTP/AVP 3
 
 ---------8<---------
 creating message from statically defined input:
@@ -1069,7 +1084,7 @@
 s=-
 c=IN IP4 192.168.181.247
 t=0 0
-m=audio 29084 RTP/AVP 255 0 8 3 18 4 96 97 101
+m=audio 29084 RTP/AVP 0 8 3 18 4 96 97 101
 a=rtpmap:0 PCMU/8000
 a=rtpmap:8 PCMA/8000
 a=rtpmap:3 gsm/8000

-- 
To view, visit https://gerrit.osmocom.org/9648
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If730d022ba6bdb217ad4e20b3fbbd1114dbb4b8f
Gerrit-Change-Number: 9648
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180623/83c0bedf/attachment.htm>


More information about the gerrit-log mailing list