Zazama / node-id3

Pure JavaScript ID3 Tag library
MIT License
285 stars 57 forks source link

Fix compression and reduce code complexity #130

Closed Zazama closed 1 year ago

Zazama commented 2 years ago

Compression was broken because the data length indicator was not passed to the frame body buffer but compression relies on it.

Zazama commented 2 years ago

It might make sense to extract an ID3Frame into a real class because right now the code is spread out so much e.g.

const frame = ID3Frame.fromBuffer(...)

then the class handles flags/indicators/unsynchronisation instead of the getFramesFromID3Body function and instead of e.g. ID3Frames[frameIdentifier].read(frame.body, ID3Version)

frame.getValue() // or something like that
pbricout commented 2 years ago

It might make sense to extract an ID3Frame into a real class because right now the code is spread out so much e.g.

Are classes supported in the node 10x.? That seems like a good idea, I am not super familiar with the code yet. That would handle a single frame or multiple frames?

frame.getValue() // or something like that

What would getValue return exactly?

Zazama commented 2 years ago

Classes are supported, you can check here: https://node.green/ An ID3Frame would handle a single frame, for multiples it would be a tag (e.g. ID3Tag), which might also make sense to create.

frame.getValue() would be what the read functions in ID3Frames.js return, but without arrays, only single ones.

// title string
let title = titleFrame.getValue();

and e.g. frame.getBuffer() would return the corresponding buffer which would be what the write functions return atm.

There would need to be a way to create a frame from a buffer and one to create it by value, e.g.

const titleFrame = ID3Frame.createFromBuffer(buffer)
const title = titleFrame.getValue()

const albumFrame = ID3Frame.createFromValue('TALB', 'some album')
const albumFrameBuffer = albumFrame.getBuffer()
Zazama commented 2 years ago

I will try to create a new PR with an example

pbricout commented 1 year ago

Classes are supported, you can check here: https://node.green/

Excellent, I was looking for something like that a bit earlier and could not find it! Really handy, now I know exactly which features we can use.

I will try to create a new PR with an example

Sounds good.

I think you can merge this one if you want. The buffer views would be nice but can be done if we better want to review the code.

Zazama commented 1 year ago

Ok I just checked quickly and the whole buffer in that function is a copy anyways, so it really doesn't matter if slice is used. I'll commit it later this week before merging

pbricout commented 1 year ago

Ok I just checked quickly and the whole buffer in that function is a copy anyways, so it really doesn't matter if slice is used. I'll commit it later this week before merging

I would expect a function called getFramesFromID3Body (i.e. get) to not modify anything. We should strive to have that always true.

pbricout commented 1 year ago
const titleFrame = ID3Frame.createFromBuffer(buffer)
const title = titleFrame.getValue()

const albumFrame = ID3Frame.createFromValue('TALB', 'some album')
const albumFrameBuffer = albumFrame.getBuffer()

I see, it would know about the specific frames, I was thinking something more generic below. But why we can try to see what it looks like.

Zazama commented 1 year ago

Turns out, buffer.slice has been deprecated since v16. Instead, Buffer.subarray can be used and is basically the same API.

https://nodejs.org/docs/latest-v16.x/api/buffer.html#bufslicestart-end