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 the subtitle stream index calculation for multiple subtitles #1219

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

cebrusfs
Copy link
Contributor

Fix #1207

This patch make the subtitles map to a different indexes rather than the same index while doing the index recalculation in adjustExternalSubtitleIndexes.The original code will let us have only one subtile available in menu.

Minor, rename variable to copy to alight the code in the below (in adjustAudioForExternalSubtitles)

@cebrusfs
Copy link
Contributor Author

cebrusfs commented Aug 31, 2024

It works on my local device (iPad). But would be nice to have some reviewer that understand what those indexes calculation is doing.

@LePips
Copy link
Member

LePips commented Sep 2, 2024

Thanks for taking a look at this, I can't remember exactly why we had to perform the adjustments (hence the TODO comments) and where I referenced the original implementation. Would probably be best to cross reference with other clients.

@cebrusfs
Copy link
Contributor Author

cebrusfs commented Sep 2, 2024

I see. I am new to here. Do you have any recommendation about who to contact? or where to find them? It would be nice if you can help to connect.

BTW, if you don't recall that, should we consider to just somehow test it and then merge it to Testflight to see if it cause any regression?

@cebrusfs
Copy link
Contributor Author

cebrusfs commented Sep 3, 2024

I got comment from discord. Thank people in Discord!

to support STRM we started listing external streams before internal (because external are known at the time of scanning, internal are not). jellyfin/jellyfin#7529
this is because strm are only probed on playback start to not pull the file on each scan

And some type reference from SDK.

Stream types: https://github.com/jellyfin/jellyfin-sdk-swift/blob/a0e848a7aaec55991610818de6128b15cfcec725/Sources/Entities/MediaStreamType.swift

I might have no time to have closer look during weekday. It seems I will need to cross-check all current implementation of indexes. @LePips feel free to take over if that make you recall what you were doing here.

Copy link
Member

@LePips LePips left a comment

Choose a reason for hiding this comment

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

I have now started to take a look at this in my work for #1203 and, at least statically without testing here, looks good to me and will moreso keep as reference and fix if needed in my work.

Thank you for reaching out and getting an explanation. After seeing the server PR, I think I remember better now.

@LePips LePips merged commit 9dea386 into jellyfin:main Sep 5, 2024
4 checks passed
@LePips LePips added the bug Something isn't working label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subtitles missing and duplicating in subtitle selection
2 participants