Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to save cover image with options.coverPath #19

Merged
merged 5 commits into from
Feb 1, 2018

Conversation

xidb
Copy link
Contributor

@xidb xidb commented Dec 27, 2017

No description provided.

@parshap
Copy link
Owner

parshap commented Jan 16, 2018

Thanks for the contribution! I'm confused what this is doing exactly though. Is this saving a cover image from e.g., a .mp3 file to a separate file? Could you add some usage documentation to the readme?

@xidb
Copy link
Contributor Author

xidb commented Jan 17, 2018

Yeah, it's doing exactly what you described =)
I updated readme.
And also i fixed and issue i had, when multiline data like lyrics are not properly saved to output.
parseLine function assumes that line has key in format "key=data", but those lyrics lines do not have it.
So i edited it, so if line does not have key, than data should be appended to last line that has key.

That's how multi-line field looks in ffmpeg output in console:
multi-line-fields

With issue, object key created for each line of lyrics:
with-issue

Issue fixed, all lyrics lines are stored in lyrics key:
without-issue

@xidb
Copy link
Contributor Author

xidb commented Jan 18, 2018

Forgot to set -codec -copy parameter for output cover image, so it can be saved without re-encoding. I will make commit later.

@parshap
Copy link
Owner

parshap commented Jan 19, 2018

This looks great! Thanks for the docs and the multi-line fix. I'm happy to merge this once you're done.

@xidb
Copy link
Contributor Author

xidb commented Jan 31, 2018

Hey!
Sorry for a long delay. I couldn't found a way to get -codec copy working with images properly... you need to know output format to provide a correct extension, and ffmpeg does not have things like mimetype.
So i think i'm done there, i hope it's ok in that state =)

By the way, You probably have also done ffmetadata for some project where you need it and maybe you will find interesting, i have some thoughts on music metadata libs for node.

I needed a lib, which could read all meta tags from audio files, even custom, and images and write certain tags like lyrics. I found your lib first and started with it. And i started this pull request, because reading images was not supported, and because reading other fields is fine.
Then i found out, that ffmetadata removes uncommon metadata and images from files on writing lyrics. I couldn't fix it, and it was not acceptable to me. Also when you reading tags for thousands of audio files in cycle it starts leaking, a lot of orphan ffmpeg processes start to appear, and then my app just crashes. It's fine for 1000 files, and maybe 10000, but 50000 is too much.
You surely did a great job on your lib, don't think i'm just criticizing, i just have an idea for my app, that need something else.
So i started digging deeper...

I found https://github.com/vankasteelj/mediainfo-wrapper. It reads all the same tags as ffmetadata, but also reads images in base64, and also size of static binary for Mediainfo is a lot smaller, around 7Mb, because it don't need all these crazy video converting features. So i think for reading tags mediainfo-wrapper is superior. But it cannot write anything.

Finally i found node-taglib2 (https://github.com/voltraco/node-taglib2). It's a node native binding to C++ library taglib2. Unfortunately, in it's master state it cannot read or write any custom tags (like lyrics). Reading and writing was done implicitly for each field in a code, and only common tags like artist were supported. On the other hand it works fine with reading-writing images.
So i also came up with pull request... I made node-taglib2's implicit custom tags property reading into dynamic, and now it can read anything like ffmetadata or mediainfo-wrapper. I also added support for writing lyrics, which i needed the most in the first place. It could be done with other custom tags, it can be even done dynamically based on input data, but i stopped on lyrics =) ...and probably my C++ code is far from perfect, because i haven't written a line in C++ before, haha. But i plan to do dynamic tags writing later, for now it's working flawlessly for my needs.
It has one downside - you need to build it, but it kinda easier for my cross-platform app than managing 3 separate static bin files of ffmpeg.
So i think now node-taglib2 on my latest branch (i haven't got any feedback on PR) is an ultimate metadata library for node. =)

If something of it helps, i'd be glad, maybe some of my findings can spare some time for anyone.

@parshap parshap merged commit c0e1e81 into parshap:master Feb 1, 2018
@parshap
Copy link
Owner

parshap commented Feb 1, 2018

Thanks @xidb!

Your comments make a lot of sense. Yes, I created node-ffmetadata for a music organizer app. I remember coming across node-taglib and finding it promising, but incomplete. Eventually I too ended up on a taglib-based solution, going through the Python bindings via a child process call instead. Pretty roundabout, but it worked at the time.

I would be happy to add your findings to the readme so users can find the best tool for their needs. Edit: Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants