stats.lua: don't use io.write from builtin script

[kasper93]

No description provided.

[kasper93]

@guidocella

[github-actions]

Download the artifacts for this pull request:

Windows

• mpv-i686-w64-mingw32
• mpv-x86_64-w64-mingw32
• mpv-x86_64-windows-msvc

macOS

• mpv-macos-13-intel
• mpv-macos-14-arm
• mpv-macos-15-arm

[avih]

I don't think it's worth the additional code and new mpv command just to avoid io.write (unless that new command does what io.write does, but using mpv logging or whatever - which it's not).

Yes, it's not very nice, but it does work, and it's simple, and there are no known issues with io.write that I know of or that you mentioned at the commit message other than "not nice", which IMHO, is not a good enough reason on its own, especially compared to the cost in complexity and code of the suggested solution.

If we keep this approach, then while mp.command("no-osd set really-quiet yes"); io.write(...) is hopefully reasonably understandable, the list of message-level commands is less obvious IMO, so a comment should probably be added about what it does and why, or maybe even move them into a function.

Also, it incorrectly clears the last output page, e.g. if the screen is 80x25, then the last 25 lines dissappear from the output.

Also, because it clears the screen and moves the cursor to the top (or so it seems), after running

mpv --idle --script-opts=stats-bindlist=yes

the screen ends up empty. One has to scroll up to see the remaining bindings list which was not cleared - which is really not nice.

Unrelated, it seems that the terminal-width argument is ignored, e.g. in

mpv --idle --script-opts=stats-bindlist=50

and instead it always adjusts to the actual terminal width.

This is definitely an improvement for the default width (--script-opts=stats-bindlist=yes), but it no longer allows manual width to be set. Additionally, the docs were not updated when this was changed, at commits 97e16be and c89b834 .

I think the best course of action with regards to this is to update the docs about the default being actual screen-width rather than 79, and restore the override functionality (which might require restoring the ellipsis function).

Also unrelated, when printed to the terminal and quitting, the title is Active Key Bindings: (hint: scroll with UP/DOWN and search with /), but the hint is incorrect in this mode.

[guidocella]

This just doesn't leave any output? The whole keybinding list is cleared is for me. Anyway the point of not using io.write would be to use the new $[term-clip-cc} which clips detecting unicode width and respecting --msg-module/--msg-time/prefixes and allows removing term_ellipsis from stats.lua, though custom width would no longer be supported, but at least clipping could still be completely disabled.

[kasper93]

@guidocella: I fixed initialization race condition and console.lua, should be fine now.

[avih]

Also fix hacky add_timeout to wait for init. It doesn't work, scripts may spend more time in init and mpv will go into idle state. Add an proper event to fire when everything is ready.

It's not hacky, it works exactly as intended, and it's intentional.

mpv waits for all scripts to finish their init phase before it starts calling all the scripts' respective event loop functions - which then starts calling registered callbacks - such as timers.

Scripts typically set bindings on their init phase, which should be quick, because otherwise it would stall mpv's init. But even if it does stall mpv, the bindings list should still wait for all scripts to init and register their key bindings, so that they can be listed by stats.lua .

Also, it's still definitely not worth adding all this code, and a new mpv event, only to avoid io.write, which while indeed is not nice, but which otherwise has no drawbacks which you mentioned in the commit message.

"not nice" is simply not a good enough reason for this PR. If you have better reasons, do mention them.

[guidocella]

It still randomly clears the whole list sometimes.

[kasper93]

It still randomly clears the whole list sometimes.

Thank you @guidocella for testing, I see there is still a way to race it.

[kasper93]

It's not hacky, it works exactly as intended, and it's intentional.

It doesn't work. We go to idle, we feed scripts, nothing is waiting for all of them, not sure where you get this idea.

[kasper93]

It still randomly clears the whole list sometimes.

Should be fixed now, I had local change in other place, sorry for that.

[guidocella]

I can confirm it's fixed.

[avih]

It doesn't work. We go to idle, we feed scripts, nothing is waiting for all of them, not sure where you get this idea.

It indeed doesn't work, but it used to, and now the docs on "script initialization and lifecycle" are incorrect.

The docs clearly states that mpv waits for all scripts to finish init before continuing. "continuing" refers to calling the event loop of all scripts, starting playback, etc.

Scripts could rely on this documented behavior in order to sync with other scripts etc, but now it seems that this doesn't work anymore, so at some stage this got broken.

Either way, the bindlist output should wait for all script to finish init, even if it takes more than few ms, in order to allow them to register key bindings on their init so that stats.lua can list them.

Also, please do mention at the commit message a reason better than "not nice" to justify all this new code and new mpv event, otherwise, if it's good enough, then it's just not worth it.

[kasper93]

It indeed doesn't work, but it used to

mpv enters idle state on wait since 5fb0594, scripts waiting were further refined in 11c573f there is no reason for scripts to block each other. mpv waits for all of them before starting playback, scripts are run asynchronously.

and now the docs on "script initialization and lifecycle" are incorrect.
The docs clearly states that mpv waits for all scripts to finish init before continuing. "continuing" refers to calling the event loop of all scripts, starting playback, etc.

Citation needed. I've read https://mpv.io/manual/master/#details-on-the-script-initialization-and-lifecycle and it clearly states what happens and at no point it tells that your script will be blocked by other scripts. It explicitly states

Your script is first run "as is", and once that is done, the event loop is entered.

And this is how mpv currently works.

Scripts could rely on this documented behavior in order to sync with other scripts etc, but now it seems that this doesn't work anymore, so at some stage this got broken.

Works as expected.

Either way, the bindlist output should wait for all script to finish init, even if it takes more than few ms, in order to allow them to register key bindings on their init so that stats.lua can list them.

Yes, that's clear. Hence the fix in this PR.

Also, please do mention at the commit message a reason better than "not nice" to justify all this new code and new mpv event, otherwise, if it's good enough, then it's just not worth it.

"not nice" and broken initialization wait is good enough for me. As for the rest, @guidocella explained why those changes were prompted in #15058 (comment)

[avih]

there is no reason for scripts to block each other. mpv waits for all of them before starting playback, scripts are run asynchronously.

they don't block eachother.

Scripts run in parallel, and so is their init phase.

It's just that mpv should wait for all scripts to finish init before continuing, i.e. mpv and scripts should be blocked after their init is complete and before all scripts finish init - and then the first event should be sent from mpv.

[avih]

"not nice" and broken initialization wait is good enough for me. As for the rest, @guidocella explained why those changes were prompted in #15058 (comment)

The commit message should explain what's broken. Not some random comment at the PR page on github.

Also, which part specifically? the ellipsis function is not used already anyway, and the output does fit the terminal width also without this PR by default.

Don't answer me. Add it to the commit message.

[kasper93]

It's just that mpv should wait for all scripts to finish init before continuing, i.e. mpv and scripts should be blocked after their init is complete and before all scripts finish init - and then the first event should be sent from mpv.

I can expose initialization event publicly. Though I don't know if there is really an use-case where this is needed. Scripts will not be driven by any events, because mpv core is dead. Only events can be triggered by other scripts or add_timeout, because it is just called on every tick.

[kasper93]

Don't answer me. Add it to the commit message.

Ok.

[avih]

I can expose initialization event publicly. Though I don't know if there is really an use-case where this is needed.

I think this could be useful to expose it publicly. There's a value in the fact that a script can know that all other scripts have finished their init - just like it has value for stats.lua. There can also be value in scripts which need to sync with other scripts, for instance sending a script-message only after all other scripts are loaded, etc.

Scripts will not be driven by any events, because mpv core is dead. Only events can be triggered by other scripts or add_timeout, because it is just called on every tick.

Not sure what this means, but if this works and has value for stats.lua, then it should work and can have value for 3rd party scripts too.

[avih]

This looks better. Thanks.

Some comments:

At the commit message of "stats.lua: wait for all other scripts to finish init correctly", there's a big block of text explaining why scripts shouldn't block, etc, with this in it:

It is currently internal because I don't want to encourage its use by scripts. Scripts should never block
and wait for other scripts during initialization...

and

If you are doing heavy processing during initialization or depend on other scripts before playback starts, you likely need to rethink your script structure

I don't think it's relevant.

Obviously it's true that scripts shouldn't block, etc, but no script was blocking before this commit, and making this event public won't encourage blocking behavior, because it's not intended to address blocking and/or slow-loading scripts at all.

It's intended to ensure all other scripts have finished their init phase.

Consider these two scripts:
script a:

mp.add_key_binding("x", function() mp.osd_message("hello") end)

script b

local x = mp.get_property_native("input-bindings")
-- do something with x

Script a definitely does everything by the book. It's not blocking more than necessary and it initializes blazingly fast.

And yet it's not guaranteed that script b will see the binding which script a added, because the script load/init order in mpv is arbitrary, and it's entirely possibe that script b init will run before script a bound the key.

In fact, without this new event, script b has no way to know where to try and grab the bindings which script a sets. It certainly shouldn't use the hacky 0-timeout method, right?

And this is what this new event allows to synchronize.

There are also other use cases where it can be useful, like with scripts which synchronize with eachother. Imagine a server script which broadcasts a message of "i'm ready to recieve messages", and it wants to do that after all other scripts are loaded, to guarantee that any client scripts will recieve this message, etc.

Or, with similar synchronized scripts scenario, a client wants to test whether there's a server script. When should it send a message to ensure that the server script is already loaded and will accept it if it exists? It can't use timeout of 0, because that doesn't guarantee to wait for other scripts. It can use timeout of 10s which will likely work, but is super ugly.

Or it could use the new event which would guarantee the correct behavior possible without arbitrary timeouts which don't guarantee anything and would likely wait more than necessary.

It's a synchronization method completely unrelated to scripts init blocking behavior (though obviously it would address such cases also).

So I don't think this whole block reasoning is relevant at all. All the stratements there are true, but theyre irrelevant as a reason to not make this public.

FWIW, I do think it would be useful to make it public still, exactly because this commit itself demonstrates how it can be useful to scripts, and it can be equally useful to 3rd party scripts. It's a great event, which is much nicer than the timeout hack, and would be really nice if 3rd party scripts would be able to use it.

The general approach was and should be to not use internal things which only internal scripts can do but 3rd party scripts can't. It's not nice at all, and it prevents one from writing their own stats.lua replacement for instance, etc.

Much work was done explicitly to ensure 3rd party clients/scripts can do as much as possible which internal clients/scripts can do (e.g. mouse position, and others), and it would be nice to keep this trend and not introduce useful features which are undocumented and unaccessible to 3rd party clients/scripts, especially when they're not hacky or fiddly.

At the last commit "stats.lua: don't use io.write from builtin script", the commit message mentions these:

Scripts and especially internal scripts shouln't bypass msg.c logging code for various resons, ranging from processing the input, filtering the log levels, truncating the output, and so on.

Mind giving an example of "processing the input"? I don't want to guess what it means.

The other two reasons are areguably not entirely relevant to the current commit, and specifically it makes it sound as if log levels should be respected here, and that it prevents terminal-truncation.

"filtering log levels" indeed doesn't happen with io.write, but this new commit adds several commands which are designed to explicitly bypass the configured log levels anyway - exactly so that it can behave roughly like what io.write does. So it's not a great reason to not use io.write when the new code bypasses the log levels anyway in the same way. Bypassing the log level is a requirement of this functionality. io.write does that trivially, and the new code does that with several commands.

"truncating the output" is interesting. I agree it's useful, but it already truncates currently anyway, using the internal term_ellipsis, and io.write doesn't prevent this truncation.

In fact, originally only the bindlist output had terminal truncation in mpv. Then it was extended to all stats.lua terminal output, as well as improved to use the actual terminal width. Then, very recently, a central facility was added to all allow it in all mpv terminal output by adding this functionality to the logging (with the respective cc char).

So what you're really doing is (rightly) wanting to remove the custom in-script truncation, now that there's new central log truncation, and io.write stands in the way towards that goal, therefore it should use logging instead, so this is what the commit message should say IMO.

And the proper course of action is to follow by removing terminal_ellipsis completely.

[kasper93]

This looks better. Thanks.

Some comments:

At the commit message of "stats.lua: wait for all other scripts to finish init correctly", there's a big block of text explaining why scripts shouldn't block, etc, with this in it:

It is currently internal because I don't want to encourage its use by scripts. Scripts should never block
and wait for other scripts during initialization...

and

If you are doing heavy processing during initialization or depend on other scripts before playback starts, you likely need to rethink your script structure

I don't think it's relevant.

Obviously it's true that scripts shouldn't block, etc, but no script was blocking before this commit, and making this event public won't encourage blocking behavior, because it's not intended to address blocking and/or slow-loading scripts at all.

It's intended to ensure all other scripts have finished their init phase.

Consider these two scripts: script a:

mp.add_key_binding("x", function() mp.osd_message("hello") end)

script b

local x = mp.get_property_native("input-bindings")
-- do something with x

Script a definitely does everything by the book. It's not blocking more than necessary and it initializes blazingly fast.

And yet it's not guaranteed that script b will see the binding which script a added, because the script load/init order in mpv is arbitrary, and it's entirely possibe that script b init will run before script a bound the key.

In fact, without this new event, script b has no way to know where to try and grab the bindings which script a sets. It certainly shouldn't use the hacky 0-timeout method, right?

And this is what this new event allows to synchronize.

Script B shouldn't do anything like that on initialization. Nor should it run any code after initialization of all scripts. This is exactly what I want to avoid by not providing a way to do that. Sure, we can theoretise scenario where this is needed, but unless you have actual use-case where this is required I won't add it. mpv as first-party takes the opportunity to listen to all scripts, 3rd-party scripts shouldn't do that. Ideally all clients should be sandboxes and don't have any access to other scripts, initialization or timing of it.

There are also other use cases where it can be useful, like with scripts which synchronize with eachother. Imagine a server script which broadcasts a message of "i'm ready to recieve messages", and it wants to do that after all other scripts are loaded, to guarantee that any client scripts will recieve this message, etc.

Or, with similar synchronized scripts scenario, a client wants to test whether there's a server script. When should it send a message to ensure that the server script is already loaded and will accept it if it exists? It can't use timeout of 0, because that doesn't guarantee to wait for other scripts. It can use timeout of 10s which will likely work, but is super ugly.

Or it could use the new event which would guarantee the correct behavior possible without arbitrary timeouts which don't guarantee anything and would likely wait more than necessary.

And they can all do that, when they need to, not during/after init. Scripts are driven by mpv events, not the other way around.

At the last commit "stats.lua: don't use io.write from builtin script", the commit message mentions these:

Scripts and especially internal scripts shouln't bypass msg.c logging code for various resons, ranging from processing the input, filtering the log levels, truncating the output, and so on.

Mind giving an example of "processing the input"? I don't want to guess what it means.

The other two reasons are areguably not entirely relevant to the current commit, and specifically it makes it sound as if log levels should be respected here, and that it prevents terminal-truncation.

"filtering log levels" indeed doesn't happen with io.write, but this new commit adds several commands which are designed to explicitly bypass the configured log levels anyway - exactly so that it can behave roughly like what io.write does. So it's not a great reason to not use io.write when the new code bypasses the log levels anyway in the same way. Bypassing the log level is a requirement of this functionality. io.write does that trivially, and the new code does that with several commands.

Any and all processing. This is general comment about bad practice. Using io.write is going out of the way to not even use print. And this makes all things that our logging system does not-relevant. Things as simple as selecting stderr vs stdout or redirecting output to a log-file. And sure as you noticed bindlist bypasses this "by design" and this is precisely the problem and why it should never exist in stats.lua. It is out-of-place there and as a results requires significant amount of doing unusual things to make it work. It should have been --help=bindlist and handled as any other help command. But this is outside of the scope of this PR and we already have this command, so I didn't just remove it completely, the support code to make it not so intrusive is not that much after all.

"truncating the output" is interesting. I agree it's useful, but it already truncates currently anyway, using the internal term_ellipsis, and io.write doesn't prevent this truncation.

In fact, originally only the bindlist output had terminal truncation in mpv. Then it was extended to all stats.lua terminal output, as well as improved to use the actual terminal width. Then, very recently, a central facility was added to all allow it in all mpv terminal output by adding this functionality to the logging (with the respective cc char).

So what you're really doing is (rightly) wanting to remove the custom in-script truncation, now that there's new central log truncation, and io.write stands in the way towards that goal, therefore it should use logging instead, so this is what the commit message should say IMO.

The future is not relevant, this PR removes io.write and add_timeout that's all it does.

And the proper course of action is to follow by removing terminal_ellipsis completely.

This is the plan.

[avih]

I'm not going to review it when you object to every single comment I make. You know best, you do best.

[kasper93]

I'm not going to review it when you object to every single comment I make. You know best, you do best.

Can I remove bindlist in this case? I'm going out of my way to make you happy. This PR is not really worth my time, arguing about irrelevant script option.

Blocked by: #15089

[avih]

Can I remove bindlist in this case?

I think it's useful to users, but I don't care where or how it's implemented. Currently it's only implemented in stats.lua, so until similar functionality is implemented elsewhere, I think it should stay.

I'm curious, don't you think it's useful to print all the bindings, including those from scripts and input.conf, from CLI, similar to how other lists are printed from CLI? (functionality only - ignoring the awkward invocation).

[avih]

Here's another idea for this changeset.

To recap, two main subjects are addressed here:

• There's an issue with "waiting for all scripts to finish init". Currently it uses 0-timeout - which doesn't wait correctly for all scripts. The suggested solution was to add an event initialized, which would it fix it perfectly, but arguably is an overkill to fix this specific non-critical issue, and also we can't seem to agree on whether it should be public or undocumented.
• Now that there's a central facility to calculate string terminal width, it's better to use it instead of the internal terminal_ellipsis. The approach which this patchset takes is to use the mpv logging facility instead of io.write, and enable this truncation at the log output.

And as a result of using the log, some additional changes were added to ensure that it can print to the output without clearing the screen on exit, such as the flush command, and some use of timeouts to workaround internal log output timing (or some such - I didn't get that part fully).

Now, this is a suggestion to make this patchset much smaller:

1. Use 250 ms timeout instead of 0 (and instead of the event). It's not very nice, and certainly less accurate than the initialized event, but it would likely cover the vast vast majority of use cases of scripts which add bindings during their init phase. Or maybe some other value, or some other trigger (i.e. not timeout) which we can use without additional infrastructure and which is still likely to cover binding-at-init. It's OK if we don't get 100% coverage, but preferably we get good coverage.
2. Add an mpv command truncate_text or text_ellipsis ot some such which can do the truncation better than the internal terminal_ellipsis (because it has wcwidth), then simply use that instead of the logging - while still using io.write. Not nice, but also not less-nice than before, but with better calculation of the truncation.

And in the future, we can still replace io.write with something else once the log output timing is less fiddly.

Or maybe take just one of those suggestions, e.g. maybe just the timeout to avoid the event bikeshed.

I also don't think this PR is bad. The only issue is the fiddly timing of the output/flush wgich I don't really understand, but I trust that it works well, so I don't have anything against it. I'm just trying to move it forward with maybe something simpler.

[kasper93]

I removed initialization thing, it is not important for this change. I will merge later today if there is no NACK.

[avih]

There's a problem with this on windows, piping the bindlist to "less":

mpv --no-config --idle --script-opts-add=stats-bindlist=yes | less -R

("less" for windows is available here)

"less" doesn't get any keypresses - like "q" to quit, or arrows to navigate, etc.

This didn't happen with io.write before this PR was merged.

Adding at the CLI: --no-input-terminal does fix it.

However, (attempt to) doing that programatically at stats.lua by adding mp.set_property_bool("input-terminal", false) together with the other initial bindlist setup (the log levels etc) doesn't fix it.

So this is a problem.

[kasper93]

This didn't happen with io.write before this PR was merged.

I can reproduce on c201c48 in cmd. Doesn't happen in pwsh or any other implementation of less. Is not related to any changes in this PR. Feel free to report issue, if you feel it is mpv fault. I will look at this closer when I have time, but likely something is conflicting with our input handling.