diff --git a/Cargo.lock b/Cargo.lock index e390e62..47eeea5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -706,6 +706,7 @@ dependencies = [ "crossterm", "dirs", "flume", + "libc", "mpris-server", "notify-rust", "ratatui", diff --git a/Cargo.toml b/Cargo.toml index 0cef08d..86299a4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,6 +49,9 @@ futures = "0.3" gloo-timers = { version = "0.3", features = ["futures"] } http = "1" include_dir = "0.7" +# `dup2` only: the TUI points stderr at a file so C-library writes (ALSA +# underrun messages) cannot scribble on the interface (cbd-tui/src/stderr.rs). +libc = "0.2" leptos = { version = "0.8", default-features = false, features = ["csr"] } notify-rust = "4" # The MPRIS D-Bus surface of cbd-tui. Built on zbus, which speaks the diff --git a/architecture/queue-order.md b/architecture/queue-order.md index cee07cf..d1e0790 100644 --- a/architecture/queue-order.md +++ b/architecture/queue-order.md @@ -90,13 +90,14 @@ the playback loop like every other queue verb. 3. **Same artist and title**, normalized. Would also catch the same song from two different providers. -**Decision: 2, falling back to 1** (D3). 3 is rejected: identical -artist/title is routinely a *different recording* — a live take, a remaster, -a radio edit, the studio version — and the server cannot tell which. Dropping -one would be a silent, unrecoverable edit of the user's queue, and the false -positives land exactly on the collections (greatest-hits, live albums) where -a user is most deliberate. A fuzzy dedup belongs behind a client-side preview -where the user confirms each pair; that is Deferred. +**Decision: 2 by default, falling back to 1** (D3) — and **3 behind an +explicit flag** (D3a). 3 cannot be the default: identical artist/title is +routinely a *different recording* — a live take, a remaster, a radio edit, the +studio version — and the server cannot tell which, so a default that merged +them would silently and unrecoverably edit the queue, with the false positives +landing exactly on the collections (greatest-hits, live albums, remix albums) +where a user is most deliberate. But it is what a listener sometimes means, so +it is reachable in one keypress and never by accident. ## Decisions @@ -115,8 +116,8 @@ where the user confirms each pair; that is Deferred. bounded result channel, the way `SaveQueue` already reports. `SortQueueResponse` stays empty: the new order *is* the answer, and it arrives on the stream. -- **D3 — Duplicate identity is `(provider, provider_item_id)`, or the whole - path.** The provider is the first path segment; ids are provider-internal +- **D3 — The default duplicate identity is `(provider, provider_item_id)`, or + the whole path.** The provider is the first path segment; ids are provider-internal (the content store keys them the same way, `by_provider_id(provider, id)`), so leaving them unscoped would let two providers' numeric ids collide and merge two unrelated tracks. When the id is empty — most providers, and @@ -125,6 +126,20 @@ where the user confirms each pair; that is Deferred. streaming original are **not** duplicates, because they are different providers. Conservative on purpose: a missed duplicate is a keypress, a wrong merge is lost queue state. +- **D3a — …and an opt-in `by_title` identity beside it.** Measured against a + real queue, D3 alone removed nothing from 121 entries — every entry was a + distinct provider item — while the *user* saw duplicates: four "Referees + Don't Fall In Love", three "Sink Into The Hips". They were remixes, edits and + album versions of the same songs, which D3 is right to keep and which a + listener may still not want three of. So `DedupQueue` takes a flag: with + `by_title` the identity is the lowercased `(artist, title)` pair, and the + survivor of a group is the **longest** take (the full version rather than a + radio edit), still yielding to the playing entry. It stays opt-in and on its + own key, because it *discards recordings that differ* — the exact loss D3 + refuses to make the default. Duration-tolerant matching was the third option + and is not worth its complexity: on the queue that motivated this, every + same-title group differed by tens of seconds, so any tolerance narrow enough + to be safe caught nothing. - **D4 — The survivor is the current track, else the earliest.** Within a group of duplicates, the entry at the current position survives if it is in the group; otherwise the earliest one does, and every later copy goes. @@ -187,8 +202,10 @@ where the user confirms each pair; that is Deferred. `play_order`/`current_offset` bookkeeping, because that invariant is theirs. The split is what keeps D3/D7/D8 testable as data instead of through a queue. -- **D14 — TUI: `u` dedups, `S` opens a sort menu.** `u` is "unique" and is - free in the queue scope. Sorting needs a choice, so `S` opens a modal +- **D14 — TUI: `u` dedups, `U` dedups by title, `S` opens a sort menu.** `u` + is "unique" and is free in the queue scope; the shifted form is the shifted + *behaviour* (D3a), which is the pattern `w`/`W` and `c`/`C` already use here + for "the same verb, more of it". Sorting needs a choice, so `S` opens a modal overlay listing the five strategies with their letters (`a` artist, `l` album, `t` title, `d` duration, `r` reverse; the capital of each sorts descending), `Esc` closes. A menu rather than a chord sequence: the @@ -323,8 +340,9 @@ loop -> client: SortQueueResponse {} ## Deferred -- **Fuzzy dedup** (artist/title matching) behind a client-side confirmation - view — the only safe home for it (see the options above). +- **A preview for `by_title`** (D3a): a confirmation view listing which + recording of each song would survive, so the aggressive identity can be + inspected before it removes anything rather than only undone by re-queueing. - **`ReorderQueue`/move**: drag-and-drop in the web client and `K`/`J` row moves in the TUI, on an RPC that identifies rows by path rather than index (Option C). diff --git a/cbd-cli/src/client.rs b/cbd-cli/src/client.rs index b945f5f..9e3a1ab 100644 --- a/cbd-cli/src/client.rs +++ b/cbd-cli/src/client.rs @@ -241,14 +241,19 @@ async fn run_queue(client: &mut Client, cmd: QueueCmd) -> Result<(), Box { + QueueCmd::Dedup { titles } => { let response = client - .dedup_queue(DedupQueueRequest {}) + .dedup_queue(DedupQueueRequest { by_title: titles }) .await .map_err(rpc_error)?; // 0 is a real answer — "there were no duplicates" — so it is // printed like any other count (architecture/queue-order.md D2). - println!("removed {} duplicate(s)", response.into_inner().removed); + let removed = response.into_inner().removed; + if titles { + println!("removed {removed} same-title duplicate(s)"); + } else { + println!("removed {removed} duplicate(s)"); + } } QueueCmd::Sort { key, desc } => { client diff --git a/cbd-cli/src/lib.rs b/cbd-cli/src/lib.rs index bdc76a6..c93d97e 100644 --- a/cbd-cli/src/lib.rs +++ b/cbd-cli/src/lib.rs @@ -107,7 +107,13 @@ pub enum QueueCmd { }, /// Drop duplicate entries, keeping one copy of each track. Prints how /// many were removed; never removes the playing track. - Dedup, + Dedup { + /// Collapse same artist+title instead of the provably-same item: one + /// entry per *song*, keeping the longest take. Aggressive — a remix and + /// the album version count as the same song. + #[arg(long)] + titles: bool, + }, /// Reorder the queue by one strategy (architecture/queue-order.md). Sort { #[arg(value_enum)] @@ -431,12 +437,19 @@ mod tests { } #[test] - fn queue_dedup_takes_no_arguments() { + fn queue_dedup_takes_an_optional_titles_flag() { let cli = TuiCli::try_parse_from(["cbd-tui", "queue", "dedup"]).expect("parse"); - assert!(matches!( - cli.command, - Some(TuiCommand::Queue(QueueCmd::Dedup)) - )); + match cli.command { + Some(TuiCommand::Queue(QueueCmd::Dedup { titles })) => { + assert!(!titles, "the safe identity unless asked") + } + other => panic!("unexpected: {other:?}"), + } + let cli = TuiCli::try_parse_from(["cbd-tui", "queue", "dedup", "--titles"]).expect("parse"); + match cli.command { + Some(TuiCommand::Queue(QueueCmd::Dedup { titles })) => assert!(titles), + other => panic!("unexpected: {other:?}"), + } } /// The strategy is a value enum, so it is spelled lowercase, completes in diff --git a/cbd-tui/Cargo.toml b/cbd-tui/Cargo.toml index e371c88..3e03146 100644 --- a/cbd-tui/Cargo.toml +++ b/cbd-tui/Cargo.toml @@ -24,6 +24,7 @@ clap.workspace = true dirs.workspace = true toml.workspace = true flume.workspace = true +libc.workspace = true mpris-server = { workspace = true, optional = true } notify-rust = { workspace = true, optional = true } ratatui.workspace = true diff --git a/cbd-tui/src/app/bindings.rs b/cbd-tui/src/app/bindings.rs index fc58309..16f4256 100644 --- a/cbd-tui/src/app/bindings.rs +++ b/cbd-tui/src/app/bindings.rs @@ -127,6 +127,10 @@ pub enum Action { /// removed appears in the pane title for a few seconds /// (architecture/queue-order.md). QueueDedup, + /// Drop entries with the same artist and title, keeping the longest take — + /// one entry per *song* rather than per recording. Aggressive and opt-in: + /// it discards remixes and edits (architecture/queue-order.md D3a). + QueueDedupTitles, /// Open the sort menu: one keypress there reorders the whole queue /// (architecture/queue-order.md D14). Modal — while it is open, this /// table is unreachable. @@ -568,6 +572,13 @@ pub const BINDINGS: &[Binding] = &[ action: Action::QueueDedup, description: "Unique: drop duplicate tracks from the queue", }, + Binding { + scope: Scope::Queue, + mods: KeyModifiers::SHIFT, + code: KeyCode::Char('U'), + action: Action::QueueDedupTitles, + description: "Unique by title: one entry per song, keeping the longest", + }, Binding { scope: Scope::Queue, mods: KeyModifiers::SHIFT, @@ -1100,7 +1111,15 @@ mod tests { ), Some(Action::QueueSortMenu) ); - for code in [KeyCode::Char('u'), KeyCode::Char('S')] { + assert_eq!( + lookup( + UiFocus::Queue, + false, + key(KeyCode::Char('U'), KeyModifiers::SHIFT) + ), + Some(Action::QueueDedupTitles) + ); + for code in [KeyCode::Char('u'), KeyCode::Char('U'), KeyCode::Char('S')] { assert_eq!( lookup(UiFocus::Library, false, key(code, KeyModifiers::NONE)), None, diff --git a/cbd-tui/src/app/mod.rs b/cbd-tui/src/app/mod.rs index bda0263..793e060 100644 --- a/cbd-tui/src/app/mod.rs +++ b/cbd-tui/src/app/mod.rs @@ -156,7 +156,10 @@ pub enum MessageFromUi { ClearQueue(bool), /// Drop duplicate queue entries; the reply comes back as /// [`MessageToUi::QueueDeduped`] (architecture/queue-order.md). - DedupQueue, + /// `by_title` asks for the aggressive same-song identity (D3a). + DedupQueue { + by_title: bool, + }, /// Reorder the queue by one strategy. Fire-and-forget: the new order /// arrives on the update stream like every other queue change. SortQueue { @@ -746,12 +749,13 @@ impl App { self.set_register(dropped); let _ = self.tx.send(MessageFromUi::ClearQueue(false)); } - Action::QueueDedup => { + Action::QueueDedup | Action::QueueDedupTitles => { // Server-side (architecture/queue-order.md D1): the count // comes back as `MessageToUi::QueueDeduped`. Nothing to do on // an empty queue. if !self.queue.is_empty() { - let _ = self.tx.send(MessageFromUi::DedupQueue); + let by_title = action == Action::QueueDedupTitles; + let _ = self.tx.send(MessageFromUi::DedupQueue { by_title }); } } Action::QueueSortMenu => { @@ -1830,7 +1834,17 @@ mod tests { app.queue.update_queue(one_track_queue()); let _ = app.dispatch(Action::QueueDedup); - assert!(matches!(rx.try_recv(), Ok(MessageFromUi::DedupQueue))); + assert!(matches!( + rx.try_recv(), + Ok(MessageFromUi::DedupQueue { by_title: false }) + )); + + // `U` is the same verb with the aggressive identity (D3a). + let _ = app.dispatch(Action::QueueDedupTitles); + assert!(matches!( + rx.try_recv(), + Ok(MessageFromUi::DedupQueue { by_title: true }) + )); } #[test] diff --git a/cbd-tui/src/lib.rs b/cbd-tui/src/lib.rs index f440aad..1f9aa6f 100644 --- a/cbd-tui/src/lib.rs +++ b/cbd-tui/src/lib.rs @@ -7,6 +7,7 @@ pub mod app; pub mod config; pub mod rpc; +pub mod stderr; #[cfg(feature = "mpris")] pub mod mpris; @@ -96,7 +97,13 @@ async fn orchestrate( let mut rpc_client = rpc::RpcClient::connect(&config.server).await?; if let Some(root_node) = rpc_client.get_library_node(crabidy_core::ROOT_PATH).await? { - tx.send(MessageToUi::ReplaceLibraryNode(root_node.clone()))?; + if tx + .send(MessageToUi::ReplaceLibraryNode(root_node.clone())) + .is_err() + { + info!("the ui closed before the library root arrived"); + return Ok(()); + } } // A desktop that cannot host the player (no session bus) leaves this @@ -108,21 +115,40 @@ async fn orchestrate( if let Some(mpris) = &mpris { mpris.publish_init(&init_data); } - tx.send_async(MessageToUi::Init(init_data)).await?; + if tx.send_async(MessageToUi::Init(init_data)).await.is_err() { + info!("the ui closed before the initial state arrived"); + return Ok(()); + } loop { - if let Err(err) = poll(&mut rpc_client, &rx, &tx, &mpris).await { - error!("request to server failed: {err}"); + match poll(&mut rpc_client, &rx, &tx, &mpris).await { + Ok(Flow::Continue) => {} + // The UI thread owns the other end; quitting drops it. That is a + // shutdown, not a failed request — reporting it as one produced an + // ERROR line ("sending on a closed channel") on every clean exit, + // and the loop then spun on a stream nobody was listening to. + Ok(Flow::UiGone) => { + info!("the ui closed, stopping the orchestrator"); + return Ok(()); + } + Err(err) => error!("request to server failed: {err}"), } } } +/// What the orchestrator does after one poll. +enum Flow { + Continue, + /// The `MessageToUi` receiver is gone: the terminal UI has exited. + UiGone, +} + async fn poll( rpc_client: &mut RpcClient, rx: &Receiver, tx: &Sender, mpris: &Option, -) -> Result<(), Box> { +) -> Result> { select! { Ok(msg) = &mut rx.recv_async() => { match msg { @@ -218,11 +244,11 @@ async fn poll( MessageFromUi::ClearQueue(exclude_current) => { rpc_client.clear_queue(exclude_current).await? } - MessageFromUi::DedupQueue => { + MessageFromUi::DedupQueue { by_title } => { // The count is the whole point (architecture/queue-order.md // D2); a failure is logged and the queue simply stays as it // is — it must not tear down the poll loop. - match rpc_client.dedup_queue().await { + match rpc_client.dedup_queue(by_title).await { Ok(removed) => { let _ = tx.send(MessageToUi::QueueDeduped { removed }); } @@ -262,7 +288,9 @@ async fn poll( if let Some(mpris) = mpris { mpris.publish(&update); } - tx.send_async(MessageToUi::Update(update)).await?; + if tx.send_async(MessageToUi::Update(update)).await.is_err() { + return Ok(Flow::UiGone); + } } } Err(err) => { @@ -275,7 +303,7 @@ async fn poll( } } - Ok(()) + Ok(Flow::Continue) } fn run_ui( diff --git a/cbd-tui/src/main.rs b/cbd-tui/src/main.rs index e787726..8ea7fa0 100644 --- a/cbd-tui/src/main.rs +++ b/cbd-tui/src/main.rs @@ -18,14 +18,12 @@ const CONFIG_FILE: &str = "cbd-tui.toml"; static CONFIG: OnceLock = OnceLock::new(); /// Logs to a file: the terminal is owned by the TUI, so writing log lines to -/// stdout/stderr would corrupt the interface. +/// stdout/stderr would corrupt the interface. The same reason stderr itself is +/// redirected — see [`cbd_tui::stderr`]. fn init_tracing() -> Option { use tracing_subscriber::{prelude::*, EnvFilter}; - let log_dir = dirs::state_dir() - .or_else(dirs::cache_dir) - .unwrap_or_else(std::env::temp_dir) - .join("crabidy"); + let log_dir = cbd_tui::stderr::log_dir(); if let Err(err) = std::fs::create_dir_all(&log_dir) { eprintln!( "could not create log directory {}: {err}", @@ -56,6 +54,16 @@ async fn main() -> Result<(), Box> { // No subcommand: load config, apply overrides, run the TUI. None => { let _log_guard = init_tracing(); + // Before the alternate screen: from here on, a write to fd 2 that + // did not go through tracing (a panic message, ALSA's underrun + // chatter) would land on the interface. + let stderr_log = cbd_tui::stderr::log_dir().join("cbd-tui.stderr.log"); + if let Err(err) = cbd_tui::stderr::capture_into(&stderr_log) { + eprintln!( + "could not redirect stderr to {}: {err}", + stderr_log.display() + ); + } let mut config = config::load_first_run(CONFIG_FILE); config::apply_overrides( &mut config, diff --git a/cbd-tui/src/rpc.rs b/cbd-tui/src/rpc.rs index 98a4eec..4bded79 100644 --- a/cbd-tui/src/rpc.rs +++ b/cbd-tui/src/rpc.rs @@ -284,10 +284,10 @@ impl RpcClient { /// (architecture/queue-order.md D2) — the one queue verb with an answer /// worth showing, because `0` means "no duplicates", not "nothing /// happened". - pub async fn dedup_queue(&mut self) -> Result> { + pub async fn dedup_queue(&mut self, by_title: bool) -> Result> { let response = self .client - .dedup_queue(Request::new(DedupQueueRequest {})) + .dedup_queue(Request::new(DedupQueueRequest { by_title })) .await?; Ok(response.into_inner().removed) } diff --git a/cbd-tui/src/stderr.rs b/cbd-tui/src/stderr.rs new file mode 100644 index 0000000..499607c --- /dev/null +++ b/cbd-tui/src/stderr.rs @@ -0,0 +1,112 @@ +//! Keeping foreign writes off the TUI's screen. +//! +//! `tracing` goes to a log file precisely because the terminal belongs to the +//! interface — but that only covers what *we* log. File descriptor 2 still +//! points at the terminal, and plenty in this process never goes through +//! `tracing`: +//! +//! - ALSA prints from C (`ALSA lib pcm.c:…: underrun occurred`), which the +//! bundled `cbd` sees because the audio stack shares its process. +//! - the default panic hook, and anything a dependency decides to +//! `eprintln!`. +//! +//! Those bytes land wherever the cursor is, scroll the terminal by a line and +//! leave the whole layout looking shifted — the queue appearing to bleed into +//! the now-playing pane. Because ratatui repaints only the cells that changed, +//! the damage persists until something forces a full redraw. Worse, the +//! message is then *lost*: it never reaches the log, so afterwards there is no +//! record of the underrun that caused it. +//! +//! So the fix is one `dup2`: point fd 2 at a file next to the log before the +//! alternate screen is entered. The diagnostics are kept, just not on screen. + +use std::path::{Path, PathBuf}; + +/// The directory both binaries log into: `crabidy/` under the state dir, +/// falling back to the cache dir and then to the temp dir. +/// +/// Shared so the log file and the captured stderr always land together. +pub fn log_dir() -> PathBuf { + dirs::state_dir() + .or_else(dirs::cache_dir) + .unwrap_or_else(std::env::temp_dir) + .join("crabidy") +} + +/// Points file descriptor 2 at `path` (appending), so writes that bypass +/// `tracing` are recorded instead of scribbling on the interface. +/// +/// Call once, before the terminal is put into raw mode. Errors are returned +/// rather than handled: a client that cannot open its log file should still +/// start — it just keeps the noisy stderr it has always had. +#[cfg(unix)] +pub fn capture_into(path: &Path) -> std::io::Result<()> { + use std::os::fd::AsRawFd; + + let file = std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(path)?; + // `dup2` makes fd 2 a second reference to this file, so dropping `file` + // (and its own descriptor) at the end of this function leaves stderr + // pointing at it. + if unsafe { libc::dup2(file.as_raw_fd(), libc::STDERR_FILENO) } == -1 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) +} + +/// Nothing to do off Unix: there is no `dup2`, and the crabidy clients are +/// terminal programs on Linux and macOS. +#[cfg(not(unix))] +pub fn capture_into(_path: &Path) -> std::io::Result<()> { + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The redirect must actually move the process's stderr, and appending + /// must not truncate what an earlier run wrote. + /// + /// The write goes to the descriptor directly rather than through + /// `eprintln!`: that is what a C library does (the case this exists for), + /// and the test harness intercepts the macro but not the descriptor. + #[cfg(unix)] + #[test] + fn captured_stderr_reaches_the_file_and_appends() { + use std::io::Write; + + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("cbd.stderr.log"); + std::fs::write(&path, "earlier run\n").expect("seed"); + + // Keep the real stderr so the rest of the suite still has one. Briefly + // process-wide, so a parallel test writing to fd 2 in this window would + // land in the file too — harmless here, and the alternative is a child + // process for one assertion. + let saved = unsafe { libc::dup(libc::STDERR_FILENO) }; + assert!(saved >= 0, "could not save stderr"); + capture_into(&path).expect("redirect"); + let wrote = std::io::stderr().write_all(b"underrun occurred\n"); + let flushed = std::io::stderr().flush(); + let restored = unsafe { libc::dup2(saved, libc::STDERR_FILENO) }; + assert!(restored >= 0, "could not restore stderr"); + unsafe { libc::close(saved) }; + wrote.expect("write to the redirected stderr"); + flushed.expect("flush"); + + let captured = std::fs::read_to_string(&path).expect("read back"); + assert!(captured.starts_with("earlier run\n"), "{captured:?}"); + assert!(captured.contains("underrun occurred"), "{captured:?}"); + } + + #[test] + fn the_log_dir_is_under_crabidy() { + assert_eq!( + log_dir().file_name().and_then(|n| n.to_str()), + Some("crabidy") + ); + } +} diff --git a/cbd-web/src/app.rs b/cbd-web/src/app.rs index 18aaf26..4f58d40 100644 --- a/cbd-web/src/app.rs +++ b/cbd-web/src/app.rs @@ -601,12 +601,13 @@ impl Store { })); } } - Action::QueueDedup => { + Action::QueueDedup | Action::QueueDedupTitles => { if self.queue.with_untracked(|q| !q.is_empty()) { + let by_title = action == Action::QueueDedupTitles; let this = *self; let Some(mut rpc) = self.rpc() else { return }; spawn_local(async move { - match rpc.dedup_queue().await { + match rpc.dedup_queue(by_title).await { // 0 is an answer, not a non-event // (architecture/queue-order.md D2). Ok(removed) => this.notify(format!("removed {removed} duplicate(s)")), diff --git a/cbd-web/src/keymap.rs b/cbd-web/src/keymap.rs index 306f7a2..fb4bf52 100644 --- a/cbd-web/src/keymap.rs +++ b/cbd-web/src/keymap.rs @@ -69,6 +69,9 @@ pub enum Action { /// Drop duplicate queue entries server-side; the count lands in a toast /// (architecture/queue-order.md D16). QueueDedup, + /// Drop entries with the same artist and title, keeping the longest take. + /// Aggressive and opt-in (architecture/queue-order.md D3a). + QueueDedupTitles, /// Open the sort menu dialog. QueueSortMenu, /// Close it without sorting (`Escape`/`q`/`S`). @@ -355,6 +358,11 @@ pub const HELP: &[HelpEntry] = &[ key: "u", description: "Unique: drop duplicate tracks from the queue", }, + HelpEntry { + scope: "Queue", + key: "U", + description: "Unique by title: one entry per song, keeping the longest", + }, HelpEntry { scope: "Queue", key: "S", @@ -460,6 +468,7 @@ pub fn lookup(focus: Focus, help_open: bool, key: &str, ctrl: bool) -> Option Some(Action::QueueClearAll), "w" => Some(Action::QueueSaveAs), "u" => Some(Action::QueueDedup), + "U" => Some(Action::QueueDedupTitles), "S" => Some(Action::QueueSortMenu), _ => None, }, @@ -574,7 +583,12 @@ mod tests { lookup(Focus::Queue, false, "S", false), Some(Action::QueueSortMenu) ); + assert_eq!( + lookup(Focus::Queue, false, "U", false), + Some(Action::QueueDedupTitles) + ); assert_eq!(lookup(Focus::Library, false, "u", false), None); + assert_eq!(lookup(Focus::Library, false, "U", false), None); assert_eq!(lookup(Focus::Library, false, "S", false), None); } diff --git a/cbd-web/src/rpc.rs b/cbd-web/src/rpc.rs index 5d1ca14..8e85506 100644 --- a/cbd-web/src/rpc.rs +++ b/cbd-web/src/rpc.rs @@ -232,10 +232,10 @@ impl Rpc { /// Drops duplicate queue entries; returns how many went /// (architecture/queue-order.md D2). - pub async fn dedup_queue(&mut self) -> Result { + pub async fn dedup_queue(&mut self, by_title: bool) -> Result { let response = self .client - .dedup_queue(Request::new(DedupQueueRequest {})) + .dedup_queue(Request::new(DedupQueueRequest { by_title })) .await?; Ok(response.into_inner().removed) } diff --git a/cbd/src/main.rs b/cbd/src/main.rs index 534fb52..fb9c5a2 100644 --- a/cbd/src/main.rs +++ b/cbd/src/main.rs @@ -53,6 +53,16 @@ async fn run_bundle(cli: CbdCli) -> Result<(), Box> { // Both halves share one file-based subscriber: the terminal belongs // to the TUI, so the server's usual stderr logging would corrupt it. let _log_guard = init_tracing(); + // Tracing is not the only writer, though. The audio stack runs *in this + // process* here, so ALSA's "underrun occurred" prints — and any panic + // message — would go straight onto the interface (cbd_tui::stderr). + let stderr_log = cbd_tui::stderr::log_dir().join("cbd.stderr.log"); + if let Err(err) = cbd_tui::stderr::capture_into(&stderr_log) { + eprintln!( + "could not redirect stderr to {}: {err}", + stderr_log.display() + ); + } // `cbd` reads its OWN config (`cbd.toml`), separate from the // standalone `cbd-tui`'s `cbd-tui.toml`. The two run side by side on // one machine — `cbd` self-contained against its in-process server, @@ -182,10 +192,7 @@ async fn wait_for_server( fn init_tracing() -> Option { use tracing_subscriber::{prelude::*, EnvFilter}; - let log_dir = dirs::state_dir() - .or_else(dirs::cache_dir) - .unwrap_or_else(std::env::temp_dir) - .join("crabidy"); + let log_dir = cbd_tui::stderr::log_dir(); if let Err(err) = std::fs::create_dir_all(&log_dir) { eprintln!( "could not create log directory {}: {err}", diff --git a/crabidy-core/crabidy/v1/crabidy.proto b/crabidy-core/crabidy/v1/crabidy.proto index 3b08bf4..e653a14 100644 --- a/crabidy-core/crabidy/v1/crabidy.proto +++ b/crabidy-core/crabidy/v1/crabidy.proto @@ -195,7 +195,16 @@ message ClearQueueResponse {} // under the same provider (the first path segment) or — when that id is // empty — the same `path`. Deliberately *not* metadata matching: the same // artist and title is routinely a different recording (D3). -message DedupQueueRequest {} +message DedupQueueRequest { + // Collapse entries with the same artist and title instead — the same song + // whatever the recording, so a remix, a radio edit and the album version + // reduce to one. Aggressive by construction: it discards versions the user + // may have queued deliberately, which is why it is opt-in and why the + // default above only merges what is provably the same item. The survivor is + // the playing entry, else the *longest* take (the full version rather than + // an edit), else the earliest. + bool by_title = 1; +} message DedupQueueResponse { // How many entries were dropped. 0 says the queue held no duplicates, // which is not the same answer as "nothing happened" — hence a response diff --git a/crabidy-server/src/lib.rs b/crabidy-server/src/lib.rs index b9424fa..be7a91f 100644 --- a/crabidy-server/src/lib.rs +++ b/crabidy-server/src/lib.rs @@ -184,31 +184,35 @@ fn spawn_spectrum_task( const FPS: u64 = 20; tokio::spawn(async move { let mut analyzer = spectrum::SpectrumAnalyzer::new(audio_player::SPECTRUM_WINDOW); - let mut last_count = tap.frame_count(); - let mut was_active = false; + let mut flow = spectrum::FlowDetector::new(tap.frame_count()); let mut interval = tokio::time::interval(std::time::Duration::from_millis(1000 / FPS)); + // Each tick costs a little (an FFT and a broadcast), and the default + // `Burst` behaviour keeps the *absolute* schedule: once that cost has + // accumulated to a whole period, two ticks fire back to back and the + // second one necessarily sees no new frames. That is what made the bars + // flick to the floor roughly once a second during playback. `Delay` + // measures each period from the previous tick instead, so a tick is + // never spent catching up. + interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); loop { interval.tick().await; // Nobody watching: do no work. if update_tx.receiver_count() == 0 { continue; } - let count = tap.frame_count(); - if count != last_count { - last_count = count; - if !was_active { - debug!("spectrum: audio flowing, streaming bars"); + match flow.observe(tap.frame_count()) { + spectrum::Tick::Bars => { + let bins = analyzer.analyze(&tap.snapshot()); + let _ = update_tx.send(Update::Spectrum(SpectrumFrame { bins })); } - was_active = true; - let bins = analyzer.analyze(&tap.snapshot()); - let _ = update_tx.send(Update::Spectrum(SpectrumFrame { bins })); - } else if was_active { - // Playback just went idle: drop the bars to the floor once. - debug!("spectrum: audio idle, bars to zero"); - was_active = false; - let _ = update_tx.send(Update::Spectrum(SpectrumFrame { - bins: vec![0.0; spectrum::SPECTRUM_BINS], - })); + spectrum::Tick::Silence => { + // Playback went idle: drop the bars to the floor once. + debug!("spectrum: audio idle, bars to zero"); + let _ = update_tx.send(Update::Spectrum(SpectrumFrame { + bins: vec![0.0; spectrum::SPECTRUM_BINS], + })); + } + spectrum::Tick::Nothing => {} } } }); @@ -668,12 +672,15 @@ impl QueueManager { /// Drops duplicate entries and returns how many were removed /// (architecture/queue-order.md D3–D5). /// - /// Two entries are the same track by [`queue_order::track_id`]. The - /// playing entry always survives its group, so this never changes what is - /// playing: the removal goes through [`Self::remove_tracks`], which - /// therefore reports no successor to start. - pub fn dedup(&mut self) -> usize { - let positions = queue_order::duplicate_positions(&self.tracks, self.current_position()); + /// Two entries are the same track by [`queue_order::track_id`] under + /// `identity` — the provably-same provider item by default, or the same + /// artist and title when the caller opts in (D3a). The playing entry always + /// survives its group, so this never changes what is playing: the removal + /// goes through [`Self::remove_tracks`], which therefore reports no + /// successor to start. + pub fn dedup(&mut self, identity: queue_order::Identity) -> usize { + let positions = + queue_order::duplicate_positions(&self.tracks, self.current_position(), identity); if positions.is_empty() { return 0; } @@ -1014,7 +1021,7 @@ mod tests { #[test] fn dedup_removes_later_copies_and_reports_the_count() { let mut q = queue_of(vec![track(0), track(1), track(0), track(2), track(1)]); - assert_eq!(q.dedup(), 2); + assert_eq!(q.dedup(queue_order::Identity::ProviderItem), 2); assert_eq!(titles(&q), vec!["track 0", "track 1", "track 2"]); assert_order_is_a_permutation(&q); } @@ -1026,7 +1033,7 @@ mod tests { let mut q = queue_of(vec![track(0), track(1), track(0)]); assert!(q.set_current_position(2)); let playing = q.current_track().expect("current"); - assert_eq!(q.dedup(), 1); + assert_eq!(q.dedup(queue_order::Identity::ProviderItem), 1); assert_eq!( q.current_track().expect("still a current track").path, playing.path @@ -1038,17 +1045,17 @@ mod tests { #[test] fn dedup_of_a_clean_queue_changes_nothing() { let mut q = queue_of(vec![track(0), track(1)]); - assert_eq!(q.dedup(), 0); + assert_eq!(q.dedup(queue_order::Identity::ProviderItem), 0); assert_eq!(titles(&q), vec!["track 0", "track 1"]); // And an empty queue is not a panic. let mut empty = QueueManager::new(); - assert_eq!(empty.dedup(), 0); + assert_eq!(empty.dedup(queue_order::Identity::ProviderItem), 0); } #[test] fn dedup_leaves_the_queue_playable() { let mut q = queue_of(vec![track(0), track(0), track(1)]); - q.dedup(); + q.dedup(queue_order::Identity::ProviderItem); assert_eq!(q.next_track().expect("advances").title, "track 1"); assert!(q.next_track().is_none()); } @@ -1291,6 +1298,9 @@ pub enum PlaybackCommand { /// queue mutation; the count travels back through `result_tx` because it /// cannot be recovered from the resulting snapshot. DedupQueue { + /// Collapse same artist+title instead of the provably-same item — the + /// aggressive, opt-in identity (architecture/queue-order.md D3a). + by_title: bool, result_tx: flume::Sender, }, /// Reorders the queue by one strategy (architecture/queue-order.md D6). diff --git a/crabidy-server/src/playback.rs b/crabidy-server/src/playback.rs index a80b733..5bbc9f3 100644 --- a/crabidy-server/src/playback.rs +++ b/crabidy-server/src/playback.rs @@ -265,7 +265,10 @@ impl Playback { } } - PlaybackCommand::DedupQueue { result_tx } => { + PlaybackCommand::DedupQueue { + by_title, + result_tx, + } => { // Never removes the playing entry (architecture/queue-order.md // D4), so unlike `Remove` there is no successor to start. let removed = { @@ -273,13 +276,18 @@ impl Playback { error!("queue lock poisoned"); return; }; - let removed = queue.dedup(); + let identity = if by_title { + crate::queue_order::Identity::ArtistTitle + } else { + crate::queue_order::Identity::ProviderItem + }; + let removed = queue.dedup(identity); if removed > 0 { self.broadcast_queue(&queue); } removed }; - debug!(removed, "de-duplicated the queue"); + debug!(removed, by_title, "de-duplicated the queue"); if let Err(err) = result_tx.send(removed) { // The caller gave up (client gone); the queue is already // deduped and broadcast, so this is only a lost count. @@ -1221,7 +1229,10 @@ mod tests { let (result_tx, result_rx) = flume::bounded(1); playback - .handle_command(PlaybackCommand::DedupQueue { result_tx }) + .handle_command(PlaybackCommand::DedupQueue { + by_title: false, + result_tx, + }) .await; assert_eq!(result_rx.recv_async().await.expect("a reply"), 1); @@ -1247,7 +1258,10 @@ mod tests { let (result_tx, result_rx) = flume::bounded(1); playback - .handle_command(PlaybackCommand::DedupQueue { result_tx }) + .handle_command(PlaybackCommand::DedupQueue { + by_title: false, + result_tx, + }) .await; assert_eq!(result_rx.recv_async().await.expect("a reply"), 0); @@ -1272,7 +1286,10 @@ mod tests { let (result_tx, result_rx) = flume::bounded(1); drop(result_rx); playback - .handle_command(PlaybackCommand::DedupQueue { result_tx }) + .handle_command(PlaybackCommand::DedupQueue { + by_title: false, + result_tx, + }) .await; assert_eq!(queue_titles(&playback), vec!["track 0"]); } @@ -1324,7 +1341,10 @@ mod tests { let (result_tx, _result_rx) = flume::bounded(1); playback - .handle_command(PlaybackCommand::DedupQueue { result_tx }) + .handle_command(PlaybackCommand::DedupQueue { + by_title: false, + result_tx, + }) .await; assert_eq!( rx.borrow().clone().expect("snapshot sent").tracks.len(), diff --git a/crabidy-server/src/queue_order.rs b/crabidy-server/src/queue_order.rs index 5870f1a..fbddec8 100644 --- a/crabidy-server/src/queue_order.rs +++ b/crabidy-server/src/queue_order.rs @@ -21,23 +21,44 @@ use crabidy_core::proto::crabidy::{QueueSort, Track}; /// /// Cheap to build (two borrows, no allocation) so a dedup pass over a long /// queue stays a single hash-set walk. -#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +#[derive(Clone, Debug, PartialEq, Eq, Hash)] pub enum TrackId<'a> { /// `(provider, provider_item_id)` — the same item however it was reached /// (through an album, a playlist, a search result). ProviderItem { provider: &'a str, id: &'a str }, /// The full library path, for tracks whose provider reports no id. Path(&'a str), + /// Lowercased artist and title: the same *song*, whatever the recording + /// ([`Identity::ArtistTitle`]). + ArtistTitle(String, String), } -/// The de-duplication identity of `track` (D3). -pub fn track_id(track: &Track) -> TrackId<'_> { - if track.provider_item_id.is_empty() { - return TrackId::Path(&track.path); - } - TrackId::ProviderItem { - provider: provider_of(&track.path), - id: &track.provider_item_id, +/// What makes two queue entries "the same track" for de-duplication. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum Identity { + /// The provably-same item: the provider's own id under the same provider, + /// or the same path (D3). The default, because it cannot discard a version + /// the user chose. + #[default] + ProviderItem, + /// The same artist and title, compared case-insensitively — a remix, a + /// radio edit and the album version collapse to one entry. Opt-in: it + /// throws away recordings that differ (D3a). + ArtistTitle, +} + +/// The de-duplication identity of `track` under `identity` (D3, D3a). +pub fn track_id(track: &Track, identity: Identity) -> TrackId<'_> { + match identity { + Identity::ArtistTitle => TrackId::ArtistTitle( + track.artist.trim().to_lowercase(), + track.title.trim().to_lowercase(), + ), + Identity::ProviderItem if track.provider_item_id.is_empty() => TrackId::Path(&track.path), + Identity::ProviderItem => TrackId::ProviderItem { + provider: provider_of(&track.path), + id: &track.provider_item_id, + }, } } @@ -59,28 +80,39 @@ fn provider_of(path: &str) -> &str { /// never in the returned list and a dedup cannot interrupt playback. /// `current` being out of range (an empty queue) simply means "no protected /// entry". -pub fn duplicate_positions(tracks: &[Track], current: usize) -> Vec { - // The survivor of each group, decided before anything is dropped: the - // playing entry when it is in the group, otherwise the first one seen. - // `current` is looked up once, so a group whose survivor is the playing - // entry can still drop entries that come *before* it (D4). - let playing = tracks.get(current).map(track_id); - let mut survivors: HashMap, usize> = HashMap::new(); - for (pos, track) in tracks.iter().enumerate() { - let id = track_id(track); - let survivor = survivors.entry(id).or_insert(pos); - if Some(id) == playing { +pub fn duplicate_positions(tracks: &[Track], current: usize, identity: Identity) -> Vec { + let keys: Vec> = tracks.iter().map(|t| track_id(t, identity)).collect(); + // The survivor of each group, decided before anything is dropped, so a + // group whose survivor is the playing entry can still drop entries that + // come *before* it (D4). + let mut survivors: HashMap<&TrackId<'_>, usize> = HashMap::new(); + for (pos, key) in keys.iter().enumerate() { + let survivor = survivors.entry(key).or_insert(pos); + if keys.get(current) == Some(key) { + // The playing entry always wins its group: a dedup that stops the + // music is a bug, not a policy. *survivor = current; + } else if identity == Identity::ArtistTitle && *survivor != current { + // Same song, different recordings: keep the longest take — the + // full version rather than a radio edit (D3a). Unknown lengths + // lose, and equal lengths keep the earlier entry. + let better = duration_of(tracks, pos) > duration_of(tracks, *survivor); + if better { + *survivor = pos; + } } } - tracks - .iter() - .enumerate() - .filter(|(pos, track)| survivors.get(&track_id(track)) != Some(pos)) - .map(|(pos, _)| pos) + (0..tracks.len()) + .filter(|pos| survivors.get(&keys[*pos]) != Some(pos)) .collect() } +/// The known duration of a queue entry in seconds; `0` when unknown, which +/// makes an unknown length lose the "longest take" comparison above. +fn duration_of(tracks: &[Track], pos: usize) -> u32 { + tracks.get(pos).and_then(|t| t.duration).unwrap_or(0) +} + /// The permutation that `sort` puts `tracks` in: `perm[new] = old`, i.e. the /// new queue is `perm.iter().map(|&old| tracks[old])`. /// @@ -216,7 +248,10 @@ mod tests { // The same Tidal track through an album and through a playlist. let via_album = track("/tidal/albums/7/99", "99"); let via_playlist = track("/tidal/playlists/p/99", "99"); - assert_eq!(track_id(&via_album), track_id(&via_playlist)); + assert_eq!( + track_id(&via_album, Identity::ProviderItem), + track_id(&via_playlist, Identity::ProviderItem) + ); } #[test] @@ -225,7 +260,10 @@ mod tests { // unrelated track (D3). let tidal = track("/tidal/albums/7/1234", "1234"); let jamendo = track("/jamendo/tracks/1234", "1234"); - assert_ne!(track_id(&tidal), track_id(&jamendo)); + assert_ne!( + track_id(&tidal, Identity::ProviderItem), + track_id(&jamendo, Identity::ProviderItem) + ); } #[test] @@ -234,7 +272,10 @@ mod tests { // provider, so it stays a separate entry (D3, stated risk). let source = track("/tidal/albums/7/99", "99"); let capture = track("/crabidy/mix/song.cbd-track.toml", "99"); - assert_ne!(track_id(&source), track_id(&capture)); + assert_ne!( + track_id(&source, Identity::ProviderItem), + track_id(&capture, Identity::ProviderItem) + ); } #[test] @@ -242,9 +283,18 @@ mod tests { let a = track("/fs/music/song.flac", ""); let same = track("/fs/music/song.flac", ""); let other = track("/fs/music/other.flac", ""); - assert_eq!(track_id(&a), track_id(&same)); - assert_ne!(track_id(&a), track_id(&other)); - assert!(matches!(track_id(&a), TrackId::Path(_))); + assert_eq!( + track_id(&a, Identity::ProviderItem), + track_id(&same, Identity::ProviderItem) + ); + assert_ne!( + track_id(&a, Identity::ProviderItem), + track_id(&other, Identity::ProviderItem) + ); + assert!(matches!( + track_id(&a, Identity::ProviderItem), + TrackId::Path(_) + )); } // ---- which copies go (D4) ----------------------------------------- @@ -257,7 +307,10 @@ mod tests { track("/tidal/b/1", "1"), track("/tidal/a/2", "2"), ]; - assert_eq!(duplicate_positions(&tracks, 0), vec![2, 3]); + assert_eq!( + duplicate_positions(&tracks, 0, Identity::ProviderItem), + vec![2, 3] + ); } #[test] @@ -269,25 +322,31 @@ mod tests { track("/tidal/a/2", "2"), track("/tidal/b/1", "1"), ]; - assert_eq!(duplicate_positions(&tracks, 2), vec![0]); + assert_eq!( + duplicate_positions(&tracks, 2, Identity::ProviderItem), + vec![0] + ); // With another entry playing, the ordinary rule applies again. - assert_eq!(duplicate_positions(&tracks, 1), vec![2]); + assert_eq!( + duplicate_positions(&tracks, 1, Identity::ProviderItem), + vec![2] + ); } #[test] fn nothing_to_do_is_an_empty_list() { let tracks = [track("/tidal/a/1", "1"), track("/tidal/a/2", "2")]; - assert!(duplicate_positions(&tracks, 0).is_empty()); - assert!(duplicate_positions(&[], 0).is_empty()); + assert!(duplicate_positions(&tracks, 0, Identity::ProviderItem).is_empty()); + assert!(duplicate_positions(&[], 0, Identity::ProviderItem).is_empty()); // An out-of-range current (an empty or freshly cleared queue) is not // a panic and protects nothing. - assert!(duplicate_positions(&[], 9).is_empty()); + assert!(duplicate_positions(&[], 9, Identity::ProviderItem).is_empty()); } #[test] fn positions_come_back_ascending_and_are_unique() { let tracks: Vec = (0..6).map(|_| track("/tidal/a/1", "1")).collect(); - let positions = duplicate_positions(&tracks, 3); + let positions = duplicate_positions(&tracks, 3, Identity::ProviderItem); let mut sorted = positions.clone(); sorted.sort_unstable(); sorted.dedup(); @@ -297,6 +356,74 @@ mod tests { assert!(!positions.contains(&3)); } + // ---- the opt-in title identity (D3a) ------------------------------- + + /// The case that motivated the mode, from a real queue: two remixes and the + /// album version of one song, three distinct provider items with three + /// different lengths. The default identity keeps all three; the title + /// identity keeps the longest. + #[test] + fn the_title_identity_collapses_different_recordings() { + // Faithful to the queue this came from: distinct ISRC-style ids under + // one provider, three lengths, two of them from the remixes album. + let with_id = |id: &str, title: &str, secs: u32| Track { + provider_item_id: id.to_string(), + path: format!("/crabidy/Mindchatter/{id}.cbd-track.toml"), + ..meta("Mindchatter", None, title, Some(secs)) + }; + let tracks = vec![ + with_id("QZES72134712", "Sink Into The Hips", 189), + with_id("QZES72136149", "Sink Into The Hips", 171), + with_id("QZMEM2002285", "Sink Into the Hips", 228), + ]; + assert!( + duplicate_positions(&tracks, 0, Identity::ProviderItem).is_empty(), + "three distinct items are not duplicates by default" + ); + // Nothing playing (an out-of-range current), so the length rule decides: + // case in the title is not a difference, and the 228s take survives. + assert_eq!( + duplicate_positions(&tracks, 9, Identity::ArtistTitle), + vec![0, 1] + ); + } + + #[test] + fn the_title_identity_keeps_the_playing_entry_over_the_longest() { + let tracks = vec![ + meta("artist", None, "song", Some(300)), + meta("artist", None, "song", Some(120)), + ]; + // Position 1 is playing: it survives even though it is the shorter + // take — playback outranks the length rule (D4). + assert_eq!( + duplicate_positions(&tracks, 1, Identity::ArtistTitle), + vec![0] + ); + } + + #[test] + fn an_unknown_length_loses_to_a_known_one() { + let tracks = vec![ + meta("artist", None, "song", None), + meta("artist", None, "song", Some(120)), + ]; + assert_eq!( + duplicate_positions(&tracks, 5, Identity::ArtistTitle), + vec![0], + "the entry with a known length survives" + ); + } + + #[test] + fn the_title_identity_does_not_merge_different_artists() { + let tracks = vec![ + meta("Aretha Franklin", None, "Respect", Some(147)), + meta("Otis Redding", None, "Respect", Some(128)), + ]; + assert!(duplicate_positions(&tracks, 0, Identity::ArtistTitle).is_empty()); + } + // ---- sorting (D6–D9) ---------------------------------------------- #[test] diff --git a/crabidy-server/src/rpc.rs b/crabidy-server/src/rpc.rs index d182061..1415385 100644 --- a/crabidy-server/src/rpc.rs +++ b/crabidy-server/src/rpc.rs @@ -356,17 +356,22 @@ impl CrabidyService for RpcService { Ok(Response::new(ClearQueueResponse {})) } - #[instrument(skip(self, _request))] + #[instrument(skip(self, request), fields(by_title))] async fn dedup_queue( &self, - _request: Request, + request: Request, ) -> Result, Status> { + let by_title = request.into_inner().by_title; + tracing::Span::current().record("by_title", by_title); debug!("received dedup_queue request"); // The count comes back from the loop (architecture/queue-order.md D2); // the dedup itself has already been broadcast by then. let (result_tx, result_rx) = flume::bounded(1); - self.send_playback(PlaybackCommand::DedupQueue { result_tx }) - .await?; + self.send_playback(PlaybackCommand::DedupQueue { + by_title, + result_tx, + }) + .await?; let removed = result_rx.recv_async().await.map_err(|err| { error!("no reply from playback loop: {err}"); Status::internal("playback loop did not reply") @@ -759,14 +764,14 @@ mod tests { .expect("command sent") .command { - PlaybackCommand::DedupQueue { result_tx } => { + PlaybackCommand::DedupQueue { result_tx, .. } => { result_tx.send(7).expect("reply accepted"); } other => panic!("expected a dedup command, got {}", other.name()), } }); let response = service - .dedup_queue(Request::new(DedupQueueRequest {})) + .dedup_queue(Request::new(DedupQueueRequest { by_title: false })) .await .expect("accepted"); assert_eq!(response.into_inner().removed, 7); diff --git a/crabidy-server/src/spectrum.rs b/crabidy-server/src/spectrum.rs index 36f5bbf..41eb888 100644 --- a/crabidy-server/src/spectrum.rs +++ b/crabidy-server/src/spectrum.rs @@ -102,12 +102,112 @@ fn log_bin_edges(mag_len: usize, bins: usize) -> Vec { .collect() } +/// How many consecutive ticks may see no new samples before the bars are +/// declared idle and dropped to zero. +/// +/// One is too few. The tap counts *frames*, so during playback every tick +/// normally sees thousands more — but a tick that arrives early (or two that +/// arrive together) sees none, and calling that silence made the bars flick to +/// the floor about once a second and wrote two log lines each time. Two ticks +/// is 100 ms at 20 fps: still immediate to the eye when playback really stops. +const IDLE_TICKS_BEFORE_ZERO: u8 = 2; + +/// Decides, tick by tick, whether audio is flowing — the only state the +/// spectrum task keeps. +/// +/// Split out from the task so the flapping that motivated it is testable +/// without a runtime or an audio device (architecture/spectrum.md D2). +pub struct FlowDetector { + last_count: u64, + idle_ticks: u8, + active: bool, +} + +/// What the spectrum task should send after one tick. +#[derive(Debug, PartialEq, Eq)] +pub enum Tick { + /// Audio is flowing: analyze the window and broadcast bars. + Bars, + /// Playback just went idle: broadcast a single zeroed frame so the bars + /// fall instead of freezing. + Silence, + /// Nothing to say. + Nothing, +} + +impl FlowDetector { + pub fn new(count: u64) -> Self { + Self { + last_count: count, + idle_ticks: 0, + active: false, + } + } + + /// Folds this tick's frame count into the decision. + pub fn observe(&mut self, count: u64) -> Tick { + if count != self.last_count { + self.last_count = count; + self.idle_ticks = 0; + self.active = true; + return Tick::Bars; + } + // No new frames. Only after enough of them in a row is this silence + // rather than a tick that merely landed between two callbacks. + self.idle_ticks = self.idle_ticks.saturating_add(1); + if self.active && self.idle_ticks >= IDLE_TICKS_BEFORE_ZERO { + self.active = false; + return Tick::Silence; + } + Tick::Nothing + } +} + #[cfg(test)] mod tests { use super::*; const WINDOW: usize = 2048; + /// The flapping this exists to stop: a single tick with no new frames is + /// not silence, so it must not zero the bars (which is what the log showed + /// happening ~1/s during playback). + #[test] + fn one_empty_tick_does_not_zero_the_bars() { + let mut flow = FlowDetector::new(0); + assert_eq!(flow.observe(1000), Tick::Bars); + assert_eq!( + flow.observe(1000), + Tick::Nothing, + "one empty tick is not silence" + ); + assert_eq!(flow.observe(2000), Tick::Bars); + } + + #[test] + fn sustained_silence_zeroes_the_bars_exactly_once() { + let mut flow = FlowDetector::new(0); + assert_eq!(flow.observe(1000), Tick::Bars); + assert_eq!(flow.observe(1000), Tick::Nothing); + assert_eq!(flow.observe(1000), Tick::Silence); + // And then stays quiet rather than re-sending zero frames forever. + for _ in 0..5 { + assert_eq!(flow.observe(1000), Tick::Nothing); + } + // Resuming reports flowing again. + assert_eq!(flow.observe(1001), Tick::Bars); + } + + /// A player that never started must not emit a zero frame just because + /// nothing is happening. + #[test] + fn an_idle_player_says_nothing() { + let mut flow = FlowDetector::new(0); + for _ in 0..10 { + assert_eq!(flow.observe(0), Tick::Nothing); + } + } + fn sine(freq: f32, sample_rate: f32, len: usize) -> Vec { (0..len) .map(|i| (2.0 * std::f32::consts::PI * freq * i as f32 / sample_rate).sin()) diff --git a/docs/src/clients/cli.md b/docs/src/clients/cli.md index b335807..03e53e2 100644 --- a/docs/src/clients/cli.md +++ b/docs/src/clients/cli.md @@ -48,11 +48,13 @@ cbd global volume -- -0.1 # lower the volume [--keep-current]`, `queue set-current `, `queue save`/`capture `, `queue shuffle`, and `queue repeat` change it. - `queue dedup` drops duplicate entries and prints how many went (`0` when - there were none); `queue sort - [--desc]` reorders it. Both are described under [Order operations: - dedup and sort](../queue.md#order-operations-dedup-and-sort) — the - strategy is a fixed choice, so a shell completes it and a typo fails - before anything reaches the server. + there were none); `--titles` switches to the blunter same-song identity + (keeping the longest take, discarding remixes and edits); + `queue sort [--desc]` reorders it. + Both are described under [Order operations: dedup and + sort](../queue.md#order-operations-dedup-and-sort) — the strategy is a + fixed choice, so a shell completes it and a typo fails before anything + reaches the server. - `global play`/`stop`/`next`/`prev`/`restart`/`mute`, `global volume `, and `global seek ` (negative seeks back, e.g. `global seek -15`) control playback. diff --git a/docs/src/clients/tui.md b/docs/src/clients/tui.md index 7be7809..83ba7f7 100644 --- a/docs/src/clients/tui.md +++ b/docs/src/clients/tui.md @@ -308,6 +308,7 @@ pane). | Queue | `c` | Clear queue except current (to register) | | Queue | `C` | Clear entire queue (to register) | | Queue | `u` | Unique: drop duplicate tracks | +| Queue | `U` | Unique by title: one entry per song | | Queue | `S` | Sort the queue (opens a strategy menu) | | Queue | `w` | Save queue under a name | | Queue | `W` | Capture the queue into /crabidy (audio) | diff --git a/docs/src/queue.md b/docs/src/queue.md index 66118dd..d99bfbf 100644 --- a/docs/src/queue.md +++ b/docs/src/queue.md @@ -182,6 +182,15 @@ your queue. Two consequences: The RPC answers with the number of entries removed; `0` is a real answer, and the clients show it ("no duplicates") rather than nothing. +```admonish warning title="Dedup by title" +The same verb with a blunter identity: **same artist and title**, whatever the +recording, keeping the longest take (still yielding to the playing entry). One +entry per *song* rather than per recording — which means it discards remixes, +live takes and radio edits you may have queued on purpose. It is on its own key +for that reason (`U` in either client, `queue dedup --titles` on the command +line); `u` never does this. +``` + **Sort** (`S` opens a menu in either client, `cbd queue sort [--desc]`) reorders by one strategy: diff --git a/quality/queue-order.md b/quality/queue-order.md index 280d80a..8a62192 100644 --- a/quality/queue-order.md +++ b/quality/queue-order.md @@ -11,10 +11,15 @@ are the things a reader has to check by reading the code. Each is pass/fail. the group, otherwise the earliest one (D4). Check the survivor selection directly, not only through a test: a "keep the first" shortcut is a silent playback stop the moment the playing copy is a later one. -- [ ] **G2 — Duplicate identity is provider-scoped.** The key is +- [ ] **G2 — The default duplicate identity is provider-scoped.** The key is `(first path segment, provider_item_id)` when the id is non-empty and the - whole path otherwise (D3). No comparison of artist/title anywhere in the - dedup path. + whole path otherwise (D3). Artist/title matching exists **only** under the + opt-in `by_title` identity (D3a), never on the default path, and the flag is + never defaulted to true anywhere between key and wire. +- [ ] **G2a — `by_title` keeps the longest take** and still yields to the + playing entry (D3a), and every client surface that reaches it is distinct + from the safe one (its own key, its own flag) — a user cannot get it by + mistyping. - [ ] **G3 — Sort is stable and total.** `sort_permutation` returns a permutation of `0..len` for every strategy and direction, equal keys keep their queue order, and blanks/unknown durations sort last in *both*