miraclx / freyr-js

A tool for downloading songs from music streaming services like Spotify and Apple Music.
https://git.io/freyr-js
Apache License 2.0
1.53k stars 95 forks source link

Replace AtomicParsley with @orsetto/meta-writer #582

Open orsett0 opened 11 months ago

orsett0 commented 11 months ago

@orsetto/meta-writer uses the library lofty-rs to write metadata to audio files.

It's currently only tested with mp4 and mp3, but it can easily be extended to the other formats supported by lofty-rs.

Fix #334

miraclx commented 11 months ago

Hi @orsett0, thanks for taking the time to look into this..

I tried to look at and audit the code for @orsetto/meta-writer and I couldn't find the source for the wasm blob.. Could you point me to a reference? Do you intend to open-source this?

miraclx commented 11 months ago

PS:


$ npm ci
npm ERR! `npm ci` can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.
orsett0 commented 11 months ago

I couldn't find the source for the wasm blob

Right, sorry. I forgot to make the repo public. Here it is now: meta-writer. I'm writing a documentation for the package, when I'll publish the new version there will be a link to the repo in the nodejs page.

PS:

$ npm ci
npm ERR! `npm ci` can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.

I'm not very familiar with npm. The solution is to push the new package-lock.json, right? If so, the new commit should fix this.

I also removed some functions that were needed for AtomicParsley and which were reported by codefactor as unused. CodeFactors also reports a lot of Insert `··`, is there an easy way of solving these errors?

miraclx commented 11 months ago

CodeFactors also reports a lot of Insert ··, is there an easy way of solving these errors?

Run eslint.

orsett0 commented 11 months ago

I don't know how to resolve the errors reported by linter and docker-publish, but it seems they do not depend on my changes

miraclx commented 11 months ago

I'll take a look.

orsett0 commented 11 months ago

Sorry for the late reply, I was out of town and I'll be moving shortly, so I don't have much time.

parseMeta method removed entries with null-ish values

I'll re-implement parseMeta to remove null values, i honestly didn't notice it.

Now, AtomicParsley had special input values that it mapped to certain output values.

Regarding rtng (this also affects stik, though it wasn't clear), its value is an uint8, but I didn't know that. So I updated meta-writer accordingly. It now accepts as parameters the same strings AtomicParsley does, and it translates them to the correct value.

The value of purd is a string. Apple specs says that it should be "the number of seconds that have passed since midnight January 1, 1904", but AtomicParsley sets it to an ISO-8601 string directly in the file (it's not just the way it displays the date):

$ atomicparsley sample.m4a --purchaseDate timestamp -W

 Started writing to temp file.
 Progress: =======================================================>100% |
 Finished writing to temp file.

$ xxd sample.m4a | grep -A3 'purd'
00000c40: 2054 696d 6529 0000 002c 7075 7264 0000   Time)...,purd..
00000c50: 0024 6461 7461 0000 0001 0000 0000 3230  .$data........20
00000c60: 3233 2d31 302d 3239 5431 383a 3139 3a35  23-10-29T18:19:5
00000c70: 365a 0000 0019 7067 6170 0000 0011 6461  6Z....pgap....da

so I decided to allow any string to be passed to meta-writer, and freyr-js will have to pass the date in the desired format.

AtomicParsley only sets the cpil tag when it is true. Not sure what effects including it as false would cause. Should be harmless.

I agree with you that it shouldn't be a problem. I didn't find it on the linked Apple specs, but ExifTool documentation says that it can hold both 1 and 0 as values.

(btw, this should be metaWriter following JS convention)

Noted ;), I'll change it with the next release.

orsett0 commented 11 months ago

I did all the changes. It should be okay.

One note: I encountered what seems to be a bug in the WebAssembly Systemm Interface. This bug causes a segfault in the function wasi.start(). I don't know why this appens, but it doesn't seem to be related to the WASM binary itself, so I'll be reporting this upstream.

In the meantime, I've found a workaround for this (I'm direcly calling the exported method in WASM), althoug this skips some checks and it does not set up shared memory. This however shouldn't be a problem in our case, and after performing some test, I'm confident that everything will work fine.

One other thing: I haven't been able to reproduce this bug outside of freyr-js. Would that be a problem for you if I were to reference this project when reporting the issue?

miraclx commented 11 months ago

Would that be a problem for you if I were to reference this project when reporting the issue?

Not at all, please proceed.

orsett0 commented 9 months ago

Hi, sorry for the (very) late reply. It's been a rough couple of months.

I recently started to work again on this, and the good news is that I've been able to identify the error. WASI has some kind of problem allocating more than (around) 8 MiB of contiguous memory. I couldn't reliably reproduce the bug, because some specific conditions must be met, but I got an easy to reproduce example for the issue.

Here's the issue, nodejs/node#51303, if you want to follow any development.

miraclx commented 5 months ago

Quite interesting that we're observing a substantial chunk (if not all) of the file being buffered in memory, m4a supports streamed mutation so this is weird. Worth investigating for sure.