j-holub / Node-MPV

A NodeJs Module for MPV Player
https://www.npmjs.com/package/node-mpv
MIT License
116 stars 73 forks source link

fyi for those running into unexpected crashes #11

Closed pussinboot closed 7 years ago

pussinboot commented 7 years ago

mpv would refuse to start with this library for me more than half of the time until i added --load-scripts=no to the command line options autoload.lua does not play well with this : )

j-holub commented 7 years ago

Thanks for the notice.

Could you explain it a little further? I used this package a lot with my MusicServer project and it works like a charm like 99,9% of the time.

I noticed there are some Error messages because the socket is created before mpv successfully opened its IPC server socket, and thus the socket I create can't connect a few times. I read somewhere recently, that mpv should have something like an ipc ready message, when the socket is successfully created.

As for the Lua stuff (I never used it), it doesn't play well with --load-scripts=noor without?

pussinboot commented 7 years ago

Could you explain it a little further?

basically, once i had mpv running if i were to send the loadFile command multiple times within short succession, mpv would hang and no longer respond to any input. my guess is that if mpv was told to load a new file before autoload.lua had finished its business there would be a crash.

I noticed there are some Error messages because the socket is created before mpv successfully opened its IPC server socket, and thus the socket I create can't connect a few times.

yes, i would get those same error messages with 'debug' : true a bunch of times until the mpv process would quit and I'd have to re execute my script.

As for the Lua stuff (I never used it), it doesn't play well with --load-scripts=no or without?

without.

j-holub commented 7 years ago

I'm working on that stuff with the Error messages (and a quit Feature). Gotto figure out that ipcReady thing. So far I've only read about it in an Issue, that was closed with a commit. But I haven't found anything in the documentation yet.

As for the issue I just did the following

bildschirmfoto 2017-02-06 um 22 29 49

and it worked without a problem. It printed all the foos and started playing the file. Could you give me some instruction on how to reproduce the issue?

What OS are you using by the way? (I assume Linux)

If people need Lua Script support --load-scripts=no does not seem like a good fix to me, more a small hack.

pussinboot commented 7 years ago

I'm on Windows (lol, ik), loading different video files from user input so maybe try loading a different video file with a short time delay in between? The key to the issues i was experiencing had to do with the fact that mpv was using autoload.lua, so maybe on startup a race condition was happening of can autoload find all of the files in the working directory before starting up the ipc socket.

are the ipcReady issue/related commit these ones? if i understand correctly, then if you have the MPV_VERBOSE flag set (eg by adding the -v to commandline options), once the socket is opened up it will print to console "Listening to IPC socket/pipe" so you'd want to monitor the stdout of your mpvPlayer that came as a result of spawn before setting up your socket.

i agree not loading scripts is a bit of a hack but it made sense to me when that fixed the problem since i didn't want my default mpv configuration for my use case, idno maybe add a line or two in the readme warning people about unexpected behavior that could result from non-standard mpv configuration

j-holub commented 7 years ago

I just tried the same with a video and it worked as well.

I haven't used Windows in about 5 years, but I could try it in a VM or ask a friend. I was surprised anyway that this library works on windows because of the Unix sockets. But if you need the right socket command it seems to work.

Yes these issue / commit is exactly the one I was talking about. I'm starting mpv with the --quiet option to prevent console output. I think it would cause the stdout error to overflow at some point and the program would crash (I had that issue years ago with a python program). That's why I didn't see the ipcReady output. But there should be a way to quiet mpv after starting it. That would be a nice way to detect when it is ready and then start the socket connection. That would get rid of all the Connection Refused error logs in the beginning.

I think what should be investigated is, wether this is a problem of the way my library communicates with mpv or a problem of mpv itself (and might have to be fixed on their side).

What mpv player version are you using? And are you able to load music files? Or do you experience the same issue with those? I'm sorry that you have so much problems with my library, it was actually intended to make using mpv with NodeJs easier, not more difficult :)

Do you know of any other persons having this issue? You can add the --load-scripts=no options when launching mpv with new mpv({}, ['--load-scripts=no']);. But if this is a common issue I could of course add some options, that does is automatically.

What would you suggest?

pussinboot commented 7 years ago

hey, i don't think this is a problem with your library or even necessarily mpv. it's an edge case that may not have a true solution other than the aforementioned workaround which i am totally okay with. i'm not having any problems with your library, it's great and it's the only one i've found that does truly work cross-platform. i wrote my own ipc interface for mpv in python but there are no universal ipc packages so integrating what i'm working on with your library saved me a lot of effort, thank you.

to get rid of your Connection Refused errors try adding this flag --msg-level=ipc=v, it will only send on creation of ipc socket/errors related to ipc and keep everything else quiet : )

i was able to recreate the error with music files once i got to more than around 50 files in the directory. (aside: i actually discovered a bug in win 10 while investigating all of this lmao) i'm on mpv 0.21.0-git-e6b85c9, with an up to date copy of autoload.lua i don't know of other people having the same issue so i don't think you need to change default behavior

j-holub commented 7 years ago

Wow thank you very much, that is actually really valuable information. Do you happen to know where I can read more about the --msg-level=ipc thing? I don't find it in the official documentation on mpv.io

bildschirmfoto 2017-02-08 um 13 57 08

I will try to take the time to refactor that stuff with starting mpv and so on. And finally implement the quit feature (It's actually lingering in a local branch for like 3 month on my Laptop already, but I'm not satisfied with it yet)

To actually address the Issue I would suggest, to add some notice in the Known Issues with your solution to the problem. What do you think?

If we go down that way, I think it'd be best if you could formulate that text, since you obviously know the issue best. Could even be down with a Pull Request if you want to.

pussinboot commented 7 years ago

where I can read more about the --msg-level=ipc thing?

I figured it out from looking at the source code and just testing commandline options, search here or here for MP_VERBOSE

quitting works fine for me when I send mpvPlayer.command("quit") ? I'm more looking forward to the ipc socket creation fix : ]

I've added a subsection to the known issues and cleaned up the language in pull request #12