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 support for SSA OutlineColour (only background) #10150

Merged
merged 2 commits into from May 9, 2022

Conversation

egor-n
Copy link
Contributor

@egor-n egor-n commented Apr 3, 2022

#8435

OutlineColour should be treated as the background color if BorderStyle=3.

Since currently BorderStyle is ignored, we can always treat OutlineColor as the background color. EDIT: with the suggestions from @icbaker, the code no longer ignores BorderStyle. Instead, the background color is applied only if BorderStyle=3.

@egor-n egor-n changed the base branch from release-v2 to dev-v2 April 3, 2022 08:38
OutlineColour should be treated as the background color if BorderStyle=3. Since currently BorderStyle is ignored, we can always treat OutlineColor as the background color.
@egor-n egor-n force-pushed the dev-v2-8435-outlinecolour branch from bf7ce23 to 388b8d1 Compare April 3, 2022 08:39
@icbaker icbaker self-requested a review April 4, 2022 08:47
@icbaker icbaker self-assigned this Apr 4, 2022
@icbaker
Copy link
Collaborator

icbaker commented Apr 4, 2022

OutlineColour should be treated as the background color if BorderStyle=3.

Can you please link to a citation/spec for this? The specs I'm looking at suggest there's a BackColour field which I'd expect would be used for the background.

I'm looking at:

Since currently BorderStyle is ignored, we can always treat OutlineColor as the background color.

I'm not sure I agree with this reasoning. The current implementation ignores both - but if we start considering only one, when the other has such a huge impact on how it's interpreted, I think the result could be very surprising. People would expect to see an outline + dropshadow and instead they'd see a big solid-coloured box.

As an extreme equivalent: What if we handled ScaleX but not ScaleY? The result would be that people expecting to grow/shrink their subtitles equally in both directions would see them change width but not height - which is probably more surprising than them not changing size at all!

It's probably not that hard to extend this PR to read BorderStyle too, and check it's set to 3? You can continue to ignore it (and this field) if it's set to 1 (i.e. you don't need to also implement dropshadow and outline in this PR as well).

@egor-n
Copy link
Contributor Author

egor-n commented Apr 4, 2022

@icbaker thank you for your input, it makes perfect sense to check that BorderStyle is set to 3 before applying the background color. I will update the PR right away 👍

OutlineColour should be treated as the background color if BorderStyle=3

I can't find a good reference for this claim, but it's what I understood from looking at how libass behaves.
https://fileformats.fandom.com/wiki/SubStation_Alpha#Data_types only mentions that BackColour is actually the color of the shadow.

@icbaker
Copy link
Collaborator

icbaker commented Apr 25, 2022

Thanks, looks good - I'll work on getting this merged (sorry for the delay at turning this round).

@icbaker icbaker merged commit a8c0a1b into google:dev-v2 May 9, 2022
@egor-n egor-n deleted the dev-v2-8435-outlinecolour branch May 9, 2022 13:11
marcbaechinger pushed a commit that referenced this pull request Jun 16, 2022
@google google locked and limited conversation to collaborators Jul 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants