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

{stats,osc}.lua: respect --osd-scale-by-window by default #14158

Merged
merged 6 commits into from
May 20, 2024

Conversation

na-na-hi
Copy link
Contributor

This lets these scripts scale the elements with OSD by default. The vidscale option now accepts and defaults to auto value which enables this behavior.

@guidocella
Copy link
Contributor

I implemented this too but was waiting for the console one to get merged. Personally I would just remove vidscale unless someone can think of a use case of having each script scale differently, especially since the name of this option is wrong, they scale with the window and not with the video, which can be between black bars.

Copy link

github-actions bot commented May 16, 2024

Download the artifacts for this pull request:

Windows
macOS

@na-na-hi
Copy link
Contributor Author

Personally I would just remove vidscale unless someone can think of a use case of having each script scale differently

This is kept for compatibility reason. If someone has already set vidscale to no but hasn't set --osd-scale-by-window to no, it no longer works.

Also since stats.lua contains too much information, there is a valid reason to set vidscale to no so that more content is shown if the window size is large without also changing --osd-scale-by-window.

@guidocella
Copy link
Contributor

FWIW the OSC behavior will change by default anyway if we're going to default to --osd-scale-by-window=no (which has been approved at least by Dudemanguy).

@na-na-hi
Copy link
Contributor Author

na-na-hi commented May 16, 2024

Changing the default of --osd-scale-by-window is going to be done later and it's out of scope for this PR, which doesn't change the behavior for --no-config. It merely provides a mechanism to respect that option, and a way to override that if the user wants to do so.

I don't see any value in removing this option from users if the mechanism isn't broken or doesn't become a significant maintenance burden.

player/lua/osc.lua Outdated Show resolved Hide resolved
@guidocella
Copy link
Contributor

You could also add mp.observe_property("osd-scale-by-window", "native", update_scale) in stats.lua and mp.observe_property("osd-scale-by-window", "native", request_init_resize) in osc.lua (I didn't add this to console.lua because it updates anyway if run cycle osd-scale-by-window).

@na-na-hi na-na-hi force-pushed the vidscale-osd branch 2 times, most recently from 32c139c to 0a15a94 Compare May 16, 2024 16:39
@na-na-hi
Copy link
Contributor Author

na-na-hi commented May 16, 2024

Added osd-scale-by-window runtime change handling.

@kasper93
Copy link
Contributor

I implemented this too but was waiting for the console one to get merged.

Since @Dudemanguy had strong opinion about this PR, I will let you guys decide what is the path forward. I personally think that scaling make sense, if you resize window often, so that OSC doesn't cover half of the video with small sizes. But as long as there is an option to control that, it will be ok.

@Dudemanguy
Copy link
Member

Hmm no, I'm fine with this. It doesn't change current behavior.

@kasper93
Copy link
Contributor

Hmm no, I'm fine with this. It doesn't change current behavior.

I refereed to #14114 which guido mentioned. This all seems like contented changes.

@Dudemanguy
Copy link
Member

My general feeling on the matter is that scaling the scripts that are primarily text (e.g. stats and console) is basically useless at best. If you make the window small, then you can't read the text anyway which defeats the whole point. It makes more sense to just pick a sensible default font size imo and stick to it like what console currently does. For the osc, I am not so sure. There may be utility in scaling it up although scaling it too far down also makes it kind of useless.

Anyways, this PR doesn't change the status quo so no objections here.

@kasper93
Copy link
Contributor

My general feeling on the matter is that scaling the scripts that are primarily text (e.g. stats and console) is basically useless at best. If you make the window small, then you can't read the text anyway which defeats the whole point.

I hear you, but there is a range of readability. Also if user makes player window small (and video) it means they can see it, presumably looking at the player up close. If the user make it big the situation is reversed. I mean scaling the text (and video) makes it usable for both close watching on small screen and far watching on big screen. Without scaling, both those cases would need different font size, manually managed by user. I think it makes little sense in practice to force it and I don't think users are complaining about it and imho it would be jarring change.

This recommends querying the value of this option when drawing UI elements.
This allows a greater level of consistency by using a single flag which
already controls the scaling behavior of the OSD to control the behavior
of all scripts.

Also fix a capitalization nearby.
This adds auto to vidscale script option, which lets the scale be
inherited from OSD --osd-scale-by-window option.
This adds auto to vidscale script option, which lets the scale be
inherited from OSD --osd-scale-by-window option.
This lets these scripts scale the elements with OSD by default.
Allows the scale mode to be changed at runtime if vidscale is set to auto.
Allows the scale mode to be changed at runtime if vidscale is set to auto.
Copy link
Contributor

@kasper93 kasper93 left a comment

Choose a reason for hiding this comment

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

LGTM. Please rebase.

@guidocella
Copy link
Contributor

For the record Kasper and I agreed to keep the current defaults for the reasons he stated and for consistency with VLC and MPC-HC (I thought only mpv scaled text, but those do too, though only subtitles for MPC-HC, and only mpv scales the UI).

@kasper93 kasper93 merged commit 691a25d into mpv-player:master May 20, 2024
19 checks passed
@mlindner
Copy link
Contributor

mlindner commented Jul 8, 2024

This change broke window scaling for me. Now osd-scale options are ignored for stats/osc. To be more clear, when osd-scale-by-window=no, the osd-scale option is also ignored, removing any ability to scale the text output from scripts resulting in the text scale of the 'i' option being tiny and 'o' option being properly scaled by osd-scale.

@na-na-hi
Copy link
Contributor Author

na-na-hi commented Jul 8, 2024

Now osd-scale options are ignored for stats/osc.

I tested some old versions with --no-config and stats never respected osd-scale for me. What's your config?

@mlindner
Copy link
Contributor

mlindner commented Jul 8, 2024

Now osd-scale options are ignored for stats/osc.

I tested some old versions with --no-config and stats never respected osd-scale for me. What's your config?

Ah you're right. I originally started using osd-scale and osd-scale-by-window to get the on screen display to match the reasonably-scaled (for me) stats. Now that you've modified the stats to follow osd-scale-by-window, the stats have become absolutely microscopic (using on a 4k TV) such that they are entirely unreadable, and because they ignore osd-scale I'm forced to shut off osd-scale-by-window in order to make them readable (unless there's some other option for scaling stats). So this solution is entirely unworkable. You need to make stats to follow the osd-scale option or add an option for scaling stats. For now I'm going to have to revert this patch on my end until this is fixed.

@na-na-hi
Copy link
Contributor Author

na-na-hi commented Jul 8, 2024

unless there's some other option for scaling stats

There is. It's written in the documentation. Set vidscale of osc and stats to yes instead of auto restores the old behavior where they are always scaled and don't respect osd-scale-by-window.

@mlindner
Copy link
Contributor

mlindner commented Jul 8, 2024

unless there's some other option for scaling stats

There is. It's written in the documentation. Set vidscale of osc and stats to yes instead of auto restores the old behavior where they are always scaled and don't respect osd-scale-by-window.

I see. I missed that option, thank you. However that's not quite ideal with a fix that makes stats follow osd-scale-by-window as without that there's no way to control the size of the stats scale.

@mlindner
Copy link
Contributor

mlindner commented Jul 8, 2024

While you're looking at this kind of thing you should check out how osd-border-size works. The border width scales with screen resolution with osd-scale-by-window turned on, but when it's turned off suddenly you need to manually specify osd-border-size in addition to osd-scale, else the black border around the text shrinks to almost nothing (with respect to the text size) on large scales needed on 4k displays.

@guidocella
Copy link
Contributor

Both the osd https://mpv.io/manual/master/#options-osd-font-size and stats https://mpv.io/manual/master/#stats-font-size have font size options used with and without scale-by-window. I didn't even know we had --osd-scale.

@mlindner
Copy link
Contributor

mlindner commented Jul 8, 2024

Both the osd https://mpv.io/manual/master/#options-osd-font-size and stats https://mpv.io/manual/master/#stats-font-size have font size options used with and without scale-by-window. I didn't even know we had --osd-scale.

Font size can be frustrating to use because those both change with screen DPI. But thank you, that should be sufficient.

@guidocella
Copy link
Contributor

mpv doesn't use the HIDPI scale factor in the OSD font size at all.

@mlindner
Copy link
Contributor

mlindner commented Jul 8, 2024

unless there's some other option for scaling stats

There is. It's written in the documentation. Set vidscale of osc and stats to yes instead of auto restores the old behavior where they are always scaled and don't respect osd-scale-by-window.

Perhaps I'm doing something wrong here but I can't get this to work. I set --no-config --osd-scale-by-window=no --script-opts=vidscale=yes on the command line and observed no change in size of the stats display when changing video window size. Also there was no error for invalid option selected. Can someone confirm it's working for them and that this is the correct command?

@guidocella
Copy link
Contributor

--script-opts=stats-vidscale=yes

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.

5 participants