search

mpv-player / mpv

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

command: add cursor-centric-zoom #15310

Closed guidocella closed 1 week ago

guidocella commented 1 week ago

Touch input is untested.

github-actions[bot] commented 1 week ago

Download the artifacts for this pull request:

Windows * [mpv-i686-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/2198445933.zip) * [mpv-x86_64-w64-mingw32](https://nightly.link/mpv-player/mpv/actions/artifacts/2198448080.zip) * [mpv-x86_64-windows-msvc](https://nightly.link/mpv-player/mpv/actions/artifacts/2198455662.zip)
macOS * [mpv-macos-13-intel](https://nightly.link/mpv-player/mpv/actions/artifacts/2198443792.zip) * [mpv-macos-14-arm](https://nightly.link/mpv-player/mpv/actions/artifacts/2198442773.zip) * [mpv-macos-15-arm](https://nightly.link/mpv-player/mpv/actions/artifacts/2198442624.zip)
na-na-hi commented 1 week ago

This should be done as a built-in/TOOLS script. There is no reason for this to be a command when everything else uses writable properties for zooming.

llyyr commented 1 week ago

I think it's a good idea to have a way to provide cursor centric zoom without external scripts. We can either do it with this PR, or a built-in script. But I don't really know why we would add more built-in scripts when a C implementation is already done

na-na-hi commented 1 week ago

But I don't really know why we would add more built-in scripts when a C implementation is already done

Providing it as a script has several advantages:

The goal of mpv is to be an extensible player, by making more functionalities possible to be implemented as scripts, instead of making everything going into C code, especially for "nice to have", non-essential features like this.

kasper93 commented 1 week ago

Writing it in a memory-safe language simplifies programming.

Not applicable here. I also don’t think memory-safe languages simplify programming; they merely prevent you from accessing memory incorrectly. C is very simple language to work with.

Avoids bloating player core and command interface.

If it is a built-in script, the interface will be bloated anyway.

Easily modifiable by users without having to recompile the whole program.

Depending on the use case, users generally don't need to modify these basic commands.

The goal of mpv is to be an extensible player, by making more functionalities possible to be implemented as scripts, instead of making everything going into C code, especially for "nice to have", non-essential features like this.

I kind of agree. All this command does is set already existing properties; it doesn't need to be in C. There are many similar cases where we could move some functionality to scripts for better flexibility. In this case, I would see it as a built-in script that computes correct values for the video-zoom and video-align properties. Such a script could also be extended to include video-dragging functionality, not just zooming. While it's valid to implement this in C, it's a decision whether we want some things directly implemented or if we should rely on the scripting backend and keep the core untouched.

But since, it's implemented in C, I don't mind having it, if it's easier this way.

llyyr commented 1 week ago

Easily modifiable by users without having to recompile the whole program.

The goal of mpv is to be an extensible player, by making more functionalities possible to be implemented as scripts, instead of making everything going into C code, especially for "nice to have", non-essential features like this.

What reasonable modifications do you envision being made to "cursor centric zoom" without it being a totally different feature entirely?

I'm ignoring the first two points on your bullet list because I don't believe even you seriously believe they're good points to bring up, please keep the trolling to a minimum next time.

avih commented 1 week ago

Extending internally with scripts does keep the core from bloating, and memory-safe languages do have less footguns than C. Just think how much care would be needed to write the OSC or ytdl-hook in C, while in lua, it's maybe not always perfect, but at least you know it's safe (as far as memory access goes), and trivial for users to start hacking.

So generally, keep using scripts to extend the code does have advantages.

However, in this case I think the functionality is simple enough to keep at the C code.

na-na-hi commented 1 week ago

they merely prevent you from accessing memory incorrectly. C is very simple language to work with.

What's lua's drawback compared to C here? There is no reason to use C when lua works for this case.

If it is a built-in script, the interface will be bloated anyway.

Not really. The command and options of playback core are unchanged. This is what had been done with SMTC, which is a built-in plugin instead of in player core.

I would see it as a built-in script that computes correct values for the video-zoom and video-align properties. Such a script could also be extended to include video-dragging functionality, not just zooming.

This is my plan. I will open a PR to implement them as a built-in script. It will be better than the C implementation for various reasons noted.

What reasonable modifications do you envision being made to "cursor centric zoom" without it being a totally different feature entirely?

For example, smooth/"kinetic" zooming with mouse wheel, or setting to control whether it centers to cursor or screen when zooming to <100% etc. Smooth zooming would be much more complicated to implement in player core compared to lua, where it can use timers.

I'm ignoring the first two points on your bullet list because I don't believe even you seriously believe they're good points to bring up, please keep the trolling to a minimum next time.

Please keep bad faith arguments to a minimum next time. This is not constructive to the discussion.

guidocella commented 1 week ago

Some points to consider:

na-na-hi commented 1 week ago
  • An advantage of being a command is that it respects the no-osd prefix for whether to show the new video-zoom, while script-message doesn't

If it's implemented as a script, this can simply be a script option.

  • We already have playlist-next-playlist and playlist-prev-playlist as a precedent for commands that could be scripts.
  • We recently went in the opposite direction by converting autoload.lua to a C option.

These are playlist management features that are expected to be a built-in player functionality for any video players, and in the loadfile case it reuses existing directory loading code, so they get a pass. A nonessential feature like this PR shouldn't be treated the same.

  • at least I can just put my existing drag-to-pan and align-to-cursor implementations there, I assume those would be much harder to convert to C since they temporarily rebind MOUSE_MOVE (and I can also osd-relative-pan).

This would be a better solution than the C one IMO. I don't see any problem with upstreaming built-in lua scripts when they enhance the player with useful features.

guidocella commented 1 week ago

Do you know if there's a way to respect cmd->scale in Lua? That would be the only disadvantage of a script. We probably have to add support to script-message itself.

na-na-hi commented 1 week ago

Do you know if there's a way to respect cmd->scale in Lua?

Not currently, but I have been looking into this. Several script features don't support scaled commands right now but I'm planning to add them. As you said this can be added to script-message.

Edit: Scripts can now add scalable key bindings with https://github.com/mpv-player/mpv/pull/15316, so your scripts can handle scaled input devices now.

llyyr commented 1 week ago

What's lua's drawback compared to C here? There is no reason to use C when lua works for this case.

The functionality will be unavailable if mpv is compiled without lua. Of course this is rare, but I don't see why we would opt for a Lua implementation when a C implementation is already done. mpv historically has always preferred C implementations except for things that are intended to be modular, like osc or stats.lua or console.lua. This is a command, it is not intended to be modular. If there are improvements to be made to it, it should be done inside mpv. Another reason would be that this increases the maintenance burden in the future when/if luajit stops being actively developed, it's yet another script that will need to be tested to work with Lua 5.3/5.4. This is something we've already discussed before. You don't have to deal with any of these problems if it's implemented in C.

Of course, none of this matters if using a built-in Lua script means we can merge more features into mpv beyond just cursor centric zoom. I was in favor of this MR purely because the work is already done, so it makes no sense to add a Lua script just for this. I like #15314 better in any case.

This is my plan. I will open a PR to implement them as a built-in script. It will be better than the C implementation for various reasons noted.

You're engaging in circular reasoning here. You can't say "better for reasons noted" when those reasons are a point of contention. We're discussing the reasons you've cited, you can't say "my reasons are valid for the reasons noted".

These are playlist management features that are expected to be a built-in player functionality for any video players, and in the loadfile case it reuses existing directory loading code, so they get a pass. A nonessential feature like this PR shouldn't be treated the same.

This is subjective and has changed over time in this project, I don't believe you get to unilaterally decide where the line is drawn for "non-essential".

guidocella commented 1 week ago

For the record, upgrading to Lua 5.4 is easy, I already showed all that's necessary in https://github.com/mpv-player/mpv/pull/14905 and it is just +16 -15 lines. positioning.lua shouldn't need any modification since it doesn't use string.format.

Samillion commented 1 week ago

My input will be about the centric-zoom itself, not whether it should be a script or not.

Honestly, a welcomed addition. mpv is capable of being a great image viewer, it is why I suggest guido's centric zoom in my osc's image mode https://github.com/Samillion/ModernZ/blob/main/docs/IMAGE_VIEWER.md

To be even more blunt, I genuinely think osc.lua should offer an image mode as well, with the relative features such as centric zoom enabled in it.

To be fair, I realize this has never been a high demand, but I honestly think it's because most users aren't aware of such capabilities to be possible within mpv.

Also, while my use case suggests "image mode", I'm sure many would appreciate it in all modes as well.

It's one of those things that you realize you need, only once you know about it.

guidocella commented 1 week ago
  • Writing it in a memory-safe language simplifies programming.

Ironically, I just noticed that change_property_cmd causes a stack-buffer-overflow when building with ASan. I'm not sure what the problem is.

guidocella commented 1 week ago

Fixed the stack-buffer-overflow.

guidocella commented 1 week ago

Superseded by #15314.

guidocella commented 6 days ago

To be even more blunt, I genuinely think osc.lua should offer an image mode as well, with the relative features such as centric zoom enabled in it.

osc.lua is terrible for images because it breaks drag-to-pan from the bottom half of the window. Unless it can be made to work like uosc I think we should just disable it for images, I don't think an OSC for images is that useful anyway, e.g. sxiv/feh/imv don't have any.