Repository navigation
feat(player): identify songs on Linux from Harbor's own audio - #82
Conversation
The song id capture on Linux links libpulse through libpulse-binding, and libpulse-sys needs the headers and pkg-config file at build time. Added to both workflows that compile the app natively on Linux: the CI check and the app build.
The Identify song button showed up on Linux but always failed with "only supported on Windows for now", because the only capture path was the WASAPI loopback. Linux now records through Pulse, which pipewire-pulse also speaks, and it records a single sink input: Harbor's own mpv stream, found by the audio-client-name we already set. Unlike the Windows loopback, a call or a browser tab playing in the background never ends up in the clip. The rest of the pipeline (silence check, WAV, AudD, Gemini) was already portable and is shared as is. The client name moves into a constant so mpv and song_id can't drift apart. As a side effect the Gemini module stops being dead code on Linux, which clears 13 warnings.
0c15b5a to
4dcb2ff
Compare
|
Notes on the force-push and on the red checks, since neither is explained by the description any more. Why the branch shrank. The first push had a third commit dropping "on Windows" from the Gemini setting description. The format gate runs on changed files only, and Dropping it also surfaced that The current red is pre-existing. Both jobs fail on
The Native failures are all in Worth noting what the Native job does prove, though: it got through building and running 1458 tests, so |
|
Thanks for the Linux song identification implementation and for verifying it on PipeWire. I reviewed the updated PR, and there are two remaining issues I'd like addressed before merging. 1. [P1] Explicitly connect to Harbor's monitor source In According to PulseAudio's documentation, Could you resolve the selected sink input's sink and its corresponding monitor source, then explicitly pass that monitor source to It would also be helpful to verify the actual recording source and test identification while another application is playing audio, ensuring that only Harbor's playback is captured. 2. [P2] Distinguish between multiple Harbor instances The current sink-input lookup selects the first uncorked stream with If two Harbor instances are playing simultaneously, both could match, potentially causing song identification to capture audio from the wrong instance. Could you associate the selected sink input with the specific Harbor instance or active mpv playback session? An instance-specific identifier or another reliable stream identity would prevent accidentally selecting another instance's audio. A test with two Harbor instances playing different audio would help verify this behavior. Once these two issues are addressed, I'll re-review the updated implementation. |
set_monitor_stream() only isolates a sink input when the recording stream sits on the monitor source of the sink that input plays to. The capture passed None to connect_record, so the server picked the default source instead, which on a normal desktop is a capture chain and not a monitor of anything. Resolve the selected sink input's sink, ask it for its monitor source and connect the recording stream to that by name. pipewire-pulse happens to honour set_monitor_stream whatever source it is given, so this is a no-op there, but it is what the documented contract asks for and it is what a plain PulseAudio server needs.
The sink-input lookup took the first uncorked stream announcing the mpv client name, so two Harbor instances playing at once could each identify the other's audio. libmpv plays from inside the Harbor process, so its Pulse client carries our own pid. Resolve the client behind each candidate sink input and keep only the ones belonging to this process. As a side effect the WebView, which registers as "harbor" from a child process, can no longer match either. The predicate is a plain function so the selection rules are covered by tests without a running sound server.
|
Both addressed, one commit each. Thanks for the [P1] Explicit monitor source — I measured the before/after against a
The third column being loud on both rows confirms the tone really was on the sink; the two zeroes confirm One honest correction to my own reasoning, since it changes how this should be read: on pipewire-pulse the old code was not broken. Columns one and two are identical, because the compat layer honours [P2] Instance identity — rather than inventing an identifier, there is already an exact one: libmpv plays from inside the Harbor process, so the Pulse client behind its sink input carries It also closes a gap I had not thought about: the WebView registers as The selection rules are a plain Verification
The two pre-existing follow-ups from my earlier comment still stand unchanged: the |
|
Correction to my previous comment, before you re-review: I claimed the P1 commit is "what a plain PulseAudio server needs per the documented contract", and said I could not test that. I have now tested it, and the claim was wrong. Why I had skipped it. This machine runs PipeWire, and the How it turned out to be testable after all. The conflict is only about owning the system audio stack. So: the real PulseAudio 17.0 daemon, with the default source deliberately set to
Identical to PipeWire: the first two columns match on both rows. And the mechanism is visible rather than inferred — while the first case was recording, So the original code was correct on both server implementations, and P1 fixes no user-visible behaviour on either. What it does buy is that the code no longer depends on that redirect at all: it names the source it wants, which is what the Given that, P1 is your call and I have no stake in keeping it:
Either way P2 stands on its own: the multiple-instance problem was real, and the pid-based client match is unaffected by any of this. Caveat on the above: one daemon version (17.0) and null sinks, not a hardware capture device as the default source. The source-output evidence suggests the redirect is independent of the source's type, but I have not tried every shape. |
Use a stable per-process UUID in the mpv PulseAudio client name and match that exact name during song identification, in addition to the Pulse client PID. Independent Flatpak sandboxes can reuse the same PID; this prevents matching their unrelated playback streams. Keep the Windows and macOS audio-client names unchanged and add same-PID isolation tests.
Summary
Song identification now works on Linux. The
Identify songbutton was already visible there, but every press came back with"Song identification is only supported on Windows for now", because the only capture path was the WASAPI loopback.Linux records through Pulse (which pipewire-pulse also speaks) and attaches to a single sink input: Harbor's own mpv stream, located by the
audio-client-namewe already set. The rest of the pipeline — silence check, WAV muxing, AudD, Gemini — was already portable and is shared as is.Two commits:
build(linux)—libpulse-devin the two workflows that compile natively on Linux.feat(player)— the Pulse capture path, plus theCargo.toml/Cargo.lockentry forlibpulse-binding.Why
Picking Harbor's sink input rather than a device loopback is the better fit, not just the portable one: a call or a browser tab playing in the background can never end up in the clip, which the Windows loopback cannot guarantee.
audio-client-nameis now apub(crate) const AUDIO_CLIENT_NAMEinmpv.rs, so the string mpv announces and the stringsong_idlooks for cannot drift apart.As a side effect the Gemini module stops being dead code on Linux, which takes
cargo checkfrom 25 warnings to 12.Verification
cargo check --manifest-path src-tauri/Cargo.toml— clean. Warning count drops from 25 onbeta-branchto 12, all 12 pre-existing and unrelated.tsc --noEmit -p tsconfig.json— clean.vp fmt --checkon the changed files in this PR — passes.pactl list sink-inputsshowsapplication.name = "Harbor"for the mpv stream, distinct from the lowercase"harbor"of the WebView. The case-sensitive match plus the corked check selects the mpv one.Platform Impact
Two follow-ups I deliberately left out of this PR, both to keep it focused:
provider-keys.tsx, plus 16 locale catalogs). I had the fix ready, butsettings-refinements.tsis unformatted relative to the repo's own convention in every locale, so CI's changed-filesvp fmt --checkgate turns that one-phrase edit into ~8,300 insertions / 4,200 deletions of pure rewrapping. That reformat deserves its own PR, or a repo-wide pass, rather than riding along with a native feature. Happy to open either.flatpak/cargo-sources.jsonwill needlibpulse-bindingandlibpulse-sysbefore the next Flatpak build. I did not touch it: the file is already ~300 entries behindbeta-branch's ownCargo.lock, so hand-inserting four entries would imply it is current when it isn't —flatpak/update-flatpak.shregenerates it properly. The Flatpak workflow only runs on pushes tomain, so it does not gate this PR.Checklist
vp checkfor the changed files.vp run typecheckafter TypeScript changes.