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 @orsetto/meta-writer #582

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

Conversation

orsett0
Copy link

@orsett0 orsett0 commented Oct 15, 2023

@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
Copy link
Owner

miraclx commented Oct 16, 2023

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
Copy link
Owner

miraclx commented Oct 16, 2023

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 also removed an unused key/pair from the object passed to
meta_writer(). That was just a leftover from the parameters passed to
AtomicParsley.
@orsett0
Copy link
Author

orsett0 commented Oct 16, 2023

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
Copy link
Owner

miraclx commented Oct 16, 2023

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

Run eslint.

@orsett0
Copy link
Author

orsett0 commented Oct 17, 2023

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
Copy link
Owner

miraclx commented Oct 18, 2023

I'll take a look.

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.

Some tests for consistency

$ git checkout master

$ mkdir atomicparsley meta-writer

$ freyr https://open.spotify.com/track/5w9upngVRHNjdZcRC7Xxr2 -d atomicparsley

$ git checkout orsett0/replace-atomicparsley
$ freyr https://open.spotify.com/track/5w9upngVRHNjdZcRC7Xxr2 -d meta-writer

On my machine, they both run in 15s, the AtomicParsley version is 10118732 bytes (10.12MB), and the meta-writer version is 10141969 bytes (10.14MB).

Tag dumping:

$ AtomicParsley "atomicparsley/Fred again/Actual Life 3 (January 1 - September 9 2022)/03 Delilah (pull me out of this).m4a" -t | sort > atomicparsley/tags
$ AtomicParsley "meta-writer/Fred again/Actual Life 3 (January 1 - September 9 2022)/03 Delilah (pull me out of this).m4a" -t | sort > meta-writer/tags

$ diff atomicparsley/tags meta-writer/tags
# see below

Dump Diffs

> Atom "©gen" contains: 
> Atom "©wrt" contains: null
14a17
> Atom "cpil" contains: false
17,18c20,21
< Atom "purd" contains: 2023-10-18T11:04:53Z
< Atom "rtng" contains: Clean Content
---
> Atom "purd" contains: timestamp
> Atom "rtng" contains: clean

Some notes: the logic in the now-removed parseMeta method removed entries with null-ish values and doesn't pass them to AtomicParsley, meta_writer (btw, this should be metaWriter following JS convention) doesn't do this..

Spotify doesn't provide Genre and Composer data - See https://github.com/miraclx/freyr-js?tab=readme-ov-file#metadata-availability

This is why we have ©wrt as null and ©gen empty.

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

Like in the case of rtng, us passing clean or explicit map to Clean Content and Explicit Content respectively.

See AtomicParsley --help

  --advisory     (string*)    Content advisory (*values: 'clean', 'explicit')

And in the case of purd, AtomicParsley accepts timestamp as a valid input for the current timestamp.

See AtomicParsley --longhelp

  --purchaseDate     ,  -D   (UTC)    Set Universal Coordinated Time of purchase on a "purd" atom
                                      (use "timestamp" to set UTC to now; can be akin to id3v2 TDTG tag)

We should be able to do without these special features and use the actual values from freyr.

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

See AtomicParsley --longhelp

  --compilation      ,  -C   (bool)   Sets the "cpil" atom (true or false to delete the atom)

@orsett0
Copy link
Author

orsett0 commented Oct 29, 2023

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
Copy link
Author

orsett0 commented Nov 1, 2023

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
Copy link
Owner

miraclx commented Nov 3, 2023

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

Not at all, please proceed.

@orsett0
Copy link
Author

orsett0 commented Dec 29, 2023

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
Copy link
Owner

miraclx commented May 7, 2024

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.

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.

Replace AtomicParsley with lofty-rs
2 participants