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: add script-opt --max-msg-level #11846

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

catcake
Copy link

@catcake catcake commented Jun 28, 2023

I have been writing scripts to extend mpv for a few months and eventually grew annoyed that the console did not allow trace messages, so I made an option to control this (mentioned in #11761).

If you aren't interested in using the console for trace messages (which is probably more sane), the option can still be useful for nearly the opposite reason. The console can be restricted to only important messages, while the terminal shows more granularity.

These commits are functional & already implement what's mentioned above, but I have a couple related ideas and questions:

debug and trace message styles/colors
I think they're identical, at least in the terminal on Windows (and trace wasn't relevant to the console before this). Looking at the theme that the console pulls from (here), would gui05 (#747369) be a good color for trace to differentiate it from debug messages?

a non-static default for --max-msg-level
Rather than info, when --max-msg-level is not explicitly set, maybe determine the most noisy level set in --msg-level and use that? I actually wrote this already, but I wasn't sure if it was desirable to anyone other than myself.

I've tried to be thorough, but please let me know if there's anything I should correct! Thanks!

@hooke007
Copy link
Contributor

hooke007 commented Jun 28, 2023

default value:
Why is it not "auto" (to follow the same value of --msg-level)?

@catcake
Copy link
Author

catcake commented Jun 30, 2023

Does the most recent commit address this sufficiently?

@hooke007
Copy link
Contributor

Sry i might misunderstand this option previously. I thought it generates logs independently.

if it uses the same lv from option --msg-level, actually it did nothing. We just need set it trace to share the same output.

@catcake
Copy link
Author

catcake commented Jun 30, 2023

I'm not sure that trace as the default is optimal. The original reasoning for discarding them is still valid:

mpv/player/lua/console.lua

Lines 828 to 832 in effc680

-- Filter out trace-level log messages, even if the terminal-default log
-- level includes them. These aren't too useful for an on-screen display
-- without scrollback and they include messages that are generated from the
-- OSD display itself.
if e.level == 'trace' then return end

If someone were to use --msg-level=all=trace, the console would be unusable. I think it could actually be lowered from debug (effectively the behavior before this PR) to verbose or info.

@hooke007
Copy link
Contributor

But the current commit is the same as set it "trace"

@catcake
Copy link
Author

catcake commented Jun 30, 2023

Lol, true. I'll swap back to my first commit tomorrow. It didn't bother trying to detect anything from --msg-level. Initially I used info as the default. Does this sound good, or should it be raised to verbose?

@hooke007
Copy link
Contributor

As a win-user, I always use console instead of cmd. verbose would be better.

@catcake catcake force-pushed the master branch 3 times, most recently from ae1856a to 8dd2f8d Compare July 7, 2023 01:15
--max-msg-level controls the maximum message level for the console. It
filters messages, discarding any at a higher level than its value. It
does not affect the terminal or log file outputs. It can be set to any
of --msg-level's levels (excluding status). The default is v.

Previously, the console would always discard trace messages, but show
all other messages. Now, if mpv is generating trace messages
(controlled by --msg-level), they can be shown in the console using
--max-msg-level=trace, as expected.

DOCS/console.rst has been updated to reflect this addition.

Also:
* Remove a variable that duplicated the error message style within the
  help command.
* Add a missing newline to an error message.

closes mpv-player#11761
@catcake
Copy link
Author

catcake commented Jul 7, 2023

Please excuse the mess above!

v (verbose) is now the default for --max_msg_level and the --msg-level detection has been removed!

Let me know if any if there are any other issues.

@hooke007
Copy link
Contributor

hooke007 commented Jul 7, 2023

I'm not sure if it was caused from mpv itself or this commit.
msg-level=all=trace showed much less info than --msg-level=all=v.

@Dudemanguy
Copy link
Member

Dudemanguy commented Jul 7, 2023

I think it might be silent mode flushing the log buffers so the log-message event never gets them, but it's not obvious to me why that would happen because of this commit.

@catcake
Copy link
Author

catcake commented Jul 7, 2023

I don't think the new changes are at fault.

A minimally modified (to output the messages it receives to a file) console.lua (from mpv/master) also results in many dropped messages at levels higher than info (compared to the number of messages at any level from --log-file). It seems like the buffer only stores a few dozen messages (seemingly below 100), which is probably fine if the console isn't active. When the console is active, it seems like it can't keep up with the incoming messages.

diff of mpv/master console.lua and min-modified-console.lua
--- mpv-master-console.lua
+++ min-modified-console.lua
@@ -813,6 +813,8 @@
 -- until enable_messages is called again without the silent: prefix.
 mp.enable_messages('silent:terminal-default')
 
+local console_log_file =  io.open('min-modified-console.log', 'w')
+
 mp.register_event('log-message', function(e)
     -- Ignore log messages from the OSD because of paranoia, since writing them
     -- to the OSD could generate more messages in an infinite loop.
@@ -829,7 +831,7 @@
     -- level includes them. These aren't too useful for an on-screen display
     -- without scrollback and they include messages that are generated from the
     -- OSD display itself.
-    if e.level == 'trace' then return end
+    if e.level == 'trace' or e.level == 'debug' then return end
 
     -- Use color for debug/v/warn/error/fatal messages. Colors are stolen from
     -- base16 Eighties by Chris Kempson.
@@ -847,6 +849,12 @@
     end
 
     log_add(style, '[' .. e.prefix .. '] ' .. e.text)
+    console_log_file:write(e.level .. '\t\t' .. e.prefix .. ': ' .. e.text)
+    console_log_file:flush()  -- Preserve the message order.
 end)
 
+mp.register_event('shutdown', function()
+    console_log_file:close()
+end)
+
 collectgarbage()

@catcake
Copy link
Author

catcake commented Jul 7, 2023

Using: --msg-level=all=v and min_modified_console.lua
Steps:

  1. open video (--pause=yes)
  2. wait for cache to fill
  3. activate the console
  4. quit mpv

files:

  • 1.log-file.log (mpv log, 449 lines total)
  • 1.min_modified_console.log (console log, 94 lines total)

mpv log lines <335 were presumably pushed out of the console's buffer.
mpv log lines 335-423 = console logs lines 1-89
mpv log lines 424-431 are purposely omitted from the console log (from an osd module).
mpv log lines 432-434 = console log lines 90-92
mpv log lines 435,436 are purposely omitted from the console log (from an osd module).
mpv log lines 437,438 = console log lines 93,94
mpv log lines >438 are about mpv exiting and are not present in the console log.

1.log-file.log
1.min_modified_console.log

I'll try to look into this more, particularly at how the console falls behind when mpv generates more messages than it can handle.

@kasper93
Copy link
Contributor

kasper93 commented May 7, 2024

@catcake: please rebase

@guidocella: do you have opinion on that?

@guidocella
Copy link
Contributor

guidocella commented May 7, 2024

Logging code is now very different with the addition of suggestion and terminal styles so it's up to the author if he's willing to update this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants