Borewit / music-metadata

Stream and file based music metadata parser for node. Supporting a wide range of audio and tag formats.
MIT License
924 stars 91 forks source link

Fields calculated wrong for APEv2 Tags #331

Closed vladotesanovic closed 4 years ago

vladotesanovic commented 4 years ago

Bug description Number of APE tags are not calculated right. There is 11 tags, while peekToken function returns 12

tags

Above ALBUM tag is number of fields function returns.

Expected behavior Function should return 11 instead of 12

Audio file demonstrating the problem Audio file can be found here: https://we.tl/t-RSvWv2s177

I reproduced this with simple unit test:

    it('should be able to parse APEv2', async () => {

        const filePath = path.join(samplePath, 'mp3', 'crim2.mp3');

        const metadata = await mm.parseFile(filePath);
    });
Borewit commented 4 years ago

I examined the file, and I have been able to reproduce the issue. I believe the APE header is indeed a little bit corrupt.

The issue is hidden in the APE footer, in the field holding the Number of items in the Tag (n).

Note that the friendly people of Monkey-audio call what I call a header a tag, and the tag an item.

The header lists the number of items. Your APE footer has the item count set to 12 However there are only 11.

This will cause the current APEv2 parser to reader 1 tag-item-header to many, when the end of the buffer has been reached.

https://github.com/Borewit/music-metadata/blob/5602e57dcdb37e1ac1bc6b78c9c776edaa9c4767/lib/apev2/APEv2Parser.ts#L100-L102

After tag (item) #10, the offset === buffer,length.

Borewit commented 4 years ago

Script to trim sample file:

#!/bin/sh

IN=crim2.mp3

APEv2OFFSET=14132877

MPEGOFFSET=194984

MPEGFRAME3=197072

#dd count=$MPEGOFFSET bs=1 "if=$IN" of=header.id3v2
dd count=$MPEGFRAME3 bs=1 "if=$IN" of=3frames.mp3
dd skip=$APEv2OFFSET bs=1 "if=$IN" of=header.apev2

cat 3frames.mp3 header.apev2 > issue-331.mp3
Borewit commented 4 years ago

I just noticed I did not read your analysis good, yeah you nailed it as well, wrong item count.

But I think the issue is in the source format as you suggested elsewhere.

Borewit commented 4 years ago

Should no longer be fatal in version v5.0.1.

vladotesanovic commented 4 years ago

@Borewit My solution was different but similar to yours.

Thanks again

Borewit commented 4 years ago

Oh you already had solution in the pipeline, my fault, sorry about that.

Thank you for making music-metadata a better module.