search

mpv-player / mpv

🎥 Command line video player
https://mpv.io
Other
26.65k stars 2.83k forks source link

console.lua: fix the max width calculation #14419

Closed guidocella closed 4 days ago

github-actions[bot] commented 6 days ago

Download the artifacts for this pull request:

Windows * [mpv-i686-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1632787125.zip) * [mpv-x86_64-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/1632792628.zip) * [mpv-x86_64-windows-msvc](https://nightly.link/mpv-player/mpv/actions/artifacts/1632804262.zip)
macOS * [mpv-macos-12-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1632789844.zip) * [mpv-macos-13-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/1632794634.zip) * [mpv-macos-14-arm](https://nightly.link/mpv-player/mpv/actions/artifacts/1632784746.zip)
MoSal commented 6 days ago

Before

select_pre

After

select_post

Wrapped character count went from :nine: to :eight:.

guidocella commented 6 days ago

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

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

guidocella commented 6 days ago

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

MoSal commented 6 days ago

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 commented 6 days ago

Is this https://github.com/mpv-player/mpv/issues/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 commented 5 days ago

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 commented 5 days ago

Here is why it osd-margin-x needs to be scaled for 720p https://github.com/mpv-player/mpv/blob/cd1b63f628a99d16183961056bb8177511a85888/sub/ass_mp.c#L60 https://github.com/mpv-player/mpv/blob/cd1b63f628a99d16183961056bb8177511a85888/sub/ass_mp.c#L76

kasper93 commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

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

christoph-heinrich commented 5 days ago

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 commented 5 days ago

@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 commented 5 days ago

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.

avih commented 4 days ago

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 commented 4 days ago

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 commented 4 days ago

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: https://github.com/microsoft/terminal/pull/3943 (other codes like show/hide cursor are listed in the documentation)

guidocella commented 4 days ago

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

guidocella commented 4 days ago

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

MoSal commented 4 days ago

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

Quite the assumption:

select_new_with_info

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

select_new2

Clipping works.

guidocella commented 4 days ago

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 commented 4 days ago

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: https://github.com/microsoft/terminal/pull/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.