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 <pma...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de>