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

Select appropriate default subtitles based on user preferences #566

Merged
merged 4 commits into from
May 23, 2022

Conversation

cthelight
Copy link
Contributor

Changes
Logic for selecting a default subtitle track when starting playback is now added. This logic is based off of the existing logic in the base Jellyfin repo, with 2 minor modifications:

  • Text-based subtitles are preferred (within the confines of the user's preferred subtitle mode)
    • If no text-based subtitles exist that match the preferred mode, then non-text-based subtitles are used anyway
    • This behavior was added sinc the roku video player is known to not support graphical subtitles, so using such subtitles would require transcoding in all cases
  • "Smart" mode subtitles behave the same as default subtitles (the fallback behavior in Jellyfin itself)
    • This is done, as the full implementation relies on knowledge about audio language preferences

Rationale
Generally I use "OnlyForced" subtitle mode for my playback, since it is mildly annoying to watch a movie with some foreign language parts that don't get subtitles (Ex Elysium, which is partially in Spanish). This works well on other clients, but based on the source here, we are currently assuming no-subtitles is the correct default option, which does not match other clients (web, android app, etc).

This patchset attempts to rectify this issue by porting over the as much of the logic from the other clients as possible.

Additionally, the reason text-based subtitles are explicitly preferred here, when that is not necessarily the behavior elsewhere, is that watching a 4k rip with forced subtitles is not very doable when the server is attempting to transcode the whole thing. 😄

@jimdogx
Copy link
Contributor

jimdogx commented Apr 18, 2022

I might be testing this wrong. I set my user settings to "Always" and "Spanish". When I play a movie on Roku, I don't get any subtitles. However, if I press "down", I do see that it has highlighted / selected "Spanish" as the subtitle of choice. It's just not actually showing them.

@cthelight
Copy link
Contributor Author

Hmm, you seem to be testing it appropriately.

Does that subtitle track work without this patch, if you select it manually after starting the movie (from the "down" menu)? Just trying to eliminate "easy" reasons for the issue. 😄

I just retested a bunch of movies with English subtitles (as that is what I have the most of) and it appears to work fine for both "Always" and "OnlyForced". I did test a couple with Spanish subtitles, and they seemed to work as well, though I have many fewer of those to test.

One thing to note is that the subtitle being selected in the "Down" menu means that the corresponding track index had to have passed to the server in the "ItemPostPlaybackInfo" call, as the index in the displayed subtitle list is computed from the track index supplied to the server. As such, we at least believe we have sent the correct index to the server.

One unrelated thought/question regarding this change that just came to mind: Would it be better to enforce the user's preferred language when selecting these subtitles (i.e. only allow displaying subtitles in the user's preferred language)? Right now we just find the "best" (based on the sorting and mode preferences) subtitle option regardless of language. I'm thinking so, but before I change anything...

Thanks for looking at this, BTW.

@jimdogx
Copy link
Contributor

jimdogx commented Apr 18, 2022

I still think it might be on my end. I tried the web client and it also doesn't "Always Play" what I have selected in settings.

I'm connecting to a beta 10.8 server. So either I'm doing something wrong or it's a server bug.

@cthelight
Copy link
Contributor Author

That sounds possible based on the differences between our experiences.

I'm still running 10.7.7, in case that is pertinent.

Note on the most recent push:
This update to the patchset adds a requirement that the language for the subtitle matches the user's preferred language. I realized after going back through the logic that that would be an issue in the "Always" case, if a title does not contain a subtitle of the preferred language.

@cthelight
Copy link
Contributor Author

Note on the most recent push:
Resolved an issue with text-based subtitles not becoming enabled in all cases, also rebased my branch on tips of master.

@jimdogx Were your subtitles text-based when you were testing this change? If so, would you mind retesting (when you have time, ofc)? I managed to reproduce an issue with text-based subtitles on my end, which should be resolved in this latest change.

@lakerssuperman
Copy link

Greetings!! If I read the comments correctly, this commit will allow the Roku client to correctly show forced subtitle tracks for things like foreign language parts of films automatically if the server is set to do so? This would be very exciting if that's the case!

@cthelight
Copy link
Contributor Author

@lakerssuperman That's the goal.

It should support the other settings as well, but forced subtitles was the main driving force behind my wanting to implement it.

@lakerssuperman
Copy link

@lakerssuperman That's the goal.

It should support the other settings as well, but forced subtitles was the main driving force behind my wanting to implement it.

That's awesome!

Thank you!!!

@neilsb
Copy link
Member

neilsb commented May 15, 2022

I did a pile of (very overdue - sorry) testing today and it all appeared to work as described.

There are another couple of tests I want to do, which I'll try and complete early this week.

But this is looking great. Thanks.

else
' If this is a text-based subtitle, set relevant settings for roku captions
video.globalCaptionMode = "On"
video.subtitleTrack = selectedSubtitle.TrackName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
video.subtitleTrack = selectedSubtitle.TrackName
video.subtitleTrack = selectedSubtitle.Track.TrackName

I think the TrackName property is within a Track property on the subtitle

@neilsb
Copy link
Member

neilsb commented May 20, 2022

So done some more testing specifically on the issue @jimdogx had mentioned previously, and I observer the same behaviour. It seems that the PR is setting the correct mode of subtitles (whether to display or not) based on the preferences, but it's always the default (or first) subtitle track that is used, at least with text subtitles.

What I'm seeing is that defaultSubtitleTrack() is identifying the correct track to use and setting the video.subtitleTrack property (although, I've added a comment that I think the TrackName is within a Track property).

However, when the video starts playing, the video.subtitleTrack is empty. To see this, add a line to print the m.top.subTitleTrack in the ReportPlayback() function in JFVideo.brs and you'll see it output on play start or pause, etc. If you change the subtitle while the media is playing, you'll see it output here too.

So I think when you're setting video.subtitleTrack it's just not the right object that is being set.

I can provide a sample mkv file with a pile of SRT subtitle tracks if that's of any use. In my case, Eng is the 1st subtitle track, and that all works fine. But when I set my user preference to use Ita as the preferred language, your code identifies the correct URL to use, but since the appropriate property is not set correct, it's still the 1st subtitle track that Roku uses (eng).

@cthelight
Copy link
Contributor Author

@neilsb Thanks for catching that issue, I had not noticed that behavior, as I only have English text-based subtitles.

I believe I have resolved the issue in the latest patch (in addition to rebasing on tips of master), at least based on m.top.subtitleTrack being set in ReportPlayback(). It seems that somewhere in the process of resetting video.content.url the subtitleTrack setting gets reset, thus erasing the track I was setting. As such, you can see I have now moved the subtitle setting logic down to after the last place we set the video.content.url.

@neilsb
Copy link
Member

neilsb commented May 22, 2022

@cthelight Something odd is certainly going on. The change you've made does indeed now set the correct subtitle track URL in the property, but it's still the 1st subtitle track that plays. If you then choose a different track then go back to the one that should have been played, then the same URL is set back into that property and then correct subtitle track is used. Very odd.

If you want to test it, here is the sample video I was using (~30 Mb). I set my preferred language as "Italian", but English was displayed (even though the URL for the Italian was being set correctly). If, during playback, I switch to "Hungarian" then back to "Italian" then the same URL is set again, but the correct track is then used.

On other players (web/andriod app) the user perferences for
subtitle behavior are taken into account, and used to make an
assumption about subtitle behavior.

This patch ports most of that logic here. "Smart" selection is not
yet fully-featured, as it requires additional knowledge about audio
language preferences. Rather it uses the fallback mechanism, which
emulates the "Default" subtitle option.

The logic for the different options was based on the main jellyfin
repo (specifically sha 49d5fdb33fc9792147c1df03e1d1b051e6b7ec79 in
file Emby.Server.Implementations/Library/MediaStreamSelector.cs)

Additionally, this implementation specifically prefers text-based
subtitles (assuming they match the user's preference) as they are
the only ones natively supported by Roku.

Also, the subtitle changing mechanism is reworked slightly to make
use of the new implementation herein
Currently, when populating subtitleTracks, we assume that the ordering
and list of populated subtitle tracks will not change when Roku moves
the list into availableSubtitleTracks. This causes an issue with some
videos as it is not always consistent.

This patch modifies the logic to no-longer inject assumed final indices
into our list of text-based subtitles, but instead search through the
availableSubtitleTracks array and locate the actual subtitle that
refers to the same URL as in our list. In this way we are guaranteed
to always tell Roku to play the subtitle we want, no matter how re-
ordered the options get.

NOTE: The URL gets mildly mangled in the process of copying from
subtitleTracks to availableSubtitleTracks, so we need so search via
substring, rather than doing a full string comparison.
@cthelight
Copy link
Contributor Author

@neilsb That mkv was very helpful in figuring this out (hopefully correctly this time 🙂).

The issue I was having, it turns out, is that when Roku moves the subtitle listing from video.content.subtitleTracks to video.availableSubtitleTracks it mangles the TrackName a bit, so it is no-longer just the URL of the track. That means that even though I was setting the subtitleTrack property to the appropriate URL, it was still being ignored for having the wrong prefix. This means I needed to copy the TrackName from within availableSubtitleTracks itself, rather than from our list of subtitles.

Additionally, I noticed another issue with the mkv you attached (which I could even reproduce on the current release). The problem is that when Roku copies the subtitle tracks, it ignores ones it cannot play (at least that is my assumption as to why). This results in the TextIndex property being out of sync with reality, as it was intended (I believe) to record which index in availableSubtitleTracks would refer to that subtitle track.

To ensure my new logic works, even in this case, in my 2nd patch, I updated the logic to now actually locate the correct subtitle in availableSubtitleTracks by looking for the URL, rather than assuming things about the indices. I wish I had a more efficient solution to the issue, but this one does appear to work (though I think I may have said that before 🙂).

Copy link
Member

@neilsb neilsb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and seems to work in all the scenario's I've tested with. Thanks!

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.

4 participants