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

Allow frontends/GUIs to write log messages #14551

Open
stax76 opened this issue Jul 15, 2024 · 15 comments
Open

Allow frontends/GUIs to write log messages #14551

stax76 opened this issue Jul 15, 2024 · 15 comments

Comments

@stax76
Copy link
Contributor

stax76 commented Jul 15, 2024

Expected behavior of the wanted feature

Allow frontends/GUIs to write log messages, support a new input command with three parameters:

  1. Log level
  2. Module name
  3. Log message

This way errors from mpv.net and mpv.net extensions can be seen directly in the on-screen console, no need to have a terminal attached.

Alternative behavior of the wanted feature

No response

Log File

No response

Sample Files

No response

@kasper93
Copy link
Contributor

What do you mean by frontend? You can use mp_client_get_log to get log object and use that to log.

@stax76
Copy link
Contributor Author

stax76 commented Jul 15, 2024

With frontend, I mean a mpv GUI like mpv.net, which uses the client API.

@kasper93
Copy link
Contributor

So, use mp_client_get_log

@stax76
Copy link
Contributor Author

stax76 commented Jul 15, 2024

I cannot find mp_client_get_log here:

https://github.com/mpv-player/mpv/blob/master/libmpv/client.h

@stax76 stax76 changed the title Allow frontends to write log messages Allow frontends/GUIs to write log messages Jul 15, 2024
@avih
Copy link
Member

avih commented Jul 15, 2024

I cannot find mp_client_get_log here:

Good catch. I also assumed there's a libmpv api for logging, but seems there isn't.

@kasper93
Copy link
Contributor

kasper93 commented Jul 15, 2024

Oh, my bad. Our public api is prefixed with mpv_. So it seems like lua/javascript have own wrappers to expose logging api, but C API doesn't have any. We could expose mpv_client_get_log and msg.h in public headers.

@na-na-hi
Copy link
Contributor

libmpv clients are expected to be responsible to logging themselves with mpv_event_log_message. If you write your own on-screen console instead of console.lua you can display mpv and client logs.

@kasper93
Copy link
Contributor

libmpv clients are expected to be responsible to logging themselves with mpv_event_log_message. If you write your own on-screen console instead of console.lua you can display mpv and client logs.

That's also true. There is possibility to log directly to terminal form libmpv with terminal=yes, but maybe we should force libmpv users to rely on mpv_event_log_message if they want to modify/add logs.

@stax76
Copy link
Contributor Author

stax76 commented Jul 15, 2024

libmpv clients are expected to be responsible to logging themselves with mpv_event_log_message.

With that, I can only receive log messages, but I want to write log messages.

@avih
Copy link
Member

avih commented Jul 15, 2024

libmpv clients are expected to be responsible to logging themselves with mpv_event_log_message. If you write your own on-screen console instead of console.lua you can display mpv and client logs.

If I understand you correctly, you mean that clients should capture the log messages, and then output/write those to wherever they want, and while at it, also insert their own messages in between as much as they like?

If yes, then I think it's not what OP requested.

The use case here, I'd imagine, is that the mpv logging backend is used normally, for instance logging to the console or to a file, and the libmpv client wants to add log messages which clearly came from that client and not from other sources in mpv, but there's no api available to them other than running a script and then using mp.log* api from the script - which is very awkward.

I think it's reasonable to expect that libmpv should have logging api.

@na-na-hi
Copy link
Contributor

If yes, then I think it's not what OP requested.

Correct. The same callback-based logging mechanism is also used by some mpv dependencies like libass. mpv captures the logs from these libraries and displays them on its own. I'm showing that this feature request isn't strictly necessary for this reason.

That said, scripts already have access to the logging API, so I think this feature request is reasonable.

@low-batt
Copy link
Contributor

Previously I ran an experiment to see if IINA could use one combined log file. I experimented with IINA capturing all mpv log messages with mpv_event_log_message. I found it to be too problematic to use as it overloaded the event system and log messages were dropped unless it was restricted to just capturing warning and error level messages.

This is not the only log related issue for clients. See issue #12250. Every once in a while the mpv log file ends up containing a large block of NUL characters.

@kasper93
Copy link
Contributor

I found it to be too problematic to use as it overloaded the event system and log messages were dropped unless it was restricted to just capturing warning and error level messages.

Wasn't that fixed by @sfan5 in #13363?

@low-batt
Copy link
Contributor

I don't know the current status. Been quite a while since I ran that experiment. Possibly that was with mpv 0.35.1. PR #13363 was merged in January.

The documentation for mpv_wait_event says;

The internal event queue has a limited size (per client handle). If you don't empty the event queue quickly enough with mpv_wait_event(), it will overflow and silently discard further events

Events are critical to the operation of the client. Bad client behavior happens if an event is dropped. I decided it was unwise to risk overflowing the event queue just to combine log files and did not look further into this.

The problem with blocks of NUL characters in the mpv log file is very rare. I had to develop a stress test to reproduce it. It was on my mind as I recently found NUL characters in a log file again. I'd have to try reproducing it against master to know for sure if it is still a problem. At the moment I'm very busy as IINA is in the process of upgrading to mpv 0.38.0.

@sfan5
Copy link
Member

sfan5 commented Jul 15, 2024

Wasn't that fixed by @sfan5 in #13363?

that's a very specific case but could very well be what @low-batt was seeing.

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

No branches or pull requests

6 participants