stats.lua: rename a variable #15068
stats.lua: rename a variable #15068
Conversation
guidocella
force-pushed
the
scale-upwards
branch
from
b7a1b36
to
311a96c
Compare
sub/sd_ass.c
Outdated
| @@ -508,7 +508,7 @@ static void configure_ass(struct sd *sd, struct mp_osd_res *dim, | |||
| if (converted || shared_opts->ass_style_override[sd->order] == ASS_STYLE_OVERRIDE_FORCE) { | |||
| set_scale_with_window = opts->sub_scale_with_window; | |||
| set_use_margins = opts->sub_use_margins; | |||
| set_scale_by_window = opts->sub_scale_by_window; | |||
| set_scale_by_window = opts->sub_scale_by_window && dim->h > 720; | |||
There was a problem hiding this comment.
Sorry to instantly start another bikeshed, but do we really want to do this for subtitles? Of course, small subtitles that you can't read are useless but having overly large ones that completely cover up the picture is maybe worse?
There was a problem hiding this comment.
I agree. scale-by-window should also scale down by default. Otherwise, for smaller window the subs can end up too big. scale-by-window is not scale-only-up-by-window (and it's not obvious up from what. 720 as the threshold seems arbitrary from a user point of view).
There was a problem hiding this comment.
I thought we settled on leaving the scaling enabled by default with #14158 (comment). I think it would be inconsistent for the OSD and subs to have different default scaling behavior.
There was a problem hiding this comment.
To me they seem like different usecases. For builtin/OSD stuff, the user actively opts into those some way (showing stats, osc, etc.) so them overly obscuring the underlying video is a given. subtitles are more passive to me and ideally should be unobtrusive. Of course some overly complicated ass subtitles disagree with me but that's a different story.
I realize that trying to optimize the UI of a small mpv window is kind of hard though.
There was a problem hiding this comment.
¯\(ツ)/¯ I don't like the scaling either but kasper's point that it keeps text including subtitles readable on large screens several meters away makes sense and both VLC and MPC-HC scale subtitles.
There was a problem hiding this comment.
If you have a use case (with specific window sizes) which this commit improves, then the commit message should mention how it behaves before and after this commit.
You simply can't optimize text size for a small window, so the best you can do is to scale it down and if it's unreadable then so be it.
Otherwise with a small window (think PIP-like mode) the text can and will definitely be too big.
There was a problem hiding this comment.
I'm not sure I understand how the 720p cutoff helps? I understand that with very small window sizes certain elements become "too small", but it is already the case for OSC, subtitles, stats and so on... and most importantly video. If someone using 100x100 window, it is already too late for them. I would argue it is better to keep relative size of everything as if it were program thumbnail.
I understand what you are trying to do to make things readable at lower window sizes, but is this really a problem? Also note that at 100x100 you won't fit any meaningful amount of subtitle text anyway.
player/lua/stats.lua
Outdated
| @@ -1525,7 +1525,7 @@ local function print_page(page, after_scroll) | |||
| end | |||
| end | |||
|
|
|||
| local function update_scale(value) | |||
| local function update_scale(height) | |||
There was a problem hiding this comment.
I don't think this is a good rename.
You can't update_scale(height).
You either set scale to a factor or value, or update_height with a new height value.
(I didn't check what it does exactly, so I don't know how that function should be called)
There was a problem hiding this comment.
Renamed to osd_height.
|
Download the artifacts for this pull request: |
guidocella
force-pushed
the
scale-upwards
branch
from
311a96c
to
3814b15
Compare
|
Anyway, I disagree with the premise of this PR. scale-by-window should scale by window. Not scale only up from an arbitrary value by window. |
guidocella
force-pushed
the
scale-upwards
branch
from
3814b15
to
57d94ff
Compare
|
Yeah I wasn't expecting this to be merged, it was just an attempt to deal with #14114 (comment) somehow. I left only the commit to rename a variable. |
guidocella
changed the title
These options keep text readable whether you use a small screen near you or a faraway large screen, but they make it unreadable in small windows, so change them to only scale upwards, i.e. when the window height is > 720.
Proposed in #15044. But I don't use these options so I don't know if you may want the downscaling. Also scrollable stats page 1 was already implemented in #12271 / https://github.com/mpv-player/mpv/compare/06f8166ae83253de79e44f62cfdceda15eef9393..3e34942c55eca8a3908f8e845787064f3157e3a8 so that can be reused.