fix(player): never let the server burn a subtitle in, and never offer one we cannot draw
Reported as "subtitles are shown even when off", and no toggle in the app cleared them — because they were not the app's subtitles at all. The server was painting them into the video. `PlaybackInfo` omitted `SubtitleStreamIndex`, which does not mean "none": the server then honours the source's own default/forced flag. On the reported episode that default is a PGS track — a bitmap, which cannot go out as a sidecar — so the server fell back to `SubtitleMethod=Encode` and composited it onto every frame. Confirmed against the live server, which answered the same PlaybackInfo request two ways: with the index omitted it returned `SubtitleStreamIndex=2` + `SubtitleMethod=Encode` and a `SubtitleCodecNotSupported` transcode reason, and its ffmpeg command carried `[0:2]…[sub];[main][sub]overlay_qsv=…`; with `-1` it selected no subtitle stream at all. The cost landed on the video, not the subtitle: burn-in rules out remuxing, so a stream that only needed its audio transcoded was re-encoded frame by frame. Three parts: - The negotiation asks for `SubtitleStreamIndex=-1` and advertises every text format we can render (srt/subrip/ass/ssa/vtt) as `External`. - The stream URL says the same thing, because the negotiation is not what opens most streams: a quality switch, a transcoded seek and an audio-track switch each rebuild the URL on their own, and an omitted index there lets the server pick the default track back up out of whatever session state it still holds. - The picker offers only subtitles the app can actually draw. Each subtitle stream now crosses the boundary carrying `supports_external_delivery`, decided in Rust where the codec vocabulary belongs, and `None` for anything that is not a subtitle so a `false` cannot be misread as a verdict. `subtitleStreamsOf()` drops the rejected ones — and since that one function feeds the menu, the `<track>` children and the native play request alike, a bitmap track disappears from all three without its URL ever being fetched. Only an explicit "no" hides a track; a stream carrying no verdict behaves exactly as before. Nothing is lost by refusing burn-in: the app already fetches the text tracks and draws them itself (UR-020), so the server's composited copy was always redundant. Image-based tracks are consequently not offered, which is honest rather than a regression — the renderer cannot composite a bitmap, and the old behaviour paid for them by making the whole stream unwatchable. Tests were written first and observed failing: the Rust one would not compile against a field that did not exist, and the frontend one resolved a URL for the PGS track it was supposed to drop. Carries with it the in-flight per-stream `PlaySessionId` work in online.rs, whose hunks sit inside the same request builder and could not be separated from these. TRACES: UR-020, UR-004 | DR-176 | UT-168
This commit is contained in:
@@ -44,6 +44,53 @@ pub fn streaming_quality() -> StreamingQuality {
|
||||
*STREAMING_QUALITY.read_safe()
|
||||
}
|
||||
|
||||
/// Every request this app makes identifies the same device, so the device id
|
||||
/// alone cannot tell two transcodes of the same item apart — see
|
||||
/// [`begin_video_play_session`].
|
||||
const DEVICE_ID: &str = "jellytau-tauri";
|
||||
|
||||
/// The `PlaySessionId` of the video transcode most recently opened, so the next
|
||||
/// open can stop it.
|
||||
///
|
||||
/// Process-wide for the same reason as [`STREAMING_QUALITY`]: it describes what
|
||||
/// *this device* currently has running on the server, and must survive the
|
||||
/// repository being rebuilt on re-login.
|
||||
///
|
||||
/// TRACES: UR-074 | DR-162
|
||||
static VIDEO_PLAY_SESSION: RwLock<Option<String>> = RwLock::new(None);
|
||||
|
||||
/// Claim a transcode identity for a stream about to be opened, returning the new
|
||||
/// `PlaySessionId` and the one it replaces (if any).
|
||||
///
|
||||
/// Jellyfin keys a transcode job by device *and* play session. Without a session
|
||||
/// id every open of the same item on this device looked like the same job, so
|
||||
/// re-opening a stream — a quality switch, a transcoded seek, an audio-track
|
||||
/// switch — left the old ffmpeg running and the server intermittently rejected
|
||||
/// segment requests for the new one (`400` on `hls1/main/0.ts`) while the two
|
||||
/// fought over one transcode path. The caller stops the returned previous
|
||||
/// session before the new stream's segments are fetched.
|
||||
///
|
||||
/// TRACES: UR-074 | DR-177 | UT-173
|
||||
pub fn begin_video_play_session() -> (String, Option<String>) {
|
||||
let new_session = uuid::Uuid::new_v4().to_string();
|
||||
let mut current = VIDEO_PLAY_SESSION.write_safe();
|
||||
let previous = current.replace(new_session.clone());
|
||||
(new_session, previous)
|
||||
}
|
||||
|
||||
/// Take ownership of a transcode this process did not build a URL for, returning
|
||||
/// the session it replaces.
|
||||
///
|
||||
/// When `PlaybackInfo` answers with a `TranscodingUrl` the server has already
|
||||
/// started the job and named the session; that id is the only handle on it we
|
||||
/// will ever have. Without adopting it, the first re-open of that stream has no
|
||||
/// previous session to stop and collides with the very job that was playing.
|
||||
///
|
||||
/// TRACES: UR-074 | DR-177 | UT-173
|
||||
pub fn adopt_video_play_session(session_id: String) -> Option<String> {
|
||||
VIDEO_PLAY_SESSION.write_safe().replace(session_id)
|
||||
}
|
||||
|
||||
/// A single actor returned by the JRay plugin's "context at time t" endpoint.
|
||||
///
|
||||
/// Mirrors the `actors[]` objects from `GET /Plugins/JRay/Items/{id}/jray?t=`.
|
||||
@@ -405,6 +452,48 @@ impl OnlineRepository {
|
||||
})
|
||||
}
|
||||
|
||||
/// Ask the server to tear down a transcode this device started.
|
||||
///
|
||||
/// Best-effort and deliberately un-retried: it runs on the path that opens a
|
||||
/// replacement stream, so a slow or failed stop must not delay playback. The
|
||||
/// worst case if it does fail is the job Jellyfin would have reaped on its
|
||||
/// own idle timer anyway — the new stream still has its own session id, so it
|
||||
/// no longer collides with the old one.
|
||||
///
|
||||
/// TRACES: UR-074 | DR-177
|
||||
async fn stop_transcode(&self, play_session_id: &str) {
|
||||
let url = format!(
|
||||
"{}/Videos/ActiveEncodings?deviceId={}&playSessionId={}",
|
||||
self.server_url, DEVICE_ID, play_session_id
|
||||
);
|
||||
|
||||
let request = self
|
||||
.http_client
|
||||
.client
|
||||
.delete(&url)
|
||||
.header("X-Emby-Authorization", self.auth_header())
|
||||
.send();
|
||||
|
||||
match request.await {
|
||||
Ok(response) if response.status().is_success() => {
|
||||
debug!("[Transcode] Stopped previous encoding {}", play_session_id);
|
||||
}
|
||||
Ok(response) => {
|
||||
debug!(
|
||||
"[Transcode] Server declined to stop encoding {}: HTTP {}",
|
||||
play_session_id,
|
||||
response.status()
|
||||
);
|
||||
}
|
||||
Err(e) => {
|
||||
debug!(
|
||||
"[Transcode] Could not stop encoding {}: {}",
|
||||
play_session_id, e
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Get a video stream URL for playback at an arbitrary position (resume,
|
||||
/// transcoded seeking, audio-track switching).
|
||||
///
|
||||
@@ -421,7 +510,7 @@ impl OnlineRepository {
|
||||
/// that suits the budget. `Original` keeps the historical 20/18 Mbps
|
||||
/// allowance, which is a transcode ceiling rather than a user-facing limit.
|
||||
///
|
||||
/// TRACES: UR-004, UR-074 | DR-140, DR-162 | UT-130, UT-156
|
||||
/// TRACES: UR-004, UR-074 | DR-140, DR-162, DR-177 | UT-130, UT-156, UT-173
|
||||
pub async fn get_video_stream_url(
|
||||
&self,
|
||||
item_id: &str,
|
||||
@@ -439,11 +528,21 @@ impl OnlineRepository {
|
||||
let max_bitrate = quality.max_bitrate().unwrap_or(20_000_000);
|
||||
let video_bitrate = quality.video_bitrate().unwrap_or(18_000_000);
|
||||
|
||||
// Claim a distinct transcode identity and retire the one it replaces, so
|
||||
// the server is never running two jobs for this device at once. Doing it
|
||||
// here covers every path that re-opens a stream (quality switch,
|
||||
// transcoded seek, audio-track switch) rather than each remembering to.
|
||||
let (play_session_id, superseded) = begin_video_play_session();
|
||||
if let Some(previous) = superseded {
|
||||
self.stop_transcode(&previous).await;
|
||||
}
|
||||
|
||||
// Build an HLS transcode URL. VideoCodec lists h264 first so the server
|
||||
// transcodes HEVC/10-bit/unsupported sources to h264 the WebView can decode.
|
||||
let mut params = vec![
|
||||
("api_key", self.access_token.clone()),
|
||||
("DeviceId", "jellytau-tauri".to_string()),
|
||||
("DeviceId", DEVICE_ID.to_string()),
|
||||
("PlaySessionId", play_session_id),
|
||||
("VideoCodec", "h264".to_string()),
|
||||
("AudioCodec", "aac".to_string()),
|
||||
("MaxStreamingBitrate", max_bitrate.to_string()),
|
||||
@@ -456,6 +555,18 @@ impl OnlineRepository {
|
||||
("SegmentContainer", "ts".to_string()),
|
||||
("TranscodingContainer", "ts".to_string()),
|
||||
("TranscodingProtocol", "hls".to_string()),
|
||||
// Say "no subtitle" rather than leaving the choice open. An omitted
|
||||
// index is not neutral: the server then picks the source's own
|
||||
// default/forced track, and an image-based one can only be delivered
|
||||
// by burning it into the picture (DR-176). The negotiation already
|
||||
// sends this sentinel, but most streams are opened by rebuilding
|
||||
// *this* URL — a quality switch, a transcoded seek, an audio-track
|
||||
// switch — so it has to hold here too, independently of whatever
|
||||
// session state the server still holds.
|
||||
(
|
||||
"SubtitleStreamIndex",
|
||||
super::device_profile::playback_subtitle_stream_index().to_string(),
|
||||
),
|
||||
];
|
||||
|
||||
// Scale the picture down to what the budget can carry. Omitted for the
|
||||
@@ -528,7 +639,7 @@ impl OnlineRepository {
|
||||
let mut params = vec![
|
||||
("UserId", self.user_id.clone()),
|
||||
("api_key", self.access_token.clone()),
|
||||
("DeviceId", "jellytau-tauri".to_string()),
|
||||
("DeviceId", DEVICE_ID.to_string()),
|
||||
// Progressive mp3 over HTTP — ExoPlayer-friendly; no HLS/ts.
|
||||
("Container", "mp3".to_string()),
|
||||
("AudioCodec", "mp3".to_string()),
|
||||
@@ -889,15 +1000,28 @@ impl JellyfinItem {
|
||||
media_streams: self.media_streams.map(|streams| {
|
||||
streams
|
||||
.into_iter()
|
||||
.map(|s| crate::repository::types::MediaStream {
|
||||
kind: crate::domain::stream_kind_from_jellyfin(&s.stream_type),
|
||||
stream_type: s.stream_type,
|
||||
codec: s.codec,
|
||||
language: s.language,
|
||||
display_title: s.display_title,
|
||||
index: s.index,
|
||||
is_default: s.is_default,
|
||||
is_forced: s.is_forced,
|
||||
.map(|s| {
|
||||
let kind = crate::domain::stream_kind_from_jellyfin(&s.stream_type);
|
||||
// Only a subtitle can be a sidecar; asked of anything
|
||||
// else the question has no answer. TRACES: UR-020 |
|
||||
// DR-176 | UT-168
|
||||
let supports_external_delivery =
|
||||
(kind == crate::domain::StreamKind::Subtitle).then(|| {
|
||||
super::device_profile::subtitle_supports_external_delivery(
|
||||
s.codec.as_deref(),
|
||||
)
|
||||
});
|
||||
crate::repository::types::MediaStream {
|
||||
kind,
|
||||
stream_type: s.stream_type,
|
||||
codec: s.codec,
|
||||
language: s.language,
|
||||
display_title: s.display_title,
|
||||
index: s.index,
|
||||
is_default: s.is_default,
|
||||
is_forced: s.is_forced,
|
||||
supports_external_delivery,
|
||||
}
|
||||
})
|
||||
.collect()
|
||||
}),
|
||||
@@ -1609,6 +1733,12 @@ impl MediaRepository for OnlineRepository {
|
||||
|
||||
// Use TranscodingUrl from response if available (Streamyfin pattern)
|
||||
let stream_url = if let Some(transcoding_url) = &source.transcoding_url {
|
||||
// The server started this job and named the session — adopt it, or a
|
||||
// later quality switch / seek on this stream has no previous job to
|
||||
// stop and ends up contending with the one currently playing.
|
||||
if let Some(previous) = adopt_video_play_session(response.play_session_id.clone()) {
|
||||
self.stop_transcode(&previous).await;
|
||||
}
|
||||
format!("{}{}", self.server_url, transcoding_url)
|
||||
} else if audio_forces_transcode {
|
||||
warn!(
|
||||
@@ -2658,6 +2788,140 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// Jellyfin keys a transcode job by device *and* play session. Every stream
|
||||
/// this app opened used the same `DeviceId` and no `PlaySessionId`, so
|
||||
/// re-opening the same item — what a mid-playback quality switch, a
|
||||
/// transcoded seek and an audio-track switch all do — handed the server a
|
||||
/// second job it could not tell apart from the one still running. Observed
|
||||
/// on-device: the new playlist is served, then `hls1/main/0.ts` 400s
|
||||
/// intermittently while the two jobs fight over the same transcode path, and
|
||||
/// playback stalls.
|
||||
///
|
||||
/// TRACES: UR-074 | DR-177 | UT-173
|
||||
#[tokio::test]
|
||||
async fn test_video_stream_url_carries_a_play_session_id() {
|
||||
let _fixture = QualityFixture::set(StreamingQuality::Original);
|
||||
let repo = create_test_repository();
|
||||
|
||||
let url = repo
|
||||
.get_video_stream_url("vid-1", None, None, None)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
url.contains("PlaySessionId="),
|
||||
"every transcode must be openable as its own job: {url}"
|
||||
);
|
||||
}
|
||||
|
||||
/// Naming no subtitle stream is not the same as asking for none. The server
|
||||
/// fills the gap with the source's own default/forced track, and an
|
||||
/// image-based one (PGS/DVD/DVB) can only be delivered by painting it into
|
||||
/// the picture — the burn-in of DR-176, arriving through the URL rather than
|
||||
/// through the negotiation.
|
||||
///
|
||||
/// The negotiation already sends the sentinel, but it is not what opens most
|
||||
/// streams: a quality switch, a transcoded seek and an audio-track switch all
|
||||
/// build this URL again, on their own. Saying it here too makes "no subtitle"
|
||||
/// a property of the request instead of something inherited from whatever
|
||||
/// session state the server happens to still hold.
|
||||
///
|
||||
/// TRACES: UR-020, UR-004 | DR-176 | UT-168
|
||||
#[tokio::test]
|
||||
async fn test_video_stream_url_asks_for_no_subtitle_stream() {
|
||||
let _fixture = QualityFixture::set(StreamingQuality::Original);
|
||||
let repo = create_test_repository();
|
||||
|
||||
let url = repo
|
||||
.get_video_stream_url("vid-1", Some("source-1"), Some(12.0), Some(1))
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(
|
||||
url.contains("SubtitleStreamIndex=-1"),
|
||||
"the stream URL must ask for no subtitle, not leave the choice open: {url}"
|
||||
);
|
||||
}
|
||||
|
||||
/// The picker must not offer a subtitle the app cannot draw. Image-based
|
||||
/// tracks are bitmaps: the only way to show one is to have the server
|
||||
/// composite it, which is exactly what DR-176 stopped asking for. Selecting
|
||||
/// one was therefore a control that could not do anything — so the verdict
|
||||
/// travels with the stream, decided here where the codec vocabulary lives.
|
||||
///
|
||||
/// TRACES: UR-020 | DR-176 | UT-168
|
||||
#[test]
|
||||
fn test_media_streams_carry_whether_the_app_can_render_them() {
|
||||
let item: JellyfinItem = serde_json::from_value(serde_json::json!({
|
||||
"Id": "ep-1",
|
||||
"Name": "Partings",
|
||||
"Type": "Episode",
|
||||
"MediaStreams": [
|
||||
{ "Type": "Video", "Index": 0, "Codec": "hevc", "IsDefault": true },
|
||||
{ "Type": "Audio", "Index": 1, "Codec": "eac3", "IsDefault": true },
|
||||
{ "Type": "Subtitle", "Index": 2, "Codec": "PGSSUB", "IsDefault": true },
|
||||
{ "Type": "Subtitle", "Index": 3, "Codec": "subrip", "IsDefault": false },
|
||||
{ "Type": "Subtitle", "Index": 4, "Codec": null, "IsDefault": false },
|
||||
],
|
||||
}))
|
||||
.expect("fixture must deserialize");
|
||||
|
||||
let streams = item.to_media_item("server-1".to_string()).media_streams;
|
||||
let streams = streams.expect("the item carries streams");
|
||||
let deliverable = |index: i32| {
|
||||
streams
|
||||
.iter()
|
||||
.find(|s| s.index == index)
|
||||
.unwrap_or_else(|| panic!("stream {index} missing"))
|
||||
.supports_external_delivery
|
||||
};
|
||||
|
||||
// The bitmap track the server would have had to burn in.
|
||||
assert_eq!(deliverable(2), Some(false));
|
||||
// Text: fetched as WebVTT and drawn by the app itself.
|
||||
assert_eq!(deliverable(3), Some(true));
|
||||
// A subtitle whose format the server did not name could be anything;
|
||||
// offering it risks a dead control, so it is not offered.
|
||||
assert_eq!(deliverable(4), Some(false));
|
||||
// Meaningless for anything that is not a subtitle — and said as `None`
|
||||
// rather than as a `false` a reader could mistake for a verdict.
|
||||
assert_eq!(deliverable(0), None);
|
||||
assert_eq!(deliverable(1), None);
|
||||
}
|
||||
|
||||
/// The session id is what makes two opens *distinguishable*, so a fresh one
|
||||
/// per open is the whole point — and the open must report the id it replaced
|
||||
/// so the caller can stop that job instead of leaving it running.
|
||||
///
|
||||
/// TRACES: UR-074 | DR-177 | UT-173
|
||||
#[test]
|
||||
fn test_each_stream_open_gets_a_fresh_session_and_reports_the_previous() {
|
||||
let _lock = QUALITY_LOCK.lock_safe();
|
||||
|
||||
let (first, _) = begin_video_play_session();
|
||||
let (second, replaced) = begin_video_play_session();
|
||||
|
||||
assert_ne!(first, second, "each open needs its own job identity");
|
||||
assert_eq!(
|
||||
replaced,
|
||||
Some(first),
|
||||
"the open must hand back the job it superseded so it can be stopped"
|
||||
);
|
||||
|
||||
// A server-started transcode (PlaybackInfo answered with a TranscodingUrl)
|
||||
// has to become the current session too — otherwise the first switch on
|
||||
// that stream stops nothing and collides with what is playing.
|
||||
let replaced_by_adoption = adopt_video_play_session("server-named-session".to_string());
|
||||
assert_eq!(replaced_by_adoption, Some(second));
|
||||
|
||||
let (_, after_adoption) = begin_video_play_session();
|
||||
assert_eq!(
|
||||
after_adoption,
|
||||
Some("server-named-session".to_string()),
|
||||
"the adopted job must be the one the next open stops"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_get_audio_only_stream_url_for_video_carries_track_and_position() {
|
||||
// TRACES: UR-040 | JA-032 | UT-059
|
||||
|
||||
Reference in New Issue
Block a user