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

console.lua: scale with the window height #14114

Merged
merged 2 commits into from
Oct 27, 2024

Conversation

guidocella
Copy link
Contributor

This makes console.lua consistent with the OSD, osc.lua and stats.lua. This reads --osd-scale-by-window so users don't have to configure vidscale in yet another script.

Copy link

github-actions bot commented May 11, 2024

Download the artifacts for this pull request:

Windows
macOS

w = 720 * aspect
end

local scale = mp.get_property_native("display-hidpi-scale", 1) * opts.scale
Copy link
Contributor

Choose a reason for hiding this comment

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

OSD and other scripts don't scale contents with hidpi scale in either scaling modes, so it still behaves differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we remove it then?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also removed scale since I don't understand its purpose.

Copy link
Member

Choose a reason for hiding this comment

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

scale is basically required for readability on a hidpi display for lower-resolution videos. Well for me at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get why we have to be the only application to scale text with window height either and use --osd-scale-by-window=no. but both kasper and nanahi requested this.

Copy link
Contributor

@kasper93 kasper93 May 11, 2024

Choose a reason for hiding this comment

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

You guys are moving the bar right now. Everything in mpv scales. OSD, OSC, stats, subtitles. Name one thing which does not scale... console. I requested this, because it makes sense in mpv context.

EDIT:

If it is undesirable behavior, we can add another option and make it disable by default.

Copy link
Member

Choose a reason for hiding this comment

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

tbh there could be an argument made for subtitles not scaling. But anyways, I don't see the advantage of this change. It's fine to have it as an option I guess, but to me it's just a strictly worse default imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this scaling only makes sense to not break ASS subtitles and they are not affected by --sub-scale-by-window anyway (despite the docs stating "Like --sub-scale, this can break ASS subtitles."). I don't understand the advantage of scaling by default elsewhere.

Copy link
Contributor

@na-na-hi na-na-hi May 11, 2024

Choose a reason for hiding this comment

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

Stay on topic please. If you don't like the defaults, lets take effort to make OSC and stats respect --osd-scale-by-window by default, then changing the default for everything consistently only needs to change the default for a single flag. This PR is a step towards that.

@kasper93
Copy link
Contributor

This reads --osd-scale-by-window so users don't have to configure vidscale in yet another script.

I think it will be hard to agree to change console.lua default scaling, so maybe vidscale is the way, after all?

I'm fine with connecting this to osd-scale-by-window, but it seems controversial change.

@guidocella
Copy link
Contributor Author

There's no point in connecting it to --osd-scale-by-window if it won't follow its value by default since the whole point was not having to configure multiple scripts. However it is not ideal to keep using the inaccurate name vidscale since text also scales with black bars around videos and scaling is confusing enough as it is.

@kasper93
Copy link
Contributor

What do you propose then? I think this scaling should be configurable, but indeed --osd-scale-by-window seems not compatible.

@guidocella
Copy link
Contributor Author

Just scale-with-window=<yes|no> would be good I think. "Scale with" is much more common than "scale by" in this context. But does anyone actually intend to scale the console with the window or are we adding this for the heck of it?

@kasper93
Copy link
Contributor

I don't know. I'm simple man, I see open PR, I ask question. If you feel we don't need it, we can close and forget about it.

@guidocella
Copy link
Contributor Author

Closing then as I only opened this PR because you and nanahi requested it. I don't see why you would want the console to be unreadable in small windows.

@guidocella
Copy link
Contributor Author

Reopened due to many users wanting to scale. It is off by default. There is no consensus on what the default should be.

@@ -20,6 +20,7 @@ local opts = {
font = "",
font_size = 16,
border_size = 1,
scale_with_window = false,
Copy link
Contributor

@na-na-hi na-na-hi Oct 17, 2024

Choose a reason for hiding this comment

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

Can you add an auto option similar to other scripts? It can still default to no of course. Also for consistency reason the option name can be vidscale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vidscale is inaccurate because it doesn't scale with the video but with window, and vidscale itself is inconsistent with "osd-scale-by-window" anyway. They should all have been called scale with window. I don't think we need to keep using the bad naming from osc and stats in other scripts.

@guidocella guidocella force-pushed the console-vidscale branch 2 times, most recently from 6a6b2b7 to d7764f0 Compare October 17, 2024 16:09
@kasper93
Copy link
Contributor

kasper93 commented Oct 26, 2024

@guidocella: We have to resolve this issue. Current select font size is unusable. I think good default would be something along the lines of --script-opt=console-scale_with_window=yes --script-opt=console-font_size=24. I know there is #14114 which is not conclusive, but the question was about console, we need to fix select. Even if it means we have different behavior for select if we cannot agree on making console font bigger as a whole. Note that current console is also not usable on high resolution screens

If this is set to yes or auto and --osd-scale-by-window is true, console
scales with the window height like everything else in mpv.

Defaults to auto.
@guidocella
Copy link
Contributor Author

guidocella commented Oct 27, 2024

Changed the default to auto as "Scale the console with the window height, respecting --osd-scale-by-window" + "Do both" now has more votes then "Keep it as is" in the poll.

@kasper93
Copy link
Contributor

kasper93 commented Oct 27, 2024

I still think the font is too small for select even with scaling. In my opinion we shouldn't aim for "console appearance" in select.

Make the console easier to read because the current default is too
small. See for example
mpv-player#14903 (reply in thread)
or mpv-player#15036 (comment)
or mpv-player#15145 (comment)
or mpv-player#15031 (comment).

This also prevents libass from decreasing performance by printing many
lines.
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.

As evident from some comments listed in the commit message, the current state of the select (and console) feature is not very usable. This PR makes it more universal and scalable for different display resolutions. While we can adjust default values to better suit most users, it’s unlikely we can satisfy everyone. These changes, however, are a significant improvement over the current state. Let’s take this step forward, and we can continue to make improvements as we go.

@kasper93 kasper93 merged commit d2fd394 into mpv-player:master Oct 27, 2024
26 checks passed
@guidocella guidocella deleted the console-vidscale branch October 27, 2024 20:07
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