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.

Sign up for GitHub

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

Jump to bottom

Fix subtitle sync #507

Merged
merged 7 commits into from
Oct 26, 2019
Merged

Fix subtitle sync #507

merged 7 commits into from
Oct 26, 2019

Conversation

redSpoutnik
Copy link
Contributor

Quick PR to fix two problems I've experienced since 10.4.0 upgrade:

  • Moving subtitle-sync slider doesn't apply offset to vtt subtitles.
  • After closing or fading, subtitle-sync doesn't recover previous subtitle offset.

Changes

  • Update code to recover subtitle-track.
  • The wrong function was tested in "getPlayerSubtitleOffset" in playbackmanager.js (test if actual player support subtitle-sync): it always returned false and subtitleOffset was interpreted as 0.

Issues

@redSpoutnik
Copy link
Contributor Author

I've made changes to fix .ass subtitles sync as well.
I pusshed it here as it is not merged yet, sorry for the multiple commits.
It should be good now.

@dkanada
Copy link
Member

dkanada commented Oct 13, 2019

Is there an easy way to add an option in the user settings to revert to the old behavior? I saw a comment that equated async subtitles to async audio tracks, which obviously isn't often desirable. Sometimes you might want the subtitles to load completely before starting a video to not miss any content.

@redSpoutnik
Copy link
Contributor Author

@dkanada Sorry, but I didn't understand what you meant by revert to old behavior.
Do you mean to disallow the subtitle-sync in player-parameters menu?

@dkanada
Copy link
Member

dkanada commented Oct 13, 2019

My bad, I was thinking of this pull request but that wasn't written by you. Nevermind!

JustAMan
JustAMan approved these changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@JustAMan JustAMan left a comment

Choose a reason for hiding this comment

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

I hope you tested that for no-subs, .vtt subs and .ass subs :)

@redSpoutnik
Copy link
Contributor Author

@JustAMan Yes, on Brave 0.69.135 and Chrome 77.0.3865.120.
I've just tested on Firefox 69.0.1 and there is still .vtt sync not working. It seems this is not finished yet...

But maybe it could be merged as a start? I will continue searching anyway.

@JustAMan
Copy link
Contributor

JustAMan commented Oct 14, 2019 via email

@JustAMan JustAMan mentioned this pull request Oct 15, 2019
Merged
@dkanada
Copy link
Member

dkanada commented Oct 25, 2019

@redSpoutnik are vtt subtitles currently working on master or are they broken? If they're broken on master then this isn't a regression and we can merge this for the next release.

dkanada
dkanada reviewed Oct 25, 2019
View reviewed changes
src/components/htmlvideoplayer/plugin.js Outdated Show resolved Hide resolved
src/components/htmlvideoplayer/plugin.js Outdated Show resolved Hide resolved
src/components/htmlvideoplayer/plugin.js Outdated Show resolved Hide resolved
redSpoutnik and others added 3 commits October 25, 2019 18:21
@redSpoutnik @dkanada
Update src/components/htmlvideoplayer/plugin.js
f6eedbb 
Co-Authored-By: dkanada <dkanada@users.noreply.github.com>
@redSpoutnik @dkanada
Update src/components/htmlvideoplayer/plugin.js
608a3e3 
Co-Authored-By: dkanada <dkanada@users.noreply.github.com>
@redSpoutnik @dkanada
Update src/components/htmlvideoplayer/plugin.js
c30e6b5 
Co-Authored-By: dkanada <dkanada@users.noreply.github.com>
@redSpoutnik
Copy link
Contributor Author

@dkanada neither ass nor vtt subtitles are broken with this PR.
.vtt and .ass subtitle sync works fine on chrome and brave, only .ass sync works currently on firefox.

@JustAMan
Copy link
Contributor

@redSpoutnik
Bug does vtt sync work on Firefox without this PR?

@redSpoutnik
Copy link
Contributor Author

@dkanada no it doesn't (broken since 10.4.0).

@dkanada dkanada added the stable backport Backport into the next stable release label Oct 26, 2019
@joshuaboniface joshuaboniface merged commit ee11496 into jellyfin:master Oct 26, 2019
joshuaboniface added a commit that referenced this pull request Oct 26, 2019
@joshuaboniface
Merge pull request #507 from redSpoutnik/fix-subtitle-sync
78a2ba7 
Fix subtitle sync

(cherry picked from commit ee11496)
Signed-off-by: Joshua Boniface <joshua@boniface.me>
@joshuaboniface
Copy link
Member

Cherry-picked into 10.4.1.

@joshuaboniface joshuaboniface removed the stable backport Backport into the next stable release label Oct 26, 2019
@JustAMan JustAMan mentioned this pull request Nov 21, 2019
Closed
@redSpoutnik redSpoutnik mentioned this pull request Dec 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cvium cvium cvium left review comments

@JustAMan JustAMan JustAMan approved these changes

@joshuaboniface joshuaboniface joshuaboniface approved these changes

@dkanada dkanada dkanada approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@redSpoutnik @dkanada @JustAMan @joshuaboniface @cvium