From 894eeae03c13f6e188b97887eeb6234f02174c84 Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Wed, 24 May 2023 13:50:38 +0200 Subject: [PATCH] impl-node: add id and name to pw_node_target Make a copy of the node name into a statically allocated array. This is for debugging purposes only but might crash if we do a name change while the data thread is reading it. Make it possible to do reposition on the client side by copying the id to the target. The client side does not have a node in the target so we can't deref it. --- src/modules/module-client-node/remote-node.c | 5 +- src/modules/module-profiler.c | 29 ++++++----- src/pipewire/impl-link.c | 10 ++-- src/pipewire/impl-node.c | 54 ++++++++++---------- src/pipewire/private.h | 2 + 5 files changed, 53 insertions(+), 47 deletions(-) diff --git a/src/modules/module-client-node/remote-node.c b/src/modules/module-client-node/remote-node.c index 7e12967ba..a939166be 100644 --- a/src/modules/module-client-node/remote-node.c +++ b/src/modules/module-client-node/remote-node.c @@ -93,7 +93,7 @@ static struct link *find_activation(struct spa_list *links, uint32_t node_id) struct link *l; spa_list_for_each(l, links, link) { - if (l->node_id == node_id) + if (l->target.id == node_id) return l; } return NULL; @@ -263,6 +263,7 @@ static int client_node_transport(void *_data, node->rt.target.activation = data->activation->ptr; node->rt.position = &node->rt.target.activation->position; node->info.id = node->rt.target.activation->position.clock.id; + node->rt.target.id = node->info.id; pw_log_debug("remote-node %p: fds:%d %d node:%u activation:%p", proxy, readfd, writefd, data->remote_id, data->activation->ptr); @@ -909,8 +910,8 @@ client_node_set_activation(void *_data, goto error_exit; } link->data = data; - link->node_id = node_id; link->map = mm; + link->target.id = node_id; link->target.activation = ptr; link->target.system = data->data_system; link->target.fd = signalfd; diff --git a/src/modules/module-profiler.c b/src/modules/module-profiler.c index acbd9a3bc..36c096b7a 100644 --- a/src/modules/module-profiler.c +++ b/src/modules/module-profiler.c @@ -168,6 +168,7 @@ static void context_do_profile(void *data, struct pw_impl_node *node) struct impl *impl = data; struct spa_pod_builder b; struct spa_pod_frame f[2]; + uint32_t id = node->info.id; struct pw_node_activation *a = node->rt.target.activation; struct spa_io_position *pos = &a->position; struct pw_node_target *t; @@ -205,7 +206,7 @@ static void context_do_profile(void *data, struct pw_impl_node *node) spa_pod_builder_prop(&b, SPA_PROFILER_driverBlock, 0); spa_pod_builder_add_struct(&b, - SPA_POD_Int(node->info.id), + SPA_POD_Int(id), SPA_POD_String(node->name), SPA_POD_Long(a->prev_signal_time), SPA_POD_Long(a->signal_time), @@ -219,22 +220,26 @@ static void context_do_profile(void *data, struct pw_impl_node *node) struct pw_node_activation *na; struct spa_fraction latency; - if (n == NULL || n == node) + if (t->id == id) continue; - latency = n->latency; - if (n->force_quantum != 0) - latency.num = n->force_quantum; - if (n->force_rate != 0) - latency.denom = n->force_rate; - else if (n->rate.denom != 0) - latency.denom = n->rate.denom; + if (n != NULL) { + latency = n->latency; + if (n->force_quantum != 0) + latency.num = n->force_quantum; + if (n->force_rate != 0) + latency.denom = n->force_rate; + else if (n->rate.denom != 0) + latency.denom = n->rate.denom; + } else { + spa_zero(latency); + } - na = n->rt.target.activation; + na = t->activation; spa_pod_builder_prop(&b, SPA_PROFILER_followerBlock, 0); spa_pod_builder_add_struct(&b, - SPA_POD_Int(n->info.id), - SPA_POD_String(n->name), + SPA_POD_Int(t->id), + SPA_POD_String(t->name), SPA_POD_Long(a->signal_time), SPA_POD_Long(na->signal_time), SPA_POD_Long(na->awake_time), diff --git a/src/pipewire/impl-link.c b/src/pipewire/impl-link.c index 4315e2e87..df4590d31 100644 --- a/src/pipewire/impl-link.c +++ b/src/pipewire/impl-link.c @@ -57,7 +57,7 @@ static struct pw_node_peer *pw_node_peer_ref(struct pw_impl_node *onode, struct struct pw_node_peer *peer; spa_list_for_each(peer, &onode->peer_list, link) { - if (peer->target.node == inode) { + if (peer->target.id == inode->info.id) { pw_log_debug("exiting peer %p from %p to %p", peer, onode, inode); peer->ref++; return peer; @@ -102,8 +102,8 @@ static void pw_node_peer_activate(struct pw_node_peer *peer) peer->target.active = true; } } - pw_log_trace("%p: node:%p state:%p pending:%d/%d", peer->output, - peer->target.node, state, state->pending, state->required); + pw_log_trace("%p: node:%s state:%p pending:%d/%d", peer->output, + peer->target.name, state, state->pending, state->required); } static void pw_node_peer_deactivate(struct pw_node_peer *peer) @@ -119,8 +119,8 @@ static void pw_node_peer_deactivate(struct pw_node_peer *peer) peer->target.active = false; } } - pw_log_trace("%p: node:%p state:%p pending:%d/%d", peer->output, - peer->target.node, state, state->pending, state->required); + pw_log_trace("%p: node:%s state:%p pending:%d/%d", peer->output, + peer->target.name, state, state->pending, state->required); } diff --git a/src/pipewire/impl-node.c b/src/pipewire/impl-node.c index 13aa3b9fc..78e031962 100644 --- a/src/pipewire/impl-node.c +++ b/src/pipewire/impl-node.c @@ -124,8 +124,8 @@ static void remove_node(struct pw_impl_node *this) if (this->exported) return; - pw_log_trace("%p: remove from driver %p %p %p", - this, this->rt.driver_target.node, + pw_log_trace("%p: remove from driver %s %p %p", + this, this->rt.driver_target.name, this->rt.driver_target.activation, this->rt.target.activation); spa_list_remove(&this->rt.target.link); @@ -148,7 +148,7 @@ static void remove_node(struct pw_impl_node *this) } spa_list_remove(&this->rt.driver_target.link); - this->rt.driver_target.node = NULL; + spa_zero(this->rt.driver_target); } static int @@ -780,6 +780,7 @@ int pw_impl_node_register(struct pw_impl_node *this, this->rt.target.activation->position.clock.id = this->global->id; this->info.id = this->global->id; + this->rt.target.id = this->info.id; pw_properties_setf(this->properties, PW_KEY_OBJECT_ID, "%d", this->info.id); pw_properties_setf(this->properties, PW_KEY_OBJECT_SERIAL, "%"PRIu64, pw_global_get_serial(this->global)); @@ -924,6 +925,7 @@ static void check_properties(struct pw_impl_node *node) (node->name == NULL || !spa_streq(node->name, str))) { free(node->name); node->name = strdup(str); + snprintf(node->rt.target.name, sizeof(node->rt.target.name), "%s", node->name); pw_log_debug("%p: name '%s'", node, node->name); } @@ -1100,20 +1102,19 @@ static void check_states(struct pw_impl_node *driver, uint64_t nsec) spa_list_for_each(t, &driver->rt.target_list, link) { struct pw_node_activation *a = t->activation; struct pw_node_activation_state *state = &a->state[0]; - if (t->node == NULL) - continue; + if (a->status == PW_NODE_ACTIVATION_TRIGGERED || a->status == PW_NODE_ACTIVATION_AWAKE) { update_xrun_stats(a, nsec / 1000, 0); pw_log(level, "(%s-%u) client too slow! rate:%u/%u pos:%"PRIu64" status:%s", - t->node->name, t->node->info.id, + t->name, t->id, (uint32_t)(cl->rate.num * cl->duration), cl->rate.denom, cl->position, str_status(a->status)); } pw_log_debug("(%s-%u) state:%p pending:%d/%d s:%"PRIu64" a:%"PRIu64" f:%"PRIu64 " waiting:%"PRIu64" process:%"PRIu64" status:%s sync:%d", - t->node->name, t->node->info.id, state, + t->name, t->id, state, state->pending, state->required, a->signal_time, a->awake_time, @@ -1676,15 +1677,15 @@ static inline int check_updates(struct pw_impl_node *node, uint32_t *reposition_ return res; } -static void do_reposition(struct pw_impl_node *driver, struct pw_impl_node *node) +static void do_reposition(struct pw_impl_node *driver, struct pw_node_target *target) { struct pw_node_activation *a = driver->rt.target.activation; struct spa_io_segment *dst, *src; - src = &node->rt.target.activation->reposition; + src = &target->activation->reposition; dst = &a->position.segments[0]; - pw_log_info("%p: update position:%"PRIu64, node, src->position); + pw_log_info("%p: %u update position:%"PRIu64, driver, target->id, src->position); dst->version = src->version; dst->flags = src->flags; @@ -1734,11 +1735,11 @@ static inline void update_position(struct pw_impl_node *node, int all_ready, uin */ static int node_ready(void *data, int status) { - struct pw_impl_node *node = data, *reposition_node = NULL; + struct pw_impl_node *node = data; struct pw_impl_node *driver = node->driver_node; struct pw_node_activation *a = node->rt.target.activation; struct spa_system *data_system = node->data_system; - struct pw_node_target *t; + struct pw_node_target *t, *reposition_target = NULL;; struct pw_impl_port *p; uint64_t nsec; @@ -1792,25 +1793,22 @@ again: spa_list_for_each(t, &driver->rt.target_list, link) { struct pw_node_activation *ta = t->activation; + uint32_t id = t->id; ta->status = PW_NODE_ACTIVATION_NOT_TRIGGERED; pw_node_activation_state_reset(&ta->state[0]); - if (SPA_LIKELY(t->node)) { - uint32_t id = t->node->info.id; + /* this is the node with reposition info */ + if (SPA_UNLIKELY(id == reposition_owner)) + reposition_target = t; - /* this is the node with reposition info */ - if (SPA_UNLIKELY(id == reposition_owner)) - reposition_node = t->node; + /* update extra segment info if it is the owner */ + if (SPA_UNLIKELY(id == owner[0])) + a->position.segments[0].bar = ta->segment.bar; + if (SPA_UNLIKELY(id == owner[1])) + a->position.segments[0].video = ta->segment.video; - /* update extra segment info if it is the owner */ - if (SPA_UNLIKELY(id == owner[0])) - a->position.segments[0].bar = ta->segment.bar; - if (SPA_UNLIKELY(id == owner[1])) - a->position.segments[0].video = ta->segment.video; - - min_timeout = SPA_MIN(min_timeout, ta->sync_timeout); - } + min_timeout = SPA_MIN(min_timeout, ta->sync_timeout); if (SPA_UNLIKELY(update_sync)) { ta->pending_sync = target_sync; @@ -1829,11 +1827,11 @@ again: a->sync_timeout = SPA_MIN(min_timeout, DEFAULT_SYNC_TIMEOUT); - if (SPA_UNLIKELY(reposition_node)) { - do_reposition(node, reposition_node); + if (SPA_UNLIKELY(reposition_target != NULL)) { + do_reposition(node, reposition_target); sync_type = SYNC_START; reposition_owner = 0; - reposition_node = NULL; + reposition_target = NULL; goto again; } diff --git a/src/pipewire/private.h b/src/pipewire/private.h index 470aef21a..d0a3b983c 100644 --- a/src/pipewire/private.h +++ b/src/pipewire/private.h @@ -591,6 +591,8 @@ static inline void pw_node_activation_state_reset(struct pw_node_activation_stat struct pw_node_target { struct spa_list link; + uint32_t id; + char name[128]; struct pw_impl_node *node; struct pw_node_activation *activation; struct spa_system *system;