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

Add ability to remember audio and subtitle selection settings #3754

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ConnorS1110
Copy link
Contributor

Changes
Adds two new settings under advanced playback settings to remember the previously selected audio and subtitle track. If these settings are enabled, the player will find the audio and/or subtitle track that matches what was selected in the previous episode. If the audio/subtitle tracks don't align 1-to-1, it makes a best effort guess by looking through the display name and language value for the media streams and makes a selection.

Issues
Fixes my own frustrations with this discrepancy between the web and Android TV versions of the app

@ConnorS1110
Copy link
Contributor Author

This is like 99% done, but I have one issue I have spent days trying to solve I can't figure out and want to turn to others for help at this point. This successfully saves the user setting, and when you move to a new episode it will remember and correctly selects the tracks it should, but the initial playback just keeps looping endlessly. It starts out black, then the first frame appears, I get a toast notification saying it is burning in subtitles, and then the player just restarts. It's black for a second, the first frame appears, the toast appears, and the cycle keeps repeating. I have stuck break points all over the place stepping one step at a time and I cannot see anything that would be causing this loop. I am hoping someone with a better understanding of how playback works on android tv can see what I have changed and offer ideas what I need to change for this.

@nielsvanvelzen
Copy link
Member

I don't think we need preferences for this. Could just be default behavior.

@ConnorS1110
Copy link
Contributor Author

I will update that. Do you have any suggestions about the playback thing I mentioned in my comment

@nielsvanvelzen
Copy link
Member

No sorry, the old playback code is not my favorite place so I don't touch it when possible. It's full of these odd behaviors.

@phidix
Copy link

phidix commented Aug 11, 2024

Hi,
I can only say that I'm really interested in having this feature working in Android TV client for Jellyfin. Fingers crossed someone from the community will help address @ConnorS1110 issues described regarding "endless loop".

Thanks!

Comment on lines +775 to 780
if (currAudioIndex == audioIndexToUse || (prevOptionsAudioIndex != null && prevOptionsAudioIndex == audioIndexToUse)) {
Timber.d("skipping setting audio stream, already set to requested index %s", audioIndexToUse);
if ((currOptionsAudioIndex == null || currOptionsAudioIndex != audioIndexToUse) || (prevOptionsAudioIndex == null || prevOptionsAudioIndex != audioIndexToUse)) {
Timber.d("setting mCurrentOptions audio stream index from %s to %s", currOptionsAudioIndex, audioIndexToUse);
mCurrentOptions.setAudioStreamIndex(audioIndexToUse);
}
Copy link

@entropicdrifter entropicdrifter Aug 28, 2024

Choose a reason for hiding this comment

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

It looks like you wanted these two conditions to be mutually exclusive. Current behavior would only attempt to change the audio stream index when the skip condition is true. That's probably what's leading to your infinite loop.

Suggested change
if (currAudioIndex == audioIndexToUse || (prevOptionsAudioIndex != null && prevOptionsAudioIndex == audioIndexToUse)) {
Timber.d("skipping setting audio stream, already set to requested index %s", audioIndexToUse);
if ((currOptionsAudioIndex == null || currOptionsAudioIndex != audioIndexToUse) || (prevOptionsAudioIndex == null || prevOptionsAudioIndex != audioIndexToUse)) {
Timber.d("setting mCurrentOptions audio stream index from %s to %s", currOptionsAudioIndex, audioIndexToUse);
mCurrentOptions.setAudioStreamIndex(audioIndexToUse);
}
if (currAudioIndex == audioIndexToUse || (prevOptionsAudioIndex != null && prevOptionsAudioIndex == audioIndexToUse)) {
Timber.d("skipping setting audio stream, already set to requested index %s", audioIndexToUse);
} else if ((currOptionsAudioIndex == null || currOptionsAudioIndex != audioIndexToUse) || (prevOptionsAudioIndex == null || prevOptionsAudioIndex != audioIndexToUse)) {
Timber.d("setting mCurrentOptions audio stream index from %s to %s", currOptionsAudioIndex, audioIndexToUse);
mCurrentOptions.setAudioStreamIndex(audioIndexToUse);
}

Choose a reason for hiding this comment

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

@ConnorS1110 forgot to tag

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Conflicts prevent merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants