search

mpvnet-player / mpv.net

🎞 mpv.net is a media player for Windows with a modern GUI.
GNU General Public License v2.0
3.39k stars 161 forks source link

Scripting: mp.add_key_binding() doesn't work #271

Closed ghost closed 2 years ago

ghost commented 3 years ago

Describe the bug This function doesn't register key binding in mpv.net, yet it does on regular mpv on Windows.

To Reproduce Steps to reproduce the behavior:

  1. put this script in AppData\Roaming\mpv.net\scripts\asdf.js 
    mp.add_key_binding("ctrl+z", "hello", function() { mp.osd_message("hello"); });
  2. launch mpv.net, press ctrl+z
  3. doesn't work :)

Manually binding or running script-message hello does work however.

Cheers

hooke007 commented 3 years ago

Because disable the built-in bindings is the default setting for mpvnet. You have to manually bind it.

stax76 commented 3 years ago

The topic was recently raised by @avih:

https://github.com/mpv-player/mpv/issues/8809

I probably need to reread the thread because I have awful bad memory, if I remember right, then there is nothing I could do other than making a feature or pull request.

It's not a bug because the behavior is desired for typical libmpv consumers, mpv.net is the only one trying to mimic mpv completely.

thorizer commented 2 years ago

how to manually bind it ?

hooke007 commented 2 years ago

https://mpv.io/manual/master/#command-interface-script-binding

And read the relative content of scripts. mp.add_key_binding

avih commented 2 years ago

The TL;DR is that if the (libmpv) builtin bindings are disabled (which is what mpv.net wants), then this also disables bindings added with mp.add_key_binding (which mpv.net doesn't want, but have to live with).

However, mp.add_forced_key_binding still works even when the builtin bindings are disabled.

It's a hard problem to solve, and for now there's no solution to both disable the builtin bindings and making mp.add_key_binding work (for keys which are not defined at input.conf, because if they are at input.conf then it overrides them also in normal mpv and even if the builtin bindings are not disabled).

This has more info - https://github.com/mpv-player/mpv/issues/8809 and the docs should be up to date - https://mpv.io/manual/master/#options-no-input-default-bindings

avih commented 2 years ago

@stax76 would you be able to test a patch to (lib)mpv?

The problem is that input-default-binding is a runtime value, so changing it at runtime enabled/disables the affected bindings. For that to work, all the bindings are loaded in a complex priority levels scheme, and in this scheme, the builtin bindings and those added with mp.add_key_bindings are considered the same "level" (which the user's input.conf can override), and splitting them into distinct levels is hard and can easily regress things.

So I had an idea to add another option, which only has an effect on startup (cannot be changed at runtime), which will simply avoid reading the builtin bindings, and that's it. Later at runtime the builtin bindings can NOT be restored. It should be a trivial option to add, and I think it will be good enough for you, right?

So would you be able to test a patch?

avih commented 2 years ago

@stax76 can you try this? https://github.com/mpv-player/mpv/pull/9284

stax76 commented 2 years ago

Thank you for working on this @avih. I can't easily build libmpv, last time I spent hours to build it but wasn't successful. It's something I'm interested to learn in the future. Currently, I'm inactive, but I'm looking forward to work on mpv.net again soon, testing would not be a problem if there is a build, shinshiro usually makes one two times a month.

avih commented 2 years ago

@stax76 https://0x0.st/-Eii.7z

This includes both the stand-alone mpv executable and the dll and (hopefully) required other files. I didn't test the dll, and you might need to rename mpv.dll.a to libmpv.dll.a

It might be enough to just replace the dll without rebuilding anything (if you can configure input-builtin-bindings=no from config files without rebuilding mpv.net so that it's applied at the pre-init stage), but not sure.

Let me know how it went.

avih commented 2 years ago

if you can configure input-builtin-bindings=no from config files

And, you need to remove the input-default-bindings=no too, because it still does exacty what it did before - disables both mpv builtin bindings and mp.add_key_bindings, while input-builtin-bindings=no disables only the builtin ones.

The new docs are here https://github.com/mpv-player/mpv/pull/9284/files#diff-98f92d4611fd20a294d43f0d41d9bc6f1d79986a4ce088f98aaa06dafcba6105

avih commented 2 years ago

So, I tested as follows, and it seems to work:

Test script test.js:

mp.add_key_binding("ctrl+z", function() { mp.osd_message("PLAIN BIND") });
mp.add_forced_key_binding("ctrl+Z", function() { mp.osd_message("FORCED BIND") });

So it seems that even if the config file doesn't have input-default-bindings = no then mpv.net still disables the default bindings - this would need to be changed to use the new input-builtin-bindings instead, and leave input-default-bindings at its default value (yes).

However, it's possible to set both input-default-bindings = yes input-builtin-bindings = no at the config file, which works and disables the mpv builtin bindings while allowing mp.add_key_binding work normally like in mpv.

avih commented 2 years ago

EDITED - previously I used an incorrect option name.

I noticed that from CLI using --input-default-bindings=no does work, but mpv.net --input-builtin-bindings=yes does not work (or rather, the property does show "yes", but it behaves like "no").

I think this means that you apply the CLI options after you initialize mpv? (so input-default-bindings does get applied because it can change after init, but input-builtin-bindings is applied but ignored if it's after init).

Is there a good reason to not apply the CLI options before you initialize mpv? There are also other options that only get applied during initiallization.

stax76 commented 2 years ago

I think this means that you apply the CLI options after you initialize mpv? (so input-default-bindings does get applied, but input-builtin-bindings is applied but ignored if it's after init).

Some CLI options are applied pre init:

"input-terminal", "terminal", "input-file", "config", "config-dir", "input-conf", "load-scripts", "scripts", "player-operation-mode"

https://github.com/stax76/mpv.net/blob/master/src/Misc/CorePlayer.cs#L975

There is probably are reason, but I don't really remember much.

mpv.net sets input-default-bindings only in the mpv.conf defaults here:

https://github.com/stax76/mpv.net/blob/master/src/Resources/mpv.conf.txt

I can change that setting both input-default-bindings = yes input-builtin-bindings = no and then I think we are good.

Users that already have a mpv.conf file don't get the change because the mpv.conf file is only generated when missing, that's OK.

Again, thanks for working on this, greatly appreciated.

avih commented 2 years ago

There is probably are reason, but I don't really remember much.

Right. If you do happen to remember, maybe it's worth changing from pre-init whitelist, to post-init whitelist - so that by default it's pre-init, unless there are reasons for specific options to apply post-init.

After all, in mpv itself all options are applied pre-init (and the config file mpv.conf does get applied pre-init).

stax76 commented 2 years ago

That is useful info, thanks. I had built most of mpv.net without looking often at the mpv code because it's probably not easy to set up a dev environment for debugging in order to learn how the code works, I hope I can achieve it someday, it's certainly something I'm interested in, it's going to take some time mostly because of a health issue, that will hopefully get better soon.

avih commented 2 years ago

Right. feel well, and if you need some help with building mpv, pop over to our irc channels and we'll help you.

avih commented 2 years ago

I see that you pushed a fix. Two things:

To avoid similar issues in the future, maybe you shouldn't write mpv.net defaults into the config files? I.e. apply them, and allow the config to override them, but if you modify the defaults and the user never overrides them in mpv.conf, then they'll just have the new defaults without issues.

stax76 commented 2 years ago

OK, thanks. I'm now subscribed to the pull request thread.

I changed it using shinchiro built from today.

To avoid similar issues in the future, maybe you shouldn't write mpv.net defaults into the config files? I.e. apply them, and allow the config to override them, but if you modify the defaults and the user never overrides them in mpv.conf, then they'll just have the new defaults without issues.

That is probably a better solution, thanks, I've rewritten it accordingly.

For input-default-bindings it's now difficult finding a good solution because it's already in existing conf files. I decided to enforce input-default-bindings=yes and input-builtin-bindings=no in the source code after init, that solves the backward compatibility problem and hopefully does not create other problems, I don't think it can make sense for mpv.net users to use mpv defaults.

avih commented 2 years ago

I decided to enforce input-default-bindings=yes and input-builtin-bindings=no in the source code after init,

input-builtin-bindings=no after init is ignored. Its value is only applied during init.

forcing input-default-bindings=yes is not a great idea IMO (and same for input-builtin-bindings=no, even if it currently doesn't have any effect if you do that after init). What if the user wants their mpv kb shortcuts?

I also thought about it and I agree it's not a trivial problem to solve.

One solution I thought could work for this case is to simply remove it from the user mpv.conf but only once (or replace it with some comment), and next time remember that you already removed it - and don't remove it again if the user put it there again.

For instance you can store some "removed-default-bindings-once=yes" at settings.xml, and next time, if it exists then don't remove it again?

avih commented 2 years ago

For instance you can store some "removed-default-bindings-once=yes"

And if you're already doing that for this settings, maybe take the opportunity to do it for all settings which you write to mpv.conf? i.e. make your default mpv.conf empty, store all the default values only inside mpv.net source code, and just never write default values mpv.conf again?

New users will get an empty mpv.conf.

Existing users will keep their current mpv.conf, but with all the defaults which mpv.net put there are now commented out (and mpv.net remember at settings.xml that you it did these comments, that so that you don't do that again in the future).

And settings which mpv.net didn't put there in the past remain unmodified.

avih commented 2 years ago

https://github.com/stax76/mpv.net/commit/6e761c0a4e02be191ad4199b604a3170f3a7ced5#diff-c8482043a97be5b4aced3348ffa18cfc898a44b1691682bf32f5d4d0c1f6ff6aL151

This completely removes the mpv.net KB delay/repeat value, right? If yes, this is a good change. I was wondering why you want to override the user's preferred/system KB timing settings... :)

stax76 commented 2 years ago

This completely removes the mpv.net KB delay/repeat value, right? If yes, this is a good change. I was wondering why you want to override the user's preferred/system KB timing settings... :)

Before the input.conf defaults had the following:

+          add volume  10
-          add volume -10

That has worked well with input-ar-delay=500 and input-ar-rate=20 with my PC and also with my old and my new remote control, the mpv defaults did not work at all with my remotes.

A user (hooke007) suggested the mpv default volume step 2 is fine, so I changed it back to 2. For remotes, it generally might not be ideal though.

Personally I still use volume step 10 and input-ar-delay=500 and input-ar-rate=20, it's ideal for me. It's awesome that mpv allows tweaking such things.

I'll go for a bike ride now and continue working on the input-default-bindings issue tomorrow, have a nice day! Again, I appreciate your help on it.

hooke007 commented 2 years ago

https://github.com/stax76/mpv.net/pull/272/files#diff-fd29fc106cb149d9950ead25efde66a80addbc4f55491aacac023ea85be31895R121

Not me. but no matter. 10 could be too rapid for some users.

it's going to take some time mostly because of a health issue, that will hopefully get better soon.

And Hope you recover soon!

stax76 commented 2 years ago

Oh sorry, wasn't you. I don't remember exactly why it wasn't working with my old MCE remote, maybe it did not auto repeat.

And Hope you recover soon!

Thanks

stax76 commented 2 years ago

input-builtin-bindings=no after init is ignored. Its value is only applied during init.

I've now moved it before init, the code is complex at the moment but can be simplified much once I have the updated library, it's not urgent since I work on another app anyway for like 1-2 months. It would normally be done much faster but with my condition it is difficult, I've a sleep and pain disorder, I spend much time and energy treating it with therapy etc. to get rid of the condition, and it's making progress, but only slowly. I want to get rid of KeyLauncher and use Flow Launcher instead, it involves building a plugin.

One solution I thought could work for this case is to simply remove it from the user mpv.conf but only once (or replace it with some comment), and next time remember that you already removed it - and don't remove it again if the user put it there again.

For instance you can store some "removed-default-bindings-once=yes" at settings.xml, and next time, if it exists then don't remove it again?

I've implemented it similarly, there is a check that happens only once.

And if you're already doing that for this settings, maybe take the opportunity to do it for all settings which you write to mpv.conf? i.e. make your default mpv.conf empty, store all the default values only inside mpv.net source code, and just never write default values mpv.conf again?

Yes, it is exactly like this now.

Existing users will keep their current mpv.conf, but with all the defaults which mpv.net put there are now commented out (and mpv.net remember at settings.xml that you it did these comments, that so that you don't do that again in the future).

I've only removed input-default-bindings because it's easier, the other settings should be harmless.

avih commented 2 years ago

Yes, it is exactly like this now.

Nice.

Also, the new option is now in mpv-master, so the next shinchiro build should have it.

stax76 commented 2 years ago

I've updated libmpv today, adjusted the code and made a last test, it works fine, again, thanks!

avih commented 2 years ago

I've updated libmpv today, adjusted the code and made a last test, it works fine, again, thanks!

@stax76 I'm not seeing a new beta file to test?

Also, I noticed at your last release "window-scale 1 does not work correctly in mpv, so I've removed support for it and added my own implementation"

This was working correctly, depending how you define "correct". The TL;DR is that window-scale is an option, which only has an effect when you change its value. So with a default value of 1, and you set it to 1 - nothing happens.

We do also have current-window-scale which reflect the actual window size (not the option value), and setting it to any value should work as expected, as well as reading it.

While we're on the subject, do you have a list of workarounds in mpv.net which you think should be fixed in mpv itself? E.g. you recently mentioned global media keys not working, is there anything else?

stax76 commented 2 years ago

I'm not seeing a new beta file to test?

I've only changed it locally and will upload it soon, my little launcher side project after a long time is finally done, so I've again time for mpv.net.

While we're on the subject, do you have a list of workarounds in mpv.net which you think should be fixed in mpv itself? E.g. you recently mentioned global media keys not working, is there anything else?

Thanks for asking, there is one minor cosmetic issue, hiding or replacing the OSC logo because sometimes it can be seen shortly in mpv.net, the overlay logo implementation has minor cosmetic issues, but it doesn't bother me enough to make a request or code it myself and make a pull request. It's something I will probably do sometime later.

Plugin support on Windows might be interesting for some users, for instance for even another script host, e.g. Python, that could be an interesting project for me. On that topic, I have a question, every Lua/JS script has an own message loop, right?

If that existed, then a message filter API would also help with certain plugin use cases, most UI toolkits offer such an API, mpv plugins would have access to window messages then, with that I think advanced things like a mpv plugin that implements a native context menu would be possible.

avih commented 2 years ago

hiding or replacing the OSC logo

Indeed, I noticed your logo overlay is not integrated very nice. This should be trivial to support with the logo ASS string as a script-opt. Probably less than 5 LOC.

Plugin support ...

Not sure what you mean by "plugin". Normally mpv has scripts, not plugins, though it does support (not on windows) loading a binary shared object.

every Lua/JS script has an own message loop?

There is an event loop for each script. For each script mpv starts a new thread, and in this thread it starts a new lua/js VM, and the first thing which gets loaded into the VM is defaults.lua or defaults.js (which adds some mp.* APIs and has an event loop function - which is not yet called), and then finally the user script is loaded into the VM and executed. This initial execution is typically quick (register observers, keys, etc), and when it completes the script thread calls the script's event loop. Once all scripts are waiting for events - mpv resumes its initialization (so if one script has a slow init - it stalls mpv init).

If that existed, then a message filter API would also help

Not sure what you mean by that, but it should be possible for a libmpv client to do nearly 100% of what mpv scripts can do, including registering events etc. I suggest you open a new issue if you have more questions on this subject.

stax76 commented 2 years ago

Indeed, I noticed your logo overlay is not integrated very nice. This should be trivial to support with the logo ASS string as a script-opt. Probably less than 5 LOC.

That looks like a nice Lua and Git exercise for me to do.

Not sure what you mean by "plugin". Normally mpv has scripts, not plugins, though it does support (not on windows) loading a binary shared object.

I meant C plugins on Windows.

Not sure what you mean by that, but it should be possible for a libmpv client to do nearly 100% of what mpv scripts can do, including registering events etc. I suggest you open a new issue if you have more questions on this subject.

A message filter feature could be interesting for C plugins for mpv (if there were C plugins on Windows), libmpv apps with an own window implementation like mpv.net wouldn't need. It is for interoperating with Win32 window messages. All Windows UI frameworks offer such an API:

WinForms

WPF

I don't know if Linux or Mac UI frameworks have a similar design. It's probably not hard to build, but it would only make sense to build if somebody actually needs it, for instance to make a context menu plugin, or something else window related.

There is an event loop for each script.

I've designed the extensibility (C# and PowerShell) in mpv.net without dedicated event loops, the code is simple. There are both synchronous and asynchronous events raised in the main event loop, and if a C# script or extension assigns to a synchronous event and blocks, then it's a problem. But these extension hosts don't have many users anyway, I've built it mostly just for fun, even myself favor Lua/JS when possible. If I ever do another script host, e.g. Python, I will use dedicated event loops too, in the beginning I didn't know that libmpv offers this possibility.

avih commented 2 years ago

if there were C plugins on Windows

I think the reason that it's not supported is that POSIX/linux has runtime symbol resolution or some such, so the c plugin can be compiled with only the libmpv header (client.h) with some compiler flags to resolve the symbols at runtime, use all the libmpv APIs, and once mpv loads it then it finds all the symbols from mpv itself.

While on windows it's not possible (I think neither on osx).

hooke007 commented 2 years ago

I checked the latest commit, no-input-builtin-bindings is not set as default in mpvnet. @stax76 did u miss it?

edit: my mistake. :-)

stax76 commented 2 years ago

You have to remove no-, it's here:

https://github.com/stax76/mpv.net/blob/master/src/Misc/CorePlayer.cs#L157