Add option to always burn in subtitles if transcoding is triggered

[gnattu]

Changes

Subtitles often have synchronization issues after video transcoding. This option allows the client to opt-in to always burn in all subtitles, preventing out-of-sync subtitles at the cost of transcoding performance.

This also fixed the helper text for subtitle burn in as that option itself will trigger transcoding.

Issues

Depends on jellyfin/jellyfin#12430

[github-actions]

ESLint doesn't pass. Please fix all ESLint issues.

[jellyfin-bot]

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

[dmitrylyzo]

The changes requested below are not necessary - it works as is.

Stale

[gnattu]

The logic is now reworked to reflect the transcoding behavior more accurately. Now depends on server PR #12676

[github-actions]

ESLint doesn't pass. Please fix all ESLint issues.

[dmitrylyzo]

A quick look.

I was also thinking that if we could get the current (sub)stream URL, we could get the "delivery mode" from there. But I'm not sure we can get such information from a native player.

[gnattu]

I was also thinking that if we could get the current (sub)stream URL, we could get the "delivery mode" from there.

Not going to work because there will be no change to that because the transcoding pipeline checking for copy codec happens way after that

[dmitrylyzo]

Not going to work because there will be no change to that because the transcoding pipeline checking for copy codec happens way after that

Doing this in the master.m3u8 endpoint would be sufficient. What if streams (main.m3u8) will have an unambiguous set of parameters (1 video codec, 1 audio codec, subtitle delivery method, etc.)?
We are currently passing all possible options, which leads to ambiguity. Btw, transcoding jobs can be distinguished by these (strict) parameters.

[gnattu]

What if streams (main.m3u8) will have an unambiguous set of parameters (1 video codec, 1 audio codec, subtitle delivery method, etc.)?

The transcoding pipeline needs to perform some sort of fallback due to reasons like encoders being unavailable iirc so we always have ambiguity because the actual codec also depends on the server configuration inside and outside jellyfin.

[dmitrylyzo]

The transcoding pipeline needs to perform some sort of fallback due to reasons like encoders being unavailable iirc so we always have ambiguity because the actual codec also depends on the server configuration inside and outside jellyfin.

If we are not going to support changing the server configuration on the fly (during playback), we can "perform some sort of fallback due to reasons like encoders being unavailable" in the master.m3u8 endpoint and generate streams with concrete set of parameters.

Moreover, we can (should) probably account for "encoders being unavailable" in the PlaybackInfo endpoint.

[dmitrylyzo]

In addition to Session promise refactoring:

#5906 (comment)
here and below

jellyfin-web/src/components/playback/playbackmanager.js

Lines 420 to 422 in e4c20df

	return Promise.resolve({
	MediaSources: [item.PresetMediaSource]
	});

[github-actions]

ESLint doesn't pass. Please fix all ESLint issues.

[dmitrylyzo]

I tried changing subtitles during playback (SRT -> ASS (burn) -> SRT). On the second change, it disables subtitles. Burn when transcoding is disabled.

It requests a session too early because PlaybackManager sets the subtitles right away.

jellyfin-web/src/components/playback/playbackmanager.js

Line 1552 in 23ee5e6

player.setSubtitleStreamIndex(selectedTrackElementIndex);

changeStream (video element/hls.js) doesn't have enough time to update the session on the server.

We could delay changing subtitles in the player and call setSubtitleStreamIndex in onStartedAndNavigatedToOsd (it's already there).

[gnattu]

Burn when transcoding is disabled.

Then nothing should be requested, why it behaves differently?

[gnattu]

I really cannot reproduce this behavior with current implementation. Nothing is requested when you turned that option off so even if you set it right away it should not disable the srt subtitle.

[dmitrylyzo]

I really cannot reproduce this behavior with current implementation. Nothing is requested when you turned that option off so even if you set it right away it should not disable the srt subtitle.

At first I tested with this feature enabled, and the session was requested before the player changed the video.
I then disabled it, but the issue (disabling subtitles) remained. Perhaps it is not related.

There may be something wrong with this code and #subtitleTrackIndexToSetOnPlaying becomes -1:

jellyfin-web/src/plugins/htmlVideoPlayer/plugin.js

Lines 486 to 499 in 23ee5e6

	this.#subtitleTrackIndexToSetOnPlaying = options.mediaSource.DefaultSubtitleStreamIndex == null ? -1 : options.mediaSource.DefaultSubtitleStreamIndex;
	if (this.#subtitleTrackIndexToSetOnPlaying != null && this.#subtitleTrackIndexToSetOnPlaying >= 0) {
	const initialSubtitleStream = options.mediaSource.MediaStreams[this.#subtitleTrackIndexToSetOnPlaying];
	if (!initialSubtitleStream || initialSubtitleStream.DeliveryMethod === 'Encode') {
	this.#subtitleTrackIndexToSetOnPlaying = -1;
	secondaryTrackValid = false;
	}
	// secondary track should not be shown if primary track is no longer a valid pair
	if (initialSubtitleStream && !playbackManager.trackHasSecondarySubtitleSupport(initialSubtitleStream, this)) {
	secondaryTrackValid = false;
	}
	} else {
	secondaryTrackValid = false;
	}

[dmitrylyzo]

Then nothing should be requested, why it behaves differently?

Btw, we hide it, so it can be enabled but hidden.

[gnattu]

Btw, we hide it, so it can be enabled but hidden.

No, if you select the subtitle again you can show it. You disabled that option, nothing will be hidden

[gnattu]

I can confirm what you found is not related to this change. You can even reproduce the index reverted to -1 behavior on current master.

It still looks like some kind of racing issue, but not related to session. The subtitle is loaded on current video stream and then the video stream is reloaded because transcode -> remux change, and then the index is reset to -1 during this reload.

[dmitrylyzo]

No, if you select the subtitle again you can show it. You disabled that option, nothing will be hidden

I mean that alwaysBurnInSubtitleWhenTranscoding remains enabled (but hidden) when you change burn-in option.

[gnattu]

I mean that alwaysBurnInSubtitleWhenTranscoding remains enabled (but hidden) when you change burn-in option.

WTF, that is so wrong. We need to always show that.

[gnattu]

Wait, it does not hide? the one becomes hidden is the PGS one not the burn in one

[dmitrylyzo]

WTF, that is so wrong. We need to always show that.

Oh, sorry - false alarm. I looked at the PGS one. 😅

[dmitrylyzo]

Technically, arrow function saves the original this by itself.
But using a helper variable with a meaningful name is clearer. 🤷‍♂️

[jellyfin-bot]

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

[sonarcloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[jellyfin-bot]

Cloudflare Pages deployment

Latest commit	da4265eb4685363fbfc07cdc3f20fb525da2cba8
Status	✅ Deployed!
Preview URL	https://b87b54e3.jellyfin-web.pages.dev
Type	🔀 Preview

stale