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

Try add MusicBrainz tag to file #82

Open
wants to merge 43 commits into
base: master
Choose a base branch
from

Conversation

Maxmystere
Copy link

@Maxmystere Maxmystere commented Sep 25, 2021

Closes #81

Did several research and managed to do most of tag gathering

Copy link
Owner

@miraclx miraclx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed response, nice work so far..
Here's a few points. Cheers.

cli.js Outdated Show resolved Hide resolved
cli.js Outdated Show resolved Hide resolved
Copy link
Owner

@miraclx miraclx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was an error, I meant to request changes.

@Maxmystere Maxmystere marked this pull request as ready for review October 1, 2021 21:08
@Maxmystere
Copy link
Author

It seems all good now !
Maybe it could be disabled by default and enabled with a command line param

@miraclx
Copy link
Owner

miraclx commented Oct 1, 2021

Yeah. I'll take a minute to apply some final touches on my end, formatting and all, and debug the logic to ensure it checks out.
But overall, looks good! Thanks so much.

Also, yeah, we could have a --no-musicbrainz tag and a conf.json entry .opts.musicBrainz.

Provided we can guarantee a high degree of accuracy, I'd prefer to embed the tags by default if it can find them. But leave the option available for users who'd rather skip them.

@Maxmystere
Copy link
Author

For the default disabling of musicbrainz it's more of a personal preference but I think it's better to disable it by default since people who don't know it won't use it, and so it would reduce the number of calls to musicbrainz servers that aren't necessary (which is good since it's a totally free service)

@miraclx
Copy link
Owner

miraclx commented Oct 2, 2021

Valid point indeed, in that case, we'll do just that. Thanks for bringing it up.

@miraclx miraclx force-pushed the master branch 2 times, most recently from 0601faa to e695eb2 Compare October 2, 2021 21:12
@miraclx
Copy link
Owner

miraclx commented Oct 2, 2021

Played around with MusicBrainz's API for a while, refactored the code
The change extracts more data from the requests to embed in the track.

After doing a lookup on MusicBrainz Picard, I noticed it filled in some missing fields. So, I thought why not replicate that.

All the new tags:

  • MusicBrainz Track Id
  • MusicBrainz Artist Id
  • MusicBrainz Album Artist Id
  • MusicBrainz Album Id
  • MusicBrainz Release Group Id
  • BARCODE
  • MusicBrainz Album Status
  • SCRIPT
  • MEDIA (this existed before but was statically fixed to "Digital Media")

From what i found I saw that using AtomicParsley was only possible for certain tags

So, it turns out that with AtomicParsley as-is, you actually can't include rDNS data whose name contains spaces.

I've patched it on a local fork which works perfectly but then I stumbled on a memory error that leads to a segmentation fault on free. I'm currently investigating and as it turns out, long rDNS names like MusicBrainz Album Artist Id cause AtomicParsley to pull its hair out.

@miraclx
Copy link
Owner

miraclx commented Oct 3, 2021

All's good on freyr's end. But this is blocked on AtomicParsley supporting spaces in and having long-er rDNS names.

@Maxmystere, you have more expertise with C++, I suppose, if you have the time, could you equally investigate this on your end?
I've gotten it to work but the file format might be broken, I'll read through the m4a spec later in the day.

https://github.com/wez/atomicparsley

Opened an issue: wez/atomicparsley#44

@Maxmystere
Copy link
Author

Hello ! I'm back from the dead with another attempt at finding a solution to this tagging issue
Tell me if you would like any changes or if you'd prefer me to do a separate PR for format parameter support

@miraclx
Copy link
Owner

miraclx commented Aug 30, 2022

Hey man, welcome back. Can you run eslint --fix before pushing, so it's easier to track the changes.

@Maxmystere
Copy link
Author

Thanks for that command I didn't know about this one to format files

@Maxmystere
Copy link
Author

Maxmystere commented Aug 30, 2022

I think if we want a clean PR I should

  • Add a warning when putting -m without -x flac as musicbrainz tagging is not supported with default m4a
  • Maybe improve how -x is handled
  • Fix some null values I saw in async function lookupISRC

But I'm kinda lacking knowledge on how to do it properly

Is there a way to force crash on fail metadata tagging to ease development ?

cli.js Outdated Show resolved Hide resolved
@Maxmystere
Copy link
Author

I tried to sync master and it seems like I didn't break anything (hopefully)

@Maxmystere
Copy link
Author

Hello ! Resurrecting this PR to know what's left I should do to improve code before merging ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MBID tag to downloaded musics
2 participants