Skip to content

Conversation

X2E4VXpZKv
Copy link

Previusly this flag was turned on with sub-ass-override >= force.
This commit makes it an independent option.

I personally prefer this flag off with sub-ass-override=force as some subs have per speaker colors implemented as separate styles, but it's useful to have an option even with sub-ass-override=scale to fix annoying / low contrast font colors.

Copy link

github-actions bot commented Jul 3, 2025

@kasper93
Copy link
Member

kasper93 commented Jul 3, 2025

Please fix lint errors, see logs for details on the issues.

@X2E4VXpZKv X2E4VXpZKv force-pushed the sub-ass-override-colors branch from 8a9cabb to 7f44c0c Compare July 4, 2025 05:09
Previusly this flag was turned on with sub-ass-override >= force.
This commit makes it an independent option.
@X2E4VXpZKv X2E4VXpZKv force-pushed the sub-ass-override-colors branch from 7f44c0c to 2e8758d Compare July 4, 2025 05:35
@@ -567,7 +567,6 @@ static void configure_ass(struct sd *sd, struct mp_osd_res *dim,
if (total_override) {
set_force_flags |= ASS_OVERRIDE_BIT_FONT_NAME
| ASS_OVERRIDE_BIT_FONT_SIZE_FIELDS
| ASS_OVERRIDE_BIT_COLORS
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this stay here? For full override?

Copy link
Author

Choose a reason for hiding this comment

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

Being able to except colors from sub-ass-override=force was my main point with this so no.

That said i do see how its a mess interface wise that it works independently of sub-ass-override levels i just didn't have a beter idea how to do it.

@X2E4VXpZKv
Copy link
Author

An interface idea that i have now that might be a better fit is to have a 3 option flag (default, yes, no). By default it follows sub-ass-overide levels but can be overriden with yes or no.

@kasper93
Copy link
Member

kasper93 commented Jul 5, 2025

I'm fine with those changes.

/cc @llyyr does this look ok to you?

@llyyr
Copy link
Contributor

llyyr commented Jul 5, 2025

I'd prefer if all of them were extracted to their own options, and the sub-ass-override option be deprecated. I had intended to do this for years now but was stopped because it would be a breaking change, but if we're doing breaking changes let's do it properly.

I like what this PR is doing, and I think it should do it for all the bits that can be toggled by sub-ass-override.

@kasper93
Copy link
Member

kasper93 commented Jul 5, 2025

If we want to go this way, we can have string list option to allow users list what bits are overridden.

@na-na-hi
Copy link
Contributor

na-na-hi commented Jul 5, 2025

I'd prefer if all of them were extracted to their own options, and the sub-ass-override option be deprecated. I had intended to do this for years now but was stopped because it would be a breaking change, but if we're doing breaking changes let's do it properly.

Something like --sub-ass-override=bits --sub-ass-override-bits=FONT_NAME,FONT_SIZE_FIELDS,BORDER,SCALE will work. This way there will be no compatibility break and one new option can handle all possible override bits.

@kasper93
Copy link
Member

kasper93 commented Jul 5, 2025

This is what I meant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants