Skip to content

sub/sd_ass: reuse PlayResX instead of hardcoding to 384#17846

Open
llyyr wants to merge 2 commits into
mpv-player:masterfrom
llyyr:fix/hard-coding-playresx
Open

sub/sd_ass: reuse PlayResX instead of hardcoding to 384#17846
llyyr wants to merge 2 commits into
mpv-player:masterfrom
llyyr:fix/hard-coding-playresx

Conversation

@llyyr

@llyyr llyyr commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Hardcoding 384 here isn't correct, we should be using the original PlayResX after all.

The problem commit 1723f8a was trying to fix is instead fixed by rounding PlayResX instead of implicitly truncating it. Before 1723f8a, PlayResX was calculated using doubles and implicitly truncated to int, causing off by one errors (e.g. 511 vs 512) due to truncation errors.

Round to nearest integer instead to fix this.

Fixes: 1723f8a
Fixes: f862d3b

@llyyr llyyr mentioned this pull request Apr 29, 2026
Open
@github-actions

github-actions Bot commented Apr 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Download the artifacts for this pull request:

Windows
macOS

@kasper93

kasper93 commented Apr 29, 2026

Copy link
Copy Markdown
Member

Have you confirmed the fix to behave correct with affected ASS sample files?

@Sneakpeakcss

Sneakpeakcss commented Apr 30, 2026

Copy link
Copy Markdown

Have you confirmed the fix to behave correct with affected ASS sample files?

Only works for the mentioned show/hide:

There is no need to resize window, just rerendering by pressing v to hide and show the subtitle is enough case

Though i cannot say if that was ever the case for me, but this broke it on the resize again:

brokenagain.mp4

@llyyr

llyyr commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

I couldn't reproduce it myself, but can you check the latest push?

@llyyr llyyr force-pushed the fix/hard-coding-playresx branch from 68392fe to 294233b Compare April 30, 2026 01:44
@kasper93

kasper93 commented Apr 30, 2026

Copy link
Copy Markdown
Member

/cc @na-na-hi as they also were involved in those margin changes before.

@na-na-hi

na-na-hi commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

The issue reappears for me with this PR, both with and without rounding.

This is not caused by rounding. The first time old_playresx is calculated it is the initial MP_ASS_FONT_PLAYRESX value initialized in sd init. Subsequent ones use previously set track->PlayResY * (double)vidw / MPMAX(vidh, 1) which is different, so first render and subsequent renders are not consistent.

@llyyr

llyyr commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

so first render and subsequent renders are not consistent.

I noticed this and was thinking of how to go about fixing this. However, the resize issue should be caused by rounding which I could reproduce before and now I can't after rounding correctly

@Sneakpeakcss

Sneakpeakcss commented Apr 30, 2026

Copy link
Copy Markdown

I couldn't reproduce it myself, but can you check the latest push?

Unfortunately no change, still broken.

kasper93
kasper93 reviewed Apr 30, 2026
View reviewed changes
Comment thread sub/sd_ass.c Outdated
@llyyr llyyr force-pushed the fix/hard-coding-playresx branch from 294233b to 16ecdc8 Compare April 30, 2026 03:00
kasper93
kasper93 reviewed Apr 30, 2026
View reviewed changes
Comment thread sub/ass_mp.c Outdated
@llyyr

llyyr commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

first render and subsequent renders are not consistent

Latest commit should fix this

@llyyr llyyr force-pushed the fix/hard-coding-playresx branch from 493b508 to 808e342 Compare April 30, 2026 03:24
kasper93
kasper93 reviewed Apr 30, 2026
View reviewed changes
Comment thread sub/ass_mp.c Outdated
@llyyr llyyr force-pushed the fix/hard-coding-playresx branch 2 times, most recently from 7b5cc00 to 843634e Compare April 30, 2026 03:51
kasper93
kasper93 reviewed Apr 30, 2026
View reviewed changes
Comment thread sub/ass_mp.c
@llyyr llyyr force-pushed the fix/hard-coding-playresx branch 2 times, most recently from f7c3117 to 1315f65 Compare April 30, 2026 04:03
@Sneakpeakcss

Sneakpeakcss commented Apr 30, 2026

Copy link
Copy Markdown

first render and subsequent renders are not consistent

Latest commit should fix this

It sure did, works as expected on my end.

@rdevshp

rdevshp commented Jun 12, 2026 •
edited
Loading

Copy link
Copy Markdown

I experienced a regression for margin calculation when --osd-scale-by-window=no. The horizontal margin calculation isn't correct when --osd-scale-by-window=no.
The mpv command that I used to reproduce the issue was:

./build/mpv \
   --no-config \
    --osd-scale-by-window=no \
    --osd-align-x=left \
    --osd-align-y=top \
    --osd-margin-x=16 \
    --osd-margin-y=16 \
    --osd-font-size=30 \
    --osd-level=1 \
    --osd-msg1='OSD_MARGIN' \
  VIDEO_FILE

I am able to fix the regression by applying the following patch:

diff --git a/sub/osd_libass.c b/sub/osd_libass.c
index b4e1563268..2a76a761a4 100644
--- a/sub/osd_libass.c
+++ b/sub/osd_libass.c
@@ -275,8 +275,10 @@ static ASS_Style *prepare_osd_ass(struct osd_state *osd, struct osd_object *obj)
     double playresx = obj->ass.track->PlayResX;
     double playresy = obj->ass.track->PlayResY;
     // Compensate for libass and mp_ass_set_style scaling the font etc.
-    if (!opts->osd_scale_by_window && obj->vo_res.h)
+    if (!opts->osd_scale_by_window && obj->vo_res.h && obj->vo_res.w) {
         playresy *= 720.0 / obj->vo_res.h;
+        playresx *= 1280.0 / obj->vo_res.w;
+    }
 
     ASS_Style *style = get_style(&obj->ass, "OSD");
     mp_ass_set_style(style, playresx, playresy, &font);

llyyr added 2 commits June 12, 2026 15:54
@llyyr
sub/sd_ass: reuse PlayResX instead of hardcoding to 384
20686bf 
Hardcoding 384 here isn't correct, we should be using the original
PlayResX after all.

The problem commit 1723f8a was trying
to fix is instead fixed by rounding PlayResX instead of implicitly
truncating it. Before 1723f8a, PlayResX
was calculated using doubles and implicitly truncated to int, causing
off by one errors (e.g. 511 vs 512) due to truncation errors.

Round to nearest integer instead to fix this.

Fixes: 1723f8a
Fixes: f862d3b
@llyyr
sub: make initial Margin{L,R} X-axis aware
8bb3370 
Previously, we scaled all initial style parameter using PlayResY because
this matched libass' old behavior, where many quantities were scaled
isotropically using only the Y-axis.

After libass started applying indepentent x/y-axis scaling, horizontal
layout parameters like Margin{L,R} are interpretated relative to
PlayRwesX. As a result, the initial margins computed by mpv differ on
libass 0.16.0 and libass 0.17.0. Fix this by incorporating PlayResX into
Margin{L,R} calculation.

Other style fields remain Y-scaled, as they are font-relative and not
layout relative.
@llyyr llyyr force-pushed the fix/hard-coding-playresx branch from 1315f65 to 8bb3370 Compare June 12, 2026 10:24
@llyyr

llyyr commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Makes sense, I didn't test that case. good catch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kasper93 kasper93 kasper93 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@llyyr @kasper93 @Sneakpeakcss @na-na-hi @rdevshp