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

.load and multiple options #41

Closed AxelTerizaki closed 4 years ago

AxelTerizaki commented 6 years ago

Hello :)

I'm not sure if it's a mpv bug or a node-mpv one, but here comes my issue :

I wanted to load a file (with the load method) with multiple options. When using just one option it works perfectly (For example ['replaygain-fallback=-10']) but if I add more options to that array, mpv doesn't load the file.

The options seem compatible as I can use them when lauching mpv from the command-line.

Options I wanted to add into the array for example :

options.push(`external-file=${defaultImageFile.replace(/\\/g,'/')}`);
options.push('force-window=yes');
options.push('image-display-duration=inf');
options.push('vid=1');

I haven't tested with command/freeCommand as I'm not too sure of how to format the thing properly though.

Any ideas?

j-holub commented 6 years ago

Hey there :),

if it works from the command line it's likely a Node-MPV bug. But at the same time it sounds like one that should be easily fixable (and important to fix).

Am I correct that you would call it like this?

player.load('file.mkv', 'replace', [`external-file=${defaultImageFile.replace(/\\/g,'/')}`, 'force-window=yes', 'image-display-duration=inf', 'vid=1']

and alternatively with an array object.

I think they way you would do it with the command (just checked the mpv documentation here and here (you have to scroll down to loadfile).

let command = {
    'command': [
        'loadfile',
        'file.mpv',
        'replace',
        `external-file=${defaultImageFile.replace(/\\/g,'/')}`,
        'force-window=yes',
        'image-display-duration=inf',
        'vid=1'

    ]
};
mpvPlayer.commandJSON(command);

I am not 100% sure though =/ I haven't really used these methods either, I just implemented them because they were pretty straight forward to implement and expand the modules possibilities to pretty much everything mpv can do. (If you miss anything as a separate method let me know).

I will have a look at it :) You're using Version 1 if I recall correctly, right?

AxelTerizaki commented 6 years ago

Nope, version 2 since I needed the promise methods.

I'll check witht he commandJSON method and see if it works.

AxelTerizaki commented 6 years ago

Strangely commandJSON throws an error at me (can't see to catch the message? err is empty)

The mpv logfile doesn't say anything (as if it never received the command)

Here's what I used :

player.commandJSON({
            'command': [
                'loadfile',
                mediaFile,
                'replace',
                `replaygain-fallback=${mediadata.gain}`,
                `external-file=${defaultImageFile.replace(/\\/g,'/')}`,
                'force-window=yes',
                'image-display-duration=inf',
                'vid=1'
            ]
        });

Note, when I tried it with the load method :

let options = [];
        options.push(`replaygain-fallback=${mediadata.gain}`) ;
        const defaultImageFile = resolve(conf.appPath,conf.PathTemp,'default.jpg');

        if (mediaFile.endsWith('.mp3')) {
            const id3tags = await getID3(mediaFile);
            if (!id3tags.image) {
                options.push(`external-file=${defaultImageFile.replace(/\\/g,'/')}`);
                options.push('force-window=yes');
                options.push('image-display-duration=inf');
                options.push('vid=1');
            }
        }
        await player.load(mediaFile,'replace', options);

It doesn't throw an error at me, but in the mpv log file I can see this :

[ 7.061][e][ipc_0] Command loadfile: has only 3 arguments.

As if it didn't properly detect all the options I passed.

The code I pasted allows my app to detect if, in case of a mp3 file, the id3 tags do not have an image file, to provide an external cover art (default.jpg) to be displayed instead. If it's a video or a mp3 file with embedded cover art, it displays just fine (as options only gets one item in its array, replaygain-fallback)

BTW, my app got its english website recently as we're about to release v2.1 : http://mugen.karaokes.moe/en/ Thought you might want to check it out since it uses node-mpv :)

j-holub commented 6 years ago

Sorry for the late response, I've been very busy with university. I'll investigate the issue now.

I checked out your website and it looks amazing. So proud to indirectly be a part of this 😃 I'll have my eye on it and I'll check it out for sure. I speak Japanese and I sometimes watch Animes (like them, just don't watch em too much) ^^

j-holub commented 6 years ago

So the issue lies within Node-MPV because it formats the JSON command wrong. The problem is, I have no idea what the correct way would be and the MPV documentation doesn't really help me.

I opened an issue about it here. Let's hope we get a good answer there.

AxelTerizaki commented 6 years ago

Great! I see you already have a response too.

Sorry for bringing out the bugs to light. We're using mpv in some pretty advanced ways (and I still have some crazy ideas... :p ) which can push node-mpv a little far.

j-holub commented 6 years ago

Don't worry, you found bugs I didn't even think of were there which ends up improving the module for everyone. I'm actually really thankful. If you find more or have suggestions, throw 'em in anytime :)

j-holub commented 6 years ago

Fixed in both version 1 and 2.

Found another bug on the way, if you use append in Version 2 with append-play and there are still songs in the playlist it doesn't resolve the promise. I have to look into that.

AxelTerizaki commented 6 years ago

I'm adding to this because I just had an issue when trying to use a lavfi-complex option : mpv fails with the message "loadfile has 3 arguments" (or something like that)

Here's the issue in question :

https://lab.shelter.moe/karaokemugen/karaokemugen-app/issues/349

I tried specifying the lavfi-complex option like I mentionned on the command-line example, but it didn't work out and displayed this error, making me wonder why it fails, while another instance where I use lavfi-complex works :

videofilter = `lavfi-complex="movie=\\'${qrCode}\\'[logo]; [logo][vid1]scale2ref=${QRCodeWidth}:${QRCodeHeight}[logo1][base];[base][logo1] overlay=${posX}:${posY}[vo]"`;
j-holub commented 6 years ago

Hey there, haven't worked on this project at all the last 2 month although I should have. Super sorry about that. I'll take my time soon to fix all issues and I'll also look into this. As always thanks for the issues :)

AxelTerizaki commented 6 years ago

No worries about that :)

I just ran into a very weird problem now.

I don't know since when or how, but it seems a recent change to node-mpv's codebase cause load to not behave like it used to.

Let me explain :

We display a wallpaper when our app is in standby, with a QRCode in the upper left corner to allow people to connect to the app more easily. I've got report from users that the QRCode disappeared at some point during our releases (but everyone thought it was us who deliberately removed it :D ).

The problem is, on my own setup, the QRCode is still displayed.

On a similar setup, another contribuer can't see it.

We display it via the load command's third option :

videofilter = `lavfi-complex="movie=\\'${qrCode}\\'[logo]; [logo][vid1]scale2ref=${QRCodeWidth}:${QRCodeHeight}[logo1][base];[base][logo1] overlay=${posX}:${posY}[vo]"`;
player.load(backgroundFile, mode, videofilter);

This usually does the trick.

However, when I look at mpv's log, here's what I find on my system (where it works) :

[   0.019][d][cplayer] Run command: loadfile, flags=0, args=[D:\dev\karaokemugen-app\app\temp\default.jpg, replace, lavfi-complex=movie=\'D:/dev/karaokemugen-app/app/temp/qrcode.png\'[logo]; [logo][vid1]scale2ref=192:192[logo1][base];[base][logo1] overlay=28:16[vo]]

However, on a system where it does not work, here's what we get :

[   0.476][d][cplayer] Run command: loadfile, flags=0, args=[J:\karaokemugen\app\temp\default.jpg, replace, =]

We're on the same mpv version (0.29 here, but it seems to behave incorrectly with 0.27 as well)

I sadly am not sure of how more I could help. I could send my node_modules/node-mpv directory perhaps to see which version I have and since when it's not working ? Since we're on a branch and we're all pulling the latest version everytime, it's hard to tell which version we're at.

AxelTerizaki commented 5 years ago

Just a +1 to remind you if you could reopen this issue? This is not fixed and I fear it might fall into oblivion if it remains closed, when you'll get more time to look into it.

What's new is that on my system, it now doesn't work anymore (I must have pulled a more recent version of the codebase)

AxelTerizaki commented 5 years ago

Since I had a bit of time in front of me I did a little research and backtracked down the commits in the Node-mpv2 branch.

So I found the culprit :

https://github.com/00SteinsGate00/Node-MPV/commit/a8bbcb5de07f868e9686de706780e77af83e9302

Past this commit, the option isn't passed to mpv as it was before, resulting in an error.

AxelTerizaki commented 5 years ago

I looked at the code and saw what was wrong.

Options seem to be formatted before being sent past this commit. However, it just splits the string into multiple options via the character =. However, the string I'm sending contains several equal signs, even though it's one, single command.

I have an idea, I'll submit a PR shortly.

j-holub commented 5 years ago

Thanks a lot for taking the time to investigate the issue and that you already found a fix. This helps me a lot, believe me I really appreciate it. Thank you.

AxelTerizaki commented 5 years ago

No problem, it was an easy fix and it was bugging me so much I had to fix it :D