[PATCH 1/9] Add function to update TRAU muxer after assignment or handover
From: Andreas Eversberg <andreas@eversberg.eu> E1 based BTS use TRAU muxer to decode TRAU frames. After changing channel from one timeslot to another (due to handover or assignment), the TRAU muxer must be updated. The call reference of the call is disconnected from the old channel and connected to the new channel. --- openbsc/include/openbsc/gsm_data.h | 14 ++++++++++++++ openbsc/include/openbsc/trau_mux.h | 3 +++ openbsc/src/libbsc/bsc_api.c | 5 +++++ openbsc/src/libbsc/handover_logic.c | 7 +++++-- openbsc/src/libmsc/gsm_04_08.c | 12 +++++++++--- openbsc/src/libtrau/trau_mux.c | 18 ++++++++++++++++++ 6 files changed, 54 insertions(+), 5 deletions(-) diff --git a/openbsc/include/openbsc/gsm_data.h b/openbsc/include/openbsc/gsm_data.h index 7c3ca84..41fe328 100644 --- a/openbsc/include/openbsc/gsm_data.h +++ b/openbsc/include/openbsc/gsm_data.h @@ -382,6 +382,20 @@ static inline int is_nokia_bts(struct gsm_bts *bts) return 0; } +static inline int is_e1_bts(struct gsm_bts *bts) +{ + switch (bts->type) { + case GSM_BTS_TYPE_BS11: + case GSM_BTS_TYPE_RBS2000: + case GSM_BTS_TYPE_NOKIA_SITE: + return 1; + default: + break; + } + + return 0; +} + enum gsm_auth_policy gsm_auth_policy_parse(const char *arg); const char *gsm_auth_policy_name(enum gsm_auth_policy policy); diff --git a/openbsc/include/openbsc/trau_mux.h b/openbsc/include/openbsc/trau_mux.h index 3de50f7..d211d8d 100644 --- a/openbsc/include/openbsc/trau_mux.h +++ b/openbsc/include/openbsc/trau_mux.h @@ -51,6 +51,9 @@ int trau_recv_lchan(struct gsm_lchan *lchan, uint32_t callref); /* send trau from application */ int trau_send_frame(struct gsm_lchan *lchan, struct gsm_data_frame *frame); +/* switch trau muxer to new lchan */ +int switch_trau_mux(struct gsm_lchan *old_lchan, struct gsm_lchan *new_lchan); + /* callback invoked if we receive TRAU frames */ int subch_cb(struct subch_demux *dmx, int ch, uint8_t *data, int len, void *_priv); diff --git a/openbsc/src/libbsc/bsc_api.c b/openbsc/src/libbsc/bsc_api.c index 86d2493..e567038 100644 --- a/openbsc/src/libbsc/bsc_api.c +++ b/openbsc/src/libbsc/bsc_api.c @@ -31,6 +31,7 @@ #include <openbsc/handover.h> #include <openbsc/debug.h> #include <openbsc/gsm_04_08.h> +#include <openbsc/trau_mux.h> #include <osmocom/gsm/protocol/gsm_08_08.h> @@ -419,6 +420,10 @@ static void handle_ass_compl(struct gsm_subscriber_connection *conn, return; } + /* switch TRAU muxer for E1 based BTS from one channel to another */ + if (is_e1_bts(conn->bts)) + switch_trau_mux(conn->lchan, conn->secondary_lchan); + /* swap channels */ osmo_timer_del(&conn->T10); diff --git a/openbsc/src/libbsc/handover_logic.c b/openbsc/src/libbsc/handover_logic.c index 9cf26af..36a758b 100644 --- a/openbsc/src/libbsc/handover_logic.c +++ b/openbsc/src/libbsc/handover_logic.c @@ -39,6 +39,7 @@ #include <openbsc/signal.h> #include <osmocom/core/talloc.h> #include <openbsc/transaction.h> +#include <openbsc/trau_mux.h> struct bsc_handover { struct llist_head list; @@ -264,6 +265,10 @@ static int ho_gsm48_ho_compl(struct gsm_lchan *new_lchan) osmo_timer_del(&ho->T3103); + /* switch TRAU muxer for E1 based BTS from one channel to another */ + if (is_e1_bts(new_lchan->conn->bts)) + switch_trau_mux(ho->old_lchan, new_lchan); + /* Replace the ho lchan with the primary one */ if (ho->old_lchan != new_lchan->conn->lchan) LOGP(DHO, LOGL_ERROR, "Primary lchan changed during handover.\n"); @@ -278,8 +283,6 @@ static int ho_gsm48_ho_compl(struct gsm_lchan *new_lchan) rsl_lchan_set_state(ho->old_lchan, LCHAN_S_INACTIVE); lchan_release(ho->old_lchan, 0, RSL_REL_LOCAL_END); - /* do something to re-route the actual speech frames ! */ - llist_del(&ho->list); talloc_free(ho); diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index 0c6b100..dbb30ec 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -1648,6 +1648,9 @@ static int tch_recv_mncc(struct gsm_network *net, uint32_t callref, int enable) lchan = trans->conn->lchan; bts = lchan->ts->trx->bts; + /* store receive state */ + trans->tch_recv = enable; + switch (bts->type) { case GSM_BTS_TYPE_NANOBTS: case GSM_BTS_TYPE_OSMO_SYSMO: @@ -1655,10 +1658,8 @@ static int tch_recv_mncc(struct gsm_network *net, uint32_t callref, int enable) LOGP(DCC, LOGL_ERROR, "Error: RTP proxy is disabled\n"); return -EINVAL; } - /* in case, we don't have a RTP socket yet, we note this - * in the transaction and try later */ + /* in case, we don't have a RTP socket yet, we try later */ if (!lchan->abis_ip.rtp_socket) { - trans->tch_recv = enable; DEBUGP(DCC, "queue tch_recv_mncc request (%d)\n", enable); return 0; } @@ -1677,6 +1678,11 @@ static int tch_recv_mncc(struct gsm_network *net, uint32_t callref, int enable) case GSM_BTS_TYPE_BS11: case GSM_BTS_TYPE_RBS2000: case GSM_BTS_TYPE_NOKIA_SITE: + /* in case we don't have a TCH with correct mode, we try later */ + if (lchan->tch_mode == GSM48_CMODE_SIGN) { + DEBUGP(DCC, "queue tch_recv_mncc request (%d)\n", enable); + return 0; + } if (enable) return trau_recv_lchan(lchan, callref); return trau_mux_unmap(NULL, callref); diff --git a/openbsc/src/libtrau/trau_mux.c b/openbsc/src/libtrau/trau_mux.c index c9d77cf..7b9bac0 100644 --- a/openbsc/src/libtrau/trau_mux.c +++ b/openbsc/src/libtrau/trau_mux.c @@ -31,6 +31,7 @@ #include <osmocom/core/talloc.h> #include <openbsc/trau_upqueue.h> #include <osmocom/core/crcgen.h> +#include <openbsc/transaction.h> /* this corresponds to the bit-lengths of the individual codec * parameters as indicated in Table 1.1 of TS 06.10 */ @@ -518,3 +519,20 @@ int trau_send_frame(struct gsm_lchan *lchan, struct gsm_data_frame *frame) return subchan_mux_enqueue(mx, dst_e1_ss->e1_ts_ss, trau_bits_out, TRAU_FRAME_BITS); } + +/* switch trau muxer to new lchan */ +int switch_trau_mux(struct gsm_lchan *old_lchan, struct gsm_lchan *new_lchan) +{ + struct gsm_network *net = old_lchan->ts->trx->bts->network; + struct gsm_trans *trans; + + /* look up transaction with TCH frame receive enabled */ + llist_for_each_entry(trans, &net->trans_list, entry) { + if (trans->conn && trans->conn->lchan == old_lchan && trans->tch_recv) { + /* switch */ + trau_recv_lchan(new_lchan, trans->callref); + } + } + + return 0; +} -- 1.8.1.5
--- openbsc/include/openbsc/rtp_proxy.h | 4 ++++ openbsc/src/libtrau/rtp_proxy.c | 20 +++++++++++++------- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/openbsc/include/openbsc/rtp_proxy.h b/openbsc/include/openbsc/rtp_proxy.h index 26cac0d..52ffefd 100644 --- a/openbsc/include/openbsc/rtp_proxy.h +++ b/openbsc/include/openbsc/rtp_proxy.h @@ -33,6 +33,10 @@ #define RTP_PT_GSM_HALF 96 #define RTP_PT_GSM_EFR 97 #define RTP_PT_AMR 98 +#define RTP_LEN_GSM_FULL 33 +#define RTP_LEN_GSM_HALF 15 +#define RTP_LEN_GSM_EFR 31 +#define RTP_GSM_DURATION 160 enum rtp_rx_action { RTP_NONE, diff --git a/openbsc/src/libtrau/rtp_proxy.c b/openbsc/src/libtrau/rtp_proxy.c index 4278fc6..94a5b2f 100644 --- a/openbsc/src/libtrau/rtp_proxy.c +++ b/openbsc/src/libtrau/rtp_proxy.c @@ -167,15 +167,21 @@ static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data) switch (rtph->payload_type) { case RTP_PT_GSM_FULL: msg_type = GSM_TCHF_FRAME; - if (payload_len != 33) { + if (payload_len != RTP_LEN_GSM_FULL) { DEBUGPC(DLMUX, "received RTP full rate frame with " - "payload length != 32 (len = %d)\n", - payload_len); + "payload length != %d (len = %d)\n", + RTP_LEN_GSM_FULL, payload_len); return -EINVAL; } break; case RTP_PT_GSM_EFR: msg_type = GSM_TCHF_FRAME_EFR; + if (payload_len != RTP_LEN_GSM_EFR) { + DEBUGPC(DLMUX, "received RTP extended full rate frame " + "with payload length != %d (len = %d)\n", + RTP_LEN_GSM_EFR, payload_len); + return -EINVAL; + } break; default: DEBUGPC(DLMUX, "received RTP frame with unknown payload " @@ -236,13 +242,13 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) switch (frame->msg_type) { case GSM_TCHF_FRAME: payload_type = RTP_PT_GSM_FULL; - payload_len = 33; - duration = 160; + payload_len = RTP_LEN_GSM_FULL; + duration = RTP_GSM_DURATION; break; case GSM_TCHF_FRAME_EFR: payload_type = RTP_PT_GSM_EFR; - payload_len = 31; - duration = 160; + payload_len = RTP_LEN_GSM_EFR; + duration = RTP_GSM_DURATION; break; default: DEBUGPC(DLMUX, "unsupported message type %d\n", -- 1.8.1.5
On Wed, Jan 22, 2014 at 10:05:51AM +0100, Andreas Eversberg wrote:
+ if (payload_len != RTP_LEN_GSM_EFR) { + DEBUGPC(DLMUX, "received RTP extended full rate frame " + "with payload length != %d (len = %d)\n", + RTP_LEN_GSM_EFR, payload_len); + return -EINVAL; + }
this is not mentioned in the commit message. I am applying it anyway.
--- openbsc/include/openbsc/mncc.h | 1 + openbsc/src/libmsc/gsm_04_08.c | 6 ++++-- openbsc/src/libmsc/mncc_builtin.c | 2 ++ openbsc/src/libmsc/mncc_sock.c | 5 +++-- openbsc/src/libtrau/rtp_proxy.c | 14 ++++++++++++++ 5 files changed, 24 insertions(+), 4 deletions(-) diff --git a/openbsc/include/openbsc/mncc.h b/openbsc/include/openbsc/mncc.h index ffc247b..ec4dac9 100644 --- a/openbsc/include/openbsc/mncc.h +++ b/openbsc/include/openbsc/mncc.h @@ -95,6 +95,7 @@ struct gsm_call { #define GSM_TCHF_FRAME 0x0300 #define GSM_TCHF_FRAME_EFR 0x0301 +#define GSM_TCHH_FRAME 0x0302 #define GSM_TCHF_BAD_FRAME 0x03ff #define MNCC_SOCKET_HELLO 0x0400 diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index dbb30ec..46ad719 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -2945,6 +2945,7 @@ int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg) return tch_recv_mncc(net, data->callref, 1); case GSM_TCHF_FRAME: case GSM_TCHF_FRAME_EFR: + case GSM_TCHH_FRAME: /* Find callref */ trans = trans_find_by_callref(net, data->callref); if (!trans) { @@ -2956,11 +2957,12 @@ int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg) LOGP(DMNCC, LOGL_NOTICE, "TCH frame for trans without conn\n"); return 0; } - if (trans->conn->lchan->type != GSM_LCHAN_TCH_F) { + if (trans->conn->lchan->type != GSM_LCHAN_TCH_F + && trans->conn->lchan->type != GSM_LCHAN_TCH_H) { /* This should be LOGL_ERROR or NOTICE, but * unfortuantely it happens for a couple of frames at * the beginning of every RTP connection */ - LOGP(DMNCC, LOGL_DEBUG, "TCH frame for lchan != TCH_F\n"); + LOGP(DMNCC, LOGL_DEBUG, "TCH frame for lchan != TCH_F/TCH_H\n"); return 0; } bts = trans->conn->lchan->ts->trx->bts; diff --git a/openbsc/src/libmsc/mncc_builtin.c b/openbsc/src/libmsc/mncc_builtin.c index be35454..506d004 100644 --- a/openbsc/src/libmsc/mncc_builtin.c +++ b/openbsc/src/libmsc/mncc_builtin.c @@ -342,6 +342,7 @@ int int_mncc_recv(struct gsm_network *net, struct msgb *msg) switch (msg_type) { case GSM_TCHF_FRAME: case GSM_TCHF_FRAME_EFR: + case GSM_TCHH_FRAME: break; default: DEBUGP(DMNCC, "(call %x) Received message %s\n", call->callref, @@ -410,6 +411,7 @@ int int_mncc_recv(struct gsm_network *net, struct msgb *msg) break; case GSM_TCHF_FRAME: case GSM_TCHF_FRAME_EFR: + case GSM_TCHH_FRAME: rc = mncc_rcv_tchf(call, msg_type, arg); break; default: diff --git a/openbsc/src/libmsc/mncc_sock.c b/openbsc/src/libmsc/mncc_sock.c index cf4bca8..8d8e505 100644 --- a/openbsc/src/libmsc/mncc_sock.c +++ b/openbsc/src/libmsc/mncc_sock.c @@ -54,8 +54,9 @@ int mncc_sock_from_cc(struct gsm_network *net, struct msgb *msg) if (net->mncc_state->conn_bfd.fd < 0) { LOGP(DMNCC, LOGL_ERROR, "mncc_sock receives %s for external CC app " "but socket is gone\n", get_mncc_name(msg_type)); - if (msg_type != GSM_TCHF_FRAME && - msg_type != GSM_TCHF_FRAME_EFR) { + if (msg_type != GSM_TCHF_FRAME + && msg_type != GSM_TCHF_FRAME_EFR + && msg_type != GSM_TCHH_FRAME) { /* release the request */ struct gsm_mncc mncc_out; memset(&mncc_out, 0, sizeof(mncc_out)); diff --git a/openbsc/src/libtrau/rtp_proxy.c b/openbsc/src/libtrau/rtp_proxy.c index 94a5b2f..2587e12 100644 --- a/openbsc/src/libtrau/rtp_proxy.c +++ b/openbsc/src/libtrau/rtp_proxy.c @@ -183,6 +183,15 @@ static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data) return -EINVAL; } break; + case RTP_PT_GSM_HALF: + msg_type = GSM_TCHH_FRAME; + if (payload_len != RTP_LEN_GSM_HALF) { + DEBUGPC(DLMUX, "received RTP half rate frame with " + "payload length != 15 (len = %d)\n", + RTP_LEN_GSM_HALF, payload_len); + return -EINVAL; + } + break; default: DEBUGPC(DLMUX, "received RTP frame with unknown payload " "type %d\n", rtph->payload_type); @@ -250,6 +259,11 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) payload_len = RTP_LEN_GSM_EFR; duration = RTP_GSM_DURATION; break; + case GSM_TCHH_FRAME: + payload_type = RTP_PT_GSM_HALF; + payload_len = RTP_LEN_GSM_HALF; + duration = RTP_GSM_DURATION; + break; default: DEBUGPC(DLMUX, "unsupported message type %d\n", frame->msg_type); -- 1.8.1.5
AMR rate is currently fixed to 5.9k. --- openbsc/include/openbsc/mncc.h | 1 + openbsc/src/libmsc/gsm_04_08.c | 1 + openbsc/src/libmsc/mncc.c | 5 ++++- openbsc/src/libmsc/mncc_builtin.c | 2 ++ openbsc/src/libmsc/mncc_sock.c | 3 ++- openbsc/src/libtrau/rtp_proxy.c | 21 +++++++++++++++++---- 6 files changed, 27 insertions(+), 6 deletions(-) diff --git a/openbsc/include/openbsc/mncc.h b/openbsc/include/openbsc/mncc.h index ec4dac9..45ad83e 100644 --- a/openbsc/include/openbsc/mncc.h +++ b/openbsc/include/openbsc/mncc.h @@ -96,6 +96,7 @@ struct gsm_call { #define GSM_TCHF_FRAME 0x0300 #define GSM_TCHF_FRAME_EFR 0x0301 #define GSM_TCHH_FRAME 0x0302 +#define GSM_TCH_FRAME_AMR 0x0303 #define GSM_TCHF_BAD_FRAME 0x03ff #define MNCC_SOCKET_HELLO 0x0400 diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index 46ad719..e36a142 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -2946,6 +2946,7 @@ int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg) case GSM_TCHF_FRAME: case GSM_TCHF_FRAME_EFR: case GSM_TCHH_FRAME: + case GSM_TCH_FRAME_AMR: /* Find callref */ trans = trans_find_by_callref(net, data->callref); if (!trans) { diff --git a/openbsc/src/libmsc/mncc.c b/openbsc/src/libmsc/mncc.c index b484772..01e9c67 100644 --- a/openbsc/src/libmsc/mncc.c +++ b/openbsc/src/libmsc/mncc.c @@ -84,7 +84,10 @@ static struct mncc_names { {"MNCC_FRAME_DROP", 0x0202}, {"MNCC_LCHAN_MODIFY", 0x0203}, - {"GSM_TCH_FRAME", 0x0300}, + {"GSM_TCHF_FRAME", 0x0300}, + {"GSM_TCHF_FRAME_EFR", 0x0301}, + {"GSM_TCHH_FRAME", 0x0302}, + {"GSM_TCH_FRAME_AMR", 0x0303}, {NULL, 0} }; diff --git a/openbsc/src/libmsc/mncc_builtin.c b/openbsc/src/libmsc/mncc_builtin.c index 506d004..5d71983 100644 --- a/openbsc/src/libmsc/mncc_builtin.c +++ b/openbsc/src/libmsc/mncc_builtin.c @@ -343,6 +343,7 @@ int int_mncc_recv(struct gsm_network *net, struct msgb *msg) case GSM_TCHF_FRAME: case GSM_TCHF_FRAME_EFR: case GSM_TCHH_FRAME: + case GSM_TCH_FRAME_AMR: break; default: DEBUGP(DMNCC, "(call %x) Received message %s\n", call->callref, @@ -412,6 +413,7 @@ int int_mncc_recv(struct gsm_network *net, struct msgb *msg) case GSM_TCHF_FRAME: case GSM_TCHF_FRAME_EFR: case GSM_TCHH_FRAME: + case GSM_TCH_FRAME_AMR: rc = mncc_rcv_tchf(call, msg_type, arg); break; default: diff --git a/openbsc/src/libmsc/mncc_sock.c b/openbsc/src/libmsc/mncc_sock.c index 8d8e505..4a880c7 100644 --- a/openbsc/src/libmsc/mncc_sock.c +++ b/openbsc/src/libmsc/mncc_sock.c @@ -56,7 +56,8 @@ int mncc_sock_from_cc(struct gsm_network *net, struct msgb *msg) "but socket is gone\n", get_mncc_name(msg_type)); if (msg_type != GSM_TCHF_FRAME && msg_type != GSM_TCHF_FRAME_EFR - && msg_type != GSM_TCHH_FRAME) { + && msg_type != GSM_TCHH_FRAME + && msg_type != GSM_TCH_FRAME_AMR) { /* release the request */ struct gsm_mncc mncc_out; memset(&mncc_out, 0, sizeof(mncc_out)); diff --git a/openbsc/src/libtrau/rtp_proxy.c b/openbsc/src/libtrau/rtp_proxy.c index 2587e12..59efa21 100644 --- a/openbsc/src/libtrau/rtp_proxy.c +++ b/openbsc/src/libtrau/rtp_proxy.c @@ -116,6 +116,7 @@ static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data) int payload_len; int msg_type; int x_len; + int amr = 0; if (msg->len < 12) { DEBUGPC(DLMUX, "received RTP frame too short (len = %d)\n", @@ -192,21 +193,26 @@ static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data) return -EINVAL; } break; + case RTP_PT_AMR: + amr = 1; + break; default: DEBUGPC(DLMUX, "received RTP frame with unknown payload " "type %d\n", rtph->payload_type); return -EINVAL; } - new_msg = msgb_alloc(sizeof(struct gsm_data_frame) + payload_len, + new_msg = msgb_alloc(sizeof(struct gsm_data_frame) + payload_len + amr, "GSM-DATA"); if (!new_msg) return -ENOMEM; frame = (struct gsm_data_frame *)(new_msg->data); frame->msg_type = msg_type; frame->callref = callref; - memcpy(frame->data, payload, payload_len); - msgb_put(new_msg, sizeof(struct gsm_data_frame) + payload_len); + if (amr) + frame->data[0] = payload_len; + memcpy(frame->data + amr, payload, payload_len); + msgb_put(new_msg, sizeof(struct gsm_data_frame) + amr + payload_len); *data = new_msg; return 0; @@ -239,6 +245,7 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) int payload_type; int payload_len; int duration; /* in samples */ + int amr = 0; if (rs->tx_action != RTP_SEND_DOWNSTREAM) { /* initialize sequences */ @@ -264,6 +271,12 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) payload_len = RTP_LEN_GSM_HALF; duration = RTP_GSM_DURATION; break; + case GSM_TCH_FRAME_AMR: + payload_type = RTP_PT_AMR; + payload_len = frame->data[0]; + duration = RTP_GSM_DURATION; + amr = 1; + break; default: DEBUGPC(DLMUX, "unsupported message type %d\n", frame->msg_type); @@ -305,7 +318,7 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) rtph->timestamp = htonl(rs->transmit.timestamp); rs->transmit.timestamp += duration; rtph->ssrc = htonl(rs->transmit.ssrc); - memcpy(msg->data + sizeof(struct rtp_hdr), frame->data, payload_len); + memcpy(msg->data + sizeof(struct rtp_hdr), frame->data + amr, payload_len); msgb_put(msg, sizeof(struct rtp_hdr) + payload_len); msgb_enqueue(&rss->tx_queue, msg); rss->bfd.when |= BSC_FD_WRITE; -- 1.8.1.5
If a bad TRAU frame is received, it is forwarded to MNCC application as GSM_BAD_FRAME. The application can now handle the GAP of missing audio. (e.g. by extrapolation) If TRAU frames are forwarded via RTP, bad frames are dropped, but frame counter and timestamp of RTP sender state is increased. --- openbsc/include/openbsc/mncc.h | 2 +- openbsc/src/libmsc/mncc_sock.c | 3 ++- openbsc/src/libtrau/rtp_proxy.c | 11 +++++++++++ openbsc/src/libtrau/trau_mux.c | 7 +++++-- openbsc/tests/trau/trau_test.c | 2 +- 5 files changed, 20 insertions(+), 5 deletions(-) diff --git a/openbsc/include/openbsc/mncc.h b/openbsc/include/openbsc/mncc.h index 45ad83e..48f4f7d 100644 --- a/openbsc/include/openbsc/mncc.h +++ b/openbsc/include/openbsc/mncc.h @@ -97,7 +97,7 @@ struct gsm_call { #define GSM_TCHF_FRAME_EFR 0x0301 #define GSM_TCHH_FRAME 0x0302 #define GSM_TCH_FRAME_AMR 0x0303 -#define GSM_TCHF_BAD_FRAME 0x03ff +#define GSM_BAD_FRAME 0x03ff #define MNCC_SOCKET_HELLO 0x0400 diff --git a/openbsc/src/libmsc/mncc_sock.c b/openbsc/src/libmsc/mncc_sock.c index 4a880c7..c8cfa6a 100644 --- a/openbsc/src/libmsc/mncc_sock.c +++ b/openbsc/src/libmsc/mncc_sock.c @@ -57,7 +57,8 @@ int mncc_sock_from_cc(struct gsm_network *net, struct msgb *msg) if (msg_type != GSM_TCHF_FRAME && msg_type != GSM_TCHF_FRAME_EFR && msg_type != GSM_TCHH_FRAME - && msg_type != GSM_TCH_FRAME_AMR) { + && msg_type != GSM_TCH_FRAME_AMR + && msg_type != GSM_BAD_FRAME) { /* release the request */ struct gsm_mncc mncc_out; memset(&mncc_out, 0, sizeof(mncc_out)); diff --git a/openbsc/src/libtrau/rtp_proxy.c b/openbsc/src/libtrau/rtp_proxy.c index 59efa21..89ed8a4 100644 --- a/openbsc/src/libtrau/rtp_proxy.c +++ b/openbsc/src/libtrau/rtp_proxy.c @@ -277,6 +277,14 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) duration = RTP_GSM_DURATION; amr = 1; break; + case GSM_BAD_FRAME: + /* in case of a bad frame, just count and drop packt */ + payload_type = 0; + payload_len = 0; + duration = RTP_GSM_DURATION; + rs->transmit.timestamp += duration; + rs->transmit.sequence++; + break; default: DEBUGPC(DLMUX, "unsupported message type %d\n", frame->msg_type); @@ -304,6 +312,9 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) } } + if (frame->msg_type == GSM_BAD_FRAME) + return 0; + msg = msgb_alloc(sizeof(struct rtp_hdr) + payload_len, "RTP-GSM-FULL"); if (!msg) return -ENOMEM; diff --git a/openbsc/src/libtrau/trau_mux.c b/openbsc/src/libtrau/trau_mux.c index 7b9bac0..bb513cc 100644 --- a/openbsc/src/libtrau/trau_mux.c +++ b/openbsc/src/libtrau/trau_mux.c @@ -242,7 +242,10 @@ struct msgb *trau_decode_fr(uint32_t callref, i++; j++; } - frame->msg_type = GSM_TCHF_FRAME; + if (tf->c_bits[11]) /* BFI */ + frame->msg_type = GSM_BAD_FRAME; + else + frame->msg_type = GSM_TCHF_FRAME; frame->callref = callref; msgb_put(msg, sizeof(struct gsm_data_frame) + 33); @@ -314,7 +317,7 @@ struct msgb *trau_decode_efr(uint32_t callref, return msg; bad_frame: - frame->msg_type = GSM_TCHF_BAD_FRAME; + frame->msg_type = GSM_BAD_FRAME; return msg; } diff --git a/openbsc/tests/trau/trau_test.c b/openbsc/tests/trau/trau_test.c index f8a48db..b95f1e8 100644 --- a/openbsc/tests/trau/trau_test.c +++ b/openbsc/tests/trau/trau_test.c @@ -57,7 +57,7 @@ void test_trau_fr_efr(unsigned char *data) msg = trau_decode_efr(1, &tf); OSMO_ASSERT(msg != NULL); frame = (struct gsm_data_frame *)msg->data; - OSMO_ASSERT(frame->msg_type == GSM_TCHF_BAD_FRAME); + OSMO_ASSERT(frame->msg_type == GSM_BAD_FRAME); msgb_free(msg); } -- 1.8.1.5
Instead of forwarding traffic through MNCC interface, traffic can be forwarded to a given RTP peer directly. A special MNCC message is used to control the peer's destination. The traffic can still be forwarded through MNCC interface when this special MNCC message is not used. It also works with E1 based BTSs. In conjunction with LCR's "rtp-bridge" feature, the RTP traffic can be directly exchanged with a remote SIP endpoint, so that the traffic is not forwarded by LCR itself. This way the performance of handling traffic only depends on OpenBSC and the remote SIP endpoint. Also the traffic is exchanged with the SIP endpoint without transcoding, to have maximum performance. --- openbsc/include/openbsc/gsm_04_08.h | 3 + openbsc/include/openbsc/mncc.h | 10 ++ openbsc/include/openbsc/rtp_proxy.h | 5 +- openbsc/include/openbsc/transaction.h | 2 + openbsc/src/ipaccess/ipaccess-config.c | 6 ++ openbsc/src/libbsc/bsc_api.c | 1 + openbsc/src/libbsc/handover_logic.c | 1 + openbsc/src/libmsc/gsm_04_08.c | 176 ++++++++++++++++++++++++++------- openbsc/src/libmsc/mncc_sock.c | 18 ++++ openbsc/src/libmsc/transaction.c | 1 + openbsc/src/libtrau/rtp_proxy.c | 20 +++- openbsc/src/libtrau/trau_mux.c | 14 ++- openbsc/src/utils/bs11_config.c | 6 ++ openbsc/tests/abis/abis_test.c | 6 ++ openbsc/tests/gbproxy/gbproxy_test.c | 6 ++ 15 files changed, 233 insertions(+), 42 deletions(-) diff --git a/openbsc/include/openbsc/gsm_04_08.h b/openbsc/include/openbsc/gsm_04_08.h index 8df7b73..93348d1 100644 --- a/openbsc/include/openbsc/gsm_04_08.h +++ b/openbsc/include/openbsc/gsm_04_08.h @@ -6,6 +6,7 @@ #include <osmocom/gsm/protocol/gsm_04_08.h> #include <openbsc/meas_rep.h> +#include <openbsc/mncc.h> struct msgb; struct gsm_bts; @@ -75,4 +76,6 @@ void gsm48_lchan2chan_desc(struct gsm48_chan_desc *cd, void release_security_operation(struct gsm_subscriber_connection *conn); void allocate_security_operation(struct gsm_subscriber_connection *conn); +int tch_frame_down(struct gsm_network *net, uint32_t callref, struct gsm_data_frame *data); + #endif diff --git a/openbsc/include/openbsc/mncc.h b/openbsc/include/openbsc/mncc.h index 48f4f7d..a380b8f 100644 --- a/openbsc/include/openbsc/mncc.h +++ b/openbsc/include/openbsc/mncc.h @@ -92,6 +92,9 @@ struct gsm_call { #define MNCC_FRAME_RECV 0x0201 #define MNCC_FRAME_DROP 0x0202 #define MNCC_LCHAN_MODIFY 0x0203 +#define MNCC_RTP_CREATE 0x0204 +#define MNCC_RTP_CONNECT 0x0205 +#define MNCC_RTP_FREE 0x0206 #define GSM_TCHF_FRAME 0x0300 #define GSM_TCHF_FRAME_EFR 0x0301 @@ -179,6 +182,13 @@ struct gsm_mncc_hello { uint32_t lchan_type_offset; }; +struct gsm_mncc_rtp { + uint32_t msg_type; + uint32_t callref; + uint32_t ip; + uint16_t port; +}; + char *get_mncc_name(int value); void mncc_set_cause(struct gsm_mncc *data, int loc, int val); void cc_tx_to_mncc(struct gsm_network *net, struct msgb *msg); diff --git a/openbsc/include/openbsc/rtp_proxy.h b/openbsc/include/openbsc/rtp_proxy.h index 52ffefd..2729f63 100644 --- a/openbsc/include/openbsc/rtp_proxy.h +++ b/openbsc/include/openbsc/rtp_proxy.h @@ -40,8 +40,9 @@ enum rtp_rx_action { RTP_NONE, - RTP_PROXY, - RTP_RECV_UPSTREAM, + RTP_PROXY, /* forward from BTS to BTS */ + RTP_RECV_UPSTREAM, /* forward to L4 application */ + RTP_RECV_L4, /* receive RTP frames from L4 application */ }; enum rtp_tx_action { diff --git a/openbsc/include/openbsc/transaction.h b/openbsc/include/openbsc/transaction.h index b6c859c..6f4258d 100644 --- a/openbsc/include/openbsc/transaction.h +++ b/openbsc/include/openbsc/transaction.h @@ -28,6 +28,7 @@ struct gsm_trans { /* reference from MNCC or other application */ uint32_t callref; + uint32_t callref_keep; /* to remember callref, even if it is removed */ /* if traffic channel receive was requested */ int tch_recv; @@ -46,6 +47,7 @@ struct gsm_trans { int T308_second; /* used to send release again */ struct osmo_timer_list timer; struct gsm_mncc msg; /* stores setup/disconnect/release message */ + struct rtp_socket *rs; /* L4 traffic via RTP */ } cc; struct { struct gsm411_smc_inst smc_inst; diff --git a/openbsc/src/ipaccess/ipaccess-config.c b/openbsc/src/ipaccess/ipaccess-config.c index c42c7bb..68b9131 100644 --- a/openbsc/src/ipaccess/ipaccess-config.c +++ b/openbsc/src/ipaccess/ipaccess-config.c @@ -87,6 +87,12 @@ static uint8_t prim_oml_attr[] = { 0x95, 0x00, 7, 0x88, 192, 168, 100, 11, 0x00, static uint8_t unit_id_attr[] = { 0x91, 0x00, 9, '2', '3', '4', '2', '/' , '0', '/', '0', 0x00 }; */ +/* dummy function to keep rtp_proxy.c happy */ +int tch_frame_down(struct gsm_network *net, uint32_t callref, struct gsm_data_frame *data) +{ + return 0; +} + extern int ipaccess_fd_cb(struct osmo_fd *bfd, unsigned int what); extern struct e1inp_line_ops ipaccess_e1inp_line_ops; diff --git a/openbsc/src/libbsc/bsc_api.c b/openbsc/src/libbsc/bsc_api.c index e567038..e6629ff 100644 --- a/openbsc/src/libbsc/bsc_api.c +++ b/openbsc/src/libbsc/bsc_api.c @@ -31,6 +31,7 @@ #include <openbsc/handover.h> #include <openbsc/debug.h> #include <openbsc/gsm_04_08.h> +#include <osmocom/abis/trau_frame.h> #include <openbsc/trau_mux.h> #include <osmocom/gsm/protocol/gsm_08_08.h> diff --git a/openbsc/src/libbsc/handover_logic.c b/openbsc/src/libbsc/handover_logic.c index 36a758b..e20d8f7 100644 --- a/openbsc/src/libbsc/handover_logic.c +++ b/openbsc/src/libbsc/handover_logic.c @@ -39,6 +39,7 @@ #include <openbsc/signal.h> #include <osmocom/core/talloc.h> #include <openbsc/transaction.h> +#include <osmocom/abis/trau_frame.h> #include <openbsc/trau_mux.h> struct bsc_handover { diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index e36a142..7bb9c87 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -1359,8 +1359,15 @@ void _gsm48_cc_trans_free(struct gsm_trans *trans) } if (trans->cc.state != GSM_CSTATE_NULL) new_cc_state(trans, GSM_CSTATE_NULL); + /* Be sure to unmap upstream traffic for our callref only. */ if (trans->conn) - trau_mux_unmap(&trans->conn->lchan->ts->e1_link, trans->callref); + trau_mux_unmap(&trans->conn->lchan->ts->e1_link, trans->callref_keep); + + /* free L4 RTP socket */ + if (trans->cc.rs) { + rtp_socket_free(trans->cc.rs); + trans->cc.rs = NULL; + } } static int gsm48_cc_tx_setup(struct gsm_trans *trans, void *arg); @@ -1478,6 +1485,7 @@ static int switch_for_handover(struct gsm_lchan *old_lchan, new_rs->receive = old_rs->receive; break; case RTP_NONE: + case RTP_RECV_L4: break; } @@ -1695,6 +1703,130 @@ static int tch_recv_mncc(struct gsm_network *net, uint32_t callref, int enable) return 0; } +/* handle RTP requests of L4 */ +static int mncc_rtp(struct gsm_network *net, uint32_t callref, struct gsm_mncc_rtp *mncc) +{ + struct rtp_socket *rs; + struct gsm_trans *trans; + int rc; + + /* Find callref */ + trans = trans_find_by_callref(net, callref); + if (!trans) { + LOGP(DCC, LOGL_ERROR, "Unknown transaction for callref=%d\n", callref); + return -EINVAL; + } + + rs = trans->cc.rs; + + switch (mncc->msg_type) { + case MNCC_RTP_CREATE: + /* use RTP instead of MNCC socket, for traffic + * open L4 RTP socket */ + if (rs) { + LOGP(DCC, LOGL_ERROR, "RTP already created.\n"); + return -EIO; + } + rs = trans->cc.rs = rtp_socket_create(); + if (!rs) { + LOGP(DCC, LOGL_ERROR, "RTP socket creation failed.\n"); + /* reply with IP/port = 0 */ + mncc->ip = 0; + mncc->port = 0; + mncc_recvmsg(net, trans, MNCC_RTP_CREATE, (struct gsm_mncc *)mncc); + return -EIO; + } + rs->rx_action = RTP_RECV_L4; + rs->receive.net = net; + rs->receive.callref = callref; + /* reply with bound IP/port */ + mncc->ip = ntohl(rs->rtp.sin_local.sin_addr.s_addr); + mncc->port = ntohs(rs->rtp.sin_local.sin_port); + mncc_recvmsg(net, trans, MNCC_RTP_CREATE, (struct gsm_mncc *)mncc); + break; + case MNCC_RTP_CONNECT: + if (!rs) { + LOGP(DCC, LOGL_ERROR, "RTP not created.\n"); + return -EIO; + } + rc = rtp_socket_connect(trans->cc.rs, mncc->ip, mncc->port); + if (rc < 0) { + LOGP(DCC, LOGL_ERROR, "RTP socket connect failed.\n"); + /* reply with IP/port = 0 */ + mncc->ip = 0; + mncc->port = 0; + mncc_recvmsg(net, trans, MNCC_RTP_CONNECT, (struct gsm_mncc *)mncc); + return -EIO; + } + /* reply with local IP/port */ + mncc->ip = ntohl(rs->rtp.sin_local.sin_addr.s_addr); + mncc->port = ntohs(rs->rtp.sin_local.sin_port); + mncc_recvmsg(net, trans, MNCC_RTP_CONNECT, (struct gsm_mncc *)mncc); + break; + case MNCC_RTP_FREE: + if (!rs) { + LOGP(DCC, LOGL_ERROR, "RTP not created.\n"); + return -EIO; + } + rtp_socket_free(trans->cc.rs); + trans->cc.rs = NULL; + /* reply */ + mncc_recvmsg(net, trans, MNCC_RTP_FREE, (struct gsm_mncc *)mncc); + break; + } + + return 0; +} + +/* handle tch frame from L4 */ +int tch_frame_down(struct gsm_network *net, uint32_t callref, struct gsm_data_frame *data) +{ + struct gsm_trans *trans; + struct gsm_bts *bts; + + /* Find callref */ + trans = trans_find_by_callref(net, data->callref); + if (!trans) { + LOGP(DMNCC, LOGL_ERROR, "TCH frame for non-existing trans\n"); + return -EIO; + } + if (!trans->conn) { + LOGP(DMNCC, LOGL_NOTICE, "TCH frame for trans without conn\n"); + return 0; + } + if (trans->conn->lchan->type != GSM_LCHAN_TCH_F + && trans->conn->lchan->type != GSM_LCHAN_TCH_H) { + /* This should be LOGL_ERROR or NOTICE, but + * unfortuantely it happens for a couple of frames at + * the beginning of every RTP connection */ + LOGP(DMNCC, LOGL_DEBUG, "TCH frame for lchan != TCH_F/TCH_H\n"); + return 0; + } + bts = trans->conn->lchan->ts->trx->bts; + switch (bts->type) { + case GSM_BTS_TYPE_NANOBTS: + case GSM_BTS_TYPE_OSMO_SYSMO: + if (!trans->conn->lchan->abis_ip.rtp_socket) { + DEBUGP(DMNCC, "TCH frame to lchan without RTP connection\n"); + return 0; + } + if (trans->conn->lchan->abis_ip.rtp_socket->receive.callref != callref) { + /* Drop frame, if not our callref. This happens, if + * the call is on hold or retrieved by another + * transaction. */ + return 0; + } + return rtp_send_frame(trans->conn->lchan->abis_ip.rtp_socket, data); + case GSM_BTS_TYPE_BS11: + case GSM_BTS_TYPE_RBS2000: + case GSM_BTS_TYPE_NOKIA_SITE: + return trau_send_frame(trans->conn->lchan, data); + default: + LOGP(DCC, LOGL_ERROR, "Unknown BTS type %u\n", bts->type); + } + return -EINVAL; +} + static int gsm48_cc_rx_status_enq(struct gsm_trans *trans, struct msgb *msg) { DEBUGP(DCC, "-> STATUS ENQ\n"); @@ -2930,7 +3062,6 @@ int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg) int i, rc = 0; struct gsm_trans *trans = NULL, *transt; struct gsm_subscriber_connection *conn = NULL; - struct gsm_bts *bts = NULL; struct gsm_mncc *data = arg, rel; DEBUGP(DMNCC, "receive message %s\n", get_mncc_name(msg_type)); @@ -2943,46 +3074,15 @@ int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg) return tch_recv_mncc(net, data->callref, 0); case MNCC_FRAME_RECV: return tch_recv_mncc(net, data->callref, 1); + case MNCC_RTP_CREATE: + case MNCC_RTP_CONNECT: + case MNCC_RTP_FREE: + return mncc_rtp(net, data->callref, (struct gsm_mncc_rtp *) arg); case GSM_TCHF_FRAME: case GSM_TCHF_FRAME_EFR: case GSM_TCHH_FRAME: case GSM_TCH_FRAME_AMR: - /* Find callref */ - trans = trans_find_by_callref(net, data->callref); - if (!trans) { - LOGP(DMNCC, LOGL_ERROR, "TCH frame for non-existing trans\n"); - return -EIO; - } - log_set_context(BSC_CTX_SUBSCR, trans->subscr); - if (!trans->conn) { - LOGP(DMNCC, LOGL_NOTICE, "TCH frame for trans without conn\n"); - return 0; - } - if (trans->conn->lchan->type != GSM_LCHAN_TCH_F - && trans->conn->lchan->type != GSM_LCHAN_TCH_H) { - /* This should be LOGL_ERROR or NOTICE, but - * unfortuantely it happens for a couple of frames at - * the beginning of every RTP connection */ - LOGP(DMNCC, LOGL_DEBUG, "TCH frame for lchan != TCH_F/TCH_H\n"); - return 0; - } - bts = trans->conn->lchan->ts->trx->bts; - switch (bts->type) { - case GSM_BTS_TYPE_NANOBTS: - case GSM_BTS_TYPE_OSMO_SYSMO: - if (!trans->conn->lchan->abis_ip.rtp_socket) { - DEBUGP(DMNCC, "TCH frame to lchan without RTP connection\n"); - return 0; - } - return rtp_send_frame(trans->conn->lchan->abis_ip.rtp_socket, arg); - case GSM_BTS_TYPE_BS11: - case GSM_BTS_TYPE_RBS2000: - case GSM_BTS_TYPE_NOKIA_SITE: - return trau_send_frame(trans->conn->lchan, arg); - default: - LOGP(DCC, LOGL_ERROR, "Unknown BTS type %u\n", bts->type); - } - return -EINVAL; + return tch_frame_down(net, data->callref, (struct gsm_data_frame *) arg); } memset(&rel, 0, sizeof(struct gsm_mncc)); diff --git a/openbsc/src/libmsc/mncc_sock.c b/openbsc/src/libmsc/mncc_sock.c index c8cfa6a..5ae9cc4 100644 --- a/openbsc/src/libmsc/mncc_sock.c +++ b/openbsc/src/libmsc/mncc_sock.c @@ -37,6 +37,8 @@ #include <openbsc/debug.h> #include <openbsc/mncc.h> #include <openbsc/gsm_data.h> +#include <openbsc/transaction.h> +#include <openbsc/rtp_proxy.h> struct mncc_sock_state { struct gsm_network *net; @@ -50,6 +52,22 @@ int mncc_sock_from_cc(struct gsm_network *net, struct msgb *msg) struct gsm_mncc *mncc_in = (struct gsm_mncc *) msgb_data(msg); int msg_type = mncc_in->msg_type; + /* L4 uses RTP for this transaction, we send our data via RTP, + * otherwise we send it through MNCC interface */ + if (msg_type == GSM_TCHF_FRAME + || msg_type == GSM_TCHF_FRAME_EFR + || msg_type == GSM_TCHH_FRAME + || msg_type == GSM_TCH_FRAME_AMR + || msg_type == GSM_BAD_FRAME) { + struct gsm_trans *trans = trans_find_by_callref(net, mncc_in->callref); + + if (trans && trans->cc.rs) { + rtp_send_frame(trans->cc.rs, (struct gsm_data_frame *) mncc_in); + msgb_free(msg); + return 0; + } + } + /* Check if we currently have a MNCC handler connected */ if (net->mncc_state->conn_bfd.fd < 0) { LOGP(DMNCC, LOGL_ERROR, "mncc_sock receives %s for external CC app " diff --git a/openbsc/src/libmsc/transaction.c b/openbsc/src/libmsc/transaction.c index e425e67..bdd3d8e 100644 --- a/openbsc/src/libmsc/transaction.c +++ b/openbsc/src/libmsc/transaction.c @@ -78,6 +78,7 @@ struct gsm_trans *trans_alloc(struct gsm_subscriber *subscr, trans->protocol = protocol; trans->transaction_id = trans_id; trans->callref = callref; + trans->callref_keep = callref; llist_add_tail(&trans->entry, &subscr->net->trans_list); diff --git a/openbsc/src/libtrau/rtp_proxy.c b/openbsc/src/libtrau/rtp_proxy.c index 89ed8a4..2049911 100644 --- a/openbsc/src/libtrau/rtp_proxy.c +++ b/openbsc/src/libtrau/rtp_proxy.c @@ -470,7 +470,7 @@ static int rtp_socket_read(struct rtp_socket *rs, struct rtp_sub_socket *rss) other_rss->bfd.when |= BSC_FD_WRITE; break; - case RTP_RECV_UPSTREAM: + case RTP_RECV_UPSTREAM: /* from BTS to application */ if (!rs->receive.callref || !rs->receive.net) { rc = -EIO; goto out_free; @@ -499,6 +499,24 @@ static int rtp_socket_read(struct rtp_socket *rs, struct rtp_sub_socket *rss) trau_tx_to_mncc(rs->receive.net, new_msg); break; + case RTP_RECV_L4: /* from L4 */ + if (!rs->receive.callref || !rs->receive.net) { + rc = -EIO; + goto out_free; + } + if (rss->bfd.priv_nr != RTP_PRIV_RTP) { + rc = ENOTSUP; + goto out_free; + } + rc = rtp_decode(msg, rs->receive.callref, &new_msg); + if (rc < 0) + goto out_free; + msgb_free(msg); + tch_frame_down(rs->receive.net, rs->receive.callref, + (struct gsm_data_frame *) new_msg->data); + msgb_free(new_msg); + break; + case RTP_NONE: /* if socket exists, but disabled by app */ msgb_free(msg); break; diff --git a/openbsc/src/libtrau/trau_mux.c b/openbsc/src/libtrau/trau_mux.c index bb513cc..15a84b0 100644 --- a/openbsc/src/libtrau/trau_mux.c +++ b/openbsc/src/libtrau/trau_mux.c @@ -171,7 +171,9 @@ int trau_mux_unmap(const struct gsm_e1_subslot *ss, uint32_t callref) llist_del(&ue->list); return 0; } - if (ss && !memcmp(&ue->src, ss, sizeof(*ss))) { + /* Only release, if no callref is given. We must ensure that + * only the transaction's upstream is removed, if exists. */ + if (ss && !callref && !memcmp(&ue->src, ss, sizeof(*ss))) { llist_del(&ue->list); return 0; } @@ -497,11 +499,21 @@ int trau_send_frame(struct gsm_lchan *lchan, struct gsm_data_frame *frame) uint8_t trau_bits_out[TRAU_FRAME_BITS]; struct gsm_e1_subslot *dst_e1_ss = &lchan->ts->e1_link; struct subch_mux *mx; + struct upqueue_entry *ue; struct decoded_trau_frame tf; mx = e1inp_get_mux(dst_e1_ss->e1_nr, dst_e1_ss->e1_ts); if (!mx) return -EINVAL; + if (!(ue = lookup_trau_upqueue(dst_e1_ss))) { + /* Call might be on hold, so we drop frames. */ + return 0; + } + if (ue->callref != frame->callref) { + /* Slot has different transaction, due to + * another call. (Ours is on hold.) */ + return 0; + } switch (frame->msg_type) { case GSM_TCHF_FRAME: diff --git a/openbsc/src/utils/bs11_config.c b/openbsc/src/utils/bs11_config.c index e8acb46..f459744 100644 --- a/openbsc/src/utils/bs11_config.c +++ b/openbsc/src/utils/bs11_config.c @@ -83,6 +83,12 @@ struct osmo_counter *osmo_counter_alloc(const char *name) return NULL; } +/* dummy function to keep rtp_proxy.c happy */ +int tch_frame_down(struct gsm_network *net, uint32_t callref, struct gsm_data_frame *data) +{ + return 0; +} + int handle_serial_msg(struct msgb *rx_msg); /* create all objects for an initial configuration */ diff --git a/openbsc/tests/abis/abis_test.c b/openbsc/tests/abis/abis_test.c index e7e78d2..6bb84cc 100644 --- a/openbsc/tests/abis/abis_test.c +++ b/openbsc/tests/abis/abis_test.c @@ -27,6 +27,12 @@ #include <openbsc/abis_nm.h> #include <openbsc/debug.h> +/* dummy function to keep rtp_proxy.c happy */ +int tch_frame_down(struct gsm_network *net, uint32_t callref, struct gsm_data_frame *data) +{ + return 0; +} + static const uint8_t simple_config[] = { /*0, 13, */ 66, 18, 0, 3, 1, 2, 3, 19, 0, 3, 3, 4, 5, diff --git a/openbsc/tests/gbproxy/gbproxy_test.c b/openbsc/tests/gbproxy/gbproxy_test.c index d32ac83..69ce58a 100644 --- a/openbsc/tests/gbproxy/gbproxy_test.c +++ b/openbsc/tests/gbproxy/gbproxy_test.c @@ -34,6 +34,12 @@ #define SGSN_NSEI 0x0100 +/* dummy function to keep rtp_proxy.c happy */ +int tch_frame_down() +{ + return 0; +} + struct gbproxy_config gbcfg; static int gprs_process_message(struct gprs_ns_inst *nsi, const char *text, -- 1.8.1.5
Holger Hans Peter Freyther wrote:
openbsc/include/openbsc/mncc.h | 10 ++ We had this before. When you change the interface, increase the interface version. the patch does not break the current interface version 2. it is just an additional feature. there is already a new version 3 in my handover branch (lcr and openbsc).
On Wed, Jan 22, 2014 at 11:10:21AM +0100, Andreas Eversberg wrote:
Holger Hans Peter Freyther wrote:
openbsc/include/openbsc/mncc.h | 10 ++ We had this before. When you change the interface, increase the interface version. the patch does not break the current interface version 2. it is just an additional feature. there is already a new version 3 in my handover branch (lcr and openbsc).
how can someone using the MNCC know that the NITB supports the direct RTP mode? Unless we want to introduce feature negotiation the only way I see is to increase the version number. Or start using major and minor numbers. holger
Holger Hans Peter Freyther wrote:
how can someone using the MNCC know that the NITB supports the direct RTP mode? Unless we want to introduce feature negotiation the only way I see is to increase the version number. Or start using major and minor numbers. dear holger,
i changed the version to 4, so there is no conflict with version 3 of my handover branch. see attachment. best regards, andreas
On Wed, Jan 22, 2014 at 10:05:55AM +0100, Andreas Eversberg wrote:
+#define MNCC_RTP_CREATE 0x0204 +#define MNCC_RTP_CONNECT 0x0205 +#define MNCC_RTP_FREE 0x0206
Increase the version. We have a uint32_t for the version number and it will take a while to overflow it.
+ RTP_PROXY, /* forward from BTS to BTS */ + RTP_RECV_UPSTREAM, /* forward to L4 application */ + RTP_RECV_L4, /* receive RTP frames from L4 application */
L4? is that the best name you can think off?
+ bts = trans->conn->lchan->ts->trx->bts; + switch (bts->type) { + case GSM_BTS_TYPE_NANOBTS: + case GSM_BTS_TYPE_OSMO_SYSMO:
...
+ case GSM_BTS_TYPE_BS11: + case GSM_BTS_TYPE_RBS2000: + case GSM_BTS_TYPE_NOKIA_SITE:
There are already methods for "is IP based BTS", "is E1 based BTS". Couldn't you use them here?
+ /* L4 uses RTP for this transaction, we send our data via RTP, + * otherwise we send it through MNCC interface */ + if (msg_type == GSM_TCHF_FRAME + || msg_type == GSM_TCHF_FRAME_EFR + || msg_type == GSM_TCHH_FRAME + || msg_type == GSM_TCH_FRAME_AMR + || msg_type == GSM_BAD_FRAME) {
I have seen lchan->type checks and message type checks like these as well. Could you create a predicate function that check mncc_is_audio_message(), or lchan_voice_chan?
Holger Hans Peter Freyther wrote: hi holger,
+#define MNCC_RTP_CREATE 0x0204 +#define MNCC_RTP_CONNECT 0x0205 +#define MNCC_RTP_FREE 0x0206 Increase the version. We have a uint32_t for the version number and it will take a while to overflow it. i already did it. but thanks for reminding me, so i did it also for the attached patch: 0001-Complete-definitions-for-all-speech-traffic-frames-a.patch + RTP_PROXY, /* forward from BTS to BTS */ + RTP_RECV_UPSTREAM, /* forward to L4 application */ + RTP_RECV_L4, /* receive RTP frames from L4 application */ L4? is that the best name you can think off? for signalling messages (mncc application) it makes sense. for rtp it might lead to misunderstanding i changed that in the attached patch: 0006-Adding-traffic-forwarding-via-RTP-to-remote-applicat.patch There are already methods for "is IP based BTS", "is E1 based BTS". Couldn't you use them here? makes sense. i updated the patch above using this is_e1_bts() macro. + /* L4 uses RTP for this transaction, we send our data via RTP, + * otherwise we send it through MNCC interface */ + if (msg_type == GSM_TCHF_FRAME + || msg_type == GSM_TCHF_FRAME_EFR + || msg_type == GSM_TCHH_FRAME + || msg_type == GSM_TCH_FRAME_AMR + || msg_type == GSM_BAD_FRAME) {
I have seen lchan->type checks and message type checks like these as well. Could you create a predicate function that check mncc_is_audio_message(), or lchan_voice_chan? this was already done mncc_is_data_frame() check in a later patch of jolly/testing branch. i rebased my testing branch, so the patch is prior the other patches now: (see 0002-Use-helper-function-to-check-if-an-MNCC-frame-is-dat.patch).
the other patches changed a bit, due to rebase and some other improvements. i do not want to post them again, so please refer to jolly/testing branch. if wanted, i will post them. best regards, andreas
On Sat, Feb 01, 2014 at 12:31:10PM +0100, Andreas Eversberg wrote: Dear Andreas, it is difficult to pick the patches from the right reply. Please re-base your branch and send the current patches you think are ready again. thanks holger
Holger Hans Peter Freyther wrote:
it is difficult to pick the patches from the right reply. Please re-base your branch and send the current patches you think are ready again. hi holger,
i rebased my patches, but the "make distcheck" fails: make[3]: Entering directory `/files/projects/gsm/openbsc/openbsc/tests/mgcp' CC mgcp_test.o CCLD mgcp_test mgcp_test.o: In function `test_packet_error_detection': /files/projects/gsm/openbsc/openbsc/tests/mgcp/mgcp_test.c:804: undefined reference to `llround' i see that at Makefile.am the "-lm" is used to link math library. any idea? best regards, andreas
Since EFR/AMR/HR codecs use dynamic RTP payload, the payload type can be set. If it is set, the frame type must be set also, so OpenBSC knows what frame types are received via RTP. This modification only affects traffic beween application and MNCC interface, not the RTP traffic between OpenBSC and BTS. --- openbsc/include/openbsc/mncc.h | 2 ++ openbsc/include/openbsc/rtp_proxy.h | 2 ++ openbsc/src/libmsc/gsm_04_08.c | 2 ++ openbsc/src/libtrau/rtp_proxy.c | 57 ++++++++++++++++++++++++++----------- 4 files changed, 46 insertions(+), 17 deletions(-) diff --git a/openbsc/include/openbsc/mncc.h b/openbsc/include/openbsc/mncc.h index a380b8f..0a4f2ef 100644 --- a/openbsc/include/openbsc/mncc.h +++ b/openbsc/include/openbsc/mncc.h @@ -187,6 +187,8 @@ struct gsm_mncc_rtp { uint32_t callref; uint32_t ip; uint16_t port; + uint32_t payload_type; + uint32_t payload_msg_type; }; char *get_mncc_name(int value); diff --git a/openbsc/include/openbsc/rtp_proxy.h b/openbsc/include/openbsc/rtp_proxy.h index 2729f63..ae49ca5 100644 --- a/openbsc/include/openbsc/rtp_proxy.h +++ b/openbsc/include/openbsc/rtp_proxy.h @@ -74,6 +74,8 @@ struct rtp_socket { struct { struct gsm_network *net; uint32_t callref; + uint8_t payload_type; /* dynamic PT */ + int msg_type; /* message type for dynamic PT */ } receive; }; enum rtp_tx_action tx_action; diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index 7bb9c87..79300d1 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -1758,6 +1758,8 @@ static int mncc_rtp(struct gsm_network *net, uint32_t callref, struct gsm_mncc_r mncc_recvmsg(net, trans, MNCC_RTP_CONNECT, (struct gsm_mncc *)mncc); return -EIO; } + rs->receive.msg_type = mncc->payload_msg_type; + rs->receive.payload_type = mncc->payload_type; /* reply with local IP/port */ mncc->ip = ntohl(rs->rtp.sin_local.sin_addr.s_addr); mncc->port = ntohs(rs->rtp.sin_local.sin_port); diff --git a/openbsc/src/libtrau/rtp_proxy.c b/openbsc/src/libtrau/rtp_proxy.c index 2049911..ed7479d 100644 --- a/openbsc/src/libtrau/rtp_proxy.c +++ b/openbsc/src/libtrau/rtp_proxy.c @@ -106,7 +106,7 @@ struct rtp_x_hdr { #define RTP_VERSION 2 /* decode an rtp frame and create a new buffer with payload */ -static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data) +static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data, int msg_type) { struct msgb *new_msg; struct gsm_data_frame *frame; @@ -114,7 +114,6 @@ static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data) struct rtp_x_hdr *rtpxh; uint8_t *payload; int payload_len; - int msg_type; int x_len; int amr = 0; @@ -165,9 +164,31 @@ static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data) } } - switch (rtph->payload_type) { - case RTP_PT_GSM_FULL: - msg_type = GSM_TCHF_FRAME; + /* If no explicit frame type is given, select frame type from + * payload type. */ + if (!msg_type) { + switch (rtph->payload_type) { + case RTP_PT_GSM_FULL: + msg_type = GSM_TCHF_FRAME; + break; + case RTP_PT_GSM_EFR: + msg_type = GSM_TCHF_FRAME_EFR; + break; + case RTP_PT_GSM_HALF: + msg_type = GSM_TCHH_FRAME; + break; + case RTP_PT_AMR: + msg_type = GSM_TCH_FRAME_AMR; + break; + default: + DEBUGPC(DLMUX, "received RTP frame with unknown " + "payload type %d\n", rtph->payload_type); + return -EINVAL; + } + } + + switch (msg_type) { + case GSM_TCHF_FRAME: if (payload_len != RTP_LEN_GSM_FULL) { DEBUGPC(DLMUX, "received RTP full rate frame with " "payload length != %d (len = %d)\n", @@ -175,8 +196,7 @@ static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data) return -EINVAL; } break; - case RTP_PT_GSM_EFR: - msg_type = GSM_TCHF_FRAME_EFR; + case GSM_TCHF_FRAME_EFR: if (payload_len != RTP_LEN_GSM_EFR) { DEBUGPC(DLMUX, "received RTP extended full rate frame " "with payload length != %d (len = %d)\n", @@ -184,8 +204,7 @@ static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data) return -EINVAL; } break; - case RTP_PT_GSM_HALF: - msg_type = GSM_TCHH_FRAME; + case GSM_TCHH_FRAME: if (payload_len != RTP_LEN_GSM_HALF) { DEBUGPC(DLMUX, "received RTP half rate frame with " "payload length != 15 (len = %d)\n", @@ -193,12 +212,11 @@ static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data) return -EINVAL; } break; - case RTP_PT_AMR: + case GSM_TCH_FRAME_AMR: amr = 1; break; default: - DEBUGPC(DLMUX, "received RTP frame with unknown payload " - "type %d\n", rtph->payload_type); + DEBUGPC(DLMUX, "Forced message type %x unknown\n", msg_type); return -EINVAL; } @@ -246,6 +264,10 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) int payload_len; int duration; /* in samples */ int amr = 0; + uint8_t dynamic_pt = 0; + + if (rs->rx_action == RTP_RECV_L4) + dynamic_pt = rs->receive.payload_type; if (rs->tx_action != RTP_SEND_DOWNSTREAM) { /* initialize sequences */ @@ -262,17 +284,17 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) duration = RTP_GSM_DURATION; break; case GSM_TCHF_FRAME_EFR: - payload_type = RTP_PT_GSM_EFR; + payload_type = (dynamic_pt) ? : RTP_PT_GSM_EFR; payload_len = RTP_LEN_GSM_EFR; duration = RTP_GSM_DURATION; break; case GSM_TCHH_FRAME: - payload_type = RTP_PT_GSM_HALF; + payload_type = (dynamic_pt) ? : RTP_PT_GSM_HALF; payload_len = RTP_LEN_GSM_HALF; duration = RTP_GSM_DURATION; break; case GSM_TCH_FRAME_AMR: - payload_type = RTP_PT_AMR; + payload_type = (dynamic_pt) ? : RTP_PT_AMR; payload_len = frame->data[0]; duration = RTP_GSM_DURATION; amr = 1; @@ -492,7 +514,7 @@ static int rtp_socket_read(struct rtp_socket *rs, struct rtp_sub_socket *rss) rc = -EINVAL; goto out_free; } - rc = rtp_decode(msg, rs->receive.callref, &new_msg); + rc = rtp_decode(msg, rs->receive.callref, &new_msg, 0); if (rc < 0) goto out_free; msgb_free(msg); @@ -508,7 +530,8 @@ static int rtp_socket_read(struct rtp_socket *rs, struct rtp_sub_socket *rss) rc = ENOTSUP; goto out_free; } - rc = rtp_decode(msg, rs->receive.callref, &new_msg); + rc = rtp_decode(msg, rs->receive.callref, &new_msg, + rs->receive.msg_type); if (rc < 0) goto out_free; msgb_free(msg); -- 1.8.1.5
The RTP stream is generated or forwarded by OpenBSC to nanoBTS. Due to switching of streams (hold/retrieve call), packet loss or even stalling of sender's process, the time stamp must be corrected. If outdated packets are received, they get dropped. --- openbsc/src/libtrau/rtp_proxy.c | 80 ++++++++++++++++++++++++++++++----------- 1 file changed, 59 insertions(+), 21 deletions(-) diff --git a/openbsc/src/libtrau/rtp_proxy.c b/openbsc/src/libtrau/rtp_proxy.c index ed7479d..3e6c462 100644 --- a/openbsc/src/libtrau/rtp_proxy.c +++ b/openbsc/src/libtrau/rtp_proxy.c @@ -236,6 +236,12 @@ static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data, in return 0; } +#define USEC_1S 1000000 +#define USEC_10MS 10000 +#define USEC_20MS 20000 +#define SAMPLES_1S 8000 +#define USEC_SAMPLE 125 + /* "to - from" */ static void tv_difference(struct timeval *diff, const struct timeval *from, const struct timeval *__to) @@ -244,13 +250,60 @@ static void tv_difference(struct timeval *diff, const struct timeval *from, if (to->tv_usec < from->tv_usec) { to->tv_sec -= 1; - to->tv_usec += 1000000; + to->tv_usec += USEC_1S; } diff->tv_usec = to->tv_usec - from->tv_usec; diff->tv_sec = to->tv_sec - from->tv_sec; } +/* add sec,usec to tv */ +static void tv_add(struct timeval *tv, int sec, int usec) +{ + + if (usec < 0) + usec += USEC_1S; + tv->tv_sec += sec; + tv->tv_usec += usec; + if (tv->tv_usec >= USEC_1S) { + tv->tv_sec++; + tv->tv_usec -= USEC_1S; + } +} + +static int correct_timestamp(struct rtp_socket *rs, int duration) +{ + struct timeval tv, tv_diff; + long int usec_diff, frame_diff; + + gettimeofday(&tv, NULL); + tv_difference(&tv_diff, &rs->transmit.last_tv, &tv); + tv_add(&rs->transmit.last_tv, 0, USEC_20MS); + + usec_diff = tv_diff.tv_sec * USEC_1S + tv_diff.tv_usec; + frame_diff = ((usec_diff + (USEC_10MS)) / USEC_20MS); /* round */ + + if (abs(frame_diff - 1) > 1) { + long int frame_diff_excess = frame_diff - 1; + long int sample_diff_excess = frame_diff_excess * duration; + + /* correct last_tv */ + tv_add(&rs->transmit.last_tv, sample_diff_excess / SAMPLES_1S, + (sample_diff_excess % SAMPLES_1S) * USEC_SAMPLE); + /* drop frame, if time stamp is in the past */ + if (frame_diff_excess < 0) + return -1; + LOGP(DLMUX, LOGL_NOTICE, + "Correcting timestamp difference of %ld frames " + "(to %s)\n", frame_diff_excess, + (rs->rx_action == RTP_RECV_L4) ? "app" : "BTS"); + rs->transmit.sequence += frame_diff_excess; + rs->transmit.timestamp += sample_diff_excess; + } + + return 0; +} + /*! \brief encode and send a rtp frame * \param[in] rs RTP socket through which we shall send * \param[in] frame GSM RTP frame to be sent @@ -265,6 +318,7 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) int duration; /* in samples */ int amr = 0; uint8_t dynamic_pt = 0; + int rc; if (rs->rx_action == RTP_RECV_L4) dynamic_pt = rs->receive.payload_type; @@ -275,6 +329,7 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) rs->transmit.ssrc = rand(); rs->transmit.sequence = random(); rs->transmit.timestamp = random(); + gettimeofday(&rs->transmit.last_tv, NULL); } switch (frame->msg_type) { @@ -313,26 +368,9 @@ int rtp_send_frame(struct rtp_socket *rs, struct gsm_data_frame *frame) return -EINVAL; } - { - struct timeval tv, tv_diff; - long int usec_diff, frame_diff; - - gettimeofday(&tv, NULL); - tv_difference(&tv_diff, &rs->transmit.last_tv, &tv); - rs->transmit.last_tv = tv; - - usec_diff = tv_diff.tv_sec * 1000000 + tv_diff.tv_usec; - frame_diff = (usec_diff / 20000); - - if (abs(frame_diff) > 1) { - long int frame_diff_excess = frame_diff - 1; - - LOGP(DLMUX, LOGL_NOTICE, - "Correcting frame difference of %ld frames\n", frame_diff_excess); - rs->transmit.sequence += frame_diff_excess; - rs->transmit.timestamp += frame_diff_excess * duration; - } - } + rc = correct_timestamp(rs, duration); + if (rc) + return 0; if (frame->msg_type == GSM_BAD_FRAME) return 0; -- 1.8.1.5
Andreas Eversberg wrote:
+/* add sec,usec to tv */ +static void tv_add(struct timeval *tv, int sec, int usec) +{ + + if (usec < 0) + usec += USEC_1S;
Doesn't this also need to do sec--; ?
+ tv->tv_sec += sec; + tv->tv_usec += usec; + if (tv->tv_usec >= USEC_1S) { + tv->tv_sec++; + tv->tv_usec -= USEC_1S; + } +}
Consider changing the second if() to while(), so that the function works correctly also when adding several seconds using only the usec parameter. Very nice use of defines! They make for excellent readability. //Peter
Peter Stuge wrote:
+ if (usec < 0)
+ usec += USEC_1S; Doesn't this also need to do sec--; ?
+ tv->tv_sec += sec; + tv->tv_usec += usec; + if (tv->tv_usec >= USEC_1S) { + tv->tv_sec++; + tv->tv_usec -= USEC_1S; + } +} Consider changing the second if() to while(), so that the function works correctly also when adding several seconds using only the usec parameter. dear peter,
thanx for looking at the patch. i fixed the missing "sec--" and added while loops. see attachment. best regards, andreas
sorry, wrong attachment in my last mail. here is the correct one.
Dear Andreas, On 23.01.2014 11:53, Andreas Eversberg wrote:
0008-Fixed-nanoBTS-delay-problems-if-RTP-stream-jitters-t.patch
@@ -244,13 +250,62 @@ static void tv_difference(struct timeval *diff, const struct timeval *from,
... Is there a reason why you are not using the BSD macros timersub and timeradd (beside the new support for denormalized usec values)?
+/* add sec,usec to tv */ +static void tv_add(struct timeval *tv, int sec, int usec) +{ + + while (usec < 0) { + usec += USEC_1S; + sec--; + } + tv->tv_sec += sec; + tv->tv_usec += usec; + while (tv->tv_usec >= USEC_1S) { + tv->tv_sec++; + tv->tv_usec -= USEC_1S; + } +}
I'm not sure whether it is a good idea to use while loops in this case since CPU usage is O(N) of the usec value. Wouldn't it be more convenient to have a function tv_add_us(tv, usec) instead that does the div/mod stuff to a temporary timeval and then just calls timeradd()? Cheers Jacob
Jacob Erlbeck wrote:
+/* add sec,usec to tv */ +static void tv_add(struct timeval *tv, int sec, int usec) +{ + + while (usec < 0) { + usec += USEC_1S; + sec--; + } + tv->tv_sec += sec; + tv->tv_usec += usec; + while (tv->tv_usec >= USEC_1S) { + tv->tv_sec++; + tv->tv_usec -= USEC_1S; + } +}
I'm not sure whether it is a good idea to use while loops in this case since CPU usage is O(N) of the usec value.
Remember that N>1 only with very small or very large usec values, which I guess will be a corner case since it wasn't handled before?
Wouldn't it be more convenient to have a function tv_add_us(tv, usec) instead that does the div/mod stuff to a temporary timeval and then just calls timeradd()?
Is there a machine where div/mod is actually more efficient than a small number of iterations around a loop? :) Is this a hot path? Then I think it makes sense to optimize harder for performance rather than for readability. But compilers are good at that too.. //Peter
On 30.01.2014 16:43, Peter Stuge wrote:
Jacob Erlbeck wrote:
+/* add sec,usec to tv */ +static void tv_add(struct timeval *tv, int sec, int usec) +{ + + while (usec < 0) { + usec += USEC_1S; + sec--; + } + tv->tv_sec += sec; + tv->tv_usec += usec; + while (tv->tv_usec >= USEC_1S) { + tv->tv_sec++; + tv->tv_usec -= USEC_1S; + } +}
I'm not sure whether it is a good idea to use while loops in this case since CPU usage is O(N) of the usec value.
Remember that N>1 only with very small or very large usec values, which I guess will be a corner case since it wasn't handled before?
I think the question here is, do I want have a generic function that could be used in other places, too? If yes, I strongly advice to take into account that bad things could happen here (e.g. due to bugs delivering broken values for usec), eventually causing very-hard-to-diagnose latency problems. If you first check for out-of-range usecs, hint it with something like UNLIKELY() and do the div/mod in that case only. That's probably slightly faster than the while approach if most of the usecs are within 0..999999 and has still an upper bound. Or are we looking at the few applications already part of the patch, which would not have any advantage of allowing out of range usecs.
Is there a machine where div/mod is actually more efficient than a small number of iterations around a loop? :)
That depends on 'small'. But sheer performance (throughput) is not always the most important, sometimes it's predictability, latency, and maintainability. And throughput at the cost of the others should be considered very carefully!
Wouldn't it be more convenient to have a function tv_add_us(tv, usec) instead that does the div/mod stuff to a temporary timeval and then just calls timeradd()?
Let's assume I had an implementation like (no it's not C) void tv_add_us(tv, usec) { struct timeval foo; foo.tv_sec = usec / 1000000; foo.tv_usec = usec % 1000000; timeradd(tv, &foo, tv); } Looking at the applications of the function: tv_add(&rs->transmit.last_tv, 0, USEC_20MS); tv_add(&rs->transmit.last_tv, sample_diff_excess / SAMPLES_1S, (sample_diff_excess % SAMPLES_1S) * USEC_SAMPLE); Which I then would write as tv_add_us(&rs->transmit.last_tv, USEC_20MS); tv_add_us(&rs->transmit.last_tv, sample_diff_excess * USEC_SAMPLE); And thanks to inlining the div/mod would vanish completely in the first case and would be without extra cost in the second. So no performance penalty but much better readability.
Is this a hot path? Then I think it makes sense to optimize harder for performance rather than for readability. But compilers are good at that too..
I'd rather go for readability by default until profiling tells me otherwise. Some well-intentioned hand optimizations lead sometimes to worse performance if the prevent the compiler from doing better optimizations. So on some platforms a branch taken can be more expensive than a div/mod. And did you understand the second application of tv_add() at first sight? It is correct but that is not obvious IMO. Jacob
Jacob Erlbeck wrote: dear jacob,
void tv_add_us(tv, usec) { struct timeval foo; foo.tv_sec = usec / 1000000; foo.tv_usec = usec % 1000000;
timeradd(tv, &foo, tv); } Which I then would write as
tv_add_us(&rs->transmit.last_tv, USEC_20MS); tv_add_us(&rs->transmit.last_tv, sample_diff_excess * USEC_SAMPLE);
And thanks to inlining the div/mod would vanish completely in the first case and would be without extra cost in the second. So no performance penalty but much better readability. this makes sense. it simplifies things. i implemented it. by getting rid of negative usecs, there is no more issue about correctly decrementing the timeval. And did you understand the second application of tv_add() at first sight? It is correct but that is not obvious IMO. the first implementation increments last_tv by the duration of one frame. (20ms) if too much time have been elapsed since when the last frame has been processed, the last_tv is additionally incremented by the duration of frames that have been missed since then. also the RTP timestamp and sequence number is incremented accordingly.
holger mentioned that you also had a problem with speech frames in conjunction with ipaccess BTS. was it a similar problem? how did you solve this? best regards, andreas
Dear Andreas, On 02.02.2014 13:09, Andreas Eversberg wrote:
holger mentioned that you also had a problem with speech frames in conjunction with ipaccess BTS. was it a similar problem? how did you solve this?
that BTS silently drops frames that do not have a RTP timestamp of T0 + 160 * k, where T0 is the first timestamp (I ignore the modulo here). In addition it ceases to transmit audio from RTP if there is a pause (e.g. no RTP for >1s) but the timestamps are not incremented accordingly (probably because they've been considered to be too late). In short: that BTS expects proper timestamps. I've solved this by optionally allowing the MGCP daemon to enforce the 160*k requirement by rounding the timestamp towards the next valid one. In addition, when SSRCs are patched, the new timestamp offset is calculated based on (monotonic) system time that has passed between the last packet of the old SSRC and the first of the new. Since the RTP timestamp problems only happened when the network side switched streams (and thus luckily SSRCs), this was sufficient. In theory, the BTS should be able to detect SSRC changes and reset its timing accordingly. But since this is not the case with that BTS, those workarounds are neccessary :-( Cheers Jacob
On Wed, Jan 22, 2014 at 10:05:57AM +0100, Andreas Eversberg wrote:
The RTP stream is generated or forwarded by OpenBSC to nanoBTS. Due to switching of streams (hold/retrieve call), packet loss or even stalling of sender's process, the time stamp must be corrected. If outdated packets are received, they get dropped.
Can you please use the rtp state/replay code to show this specific issue? From Jacob's latest experiment the nanoBTS software only appears to have issues when the same timestamp is sent twice or the sample delta is not a multiple of 160. The replaying code and the generation of rtp_headers can be found in the contrib/rtp directory. holger
Holger Hans Peter Freyther wrote:
The replaying code and the generation of rtp_headers can be found in the contrib/rtp directory. is there any tutorial about what is does and on how to use it?
When reading from RTP socket, the first read() may fail right after connecting to remote socket. Subsequent read() will work as it should. I have not discovered why this read fails, but I don't see any reason why we should stop reading, just because one read() fails at the beginning. --- openbsc/src/libtrau/rtp_proxy.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/openbsc/src/libtrau/rtp_proxy.c b/openbsc/src/libtrau/rtp_proxy.c index 3e6c462..8b19c93 100644 --- a/openbsc/src/libtrau/rtp_proxy.c +++ b/openbsc/src/libtrau/rtp_proxy.c @@ -501,10 +501,8 @@ static int rtp_socket_read(struct rtp_socket *rs, struct rtp_sub_socket *rss) return -ENOMEM; rc = read(rss->bfd.fd, msg->data, RTP_ALLOC_SIZE); - if (rc <= 0) { - rss->bfd.when &= ~BSC_FD_READ; + if (rc <= 0) return rc; - } msgb_put(msg, rc); -- 1.8.1.5
Andreas Eversberg wrote:
When reading from RTP socket, the first read() may fail right after connecting to remote socket. Subsequent read() will work as it should.
I have not discovered why this read fails, but I don't see any reason why we should stop reading, just because one read() fails at the beginning.
What is errno set to when this failure happens? How to handle read() failures should probably depend on exactly what the failure is, ie. might need testing errno for known benign values and letting anything else cause a more severe error which bubbles up the stack. //Peter
Peter Stuge wrote:
What is errno set to when this failure happens?
How to handle read() failures should probably depend on exactly what the failure is, ie. might need testing errno for known benign values and letting anything else cause a more severe error which bubbles up the stack. i did some tests. it actually happens when the remote peer has not yet bound the RTP socket, so the local peer receives a "connection refused". i updated the patch, see attachment.
On Thu, Jan 23, 2014 at 11:57:00AM +0100, Andreas Eversberg wrote:
+ if (errno == 111)
again, please use fewer magic numbers. Use ECONNREFUSED then.
+ DEBUGPC(DLMUX, "Read of RTP socket (%p) failed (errno %d)\n", + rs, errno);
Please consider using strerror next to the errno
Holger Hans Peter Freyther wrote:
+ if (errno == 111) again, please use fewer magic numbers. Use ECONNREFUSED then.
+ DEBUGPC(DLMUX, "Read of RTP socket (%p) failed (errno %d)\n", + rs, errno); Please consider using strerror next to the errno
ok, here it is.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 01/23/2014 07:32 AM, Andreas Eversberg wrote:
ok, here it is. So, what's the timeline for these patches to be integrated into the master branch? There are some specific things in there (half-rate support, AMR support, rtp-bridge) which could be very handy to us (Rhizomatica) both in terms of network capacity and also sound quality (at the moment we are routing everything to LCR, which means a GSM<->PCMA<->GSM codec conversion even on internal calls). Cheers
Ciaby -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iF4EAREKAAYFAlLoPpgACgkQC30ZhxNccpEDKwEAmGYdMeWvugoRPnW4QTMTfYaj cPduR9Bo+ipmDtN7pQ4A/14k6ZgRH6l73AJA6AqmNmMhxILgOSGRIAoP8PNCnXRv =bBw9 -----END PGP SIGNATURE-----
On Tue, Jan 28, 2014 at 05:34:53PM -0600, Ciaby wrote: Hi,
ok, here it is. So, what's the timeline for these patches to be integrated into the master branch? There are some specific things in there (half-rate support, AMR support, rtp-bridge) which could be very handy to us (Rhizomatica) both in terms of network capacity and also sound quality (at the moment we are routing everything to LCR, which means a GSM<->PCMA<->GSM codec conversion even on internal calls).
the answer is as time permits. As you see with the responses there are various things that are not right with them and as a maintainer it is frustrating to have to start at zero at every new patch.
On Wed, Jan 22, 2014 at 10:05:50AM +0100, Andreas Eversberg wrote:
- /* in case, we don't have a RTP socket yet, we note this - * in the transaction and try later */ + /* in case, we don't have a RTP socket yet, we try later */
case GSM_BTS_TYPE_NOKIA_SITE: + /* in case we don't have a TCH with correct mode, we try later */
Please attempt to write comments in a way that one can understand them in a year later. What will be tried later? But actually try for me means that the code would actually execute a deferred action while here it will simply react to another function call. PLease update the patch.
Holger Hans Peter Freyther wrote:
Please attempt to write comments in a way that one can understand them in a year later. What will be tried later? But actually try for me means that the code would actually execute a deferred action while here it will simply react to another function call.
PLease update the patch. ok, here is the updated patch.
On Sun, Jan 26, 2014 at 09:57:03AM +0100, Andreas Eversberg wrote:
Holger Hans Peter Freyther wrote:
Please attempt to write comments in a way that one can understand them in a year later. What will be tried later? But actually try for me means that the code would actually execute a deferred action while here it will simply react to another function call.
+ /* In case, we don't have a RTP socket to the BTS yet, we try + * later when the socket has been created and connected. */ if (!lchan->abis_ip.rtp_socket) { - trans->tch_recv = enable;
sorry to nitpick but these things matter when people read someone else's code in order to understand how things work. Right now I am not able to understand "what" will be tried later? Maybe something like this: /* In case we don't have a RTP socket to the BTS yet, the code * can not connect the sockets and will return. This method will * be called for the next NET->BTS frame and once audio is connected * from the BTS. */ or something along the lines.
+ /* In case we don't have a TCH with correct mode, we try later + * after assignment or handover is complete. This is performed + * by switch_trau_mux() then. */
Similar. s/try/will be done/
Holger Hans Peter Freyther wrote:
sorry to nitpick but these things matter when people read someone else's code in order to understand how things work. Right now I am not able to understand "what" will be tried later? dear holger,
no problem. i thought it was clear enough, by reading further comments in the code. in the attachment i describes "what" and "when". i hope that it is clear enough now. best regards, andreas
participants (6)
-
Andreas Eversberg -
Andreas Eversberg -
Ciaby -
Holger Hans Peter Freyther -
Jacob Erlbeck -
Peter Stuge