console.lua: fix the max width calculation #14419

guidocella · 2024-06-22T22:14:17Z

No description provided.

github-actions · 2024-06-22T22:28:04Z

Download the artifacts for this pull request:

Windows

macOS

christoph-heinrich · 2024-06-22T22:47:59Z

player/lua/console.lua

@@ -546,7 +547,8 @@ local function update()

    local lines_max = calculate_max_log_lines()
    -- Estimate how many characters fit in one line
-    local width_max = math.ceil(screenx / opts.font_size * get_font_hw_ratio())
+    local width_max = math.floor((screenx - margin - mp.get_property_native('osd-margin-x')) /


Shouldn't osd-margin-x get subtracted from screenx before it gets divided by dpi_scale?

I've done that for the margin too.

Actually it doesn't seem right to substract the margins from the clip coordinates, so I divided them by the scale afterwards.

MoSal · 2024-06-23T00:35:13Z

Before

After

Wrapped character count went from 9️⃣ to 8️⃣.

guidocella · 2024-06-23T07:01:33Z

Wrapped character count went from 9️⃣ to 8️⃣.

Does --osd-margin-x=0 fix it for you too?

guidocella · 2024-06-23T08:14:32Z

Scaling --osd-margin-x relatively to 720 seems to fix this.

MoSal · 2024-06-23T10:58:47Z

Scaling --osd-margin-x relatively to 720 seems to fix this.

That's not the issue.

Did some logging.

max_width=98 screenx=960 bottom_left_margin=6 dpi_scale=4
osd-margin-x=0 aspect=1.7777777777778 font_size=28 hw_ratio=2.8863296703297

That hw_ratio is big. If hw_ratio was 2.65. We get max_width=90.147321428571 flooring to 90.

Figuring out what font is being used, it's Noto Sans Mono. So I set that in cosole config. And grep for fontselect in debug output (Note: Exo 2 Black is my osd font).

[osd/libass] fontselect: (Exo 2 Black, 700, 0) -> /usr/share/fonts/OTF/Exo2-Black.otf, 0, Exo2-Black
[osd/libass] fontselect: (sans-serif, 400, 0) -> /usr/share/fonts/noto/NotoSans-Condensed.ttf, 0, NotoSans-Condensed
[osd/libass] fontselect: (Noto Sans Mono, 400, 0) -> /usr/share/fonts/noto/NotoSansMono-Condensed.ttf, 0, NotoSansMono-Condensed
[osd/libass] fontselect: (Exo 2 Black, 700, 0) -> /usr/share/fonts/OTF/Exo2-Black.otf, 0, Exo2-Black
[osd/libass] fontselect: (Noto Sans Mono, 700, 0) -> /usr/share/fonts/noto/NotoSansMono-Bold.ttf, 0, NotoSansMono-Bold
[osd/libass] fontselect: (sans-serif, 400, 0) -> /usr/share/fonts/noto/NotoSans-Condensed.ttf, 0, NotoSans-Condensed
[osd/libass] fontselect: (Noto Sans Mono, 700, 0) -> /usr/share/fonts/TTF/custom-FiraMonoWithFallback-Bold.ttf, 0, FiraMonoWithFallback-Bold

So I change console font to 'Fira Mono Medium' which has hw_ratio=1.8945181765724 which is withing expected range, and the problem is gone:

[osd/libass] fontselect: (Exo 2 Black, 700, 0) -> /usr/share/fonts/OTF/Exo2-Black.otf, 0, Exo2-Black
[osd/libass] fontselect: (sans-serif, 400, 0) -> /usr/share/fonts/noto/NotoSans-Condensed.ttf, 0, NotoSans-Condensed
[osd/libass] fontselect: (Fira Mono Medium, 400, 0) -> /usr/share/fonts/OTF/FiraMono-Medium.otf, 0, FiraMono-Medium
[osd/libass] fontselect: (Exo 2 Black, 700, 0) -> /usr/share/fonts/OTF/Exo2-Black.otf, 0, Exo2-Black
[osd/libass] fontselect: (Fira Mono Medium, 700, 0) -> /usr/share/fonts/OTF/FiraMono-Medium.otf, 0, FiraMono-Medium
[osd/libass] fontselect: (sans-serif, 400, 0) -> /usr/share/fonts/noto/NotoSans-Condensed.ttf, 0, NotoSans-Condensed
[osd/libass] fontselect: (Fira Mono Medium, 700, 0) -> /usr/share/fonts/TTF/custom-FiraMonoWithFallback-Bold.ttf, 0, FiraMonoWithFallback-Bold

So the console using NotoSans-Condensed.ttf triggers the issue!
hw_ratio values for condensed fonts is maybe an issue!

guidocella · 2024-06-23T11:07:55Z

Is this #14249 again? Does libass master fix it? Anyway --osd-margin-x=0 fixes the wrapping for me even without this patch so that definitely is an issue to be fixed too.

MoSal · 2024-06-23T11:54:15Z

Is this #14249 again? Does libass master fix it? Anyway --osd-margin-x=0 fixes the wrapping for me even without this patch so that definitely is an issue to be fixed too.

Bingo.
Using libass master does fix it.

So the hw_ratio calculations used the condensed font, but the regular font was actually used for rendering!
Why wasn't the wrong font used for rendering anyway?
Shouldn't the font selected by libass be used to avoid any potential mismatches?

christoph-heinrich · 2024-06-23T17:37:47Z

Here is why it osd-margin-x needs to be scaled for 720p

mpv/sub/ass_mp.c

Line 60 in cd1b63f

double scale = res_y / 720.0;

mpv/sub/ass_mp.c

Line 76 in cd1b63f

style->MarginL = opts->margin_x * scale;

kasper93 · 2024-06-23T17:45:52Z

Good that we have those magic numbers hardcoded around the codebase and no one knows why they are needed, until digging into it. /s

guidocella · 2024-06-23T17:50:56Z

Something is still off in the calculation as the lines can still wrap. Also I don't understand why it doesn't seem to be affected by --osd-scale-by-window.

MoSal · 2024-06-23T18:01:54Z

@guidocella
Are you sure no glyphs are using a different font?

christoph-heinrich · 2024-06-23T18:07:45Z

Also I don't understand why it doesn't seem to be affected by --osd-scale-by-window.

I'm pretty sure that's because the resolution to use for the supplied ass is set in mp.set_osd_ass(screenx, screeny, ass.text)

guidocella · 2024-06-23T18:14:28Z

@guidocella Are you sure no glyphs are using a different font?

Yeah you should be able to reproduce it by making the window smaller.

I'm pretty sure that's because the resolution to use for the supplied ass is set in mp.set_osd_ass(screenx, screeny, ass.text)

It's because prepare_osd_ass which checks opts->osd_scale_by_window isn't called from osd-overlay, I checked with gdb. But it still affected by the 720 scaling.

christoph-heinrich · 2024-06-23T18:28:08Z

It's because prepare_osd_ass which checks opts->osd_scale_by_window isn't called from osd-overlay, I checked with gdb. But it still affected by the 720 scaling.

Oh you meant why osd-margin-x isn't affected by osd-scale-by-window, I thought you meant the output text in general.

christoph-heinrich

It's an improvement over the status quo, even if it's not perfect for everyone.

Even if ass:pos() hardcodes the position at 6px from the left, we still need to subtract the left --osd-margin-x from the available text width. See how libass/ass_render.c:ass_render_event() subtracts it from max_text_width without checking if the event is positioned. The calculation is still not perfect as the width could be made a bit larger before the text wraps.

In select mode and when printing the OSD, use \q2 to let libass clip lines longer than the OSD. This works around having to leave a large horizontal space unused due to libass subtracting the left margin from the max text width even though the ASS event is anchored to the bottom left. It also fixes truncating wide Unicode characters.

christoph-heinrich

Works well

christoph-heinrich

No wrap suggestions in the terminal also work.

avih · 2024-06-24T15:12:38Z

disable_wrap = '\027[?7l'
enable_wrap = '\027[?7h'

These are DEC private mode reset/set auto-wrap mode (DECAWM).

mpv doesn't use terminal escape sequences "correctly" - it doesn't use terminfo or ncurses etc (which only use features supported by the terminal), which is why mpv should use only the most common escape sequences which are likely to work on most terminals out there.

I don't think It's guaranteed that this sequence is supported by all terminals, and I don't think this is generally a commonly-used escape sequence elsewhere. At least I don't recall encountering it in the past.

Off topic here, but same goes about using ALT-screen (iirc in sixel and/or tct), because it's not guaranteed to be supported or enabled on all terminals, for instance xterm on fedora by default doesn't have alt-screen AFAIK.

So mpv should try to limit itself as much as possible only to very common terminal features and sequences.

guidocella · 2024-06-24T15:50:09Z

It did work in all terminals I tried including xterm. If it wasn't supported log lines would just be printed as they are. I don't think there is another sane way to predict terminal unicode width from Lua.

na-na-hi · 2024-06-24T15:58:18Z

I don't think It's guaranteed that this sequence is supported by all terminals, and I don't think this is generally a commonly-used escape sequence elsewhere.

According to https://invisible-island.net/xterm/ctlseqs/ctlseqs.html:

Ps = 7 ⇒ Auto-Wrap Mode (DECAWM), VT100.

I think VT100 should be OK as a baseline, and is the default terminal assumed by most *nix TUI programs. It won't work on VT52 terminals but I doubt it's used in any meaningful capacity.

However, this won't work with Windows console if VT processing is disabled. And even if VT processing is enabled, this auto-wrap mode code isn't listed on the MS documentation. It's only supported for recent terminal versions: microsoft/terminal#3943 (other codes like show/hide cursor are listed in the documentation)

guidocella · 2024-06-24T16:19:44Z

Is it worth keeping truncate_utf8 as a fallback for Windows then?

This prevents line wraps in select mode with terminal output which hide the top items (OSD output no longer uses truncate_utf8 since the previous commit). This is not ideal as even accented letters are assumed to be 2 cells wide. The alternative is using \e[?7l and \e[?7h to disable and enable text wrapping, but it won't work in Windows console with VT processing disabled.

guidocella · 2024-06-24T17:46:21Z

Changed to assume all non-ASCII characters span 2 cells instead.

MoSal · 2024-06-24T20:20:29Z

Changed to assume all non-ASCII characters span 2 cells instead.

Quite the assumption:

On the other hand (not a real-world example, just to be clear):

Clipping works.

guidocella · 2024-06-24T20:28:23Z

It still clips graphical output. I liked clipping in the terminal too but we will probably integrate wcwidth in term_disp_width() and expose it to Lua due to this Windows console issue and needing to fix width calculation for msg.c anyway.

kasper93 · 2024-06-24T20:40:15Z

due to this Windows console issue

And even if VT processing is enabled, this auto-wrap mode code isn't listed on the MS documentation. It's only supported for recent terminal versions: microsoft/terminal#3943 (other codes like show/hide cursor are listed in the documentation)

Ok, let's not worry too much about it. This has been merged into conhost in build 19603, which was 4 years ago. Sure we might left this one guy who uses 1507 and in the same time uses terminal... on Windows. I would argue that we can afford having this slightly broken. To trigger this you need to use non-ascii chars, use terminal and use old Windows build. Maybe there is one guy affected, but really... Non-VT mode is indeed a problem, but in this case we could fallback to wrapping.

Either way, I don't think preventing terminal wrapping, really solves the issue, because msg.c code expect that and will clear wrong lines because of line count mismatch. So the proper fix is to have common code term_disp_width() that calculates this width.

guidocella changed the title ~~console.lua: calculate the max width more accurately~~ console.lua: fix the max width calculation Jun 22, 2024

guidocella force-pushed the console-width branch from f366a4c to be2fd83 Compare June 22, 2024 22:39

christoph-heinrich reviewed Jun 22, 2024

View reviewed changes

guidocella force-pushed the console-width branch from be2fd83 to d21a4a0 Compare June 23, 2024 07:01

guidocella force-pushed the console-width branch from d21a4a0 to d47508a Compare June 23, 2024 08:12

guidocella force-pushed the console-width branch 2 times, most recently from f986171 to cba7e52 Compare June 23, 2024 08:57

guidocella force-pushed the console-width branch 2 times, most recently from 5b2dc12 to 43e154e Compare June 23, 2024 15:29

guidocella force-pushed the console-width branch from 43e154e to d8fb0c7 Compare June 23, 2024 18:27

christoph-heinrich approved these changes Jun 23, 2024

View reviewed changes

guidocella force-pushed the console-width branch from d8fb0c7 to f20ddfa Compare June 23, 2024 19:14

guidocella force-pushed the console-width branch from f20ddfa to 4412633 Compare June 23, 2024 20:23

christoph-heinrich approved these changes Jun 24, 2024

View reviewed changes

guidocella force-pushed the console-width branch from b8216c1 to 82f2739 Compare June 24, 2024 13:27

christoph-heinrich approved these changes Jun 24, 2024

View reviewed changes

guidocella force-pushed the console-width branch from 82f2739 to c60d3d1 Compare June 24, 2024 17:46

kasper93 approved these changes Jun 24, 2024

View reviewed changes

kasper93 merged commit 01330db into mpv-player:master Jun 24, 2024
20 checks passed

guidocella deleted the console-width branch June 24, 2024 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

console.lua: fix the max width calculation #14419

console.lua: fix the max width calculation #14419

guidocella commented Jun 22, 2024

github-actions bot commented Jun 22, 2024 •

edited

Loading

christoph-heinrich Jun 22, 2024 •

edited

Loading

guidocella Jun 23, 2024

guidocella Jun 23, 2024

MoSal commented Jun 23, 2024

guidocella commented Jun 23, 2024

guidocella commented Jun 23, 2024

MoSal commented Jun 23, 2024

guidocella commented Jun 23, 2024

MoSal commented Jun 23, 2024

christoph-heinrich commented Jun 23, 2024

kasper93 commented Jun 23, 2024

guidocella commented Jun 23, 2024

MoSal commented Jun 23, 2024

christoph-heinrich commented Jun 23, 2024

guidocella commented Jun 23, 2024

christoph-heinrich commented Jun 23, 2024

christoph-heinrich left a comment

christoph-heinrich left a comment

christoph-heinrich left a comment

avih commented Jun 24, 2024

guidocella commented Jun 24, 2024

na-na-hi commented Jun 24, 2024 •

edited

Loading

guidocella commented Jun 24, 2024

guidocella commented Jun 24, 2024

MoSal commented Jun 24, 2024

guidocella commented Jun 24, 2024

kasper93 commented Jun 24, 2024 •

edited

Loading

console.lua: fix the max width calculation #14419

console.lua: fix the max width calculation #14419

Conversation

guidocella commented Jun 22, 2024

github-actions bot commented Jun 22, 2024 • edited Loading

christoph-heinrich Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

guidocella Jun 23, 2024

Choose a reason for hiding this comment

guidocella Jun 23, 2024

Choose a reason for hiding this comment

MoSal commented Jun 23, 2024

guidocella commented Jun 23, 2024

guidocella commented Jun 23, 2024

MoSal commented Jun 23, 2024

guidocella commented Jun 23, 2024

MoSal commented Jun 23, 2024

christoph-heinrich commented Jun 23, 2024

kasper93 commented Jun 23, 2024

guidocella commented Jun 23, 2024

MoSal commented Jun 23, 2024

christoph-heinrich commented Jun 23, 2024

guidocella commented Jun 23, 2024

christoph-heinrich commented Jun 23, 2024

christoph-heinrich left a comment

Choose a reason for hiding this comment

christoph-heinrich left a comment

Choose a reason for hiding this comment

christoph-heinrich left a comment

Choose a reason for hiding this comment

avih commented Jun 24, 2024

guidocella commented Jun 24, 2024

na-na-hi commented Jun 24, 2024 • edited Loading

guidocella commented Jun 24, 2024

guidocella commented Jun 24, 2024

MoSal commented Jun 24, 2024

guidocella commented Jun 24, 2024

kasper93 commented Jun 24, 2024 • edited Loading

github-actions bot commented Jun 22, 2024 •

edited

Loading

christoph-heinrich Jun 22, 2024 •

edited

Loading

na-na-hi commented Jun 24, 2024 •

edited

Loading

kasper93 commented Jun 24, 2024 •

edited

Loading