Skip to content

Commit 18b6979

Browse files
thewrzclaude
andauthored
fix(audio): async decode playback edge cases (#186)
* fix async decode playback edges * fix(audio): restore snappy optimistic highlight on cold press #152 deferred the cold-press highlight until decode succeeded, so the first press of every sound in a session showed no feedback until decode completed — against the snappy-UI doctrine (#111: keep the optimistic instant highlight, never slow the click->play path). Restore it: a cold cache miss claims `playing` and the pending now-playing state immediately, then the async decode confirms the playhead on success. A decode failure, or a sound removed mid-decode, now releases the optimistic highlight through a shared owns-the-UI guard, so it is never left stuck on a sound that produced no audio. The generation/terminal-event invariant is preserved: a superseded older press never retakes a newer press's highlight. Reviving the optimistic path also un-orphans now_playing::pending(), which #152 had left as dead code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(audio): stop orphaned voice on library reconcile; make command boundary testable Two review follow-ups for #152, plus the test infrastructure they needed. - reconcile_playback_with_library now sends StopVoice for the highlighted sound when its library entry vanishes mid-play, instead of only clearing the UI and leaving the engine to honk the removed sound to completion ("ghost honk"). The highlighted voice id is the current play_generation, since only the owning press holds the highlight. - Add a test-only command tap to AudioHandle (the pipewire command channel can't be drained synchronously, so no test could previously assert what reached the engine). Pin the core invariant the PR is about: a cold press fires exactly one Play when its decode lands, and a decode landing after StopAll is stale and fires none. - Extract AudioHandle + test_handle out of the over-budget engine.rs (927 lines) into a focused handle.rs (78), so the tap lands in a compliant file and engine.rs shrinks rather than grows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style: apply cargo fmt to audio module exports Fixes the CI lint (cargo fmt --check) failure on this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(app): stop stale same-id decodes consuming the new pending decode A decode cancelled by StopAll/interrupt shares its sound id with a re-press's fresh pending entry. Landing first, it consumed that entry: a stale error cleared the new press's optimistic highlight and the real decode result was then dropped on arrival, so the sound never played. Pending entries are now consumed only by the task that owns them (matching voice id); stale landings fall through and are dropped. Codex review finding [P2] on PR #186. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ui): keep the active sound's waveform envelope during PCM eviction A late decode evicting the active sound's PCM under cache pressure also removed its waveform envelope, blanking the visible waveform mid-play even though the engine keeps playing from its own Arc. The active sound's envelope is now exempt from eviction. Codex review finding [P3] / CodeRabbit nitpick on PR #186. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(deny): ignore quick-xml RUSTSEC-2026-0194/0195 pending upstream bump Both advisories are DoS-via-untrusted-XML in quick-xml < 0.41. The sole consumer is wayland-scanner, a build-time proc-macro parsing vendored Wayland protocol XML; no untrusted XML is parsed at runtime, so neither issue is reachable in the shipped binary. wayland-scanner 0.31.10 (latest) still pins quick-xml ^0.39 — drop these ignores when it moves to 0.41. Closes #192. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(deps): bump anyhow to 1.0.103 (RUSTSEC-2026-0190) anyhow 1.0.102 is flagged unsound (Error::downcast_mut UB after Error::context); 1.0.103 is the patched release already on main. Regenerates the Flatpak cargo-sources.json for the lock change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(tests): split playback tests under the 400-line file cap tests.rs hit 465 lines after the decode-ownership regressions landed. Fixtures move to test_support.rs; warm-path, eviction, and library- reconciliation tests move to cache_tests.rs; decode-ownership lifecycle tests stay in tests.rs. No test behavior changes. Addresses CodeRabbit review on PR #186. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(tests): capture generation values instead of hardcoding literals Addresses CodeRabbit nitpick on PR #186; keeps the concurrent-decode tests resilient to generation-counter changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: retrigger checks (pull_request events for previous pushes were dropped) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent b4f71b7 commit 18b6979

11 files changed

Lines changed: 747 additions & 299 deletions

File tree

Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/app/mod.rs

Lines changed: 16 additions & 151 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use std::collections::HashSet;
1+
use std::collections::{HashMap, HashSet};
22
use std::sync::mpsc::Receiver;
33
use std::sync::{Arc, Mutex};
44
use std::time::Instant;
@@ -247,13 +247,13 @@ pub struct HonkHonk {
247247
/// Monotonic counter bumped on every play dispatch. Stamped onto the `Play`
248248
/// command and echoed back on `PlaybackFinished` to tell a genuine end from
249249
/// the stale `Finished` of a re-pressed voice (#149), and onto each
250-
/// off-thread decode so a `Message::Decoded` from a superseded press is
251-
/// dropped on arrival (#151). A decode for a play that was *cancelled*
252-
/// (StopAll) is caught by `handle_decoded`'s `playing` check instead.
250+
/// off-thread decode so the latest same-id cold repeat can claim ownership
251+
/// when the shared decode lands (#151/#152).
253252
play_generation: u64,
254253
/// Hot-path decoded-PCM cache (#151).
255254
audio_store: crate::audio::AudioStore,
256255
pending_play_ids: HashSet<u64>,
256+
pending_decodes: HashMap<String, playback::PendingDecode>,
257257
/// Persisted macro collection (#165).
258258
macros: crate::state::MacroStore,
259259
/// Active live macro capture, if recording is enabled (#167).
@@ -446,6 +446,7 @@ impl HonkHonk {
446446
play_generation: 0,
447447
audio_store: crate::audio::AudioStore::new(crate::audio::DEFAULT_PCM_CAP_BYTES),
448448
pending_play_ids: HashSet::new(),
449+
pending_decodes: HashMap::new(),
449450
macros: crate::state::MacroStore::load(),
450451
recording: None,
451452
macro_editor_draft: None,
@@ -507,6 +508,7 @@ impl HonkHonk {
507508
play_generation: 0,
508509
audio_store: crate::audio::AudioStore::new(crate::audio::DEFAULT_PCM_CAP_BYTES),
509510
pending_play_ids: HashSet::new(),
511+
pending_decodes: HashMap::new(),
510512
macros: crate::state::MacroStore::default(),
511513
recording: None,
512514
macro_editor_draft: None,
@@ -699,16 +701,14 @@ impl HonkHonk {
699701
sound_id,
700702
generation,
701703
} => {
702-
// Every play path sets `playing` optimistically at
703-
// dispatch, so a Started for a *different* sound can
704-
// only be a stale event from an older press still in
705-
// the queue — don't let it steal the highlight (#111).
706-
// When the UI is idle, only the current generation may
707-
// claim it: a late superseded concurrent voice (older
708-
// generation) finishing its decode after a newer short
709-
// sound already ended would otherwise re-highlight its
710-
// tile, and its stale Finished is ignored, leaving it
711-
// stuck (#149/#164).
704+
// Warm plays and successful cold decodes claim
705+
// `playing` before the engine's Started event. When the
706+
// UI is idle, only the current generation may claim it:
707+
// a late superseded concurrent voice (older generation)
708+
// finishing after a newer short sound already ended
709+
// would otherwise re-highlight its tile and then leave
710+
// it stuck when the stale Finished is ignored
711+
// (#149/#152/#164).
712712
let confirms_current = self.playing.as_deref() == Some(sound_id.as_str());
713713
let claims_idle =
714714
self.playing.is_none() && generation == self.play_generation;
@@ -812,6 +812,7 @@ impl HonkHonk {
812812
// for the stopped sound is dropped on arrival rather than
813813
// resurrecting it (#151).
814814
self.pending_play_ids.clear();
815+
self.pending_decodes.clear();
815816
self.clear_playback_state();
816817
self.cancel_macro();
817818
Task::none()
@@ -1006,6 +1007,7 @@ impl HonkHonk {
10061007
.map(|s| (s.id.clone(), s.path.clone()))
10071008
.collect();
10081009
self.sounds = new_sounds;
1010+
self.reconcile_playback_with_library();
10091011
self.duration_scan_pairs = std::sync::Arc::new(pairs);
10101012
self.durations_loaded = false;
10111013
Task::none()
@@ -2012,31 +2014,6 @@ mod tests {
20122014
assert_eq!(app.playing(), Some("newer"));
20132015
}
20142016

2015-
#[test]
2016-
fn play_sound_sets_playing_immediately() {
2017-
let mut app = HonkHonk::new_for_test();
2018-
let (handle, _evt_tx) = crate::audio::test_handle();
2019-
app.audio = Some(handle);
2020-
2021-
let dir = tempfile::tempdir().expect("tempdir");
2022-
let wav_path = dir.path().join("honk.wav");
2023-
write_test_wav(&wav_path);
2024-
app.sounds = vec![SoundEntry {
2025-
id: "wav1".into(),
2026-
name: "Honk".into(),
2027-
path: wav_path,
2028-
format: crate::state::AudioFormat::Wav,
2029-
duration_ms: Some(100),
2030-
category: "Test".into(),
2031-
}];
2032-
2033-
// The tile highlight must track the press itself, not wait for the
2034-
// engine's PlaybackStarted to round-trip through the event queue
2035-
// (issue #111).
2036-
let _ = app.update(Message::PlaySound("wav1".into()));
2037-
assert_eq!(app.playing(), Some("wav1"));
2038-
}
2039-
20402017
#[test]
20412018
fn drain_audio_events_processes_entire_backlog() {
20422019
let mut app = HonkHonk::new_for_test();
@@ -2082,29 +2059,6 @@ mod tests {
20822059
assert!((app.progress() - 0.5).abs() < f32::EPSILON);
20832060
}
20842061

2085-
#[test]
2086-
fn shortcut_activation_sets_playing_immediately() {
2087-
let mut app = HonkHonk::new_for_test();
2088-
let (handle, _evt_tx) = crate::audio::test_handle();
2089-
app.audio = Some(handle);
2090-
2091-
let dir = tempfile::tempdir().expect("tempdir");
2092-
let wav_path = dir.path().join("honk.wav");
2093-
write_test_wav(&wav_path);
2094-
app.sounds = vec![SoundEntry {
2095-
id: "wav1".into(),
2096-
name: "Honk".into(),
2097-
path: wav_path.clone(),
2098-
format: crate::state::AudioFormat::Wav,
2099-
duration_ms: Some(100),
2100-
category: "Test".into(),
2101-
}];
2102-
app.slots.set(0, wav_path);
2103-
2104-
let _ = app.update(Message::ShortcutActivated(0));
2105-
assert_eq!(app.playing(), Some("wav1"));
2106-
}
2107-
21082062
#[test]
21092063
fn play_sound_no_op_for_unknown_id() {
21102064
let mut app = HonkHonk::new_for_test();
@@ -3325,93 +3279,4 @@ mod tests {
33253279
);
33263280
assert_eq!(app.playing(), Some("newer"));
33273281
}
3328-
3329-
#[test]
3330-
fn current_decoded_starts_playhead_and_caches_pcm() {
3331-
let mut app = HonkHonk::new_for_test();
3332-
let (handle, _evt_tx) = crate::audio::test_handle();
3333-
app.audio = Some(handle);
3334-
app.play_generation = 2;
3335-
app.playing = Some("snd".into());
3336-
app.pending_play_ids.insert(2);
3337-
3338-
let pcm = crate::audio::CachedPcm {
3339-
samples: std::sync::Arc::new(vec![0.25_f32; 64]),
3340-
sample_rate: 48_000,
3341-
channels: 2,
3342-
duration: std::time::Duration::from_secs(3),
3343-
};
3344-
let _ = app.update(Message::Decoded {
3345-
generation: 2,
3346-
voice_id: 2,
3347-
id: "snd".into(),
3348-
result: Ok(pcm),
3349-
gain: 1.0,
3350-
effects: crate::audio::effects::EffectSettings::default(),
3351-
mode: PlayMode::Concurrent,
3352-
});
3353-
3354-
assert!(
3355-
app.now_playing.has_playhead(),
3356-
"current decode must start the playhead"
3357-
);
3358-
assert!(
3359-
app.audio_store.get_pcm("snd").is_some(),
3360-
"decode result must be cached for instant re-fire"
3361-
);
3362-
}
3363-
3364-
#[test]
3365-
fn stopall_mid_decode_does_not_resurrect_playback() {
3366-
// A cold-cache press dispatches an off-thread decode but sends no engine
3367-
// Play yet. If the user hits StopAll before the decode lands, the stale
3368-
// `Decoded` must be dropped — not resurrect the stopped sound (#151).
3369-
let mut app = HonkHonk::new_for_test();
3370-
let (handle, _evt_tx) = crate::audio::test_handle();
3371-
app.audio = Some(handle);
3372-
3373-
let dir = tempfile::tempdir().expect("tempdir");
3374-
let wav_path = dir.path().join("honk.wav");
3375-
write_test_wav(&wav_path);
3376-
app.sounds = vec![SoundEntry {
3377-
id: "wav1".into(),
3378-
name: "Honk".into(),
3379-
path: wav_path,
3380-
format: crate::state::AudioFormat::Wav,
3381-
duration_ms: Some(100),
3382-
category: "Test".into(),
3383-
}];
3384-
3385-
// Cold press → generation bumped, decode Task in flight (ignored here).
3386-
let sound = app.sounds[0].clone();
3387-
let _ = app.request_play(&sound, false);
3388-
let in_flight_gen = app.play_generation;
3389-
assert_eq!(app.playing(), Some("wav1"));
3390-
3391-
// StopAll tears down playback and must invalidate the in-flight decode.
3392-
let _ = app.update(Message::StopAll);
3393-
assert_eq!(app.playing(), None);
3394-
3395-
// The decode lands carrying the now-stale generation.
3396-
let _ = app.update(Message::Decoded {
3397-
generation: in_flight_gen,
3398-
voice_id: in_flight_gen,
3399-
id: "wav1".into(),
3400-
result: Ok(crate::audio::CachedPcm {
3401-
samples: std::sync::Arc::new(vec![0.0_f32; 8]),
3402-
sample_rate: 48_000,
3403-
channels: 2,
3404-
duration: std::time::Duration::from_secs(1),
3405-
}),
3406-
gain: 1.0,
3407-
effects: crate::audio::effects::EffectSettings::default(),
3408-
mode: PlayMode::Concurrent,
3409-
});
3410-
3411-
assert_eq!(app.playing(), None, "StopAll must win — no resurrection");
3412-
assert!(
3413-
!app.now_playing.has_playhead(),
3414-
"no playhead after a stopped, stale decode"
3415-
);
3416-
}
34173282
}

0 commit comments

Comments
 (0)