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 subtitle synchronization on HTML video player #220

Merged
merged 6 commits into from Apr 30, 2019

Conversation

redSpoutnik
Copy link
Contributor

@redSpoutnik redSpoutnik commented Mar 31, 2019

This PR add the ability to manually set an offset to some subtitles in HTML video player:

  • subtitle stream codec must be compatible with vtt (e.g. srt) or ass.
  • subtitle stream display mode must be external (subtitle extraction must be allowed).

Changes

  • If previous conditions are fullfilled, a slider shown between subtitle and parameters buttons.
  • Current offset range is -30 sec to +30 sec.
  • On selected subtitle changes, the offset is reset to 0.
  • This feature works only in UI and doesn't require backend call.

Tested on Linux Mint 18.2 with :

  • Brave Browser 0.61.51
  • Google Chrome 73.0.3683.75
  • Firefox Quantum 65.0.1

Issues

May partially solve jellyfin/jellyfin#130.

@redSpoutnik
Copy link
Contributor Author

null-offset
negative-offset
positive-offset

@cvium
Copy link
Member

cvium commented Mar 31, 2019

I'd prefer if it wasn't a slider and it should be inside the options menu tbh. The current placement is a bit crowded

@JustAMan
Copy link
Contributor

JustAMan commented Apr 1, 2019

I would say it should be inside CC menu which appears when one clicks respective button.
Also having a field which could be filled manually (in addition to a slider) would be good.

@redSpoutnik redSpoutnik changed the title Add subtitle synchronization on HTML video player [WIP] Add subtitle synchronization on HTML video player Apr 1, 2019
@redSpoutnik
Copy link
Contributor Author

Thanks, I will take the time to test those changes and I will give you some feedback.

@dkanada
Copy link
Member

dkanada commented Apr 6, 2019

One quick note, I fixed the CI issue here in another PR so you won't have any issues in the future :) The backend changes look good, I agree about moving it out of the main bar though. I would say inside the settings button since that one is for general options. I think the subtitle and audio buttons should stay as track selection.

@redSpoutnik
Copy link
Contributor Author

Hi all,
I want to share with you my recent tests on this PR.

To avoid grouping slider and editable field in a tight menu item, I took inspiration from kodi subtitle offset.

If a subtitle track is selected, a localized "SubtitleOffset" option appears in the settings:
subtitleoffset-menu

This option display a panel with the slider and a text field:
subtitle-sync-overlay
subtitle-sync-overlay-background

The slider has the exact same behavior as before:
subtitle-sync-slider

The textfield is editable:
subtitle-sync-textfield

  • Slider and textfield values are synchronized.
  • On mouse inactivity the panel fades with bottom controls, except if the textfield is being edited.
  • If the panel is closed using the cross, it will not show up until the "SubtitleOffset" option is clicked again, but the subtitle offset remain.

What do you think?

@redSpoutnik redSpoutnik changed the title [WIP] Add subtitle synchronization on HTML video player Add subtitle synchronization on HTML video player Apr 13, 2019
@dkanada
Copy link
Member

dkanada commented Apr 29, 2019

@redSpoutnik there seem to be a few conflicts but I think this is ready to merge once those are fixed up.

cc @JustAMan @cvium for comments

@anthonylavado
Copy link
Member

Ready to put this in as soon as the conflicts are sorted.

@redSpoutnik
Copy link
Contributor Author

@dkanada @anthonylavado Conflicts merged :) but be aware that I let the "subtitle offset" button in settings, whereas the "subtitle settings" has been removed.
This PR doesn't include translations of the button's name btw.

@anthonylavado anthonylavado added this to To Do in Release 10.4.0 via automation Apr 30, 2019
Release 10.4.0 automation moved this from To Do to To Merge Apr 30, 2019
@anthonylavado
Copy link
Member

anthonylavado commented Apr 30, 2019

This PR doesn't include translations of the button's name btw.

We'll want to make an issue to track this!
Thanks for the great PR @redSpoutnik 👏

@anthonylavado anthonylavado merged commit d7a0a9b into jellyfin:master Apr 30, 2019
Release 10.4.0 automation moved this from To Merge to Done Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants