Add secondary subtitle support to html video player

[is343]

This PR adds the ability to view multiple subtitle tracks via the html video player as requested in this feature request. I have done this by allowing the user to select a secondary track when a primary subtitle track is already playing. Depending on browser support, it will play natively or use the custom subtitle renderer. The selected secondary track index is saved in the player instance's state, so the selected secondary subtitle will be remembered for the current play session. This is convenient for watching several episodes of a show, for example. The secondary track index is not saved anywhere else, so it will need to be set again when coming back for a new session. Subtitle offsets will also apply to both tracks. I also applied a fix for a firefox bug where the current visible track would not clear when setting an offset.

Since we are only handling the secondary subtitles on the client-side, only External subtitles are currently supported. Anything burned in or done on the server-side will require changes there before we can make them work.

I was originally planning on allowing the user to select any number of extra subtitles by handling arrays of indexes, but that seemed like a lot more work to build in such a way that would not break current compatibility with existing methods or plugins. Also, I don't see the number of users wanting more than 2 subtitles to be that many.

Changes

• ~~fix issue with opening multiple actionsheets~~ #3900
  • ~~required since I added a new nested actionsheet~~
• add actionsheet for selecting secondary subtitles
  • All of the other actionsheets that are opened within another actionsheet do not have titles
  • I added a title to the secondary subtitle actionsheet because the contents of this actionsheet are likely to be the same as the one we just came from (main subtitle actionsheet). It might be easy for the user to not notice that they are on a new menu. So, I added the title to this one to let the user know what they are looking at.
• added new playbackmanager methods for handling secondary subtitles, getters, setters
  • resetting secondary subtitle state in setSubtitleStreamIndex when primary subtitle is cleared
• added new methods and updated logic to the main html player plugin
  • added logic to make sure we know which subtitle to manage (create, update, destroy) by adding a new argument for the target track index
  • updated logic for subtitle offsets. There was an issue when passing the offset to the second track. It was not getting the correct relative offset. Also no longer looping through all cues if the calculated offset === 0
  • added a workaround to fixed a bug in firefox where the currently visible text track would not clear when offset was changed
  • add logic to fetch the currently selected secondary track and start playing it
• added a new css class for the custom rendered secondary subtitles.
  • using !important because the min-height is set to over 4em directly to the element's styles. This makes the spacing between the subtitles too big. Also for used in margin-bottom just to give a little bit of space between the two, while still being able to use margin: auto.

Issues

multi-language-subtitles feature request

Sorry for the bad quality gifs. I had to keep them under 10mb to add them to this PR.

Chrome - native subtitles

Chrome - custom renderer - tested by setting requiresCustomSubtitlesElement return true. (this was before I updated the spacing in the custom renderer css)

(with current updated spacing for custom rendered subtitles)

Firefox - native

Firefox - (before fix) showing the bug where the current visible track does not clear when changing the offset. You can see the currently visible text (music notes) remains rendered until it reaches the point where it would be rendered again with the new offset timing

Firefox - (after fix) no longer showing the the bug from above

[is343]

I think we should only allow a combination of text-based subtitles. At the moment, ASS+ASS obviously doesn't work - it requires a second instance of JSO, but makes no sense since they will overlap. SRT+ASS or ASS+SRT are possible, but sometimes ASS is turned off when changing the subtitles. But again, this will look weird.

I definitely agree that any embedded subtitles should not be used with text based ones. Right now I'm getting the valid secondary subtitle list is generated with secondarySubtitleTracks in playbackmanager.js by filtering for only External ones.

This was me covering the problem for secondary subtitles, but for some reason I completely forgot about the scenario of if the primary ones are also embedded. Thanks for pointing this out @dmitrylyzo.

So, in this case I think I can also make a check to only allow the secondary subtitle menu if the current subtitle is also External. I would also probably need a check when the user resumes playback to ensure that the primary is sub is still a valid pair for a secondary.

[dmitrylyzo]

I definitely agree that any embedded subtitles should not be used with text based ones. Right now I'm getting the valid secondary subtitle list is generated with secondarySubtitleTracks in playbackmanager.js by filtering for only External ones. ...

SSA/ASS can be external. In fact, we currently only support external ones and render them using JavascriptSubtitlesOctopus. I think you could filter secondary subtitles by Codec in addition to DeliveryMethod.

JSO is currently destroyed when ASS+SRT:

1. Prepare video with SSA/ASS and SRT subtitles (both external).
2. Select SSA/ASS as first.
3. Select SRT as second.
4. setCurrentTrackElement -> setTrackForDisplay -> destroyCustomTrack -> octopus is destroyed and ASS stops showing.

[is343]

SSA/ASS can be external. In fact, we currently only support external ones and render them using JavascriptSubtitlesOctopus. I think you could filter secondary subtitles by Codec in addition to DeliveryMethod. ...

Ohhh okay. Sorry that initially flew over my head. I completely did not think of the complexities of rendering SSA/ASS subtitles and also the risk of subtitle overlap.

I understand what you mean now. Thank you for providing more context on that. That was a really great callout.

I refactored the method logic for checking for both valid secondary subtitle and primary subtitle pair. It's now checking to make sure the codec format is not SSA, not ASS, and that the delivery method is External

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.4% Duplication

[is343]

I was away for awhile, so I was not able to respond to this feedback in a timely manner.

A real great thanks to @dmitrylyzo for really providing some excellent feedback and helping find bugs that I had not considered! I really truly do appreciate having someone wayyy more knowledgeable with the codebase and project taking a look and helping me improve my code!

@thornbill, @dmitrylyzo made a good comment about me creating several secondary subtitle helper functions/methods. Should we keep everything separate like how I currently have it? Or, should I be putting this logic directly in the existing methods?

My thinking was that I did not want to clutter the existing code, and I also wanted to keep the logic separate and tidy with their own related function names.

[is343]

I can still combine SRT as secondary and ASS as primary.

1. Select SRT as primary.
2. Select SRT as secondary.
3. Select ASS as primary.

Shouldn't we disable the secondary if non-eligible primary is selected?

Yes, it should have been removing the secondary one when you changed back to ASS for the primary. I added a check in setSubtitleStreamIndex that fixes this.

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

[is343]

@thornbill Please let me know what you think of this whenever you get a chance to check it out again. The code is much better thanks to all the great feedback from @dmitrylyzo. I would love to get your eyes on it once more too.

[is343]

@thornbill any idea when this might get a final look through? I'd love to get this feature added.

[is343]

@thornbill @dmitrylyzo Happy new year to you both. I wanted to check in to see if I could get a final review on this.

[thornbill]

Hey @is343 thanks for your patience. I was out for a few weeks due to illness and holidays, but I'm slowly starting to make my way through the backlog of open PRs now. The bigger PRs can take a bit of time to get through, but I'll try to look over this sometime in the next week.

[is343]

Thanks for the update @thornbill. I'm sorry to hear that you were ill. I hope you are now doing well. I did not mean to sound like I was rushing you. I hope it didn't come off that way, just wanted to see if there were any updates. It's a relief to know that it's at least in the pipeline down the line!

[is343]

I only have one minor comment regarding logging really. I did some quick testing and it seems to work as expected. 🎉 Unless @dmitrylyzo has some additional feedback, I'd like to get this merged so people can start testing it in unstable.

Thanks for checking it out! I went ahead and committed your logging suggestion and rebased. Hopefully it should be good to go!

[is343]

EDIT: fix and explanation here

~~@dmitrylyzo @thornbill Just a heads up to you both, I noticed that direct play now seems to be broken in this branch. The break seems to be happening in the html player plugin file. Reverting only that file to the current master seems to fix it. I'm going through commits and diffs to see what has changed in master that broke this.~~

~~I'll update once I figure it out.~~

[is343]

I have removed the work to fix the stuck track cues for dev firefox builds into a separate draft PR #4349. This work is out of the scope of this PR. I also fixed the previous direct playback regression bug that was found after the last rebase with master.

Hopefully we can move this into unstable so we can get more people testing this 🤞.

@thornbill @dmitrylyzo, please take another look to make sure it is good to go. Thank you both for really helping me improve the quality of this PR with your reviews. And thank you both for your patience.

[is343]

I also have a general question. I noticed as I was working on the last commit that the getPlayerData method prepares an object for state, but we never actually use it. We instead save everything to the player directly.

I tried changing it to return state; instead, and everything seemed to continue to work fine. I don't know if there are any serious consequences to keeping the state in the player compared to an separate state object, but as it is now, we don't use the playerStates for anything at the moment. It looks like it's been this way for several years.

https://github.com/jellyfin/jellyfin-web/blob/85fac85968a877679ecde866358172a824e3ab62/src/components/playback/playbackmanager.js#L1919

[dmitrylyzo]

I also have a general question. I noticed as I was working on the last commit that the getPlayerData method prepares an object for state, but we never actually use it. We instead save everything to the player directly.

I tried changing it to return state; instead, and everything seemed to continue to work fine. I don't know if there are any serious consequences to keeping the state in the player compared to an separate state object, but as it is now, we don't use the playerStates for anything at the moment. It looks like it's been this way for several years.

As you may have noticed, the current sources are like spaghetti, so the main reason they are left as is "I don't know if there are any serious consequences ...".

Ideally, PlaybackManager should mediate the playback process, and players are just wrappers for the media API. The mentioned data can be stored by the player and should be wrapped in getters so that the state is always in sync (currently, it may differ if you have made a mistake).

[is343]

And the second one: playerData.secondarySubtitleStreamIndex is still not reset if the secondary subtitle is wrong (bug above), but it is now unnoticeable. This line reverts it even if we call playbackManager.setSecondarySubtitleStreamIndex(-1, this); in htmlVideoPlayer.

Maybe we should leave the second bug for later, since I dunno how to fix it right now? 😅 This block is probably completely wrong.

I think if we also check if the "best track index" is a valid secondary track inside of rankStreamType, we might be able to reset it properly. We are already passing a isSecondarySubtitle flag, so we should be able to also check if it's valid. Let me try this and I will let you know

[dmitrylyzo]

I think if we also check if the "best track index" is a valid secondary track inside of rankStreamType, we might be able to reset it properly. We are already passing a isSecondarySubtitle flag, so we should be able to also check if it's valid. Let me try this and I will let you know

You may need to check if both subtitles are compatible for secondary (in autoSetNextTracks) and reset DefaultSecondarySubtitleStreamIndex if they are not. This will probably work (as a workaround), but the problem is that subtitle tracks are stored in playbackManager (playerData) and the player (#customTrackIndex and #customSecondaryTrackIndex) and should be always in sync. Storing and managing them in one place should eliminate the synchronization problem. But this is for the next PR :wink:

[is343]

You may need to check if both subtitles are compatible for secondary (in autoSetNextTracks) and reset DefaultSecondarySubtitleStreamIndex if they are not. This will probably work (as a workaround), but the problem is that subtitle tracks are stored in playbackManager (playerData) and the player (#customTrackIndex and #customSecondaryTrackIndex) and should be always in sync. Storing and managing them in one place should eliminate the synchronization problem. But this is for the next PR 😉

Yes I agree. The way we have to manage two states is not ideal. We will definitely need to followup on this. But for now, I think I have it working. It will now check the secondary track and only carry the best match index if it is a valid secondary track. When carrying over the values after we do the ranking, we will also ensure the primary track is valid before setting the secondary track as well.

[is343]

@thornbill Alright, we figured out the kinks. Please give this a final look when you get the chance. :raised_hands:

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

[WarlockFish]

why i cant see the selecting secondary subtitles button？

[is343]

why i cant see the selecting secondary subtitles button？

Hi, this feature is going to be included in the version 10.9 release.

Right now, the only way to test this out is to try one of the nightly/unstable builds.

If you do try to update to one of the 10.9 unstable builds, please be aware that you won't be able to downgrade back to the 10.8 builds.

[Soulxy1993]

@is343 Is this included in the version 10.9.1? I updated to the newest version but can't find the function.

[thornbill]

If you have a question, pick a method from https://jellyfin.org/contact to ask it... don't comment on old PRs.