[PATCH 1/4] rtp: Fixed size check in padded RTP packets
This patch fixes a corner case (padding 1, payload size incl padding 0) which is not being detected correctly. Sponsored-by: On-Waves ehf --- openbsc/src/libtrau/rtp_proxy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openbsc/src/libtrau/rtp_proxy.c b/openbsc/src/libtrau/rtp_proxy.c index 0074b4a..4278fc6 100644 --- a/openbsc/src/libtrau/rtp_proxy.c +++ b/openbsc/src/libtrau/rtp_proxy.c @@ -151,7 +151,7 @@ static int rtp_decode(struct msgb *msg, uint32_t callref, struct msgb **data) } } if (rtph->padding) { - if (payload_len < 0) { + if (payload_len < 1) { DEBUGPC(DLMUX, "received RTP frame too short for " "padding length\n"); return -EINVAL; -- 1.7.9.5
This patch modifies the patch_and_count() function to check for RTP timestamp inconsistencies. It basically checks, whether dTS/dSeqNo remains constant. If this fails, the corresponding counter is incremented. There are four counter for this: Incoming and outgoing, each for streams from the BTS and the net. Note that this approach presumes, that the per RTP packet duration (in samples) remains the same throughout the entire stream. Changing the number of speech frames per channel and packet will be detected as error. In addition, the VTY command 'show mgcp' is extended by an optional 'stats' to show the counter values, too. Ticket: OW#964 Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/mgcp_internal.h | 4 ++ openbsc/src/libmgcp/mgcp_network.c | 73 +++++++++++++++++++++++++++++-- openbsc/src/libmgcp/mgcp_protocol.c | 24 ++++++++-- openbsc/src/libmgcp/mgcp_vty.c | 23 +++++++--- openbsc/tests/mgcp/mgcp_test.c | 3 +- 5 files changed, 114 insertions(+), 13 deletions(-) diff --git a/openbsc/include/openbsc/mgcp_internal.h b/openbsc/include/openbsc/mgcp_internal.h index d5bd3dd..2ad7058 100644 --- a/openbsc/include/openbsc/mgcp_internal.h +++ b/openbsc/include/openbsc/mgcp_internal.h @@ -56,6 +56,10 @@ struct mgcp_rtp_state { int32_t timestamp_offset; uint32_t jitter; int32_t transit; + + int32_t last_tsdelta; + uint32_t err_ts_in_counter; + uint32_t err_ts_out_counter; }; struct mgcp_rtp_end { diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index 2b55527..14bc024 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -143,13 +143,18 @@ int mgcp_send_dummy(struct mgcp_endpoint *endp) * we receive will be seen as a switch in streams. */ static void patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *state, - int payload, struct sockaddr_in *addr, char *data, int len) + struct mgcp_rtp_end *rtp_end, struct sockaddr_in *addr, + char *data, int len) { uint32_t arrival_time; int32_t transit, d; uint16_t seq, udelta; uint32_t timestamp; struct rtp_hdr *rtp_hdr; + int32_t tsdelta = 0; + int tsdelta_valid = 0; + int last_tsdelta = state->last_tsdelta; + int payload = rtp_end->payload_type; if (len < sizeof(*rtp_hdr)) return; @@ -176,6 +181,41 @@ static void patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *s "The SSRC changed on 0x%x SSRC: %u offset: %d from %s:%d in %d\n", ENDPOINT_NUMBER(endp), state->ssrc, state->seq_offset, inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), endp->conn_mode); + } else { + /* Compute current per-packet timestamp delta */ + if (state->initialized && seq != state->max_seq) { + int corr_timestamp = state->timestamp_offset + timestamp; + tsdelta = + (int32_t)(corr_timestamp - state->last_timestamp) / + (int16_t)(seq - state->max_seq); + + if (tsdelta == 0) { + state->err_ts_in_counter += 1; + LOGP(DMGCP, LOGL_ERROR, + "Input timestamp delta is %d " + "on 0x%x SSRC: %u timestamp: %u " + "from %s:%d in %d\n", + tsdelta, + ENDPOINT_NUMBER(endp), state->ssrc, timestamp, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); + } else + tsdelta_valid = 1; + } + + if (tsdelta_valid && state->last_tsdelta != tsdelta) { + if (state->last_tsdelta) { + state->err_ts_in_counter += 1; + LOGP(DMGCP, LOGL_ERROR, + "Input timestamp delta changes from %d to %d " + "on 0x%x SSRC: %u timestamp: %u from %s:%d in %d\n", + state->last_tsdelta, tsdelta, + ENDPOINT_NUMBER(endp), state->ssrc, timestamp, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); + } + state->last_tsdelta = tsdelta; + } } /* apply the offset and store it back to the packet */ @@ -188,6 +228,33 @@ static void patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *s rtp_hdr->timestamp = htonl(timestamp); } + /* Check again, whether the timestamps are still valid */ + if (last_tsdelta && seq != state->max_seq) { + tsdelta = (timestamp - state->last_timestamp) / + (seq - state->max_seq); + + if (tsdelta == 0) { + state->err_ts_out_counter += 1; + LOGP(DMGCP, LOGL_ERROR, + "Output timestamp delta is %d " + "on 0x%x SSRC: %u timestamp: %u " + "from %s:%d in %d\n", + tsdelta, + ENDPOINT_NUMBER(endp), state->ssrc, timestamp, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); + } else if (last_tsdelta != tsdelta) { + state->err_ts_out_counter += 1; + LOGP(DMGCP, LOGL_ERROR, + "Output timestamp delta changes from %d to %d " + "on 0x%x SSRC: %u timestamp: %u from %s:%d in %d\n", + last_tsdelta, tsdelta, + ENDPOINT_NUMBER(endp), state->ssrc, timestamp, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); + } + } + /* * The below takes the shape of the validation from Appendix A. Check * if there is something weird with the sequence number, otherwise check @@ -280,7 +347,7 @@ static int mgcp_send(struct mgcp_endpoint *endp, int dest, int is_rtp, if (dest == MGCP_DEST_NET) { if (is_rtp) { patch_and_count(endp, &endp->bts_state, - endp->net_end.payload_type, + &endp->net_end, addr, buf, rc); forward_data(endp->net_end.rtp.fd, &endp->taps[MGCP_TAP_NET_OUT], buf, rc); @@ -295,7 +362,7 @@ static int mgcp_send(struct mgcp_endpoint *endp, int dest, int is_rtp, } else { if (is_rtp) { patch_and_count(endp, &endp->net_state, - endp->bts_end.payload_type, + &endp->bts_end, addr, buf, rc); forward_data(endp->bts_end.rtp.fd, &endp->taps[MGCP_TAP_BTS_OUT], buf, rc); diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index 616e0a9..b19b8a0 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -1163,14 +1163,30 @@ void mgcp_format_stats(struct mgcp_endpoint *endp, char *msg, size_t size) { uint32_t expected, jitter; int ploss; + int nchars; mgcp_state_calc_loss(&endp->net_state, &endp->net_end, &expected, &ploss); jitter = mgcp_state_calc_jitter(&endp->net_state); - snprintf(msg, size, "\r\nP: PS=%u, OS=%u, PR=%u, OR=%u, PL=%d, JI=%u", - endp->bts_end.packets, endp->bts_end.octets, - endp->net_end.packets, endp->net_end.octets, - ploss, jitter); + nchars = snprintf(msg, size, + "\r\nP: PS=%u, OS=%u, PR=%u, OR=%u, PL=%d, JI=%u", + endp->bts_end.packets, endp->bts_end.octets, + endp->net_end.packets, endp->net_end.octets, + ploss, jitter); + if (nchars < 0 || nchars >= size) + goto truncate; + + msg += nchars; + size -= nchars; + + /* Error Counter */ + snprintf(msg, size, + "\r\nX-Osmo: EC TIS=%u, TOS=%u, TIR=%u, TOR=%u", + endp->net_state.err_ts_in_counter, + endp->net_state.err_ts_out_counter, + endp->bts_state.err_ts_in_counter, + endp->bts_state.err_ts_out_counter); +truncate: msg[size - 1] = '\0'; } diff --git a/openbsc/src/libmgcp/mgcp_vty.c b/openbsc/src/libmgcp/mgcp_vty.c index 3c239d8..528a312 100644 --- a/openbsc/src/libmgcp/mgcp_vty.c +++ b/openbsc/src/libmgcp/mgcp_vty.c @@ -114,7 +114,7 @@ static int config_write_mgcp(struct vty *vty) return CMD_SUCCESS; } -static void dump_trunk(struct vty *vty, struct mgcp_trunk_config *cfg) +static void dump_trunk(struct vty *vty, struct mgcp_trunk_config *cfg, int verbose) { int i; @@ -139,18 +139,31 @@ static void dump_trunk(struct vty *vty, struct mgcp_trunk_config *cfg) endp->bts_end.packets, endp->net_end.packets, endp->trans_net.packets, endp->trans_bts.packets, VTY_NEWLINE); + + if (verbose) + vty_out(vty, + " Timestamp Errs: BTS %d->%d, Net %d->%d%s", + endp->bts_state.err_ts_in_counter, + endp->bts_state.err_ts_out_counter, + endp->net_state.err_ts_in_counter, + endp->net_state.err_ts_out_counter, + VTY_NEWLINE); } } -DEFUN(show_mcgp, show_mgcp_cmd, "show mgcp", - SHOW_STR "Display information about the MGCP Media Gateway") +DEFUN(show_mcgp, show_mgcp_cmd, + "show mgcp [stats]", + SHOW_STR + "Display information about the MGCP Media Gateway\n" + "Include Statistics\n") { struct mgcp_trunk_config *trunk; + int show_stats = argc >= 1; - dump_trunk(vty, &g_cfg->trunk); + dump_trunk(vty, &g_cfg->trunk, show_stats); llist_for_each_entry(trunk, &g_cfg->trunks, entry) - dump_trunk(vty, trunk); + dump_trunk(vty, trunk, show_stats); return CMD_SUCCESS; } diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index 5565e73..3499ee3 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -97,7 +97,8 @@ "C: 2\r\n" #define DLCX_RET "250 7 OK\r\n" \ - "P: PS=0, OS=0, PR=0, OR=0, PL=0, JI=0\r\n" + "P: PS=0, OS=0, PR=0, OR=0, PL=0, JI=0\r\n" \ + "X-Osmo: EC TIS=0, TOS=0, TIR=0, TOR=0\r\n" #define RQNT "RQNT 186908780 1@mgw MGCP 1.0\r\n" \ "X: B244F267488\r\n" \ -- 1.7.9.5
On Thu, Nov 21, 2013 at 07:05:43PM +0100, Jacob Erlbeck wrote: Thank you for the patch. I have one wish for you to move the code to separate method (with the side effect on the data structure).
+ } else {
I would prefer if we could move this code into another method and re-use it later on. Sure this requires to pass "Input"/"Output" into the function and the right counters. But I think I prefer this over the code clone. You just happened to be late to if/elseif/else and I think it is getting a bit crowded in this method.
+ /* Compute current per-packet timestamp delta */ + if (state->initialized && seq != state->max_seq) {
state->initialized == TRUE in this else branc.
+ int corr_timestamp = state->timestamp_offset + timestamp; + tsdelta = + (int32_t)(corr_timestamp - state->last_timestamp) / + (int16_t)(seq - state->max_seq);
great!
+ /* Check again, whether the timestamps are still valid */ + if (last_tsdelta && seq != state->max_seq) {
seq == state->max_seq => retransmission or just the guard against the division by 0?
+ tsdelta = (timestamp - state->last_timestamp) / + (seq - state->max_seq);
okay subtle change as the timestamp_offset has already been applied? My idea of taking this code out might be difficult. Can you try anyway?
On 11/21/2013 08:30 PM, Holger Hans Peter Freyther wrote:
On Thu, Nov 21, 2013 at 07:05:43PM +0100, Jacob Erlbeck wrote:
Thank you for the patch. I have one wish for you to move the code to separate method (with the side effect on the data structure).
Ok, see remark at the end of the mail.
+ } else {
I would prefer if we could move this code into another method and re-use it later on. Sure this requires to pass "Input"/"Output" into the function and the right counters. But I think I prefer this over the code clone.
You just happened to be late to if/elseif/else and I think it is getting a bit crowded in this method.
It does ;-)
+ /* Check again, whether the timestamps are still valid */ + if (last_tsdelta && seq != state->max_seq) {
seq == state->max_seq => retransmission or just the guard against the division by 0?
The latter, but dSeqNo == 0 => dTS == 0 should be checked, too.
+ tsdelta = (timestamp - state->last_timestamp) / + (seq - state->max_seq);
okay subtle change as the timestamp_offset has already been applied? My idea of taking this code out might be difficult. Can you try anyway?
Yes, I didn't find a clean solution without code duplication yesterday. But I think it should be possible by moving this into a separate function which does checking and tsdelta computation. This makes it easier to improve the tests (e.g. the dSeqNo check above or more complex packet duration stuff) and should help to clean up mgcp_patch_and_count, too. I'll update the patch accordingly. Jacob
On 11/21/2013 08:30 PM, Holger Hans Peter Freyther wrote:
On Thu, Nov 21, 2013 at 07:05:43PM +0100, Jacob Erlbeck wrote:
Thank you for the patch. I have one wish for you to move the code to separate method (with the side effect on the data structure).
I also would like to have global mgcp (or per trunk) summary counters (the same 4 values), preferably as rate counters. But I'd rather put that into another patch. Jacob
This patch modifies the patch_and_count() function to check for RTP timestamp inconsistencies. It basically checks, whether dTS/dSeqNo remains constant. If this fails, the corresponding counter is incremented. There are four counter for this: Incoming and outgoing, each for streams from the BTS and the net. Note that this approach presumes, that the per RTP packet duration (in samples) remains the same throughout the entire stream. Changing the number of speech frames per channel and packet will be detected as error. In addition, the VTY command 'show mgcp' is extended by an optional 'stats' to show the counter values, too. Ticket: OW#964 Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/mgcp_internal.h | 14 +++- openbsc/src/libmgcp/mgcp_network.c | 126 ++++++++++++++++++++++++++----- openbsc/src/libmgcp/mgcp_protocol.c | 24 +++++- openbsc/src/libmgcp/mgcp_vty.c | 23 ++++-- openbsc/tests/mgcp/mgcp_test.c | 5 +- 5 files changed, 160 insertions(+), 32 deletions(-) diff --git a/openbsc/include/openbsc/mgcp_internal.h b/openbsc/include/openbsc/mgcp_internal.h index d5bd3dd..8b6a56b 100644 --- a/openbsc/include/openbsc/mgcp_internal.h +++ b/openbsc/include/openbsc/mgcp_internal.h @@ -40,22 +40,30 @@ enum mgcp_trunk_type { MGCP_TRUNK_E1, }; +struct mgcp_rtp_stream_state { + uint32_t ssrc; + uint16_t last_seq; + uint32_t last_timestamp; + uint32_t err_ts_counter; + int32_t last_tsdelta; +}; + struct mgcp_rtp_state { int initialized; int patch; uint32_t orig_ssrc; - uint32_t ssrc; uint16_t base_seq; - uint16_t max_seq; int seq_offset; int cycles; - uint32_t last_timestamp; int32_t timestamp_offset; uint32_t jitter; int32_t transit; + + struct mgcp_rtp_stream_state in_stream; + struct mgcp_rtp_stream_state out_stream; }; struct mgcp_rtp_end { diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index 2b55527..ba84956 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -134,6 +134,73 @@ int mgcp_send_dummy(struct mgcp_endpoint *endp) endp->net_end.rtp_port, buf, 1); } +static int check_rtp_timestamp(struct mgcp_endpoint *endp, + struct mgcp_rtp_stream_state *state, + struct mgcp_rtp_end *rtp_end, + struct sockaddr_in *addr, + uint16_t seq, uint32_t timestamp, + const char *text, int32_t *tsdelta_out) +{ + int32_t tsdelta; + + if (state->last_tsdelta == 0 && timestamp == state->last_timestamp) + /* Not fully intialized, skip */ + return 0; + + if (seq == state->last_seq) { + if (timestamp != state->last_timestamp) { + state->err_ts_counter += 1; + LOGP(DMGCP, LOGL_ERROR, + "The %s timestamp delta is != 0 but the sequence " + "number %d is the same" + "on 0x%x SSRC: %u timestamp: %u " + "from %s:%d in %d\n", + text, seq, + ENDPOINT_NUMBER(endp), state->ssrc, timestamp, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); + } + return 0; + } + + tsdelta = + (int32_t)(timestamp - state->last_timestamp) / + (int16_t)(seq - state->last_seq); + + if (tsdelta == 0) { + state->err_ts_counter += 1; + LOGP(DMGCP, LOGL_ERROR, + "The %s timestamp delta is %d " + "on 0x%x SSRC: %u timestamp: %u " + "from %s:%d in %d\n", + text, tsdelta, + ENDPOINT_NUMBER(endp), state->ssrc, timestamp, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); + + return 0; + } + + if (state->last_tsdelta != tsdelta) { + if (state->last_tsdelta) { + state->err_ts_counter += 1; + LOGP(DMGCP, LOGL_ERROR, + "The %s timestamp delta changes from %d to %d " + "on 0x%x SSRC: %u timestamp: %u from %s:%d in %d\n", + text, state->last_tsdelta, tsdelta, + ENDPOINT_NUMBER(endp), state->ssrc, timestamp, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); + } + } + + if (tsdelta_out) + *tsdelta_out = tsdelta; + + return 1; +} + + /** * The RFC 3550 Appendix A assumes there are multiple sources but * some of the supported endpoints (e.g. the nanoBTS) can only handle @@ -143,13 +210,15 @@ int mgcp_send_dummy(struct mgcp_endpoint *endp) * we receive will be seen as a switch in streams. */ static void patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *state, - int payload, struct sockaddr_in *addr, char *data, int len) + struct mgcp_rtp_end *rtp_end, struct sockaddr_in *addr, + char *data, int len) { uint32_t arrival_time; int32_t transit, d; uint16_t seq, udelta; uint32_t timestamp; struct rtp_hdr *rtp_hdr; + int payload = rtp_end->payload_type; if (len < sizeof(*rtp_hdr)) return; @@ -160,24 +229,37 @@ static void patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *s arrival_time = get_current_ts(); if (!state->initialized) { + state->in_stream.last_seq = seq - 1; + state->in_stream.ssrc = state->orig_ssrc = rtp_hdr->ssrc; + state->in_stream.last_tsdelta = 0; state->base_seq = seq; - state->max_seq = seq - 1; - state->ssrc = state->orig_ssrc = rtp_hdr->ssrc; state->initialized = 1; - state->last_timestamp = timestamp; state->jitter = 0; state->transit = arrival_time - timestamp; - } else if (state->ssrc != rtp_hdr->ssrc) { - state->ssrc = rtp_hdr->ssrc; - state->seq_offset = (state->max_seq + 1) - seq; - state->timestamp_offset = state->last_timestamp - timestamp; + state->out_stream = state->in_stream; + } else if (state->in_stream.ssrc != rtp_hdr->ssrc) { + state->in_stream.ssrc = rtp_hdr->ssrc; + state->seq_offset = (state->out_stream.last_seq + 1) - seq; + state->timestamp_offset = state->out_stream.last_timestamp - timestamp; state->patch = endp->allow_patch; LOGP(DMGCP, LOGL_NOTICE, "The SSRC changed on 0x%x SSRC: %u offset: %d from %s:%d in %d\n", - ENDPOINT_NUMBER(endp), state->ssrc, state->seq_offset, - inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), endp->conn_mode); + ENDPOINT_NUMBER(endp), state->in_stream.ssrc, + state->seq_offset, inet_ntoa(addr->sin_addr), + ntohs(addr->sin_port), endp->conn_mode); + + state->in_stream.last_tsdelta = 0; + } else { + /* Compute current per-packet timestamp delta */ + check_rtp_timestamp(endp, &state->in_stream, rtp_end, addr, + seq, timestamp, "input", + &state->in_stream.last_tsdelta); } + /* Save before patching */ + state->in_stream.last_timestamp = timestamp; + state->in_stream.last_seq = seq; + /* apply the offset and store it back to the packet */ if (state->patch) { seq += state->seq_offset; @@ -188,14 +270,21 @@ static void patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *s rtp_hdr->timestamp = htonl(timestamp); } + /* Check again, whether the timestamps are still valid */ + check_rtp_timestamp(endp, &state->out_stream, rtp_end, addr, + seq, timestamp, "output", + &state->out_stream.last_tsdelta); + /* * The below takes the shape of the validation from Appendix A. Check * if there is something weird with the sequence number, otherwise check * for a wrap around in the sequence number. + * + * Note that last_seq is used where the appendix mentions max_seq. */ - udelta = seq - state->max_seq; + udelta = seq - state->out_stream.last_seq; if (udelta < RTP_MAX_DROPOUT) { - if (seq < state->max_seq) + if (seq < state->out_stream.last_seq) state->cycles += RTP_SEQ_MOD; } else if (udelta <= RTP_SEQ_MOD - RTP_MAX_MISORDER) { LOGP(DMGCP, LOGL_NOTICE, @@ -215,9 +304,10 @@ static void patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *s d = -d; state->jitter += d - ((state->jitter + 8) >> 4); - - state->max_seq = seq; - state->last_timestamp = timestamp; + /* Save output values */ + state->out_stream.last_seq = seq; + state->out_stream.last_timestamp = timestamp; + state->out_stream.ssrc = rtp_hdr->ssrc; if (payload < 0) return; @@ -280,7 +370,7 @@ static int mgcp_send(struct mgcp_endpoint *endp, int dest, int is_rtp, if (dest == MGCP_DEST_NET) { if (is_rtp) { patch_and_count(endp, &endp->bts_state, - endp->net_end.payload_type, + &endp->net_end, addr, buf, rc); forward_data(endp->net_end.rtp.fd, &endp->taps[MGCP_TAP_NET_OUT], buf, rc); @@ -295,7 +385,7 @@ static int mgcp_send(struct mgcp_endpoint *endp, int dest, int is_rtp, } else { if (is_rtp) { patch_and_count(endp, &endp->net_state, - endp->bts_end.payload_type, + &endp->bts_end, addr, buf, rc); forward_data(endp->bts_end.rtp.fd, &endp->taps[MGCP_TAP_BTS_OUT], buf, rc); @@ -684,7 +774,7 @@ void mgcp_state_calc_loss(struct mgcp_rtp_state *state, struct mgcp_rtp_end *end, uint32_t *expected, int *loss) { - *expected = state->cycles + state->max_seq; + *expected = state->cycles + state->out_stream.last_seq; *expected = *expected - state->base_seq + 1; if (!state->initialized) { diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index 616e0a9..6d22cf3 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -1163,14 +1163,30 @@ void mgcp_format_stats(struct mgcp_endpoint *endp, char *msg, size_t size) { uint32_t expected, jitter; int ploss; + int nchars; mgcp_state_calc_loss(&endp->net_state, &endp->net_end, &expected, &ploss); jitter = mgcp_state_calc_jitter(&endp->net_state); - snprintf(msg, size, "\r\nP: PS=%u, OS=%u, PR=%u, OR=%u, PL=%d, JI=%u", - endp->bts_end.packets, endp->bts_end.octets, - endp->net_end.packets, endp->net_end.octets, - ploss, jitter); + nchars = snprintf(msg, size, + "\r\nP: PS=%u, OS=%u, PR=%u, OR=%u, PL=%d, JI=%u", + endp->bts_end.packets, endp->bts_end.octets, + endp->net_end.packets, endp->net_end.octets, + ploss, jitter); + if (nchars < 0 || nchars >= size) + goto truncate; + + msg += nchars; + size -= nchars; + + /* Error Counter */ + snprintf(msg, size, + "\r\nX-Osmo: EC TIS=%u, TOS=%u, TIR=%u, TOR=%u", + endp->net_state.in_stream.err_ts_counter, + endp->net_state.out_stream.err_ts_counter, + endp->bts_state.in_stream.err_ts_counter, + endp->bts_state.out_stream.err_ts_counter); +truncate: msg[size - 1] = '\0'; } diff --git a/openbsc/src/libmgcp/mgcp_vty.c b/openbsc/src/libmgcp/mgcp_vty.c index 3c239d8..5aeb393 100644 --- a/openbsc/src/libmgcp/mgcp_vty.c +++ b/openbsc/src/libmgcp/mgcp_vty.c @@ -114,7 +114,7 @@ static int config_write_mgcp(struct vty *vty) return CMD_SUCCESS; } -static void dump_trunk(struct vty *vty, struct mgcp_trunk_config *cfg) +static void dump_trunk(struct vty *vty, struct mgcp_trunk_config *cfg, int verbose) { int i; @@ -139,18 +139,31 @@ static void dump_trunk(struct vty *vty, struct mgcp_trunk_config *cfg) endp->bts_end.packets, endp->net_end.packets, endp->trans_net.packets, endp->trans_bts.packets, VTY_NEWLINE); + + if (verbose) + vty_out(vty, + " Timestamp Errs: BTS %d->%d, Net %d->%d%s", + endp->bts_state.in_stream.err_ts_counter, + endp->bts_state.out_stream.err_ts_counter, + endp->net_state.in_stream.err_ts_counter, + endp->net_state.out_stream.err_ts_counter, + VTY_NEWLINE); } } -DEFUN(show_mcgp, show_mgcp_cmd, "show mgcp", - SHOW_STR "Display information about the MGCP Media Gateway") +DEFUN(show_mcgp, show_mgcp_cmd, + "show mgcp [stats]", + SHOW_STR + "Display information about the MGCP Media Gateway\n" + "Include Statistics\n") { struct mgcp_trunk_config *trunk; + int show_stats = argc >= 1; - dump_trunk(vty, &g_cfg->trunk); + dump_trunk(vty, &g_cfg->trunk, show_stats); llist_for_each_entry(trunk, &g_cfg->trunks, entry) - dump_trunk(vty, trunk); + dump_trunk(vty, trunk, show_stats); return CMD_SUCCESS; } diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index 5565e73..e5e6245 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -97,7 +97,8 @@ "C: 2\r\n" #define DLCX_RET "250 7 OK\r\n" \ - "P: PS=0, OS=0, PR=0, OR=0, PL=0, JI=0\r\n" + "P: PS=0, OS=0, PR=0, OR=0, PL=0, JI=0\r\n" \ + "X-Osmo: EC TIS=0, TOS=0, TIR=0, TOR=0\r\n" #define RQNT "RQNT 186908780 1@mgw MGCP 1.0\r\n" \ "X: B244F267488\r\n" \ @@ -309,7 +310,7 @@ static void test_packet_loss_calc(void) state.initialized = 1; state.base_seq = pl_test_dat[i].base_seq; - state.max_seq = pl_test_dat[i].max_seq; + state.out_stream.last_seq = pl_test_dat[i].max_seq; state.cycles = pl_test_dat[i].cycles; rtp.packets = pl_test_dat[i].packets; -- 1.7.9.5
This patch adds a test case to check, whether RTP timestamps are generated properly after SSRC changes and whether the error counters work properly. Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_network.c | 18 +++--- openbsc/tests/mgcp/mgcp_test.c | 106 ++++++++++++++++++++++++++++++++++++ openbsc/tests/mgcp/mgcp_test.ok | 16 ++++++ 3 files changed, 131 insertions(+), 9 deletions(-) diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index ba84956..fa10cec 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -209,9 +209,9 @@ static int check_rtp_timestamp(struct mgcp_endpoint *endp, * There is also no probation period for new sources. Every package * we receive will be seen as a switch in streams. */ -static void patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *state, - struct mgcp_rtp_end *rtp_end, struct sockaddr_in *addr, - char *data, int len) +void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *state, + struct mgcp_rtp_end *rtp_end, struct sockaddr_in *addr, + char *data, int len) { uint32_t arrival_time; int32_t transit, d; @@ -369,9 +369,9 @@ static int mgcp_send(struct mgcp_endpoint *endp, int dest, int is_rtp, if (dest == MGCP_DEST_NET) { if (is_rtp) { - patch_and_count(endp, &endp->bts_state, - &endp->net_end, - addr, buf, rc); + mgcp_patch_and_count(endp, &endp->bts_state, + &endp->net_end, + addr, buf, rc); forward_data(endp->net_end.rtp.fd, &endp->taps[MGCP_TAP_NET_OUT], buf, rc); return mgcp_udp_send(endp->net_end.rtp.fd, @@ -384,9 +384,9 @@ static int mgcp_send(struct mgcp_endpoint *endp, int dest, int is_rtp, } } else { if (is_rtp) { - patch_and_count(endp, &endp->net_state, - &endp->bts_end, - addr, buf, rc); + mgcp_patch_and_count(endp, &endp->net_state, + &endp->bts_end, + addr, buf, rc); forward_data(endp->bts_end.rtp.fd, &endp->taps[MGCP_TAP_BTS_OUT], buf, rc); return mgcp_udp_send(endp->bts_end.rtp.fd, diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index e5e6245..d3c5687 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -348,6 +348,111 @@ static void test_mgcp_stats(void) msgb_free(msg); } +struct rtp_packet_info { + float txtime; + int len; + char *data; +}; + +struct rtp_packet_info test_rtp_packets1[] = { + /* RTP: SeqNo=0, TS=0 */ + {0.000000, 20, "\x80\x62\x00\x00\x00\x00\x00\x00\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=1, TS=160 */ + {0.020000, 20, "\x80\x62\x00\x01\x00\x00\x00\xA0\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=2, TS=320 */ + {0.040000, 20, "\x80\x62\x00\x02\x00\x00\x01\x40\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* Repeat RTP timestamp: */ + /* RTP: SeqNo=3, TS=320 */ + {0.060000, 20, "\x80\x62\x00\x03\x00\x00\x01\x40\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=4, TS=480 */ + {0.080000, 20, "\x80\x62\x00\x04\x00\x00\x01\xE0\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=5, TS=640 */ + {0.100000, 20, "\x80\x62\x00\x05\x00\x00\x02\x80\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* Double skip RTP timestamp (delta = 2*160): */ + /* RTP: SeqNo=6, TS=960 */ + {0.120000, 20, "\x80\x62\x00\x06\x00\x00\x03\xC0\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=7, TS=1120 */ + {0.140000, 20, "\x80\x62\x00\x07\x00\x00\x04\x60\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=8, TS=1280 */ + {0.160000, 20, "\x80\x62\x00\x08\x00\x00\x05\x00\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* Non 20ms RTP timestamp (delta = 120): */ + /* RTP: SeqNo=9, TS=1400 */ + {0.180000, 20, "\x80\x62\x00\x09\x00\x00\x05\x78\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=10, TS=1560 */ + {0.200000, 20, "\x80\x62\x00\x0A\x00\x00\x06\x18\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=11, TS=1720 */ + {0.220000, 20, "\x80\x62\x00\x0B\x00\x00\x06\xB8\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* SSRC changed to 0x10203040, RTP timestamp jump */ + /* RTP: SeqNo=12, TS=34688 */ + {0.240000, 20, "\x80\x62\x00\x0C\x00\x00\x87\x80\x10\x20\x30\x40" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=13, TS=34848 */ + {0.260000, 20, "\x80\x62\x00\x0D\x00\x00\x88\x20\x10\x20\x30\x40" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=14, TS=35008 */ + {0.280000, 20, "\x80\x62\x00\x0E\x00\x00\x88\xC0\x10\x20\x30\x40" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, +}; + +void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *state, + struct mgcp_rtp_end *rtp_end, struct sockaddr_in *addr, + char *data, int len); + +static void test_packet_error_detection(void) +{ + int i; + + struct mgcp_trunk_config trunk; + struct mgcp_endpoint endp; + struct mgcp_rtp_state state; + struct mgcp_rtp_end rtp; + struct sockaddr_in addr = {0}; + char buffer[4096]; + + printf("Testing packet error detection.\n"); + + memset(&trunk, 0, sizeof(trunk)); + memset(&endp, 0, sizeof(endp)); + memset(&state, 0, sizeof(state)); + memset(&rtp, 0, sizeof(rtp)); + + trunk.number_endpoints = 1; + trunk.endpoints = &endp; + + rtp.payload_type = 98; + endp.allow_patch = 1; + endp.tcfg = &trunk; + + for (i = 0; i < ARRAY_SIZE(test_rtp_packets1); ++i) { + struct rtp_packet_info *info = test_rtp_packets1 + i; + + OSMO_ASSERT(info->len <= sizeof(buffer)); + OSMO_ASSERT(info->len >= 0); + memmove(buffer, info->data, info->len); + + mgcp_patch_and_count(&endp, &state, &rtp, &addr, + buffer, info->len); + + printf("TS: %d, dTS: %d, TS Errs: in %d, out %d\n", + state.out_stream.last_timestamp, + state.out_stream.last_tsdelta, + state.in_stream.err_ts_counter, + state.out_stream.err_ts_counter); + } +} + int main(int argc, char **argv) { osmo_init_logging(&log_info); @@ -357,6 +462,7 @@ int main(int argc, char **argv) test_packet_loss_calc(); test_rqnt_cb(); test_mgcp_stats(); + test_packet_error_detection(); printf("Done\n"); return EXIT_SUCCESS; diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 8711e38..8c3fa26 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -28,4 +28,20 @@ Testing packet loss calculation. Testing stat parsing Parsing result: 0 Parsing result: 0 +Testing packet error detection. +TS: 0, dTS: 0, TS Errs: in 0, out 0 +TS: 160, dTS: 160, TS Errs: in 0, out 0 +TS: 320, dTS: 160, TS Errs: in 0, out 0 +TS: 320, dTS: 160, TS Errs: in 1, out 1 +TS: 480, dTS: 160, TS Errs: in 1, out 1 +TS: 640, dTS: 160, TS Errs: in 1, out 1 +TS: 960, dTS: 320, TS Errs: in 2, out 2 +TS: 1120, dTS: 160, TS Errs: in 3, out 3 +TS: 1280, dTS: 160, TS Errs: in 3, out 3 +TS: 1400, dTS: 120, TS Errs: in 4, out 4 +TS: 1560, dTS: 160, TS Errs: in 5, out 5 +TS: 1720, dTS: 160, TS Errs: in 5, out 5 +TS: 1720, dTS: 160, TS Errs: in 5, out 6 +TS: 1880, dTS: 160, TS Errs: in 5, out 6 +TS: 2040, dTS: 160, TS Errs: in 5, out 6 Done -- 1.7.9.5
The current implementation increments the seqno but does not increment the RTP timestamp, leading to two identical timestamps following one after the other. This patch fixes this by adding the computed tsdelta when the offset is calulated. In the unlikely case, that a tsdelta hasn't been computed yet when the SSRC changes, a tsdelta is computed based on the RTP rate and a RTP packet duration of 20ms (one speech frame per channel and packet). If the RTP rate is not known, a rate of 8000 is assumed. Note that this approach presumes, that the per RTP packet duration (in samples) is the same for the last two packets of the stream being replaced (the first one). Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/mgcp_internal.h | 5 +++++ openbsc/src/libmgcp/mgcp_network.c | 24 +++++++++++++++++++----- openbsc/src/libmgcp/mgcp_protocol.c | 14 +++++++++++++- openbsc/tests/mgcp/mgcp_test.c | 2 ++ openbsc/tests/mgcp/mgcp_test.ok | 6 +++--- 5 files changed, 42 insertions(+), 9 deletions(-) diff --git a/openbsc/include/openbsc/mgcp_internal.h b/openbsc/include/openbsc/mgcp_internal.h index 8b6a56b..c581c24 100644 --- a/openbsc/include/openbsc/mgcp_internal.h +++ b/openbsc/include/openbsc/mgcp_internal.h @@ -77,6 +77,10 @@ struct mgcp_rtp_end { /* per endpoint data */ int payload_type; + uint32_t rate; + uint32_t frame_duration_num; + uint32_t frame_duration_den; + int frames_per_packet; char *fmtp_extra; /* @@ -176,5 +180,6 @@ void mgcp_state_calc_loss(struct mgcp_rtp_state *s, struct mgcp_rtp_end *, uint32_t *expected, int *loss); uint32_t mgcp_state_calc_jitter(struct mgcp_rtp_state *); +void mgcp_rtp_end_init(struct mgcp_rtp_end *end); #endif diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index fa10cec..2fd9a80 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -78,7 +78,6 @@ struct rtp_hdr { #define RTP_MAX_DROPOUT 3000 #define RTP_MAX_MISORDER 100 - enum { MGCP_DEST_NET = 0, MGCP_DEST_BTS, @@ -238,15 +237,30 @@ void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *sta state->transit = arrival_time - timestamp; state->out_stream = state->in_stream; } else if (state->in_stream.ssrc != rtp_hdr->ssrc) { + int32_t tsdelta = state->out_stream.last_tsdelta; + if (tsdelta != 0) { + tsdelta = rtp_end->rate * rtp_end->frames_per_packet * + rtp_end->frame_duration_num / + rtp_end->frame_duration_den; + LOGP(DMGCP, LOGL_NOTICE, + "Computed timestamp delta %d based on " + "rate %d, num frames %d, frame duration %d/%d\n", + tsdelta, rtp_end->rate, rtp_end->frames_per_packet, + rtp_end->frame_duration_num, + rtp_end->frame_duration_den); + } state->in_stream.ssrc = rtp_hdr->ssrc; state->seq_offset = (state->out_stream.last_seq + 1) - seq; - state->timestamp_offset = state->out_stream.last_timestamp - timestamp; + state->timestamp_offset = + (state->out_stream.last_timestamp + tsdelta) - timestamp; state->patch = endp->allow_patch; LOGP(DMGCP, LOGL_NOTICE, - "The SSRC changed on 0x%x SSRC: %u offset: %d from %s:%d in %d\n", + "The SSRC changed on 0x%x SSRC: %u offset: %d tsdelta: %d " + "from %s:%d in %d\n", ENDPOINT_NUMBER(endp), state->in_stream.ssrc, - state->seq_offset, inet_ntoa(addr->sin_addr), - ntohs(addr->sin_port), endp->conn_mode); + state->seq_offset, tsdelta, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); state->in_stream.last_tsdelta = 0; } else { diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index 6d22cf3..ef1b666 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -40,6 +40,12 @@ for (line = strtok_r(NULL, "\r\n", &save); line;\ line = strtok_r(NULL, "\r\n", &save)) +/* Assume audio frame length of 20ms */ +#define DEFAULT_RTP_AUDIO_FRAME_DUR_NUM 20 +#define DEFAULT_RTP_AUDIO_FRAME_DUR_DEN 1000 +#define DEFAULT_RTP_AUDIO_FRAMES_PER_PACKET 1 +#define DEFAULT_RTP_AUDIO_DEFAULT_RATE 8000 + static void mgcp_rtp_end_reset(struct mgcp_rtp_end *end); struct mgcp_parse_data { @@ -967,11 +973,17 @@ static void mgcp_rtp_end_reset(struct mgcp_rtp_end *end) end->fmtp_extra = NULL; } -static void mgcp_rtp_end_init(struct mgcp_rtp_end *end) +void mgcp_rtp_end_init(struct mgcp_rtp_end *end) { mgcp_rtp_end_reset(end); end->rtp.fd = -1; end->rtcp.fd = -1; + + /* Set default values */ + end->frame_duration_num = DEFAULT_RTP_AUDIO_FRAME_DUR_NUM; + end->frame_duration_den = DEFAULT_RTP_AUDIO_FRAME_DUR_DEN; + end->frames_per_packet = DEFAULT_RTP_AUDIO_FRAMES_PER_PACKET; + end->rate = DEFAULT_RTP_AUDIO_DEFAULT_RATE; } int mgcp_endpoints_allocate(struct mgcp_trunk_config *tcfg) diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index d3c5687..b25619f 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -428,6 +428,8 @@ static void test_packet_error_detection(void) memset(&state, 0, sizeof(state)); memset(&rtp, 0, sizeof(rtp)); + mgcp_rtp_end_init(&rtp); + trunk.number_endpoints = 1; trunk.endpoints = &endp; diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 8c3fa26..5666424 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -41,7 +41,7 @@ TS: 1280, dTS: 160, TS Errs: in 3, out 3 TS: 1400, dTS: 120, TS Errs: in 4, out 4 TS: 1560, dTS: 160, TS Errs: in 5, out 5 TS: 1720, dTS: 160, TS Errs: in 5, out 5 -TS: 1720, dTS: 160, TS Errs: in 5, out 6 -TS: 1880, dTS: 160, TS Errs: in 5, out 6 -TS: 2040, dTS: 160, TS Errs: in 5, out 6 +TS: 1880, dTS: 160, TS Errs: in 5, out 5 +TS: 2040, dTS: 160, TS Errs: in 5, out 5 +TS: 2200, dTS: 160, TS Errs: in 5, out 5 Done -- 1.7.9.5
The current implementation increments the seqno but does not increment the RTP timestamp, leading to two identical timestamps following one after the other. This patch fixes this by adding the computed tsdelta when the offset is calulated. In the unlikely case, that a tsdelta hasn't been computed yet when the SSRC changes, a tsdelta is computed based on the RTP rate and a RTP packet duration of 20ms (one speech frame per channel and packet). If the RTP rate is not known, a rate of 8000 is assumed. Note that this approach presumes, that the per RTP packet duration (in samples) is the same for the last two packets of the stream being replaced (the first one). Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/mgcp_internal.h | 4 ++++ openbsc/src/libmgcp/mgcp_network.c | 23 +++++++++++++++++++---- openbsc/src/libmgcp/mgcp_protocol.c | 12 ++++++++++++ openbsc/tests/mgcp/mgcp_test.c | 16 +++++++++++----- openbsc/tests/mgcp/mgcp_test.ok | 6 +++--- 5 files changed, 49 insertions(+), 12 deletions(-) diff --git a/openbsc/include/openbsc/mgcp_internal.h b/openbsc/include/openbsc/mgcp_internal.h index 8b6a56b..0b52c1c 100644 --- a/openbsc/include/openbsc/mgcp_internal.h +++ b/openbsc/include/openbsc/mgcp_internal.h @@ -77,6 +77,10 @@ struct mgcp_rtp_end { /* per endpoint data */ int payload_type; + uint32_t rate; + uint32_t frame_duration_num; + uint32_t frame_duration_den; + int frames_per_packet; char *fmtp_extra; /* diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index fa10cec..8f7fd09 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -238,15 +238,30 @@ void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *sta state->transit = arrival_time - timestamp; state->out_stream = state->in_stream; } else if (state->in_stream.ssrc != rtp_hdr->ssrc) { + int32_t tsdelta = state->out_stream.last_tsdelta; + if (tsdelta != 0) { + tsdelta = rtp_end->rate * rtp_end->frames_per_packet * + rtp_end->frame_duration_num / + rtp_end->frame_duration_den; + LOGP(DMGCP, LOGL_NOTICE, + "Computed timestamp delta %d based on " + "rate %d, num frames %d, frame duration %d/%d\n", + tsdelta, rtp_end->rate, rtp_end->frames_per_packet, + rtp_end->frame_duration_num, + rtp_end->frame_duration_den); + } state->in_stream.ssrc = rtp_hdr->ssrc; state->seq_offset = (state->out_stream.last_seq + 1) - seq; - state->timestamp_offset = state->out_stream.last_timestamp - timestamp; + state->timestamp_offset = + (state->out_stream.last_timestamp + tsdelta) - timestamp; state->patch = endp->allow_patch; LOGP(DMGCP, LOGL_NOTICE, - "The SSRC changed on 0x%x SSRC: %u offset: %d from %s:%d in %d\n", + "The SSRC changed on 0x%x SSRC: %u offset: %d tsdelta: %d " + "from %s:%d in %d\n", ENDPOINT_NUMBER(endp), state->in_stream.ssrc, - state->seq_offset, inet_ntoa(addr->sin_addr), - ntohs(addr->sin_port), endp->conn_mode); + state->seq_offset, tsdelta, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); state->in_stream.last_tsdelta = 0; } else { diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index 6d22cf3..71fb89d 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -40,6 +40,12 @@ for (line = strtok_r(NULL, "\r\n", &save); line;\ line = strtok_r(NULL, "\r\n", &save)) +/* Assume audio frame length of 20ms */ +#define DEFAULT_RTP_AUDIO_FRAME_DUR_NUM 20 +#define DEFAULT_RTP_AUDIO_FRAME_DUR_DEN 1000 +#define DEFAULT_RTP_AUDIO_FRAMES_PER_PACKET 1 +#define DEFAULT_RTP_AUDIO_DEFAULT_RATE 8000 + static void mgcp_rtp_end_reset(struct mgcp_rtp_end *end); struct mgcp_parse_data { @@ -965,6 +971,12 @@ static void mgcp_rtp_end_reset(struct mgcp_rtp_end *end) end->local_alloc = -1; talloc_free(end->fmtp_extra); end->fmtp_extra = NULL; + + /* Set default values */ + end->frame_duration_num = DEFAULT_RTP_AUDIO_FRAME_DUR_NUM; + end->frame_duration_den = DEFAULT_RTP_AUDIO_FRAME_DUR_DEN; + end->frames_per_packet = DEFAULT_RTP_AUDIO_FRAMES_PER_PACKET; + end->rate = DEFAULT_RTP_AUDIO_DEFAULT_RATE; } static void mgcp_rtp_end_init(struct mgcp_rtp_end *end) diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index d3c5687..84c55aa 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -417,7 +417,7 @@ static void test_packet_error_detection(void) struct mgcp_trunk_config trunk; struct mgcp_endpoint endp; struct mgcp_rtp_state state; - struct mgcp_rtp_end rtp; + struct mgcp_rtp_end *rtp = &endp.net_end; struct sockaddr_in addr = {0}; char buffer[4096]; @@ -426,14 +426,20 @@ static void test_packet_error_detection(void) memset(&trunk, 0, sizeof(trunk)); memset(&endp, 0, sizeof(endp)); memset(&state, 0, sizeof(state)); - memset(&rtp, 0, sizeof(rtp)); trunk.number_endpoints = 1; trunk.endpoints = &endp; + endp.tcfg = &trunk; + + /* This doesn't free endp but resets/frees all fields of the structure + * and invokes mgcp_rtp_end_reset() for each mgcp_rtp_end. OTOH, it + * expects valid pointer fields (either NULL or talloc'ed), so the + * memset is still needed. It also requires that endp.tcfg and + * trunk.endpoints are set up properly. */ + mgcp_free_endp(&endp); - rtp.payload_type = 98; + rtp->payload_type = 98; endp.allow_patch = 1; - endp.tcfg = &trunk; for (i = 0; i < ARRAY_SIZE(test_rtp_packets1); ++i) { struct rtp_packet_info *info = test_rtp_packets1 + i; @@ -442,7 +448,7 @@ static void test_packet_error_detection(void) OSMO_ASSERT(info->len >= 0); memmove(buffer, info->data, info->len); - mgcp_patch_and_count(&endp, &state, &rtp, &addr, + mgcp_patch_and_count(&endp, &state, rtp, &addr, buffer, info->len); printf("TS: %d, dTS: %d, TS Errs: in %d, out %d\n", diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 8c3fa26..5666424 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -41,7 +41,7 @@ TS: 1280, dTS: 160, TS Errs: in 3, out 3 TS: 1400, dTS: 120, TS Errs: in 4, out 4 TS: 1560, dTS: 160, TS Errs: in 5, out 5 TS: 1720, dTS: 160, TS Errs: in 5, out 5 -TS: 1720, dTS: 160, TS Errs: in 5, out 6 -TS: 1880, dTS: 160, TS Errs: in 5, out 6 -TS: 2040, dTS: 160, TS Errs: in 5, out 6 +TS: 1880, dTS: 160, TS Errs: in 5, out 5 +TS: 2040, dTS: 160, TS Errs: in 5, out 5 +TS: 2200, dTS: 160, TS Errs: in 5, out 5 Done -- 1.7.9.5
This patch adds a test case to check, whether RTP timestamps are generated properly after SSRC changes and whether the error counters work properly. Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_network.c | 18 +++--- openbsc/tests/mgcp/mgcp_test.c | 106 ++++++++++++++++++++++++++++++++++++ openbsc/tests/mgcp/mgcp_test.ok | 16 ++++++ 3 files changed, 131 insertions(+), 9 deletions(-) diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index 14bc024..17e3d87 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -142,9 +142,9 @@ int mgcp_send_dummy(struct mgcp_endpoint *endp) * There is also no probation period for new sources. Every package * we receive will be seen as a switch in streams. */ -static void patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *state, - struct mgcp_rtp_end *rtp_end, struct sockaddr_in *addr, - char *data, int len) +void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *state, + struct mgcp_rtp_end *rtp_end, struct sockaddr_in *addr, + char *data, int len) { uint32_t arrival_time; int32_t transit, d; @@ -346,9 +346,9 @@ static int mgcp_send(struct mgcp_endpoint *endp, int dest, int is_rtp, if (dest == MGCP_DEST_NET) { if (is_rtp) { - patch_and_count(endp, &endp->bts_state, - &endp->net_end, - addr, buf, rc); + mgcp_patch_and_count(endp, &endp->bts_state, + &endp->net_end, + addr, buf, rc); forward_data(endp->net_end.rtp.fd, &endp->taps[MGCP_TAP_NET_OUT], buf, rc); return mgcp_udp_send(endp->net_end.rtp.fd, @@ -361,9 +361,9 @@ static int mgcp_send(struct mgcp_endpoint *endp, int dest, int is_rtp, } } else { if (is_rtp) { - patch_and_count(endp, &endp->net_state, - &endp->bts_end, - addr, buf, rc); + mgcp_patch_and_count(endp, &endp->net_state, + &endp->bts_end, + addr, buf, rc); forward_data(endp->bts_end.rtp.fd, &endp->taps[MGCP_TAP_BTS_OUT], buf, rc); return mgcp_udp_send(endp->bts_end.rtp.fd, diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index 3499ee3..705db80 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -348,6 +348,111 @@ static void test_mgcp_stats(void) msgb_free(msg); } +struct rtp_packet_info { + float txtime; + int len; + char *data; +}; + +struct rtp_packet_info test_rtp_packets1[] = { + /* RTP: SeqNo=0, TS=0 */ + {0.000000, 20, "\x80\x62\x00\x00\x00\x00\x00\x00\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=1, TS=160 */ + {0.020000, 20, "\x80\x62\x00\x01\x00\x00\x00\xA0\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=2, TS=320 */ + {0.040000, 20, "\x80\x62\x00\x02\x00\x00\x01\x40\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* Repeat RTP timestamp: */ + /* RTP: SeqNo=3, TS=320 */ + {0.060000, 20, "\x80\x62\x00\x03\x00\x00\x01\x40\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=4, TS=480 */ + {0.080000, 20, "\x80\x62\x00\x04\x00\x00\x01\xE0\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=5, TS=640 */ + {0.100000, 20, "\x80\x62\x00\x05\x00\x00\x02\x80\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* Double skip RTP timestamp (delta = 2*160): */ + /* RTP: SeqNo=6, TS=960 */ + {0.120000, 20, "\x80\x62\x00\x06\x00\x00\x03\xC0\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=7, TS=1120 */ + {0.140000, 20, "\x80\x62\x00\x07\x00\x00\x04\x60\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=8, TS=1280 */ + {0.160000, 20, "\x80\x62\x00\x08\x00\x00\x05\x00\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* Non 20ms RTP timestamp (delta = 120): */ + /* RTP: SeqNo=9, TS=1400 */ + {0.180000, 20, "\x80\x62\x00\x09\x00\x00\x05\x78\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=10, TS=1560 */ + {0.200000, 20, "\x80\x62\x00\x0A\x00\x00\x06\x18\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=11, TS=1720 */ + {0.220000, 20, "\x80\x62\x00\x0B\x00\x00\x06\xB8\x11\x22\x33\x44" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* SSRC changed to 0x10203040, RTP timestamp jump */ + /* RTP: SeqNo=12, TS=34688 */ + {0.240000, 20, "\x80\x62\x00\x0C\x00\x00\x87\x80\x10\x20\x30\x40" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=13, TS=34848 */ + {0.260000, 20, "\x80\x62\x00\x0D\x00\x00\x88\x20\x10\x20\x30\x40" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, + /* RTP: SeqNo=14, TS=35008 */ + {0.280000, 20, "\x80\x62\x00\x0E\x00\x00\x88\xC0\x10\x20\x30\x40" + "\x01\x23\x45\x67\x89\xAB\xCD\xEF"}, +}; + +void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *state, + struct mgcp_rtp_end *rtp_end, struct sockaddr_in *addr, + char *data, int len); + +static void test_packet_error_detection(void) +{ + int i; + + struct mgcp_trunk_config trunk; + struct mgcp_endpoint endp; + struct mgcp_rtp_state state; + struct mgcp_rtp_end rtp; + struct sockaddr_in addr = {0}; + char buffer[4096]; + + printf("Testing packet error detection.\n"); + + memset(&trunk, 0, sizeof(trunk)); + memset(&endp, 0, sizeof(endp)); + memset(&state, 0, sizeof(state)); + memset(&rtp, 0, sizeof(rtp)); + + trunk.number_endpoints = 1; + trunk.endpoints = &endp; + + rtp.payload_type = 98; + endp.allow_patch = 1; + endp.tcfg = &trunk; + + for (i = 0; i < ARRAY_SIZE(test_rtp_packets1); ++i) { + struct rtp_packet_info *info = test_rtp_packets1 + i; + + OSMO_ASSERT(info->len <= sizeof(buffer)); + OSMO_ASSERT(info->len >= 0); + memmove(buffer, info->data, info->len); + + mgcp_patch_and_count(&endp, &state, &rtp, &addr, + buffer, info->len); + + printf("TS: %d, dTS: %d, TS Errs: in %d, out %d\n", + state.last_timestamp, + state.last_tsdelta, + state.err_ts_in_counter, + state.err_ts_out_counter); + } +} + int main(int argc, char **argv) { osmo_init_logging(&log_info); @@ -357,6 +462,7 @@ int main(int argc, char **argv) test_packet_loss_calc(); test_rqnt_cb(); test_mgcp_stats(); + test_packet_error_detection(); printf("Done\n"); return EXIT_SUCCESS; diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 8711e38..8c3fa26 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -28,4 +28,20 @@ Testing packet loss calculation. Testing stat parsing Parsing result: 0 Parsing result: 0 +Testing packet error detection. +TS: 0, dTS: 0, TS Errs: in 0, out 0 +TS: 160, dTS: 160, TS Errs: in 0, out 0 +TS: 320, dTS: 160, TS Errs: in 0, out 0 +TS: 320, dTS: 160, TS Errs: in 1, out 1 +TS: 480, dTS: 160, TS Errs: in 1, out 1 +TS: 640, dTS: 160, TS Errs: in 1, out 1 +TS: 960, dTS: 320, TS Errs: in 2, out 2 +TS: 1120, dTS: 160, TS Errs: in 3, out 3 +TS: 1280, dTS: 160, TS Errs: in 3, out 3 +TS: 1400, dTS: 120, TS Errs: in 4, out 4 +TS: 1560, dTS: 160, TS Errs: in 5, out 5 +TS: 1720, dTS: 160, TS Errs: in 5, out 5 +TS: 1720, dTS: 160, TS Errs: in 5, out 6 +TS: 1880, dTS: 160, TS Errs: in 5, out 6 +TS: 2040, dTS: 160, TS Errs: in 5, out 6 Done -- 1.7.9.5
The current implementation increments the seqno but does not increment the RTP timestamp, leading to two identical timestamps following one after the other. This patch fixes this by adding the computed tsdelta when the offset is calulated. In the unlikely case, that a tsdelta hasn't been computed yet when the SSRC changes, a tsdelta is computed based on the RTP rate and a RTP packet duration of 20ms (one speech frame per channel and packet). If the RTP rate is not known, a rate of 8000 is assumed. Note that this approach presumes, that the per RTP packet duration (in samples) is the same for the last two packets of the stream being replaced (the first one). Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/mgcp_internal.h | 1 + openbsc/src/libmgcp/mgcp_network.c | 33 ++++++++++++++++++++++++++++--- openbsc/tests/mgcp/mgcp_test.ok | 6 +++--- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/openbsc/include/openbsc/mgcp_internal.h b/openbsc/include/openbsc/mgcp_internal.h index 2ad7058..b9bff53 100644 --- a/openbsc/include/openbsc/mgcp_internal.h +++ b/openbsc/include/openbsc/mgcp_internal.h @@ -73,6 +73,7 @@ struct mgcp_rtp_end { /* per endpoint data */ int payload_type; + uint32_t rate; char *fmtp_extra; /* diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index 17e3d87..2ac873e 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -78,6 +78,10 @@ struct rtp_hdr { #define RTP_MAX_DROPOUT 3000 #define RTP_MAX_MISORDER 100 +/* Assume audio frame length of 20ms */ +#define RTP_AUDIO_FRAME_DUR_NUM 20 +#define RTP_AUDIO_FRAME_DUR_DEN 1000 +#define RTP_AUDIO_DEFAULT_RATE 8000 enum { MGCP_DEST_NET = 0, @@ -173,14 +177,37 @@ void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *sta state->jitter = 0; state->transit = arrival_time - timestamp; } else if (state->ssrc != rtp_hdr->ssrc) { + tsdelta = state->last_tsdelta; + tsdelta_valid = tsdelta != 0; + if (!tsdelta_valid) { + int rate = rtp_end->rate; + if (!rate) { + rate = RTP_AUDIO_DEFAULT_RATE; + LOGP(DMGCP, LOGL_NOTICE, + "Using default RTP rate %d which might be " + "different from the rate contained in the " + "SDP data.\n", + rate + ); + } + tsdelta = rate * RTP_AUDIO_FRAME_DUR_NUM / + RTP_AUDIO_FRAME_DUR_DEN; + state->last_tsdelta = tsdelta; + tsdelta_valid = 1; + } + state->ssrc = rtp_hdr->ssrc; state->seq_offset = (state->max_seq + 1) - seq; - state->timestamp_offset = state->last_timestamp - timestamp; + state->timestamp_offset = (state->last_timestamp + tsdelta) - + timestamp; state->patch = endp->allow_patch; LOGP(DMGCP, LOGL_NOTICE, - "The SSRC changed on 0x%x SSRC: %u offset: %d from %s:%d in %d\n", + "The SSRC changed on 0x%x SSRC: %u offset: %d tsdelta: %d " + "from %s:%d in %d\n", ENDPOINT_NUMBER(endp), state->ssrc, state->seq_offset, - inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), endp->conn_mode); + tsdelta, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); } else { /* Compute current per-packet timestamp delta */ if (state->initialized && seq != state->max_seq) { diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 8c3fa26..5666424 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -41,7 +41,7 @@ TS: 1280, dTS: 160, TS Errs: in 3, out 3 TS: 1400, dTS: 120, TS Errs: in 4, out 4 TS: 1560, dTS: 160, TS Errs: in 5, out 5 TS: 1720, dTS: 160, TS Errs: in 5, out 5 -TS: 1720, dTS: 160, TS Errs: in 5, out 6 -TS: 1880, dTS: 160, TS Errs: in 5, out 6 -TS: 2040, dTS: 160, TS Errs: in 5, out 6 +TS: 1880, dTS: 160, TS Errs: in 5, out 5 +TS: 2040, dTS: 160, TS Errs: in 5, out 5 +TS: 2200, dTS: 160, TS Errs: in 5, out 5 Done -- 1.7.9.5
On Thu, Nov 21, 2013 at 07:05:45PM +0100, Jacob Erlbeck wrote:
The current implementation increments the seqno but does not increment the RTP timestamp, leading to two identical timestamps following one after the other.
thanks for spotting this!
+/* Assume audio frame length of 20ms */ +#define RTP_AUDIO_FRAME_DUR_NUM 20 +#define RTP_AUDIO_FRAME_DUR_DEN 1000 +#define RTP_AUDIO_DEFAULT_RATE 8000
can't we initialize the rate in mgcp_rtp_end_init or such? This way we know that this is either default or (read from the SDP file).
state->patch = endp->allow_patch; -TS: 2040, dTS: 160, TS Errs: in 5, out 6 +TS: 1880, dTS: 160, TS Errs: in 5, out 5
state->patch is most likely 0 here but we do patch things. Now we might want to fix the tsdelta in all cases but then I think we should rename state->patch to state->patch_ssrc (or state->patch_stream_change)? what do you think?
On 11/21/2013 08:40 PM, Holger Hans Peter Freyther wrote:
On Thu, Nov 21, 2013 at 07:05:45PM +0100, Jacob Erlbeck wrote:
+/* Assume audio frame length of 20ms */ +#define RTP_AUDIO_FRAME_DUR_NUM 20 +#define RTP_AUDIO_FRAME_DUR_DEN 1000 +#define RTP_AUDIO_DEFAULT_RATE 8000
can't we initialize the rate in mgcp_rtp_end_init or such? This way we know that this is either default or (read from the SDP file).
We can, but then I would like to put all of the above into the rtp_end structure.
state->patch = endp->allow_patch; -TS: 2040, dTS: 160, TS Errs: in 5, out 6 +TS: 1880, dTS: 160, TS Errs: in 5, out 5
state->patch is most likely 0 here but we do patch things.
I don't know what you mean exactly, in the test case allow_patch is explicitely set. Or do you mean in the wild?
Now we might want to fix the tsdelta in all cases but then I think we should rename state->patch to state->patch_ssrc (or state->patch_stream_change)?
what do you think?
Currently (with and without the patch) the packet is only patched after SSRC changes. Do you mean, that you would like to add a feature to always patch timestamps, even when the is no SSRC change? I'm not shure about the audio profiles that are to be expected, but rfc4867 allows for a lot of inconviniences w.r.t. packet durations (e.g. multiple frames per packet for a single channel). Even changing the offset after SSRC changes is just based on heuristics and may fail theoretically. If the SSRC changes when SSRC patching is disabled there is (in theory) no need to patch timestamps, since the receiver has to reinitialize timing anyway. So I conclude we have four options concerning patching: 1. Do nothing (offset is always 0) 2. Update the offset on SSRC changes and fix the SSRC 3. Check timestamps and fix them unless the SSRC changes (which is not fixed) 4. Fix SSRC and update the offset on all (detected) timestamp errors This can be divided into 2 orthogonal aspects: A. When the SSRC changes: Fix or don't fix the SSRC (including updating the offset on SSRC changes) B. When the SSRC does not change: Monitor the stream (per SSRC) for (suspected) timestamp errors and update the offset if an error is detected If the receiver was able to handle SSRC changes, we could also do the reverse to fix broken timimngs: change the SSRC on each detected timing problem. Anyway, in case B if there is a single packet in the stream which contains a different number of frames, the timing will be broken after that. So we can either a. forbid that and count packet size changes, b. disable offset updates when the packet size changes, or c. use everything we get from SDP, parse into AMR or whatever audio codec is used to determine the packet duration and use that for verification. What do you think? Jacob
On Fri, Nov 22, 2013 at 09:59:10AM +0100, Jacob Erlbeck wrote:
We can, but then I would like to put all of the above into the rtp_end structure.
sure.
state->patch is most likely 0 here but we do patch things.
I don't know what you mean exactly, in the test case allow_patch is explicitely set. Or do you mean in the wild?
Now. this is what I missed then. I git grepped my local tree for allow_patch but didn't look into patch 2.
Currently (with and without the patch) the packet is only patched after SSRC changes.
It was my understanding that the TS issue we were seing also appeared without a change in the SSRC? In case this is something I asked you to do after this patch we can postpone this discussion.
just based on heuristics and may fail theoretically.
SCNR: And in theory ip.access should be able to create working software with the capital they received. ;)
If the SSRC changes when SSRC patching is disabled there is (in theory) no need to patch timestamps, since the receiver has to reinitialize timing anyway.
true. The reason we patch the SSRC is that the nanoBTS software does not re-initialize the state.
What do you think?
I would like to understand if the TS problem only occurs when the TS is changing or if the ts_delta issue is only introduced by the patching I do.
On 11/22/2013 10:22 AM, Holger Hans Peter Freyther wrote:
On Fri, Nov 22, 2013 at 09:59:10AM +0100, Jacob Erlbeck wrote:
We can, but then I would like to put all of the above into the rtp_end structure.
sure.
Ok, then I'll refactor it this way.
Currently (with and without the patch) the packet is only patched after SSRC changes.
It was my understanding that the TS issue we were seing also appeared without a change in the SSRC? In case this is something I asked you to do after this patch we can postpone this discussion.
We've settled to just add counters and not to fix the timing for streams with a constant SSRC yet. So I'd rather put that feature into another commit after we've decided on the concept.
just based on heuristics and may fail theoretically.
SCNR: And in theory ip.access should be able to create working software with the capital they received. ;)
And the audio source should deliver non-broken audio streams either.
I would like to understand if the TS problem only occurs when the TS is changing or if the ts_delta issue is only introduced by the patching I do.
The TS problem in question only occured within streams of a singly SSRC and are thus not fixed by the patch (but detected). I've only fixed the SSRC patching because I didn't want the osmo MGW/NAT to introduce the same kind of errors we are getting from the other audio source. Jacob
On Fri, Nov 22, 2013 at 10:39:00AM +0100, Jacob Erlbeck wrote:
I've only fixed the SSRC patching because I didn't want the osmo MGW/NAT to introduce the same kind of errors we are getting from the other audio source.
thanks. I think we are in sync now.
From: Jacob Erlbeck <jerlbeck@sysmocom.de> The current implementation increments the seqno but does not increment the RTP timestamp, leading to two identical timestamps following one after the other. This patch fixes this by adding the computed tsdelta when the offset is calulated. In the unlikely case, that a tsdelta hasn't been computed yet when the SSRC changes, a tsdelta is computed based on the RTP rate and a RTP packet duration of 20ms (one speech frame per channel and packet). If the RTP rate is not known, a rate of 8000 is assumed. Note that this approach presumes, that the per RTP packet duration (in samples) is the same for the last two packets of the stream being replaced (the first one). Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/mgcp_internal.h | 4 ++++ openbsc/src/libmgcp/mgcp_network.c | 23 +++++++++++++++++++---- openbsc/src/libmgcp/mgcp_protocol.c | 12 ++++++++++++ openbsc/tests/mgcp/mgcp_test.c | 16 +++++++++++----- openbsc/tests/mgcp/mgcp_test.ok | 6 +++--- 5 files changed, 49 insertions(+), 12 deletions(-) diff --git a/openbsc/include/openbsc/mgcp_internal.h b/openbsc/include/openbsc/mgcp_internal.h index 8b6a56b..0b52c1c 100644 --- a/openbsc/include/openbsc/mgcp_internal.h +++ b/openbsc/include/openbsc/mgcp_internal.h @@ -77,6 +77,10 @@ struct mgcp_rtp_end { /* per endpoint data */ int payload_type; + uint32_t rate; + uint32_t frame_duration_num; + uint32_t frame_duration_den; + int frames_per_packet; char *fmtp_extra; /* diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index fa10cec..8f7fd09 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -238,15 +238,30 @@ void mgcp_patch_and_count(struct mgcp_endpoint *endp, struct mgcp_rtp_state *sta state->transit = arrival_time - timestamp; state->out_stream = state->in_stream; } else if (state->in_stream.ssrc != rtp_hdr->ssrc) { + int32_t tsdelta = state->out_stream.last_tsdelta; + if (tsdelta != 0) { + tsdelta = rtp_end->rate * rtp_end->frames_per_packet * + rtp_end->frame_duration_num / + rtp_end->frame_duration_den; + LOGP(DMGCP, LOGL_NOTICE, + "Computed timestamp delta %d based on " + "rate %d, num frames %d, frame duration %d/%d\n", + tsdelta, rtp_end->rate, rtp_end->frames_per_packet, + rtp_end->frame_duration_num, + rtp_end->frame_duration_den); + } state->in_stream.ssrc = rtp_hdr->ssrc; state->seq_offset = (state->out_stream.last_seq + 1) - seq; - state->timestamp_offset = state->out_stream.last_timestamp - timestamp; + state->timestamp_offset = + (state->out_stream.last_timestamp + tsdelta) - timestamp; state->patch = endp->allow_patch; LOGP(DMGCP, LOGL_NOTICE, - "The SSRC changed on 0x%x SSRC: %u offset: %d from %s:%d in %d\n", + "The SSRC changed on 0x%x SSRC: %u offset: %d tsdelta: %d " + "from %s:%d in %d\n", ENDPOINT_NUMBER(endp), state->in_stream.ssrc, - state->seq_offset, inet_ntoa(addr->sin_addr), - ntohs(addr->sin_port), endp->conn_mode); + state->seq_offset, tsdelta, + inet_ntoa(addr->sin_addr), ntohs(addr->sin_port), + endp->conn_mode); state->in_stream.last_tsdelta = 0; } else { diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index 6d22cf3..71fb89d 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -40,6 +40,12 @@ for (line = strtok_r(NULL, "\r\n", &save); line;\ line = strtok_r(NULL, "\r\n", &save)) +/* Assume audio frame length of 20ms */ +#define DEFAULT_RTP_AUDIO_FRAME_DUR_NUM 20 +#define DEFAULT_RTP_AUDIO_FRAME_DUR_DEN 1000 +#define DEFAULT_RTP_AUDIO_FRAMES_PER_PACKET 1 +#define DEFAULT_RTP_AUDIO_DEFAULT_RATE 8000 + static void mgcp_rtp_end_reset(struct mgcp_rtp_end *end); struct mgcp_parse_data { @@ -965,6 +971,12 @@ static void mgcp_rtp_end_reset(struct mgcp_rtp_end *end) end->local_alloc = -1; talloc_free(end->fmtp_extra); end->fmtp_extra = NULL; + + /* Set default values */ + end->frame_duration_num = DEFAULT_RTP_AUDIO_FRAME_DUR_NUM; + end->frame_duration_den = DEFAULT_RTP_AUDIO_FRAME_DUR_DEN; + end->frames_per_packet = DEFAULT_RTP_AUDIO_FRAMES_PER_PACKET; + end->rate = DEFAULT_RTP_AUDIO_DEFAULT_RATE; } static void mgcp_rtp_end_init(struct mgcp_rtp_end *end) diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index d3c5687..84c55aa 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -417,7 +417,7 @@ static void test_packet_error_detection(void) struct mgcp_trunk_config trunk; struct mgcp_endpoint endp; struct mgcp_rtp_state state; - struct mgcp_rtp_end rtp; + struct mgcp_rtp_end *rtp = &endp.net_end; struct sockaddr_in addr = {0}; char buffer[4096]; @@ -426,14 +426,20 @@ static void test_packet_error_detection(void) memset(&trunk, 0, sizeof(trunk)); memset(&endp, 0, sizeof(endp)); memset(&state, 0, sizeof(state)); - memset(&rtp, 0, sizeof(rtp)); trunk.number_endpoints = 1; trunk.endpoints = &endp; + endp.tcfg = &trunk; + + /* This doesn't free endp but resets/frees all fields of the structure + * and invokes mgcp_rtp_end_reset() for each mgcp_rtp_end. OTOH, it + * expects valid pointer fields (either NULL or talloc'ed), so the + * memset is still needed. It also requires that endp.tcfg and + * trunk.endpoints are set up properly. */ + mgcp_free_endp(&endp); - rtp.payload_type = 98; + rtp->payload_type = 98; endp.allow_patch = 1; - endp.tcfg = &trunk; for (i = 0; i < ARRAY_SIZE(test_rtp_packets1); ++i) { struct rtp_packet_info *info = test_rtp_packets1 + i; @@ -442,7 +448,7 @@ static void test_packet_error_detection(void) OSMO_ASSERT(info->len >= 0); memmove(buffer, info->data, info->len); - mgcp_patch_and_count(&endp, &state, &rtp, &addr, + mgcp_patch_and_count(&endp, &state, rtp, &addr, buffer, info->len); printf("TS: %d, dTS: %d, TS Errs: in %d, out %d\n", diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 8c3fa26..5666424 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -41,7 +41,7 @@ TS: 1280, dTS: 160, TS Errs: in 3, out 3 TS: 1400, dTS: 120, TS Errs: in 4, out 4 TS: 1560, dTS: 160, TS Errs: in 5, out 5 TS: 1720, dTS: 160, TS Errs: in 5, out 5 -TS: 1720, dTS: 160, TS Errs: in 5, out 6 -TS: 1880, dTS: 160, TS Errs: in 5, out 6 -TS: 2040, dTS: 160, TS Errs: in 5, out 6 +TS: 1880, dTS: 160, TS Errs: in 5, out 5 +TS: 2040, dTS: 160, TS Errs: in 5, out 5 +TS: 2200, dTS: 160, TS Errs: in 5, out 5 Done -- 1.7.9.5
participants (3)
-
Holger Hans Peter Freyther -
Jacob Erlbeck -
y