lucas/nanoKontroller2
lucas/nanoKontroller2
Watch
1
Fork
You've already forked nanoKontroller2
 0
Code Issues 13 Pull requests Activity Actions

Application with multiple nodes only control a random one #18

New issue
Closed
opened 2026-11-13 00:33:08 +01:00 by lucas · 1 comment
lucas commented 2026-11-13 00:33:08 +01:00
Owner

Problem

Firefox often shows up three or four times in pw-dump and in Plasma's tray widget, depending on
which tabs currently claim to send data. A strip targeting Firefox should control every stream
matching that name — they all carry the same node.name — but it controls one at most, sometimes
none, and which one changes over time.

The cause is that the matching layer and the addressing layer disagree about what identifies a node.
AudioService caches nodes in std::unordered_map<std::string, WpNode*> keyed by node.name
(includes/AudioService.h:58), and PipeWire node names are not unique:

  • Insertion is first-come-wins. on_node_added stores the node only when the name is absent, and
    connects the params-changed / ports-changed handlers inside that same guard
    (src/AudioService.cpp:446-450). Firefox nodes two, three and four pass the media.class filter
    and are then dropped, subscriptions and all.
  • Enumeration cannot return them. GetMatchingNodes iterates the cache and returns names
    (src/AudioService.cpp:88-116), so identically named streams collapse to one entry whatever the
    configured pattern is.
  • Actuation re-resolves by name. Action_Volume::on_midi loops over MatchedNodes(), but
    SetVolume/SetMute/GetMute/SetDefaultNode/IsDefaultNode all do Nodes.find(nodeName) and
    act on the one cached proxy (src/AudioService.cpp:118-204).
  • Removal evicts by name. on_node_removed does Nodes.erase(NodeName) without checking which
    object is leaving (src/AudioService.cpp:475). One tab ending deletes the slot while three live
    Firefox nodes remain, and those survivors are never re-added because object-added already fired
    for them. That is the "sometimes none", and which object owns the slot at any moment is decided by
    whichever add happened to arrive while the slot was free — the "it changes".

So the fan-out contract the README promises ("a press mutes or unmutes every matched node") holds
only for targets whose matches happen to have distinct node.name values: invisible for sinks,
total for application streams.

Separately, Plasma's second label line — the tab title — has no representation in the config at all.
Every call site reads exactly one property, wp_pipewire_object_get_property(…, "node.name")
(src/AudioService.cpp:433, :469, :490, :508), and NodeMatcher matches a single opaque
string with no notion of which field it came from.

Plan

1. NodeMatcher matches a property bag, with a second candidate string

In includes/NodeMatcher.h, above the class, add the bag and the separator. Keep both here (not in
AudioService.h): the unit-test target compiles NodeMatcher.cpp and AppSettings.cpp only and
links no WirePlumber (CMakeLists.txt:69-74), so the matcher must be exercisable without a WpNode.

// Separates node.name from media.name in the second match candidate. Chosen to
// be easy to type in a config pattern and rare inside a PipeWire node name.
inline constexpr std::string_view NodeNameSeparator = ": ";

// The PipeWire properties a pattern is matched against. The views point into
// storage owned by the node proxy, which WirePlumber guarantees alive until
// execution returns to the event loop, so a bag must never outlive the call it
// was built for.
struct NodeProperties {
    std::string_view NodeName;
    // Empty when the node carries no media.name.
    std::string_view MediaName;
};

Change both query methods to take the bag — FirstMatchingEntry(const NodeProperties&) and
Matches(const NodeProperties&); there is no string overload left, so every caller is forced to
supply properties. In src/NodeMatcher.cpp, match each entry against two candidates, in order:

  1. props.NodeName;
  2. props.NodeName + ": " + props.MediaName, formed only when props.MediaName is non-empty.

Build the concatenation once before the entry loop, not per entry. Match with iterators
(std::regex_match(sv.data(), sv.data() + sv.size(), m_Regexes[i])) so no std::string is
allocated for the bare-name candidate. Compilation and anchoring are unchanged: both candidates are
matched against the same ^(?:pattern)$ regex, so a plain name still matches only itself.

Entry ranking does not change and does not record which candidate matched: FirstMatchingEntry
returns the index of the first entry that matched on either candidate. A node whose media.name
is absent simply has one candidate.

Update the README's "Specifying nodes" section in the same change: a pattern is matched against
node.name and, when the node has a media.name, against node.name: media.name, so a pattern
broad enough to match arbitrary text (.*, or a negative-lookahead "everything else" pattern) now
also matches via the second candidate and exclusion lists must account for that.

No config syntax is added. Selecting a specific property per entry is deliberately left out; the bag
carries only the two fields that feed the candidates, and a third field is a one-line addition when
a syntax issue needs it.

2. Key the node cache by PipeWire bound id

In includes/AudioService.h, replace the name-keyed map with:

// One entry per live node. The key is the PipeWire bound id, which is what
// every actuator and every link endpoint is expressed in, so a shared
// node.name no longer collapses several nodes into one slot.
struct CachedNode {
    std::unique_ptr<WpNode, WpNodeDeleter> Node;
    // Listeners whose matcher matched this node at the last evaluation.
    // Membership changes while a node lives, because media.name does.
    std::vector<IActionBase*> MatchedListeners;
};
std::unordered_map<guint32, CachedNode> Nodes;

Add a file-local helper in src/AudioService.cpp that builds a bag from a proxy, used by every
match site:

NodeProperties MakeNodeProperties(WpPipewireObject* Object) {
    const char* NodeName  = wp_pipewire_object_get_property(Object, PW_KEY_NODE_NAME);
    const char* MediaName = wp_pipewire_object_get_property(Object, PW_KEY_MEDIA_NAME);
    return { NodeName ? NodeName : "", MediaName ? MediaName : "" };
}

PW_KEY_NODE_NAME and PW_KEY_MEDIA_NAME come from pipewire-0.3/pipewire/keys.h, already
included (src/AudioService.cpp:13) and already used for PW_KEY_MEDIA_CLASS.

GetMatchingNodes returns std::vector<guint32>. Its ranking is unchanged in intent — configured
entry index, then ascending bound id — but RankedNode now holds the id directly and the match runs
on MakeNodeProperties(WP_PIPEWIRE_OBJECT(Entry.Node.get())). Update the header comment: it returns
ids, not names, and front() is still the representative.

3. Address nodes by id throughout AudioService

Change the actuator signatures in includes/AudioService.h and their bodies in
src/AudioService.cpp from const std::string& nodeName to guint32 nodeId:
GetMute, SetVolume, SetMute, SetDefaultNode, IsDefaultNode, GetConnectionState(guint32 leftNode, guint32 rightNode), SetConnectionState(guint32 leftNode, guint32 rightNode, bool bConnect). Each body replaces Nodes.find(nodeName) with Nodes.find(nodeId) and uses
Itr->second.Node.get(); every wp_proxy_get_bound_id(WP_PROXY(Itr->second.get())) that only
recomputed the key is deleted and the parameter used instead.

Two places genuinely need the name, so add one accessor rather than caching strings:

// node.name of a cached node, or an empty string when the id is unknown.
[[nodiscard]] std::string GetNodeName(guint32 nodeId) const;
  • SetDefaultNode resolves the name itself for set-default-configured-node-name
    (src/AudioService.cpp:187) and returns false when the node has no node.name.
  • SetConnectionState's LOG_DEBUG (:229) prints both ids and names.

Both link handlers (on_link_added :372-377, on_link_removed :409-414) lose their O(n) scan
over the cache: wp_link_get_linked_object_ids already yields bound ids, so look both up with
Nodes.find. Dispatch only when both endpoints are cached, as today, and match with
Sub.Left.Matches(MakeNodeProperties(...)) / Sub.Right.Matches(...) built from the two cached
proxies.

4. Node events carry ids, and membership is re-evaluated when properties change

Change the listener callbacks in includes/Actions/IAction.h to pass ids:

virtual void on_node_available(WpNode* node, guint32 nodeId) {}
virtual void on_node_removed(WpNode* node, guint32 nodeId) {}
virtual void on_node_params_changed(WpPipewireObject* node, guint32 nodeId) {}
virtual void on_node_ports_changed(WpNode* node, guint32 nodeId, gulong portsNum) {}
virtual void on_link_created(WpLink* link, guint32 leftNodeId, guint32 rightNodeId) {}
virtual void on_link_removed(WpLink* link, guint32 leftNodeId, guint32 rightNodeId) {}

Document on on_node_removed that it also fires when a node is still alive but has stopped matching
the listener's target, and that the state the action applied to it is deliberately left in place.

Add one private method and one handler to AudioService:

// Recomputes which listeners match the node, calls on_node_available for the
// ones that newly match and on_node_removed for the ones that stopped, and
// stores the new set.
void DispatchMatchChanges(guint32 nodeId, WpNode* node);

static void on_node_properties_changed(GObject* Object, GParamSpec* ParamSpec, gpointer UserData);

DispatchMatchChanges builds the bag once, walks NodeListeners to build the new matched vector,
diffs it against CachedNode::MatchedListeners (linear scans; a node is matched by a handful of
listeners at most), stores the new vector before invoking callbacks — an action may call back into
AudioService from them — and then calls on_node_available(node, nodeId) for additions and
on_node_removed(node, nodeId) for removals.

Rewrite the four node handlers:

  • on_node_added: keep the node.name requirement and the media.class contains Audio filter
    (:433-439). Compute BoundId. If Nodes.contains(BoundId), return — same double-add guard the
    link path already uses (:362). Otherwise insert a CachedNode with an empty listener vector and
    connect three handlers to the node object: params-changed, ports-changed, and
    notify::properties → on_node_properties_changed. Then call DispatchMatchChanges, which
    delivers the initial on_node_available to every matching listener. Extend the existing
    LOG_DEBUG to print the bound id alongside the name; "which Firefox is this" is the question this
    bug makes unanswerable.
  • on_node_removed: look up by bound id. Copy the matched listener vector, g_signal_handlers_ disconnect_by_data(Object, Service) so a node that outlives the cache entry cannot call back into
    the service, erase the entry, then call on_node_removed(node, BoundId) on the copied listeners.
    Dispatch from the bookkeeping, not from a fresh match: the two must stay symmetric, and a node
    whose properties changed concurrently must not silently skip its removal callback.
  • on_node_params_changed and on_node_ports_changed: dispatch to CachedNode::MatchedListeners
    instead of re-matching every subscription. These are Props/port events, not membership changes.
  • on_node_properties_changed: resolve the service and the node, then call DispatchMatchChanges.
    This is the path that makes node.name: media.name matching live — a tab retitling into a
    strip's pattern gets that strip's sticky mute/volume pushed onto it immediately, and one
    retitling out of it stops being written to on the next press.

RegisterNodeListener keeps its current semantics: a listener registered after nodes are already
cached receives no retroactive on_node_available. Nothing needs it — main.cpp:136-148 builds
every action before AudioService::Init installs the object manager, and the reconnect path
(main.cpp:54-62) calls Reset() then Init(), which replays object-added for everything.

Reset() keeps Nodes.clear(); the CachedNode destructor unrefs the proxy as before.

5. Actions consume ids

src/Actions/IAction.cpp: MatchedNodes() returns std::vector<guint32> and RepresentativeNode()
returns std::optional<guint32>; update the declarations and comments in includes/Actions/IAction.h.

  • Volume (src/Actions/Action_Volume.cpp, includes/Actions/Action_Volume.h): the on_midi
    loop and on_node_available call SetVolume(Id, …).
  • Mute (Action_Mute.cpp/.h): MatchedNodes() yields ids for the SetMute loop; RefreshState
    reads GetMute(*Representative); the three overrides take guint32 nodeId.
  • SetDefault (Action_SetDefault.cpp/.h): SetDefaultNode(*Representative); the failure message
    at :34-35 prints m_audioService->GetNodeName(*Representative).
  • Connection (Action_Connection.cpp/.h): Pairs() returns
    std::vector<std::pair<guint32, guint32>>; the self-link guard compares ids instead of names,
    which also fixes the case where two different nodes sharing a name were skipped; RefreshState,
    EnforceDesired, on_midi and all five overrides take ids. Init_Internal's combined
    left+right watch matcher (:126-131) is unchanged — it is built from pattern strings, not names.

With ids in hand, MatchedNodes() for a target of Firefox now yields one entry per live Firefox
stream, and each SetVolume/SetMute lands on that specific node.

Verification

Prerequisites: the project's declared toolchain and dev packages (mise.toml [bootstrap.packages],
including libwireplumber-0.5-dev, libglaze-dev, libcxxopts-dev, libconcurrentqueue-dev,
libmagicenum-dev) installed; commands run from the repository root.

1. Unit tests (step 1). Extend tests/NodeMatcher_tests.cpp; its existing Matches("name")
call sites become Matches(NodeProperties{"name", ""}). Add a TestPropertyCandidates() registered
in main() covering, with NodeMatcher{{"Firefox"}} and NodeMatcher{{"Firefox: .*YouTube.*"}}:

properties pattern expected
{"Firefox", ""} Firefox matches (bare name, unchanged behaviour)
{"Firefox", "Lofi girl - YouTube"} Firefox matches (bare name wins, media.name ignored)
{"Firefox", "Lofi girl - YouTube"} Firefox: .*YouTube.* matches (concatenated candidate)
{"Firefox", "Some blog"} Firefox: .*YouTube.* no match
{"Firefox", ""} Firefox: .* no match (no candidate without media.name)
{"Firefox", "x"} irefox no match (anchoring holds on both candidates)

Also assert FirstMatchingEntry(NodeProperties{"Firefox", "x - YouTube"}) on
NodeMatcher{{"Spotify", "Firefox: .*YouTube.*", "Firefox"}} returns 1, i.e. a concatenated match
ranks by its entry position like any other.

cmake -B build -DCMAKE_BUILD_TYPE=Debug -DNANOKONTROLLER2_BUILD_TESTS=ON
cmake --build build -j$(nproc)
ctest --test-dir build --output-on-failure

2. Fan-out on duplicate names (steps 2-5, the reported bug). Needs the nanoKONTROL2 attached and
a config whose Fader3/Mute3 target Firefox. Play audio in three Firefox tabs and confirm
PipeWire really shows several identically named nodes:

pw-dump | jq -r '.[] | select(.info.props["media.class"] // "" | contains("Audio")) | "\(.id)\t\(.info.props["node.name"])\t\(.info.props["media.name"])"'

Run the debug build (LOG_DEBUG is compiled in) with ./build/nanoKontroller2 -c config.json.
Expected, each checked by re-running the pw-dump command above or wpctl status:

  • startup logs one Node added line per Firefox id, not one in total;
  • moving Fader3 sets every Firefox node's volume, not one;
  • pressing Mute3 mutes all of them and the LED lights;
  • closing one tab's stream leaves the others muted and still controlled — the next Fader3 move still
    moves them. This is the exact regression that previously left the strip dead;
  • opening a new tab that starts streaming brings it up already muted and at the fader's volume.

3. Title matching and live re-evaluation (steps 1 and 4). Temporarily point a spare fader at
Firefox: .*YouTube.*. With one YouTube tab and one non-YouTube tab both producing audio, moving
that fader must change only the YouTube node's volume. Then switch the YouTube tab to another video
whose title still contains YouTube (node keeps matching, volume stays) and navigate the other
tab to a YouTube page: without touching the fader, that node must be picked up by the strip — a
Node added-free notify::properties path — and receive the strip's sticky volume/mute.

Assumptions & contingencies

  • notify::properties fires on a WpNode when PipeWire reports a props change: verified in
    WirePlumber 0.5.8 (lib/wp/private/pipewire-object-mixin.c:788, :961) and on upstream
    master. If the installed WirePlumber turns out not to emit it for media.name changes in
    practice, keep every other step and drop step 4's on_node_properties_changed wiring: matching
    still runs on every GetMatchingNodes call, so membership stays correct at each press and fader
    move, and only the sticky-state push for retitled nodes is lost.
  • PipeWire bound ids are assumed unique among live nodes, not forever; an id reused after its node
    was removed is treated as a new node, which is correct because the cache entry is erased on
    removal. If a Node added ever arrives for an id already cached, the double-add guard keeps the
    first entry, matching the existing link behaviour.
## Problem Firefox often shows up three or four times in `pw-dump` and in Plasma's tray widget, depending on which tabs currently claim to send data. A strip targeting `Firefox` should control every stream matching that name — they all carry the same `node.name` — but it controls one at most, sometimes none, and which one changes over time. The cause is that the matching layer and the addressing layer disagree about what identifies a node. `AudioService` caches nodes in `std::unordered_map<std::string, WpNode*>` keyed by `node.name` (`includes/AudioService.h:58`), and PipeWire node names are not unique: * **Insertion is first-come-wins.** `on_node_added` stores the node only when the name is absent, and connects the `params-changed` / `ports-changed` handlers inside that same guard (`src/AudioService.cpp:446-450`). Firefox nodes two, three and four pass the `media.class` filter and are then dropped, subscriptions and all. * **Enumeration cannot return them.** `GetMatchingNodes` iterates the cache and returns *names* (`src/AudioService.cpp:88-116`), so identically named streams collapse to one entry whatever the configured pattern is. * **Actuation re-resolves by name.** `Action_Volume::on_midi` loops over `MatchedNodes()`, but `SetVolume`/`SetMute`/`GetMute`/`SetDefaultNode`/`IsDefaultNode` all do `Nodes.find(nodeName)` and act on the one cached proxy (`src/AudioService.cpp:118-204`). * **Removal evicts by name.** `on_node_removed` does `Nodes.erase(NodeName)` without checking which object is leaving (`src/AudioService.cpp:475`). One tab ending deletes the slot while three live Firefox nodes remain, and those survivors are never re-added because `object-added` already fired for them. That is the "sometimes none", and which object owns the slot at any moment is decided by whichever add happened to arrive while the slot was free — the "it changes". So the fan-out contract the README promises ("a press mutes or unmutes every matched node") holds only for targets whose matches happen to have distinct `node.name` values: invisible for sinks, total for application streams. Separately, Plasma's second label line — the tab title — has no representation in the config at all. Every call site reads exactly one property, `wp_pipewire_object_get_property(…, "node.name")` (`src/AudioService.cpp:433`, `:469`, `:490`, `:508`), and `NodeMatcher` matches a single opaque string with no notion of which field it came from. ## Plan ### 1. `NodeMatcher` matches a property bag, with a second candidate string In `includes/NodeMatcher.h`, above the class, add the bag and the separator. Keep both here (not in `AudioService.h`): the unit-test target compiles `NodeMatcher.cpp` and `AppSettings.cpp` only and links no WirePlumber (`CMakeLists.txt:69-74`), so the matcher must be exercisable without a `WpNode`. ```cpp // Separates node.name from media.name in the second match candidate. Chosen to // be easy to type in a config pattern and rare inside a PipeWire node name. inline constexpr std::string_view NodeNameSeparator = ": "; // The PipeWire properties a pattern is matched against. The views point into // storage owned by the node proxy, which WirePlumber guarantees alive until // execution returns to the event loop, so a bag must never outlive the call it // was built for. struct NodeProperties { std::string_view NodeName; // Empty when the node carries no media.name. std::string_view MediaName; }; ``` Change both query methods to take the bag — `FirstMatchingEntry(const NodeProperties&)` and `Matches(const NodeProperties&)`; there is no string overload left, so every caller is forced to supply properties. In `src/NodeMatcher.cpp`, match each entry against two candidates, in order: 1. `props.NodeName`; 2. `props.NodeName + ": " + props.MediaName`, formed only when `props.MediaName` is non-empty. Build the concatenation once before the entry loop, not per entry. Match with iterators (`std::regex_match(sv.data(), sv.data() + sv.size(), m_Regexes[i])`) so no `std::string` is allocated for the bare-name candidate. Compilation and anchoring are unchanged: both candidates are matched against the same `^(?:pattern)$` regex, so a plain name still matches only itself. Entry ranking does not change and does not record which candidate matched: `FirstMatchingEntry` returns the index of the first *entry* that matched on either candidate. A node whose `media.name` is absent simply has one candidate. Update the README's "Specifying nodes" section in the same change: a pattern is matched against `node.name` and, when the node has a `media.name`, against `node.name: media.name`, so a pattern broad enough to match arbitrary text (`.*`, or a negative-lookahead "everything else" pattern) now also matches via the second candidate and exclusion lists must account for that. No config syntax is added. Selecting a specific property per entry is deliberately left out; the bag carries only the two fields that feed the candidates, and a third field is a one-line addition when a syntax issue needs it. ### 2. Key the node cache by PipeWire bound id In `includes/AudioService.h`, replace the name-keyed map with: ```cpp // One entry per live node. The key is the PipeWire bound id, which is what // every actuator and every link endpoint is expressed in, so a shared // node.name no longer collapses several nodes into one slot. struct CachedNode { std::unique_ptr<WpNode, WpNodeDeleter> Node; // Listeners whose matcher matched this node at the last evaluation. // Membership changes while a node lives, because media.name does. std::vector<IActionBase*> MatchedListeners; }; std::unordered_map<guint32, CachedNode> Nodes; ``` Add a file-local helper in `src/AudioService.cpp` that builds a bag from a proxy, used by every match site: ```cpp NodeProperties MakeNodeProperties(WpPipewireObject* Object) { const char* NodeName = wp_pipewire_object_get_property(Object, PW_KEY_NODE_NAME); const char* MediaName = wp_pipewire_object_get_property(Object, PW_KEY_MEDIA_NAME); return { NodeName ? NodeName : "", MediaName ? MediaName : "" }; } ``` `PW_KEY_NODE_NAME` and `PW_KEY_MEDIA_NAME` come from `pipewire-0.3/pipewire/keys.h`, already included (`src/AudioService.cpp:13`) and already used for `PW_KEY_MEDIA_CLASS`. `GetMatchingNodes` returns `std::vector<guint32>`. Its ranking is unchanged in intent — configured entry index, then ascending bound id — but `RankedNode` now holds the id directly and the match runs on `MakeNodeProperties(WP_PIPEWIRE_OBJECT(Entry.Node.get()))`. Update the header comment: it returns ids, not names, and `front()` is still the representative. ### 3. Address nodes by id throughout `AudioService` Change the actuator signatures in `includes/AudioService.h` and their bodies in `src/AudioService.cpp` from `const std::string& nodeName` to `guint32 nodeId`: `GetMute`, `SetVolume`, `SetMute`, `SetDefaultNode`, `IsDefaultNode`, `GetConnectionState(guint32 leftNode, guint32 rightNode)`, `SetConnectionState(guint32 leftNode, guint32 rightNode, bool bConnect)`. Each body replaces `Nodes.find(nodeName)` with `Nodes.find(nodeId)` and uses `Itr->second.Node.get()`; every `wp_proxy_get_bound_id(WP_PROXY(Itr->second.get()))` that only recomputed the key is deleted and the parameter used instead. Two places genuinely need the name, so add one accessor rather than caching strings: ```cpp // node.name of a cached node, or an empty string when the id is unknown. [[nodiscard]] std::string GetNodeName(guint32 nodeId) const; ``` * `SetDefaultNode` resolves the name itself for `set-default-configured-node-name` (`src/AudioService.cpp:187`) and returns `false` when the node has no `node.name`. * `SetConnectionState`'s `LOG_DEBUG` (`:229`) prints both ids and names. Both link handlers (`on_link_added` `:372-377`, `on_link_removed` `:409-414`) lose their O(n) scan over the cache: `wp_link_get_linked_object_ids` already yields bound ids, so look both up with `Nodes.find`. Dispatch only when both endpoints are cached, as today, and match with `Sub.Left.Matches(MakeNodeProperties(...))` / `Sub.Right.Matches(...)` built from the two cached proxies. ### 4. Node events carry ids, and membership is re-evaluated when properties change Change the listener callbacks in `includes/Actions/IAction.h` to pass ids: ```cpp virtual void on_node_available(WpNode* node, guint32 nodeId) {} virtual void on_node_removed(WpNode* node, guint32 nodeId) {} virtual void on_node_params_changed(WpPipewireObject* node, guint32 nodeId) {} virtual void on_node_ports_changed(WpNode* node, guint32 nodeId, gulong portsNum) {} virtual void on_link_created(WpLink* link, guint32 leftNodeId, guint32 rightNodeId) {} virtual void on_link_removed(WpLink* link, guint32 leftNodeId, guint32 rightNodeId) {} ``` Document on `on_node_removed` that it also fires when a node is still alive but has stopped matching the listener's target, and that the state the action applied to it is deliberately left in place. Add one private method and one handler to `AudioService`: ```cpp // Recomputes which listeners match the node, calls on_node_available for the // ones that newly match and on_node_removed for the ones that stopped, and // stores the new set. void DispatchMatchChanges(guint32 nodeId, WpNode* node); static void on_node_properties_changed(GObject* Object, GParamSpec* ParamSpec, gpointer UserData); ``` `DispatchMatchChanges` builds the bag once, walks `NodeListeners` to build the new matched vector, diffs it against `CachedNode::MatchedListeners` (linear scans; a node is matched by a handful of listeners at most), stores the new vector *before* invoking callbacks — an action may call back into `AudioService` from them — and then calls `on_node_available(node, nodeId)` for additions and `on_node_removed(node, nodeId)` for removals. Rewrite the four node handlers: * `on_node_added`: keep the `node.name` requirement and the `media.class` contains `Audio` filter (`:433-439`). Compute `BoundId`. If `Nodes.contains(BoundId)`, return — same double-add guard the link path already uses (`:362`). Otherwise insert a `CachedNode` with an empty listener vector and connect three handlers to the node object: `params-changed`, `ports-changed`, and `notify::properties` → `on_node_properties_changed`. Then call `DispatchMatchChanges`, which delivers the initial `on_node_available` to every matching listener. Extend the existing `LOG_DEBUG` to print the bound id alongside the name; "which Firefox is this" is the question this bug makes unanswerable. * `on_node_removed`: look up by bound id. Copy the matched listener vector, `g_signal_handlers_ disconnect_by_data(Object, Service)` so a node that outlives the cache entry cannot call back into the service, erase the entry, then call `on_node_removed(node, BoundId)` on the copied listeners. Dispatch from the bookkeeping, not from a fresh match: the two must stay symmetric, and a node whose properties changed concurrently must not silently skip its removal callback. * `on_node_params_changed` and `on_node_ports_changed`: dispatch to `CachedNode::MatchedListeners` instead of re-matching every subscription. These are `Props`/port events, not membership changes. * `on_node_properties_changed`: resolve the service and the node, then call `DispatchMatchChanges`. This is the path that makes `node.name: media.name` matching live — a tab retitling into a strip's pattern gets that strip's sticky mute/volume pushed onto it immediately, and one retitling out of it stops being written to on the next press. `RegisterNodeListener` keeps its current semantics: a listener registered after nodes are already cached receives no retroactive `on_node_available`. Nothing needs it — `main.cpp:136-148` builds every action before `AudioService::Init` installs the object manager, and the reconnect path (`main.cpp:54-62`) calls `Reset()` then `Init()`, which replays `object-added` for everything. `Reset()` keeps `Nodes.clear()`; the `CachedNode` destructor unrefs the proxy as before. ### 5. Actions consume ids `src/Actions/IAction.cpp`: `MatchedNodes()` returns `std::vector<guint32>` and `RepresentativeNode()` returns `std::optional<guint32>`; update the declarations and comments in `includes/Actions/IAction.h`. * **Volume** (`src/Actions/Action_Volume.cpp`, `includes/Actions/Action_Volume.h`): the `on_midi` loop and `on_node_available` call `SetVolume(Id, …)`. * **Mute** (`Action_Mute.cpp/.h`): `MatchedNodes()` yields ids for the `SetMute` loop; `RefreshState` reads `GetMute(*Representative)`; the three overrides take `guint32 nodeId`. * **SetDefault** (`Action_SetDefault.cpp/.h`): `SetDefaultNode(*Representative)`; the failure message at `:34-35` prints `m_audioService->GetNodeName(*Representative)`. * **Connection** (`Action_Connection.cpp/.h`): `Pairs()` returns `std::vector<std::pair<guint32, guint32>>`; the self-link guard compares ids instead of names, which also fixes the case where two *different* nodes sharing a name were skipped; `RefreshState`, `EnforceDesired`, `on_midi` and all five overrides take ids. `Init_Internal`'s combined left+right watch matcher (`:126-131`) is unchanged — it is built from pattern strings, not names. With ids in hand, `MatchedNodes()` for a target of `Firefox` now yields one entry per live Firefox stream, and each `SetVolume`/`SetMute` lands on that specific node. ## Verification Prerequisites: the project's declared toolchain and dev packages (`mise.toml` `[bootstrap.packages]`, including `libwireplumber-0.5-dev`, `libglaze-dev`, `libcxxopts-dev`, `libconcurrentqueue-dev`, `libmagicenum-dev`) installed; commands run from the repository root. **1. Unit tests (step 1).** Extend `tests/NodeMatcher_tests.cpp`; its existing `Matches("name")` call sites become `Matches(NodeProperties{"name", ""})`. Add a `TestPropertyCandidates()` registered in `main()` covering, with `NodeMatcher{{"Firefox"}}` and `NodeMatcher{{"Firefox: .*YouTube.*"}}`: | properties | pattern | expected | |---|---|---| | `{"Firefox", ""}` | `Firefox` | matches (bare name, unchanged behaviour) | | `{"Firefox", "Lofi girl - YouTube"}` | `Firefox` | matches (bare name wins, media.name ignored) | | `{"Firefox", "Lofi girl - YouTube"}` | `Firefox: .*YouTube.*` | matches (concatenated candidate) | | `{"Firefox", "Some blog"}` | `Firefox: .*YouTube.*` | no match | | `{"Firefox", ""}` | `Firefox: .*` | no match (no candidate without media.name) | | `{"Firefox", "x"}` | `irefox` | no match (anchoring holds on both candidates) | Also assert `FirstMatchingEntry(NodeProperties{"Firefox", "x - YouTube"})` on `NodeMatcher{{"Spotify", "Firefox: .*YouTube.*", "Firefox"}}` returns `1`, i.e. a concatenated match ranks by its entry position like any other. ```bash cmake -B build -DCMAKE_BUILD_TYPE=Debug -DNANOKONTROLLER2_BUILD_TESTS=ON cmake --build build -j$(nproc) ctest --test-dir build --output-on-failure ``` **2. Fan-out on duplicate names (steps 2-5, the reported bug).** Needs the nanoKONTROL2 attached and a config whose `Fader3`/`Mute3` target `Firefox`. Play audio in three Firefox tabs and confirm PipeWire really shows several identically named nodes: ```bash pw-dump | jq -r '.[] | select(.info.props["media.class"] // "" | contains("Audio")) | "\(.id)\t\(.info.props["node.name"])\t\(.info.props["media.name"])"' ``` Run the debug build (`LOG_DEBUG` is compiled in) with `./build/nanoKontroller2 -c config.json`. Expected, each checked by re-running the `pw-dump` command above or `wpctl status`: * startup logs one `Node added` line per Firefox id, not one in total; * moving Fader3 sets *every* Firefox node's volume, not one; * pressing Mute3 mutes all of them and the LED lights; * closing one tab's stream leaves the others muted and still controlled — the next Fader3 move still moves them. This is the exact regression that previously left the strip dead; * opening a new tab that starts streaming brings it up already muted and at the fader's volume. **3. Title matching and live re-evaluation (steps 1 and 4).** Temporarily point a spare fader at `Firefox: .*YouTube.*`. With one YouTube tab and one non-YouTube tab both producing audio, moving that fader must change only the YouTube node's volume. Then switch the YouTube tab to another video whose title still contains `YouTube` (node keeps matching, volume stays) and navigate the *other* tab to a YouTube page: without touching the fader, that node must be picked up by the strip — a `Node added`-free `notify::properties` path — and receive the strip's sticky volume/mute. ## Assumptions & contingencies * `notify::properties` fires on a `WpNode` when PipeWire reports a props change: verified in WirePlumber `0.5.8` (`lib/wp/private/pipewire-object-mixin.c:788`, `:961`) and on upstream `master`. If the installed WirePlumber turns out not to emit it for `media.name` changes in practice, keep every other step and drop step 4's `on_node_properties_changed` wiring: matching still runs on every `GetMatchingNodes` call, so membership stays correct at each press and fader move, and only the sticky-state push for retitled nodes is lost. * PipeWire bound ids are assumed unique among *live* nodes, not forever; an id reused after its node was removed is treated as a new node, which is correct because the cache entry is erased on removal. If a `Node added` ever arrives for an id already cached, the double-add guard keeps the first entry, matching the existing link behaviour.
omp-deck commented 2026-11-13 09:48:31 +01:00
Collaborator

Analysis

There are two separate things in this report: a genuine defect (fan-out silently collapses to one node) and a missing capability (only one PipeWire property is ever matched). They have different causes and only the first one explains the "random/none" behaviour.

1. Why only one Firefox node is controlled, and why it moves

The matching layer and the addressing layer disagree about what identifies a node.

NodeMatcher matches strings, and AudioService caches nodes in std::unordered_map<std::string, WpNode*> Nodes keyed by node.name (includes/AudioService.h:58, src/AudioService.cpp:446). PipeWire node names are not unique — a per-stream node name like Firefox is shared by every stream the application opens — but the cache treats the name as a primary key. One name, one slot. Everything downstream inherits that:

  • Insertion is first-come-wins. on_node_added only stores the node when the name is not present yet, and the params-changed / ports-changed handlers are connected inside that same guard (src/AudioService.cpp:446-450). Firefox nodes two, three and four reach the callback, pass the media.class contains Audio filter, and are then dropped on the floor — including their signal subscriptions.
  • Enumeration can never return them. GetMatchingNodes iterates Nodes and returns names (src/AudioService.cpp:87-114). With all streams called Firefox it returns a one-element vector no matter how many streams exist. The careful ranking in that function (entry index, then bound id, "front() is the representative") only ever ranks the survivors of the collapse.
  • Actuation re-resolves by name. Action_Volume::on_midi does loop over MatchedNodes() and calls SetVolume(Name, …) per entry (src/Actions/Action_Volume.cpp:17-20), but SetVolume/SetMute/GetMute/SetDefaultNode/IsDefaultNode all do Nodes.find(nodeName) and act on the single cached proxy. So even the dispatch in on_node_available, which is delivered for every matching node including the uncached duplicates, writes the volume onto the one cached object instead of the node that just appeared.

This is what makes it one at most. The "sometimes none" and "it changes" parts come from removal:

  • on_node_removed does Nodes.erase(NodeName) (src/AudioService.cpp:475) — by name, with no check that the object leaving is the object stored under that name. A tab that stops claiming to send data evicts the cache entry even when three other Firefox nodes are still alive. Those survivors will never be re-added: object-added already fired for them and will not fire again. The strip then controls nothing called Firefox until some Firefox stream appears afresh.
  • Which object owns the slot at any moment is therefore decided by the accident of which add arrived while the slot happened to be free. With tabs starting and stopping streams, that is effectively random, and it changes under you — matching your observation exactly.

So the fan-out semantics the README promises ("A press mutes or unmutes every matched node") hold only for targets whose matched nodes happen to have distinct node.name values. The multi-match work from #3 / #13 built list and regex matching on top of a name-keyed cache that predates it; the collapse is invisible for sinks (unique names) and total for application streams. The LED side has the same root: RepresentativeNode() returns a name, and Action_Mute reads its state back through the same single-slot lookup, so the LED reports a node that may no longer be the one your fader moved.

2. Why only node.name is matchable

Independently of the above, every call site reads exactly one property: wp_pipewire_object_get_property(…, "node.name") in on_node_added, on_node_removed, on_node_params_changed and on_node_ports_changed. application.name and media.name are never read, and NodeMatcher has no notion of which field a pattern applies to — its API is Matches(const std::string& nodeName), a single opaque string. That is why what Plasma shows as a two-line label (application plus stream/tab title) has no representation in the config at all.

Worth noting that the two issues are coupled in one direction: matching on media.name would let you distinguish Firefox tabs, but as long as the cache key and the whole action-facing API (SetVolume(name), GetMute(name), RepresentativeNode() -> string) are a node name, any selector that resolves to several live nodes keeps collapsing to one. Conversely, media.name is a moving target by nature — it tracks the tab title and changes while the node lives — so it is a matching key with different stability properties than node.name, which the current add-time-only matching does not account for (matching happens on object-added; a node whose matched property changes later is never re-evaluated).

Related

  • #3 / #13 — introduced list/regex targets and the fan-out contract that this issue shows is not actually reached.
  • #4 ("other" node alias) and #10 ("current" node alias) — both are selectors that resolve to a set of live nodes, so both sit on the same name-as-identity assumption.
  • #16 (retarget overlay) — depends on addressing an individual node unambiguously.

Questions

  1. When the strip controls nothing: does it recover on its own if you let every Firefox stream end and then start a new one? If yes, that confirms the erase-by-name eviction path above; if the strip stays dead even then, something else is also wrong.
  2. In pw-dump, do the Firefox nodes differ in object.serial/node.id only, or does any of them carry a distinct node.name (e.g. a separate WebRTC or decoder node)? This decides whether the collapse is total for Firefox or whether you are seeing a partial one.
  3. Does the strip that misbehaves target Firefox as a plain entry, as a regex, or through a FriendlyNames alias such as players? Entry-index ranking interacts with the collapse, and an alias list would put a second collapsing name in play.
## Analysis There are two separate things in this report: a genuine defect (fan-out silently collapses to one node) and a missing capability (only one PipeWire property is ever matched). They have different causes and only the first one explains the "random/none" behaviour. ### 1. Why only one Firefox node is controlled, and why it moves The matching layer and the addressing layer disagree about what identifies a node. `NodeMatcher` matches *strings*, and `AudioService` caches nodes in `std::unordered_map<std::string, WpNode*> Nodes` keyed by `node.name` (`includes/AudioService.h:58`, `src/AudioService.cpp:446`). PipeWire node names are not unique — a per-stream node name like `Firefox` is shared by every stream the application opens — but the cache treats the name as a primary key. One name, one slot. Everything downstream inherits that: * **Insertion is first-come-wins.** `on_node_added` only stores the node when the name is not present yet, and the `params-changed` / `ports-changed` handlers are connected inside that same guard (`src/AudioService.cpp:446-450`). Firefox nodes two, three and four reach the callback, pass the `media.class` contains `Audio` filter, and are then dropped on the floor — including their signal subscriptions. * **Enumeration can never return them.** `GetMatchingNodes` iterates `Nodes` and returns *names* (`src/AudioService.cpp:87-114`). With all streams called `Firefox` it returns a one-element vector no matter how many streams exist. The careful ranking in that function (entry index, then bound id, "front() is the representative") only ever ranks the survivors of the collapse. * **Actuation re-resolves by name.** `Action_Volume::on_midi` does loop over `MatchedNodes()` and calls `SetVolume(Name, …)` per entry (`src/Actions/Action_Volume.cpp:17-20`), but `SetVolume`/`SetMute`/`GetMute`/`SetDefaultNode`/`IsDefaultNode` all do `Nodes.find(nodeName)` and act on the single cached proxy. So even the dispatch in `on_node_available`, which *is* delivered for every matching node including the uncached duplicates, writes the volume onto the one cached object instead of the node that just appeared. This is what makes it one at most. The "sometimes none" and "it changes" parts come from removal: * `on_node_removed` does `Nodes.erase(NodeName)` (`src/AudioService.cpp:475`) — by name, with no check that the object leaving is the object stored under that name. A tab that stops claiming to send data evicts the cache entry even when three other Firefox nodes are still alive. Those survivors will never be re-added: `object-added` already fired for them and will not fire again. The strip then controls *nothing* called Firefox until some Firefox stream appears afresh. * Which object owns the slot at any moment is therefore decided by the accident of which add arrived while the slot happened to be free. With tabs starting and stopping streams, that is effectively random, and it changes under you — matching your observation exactly. So the fan-out semantics the README promises ("A press mutes or unmutes every matched node") hold only for targets whose matched nodes happen to have distinct `node.name` values. The multi-match work from #3 / #13 built list and regex matching on top of a name-keyed cache that predates it; the collapse is invisible for sinks (unique names) and total for application streams. The LED side has the same root: `RepresentativeNode()` returns a name, and `Action_Mute` reads its state back through the same single-slot lookup, so the LED reports a node that may no longer be the one your fader moved. ### 2. Why only `node.name` is matchable Independently of the above, every call site reads exactly one property: `wp_pipewire_object_get_property(…, "node.name")` in `on_node_added`, `on_node_removed`, `on_node_params_changed` and `on_node_ports_changed`. `application.name` and `media.name` are never read, and `NodeMatcher` has no notion of *which* field a pattern applies to — its API is `Matches(const std::string& nodeName)`, a single opaque string. That is why what Plasma shows as a two-line label (application plus stream/tab title) has no representation in the config at all. Worth noting that the two issues are coupled in one direction: matching on `media.name` would let you *distinguish* Firefox tabs, but as long as the cache key and the whole action-facing API (`SetVolume(name)`, `GetMute(name)`, `RepresentativeNode() -> string`) are a node *name*, any selector that resolves to several live nodes keeps collapsing to one. Conversely, `media.name` is a moving target by nature — it tracks the tab title and changes while the node lives — so it is a matching key with different stability properties than `node.name`, which the current add-time-only matching does not account for (matching happens on `object-added`; a node whose matched property changes later is never re-evaluated). ### Related * #3 / #13 — introduced list/regex targets and the fan-out contract that this issue shows is not actually reached. * #4 ("other" node alias) and #10 ("current" node alias) — both are selectors that resolve to a set of live nodes, so both sit on the same name-as-identity assumption. * #16 (retarget overlay) — depends on addressing an individual node unambiguously. ### Questions 1. When the strip controls *nothing*: does it recover on its own if you let every Firefox stream end and then start a new one? If yes, that confirms the erase-by-name eviction path above; if the strip stays dead even then, something else is also wrong. 2. In `pw-dump`, do the Firefox nodes differ in `object.serial`/`node.id` only, or does any of them carry a distinct `node.name` (e.g. a separate WebRTC or decoder node)? This decides whether the collapse is total for Firefox or whether you are seeing a partial one. 3. Does the strip that misbehaves target `Firefox` as a plain entry, as a regex, or through a `FriendlyNames` alias such as `players`? Entry-index ranking interacts with the collapse, and an alias list would put a second collapsing name in play.
lucas closed this issue 2026-11-13 14:07:26 +01:00
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
v1.0.2
v1.0.1
v1.0.0
Labels
Clear labels   
Agent
Ignore
 omp-deck skips this pull request

  
Agent
In Progress
 An omp-deck implementation is running

  
Agent
Planned
 An approved plan has been written to the issue

  
Agent
Planning
 An omp-deck planning session is running

  
Agent
Ready
 Starts an omp-deck implementation of the issue

  
Agent
Triage
 Starts an omp-deck triage session on the issue

  
Compat
Breaking
 Breaking change that won't be backward compatible

  
Kind
Bug
 Something is not working

  
Kind
Documentation
 Documentation changes

  
Kind
Enhancement
 Improve existing functionality

  
Kind
Feature
 New functionality

  
Kind
Security
 This is security issue

  
Kind
Testing
 Issue or pull request related to testing

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Reviewed
Confirmed
 Issue has been confirmed

  
Reviewed
Duplicate
 This issue or pull request already exists

  
Reviewed
Invalid
 Invalid issue

  
Reviewed
Won't Fix
 This issue won't be fixed

  
Status
Abandoned
 Somebody has started to work on this but abandoned work

  
Status
Blocked
 Something is blocking this issue or pull request

  
Status
Need More Info
 Feedback is required to reproduce issue or to continue work

No labels
Agent
Ignore
Agent
In Progress
Agent
Planned
Agent
Planning
Agent
Ready
Agent
Triage
Compat
Breaking
Kind
Bug
Kind
Documentation
Kind
Enhancement
Kind
Feature
Kind
Security
Kind
Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
lucas/nanoKontroller2#18
No description provided.