feat(player): seek strategy follows what the engine says it can do
DR-246. The strategy used to turn on `is_hls` and `use_html5`, decided in a command handler on behalf of engines it does not own. That is how "who renders" came to mean "how do I seek", and why a transcoded seek silently did nothing the moment native video changed the renderer (DR-238). Engines now declare `Capabilities::seeks_transcoded_in_place` — true for hls.js, which seeks within the VOD playlist it was handed and lets the server catch up; false for mpv, whose HLS demuxer cannot make the server transcode from a new offset. The command asks whichever engine is rendering. Adding an engine no longer means editing a shared truth table. The item's transport is not read at the seek site any more; the compiler flagged it unused, which is the URL-shape input finally disappearing. A deviation from the spec, recorded deliberately: it called for the engine to own the decision outright. It cannot. Re-negotiating a stream needs the repository, which sits above the engine, so the engine states the ability and the caller acts on it. That still removes the defect — nobody guesses on another component's behalf — without pretending an engine can reach upward. Also fixes a latent race in the conformance suite, found by running it: the seek case asserted immediately, which passes on an engine that records the target when it accepts a seek and races on one that waits for the decoder to move. `Harness::await_seek` polls instead, the way the Android suite already did. It failed with machine load rather than with the code, which is the kind of test that teaches people to re-run until green. MpvPlayer 9/9 LegacyPlayer 8/9 - still only the mute/rate gap in the old trait 789 tests, clippy -D warnings clean with and without the feature.
This commit is contained in:
@@ -53,6 +53,17 @@ pub trait Harness {
|
||||
fn seek_tolerance(&self) -> Duration {
|
||||
Duration::from_secs(5)
|
||||
}
|
||||
|
||||
/// Wait for a completed seek to be visible in `snapshot()`.
|
||||
///
|
||||
/// Engines differ in when that happens: one may record the target the
|
||||
/// moment it accepts the seek, another may not report it until the decoder
|
||||
/// has actually moved. Asserting immediately therefore passes on the first
|
||||
/// and races on the second — which is precisely how this suite produced a
|
||||
/// failure that came and went with machine load rather than with the code.
|
||||
///
|
||||
/// Default is a no-op, for engines whose snapshot is synchronous.
|
||||
fn await_seek(&mut self, _target: Duration) {}
|
||||
}
|
||||
|
||||
fn assert_near(actual: Duration, expected: Duration, tolerance: Duration, what: &str) {
|
||||
@@ -151,6 +162,7 @@ pub fn seeks_after_open<H: Harness>(h: &mut H) {
|
||||
|
||||
let target = Duration::from_secs(420);
|
||||
h.player().seek(target).expect("seek failed");
|
||||
h.await_seek(target);
|
||||
|
||||
assert_near(
|
||||
h.player().snapshot().position,
|
||||
|
||||
@@ -72,6 +72,8 @@ impl FakePlayer {
|
||||
audio_settings: true,
|
||||
subtitle_switching: true,
|
||||
audio_track_switching: true,
|
||||
// The fake honours a seek in any phase, so it can claim this.
|
||||
seeks_transcoded_in_place: true,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
@@ -26,6 +26,11 @@ use super::state::PlayerState;
|
||||
|
||||
pub struct LegacyPlayer<B: PlayerBackend> {
|
||||
inner: B,
|
||||
/// Declared at construction: this wrapper is generic over engines with very
|
||||
/// different abilities, and only the composition root knows which one it
|
||||
/// just built. Guessing here would reintroduce exactly the inference DR-238
|
||||
/// removed.
|
||||
capabilities: Capabilities,
|
||||
/// The old trait has no notion of "opening", so this is the best the wrapper
|
||||
/// can do: it knows an item was handed over, not whether the engine is ready
|
||||
/// for one. That gap is the whole problem.
|
||||
@@ -33,9 +38,10 @@ pub struct LegacyPlayer<B: PlayerBackend> {
|
||||
}
|
||||
|
||||
impl<B: PlayerBackend> LegacyPlayer<B> {
|
||||
pub fn new(inner: B) -> Self {
|
||||
pub fn new(inner: B, capabilities: Capabilities) -> Self {
|
||||
Self {
|
||||
inner,
|
||||
capabilities,
|
||||
has_item: false,
|
||||
}
|
||||
}
|
||||
@@ -141,11 +147,6 @@ impl<B: PlayerBackend + Send> MediaPlayer for LegacyPlayer<B> {
|
||||
}
|
||||
|
||||
fn capabilities(&self) -> Capabilities {
|
||||
Capabilities {
|
||||
video: false,
|
||||
audio_settings: true,
|
||||
subtitle_switching: true,
|
||||
audio_track_switching: true,
|
||||
}
|
||||
self.capabilities
|
||||
}
|
||||
}
|
||||
|
||||
@@ -121,6 +121,50 @@ pub struct Capabilities {
|
||||
pub subtitle_switching: bool,
|
||||
/// Audio tracks can be selected without re-opening.
|
||||
pub audio_track_switching: bool,
|
||||
/// A *server-side transcode* can be seeked without re-opening the stream.
|
||||
///
|
||||
/// True for hls.js, which seeks within the VOD playlist it is handed and
|
||||
/// lets the server catch up. False for mpv, whose HLS demuxer cannot make
|
||||
/// the server transcode from a new offset.
|
||||
///
|
||||
/// Declared by the engine rather than inferred by the caller. The previous
|
||||
/// design decided this from `is_hls` and `use_html5` in a command handler —
|
||||
/// on behalf of engines it did not own — which is how "who renders" came to
|
||||
/// mean "how do I seek" and why a transcoded seek silently did nothing the
|
||||
/// moment native video changed the renderer (DR-238).
|
||||
///
|
||||
/// Re-negotiating a stream needs the repository, which sits above the
|
||||
/// engine, so the engine states the capability and the caller acts on it.
|
||||
pub seeks_transcoded_in_place: bool,
|
||||
}
|
||||
|
||||
impl Capabilities {
|
||||
/// What a native engine of this project's kind can do.
|
||||
///
|
||||
/// `seeks_transcoded_in_place` is false: both native engines decode the
|
||||
/// stream themselves and neither can make the server transcode from a new
|
||||
/// offset. hls.js is the exception, and says so for itself.
|
||||
pub fn native() -> Self {
|
||||
Self {
|
||||
video: true,
|
||||
audio_settings: true,
|
||||
subtitle_switching: true,
|
||||
audio_track_switching: true,
|
||||
seeks_transcoded_in_place: false,
|
||||
}
|
||||
}
|
||||
|
||||
/// An engine that renders through the webview element, where hls.js seeks
|
||||
/// within the playlist it was handed.
|
||||
pub fn webview() -> Self {
|
||||
Self {
|
||||
video: true,
|
||||
audio_settings: false,
|
||||
subtitle_switching: true,
|
||||
audio_track_switching: false,
|
||||
seeks_transcoded_in_place: true,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// A request to present an item.
|
||||
|
||||
@@ -976,6 +976,13 @@ impl PlayerController {
|
||||
}
|
||||
}
|
||||
|
||||
/// What the engine currently rendering can do.
|
||||
///
|
||||
/// TRACES: UR-081 | DR-246
|
||||
pub fn capabilities(&self) -> crate::player::media_player::Capabilities {
|
||||
self.backend.lock_safe().capabilities()
|
||||
}
|
||||
|
||||
/// Get current position
|
||||
pub fn position(&self) -> f64 {
|
||||
self.backend.lock_safe().snapshot().position.as_secs_f64()
|
||||
@@ -2244,7 +2251,10 @@ impl Default for PlayerController {
|
||||
let playback_reporter = Arc::new(TokioMutex::new(None));
|
||||
let position_throttler = Arc::new(EventThrottler::new());
|
||||
Self::new(
|
||||
Box::new(LegacyPlayer::new(NullBackend::new())),
|
||||
Box::new(LegacyPlayer::new(
|
||||
NullBackend::new(),
|
||||
crate::player::media_player::Capabilities::native(),
|
||||
)),
|
||||
playback_reporter,
|
||||
position_throttler,
|
||||
)
|
||||
|
||||
@@ -373,6 +373,9 @@ impl MediaPlayer for MpvPlayer {
|
||||
audio_settings: true,
|
||||
subtitle_switching: true,
|
||||
audio_track_switching: true,
|
||||
// mpv's HLS demuxer cannot make the server transcode from a new
|
||||
// offset, so a transcoded seek must re-open the stream.
|
||||
seeks_transcoded_in_place: false,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -25,12 +25,14 @@ pub enum VideoSeekStrategy {
|
||||
///
|
||||
/// # Arguments
|
||||
/// * `is_local` - Whether the file is a local download
|
||||
/// * `is_hls` - Whether the stream URL contains ".m3u8" (HLS stream)
|
||||
/// * `seeks_transcoded_in_place` - Whether the engine rendering this stream
|
||||
/// can seek a server-side transcode without re-opening it. Declared by the
|
||||
/// engine via `Capabilities`, never inferred from the URL or the renderer.
|
||||
/// * `needs_transcoding` - Whether the content needs transcoding
|
||||
/// * `use_html5` - Whether frontend is using HTML5 video element
|
||||
pub fn determine_video_seek_strategy(
|
||||
is_local: bool,
|
||||
is_hls: bool,
|
||||
seeks_transcoded_in_place: bool,
|
||||
needs_transcoding: bool,
|
||||
use_html5: bool,
|
||||
) -> VideoSeekStrategy {
|
||||
@@ -53,14 +55,15 @@ pub fn determine_video_seek_strategy(
|
||||
// native video on routed every transcoded seek into a backend seek that
|
||||
// silently does nothing, and presents as "resume does not work".
|
||||
if needs_transcoding {
|
||||
return if use_html5 {
|
||||
if is_hls {
|
||||
VideoSeekStrategy::Html5NativeSeek
|
||||
} else {
|
||||
VideoSeekStrategy::Html5ReloadStream
|
||||
}
|
||||
} else {
|
||||
VideoSeekStrategy::BackendReloadStream
|
||||
// Whether a transcode can be seeked in place is a property of the
|
||||
// engine, and the engine states it. This used to be inferred from
|
||||
// `is_hls`, which held only while hls.js was the sole HLS renderer —
|
||||
// and stopped holding the moment mpv became one (DR-238).
|
||||
return match (seeks_transcoded_in_place, use_html5) {
|
||||
(true, true) => VideoSeekStrategy::Html5NativeSeek,
|
||||
(true, false) => VideoSeekStrategy::BackendNativeSeek,
|
||||
(false, true) => VideoSeekStrategy::Html5ReloadStream,
|
||||
(false, false) => VideoSeekStrategy::BackendReloadStream,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -235,20 +238,21 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// Test video seek strategy for HLS streams
|
||||
/// Non-transcoded streams seek in place regardless of the engine's
|
||||
/// transcode ability, which only applies to transcodes.
|
||||
#[test]
|
||||
fn test_seek_strategy_hls_stream() {
|
||||
// HLS with HTML5 - frontend handles seek, don't call backend
|
||||
fn test_seek_strategy_direct_stream() {
|
||||
// HTML5 renders, so the frontend seeks the element
|
||||
assert_eq!(
|
||||
determine_video_seek_strategy(false, true, false, true),
|
||||
VideoSeekStrategy::Html5NativeSeek
|
||||
);
|
||||
// HLS with native backend - backend handles seek
|
||||
// The native engine renders, so it seeks
|
||||
assert_eq!(
|
||||
determine_video_seek_strategy(false, true, false, false),
|
||||
VideoSeekStrategy::BackendNativeSeek
|
||||
);
|
||||
// HLS even with needs_transcoding flag - still native seek (HLS supports it)
|
||||
// A transcode an engine says it can move: seek in place
|
||||
assert_eq!(
|
||||
determine_video_seek_strategy(false, true, true, true),
|
||||
VideoSeekStrategy::Html5NativeSeek
|
||||
@@ -265,18 +269,30 @@ mod tests {
|
||||
/// every transcoded seek into a native seek that silently does nothing,
|
||||
/// which presents as "resume does not work".
|
||||
///
|
||||
/// TRACES: UR-040 | DR-238 | UT-217
|
||||
/// TRACES: UR-040 | DR-238, DR-246 | UT-217
|
||||
#[test]
|
||||
fn test_seek_strategy_transcoded_hls_native_backend() {
|
||||
fn test_transcoded_seek_follows_the_engines_declared_ability() {
|
||||
// An engine that cannot move a server-side transcode re-opens it,
|
||||
// whichever side is rendering.
|
||||
assert_eq!(
|
||||
determine_video_seek_strategy(false, true, true, false),
|
||||
determine_video_seek_strategy(false, false, true, false),
|
||||
VideoSeekStrategy::BackendReloadStream
|
||||
);
|
||||
// The HTML5 side of the same case is unchanged: hls.js seeks in-playlist.
|
||||
assert_eq!(
|
||||
determine_video_seek_strategy(false, false, true, true),
|
||||
VideoSeekStrategy::Html5ReloadStream
|
||||
);
|
||||
// hls.js can, and says so, so it seeks in place.
|
||||
assert_eq!(
|
||||
determine_video_seek_strategy(false, true, true, true),
|
||||
VideoSeekStrategy::Html5NativeSeek
|
||||
);
|
||||
// The container the stream arrives in no longer decides anything: the
|
||||
// same declared ability gives the same answer on the native side.
|
||||
assert_eq!(
|
||||
determine_video_seek_strategy(false, true, true, false),
|
||||
VideoSeekStrategy::BackendNativeSeek
|
||||
);
|
||||
}
|
||||
|
||||
/// Test video seek strategy for direct play (non-transcoded) streams
|
||||
|
||||
Reference in New Issue
Block a user