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

Replace AtomicParsley with lofty-rs #334

Open
miraclx opened this issue Sep 2, 2022 · 6 comments · May be fixed by #582
Open

Replace AtomicParsley with lofty-rs #334

miraclx opened this issue Sep 2, 2022 · 6 comments · May be fixed by #582
Labels
enhancement New feature or request

Comments

@miraclx
Copy link
Owner

miraclx commented Sep 2, 2022

Related: #82

In certain cases, AtomicParsley falls short. Like embedding long rDNSatom keys. wez/atomicparsley#44

In the spirit of #305, we could replace it with a wasm bundled version of https://github.com/Serial-ATA/lofty-rs

Which should allow us to support more formats beyond AAC and even export the original audio with only metadata embedded. #168

And be free of native dependencies.

@orsett0
Copy link

orsett0 commented May 16, 2023

Couldn't we just use ffmpeg? This way i think we could even embed the metadata during the encoding process

@miraclx
Copy link
Owner Author

miraclx commented May 21, 2023

Even ffmpeg uses AtomicParsley to embed metadata.

Looks like I misread, I thought you had said youtube-dl.

ffmpeg doesn't support metadata embedding into AAC containers which the mp4 format (.m4a) uses, so this doesn't work.

@orsett0
Copy link

orsett0 commented May 22, 2023

That's not entirely true. It supports all the tags currently used by freyr-js, for the exception of apID, purd, ©enc, rDNSatom and rtng, unless you specify a flag in ffmpeg which however messes with the other metadata. (*)

Also, metadata written with ffmpeg can't be read by AtomicParsley, and this makes me think that they are not readable by iTunes too. They can however be read by other players.

I don't know if you consider these metadata to be important, but if they aren't, I could make a PR in a few days implementing the use of ffmpeg to embed metadata. Otherwise I can't offer a solution, as I'm not experienced in rust nor wasm.

(*) the flag is -movflags use_metadata_tags. I don't know exactly how it works, given that I can't find proper documentation, but it renders metadata such as the track number not readable by vlc and others.

@miraclx
Copy link
Owner Author

miraclx commented Aug 19, 2023

Also, metadata written with ffmpeg can't be read by AtomicParsley, and this makes me think that they are not readable by iTunes too.

That is a point of concern for this project.

I don't have much time these days, but this is worth investigating. I will if I ever find the time. Or if you do, and can submit a PR, I'd be happy to test and review it. Reusing the embedded ffmpeg wasm binary for this would save on third-party requirements and make freyr a "download only" solution.

The help would be very much appreciated.

Otherwise, I'm pretty happy with the state of things as it is.

@orsett0
Copy link

orsett0 commented Sep 27, 2023

Hi, I was working on using ffmpeg to add metadata, but then I noticed that some tags were not recognized correctly by other music players, so I abandoned the idea.

I wanted to develop a WASM version of lofty-rs, but due to university I didn't have much time. I think I can start working on it in a couple of days and I'll be happy to submit a PR once I get things to work.

@miraclx
Copy link
Owner Author

miraclx commented Oct 2, 2023

That would be amazing, feel free to reach out if you need any help. Thank you!

orsett0 added a commit to orsett0/freyr-js that referenced this issue Oct 15, 2023
orsett0 added a commit to orsett0/freyr-js that referenced this issue Oct 15, 2023
@orsett0 orsett0 linked a pull request Oct 15, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants