From fd8273824ab43a5c6720681fc011a89bf1f8a4e2 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 23 Aug 2026 10:54:49 +0200 Subject: [PATCH] test(player): drive an engine that answers badly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fair criticism: hardware time went into writing a checklist describing what the tablet found, when it should have gone into making the suites able to find it. A checklist decays and depends on someone following it. A test does not. The gap was specific. Every engine the conformance suite drives reports sane numbers, so it stayed green while a real one took the backend down. The old PlayerBackend contract is a plain f64 — it never promised finite, never promised positive, and nothing enforced it. UT-223 adds the engine that was missing: a HostileBackend answering with C.TIME_UNSET as seconds, NaN, both infinities, a negative and a zero. Reading a snapshot must yield no duration and a zero position rather than panicking. Against the adapter as originally written it fails with cannot convert float seconds to Duration: value is negative which is the exact panic that produced a black screen on the tablet — now reproduced in 0.00s on a laptop instead of by backgrounding an app. UT-224 pins the other hardware-only finding: stopping clears an active background-audio handoff, flag and base offset both. That was verified by listening to a device, which is not a test. Both were confirmed to fail against the pre-fix code before being kept. The verification plan now says to prefer moving cases out of it and into tests, and that what remains should be what genuinely needs eyes, ears or a display — not what merely has not been automated yet. 793 Rust tests. --- docs/native-player-verification.md | 15 +++- docs/requirements.md | 2 + src-tauri/src/player/legacy_player.rs | 104 ++++++++++++++++++++++++++ src-tauri/src/player/mod.rs | 39 ++++++++++ 4 files changed, 158 insertions(+), 2 deletions(-) diff --git a/docs/native-player-verification.md b/docs/native-player-verification.md index 4f0e9cf0..1620e5c9 100644 --- a/docs/native-player-verification.md +++ b/docs/native-player-verification.md @@ -25,8 +25,19 @@ release mechanics. This covers whether the player is fit to release at all. a wrong capability for ExoPlayer (DR-246 follow-up) and a `Duration` panic (DR-252). Both were invisible to the test suites. -The suites verify engines that behave. **The manual passes exist to catch -engines that do not.** +The suites originally verified only engines that *behave*, which is why both +regressions passed them. That gap is now partly closed in code rather than in +this document: `UT-223` drives a deliberately hostile engine — `C.TIME_UNSET`, +NaN, infinities, negatives — through the adapter, and fails with the exact +panic that produced a black screen on a tablet. `UT-224` pins the handoff +clearing that was previously verified by listening to a device. + +**Prefer moving cases out of this file and into tests.** Anything here that +could fail automatically should; a checklist depends on someone remembering to +follow it, and the two defects it was written for cost hardware time that would +have been better spent making the suites realistic. What is left below is what +genuinely needs eyes, ears, or a display — not what merely has not been +automated yet. ## 1. Automated gates diff --git a/docs/requirements.md b/docs/requirements.md index d6d2ca3f..560759f7 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -760,6 +760,8 @@ Internal architecture, components, and application logic. | UT-220 | The conformance suite: opening at a position starts there and never at zero, a seek issued while opening is honoured and overrides the start it overtook, pause and play are observable, close is silent and idempotent, and an open cancelled by close never begins playing | DR-242, DR-243 | In Progress | | UT-221 | An engine that cannot report a duration does not erase the one the item carries: with the queue holding a 1800s item and the engine answering nothing usable, the controller still reports 1800s | DR-251 | Done | | UT-222 | The values that killed the backend are rejected rather than converted: `C.TIME_UNSET` as seconds, negatives, zero, NaN and both infinities all yield no duration, while a real runtime survives | DR-252 | Done | +| UT-223 | The adapter survives an engine that answers badly. A `HostileBackend` reports `C.TIME_UNSET` as seconds, NaN, both infinities, a negative and a zero; reading a snapshot yields no duration and a zero position rather than panicking, and a well-behaved engine still round-trips. The conformance suite could not have caught this — it only ever drives engines that report sane numbers, which is why it stayed green while a real one took the backend down | DR-252 | Done | +| UT-224 | Stopping clears an active background-audio handoff, both the flag and the base offset, so a later position read cannot be interpreted against a handoff that no longer exists. Previously verified only by listening to a device | DR-250 | Done | ### Integration Tests diff --git a/src-tauri/src/player/legacy_player.rs b/src-tauri/src/player/legacy_player.rs index 03675497..d5a7dd4a 100644 --- a/src-tauri/src/player/legacy_player.rs +++ b/src-tauri/src/player/legacy_player.rs @@ -152,3 +152,107 @@ impl MediaPlayer for LegacyPlayer { self.capabilities } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::player::media::MediaItem; + use crate::settings::AudioSettings; + + /// A backend that answers badly, on purpose. + /// + /// Every engine the conformance suite drives reports sane numbers, which is + /// why it passed while a real one did not: ExoPlayer returns + /// `C.TIME_UNSET` — `Long::MIN_VALUE`, about -9.2e15 seconds — for any + /// stream whose length it does not know, and the adapter converted that + /// straight into a `Duration` and panicked the whole backend. + /// + /// The old `PlayerBackend` contract is a plain `f64`. It never promised + /// finite, never promised positive, and nothing enforced it. So this is the + /// engine the suites were missing. + struct HostileBackend { + duration: f64, + position: f64, + } + + impl PlayerBackend for HostileBackend { + fn load(&mut self, _media: &MediaItem) -> Result<(), PlayerError> { + Ok(()) + } + fn play(&mut self) -> Result<(), PlayerError> { + Ok(()) + } + fn pause(&mut self) -> Result<(), PlayerError> { + Ok(()) + } + fn stop(&mut self) -> Result<(), PlayerError> { + Ok(()) + } + fn seek(&mut self, _position: f64) -> Result<(), PlayerError> { + Ok(()) + } + fn set_volume(&mut self, _volume: f32) -> Result<(), PlayerError> { + Ok(()) + } + fn position(&self) -> f64 { + self.position + } + fn duration(&self) -> Option { + Some(self.duration) + } + fn state(&self) -> PlayerState { + PlayerState::Idle + } + fn volume(&self) -> f32 { + 1.0 + } + fn set_audio_settings(&mut self, _s: &AudioSettings) -> Result<(), PlayerError> { + Ok(()) + } + fn audio_settings(&self) -> AudioSettings { + AudioSettings::default() + } + fn set_audio_track(&mut self, _i: i32) -> Result<(), PlayerError> { + Ok(()) + } + fn set_subtitle_track(&mut self, _i: Option) -> Result<(), PlayerError> { + Ok(()) + } + } + + fn hostile(duration: f64, position: f64) -> LegacyPlayer { + LegacyPlayer::new( + HostileBackend { duration, position }, + crate::player::media_player::Capabilities::mpv(), + ) + } + + /// Reading an engine that answers badly must not take the process down. + /// + /// This is DR-252 as a test. It fails — by panicking — against the adapter + /// as originally written, which is the property the conformance suite could + /// not have: it only ever drove engines that behave. + /// + /// TRACES: UR-005 | DR-252 | UT-223 + #[test] + fn test_snapshot_survives_an_engine_that_answers_badly() { + // The exact value ExoPlayer reports for an unknown length. + let s = hostile(-9_223_372_036_854_776.0, 0.0).snapshot(); + assert_eq!(s.duration, None, "a negative duration is not a duration"); + + for bad in [f64::NAN, f64::NEG_INFINITY, f64::INFINITY, -1.0, 0.0] { + let s = hostile(bad, bad).snapshot(); + assert_eq!(s.duration, None, "{bad} should not become a duration"); + assert_eq!( + s.position, + Duration::ZERO, + "{bad} should not become a position" + ); + } + + // And a well-behaved engine still works. + let s = hostile(6997.024, 540.0).snapshot(); + assert_eq!(s.duration, Some(Duration::from_secs_f64(6997.024))); + assert_eq!(s.position, Duration::from_secs_f64(540.0)); + } +} diff --git a/src-tauri/src/player/mod.rs b/src-tauri/src/player/mod.rs index c630ce19..d9a7ff60 100644 --- a/src-tauri/src/player/mod.rs +++ b/src-tauri/src/player/mod.rs @@ -2313,6 +2313,45 @@ impl Default for PlayerController { #[cfg(test)] mod tests { + /// Stopping clears a background-audio handoff. + /// + /// This was verified by listening to a tablet, which is not a test. The + /// handoff swaps which renderer owns playback, and the swap is bookkeeping: + /// leaving the base offset and the active flag behind after a stop lets a + /// later position read be interpreted against a handoff that no longer + /// exists, and left the film playing on as an audio track in the mini + /// player. + /// + /// TRACES: UR-040, UR-005 | DR-250 | UT-224 + #[test] + fn test_stop_clears_an_active_background_audio_handoff() { + let controller = PlayerController::default(); + let item = MediaItem::sample("item-1", "https://example.invalid/a.mp4"); + { + let queue_arc = controller.queue(); + let mut queue = queue_arc.lock_safe(); + queue.set_queue(vec![item], 0); + } + + controller.enter_background_audio(557.5); + assert!( + controller.is_background_audio_active(), + "precondition: the handoff is active" + ); + + controller.stop().expect("stop failed"); + + assert!( + !controller.is_background_audio_active(), + "a stop must not leave a handoff behind for the next position read" + ); + assert_eq!( + *controller.background_audio_base.lock_safe(), + 0.0, + "the handoff base must be cleared with it" + ); + } + /// A duration the engine does not know must fall back to the one the item /// carries, and zero must count as "does not know". ///