Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix subtitle encoder if webvtt is requested #9669

Merged
merged 2 commits into from
Aug 10, 2023

Conversation

sleepycatcoding
Copy link
Contributor

Changes
Properly convert subtitles to webvtt if requested.

Issues
Fixes jellyfin/jellyfin-android#1028

@nielsvanvelzen
Copy link
Member

The app is requesting the wrong URL. The extension for WebVTT is .vtt and the app is requesting .webvtt. So IMO this is not a bug in the server.

@sleepycatcoding
Copy link
Contributor Author

Isn't then requesting .subrip also a bug in the app?

@sleepycatcoding
Copy link
Contributor Author

sleepycatcoding commented Apr 24, 2023

The app is requesting the wrong URL. The extension for WebVTT is .vtt and the app is requesting .webvtt. So IMO this is not a bug in the server.

This is a bug in the server, because the app uses the deliveryUrl from the server that is already wrong.

The URL is generated here:

info.Url = string.Format(
CultureInfo.InvariantCulture,
"{0}/Videos/{1}/{2}/Subtitles/{3}/{4}/Stream.{5}",
baseUrl,
ItemId,
MediaSourceId,
stream.Index.ToString(CultureInfo.InvariantCulture),
startPositionTicks.ToString(CultureInfo.InvariantCulture),
subtitleProfile.Format);

Later set as deliveryUrl here:

if (profile.DeliveryMethod == SubtitleDeliveryMethod.External)
{
stream.DeliveryUrl = profile.Url.TrimStart('-');
stream.IsExternalUrl = profile.IsExternalUrl;
}

The subtitleProfile.Format is webvtt. A better fix probably would be to use file extensions instead of format in the URL.

@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Jun 13, 2023
@jellyfin-bot
Copy link
Contributor

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

@jellyfin-bot jellyfin-bot removed the merge conflict Merge conflicts should be resolved before a merge label Jun 13, 2023
@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Aug 1, 2023
@jellyfin-bot
Copy link
Contributor

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

@jellyfin-bot jellyfin-bot removed the merge conflict Merge conflicts should be resolved before a merge label Aug 2, 2023
@aseering
Copy link

aseering commented Aug 5, 2023

I'm running into this as well. Is the patch in need of any tweaks or further changes? Happy to help.

@cvium cvium merged commit 710f591 into jellyfin:master Aug 10, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebVTT Subtitles are not rendered when using integrated player
6 participants