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 bug lineheight was not correctly applied #1320

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

Florent-Bouisset
Copy link
Collaborator

The lineheight attribute for TTML subtitle was not correctly applied.

trimmedLineHeight is a string and trimmedLineHeight[0] is a single character,
Regex was never matching a single character and always returned null.

I guess the devs expected to acces first element of the array when doing trimmedLineHeight[0]
(as it is done in a similar way in `apply_padding.ts)

@peaBerberian peaBerberian added this to the 3.33.0 milestone Nov 23, 2023
@Florent-Bouisset Florent-Bouisset added bug This is an RxPlayer issue (unexpected result when comparing to the API) Subtitles Relative to subtitles TTML Relative to text tracks in the TTML format labels Dec 4, 2023
@peaBerberian
Copy link
Collaborator

Nice, I want to merge it as it is a real fix. Some of canal+ contents are poorly-packaged but we already have some ways of doing a special treatments for them.

Maybe in the longer term, a useful subtitle styling API which would overwrite some properties could be added. It does not look so simple to define though.

@Florent-Bouisset
Copy link
Collaborator Author

Nice, I want to merge it as it is a real fix. Some of canal+ contents are poorly-packaged but we already have some ways of doing a special treatments for them.

Maybe in the longer term, a useful subtitle styling API which would overwrite some properties could be added. It does not look so simple to define though.

I can create merge it and create an other PR specific for the branch canal where I set manually the lineHeight to default (around 120%)

@peaBerberian
Copy link
Collaborator

I think we'll just create a patch with the other work-arounds we currently set on specific releases:

  • if the second origin number (y position) is more than 100% or less than 0%, we put the cue at a fixed position centered at the bottom of the screen instead.

  • if the second extent number (height) is more than 100% or less than 0%, we set an automatic height and a width of 80% (subtitles are still usually centered)

  • For v3.x.x releases only*, if playback is stuck at a given position and our heuristic has a good reason to think that's because the browser is not relying on the decryption key we pushed (it's a PlayReady issue as you would imagine), we reload/

    *This patch is not needed in v4 because we already reload in the base v4. We cannot do this in base v3 because reloading is linked to a subtle API breakage of the playerStateChange event and wasn't present in v3.0.0

This would be yet another addition here.

For now those are just in two commits:

@peaBerberian peaBerberian merged commit 05e44de into master Dec 6, 2023
6 checks passed
@peaBerberian peaBerberian mentioned this pull request Jan 24, 2024
@peaBerberian peaBerberian deleted the bugfix-ttml-subtitle-lineheight branch February 7, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This is an RxPlayer issue (unexpected result when comparing to the API) Subtitles Relative to subtitles TTML Relative to text tracks in the TTML format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants