From ad4de14e13145561529170c4f746fc3cb62b2829 Mon Sep 17 00:00:00 2001 From: Sebastian Reimers Date: Thu, 5 Oct 2023 13:52:37 +0200 Subject: [PATCH 1/4] ice/icesdp: split IPv4 and IPv6 mDNS resolving (#934) mDNS resolving can lead to long timeouts (~5s), so it's better to resolve IPv4 and IPv6 separately to avoid long ice startup delays. --- src/ice/connchk.c | 6 ++++++ src/ice/icesdp.c | 55 +++++++++++++++++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/src/ice/connchk.c b/src/ice/connchk.c index e9809e688..230d02c46 100644 --- a/src/ice/connchk.c +++ b/src/ice/connchk.c @@ -404,6 +404,12 @@ static void rcand_wait_timeout(void *arg) { struct icem *icem = arg; + /* Avoid long startup delay */ + icem->rcand_wait = false; + + icem_printf(icem, "conncheck_start: " + "mDNS timeout for remote candidate...\n"); + icem_conncheck_start(icem); } diff --git a/src/ice/icesdp.c b/src/ice/icesdp.c index 7aa596ae6..41a8b113f 100644 --- a/src/ice/icesdp.c +++ b/src/ice/icesdp.c @@ -40,6 +40,7 @@ static const char rel_port_str[] = "rport"; struct rcand { + int ai_family; struct icem *icem; enum ice_cand_type type; unsigned cid; @@ -208,14 +209,15 @@ static int getaddr_rcand(void *arg) { struct rcand *rcand = arg; struct addrinfo *res, *res0 = NULL; + struct addrinfo hints = {.ai_flags = AI_V4MAPPED | AI_ADDRCONFIG, + .ai_family = rcand->ai_family}; int err; - err = getaddrinfo(rcand->domain, NULL, NULL, &res0); + err = getaddrinfo(rcand->domain, NULL, &hints, &res0); if (err) return EADDRNOTAVAIL; for (res = res0; res; res = res->ai_next) { - err = sa_set_sa(&rcand->caddr, res->ai_addr); if (err) continue; @@ -238,6 +240,11 @@ static void delayed_rcand(int err, void *arg) if (err) goto out; + if (!rcand->icem->rcand_wait) { + DEBUG_WARNING("late mDNS candidate: %s\n", rcand->domain); + goto out; + } + /* add only if not exist */ if (icem_cand_find(&rcand->icem->rcandl, rcand->cid, &rcand->caddr)) goto out; @@ -246,7 +253,6 @@ static void delayed_rcand(int err, void *arg) &rcand->caddr, &rcand->rel_addr, &rcand->foundation); out: - rcand->icem->rcand_wait = false; mem_deref(rcand); } @@ -314,27 +320,54 @@ static int cand_decode(struct icem *icem, const char *val) if (pl_strstr(&addr, ".local") != NULL) { /* try non blocking getaddr mdns resolution */ icem_printf(icem, "mDNS remote cand: %r\n", &addr); + icem->rcand_wait = true; + + /* AF_INET IPv4 candidate */ struct rcand *rcand = mem_zalloc(sizeof(struct rcand), rcand_dealloc); if (!rcand) return ENOMEM; - rcand->icem = mem_ref(icem); - rcand->type = ice_cand_name2type(type); - rcand->cid = cid; - rcand->prio = pl_u32(&prio); - rcand->port = pl_u32(&port); - rcand->rel_addr = rel_addr; + rcand->ai_family = AF_INET; + rcand->icem = mem_ref(icem); + rcand->type = ice_cand_name2type(type); + rcand->cid = cid; + rcand->prio = pl_u32(&prio); + rcand->port = pl_u32(&port); + rcand->rel_addr = rel_addr; pl_dup(&rcand->foundation, &foundation); (void)pl_strcpy(&addr, rcand->domain, sizeof(rcand->domain)); - icem->rcand_wait = true; - err = re_thread_async(getaddr_rcand, delayed_rcand, rcand); if (err) mem_deref(rcand); + /* AF_INET6 IPv6 candidate + * mDNS resolving can lead to long timeouts (~5s), so it's + * better to resolve IPv4 and IPv6 separately to avoid long ice + * startup delays. + */ + struct rcand *rcand6 = + mem_zalloc(sizeof(struct rcand), rcand_dealloc); + if (!rcand6) + return ENOMEM; + + rcand6->ai_family = AF_INET6; + rcand6->icem = mem_ref(icem); + rcand6->type = ice_cand_name2type(type); + rcand6->cid = cid; + rcand6->prio = pl_u32(&prio); + rcand6->port = pl_u32(&port); + rcand6->rel_addr = rel_addr; + + pl_dup(&rcand6->foundation, &foundation); + (void)pl_strcpy(&addr, rcand6->domain, sizeof(rcand6->domain)); + + err = re_thread_async(getaddr_rcand, delayed_rcand, rcand6); + if (err) + mem_deref(rcand6); + return err; } From 33599fae4cdf1c84cd14148bbc1b47b571ba7a49 Mon Sep 17 00:00:00 2001 From: Sebastian Reimers Date: Fri, 6 Oct 2023 12:23:25 +0200 Subject: [PATCH 2/4] trace: add flush worker and optimize memory usage (#967) --- src/trace/trace.c | 66 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 58 insertions(+), 8 deletions(-) diff --git a/src/trace/trace.c b/src/trace/trace.c index 472aeeea7..d927953d7 100644 --- a/src/trace/trace.c +++ b/src/trace/trace.c @@ -9,7 +9,9 @@ #include #include #include +#include #include +#include #ifdef HAVE_PTHREAD #include @@ -27,7 +29,22 @@ #include #endif -#define TRACE_BUFFER_SIZE 1000000 +#define DEBUG_MODULE "trace" +#define DEBUG_LEVEL 5 +#include + +#ifndef TRACE_BUFFER_SIZE +#define TRACE_BUFFER_SIZE 100000 +#endif + +#ifndef TRACE_FLUSH_THRESHOLD +#define TRACE_FLUSH_THRESHOLD 1000 +#endif + +#ifndef TRACE_FLUSH_TMR +#define TRACE_FLUSH_TMR 1000 +#endif + struct trace_event { const char *name; @@ -47,15 +64,16 @@ struct trace_event { /** Trace configuration */ static struct { + RE_ATOMIC bool init; int process_id; FILE *f; int event_count; struct trace_event *event_buffer; struct trace_event *event_buffer_flush; mtx_t lock; - bool init; bool new; uint64_t start_time; + struct tmr flush_tmr; } trace = { .init = false }; @@ -90,6 +108,33 @@ static inline int get_process_id(void) } +static int flush_worker(void *arg) +{ + (void)arg; + + mtx_lock(&trace.lock); + if (trace.event_count < TRACE_FLUSH_THRESHOLD) { + mtx_unlock(&trace.lock); + return 0; + } + mtx_unlock(&trace.lock); + + re_trace_flush(); + + return 0; +} + + +static void flush_tmr(void *arg) +{ + (void)arg; + + re_thread_async(flush_worker, NULL, NULL); + + tmr_start(&trace.flush_tmr, TRACE_FLUSH_TMR, flush_tmr, NULL); +} + + /** * Init new trace json file * @@ -108,7 +153,7 @@ int re_trace_init(const char *json_file) if (!json_file) return EINVAL; - if (trace.init) + if (re_atomic_rlx(&trace.init)) return EALREADY; trace.event_buffer = mem_zalloc( @@ -137,12 +182,15 @@ int re_trace_init(const char *json_file) (void)fflush(trace.f); trace.start_time = tmr_jiffies_usec(); - trace.init = true; + re_atomic_rlx_set(&trace.init, true); trace.new = true; + tmr_init(&trace.flush_tmr); + tmr_start(&trace.flush_tmr, TRACE_FLUSH_TMR, flush_tmr, NULL); + out: if (err) { - trace.init = false; + re_atomic_rlx_set(&trace.init, false); mem_deref(trace.event_buffer); mem_deref(trace.event_buffer_flush); } @@ -164,12 +212,13 @@ int re_trace_close(void) return 0; #endif + tmr_cancel(&trace.flush_tmr); re_trace_flush(); + re_atomic_rlx_set(&trace.init, false); trace.event_buffer = mem_deref(trace.event_buffer); trace.event_buffer_flush = mem_deref(trace.event_buffer_flush); mtx_destroy(&trace.lock); - trace.init = false; (void)re_fprintf(trace.f, "\n\t]\n}\n"); if (trace.f) @@ -201,7 +250,7 @@ int re_trace_flush(void) return 0; #endif - if (!trace.init) + if (!re_atomic_rlx(&trace.init)) return 0; mtx_lock(&trace.lock); @@ -266,11 +315,12 @@ void re_trace_event(const char *cat, const char *name, char ph, void *id, return; #endif - if (!trace.init) + if (!re_atomic_rlx(&trace.init)) return; mtx_lock(&trace.lock); if (trace.event_count >= TRACE_BUFFER_SIZE) { + DEBUG_WARNING("Increase TRACE_BUFFER_SIZE\n"); mtx_unlock(&trace.lock); return; } From f58c9f6778783de187df1137acd226ce569938e3 Mon Sep 17 00:00:00 2001 From: Sebastian Reimers Date: Sat, 7 Oct 2023 00:01:17 +0200 Subject: [PATCH 3/4] rtp: add ts_arrive header The arrival time is useful for jitter handling and local playout time calculation. --- include/re_rtp.h | 1 + src/rtp/rtcp.h | 5 ++--- src/rtp/rtp.c | 6 ++---- src/rtp/sess.c | 24 +++++++++++------------- 4 files changed, 16 insertions(+), 20 deletions(-) diff --git a/include/re_rtp.h b/include/re_rtp.h index 4a654afbf..5855e27a6 100644 --- a/include/re_rtp.h +++ b/include/re_rtp.h @@ -23,6 +23,7 @@ struct rtp_header { uint8_t pt; /**< Payload type */ uint16_t seq; /**< Sequence number */ uint32_t ts; /**< Timestamp */ + uint32_t ts_arrive; /**< Arrival Timestamp */ uint32_t ssrc; /**< Synchronization source */ uint32_t csrc[16]; /**< Contributing sources */ struct { diff --git a/src/rtp/rtcp.h b/src/rtp/rtcp.h index f2635954d..194b6b692 100644 --- a/src/rtp/rtcp.h +++ b/src/rtp/rtcp.h @@ -116,6 +116,5 @@ int rtcp_send(struct rtp_sock *rs, struct mbuf *mb); void rtcp_handler(struct rtcp_sess *sess, struct rtcp_msg *msg); void rtcp_sess_tx_rtp(struct rtcp_sess *sess, uint32_t ts, uint64_t jfs_rt, size_t payload_size); -void rtcp_sess_rx_rtp(struct rtcp_sess *sess, uint16_t seq, uint32_t ts, - uint32_t src, size_t payload_size, - const struct sa *peer); +void rtcp_sess_rx_rtp(struct rtcp_sess *sess, struct rtp_header *hdr, + size_t payload_size, const struct sa *peer); diff --git a/src/rtp/rtp.c b/src/rtp/rtp.c index 3082c7d40..9f5205d46 100644 --- a/src/rtp/rtp.c +++ b/src/rtp/rtp.c @@ -205,10 +205,8 @@ static void udp_recv_handler(const struct sa *src, struct mbuf *mb, void *arg) if (err) return; - if (rs->rtcp) { - rtcp_sess_rx_rtp(rs->rtcp, hdr.seq, hdr.ts, - hdr.ssrc, mbuf_get_left(mb), src); - } + if (rs->rtcp) + rtcp_sess_rx_rtp(rs->rtcp, &hdr, mbuf_get_left(mb), src); if (rs->recvh) rs->recvh(src, &hdr, mb, rs->arg); diff --git a/src/rtp/sess.c b/src/rtp/sess.c index d4d970cbc..bba3a387a 100644 --- a/src/rtp/sess.c +++ b/src/rtp/sess.c @@ -563,47 +563,45 @@ void rtcp_sess_tx_rtp(struct rtcp_sess *sess, uint32_t ts, uint64_t jfs_rt, } -void rtcp_sess_rx_rtp(struct rtcp_sess *sess, uint16_t seq, uint32_t ts, - uint32_t ssrc, size_t payload_size, - const struct sa *peer) +void rtcp_sess_rx_rtp(struct rtcp_sess *sess, struct rtp_header *hdr, + size_t payload_size, const struct sa *peer) { struct rtp_member *mbr; if (!sess) return; - mbr = get_member(sess, ssrc); + mbr = get_member(sess, hdr->ssrc); if (!mbr) { - DEBUG_NOTICE("could not add member: 0x%08x\n", ssrc); + DEBUG_NOTICE("could not add member: 0x%08x\n", hdr->ssrc); return; } if (!mbr->s) { mbr->s = mem_zalloc(sizeof(*mbr->s), NULL); if (!mbr->s) { - DEBUG_NOTICE("could not add sender: 0x%08x\n", ssrc); + DEBUG_NOTICE("could not add sender: 0x%08x\n", + hdr->ssrc); return; } /* first packet - init sequence number */ - source_init_seq(mbr->s, seq); + source_init_seq(mbr->s, hdr->seq); /* probation not used */ sa_cpy(&mbr->s->rtp_peer, peer); ++sess->senderc; } - if (!source_update_seq(mbr->s, seq)) { + if (!source_update_seq(mbr->s, hdr->seq)) { DEBUG_WARNING("rtp_update_seq() returned 0\n"); } if (sess->srate_rx) { - - uint64_t ts_arrive; - /* Convert from wall-clock time to timestamp units */ - ts_arrive = tmr_jiffies() * sess->srate_rx / 1000; + hdr->ts_arrive = + (uint32_t)(tmr_jiffies() * sess->srate_rx / 1000); - source_calc_jitter(mbr->s, ts, (uint32_t)ts_arrive); + source_calc_jitter(mbr->s, hdr->ts, hdr->ts_arrive); } mbr->s->rtp_rx_bytes += payload_size; From 85492608c661b70fb3f523a1ebd75a98eac8166b Mon Sep 17 00:00:00 2001 From: Sebastian Reimers Date: Sat, 7 Oct 2023 09:41:42 +0200 Subject: [PATCH 4/4] rtp: use uint64_t for ts_arrive and fix video jitter calculation --- include/re_rtp.h | 2 +- src/rtp/rtcp.h | 1 + src/rtp/sess.c | 15 +++++++++++---- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/include/re_rtp.h b/include/re_rtp.h index 5855e27a6..2e8bb71b7 100644 --- a/include/re_rtp.h +++ b/include/re_rtp.h @@ -23,7 +23,7 @@ struct rtp_header { uint8_t pt; /**< Payload type */ uint16_t seq; /**< Sequence number */ uint32_t ts; /**< Timestamp */ - uint32_t ts_arrive; /**< Arrival Timestamp */ + uint64_t ts_arrive; /**< Arrival Timestamp */ uint32_t ssrc; /**< Synchronization source */ uint32_t csrc[16]; /**< Contributing sources */ struct { diff --git a/src/rtp/rtcp.h b/src/rtp/rtcp.h index 194b6b692..d85404866 100644 --- a/src/rtp/rtcp.h +++ b/src/rtp/rtcp.h @@ -44,6 +44,7 @@ struct rtp_source { uint64_t sr_recv; /**< When the last SR was received */ struct ntp_time last_sr; /**< NTP Timestamp from last SR received */ uint32_t rtp_ts; /**< RTP timestamp */ + uint32_t last_rtp_ts; /**< Last RTP timestamp */ uint32_t psent; /**< RTP packets sent */ uint32_t osent; /**< RTP octets sent */ }; diff --git a/src/rtp/sess.c b/src/rtp/sess.c index bba3a387a..4d6e5f565 100644 --- a/src/rtp/sess.c +++ b/src/rtp/sess.c @@ -598,12 +598,19 @@ void rtcp_sess_rx_rtp(struct rtcp_sess *sess, struct rtp_header *hdr, if (sess->srate_rx) { /* Convert from wall-clock time to timestamp units */ - hdr->ts_arrive = - (uint32_t)(tmr_jiffies() * sess->srate_rx / 1000); - - source_calc_jitter(mbr->s, hdr->ts, hdr->ts_arrive); + hdr->ts_arrive = tmr_jiffies() * sess->srate_rx / 1000; + + /* + * Calculate jitter only when the timestamp is different than + * last packet (see RTP FAQ + * https://www.cs.columbia.edu/~hgs/rtp/faq.html#jitter). + */ + if (hdr->ts != mbr->s->last_rtp_ts) + source_calc_jitter(mbr->s, hdr->ts, + (uint32_t)hdr->ts_arrive); } + mbr->s->last_rtp_ts = hdr->ts; mbr->s->rtp_rx_bytes += payload_size; }