spawn arg --o=- prevents start() from resolving.
twilson90 opened this issue · 20 comments
Bug Description
I'm trying to output to a file or a pipe using --o=- and it doesn't work.
mpv.start() never resolves, as a result any method results in 'MPV is not running' errors.
I had a look at the source code and quickly realised it's due to the way node-mpv checks for the 'Listening to IPC socket'.
When --o is piping to stdout, mpv uses stderr for messages.
This is quite easily fixed by checking both stderr and stdout for the IPC Socket messages.
Software Versions
- Node-Mpv: Version 2
- MPV: mpv 0.31.0-67-g63bf362d11
--out is not a supported option by mpv. It crashes but does not yield any error message, which is something I should work on.
Apologies, I meant --o
Updated the issue.
No problem at all, could've happened to me too :)
I added the option --o=foo.mp3 and it actually worked in my case without changing anything in the code. The only difference I see, is that I'm using mpv 0.32.0 which is one (let's call it major) version ahead of the one you're using. Is there any reason that you cannot use the latest version?
I will see if I can build or get a compiled version 31 and I will check if it does or does work in my case
It's specifically when you pipe to stdout with the arg --o=- (that's what the dash means)
When you pipe the video output to stdout, mpv cleverly knows to use stderr for writing logs and such.
But node-mpv only ever checks stdout to see if the IPC socket has initialized, so it never resolves.
I fixed this by simply listening on both stdout and stderr in _startStop.js:
var stdCallback = (data) => {
// stdout output
const output = data.toString();
// "Listening to IPC socket" - message
if(output.match(/Listening/)){
// remove the event listener on stdout
this.mpvPlayer.stderr.removeAllListeners('data');
this.mpvPlayer.stdout.removeAllListeners('data');
resolve();
}
// "Could not bind IPC Socket" - message
else if(output.match(/bind/)){
// remove the event listener on stdout
this.mpvPlayer.stderr.removeAllListeners('data');
this.mpvPlayer.stdout.removeAllListeners('data');
reject(this.errorHandler.errorMessage(4, 'startStop()', [this.options.socket]));
}
};
this.mpvPlayer.stdout.on('data', stdCallback);
this.mpvPlayer.stderr.on('data', stdCallback);
My code to fix this looked pretty much the same as your's, but it also crashed. So I tried to just start mpv from the command line using mpv somefile --o=-. However, it crashes because it doesn't find the encode format. Am I missing some encoding library?
This is what I use:
mpv "path/to/video" -keep-open=yes --merge-files --of=mpegts --ovc=libx264 --ovcopts=profile=main,preset=veryfast,level=4,crf=20,maxrate=5000k --oac=aac --oacopts=ac=2,b=160k --o=- | ffmpeg -i - -c copy out.mkv
Notice I pipe the output into ffmpeg (which I use to stream it to an RTMP server), but outputting to a file works as well.
PS - If you're wondering what I'm trying to do, I'm using mpv on a remote server to stream a dynamic playlist to a streaming service, and controlling this mpv instance with node-mpv via a web-interface.
I tried using VLC at first but it was waaay too buggy. MPV works much better.
node-mpv was a godsend for me, many thanks for designing and maintaining it :)
start() does resolve in my case, however the file is not encoded into out.mkv, but I receive some file called - | ffmpeg -i - -c copy out.mkv. How do you pass the pipe option to Node-MPV?
And thanks for the kind words, glad it helps you out. I didn't do a good job maintaining it throughout the last year however. I'm currently between university and working, so I got some free time and I try to clean up the library as much as possible, hopefully leading to an official Version2 release.
I should have put it in node js terms. Apologies for the constant back and forth.
This code should run as expected:
var mpvArgs = ["--keep-open=yes", "--merge-files", "--of=mpegts", "--ovc=libx264", `--ovcopts=profile=main,preset=veryfast,level=4,crf=20,maxrate=5000k`, `--oac=aac`, `--oacopts=ac=2,b=160k`, "--o=-"];
var mpv = new mpvAPI({"debug":true, "verbose": true}, mpvArgs);
var ffmpegArgs = ["-i", "-", "-c", "copy", "-f", "flv", "out.mkv"];
ffmpeg = child_process.spawn("ffmpeg", ffmpegArgs);
mpv.mpvPlayer.stdout.pipe(ffmpeg.stdin);
await mpv.start();
// never resolves in current version of node-mpv
It worked in case if I switched the line mpv.mpvPlayer.stdout.pipe(ffmpeg.stdin) with await mpv.start(), because before you call start(), mpv.mpvPlayer is undefined.
If I use the following code, it works without changing anything in the Node-MPV code.
const mpvArgs = ["--keep-open=yes", "--merge-files", "--of=mpegts", "--ovc=libx264", `--ovcopts=profile=main,preset=veryfast,level=4,crf=20,maxrate=5000k`, `--oac=aac`, `--oacopts=ac=2,b=160k`, "--o=-"];
const mpv = new mpvLib({"debug":true, "verbose": true}, mpvArgs);
const ffmpegArgs = ["-i", "-", "-c", "copy", "-f", "flv", "out.mkv"];
const ffmpeg = child_process.spawn("ffmpeg", ffmpegArgs);
await mpv.start();
mpv.mpvPlayer.stdout.pipe(ffmpeg.stdin);
mpv.load('DJI_0116.mov')Apologies again, the code I posted was an ersatz version of my actual code, and I managed to screw it up (again!)
mpv.mpvPlayer.stdout.pipe(ffmpeg.stdin) does come after await mpv.start() in my original code, otherwise I would just get a simple null reference error.
The fact remains, if I run my code or your nearly identical code without the previously mentioned fix, mpv.start() never resolves.
Judging by the screenshot, it looks like you're running Linux. I'm on Windows. I wouldn't be surprised if we're seeing slightly different behaviour because of this.
For me, I have to listen to MPV's stderr when initializing the IPC socket if I have --o=- in the commandline args, and it doesn't matter when or if I hook the ffmpeg pipe up.
I haven't checked, but I would assume my mpv's stdout is outputting nothing but video data.
Really no worries man, stuff like that happens :)
I'm on MacOS, but that's really close to Linux for the most part. Unfortunately I don't have access to a Windowsmachine and to be honest, it's a problem that I never really tested this lib on Windows.
Ususually I'd say it's no big deal to include the stderr line, it wouldn't have any affect on Linux or MacOS and it would make it run on Windows. I'll have to see if I'll need the stderr for real error messages. Because when I tested your code with --out=-, Node-MPV crashed without any error message, which is not good obviously.
I'd still recommend implementing this code, with the more specific string matching in the latest version.
I can't see it causing any problems on other systems.
Hmm, with the new then block in _startStop checking for the 'idle' event a few times it wasn't resolving...
However, I tested it a bunch more times and I haven't had that problem again. Bit concerning though.
Yeah I talked a little bit more to the guy maintaining mpv and it seems waiting for the idle event might still cause race conditions, so there will be a small change probably today.
As for the code, it could cause problems. Because I want to listen to stderr to check for errors why mpv could not be started. If I listen to stderr to see if it has started, because it some scenario on windows stdout is output on stderr I cannot do that anymore. That's why I first want to investigate if there is a need to listen to stderr for actual errors.
I think you're possibly confused, there's nothing that prevents you from listening to stderr more than once.
stderr is an event dispatcher and can have many listeners.
btw I figured out why listening for the 'idle' event wasn't firing sometimes. When I was testing I was placing breakpoints after spawn but before the socket listener was set up, between which the idle event was sent but there was no socket to listen.
Also, I've just discovered if you try to pass input files in the mpv startup args, the 'idle' event will never fire... so start() never resolves.
I've changed the code so instead of just checking for 'idle' events, it now also checks for 'file-loaded' events.
Let me know if this is good and I'll update my hodge-podge pull-request to include this improvement as well.
I‘ll have a look into it. Checken for file-loaded seems weird because you‘re not loading any file when you‘re starting mpv. Is that event fired even if no file is being loaded?
It’s weird that the further input arguments would overwrite the —idle options. Could you give me a list of your input arguments that cause the error? Or are they the same as above?
Edit: My input arguments (in context of node-mpv) look like this:
new mpvAPI({"debug":true, "verbose": true}, ["file1.mkv", "file2.mkv"]);
Just to clarify, idle is not being overridden, this is the correct behaviour. In the documentation on --idle:
Makes mpv wait idly instead of quitting when there is no file to play
So when there is a file to play (ie, a file that has been given as a command line argument), it immediately loads the file and never fires an 'idle' event.
This is a problem with the new code you comitted as start() never resolves in this scenario.
It will however fire a bunch of events when the first file is loaded, and the first of these is 'file-loaded', so it made sense to use this to detect when to 'start' mpv.
But if no files are passed, we can resolve the promise when we detect an 'idle' event )and much quicker I might add).
PS - If you want to load a file via the commandline and prevent it from playing, you can add the --pause argument.
Well, the library was never intended to be used in that way so I don't really see it as a big problem. The fix to this doesn't really interfere and I don't see it causing any debugging problems in the future, so I might as well add it. Better safe than sorry.
Still, I never thought of this scenario because it was more intended to be used like this
mpv.start({
"debug": true,
"verbose": true
})
.then(() => {
mpv.load("file1.mkv");
mpv.load("file2.mkv", mode="append");
});Anyway, doesn't cause problems to add the fix I guess :)