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

Fix #119 #120

Merged
merged 1 commit into from
Oct 23, 2016
Merged

Fix #119 #120

merged 1 commit into from
Oct 23, 2016

Conversation

ohnx
Copy link
Contributor

@ohnx ohnx commented Oct 22, 2016

Also switched to my fork of node-ffmetadata, where the issues writing covers should be fixed. (from testing, this seems to be true.)

@benkaiser
Copy link
Owner

Looks good, I'll test it out locally later today and merge if it all works fine :)

@benkaiser
Copy link
Owner

Just tested it out, works great! Thanks @ohnx

@benkaiser benkaiser merged commit fc01413 into benkaiser:master Oct 23, 2016
@benkaiser
Copy link
Owner

@ohnx can you also create a PR to node-ffmetadata with your fix? I'm happy keeping it as your fork until they accept the PR though

@benkaiser
Copy link
Owner

Also in future, you don't need to publish your fork to use it in package.json, you can directly refer to a git repo :)

@ohnx
Copy link
Contributor Author

ohnx commented Oct 23, 2016

@benkaiser The repo creator doesn't like having the flag map 0 set, since it breaks some part of their workflow.

parshap/node-ffmetadata#11 : "This would have to be an option, as in my own workflow I like to strip all extra data out of audio file and re-attach my own cover art."

I can make it an option, I suppose, and then make a PR.

@benkaiser
Copy link
Owner

I see, so does your change break for m4a files?

I'd at least recommend commenting on that issue and asking how they would like a fix implemented, if parshap wants it behind an option then we can use that option in Stretto.

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.

2 participants