msg: print all messages to stderr

[kasper93]

This change allows stdout to remain free for encoder output or similar tasks like vo=tct.

Before commit 4939570e17, all messages were printed to stdout, except status messages, which were printed to stderr. This approach was reasonable because one could redirect the stream to a file with messages or status output.

After commit 4939570e17, however, some messages started being printed to stderr, while others continued to be output to stdout. This inconsistency involved two output streams printing related messages.

After this change, messages are printed to stderr always, keeping stdout open for all other tasks.

This adjustment also fixes the issue of status messages being printed to both streams after commit 82451bdf04.

[kasper93]

@sfan5 @Dudemanguy : This probably should be included in release, splitting messages into two streams makes more issues than it is worth.

[github-actions[bot]]

Download the artifacts for this pull request:

Windows

• mpv-i686-w64-mingw32
• mpv-x86_64-w64-mingw32

macOS

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

[DanOscarsson]

I am not sure this is a good idea. For example I might do a mpv | grep XYZ to only see a special type of lines. As mpv is a tool that normally outputs data to a vo (not the terminal) and uses the terminal for information, all output should go to stdout except real errors. That is the normal linux way. And if you do a pipe so stdout or stderr is not a tty, screen commands like for positioning or colours should not be output. It should probably be ok to never include screen commands in the stderr stream. So you only need to know if output is terminal for stdout. If you want to use stdout for things like vo=tct or a encoding to stdout, you should either automatically identify that this is so and redirect all messages to stderr (or not at all) or you could have an option for user to select where messages should go with default being to stdout. I assume the absolute majority of users of mpv do not use vo=tct or encode to stdout.

[kasper93]

For example I might do a mpv | grep XYZ

Then you grep stderr, what is the problem?

to only see a special type of lines.

This ship has long sailed in 4939570e17d4659944924ed6012793b433109ab1 making it impossible to do what you want, because you need to grep both streams at current master.

And if you do a pipe so stdout or stderr is not a tty, screen commands like for positioning or colours should not be output.

They does not output.

If you want to use stdout for things like vo=tct or a encoding to stdout, you should either automatically identify that this is so and redirect all messages to stderr (or not at all)

See the discussion in https://github.com/mpv-player/mpv/pull/11870 why this doesn't work.

I assume the absolute majority of users of mpv do not use vo=tct or encode to stdout.

But what's wrong with using stderr?

[sfan5]

This probably should be included in release, splitting messages into two streams makes more issues than it is worth.

which kind of issues?

[kasper93]

which kind of issues?

Cursor repositioning is not working correctly, because it calculates the position based on the content that we print. And half of messages could redirected to not be printed to terminal. So you can create test scenario where it breaks. Either we print all in one stream or add a check everywhere if both stderr and stdout is printed to tty (same one). Also the streams in general can desync, so we also would need to add fflush before printing new message in case something else were printed without flushing. It is all fixable, but I don't think the added complexity is worth when only gain is having some messages on different stream (it was changed in this release cycle by 4939570e17d4659944924ed6012793b433109ab1)

And in general I think all messages should be on one stream, either stdout like before or stderr, spiting them serves no purpose. Makes only harder to grep or redirect them.

EDIT: also the issue is on windows, because the on linux we have working terminal in background check

[sfan5]

Splitting error messages from "normal output" via stderr/stdout matches the convention on Unix-like systems, so I don't think it's bad in principle.

Where do we do cursor positioning that relies on sync between stdout and stderr? for stuff like vo_tct?

desync is unfortunate but usually only happens in one direction (by default stderr is line buffered, stdout isn't I think?)

If we want to put all stuff back onto stdout like it used to be I'd prefer the solution for encoding from here:

So this commit is trying to change the right thing. fflush won't work though. You'll still end up with garbage data in the stream. The previously mentioned fseek(stdout, 0, SEEK_SET) works however and is logical.

Not sure if you're still interested in this, but I would suggest adding that in mp_msg_force_stderr, changing that function to a bool, doing error checking on the fseek call, and then failing completely if that errors out for any reason.

(why did we do that from the beginning?)

[Dudemanguy]

The fseek trick wasn't used because it's not foolproof (e.g. if the stdout location was a pipe).

[kasper93]

Where do we do cursor positioning that relies on sync between stdout and stderr? for stuff like vo_tct?

For status line printing. More pronounced after clearing status line, the issue was there before, but arguably now it is more annoying. Also just for the record, redirecting one of the stream and not the other is also a problem.

[forthrin]

curl https://patch-diff.githubusercontent.com/raw/mpv-player/mpv/pull/12924.patch | git am -3 - now gives conflicts. When I resolve them to either "ours" or "theirs", I get compilation errors. Little help?

./common/msg.c:255:30: error: subscripted value is not an array, pointer, or vector
        if (log->root->isatty[STDERR_FILENO])

[kasper93]

@forthrin: Rebased, you can use it again.

[forthrin]

Now compiles, and brings back the cursor. THX!

[kasper93]

I know this change is hot potato, but we need to fix it one way or another. I still think and propose to use stderr for message stream and stdout for encode/tct/kitty output. Alternatively it is awkward to handle our message stream, where we print similar messages assuming that both are to the same terminal.

[Dudemanguy]

If split streams are too much of an issue, we could go back to stdout and just do the fseek trick for encoding mode and accept that it's just the best we can do.

[kasper93]

If split streams are too much of an issue

Supporting split streams just makes everything more complex for no apparent gain.

we could go back to stdout and just do the fseek trick for encoding mode and accept that it's just the best we can do.

It really is forcing both things on one stream. Maybe we could have --terminal-msg-stream to add an option where should we print logs.

We already print most messages to stderr, while keeping some on stdout. Except from scripts parsing mpv command line do you see advantage of this split? The real question is what advantage it has compared to single stream. Maybe if I understand better the requirements the better solution can be implemented.

[kasper93]

@Dudemanguy: actually we can just force stderr early if needed, pre-parse cli options are designed to do so. What do you think?

[Dudemanguy]

Nice, this is probably the best solution. #8608 is still fixed with this change. The only thing I am not sure about is moving the status line. Maybe just keep it on stderr?

[kasper93]

I would like to keep all msg output in one stream, that includes status line.

EDIT: This basically is required to correctly count lines to clear and so on, else other solution to check if we output to the same terminal would be needed.

[Dudemanguy]

Looks like statusline on stderr started here: https://github.com/mpv-player/mpv/commit/020a954b60fccb15711d8e5b26a71e54bc6050c7. Not really sure I understand the justification?

[kasper93]

Probably to avoid having status line spam when in less, but might as well use --log-file in this case, which is better solution.

[kasper93]

I'm merging this, it is improvement over current status quo. If we get requests for status line on stderr, I will need to do more changes to support it correctly, so let's take baby steps.