[PATCH] libosmocore[master]: gsm0808: fix AoIP speech codec element parser/generator

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/.

dexter gerrit-no-reply at lists.osmocom.org
Mon Jun 19 14:29:39 UTC 2017


Hello Neels Hofmeyr, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/2820

to look at the new patch set (#5).

gsm0808: fix AoIP speech codec element parser/generator

The implementation of the parser/generator for the speech codec
information element slightly wrong, making it impossible to use
it properly.

(See also: 3GPP TS 48.008, 3.2.2.103)

Change-Id: Idabb0f9620659557672e1c6b90c75481192e5c89
---
M include/osmocom/gsm/protocol/gsm_08_08.h
M src/gsm/gsm0808_utils.c
M tests/gsm0808/gsm0808_test.c
3 files changed, 136 insertions(+), 79 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/20/2820/5

diff --git a/include/osmocom/gsm/protocol/gsm_08_08.h b/include/osmocom/gsm/protocol/gsm_08_08.h
index 7656c38..9e00d23 100644
--- a/include/osmocom/gsm/protocol/gsm_08_08.h
+++ b/include/osmocom/gsm/protocol/gsm_08_08.h
@@ -447,8 +447,6 @@
 	bool tf;
 	uint8_t type;
 	uint16_t cfg;
-	bool type_extended;
-	bool cfg_present;
 };
 
 /* 3GPP TS 48.008 3.2.2.103 Speech Codec List */
diff --git a/src/gsm/gsm0808_utils.c b/src/gsm/gsm0808_utils.c
index bdd02e5..126ce17 100644
--- a/src/gsm/gsm0808_utils.c
+++ b/src/gsm/gsm0808_utils.c
@@ -151,6 +151,32 @@
 	/* See also 3GPP TS 48.008 3.2.2.103 Speech Codec List */
 	uint8_t header = 0;
 	uint8_t *old_tail;
+	bool type_extended;
+
+	/* Note: Extended codec types are codec types that require 8 instead
+	 * of 4 bit to fully specify the selected codec. In the following,
+	 * we check if we work with an extended type or not. We also check
+	 * if the codec type is valid at all. */
+	switch(sc->type) {
+	case GSM0808_SCT_FR1:
+	case GSM0808_SCT_FR2:
+	case GSM0808_SCT_FR3:
+	case GSM0808_SCT_FR4:
+	case GSM0808_SCT_FR5:
+	case GSM0808_SCT_HR1:
+	case GSM0808_SCT_HR3:
+	case GSM0808_SCT_HR4:
+	case GSM0808_SCT_HR6:
+		type_extended = false;
+		break;
+	case GSM0808_SCT_CSD:
+		type_extended = true;
+		break;
+	default:
+		/* Invalid codec type specified */
+		OSMO_ASSERT(false);
+		break;
+	}
 
 	old_tail = msg->tail;
 
@@ -162,20 +188,37 @@
 		header |= (1 << 5);
 	if (sc->tf)
 		header |= (1 << 4);
-	if (sc->type_extended) {
+
+	if (type_extended) {
 		header |= 0x0f;
 		msgb_put_u8(msg, header);
+		msgb_put_u8(msg, sc->type);
 	} else {
 		OSMO_ASSERT(sc->type < 0x0f);
 		header |= sc->type;
 		msgb_put_u8(msg, header);
-		return (uint8_t) (msg->tail - old_tail);
 	}
 
-	msgb_put_u8(msg, sc->type);
-
-	if (sc->cfg_present)
+	/* Note: Whether a configuration is present or not depends on the
+	 * selected codec type. If present, it can either consist of one
+	 * or two octets, depending on the codec type */
+	switch (sc->type) {
+	case GSM0808_SCT_FR3:
+	case GSM0808_SCT_HR3:
+	case GSM0808_SCT_HR6:
 		msgb_put_u16(msg, sc->cfg);
+		break;
+	case GSM0808_SCT_FR4:
+	case GSM0808_SCT_FR5:
+	case GSM0808_SCT_HR4:
+	case GSM0808_SCT_CSD:
+		OSMO_ASSERT((sc->cfg & 0xff00) == 0)
+		msgb_put_u8(msg, (uint8_t) sc->cfg & 0xff);
+		break;
+	default:
+		OSMO_ASSERT(sc->cfg == 0);
+		break;
+	}
 
 	return (uint8_t) (msg->tail - old_tail);
 }
@@ -244,21 +287,42 @@
 
 	if ((header & 0x0F) != 0x0F) {
 		sc->type = (header & 0x0F);
-		return (int)(elem - old_elem);
+	} else {
+		sc->type = *elem;
+		elem++;
+		len--;
 	}
 
-	sc->type = *elem;
-	elem++;
-	len--;
-
-	sc->type_extended = true;
-
-	if (len < 2)
-		return (int)(elem - old_elem);
-
-	sc->cfg = osmo_load16be(elem);
-	elem += 2;
-	sc->cfg_present = true;
+	/* Note: Whether a configuration is present or not depends on the
+	 * selected codec type. If present, it can either consist of one or
+	 * two octets depending on the codec type */
+	switch (sc->type) {
+	case GSM0808_SCT_FR1:
+	case GSM0808_SCT_FR2:
+	case GSM0808_SCT_HR1:
+		break;
+	case GSM0808_SCT_HR4:
+	case GSM0808_SCT_CSD:
+	case GSM0808_SCT_FR4:
+	case GSM0808_SCT_FR5:
+		if(len < 1)
+			return -EINVAL;
+		sc->cfg = *elem;
+		elem++;
+		break;
+	case GSM0808_SCT_FR3:
+	case GSM0808_SCT_HR3:
+	case GSM0808_SCT_HR6:
+		if(len < 2)
+			return -EINVAL;
+		sc->cfg = osmo_load16be(elem);
+		elem += 2;
+		break;
+	default:
+		/* Invalid codec type => malformed speech codec element! */
+		return -EINVAL;
+		break;
+	}
 
 	return (int)(elem - old_elem);
 }
diff --git a/tests/gsm0808/gsm0808_test.c b/tests/gsm0808/gsm0808_test.c
index 8304052..68771a4 100644
--- a/tests/gsm0808/gsm0808_test.c
+++ b/tests/gsm0808/gsm0808_test.c
@@ -49,19 +49,17 @@
 
 	scl->codec[0].pi = true;
 	scl->codec[0].tf = true;
-	scl->codec[0].type = 0xab;
-	scl->codec[0].type_extended  = true;
-	scl->codec[0].cfg_present = true;
+	scl->codec[0].type = GSM0808_SCT_FR3;
 	scl->codec[0].cfg = 0xcdef;
 
 	scl->codec[1].fi = true;
 	scl->codec[1].pt = true;
-	scl->codec[1].type = 0x05;
+	scl->codec[1].type = GSM0808_SCT_FR2;
 
 	scl->codec[2].fi = true;
 	scl->codec[2].tf = true;
-	scl->codec[2].type = 0xf2;
-	scl->codec[2].type_extended = true;
+	scl->codec[2].type = GSM0808_SCT_CSD;
+	scl->codec[2].cfg = 0xc0;
 
 	scl->len = 3;
 }
@@ -89,8 +87,9 @@
 	static const uint8_t res[] = {
 		0x00, 0x17, 0x57, 0x05, 0x08, 0x00, 0x77, 0x62,
 		0x83, 0x33, 0x66, 0x44, 0x88, 0x17, 0x01, 0x23,
-		    GSM0808_IE_SPEECH_CODEC_LIST, 0x07, 0x5f, 0xab, 0xcd, 0xef,
-		    0xa5, 0x9f, 0xf2
+		GSM0808_IE_SPEECH_CODEC_LIST, 0x07, GSM0808_SCT_FR3 | 0x50,
+		0xcd, 0xef, GSM0808_SCT_FR2 | 0xa0, 0x9f,
+		GSM0808_SCT_CSD | 0x90, 0xc0
 	};
 
 	struct msgb *msg, *in_msg;
@@ -282,9 +281,10 @@
 	static const uint8_t res2[] =
 	    { 0x00, 0x20, 0x01, 0x0b, 0x04, 0x01, 0x0b, 0xa1, 0x25, 0x01, 0x00,
 	      0x04, GSM0808_IE_AOIP_TRASP_ADDR, 0x06, 0xc0, 0xa8, 0x64, 0x17,
-	      0x04, 0xd2, GSM0808_IE_SPEECH_CODEC_LIST, 0x07, 0x5f, 0xab, 0xcd,
-	      0xef, 0xa5, 0x9f, 0xf2, GSM0808_IE_CALL_ID, 0xaa, 0xbb, 0xcc,
-	      0xdd };
+	      0x04, 0xd2, GSM0808_IE_SPEECH_CODEC_LIST, 0x07,
+	      GSM0808_SCT_FR3 | 0x50, 0xcd, 0xef, GSM0808_SCT_FR2 | 0xa0, 0x9f,
+	      GSM0808_SCT_CSD | 0x90, 0xc0, GSM0808_IE_CALL_ID, 0xaa, 0xbb,
+	      0xcc, 0xdd };
 
 	struct msgb *msg;
 	struct gsm0808_channel_type ct;
@@ -351,9 +351,9 @@
 	static const uint8_t res[] =
 	    { 0x00, 0x1d, 0x02, 0x15, 0x23, 0x21, 0x42, 0x2c, 0x11, 0x40, 0x22,
 	      GSM0808_IE_AOIP_TRASP_ADDR, 0x06, 0xc0, 0xa8, 0x64, 0x17, 0x04,
-	      0xd2, GSM0808_IE_SPEECH_CODEC, 0x01, 0x9a,
-	      GSM0808_IE_SPEECH_CODEC_LIST, 0x07, 0x5f, 0xab, 0xcd, 0xef, 0xa5,
-	      0x9f, 0xf2 };
+	      0xd2, GSM0808_IE_SPEECH_CODEC, 0x01, GSM0808_SCT_HR1 | 0x90,
+	      GSM0808_IE_SPEECH_CODEC_LIST, 0x07, GSM0808_SCT_FR3 | 0x50, 0xcd,
+	      0xef, GSM0808_SCT_FR2 | 0xa0, 0x9f, GSM0808_SCT_CSD | 0x90, 0xc0 };
 	struct msgb *msg;
 
 	memset(&sin, 0, sizeof(sin));
@@ -367,7 +367,7 @@
 	memset(&sc, 0, sizeof(sc));
 	sc.fi = true;
 	sc.tf = true;
-	sc.type = 0x0a;
+	sc.type = GSM0808_SCT_HR1;
 
 	setup_codec_list(&sc_list);
 
@@ -400,11 +400,12 @@
 {
 	static const uint8_t res1[] =
 	    { 0x00, 0x0d, 0x03, 0x04, 0x01, 0x23, GSM0808_IE_SPEECH_CODEC_LIST,
-		0x07, 0x5f, 0xab, 0xcd, 0xef, 0xa5, 0x9f, 0xf2 };
+	      0x07, GSM0808_SCT_FR3 | 0x50, 0xcd, 0xef, GSM0808_SCT_FR2 | 0xa0,
+	      0x9f, GSM0808_SCT_CSD | 0x90, 0xc0 };
 	static const uint8_t res2[] =
 	    { 0x00, 0x0f, 0x03, 0x04, 0x01, 0x23, 0x15, 0x02,
-		GSM0808_IE_SPEECH_CODEC_LIST, 0x07, 0x5f, 0xab,
-		0xcd, 0xef, 0xa5, 0x9f, 0xf2 };
+	      GSM0808_IE_SPEECH_CODEC_LIST, 0x07, GSM0808_SCT_FR3 | 0x50, 0xcd,
+	      0xef, GSM0808_SCT_FR2 | 0xa0, 0x9f, GSM0808_SCT_CSD | 0x90, 0xc0 };
 	uint8_t rr_res = 2;
 	struct msgb *msg;
 	struct gsm0808_speech_codec_list sc_list;
@@ -574,7 +575,7 @@
 	memset(&enc_sc, 0, sizeof(enc_sc));
 	enc_sc.fi = true;
 	enc_sc.pt = true;
-	enc_sc.type = 0x05;
+	enc_sc.type = GSM0808_SCT_FR2;
 
 	msg = msgb_alloc(1024, "output buffer");
 	rc_enc = gsm0808_enc_speech_codec(msg, &enc_sc);
@@ -589,6 +590,31 @@
 }
 
 
+static void test_gsm0808_enc_dec_speech_codec_with_cfg()
+{
+	struct gsm0808_speech_codec enc_sc;
+	struct gsm0808_speech_codec dec_sc;
+	struct msgb *msg;
+	uint8_t rc_enc;
+	int rc_dec;
+
+	enc_sc.pi = true;
+	enc_sc.tf = true;
+	enc_sc.type = GSM0808_SCT_FR3;
+	enc_sc.cfg = 0xabcd;
+
+	msg = msgb_alloc(1024, "output buffer");
+	rc_enc = gsm0808_enc_speech_codec(msg, &enc_sc);
+	OSMO_ASSERT(rc_enc == 5);
+
+	rc_dec = gsm0808_dec_speech_codec(&dec_sc, msg->data + 2, msg->len - 2);
+	OSMO_ASSERT(rc_dec == 3);
+
+	OSMO_ASSERT(memcmp(&enc_sc, &dec_sc, sizeof(enc_sc)) == 0);
+
+	msgb_free(msg);
+}
+
 static void test_gsm0808_enc_dec_speech_codec_ext_with_cfg()
 {
 	struct gsm0808_speech_codec enc_sc;
@@ -599,44 +625,15 @@
 
 	enc_sc.pi = true;
 	enc_sc.tf = true;
-	enc_sc.type = 0xab;
-	enc_sc.type_extended = true;
-	enc_sc.cfg_present = true;
-	enc_sc.cfg = 0xcdef;
+	enc_sc.type = GSM0808_SCT_CSD;
+	enc_sc.cfg = 0xc0;
 
 	msg = msgb_alloc(1024, "output buffer");
 	rc_enc = gsm0808_enc_speech_codec(msg, &enc_sc);
-	OSMO_ASSERT(rc_enc == 6);
+	OSMO_ASSERT(rc_enc == 5);
 
 	rc_dec = gsm0808_dec_speech_codec(&dec_sc, msg->data + 2, msg->len - 2);
-	OSMO_ASSERT(rc_dec == 4);
-
-	OSMO_ASSERT(memcmp(&enc_sc, &dec_sc, sizeof(enc_sc)) == 0);
-
-	msgb_free(msg);
-}
-
-static void test_gsm0808_enc_dec_speech_codec_ext()
-{
-	struct gsm0808_speech_codec enc_sc;
-	struct gsm0808_speech_codec dec_sc;
-	struct msgb *msg;
-	uint8_t rc_enc;
-	int rc_dec;
-
-	enc_sc.fi = true;
-	enc_sc.tf = true;
-	enc_sc.type = 0xf2;
-	enc_sc.type_extended = true;
-	enc_sc.cfg_present = false;
-	enc_sc.cfg = 0x0000;
-
-	msg = msgb_alloc(1024, "output buffer");
-	rc_enc = gsm0808_enc_speech_codec(msg, &enc_sc);
-	OSMO_ASSERT(rc_enc == 4);
-
-	rc_dec = gsm0808_dec_speech_codec(&dec_sc, msg->data + 2, msg->len - 2);
-	OSMO_ASSERT(rc_dec == 2);
+	OSMO_ASSERT(rc_dec == 3);
 
 	OSMO_ASSERT(memcmp(&enc_sc, &dec_sc, sizeof(enc_sc)) == 0);
 
@@ -655,19 +652,17 @@
 
 	enc_scl.codec[0].pi = true;
 	enc_scl.codec[0].tf = true;
-	enc_scl.codec[0].type = 0xab;
-	enc_scl.codec[0].type_extended  = true;
-	enc_scl.codec[0].cfg_present = true;
+	enc_scl.codec[0].type = GSM0808_SCT_FR3;
 	enc_scl.codec[0].cfg = 0xcdef;
 
 	enc_scl.codec[1].fi = true;
 	enc_scl.codec[1].pt = true;
-	enc_scl.codec[1].type = 0x05;
+	enc_scl.codec[1].type = GSM0808_SCT_FR2;
 
 	enc_scl.codec[2].fi = true;
 	enc_scl.codec[2].tf = true;
-	enc_scl.codec[2].type = 0xf2;
-	enc_scl.codec[2].type_extended = true;
+	enc_scl.codec[2].type = GSM0808_SCT_CSD;
+	enc_scl.codec[2].cfg = 0xc0;
 
 	enc_scl.len = 3;
 
@@ -860,8 +855,8 @@
 	test_enc_dec_aoip_trasp_addr_v4();
 	test_enc_dec_aoip_trasp_addr_v6();
 	test_gsm0808_enc_dec_speech_codec();
-	test_gsm0808_enc_dec_speech_codec_ext();
 	test_gsm0808_enc_dec_speech_codec_ext_with_cfg();
+	test_gsm0808_enc_dec_speech_codec_with_cfg();
 	test_gsm0808_enc_dec_speech_codec_list();
 	test_gsm0808_enc_dec_channel_type();
 	test_gsm0808_enc_dec_encrypt_info();

-- 
To view, visit https://gerrit.osmocom.org/2820
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idabb0f9620659557672e1c6b90c75481192e5c89
Gerrit-PatchSet: 5
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>



More information about the gerrit-log mailing list