search

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

Fixes formatOptions in case several = characters are present (refs #41) #50

Closed AxelTerizaki closed 4 years ago

AxelTerizaki commented 5 years ago

TL;DR :

When using a lavfi-filter option on mpv, you need to pass several sub-options. These sub-options are also defined by = character, which means a simple split by = wouldn't work.

Instead, I replaced that split by a split on only the first =.

Here's how the option string looked initially, you'll understand easily why it didn't work with the former split line :

lavfi-complex=movie=\'D:/perso/karaokemugen-app/app/temp/qrcode.png\'[logo];[logo][vid1]scale2ref=192:192[logo1][base];[base][logo1]overlay=28:16[vo]

Note that I had to remove " from my initial option line for it to work. Before https://github.com/00SteinsGate00/Node-MPV/commit/a8bbcb5de07f868e9686de706780e77af83e9302 it worked with this :

lavfi-complex="movie=\'D:/perso/karaokemugen-app/app/temp/qrcode.png\'[logo];[logo][vid1]scale2ref=192:192[logo1][base];[base][logo1]overlay=28:16[vo]"

because the string was sent as is, and not formatted into JSON (it didn't go through util/formatOption)

I don't think there would be any side effects, and it allows formatOptions to work with more complex options in mpv.

j-holub commented 5 years ago

Hey man, super sorry to have kept you waiting for so long. I'm currently living in Japan to work on my Master Thesis.

I gotto check my local changes an see how I can merge this stuff cleany, haven't worked on the project for a while so it will take a while to get back into it. I have a lot of changes locally I haven't pushed yet. Version 2 is almost ready but the last bits and pieces are very difficult.

AxelTerizaki commented 5 years ago

Hey, good to see you're still alive :)

I'm using this fix on a forked version of this repo and have not seen any problems so far.

AxelTerizaki commented 4 years ago

Ack, I just realized this PR is referencing the wrong branch now. I'll have to fix that.

j-holub commented 4 years ago

Just let me know when you have fixed it and I'll about merging it :) I started working on the project again, took a little to get into my own code and changes again