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 subtitle converter misinterpreting 0 valued endTimeTicks #629

Merged
merged 2 commits into from Jan 20, 2019

Conversation

Projects
2 participants
@cvium
Copy link
Member

commented Jan 20, 2019

Changes
Changed a nullable long to a non-nullable.

Problem was that endTimeTicks was always 0 for (at least) direct streams and then it filtered out any timestamps that were not before the endtime...
Issues
Fixes #254

@cvium

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2019

This PR should be merged with #598, BUT if #598 is postponed, we should cherry pick this line: https://github.com/jellyfin/jellyfin/pull/598/files#diff-fec17a889e496ad8426f3388eeabf537R201

The code page provider fix is high priority since .NET Core by default has a LOT FEWER code pages enabled than standard .NET.

@joshuaboniface joshuaboniface added this to In progress in 10.1.0 Release via automation Jan 20, 2019

@cvium cvium referenced this pull request Jan 20, 2019

Closed

SRT subtitles not working #254

{
long endTime = endTimeTicks.Value;
long endTime = endTimeTicks;

This comment has been minimized.

Copy link
@nvllsvm

nvllsvm Jan 20, 2019

Member

The endTime variable is redundant now that endTimeTicks is no longer long?. Please remove it.

This comment has been minimized.

Copy link
@cvium

cvium Jan 20, 2019

Author Member

Yes. I guess I wasn't being super critical when I submitted this PR lol.

It's done.

10.1.0 Release automation moved this from In progress to Needs review Jan 20, 2019

10.1.0 Release automation moved this from Needs review to Reviewer approved Jan 20, 2019

@nvllsvm

This comment has been minimized.

Copy link
Member

commented Jan 20, 2019

Nice fix, thank you!

@nvllsvm nvllsvm merged commit b4c170a into jellyfin:dev Jan 20, 2019

1 check passed

continuous-integration/drone/pr Build is passing
Details

10.1.0 Release automation moved this from Reviewer approved to Done Jan 20, 2019

@cvium cvium deleted the cvium:fix_subtitleencoder branch Jan 20, 2019

@joshuaboniface joshuaboniface referenced this pull request Jan 21, 2019

Merged

Release 10.1.0 #651

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.