fix(player): three defects from review, and one duplicate removed
Build & Release / Create Release (push) Blocked by required conditions
Publish Documentation / Build & publish docs to gitea-pages (push) Canceled after 0s
🏗️ Build and Test JellyTau / Run Tests (push) Successful in 21m37s
🏗️ Build and Test JellyTau / Supply Chain (push) Successful in 2m55s
Traceability Validation / Check Requirement Traces (push) Successful in 23s
Build & Release / Run Tests (push) Successful in 15m34s
Build & Release / Build Windows (push) Waiting to run
Build & Release / Build Android (push) Waiting to run
🏗️ Build and Test JellyTau / Android Compile Check (push) Successful in 4m26s
Build & Release / Build Linux (push) In progress
Build & Release / Create Release (push) Blocked by required conditions
Publish Documentation / Build & publish docs to gitea-pages (push) Canceled after 0s
🏗️ Build and Test JellyTau / Run Tests (push) Successful in 21m37s
🏗️ Build and Test JellyTau / Supply Chain (push) Successful in 2m55s
Traceability Validation / Check Requirement Traces (push) Successful in 23s
Build & Release / Run Tests (push) Successful in 15m34s
Build & Release / Build Windows (push) Waiting to run
Build & Release / Build Android (push) Waiting to run
🏗️ Build and Test JellyTau / Android Compile Check (push) Successful in 4m26s
Build & Release / Build Linux (push) In progress
Verified each against the code before acting; four of the five findings held, one did not. DR-253 — a deferred seek outlived its file. `seek` holds a position while MPV has nothing loaded and `FileLoaded` applies it (DR-241), but neither `load` nor `stop` discarded it. Scrub near the end of a transcoded item — which re-opens the stream — then skip to the next item before the reload completes, and the old position lands on the new item. It starts wherever the previous one was scrubbed to, silently. Both lifecycle points clear it now. DR-254 — a per-playback quality ceiling outlived its playback. The override is process-wide and describes one playback: dropping to 720p for a struggling episode says nothing about the next. Every advance the frontend drives clears it through player_play_item, but the background audio-only advance loads the next episode in Rust and skipped all three clearing sites — so every later episode stayed capped, with nothing in the UI explaining why. DR-255 — `playable_url` was a byte-identical copy of `playback_url`, added for the cross-platform open path. The original is `#[cfg(target_os = "android")]`, so it does not exist in a Linux build and nothing warned. Two matches over MediaSource meant a new variant could be handled in one and forgotten in the other. The gate is gone and the copy with it. The fifth finding — that the comment on `video_audio_codecs` describes a renderer switch the code no longer has — does not hold. `get_player_status` hard-codes Android to Native, but `experimentalNativeVideo` is still live in VideoPlayer.svelte as a suppressor that can force HTML5 even when Rust says native. The switch exists, so the narrow codec list is still doing its job. Both correctness fixes are red-then-green. The tests are wiring assertions in the style of UT-218: what matters is the call site, and reaching these at runtime needs a live MPV handle or a repository, a server and a player. That technique now appears three times and is worth watching — it pins call sites, not behaviour. The review's sharpest point is one it raised as redundancy: MpvPlayer already handles DR-253 correctly, resetting deferred state on every open, and the old path had to be patched separately. That is the drift two parallel engines produce, and the argument for finishing DR-248/249 rather than leaving LegacyPlayer in place indefinitely. 795 Rust tests, 1088 frontend, every CI check green locally.
This commit is contained in:
@@ -171,19 +171,6 @@ pub enum MediaSource {
|
||||
DirectUrl { url: String },
|
||||
}
|
||||
|
||||
impl MediaItem {
|
||||
/// The URL or path an engine should open.
|
||||
///
|
||||
/// TRACES: UR-081 | DR-245
|
||||
pub fn playable_url(&self) -> String {
|
||||
match &self.source {
|
||||
MediaSource::Remote { stream_url, .. } => stream_url.clone(),
|
||||
MediaSource::Local { file_path, .. } => file_path.to_string_lossy().into_owned(),
|
||||
MediaSource::DirectUrl { url } => url.clone(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl MediaItem {
|
||||
/// Get the Jellyfin item ID if available
|
||||
pub fn jellyfin_id(&self) -> Option<&str> {
|
||||
@@ -198,10 +185,16 @@ impl MediaItem {
|
||||
}
|
||||
}
|
||||
|
||||
/// Get the playback URL or file path
|
||||
/// The URL or path an engine should open.
|
||||
///
|
||||
/// Only available on Android where ExoPlayer needs direct URL access
|
||||
#[cfg(target_os = "android")]
|
||||
/// Not gated to Android any more. It was, back when only ExoPlayer needed
|
||||
/// direct URL access — and that gate is why a byte-identical copy was later
|
||||
/// added for the cross-platform `MediaPlayer::open` path without anyone
|
||||
/// noticing this existed: it is invisible in a Linux build, so nothing
|
||||
/// warned. Two matches over `MediaSource` meant a new variant could be
|
||||
/// handled in one and forgotten in the other, silently.
|
||||
///
|
||||
/// TRACES: UR-081 | DR-245, DR-255
|
||||
pub fn playback_url(&self) -> String {
|
||||
match &self.source {
|
||||
MediaSource::Remote { stream_url, .. } => stream_url.clone(),
|
||||
|
||||
@@ -582,7 +582,7 @@ impl PlayerController {
|
||||
backend.open(OpenRequest::new(
|
||||
item.clone(),
|
||||
StreamSelection::for_queued_item(
|
||||
item.playable_url(),
|
||||
item.playback_url(),
|
||||
item.transport,
|
||||
item.needs_transcoding,
|
||||
),
|
||||
@@ -1995,6 +1995,15 @@ impl PlayerController {
|
||||
&self,
|
||||
next_episode_id: &str,
|
||||
) -> Result<(), String> {
|
||||
// A new episode is a new playback, so a ceiling chosen for the previous
|
||||
// one does not carry into it. Every advance the frontend drives goes
|
||||
// through `player_play_item` and is cleared there; this one loads the
|
||||
// next episode in Rust and would otherwise keep the old cap forever,
|
||||
// with nothing in the UI saying why. Cleared before the URL is built,
|
||||
// since that is what reads it.
|
||||
// TRACES: UR-074 | DR-254
|
||||
crate::repository::online::clear_playback_quality_override();
|
||||
|
||||
let repo = self
|
||||
.repository
|
||||
.lock_safe()
|
||||
@@ -2313,6 +2322,39 @@ impl Default for PlayerController {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
|
||||
/// Advancing to the next episode drops a per-playback quality override.
|
||||
///
|
||||
/// The override is process-wide and describes *one* playback: a viewer who
|
||||
/// drops to 720p for a struggling episode has said nothing about the next
|
||||
/// one. `player_play_item`, `player_play_queue` and `player_play_tracks`
|
||||
/// all clear it, so every advance the frontend drives is covered — but the
|
||||
/// background audio-only advance loads the next episode in Rust and skips
|
||||
/// all three, so every later episode stayed capped at the old quality with
|
||||
/// nothing in the UI saying so.
|
||||
///
|
||||
/// A wiring assertion, like UT-218 and UT-225: the call site is what
|
||||
/// matters, and reaching it at runtime needs a repository, a server and a
|
||||
/// live player.
|
||||
///
|
||||
/// TRACES: UR-074 | DR-254 | UT-226
|
||||
#[test]
|
||||
fn test_background_episode_advance_clears_the_quality_override() {
|
||||
let src = include_str!("mod.rs");
|
||||
let start = src
|
||||
.find("fn advance_to_next_episode_audio_only")
|
||||
.expect("advance_to_next_episode_audio_only not found");
|
||||
let rest = &src[start..];
|
||||
let end = rest.find("\n pub ").unwrap_or(rest.len());
|
||||
let body = &rest[..end];
|
||||
|
||||
assert!(
|
||||
body.contains("clear_playback_quality_override"),
|
||||
"the background episode advance does not clear the per-playback \
|
||||
quality override, so a ceiling chosen for one episode silently \
|
||||
caps every episode after it"
|
||||
);
|
||||
}
|
||||
|
||||
/// Stopping clears a background-audio handoff.
|
||||
///
|
||||
/// This was verified by listening to a tablet, which is not a test. The
|
||||
|
||||
@@ -592,6 +592,14 @@ impl PlayerBackend for MpvBackend {
|
||||
// one's "last observed" position.
|
||||
self.observed.lock_safe().reset();
|
||||
|
||||
// Nor its deferred seek. A seek held for a file that is no longer the
|
||||
// one loading would be applied to this one by the `FileLoaded` handler
|
||||
// — so scrubbing near the end of a transcoded item, which re-opens the
|
||||
// stream, and then skipping to the next item before the reload finished
|
||||
// started the new item wherever the old one had been scrubbed to.
|
||||
// TRACES: UR-040, UR-005 | DR-253
|
||||
*self.pending_seek.lock_safe() = None;
|
||||
|
||||
// Load the media file
|
||||
self.mpv
|
||||
.command("loadfile", &[&stream_url])
|
||||
@@ -634,6 +642,10 @@ impl PlayerBackend for MpvBackend {
|
||||
message: format!("Failed to stop: {:?}", e),
|
||||
})?;
|
||||
|
||||
// Stopping ends the seek's subject along with the playback.
|
||||
// TRACES: UR-040, UR-005 | DR-253
|
||||
*self.pending_seek.lock_safe() = None;
|
||||
|
||||
let mut state = self.state.lock_safe();
|
||||
state.current_media = None;
|
||||
|
||||
|
||||
@@ -59,6 +59,43 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// A deferred seek belongs to the file it was issued against.
|
||||
///
|
||||
/// `seek` holds a position when MPV has nothing loaded yet, and the
|
||||
/// `FileLoaded` handler applies it (DR-241). Nothing discarded it when a
|
||||
/// *different* file was loaded or playback stopped — so scrubbing near the
|
||||
/// end of a transcoded item (which re-opens the stream) and then skipping to
|
||||
/// the next item before the reload completed applied the old position to the
|
||||
/// new item. It silently started wherever you had scrubbed to in the
|
||||
/// previous one.
|
||||
///
|
||||
/// Asserted against the source: the state lives behind a live MPV handle,
|
||||
/// and constructing one needs libmpv and an audio device that CI cannot be
|
||||
/// assumed to have. Crude, but it pins the one thing that matters — that
|
||||
/// both lifecycle points discard it.
|
||||
///
|
||||
/// TRACES: UR-040, UR-005 | DR-253 | UT-225
|
||||
#[test]
|
||||
fn test_load_and_stop_discard_a_deferred_seek() {
|
||||
let src = include_str!("mpv_backend.rs");
|
||||
|
||||
for func in ["fn load(", "fn stop("] {
|
||||
let start = src
|
||||
.find(func)
|
||||
.unwrap_or_else(|| panic!("{func} not found - has the backend been restructured?"));
|
||||
// The body runs to the next top-level ` fn ` at the same depth.
|
||||
let rest = &src[start + func.len()..];
|
||||
let end = rest.find("\n fn ").unwrap_or(rest.len());
|
||||
let body = &rest[..end];
|
||||
|
||||
assert!(
|
||||
body.contains("pending_seek"),
|
||||
"{func} does not discard `pending_seek`. A seek held for a file \
|
||||
that is no longer loading will be applied to whatever loads next."
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Test that simulates the position update thread spawning async tasks
|
||||
/// without a Tokio runtime (the bug we just fixed)
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user