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

[subtitles] Align to screen image based subtitles when video source is cropped #21162

Merged
merged 1 commit into from Mar 25, 2022

Conversation

CastagnaIT
Copy link
Collaborator

@CastagnaIT CastagnaIT commented Mar 21, 2022

Description

This introduce a different behaviour for scaling/stretching for image based subtitles, that concern cropped video cases only.

Now if a video has been cropped and the subtitles have been created for non-cropped video,
subtitles will be aligned to the screen by keeping the aspect-ratio, instead of align to video that cause to show subtitles distorted/squashed.

Motivation and context

Time ago merged PR #20204 (with some revert with #20794) to fix issue #14866,
mainly i have changed the alignment from ALIGN_SCREEN to ALIGN_VIDEO.
This change allow to show the PGS/Vobsub correctly as happens on VLC/MPV players, respecting the correct positioning and appearance (even when the window is resized).

But (as happens on all other players) this cause a side effect with the videos that has been remuxed and cropped (removed black bars).

When a video is cropped have less height than the original source, if the subtitle images are scaled to the cropped video source (ALIGN_VIDEO) the subtitles will be displayed distorted/squashed, as i example in this issue #21141.

Then instead add another setting i have add a check to compare the subtitle size param to the video source, in this way we can understand if the video has been cropped, and align the subtitles to screen by keeping the aspect ratio ALIGN_SCREEN_AR.

This fix #21141 i have tested with the provided video and all my PGS/Vobsub/SUP with success

This partially re-add the code of #20204 to keep aspect ratio, but should not affect bluray menu, because use full resolution for menu. I have tested with a my bluray sample but it do not have a java menu and kodi do not handle it in good way, in any case i do not see regression for this case.

@thexai please can you try with your "Viuda negra" bluray that have java menu?

i will prepare an artifact to allow testing

How has this been tested?

From our FTP folder: samples_new/subtitles/PGS/

La.dolce.vita.1960.1080p.Criterion.Bluray.DTS.x264.mkv (cropped video with cropped subtitles resolution, subtitles must be shown inside video not in the black bars and aligned to the video)

PGS_StretchingTest.mkv (4/3 video and same thing for subtitles, subtitles must be shown with right aspect ratio and aligned to the video)

A Global Journey_cropped.mkv (this is a cropped video with full 1080p subtitle. With the first sub track : original english, subtitles must be appears with right aspect ratio, aligned to the screen)

Bohemian.Rhapsody.2018.4K.PGS.at.1080.mkv (this is a full 4K video resolution with full 1080p subtitle, subtitles must be appears aligned with the video)

What is the effect on users?

Have a better aspect ratio of subtitles when used with cropped videos,
and so allow to see subtitles with right aspect ratio on ultrawide monitor's

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@CastagnaIT CastagnaIT added Type: Improvement non-breaking change which improves existing functionality v20 Nexus Component: Subtitles labels Mar 21, 2022
@CastagnaIT CastagnaIT added this to the Nexus 20.0 Alpha 1 milestone Mar 21, 2022
@CastagnaIT CastagnaIT changed the title [subtitles] Stretch image based subtitles when video source is cropped [subtitles] Align to screen image based subtitles when video source is cropped Mar 21, 2022
@CastagnaIT
Copy link
Collaborator Author

CastagnaIT commented Mar 22, 2022

test builds:
--removed-- deprecated

@enen92
Copy link
Member

enen92 commented Mar 22, 2022

I've runtime tested this and seems to work just fine. I like the approach to avoid yet another setting. Bluray menus work fine too.

Using vobsub subs (while toggling different zoom modes):

Screenshot from 2022-03-22 09-42-51

Screenshot from 2022-03-22 09-43-12

Screenshot from 2022-03-22 09-43-30

Subtitles scale correctly depending on the "crop" percentage.

I am wondering if we could have the same for text based subtitles though. Adding an external subtitle to the same DVD and the behavior is different:

image

Screenshot from 2022-03-22 09-40-00

Screenshot from 2022-03-22 09-40-46

Copy link
Member

@enen92 enen92 left a comment

Choose a reason for hiding this comment

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

Nice improvement! As long as the BR menus don't break (and @thexai reports a +1) I am fine with this.
Wondering if we couldn't have similar behavior for text subs.

@thexai
Copy link
Member

thexai commented Mar 22, 2022

Sorry to disagree but this PR changes everything again and introduces a lot of side effects on original 4K sources (not cropped):

current master:
ma-Captura de pantalla 2022-03-22 110346

ma-Captura de pantalla 2022-03-22 105654

PR:
Captura de pantalla 2022-03-22 110836

Captura de pantalla 2022-03-22 111644

PR stretch all subtitles and UHD overlays on 4K sources (not cropped)

Copy link
Member

@thexai thexai left a comment

Choose a reason for hiding this comment

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

This introduce a different behaviour for scaling/stretching for image based subtitles, that concern cropped video cases only.

I don't approve of it because it doesn't do what it's supposed to do.
That is, with original video sources (not cropped) it should not change anything.

Probably the only solution is a new setting to enable this optionally (default disabled). I'm not asking for it, for me the master branch is fine as it is now.

@thexai
Copy link
Member

thexai commented Mar 22, 2022

Now if a video has been cropped and the subtitles have been created for non-cropped video,

This is a "bad source"

@CastagnaIT
Copy link
Collaborator Author

CastagnaIT commented Mar 22, 2022

it is not a bad source, often black bars are cropped from video dvd/bd rip
i will try to see a way to exclude bluray from this behaviour

oh i was tried overscan but not zoom, i will check for text based subs separately

@thexai
Copy link
Member

thexai commented Mar 22, 2022

i will try to see a way to exclude bluray from this behaviour

It's not about "excluding" DVD / Blu-Ray / UHD Blu-Ray / MKV untouched remuxes...

It is about adding a "special corner case" cropped video + not cropped PGS subs without break all other things.

Copy link
Member

@thexai thexai left a comment

Choose a reason for hiding this comment

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

Tested and confirmed that now is working fine with 1080p and 4K untouched (AR = 16/9) video sources.

@CastagnaIT
Copy link
Collaborator Author

please wait to merge, i want test a bit more

@CastagnaIT
Copy link
Collaborator Author

ok i have done,
i have add a condition check also for the width, no change behaviour,
but this fix cases with 4/3 video resolution + 1080p subtitles

also done a couple of test with DVB subtitles samples all ok

@thexai thexai merged commit 3048780 into xbmc:master Mar 25, 2022
@MediaBrasil
Copy link

MediaBrasil commented Mar 27, 2022

all fine thanks

@sugatam
Copy link

sugatam commented Feb 23, 2023

For cropped video where verticalshift is introduced, PGS subtitles don't honor the shift. I routinely shift cropped 1080p videos by -0.75 in my projector setup that has bottom masking only, but this causes the subtitles to drop down into the black bars.

@CastagnaIT
Copy link
Collaborator Author

on kodi there are no settings for image based subtitles, then you cant move PGS subtitles
the vertical shift that you use on video settings, is for video stream purpose only,
so what you want is to have a new feature to add vertical shift setting for text and/or image based subtitles

you can request on the Kodi forum on feature request section,
and please dont write for bugs or other type of requests on development PR's

@sugatam
Copy link

sugatam commented Feb 23, 2023

Sorry about that Stefano. TBC, when I'm shifting the video via supported methods, I expect overlays to shift with it, so I consider this a bug (it does the right thing when shifting uncropped video). But sorry about posting on this PR, I'll use the right channel (or try submitting a PR myself).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Subtitles Type: Improvement non-breaking change which improves existing functionality v20 Nexus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image based subtitles in Nexus
5 participants