egoroof/browser-id3-writer

Album cover failed to be written into the file

timdream opened this issue · 11 comments

I am testing with a file like this.

The picture will be encoded into the file but with a few bytes missing from the beginning, therefore it will not be decoded correctly (this is a conclusion I found from extracting the encoded bytes with ID3 Reader).

This can be reproduced with the online demo. I couldn't see obvious issues by looking at the code for 10 min, so I decided to file an issue first. Will look into it later.

Thanks for the great work!

Don't you think it's a reader issue? I can decode it in different programs (after writing with the online demo):

screenshot 2017-07-04 19 18 23

screenshot 2017-07-04 19 19 37

But if you find something with the writer then I can help you.

The cover does not show up on macOS Finder, so I suspect it's a writer issue. :'( where do you recommend me to look up ...? Maybe iTunes and Finder on Mac simply use a different layout?

I'll look at it on macOS later (actually I didn't do that previously, so thanks for info).

So I turned to HEX editor to inspect the files. Strangely the ID3 tags iTunes give me is in v2.2. Unfortunately, iTunes has since removed the option allow users to switch between ID3 versions.

This is what the library generates, from 0th position to the image data:
49 44 33 03 00 00 00 03 2A 55 41 50 49 43 00 00 C5 4B 00 00 01 69 6D 61 67 65 2F 6A 70 65 67 00 03 FF FE 00 00 ID3�����*UAPIC��ÅK���image/jpeg��ÿþ��

This is what iTunes generates:
49 44 33 02 00 00 00 03 5A 46 50 49 43 00 C5 40 00 4A 50 47 00 00 ID3�����ZFPIC�Å@�JPG��

I will poke around my library and find an ID3 v2.3 file that can display picture correctly, and try to figure it out from there.

I've managed to "fix" a file generated by the library:

Here are the unchanged bytes of the frame, annotated with comments from the source code:

41 50 49 43 "APIC"
00 00 C5 4B (frame size: 50507)
00 00 (flags)
01 (encoding)
69 6D 61 67 65 2F 6A 70 65 67 (mime type: image/jpeg)
00 03 FF FE (separator, pic type, BOM)
(description)
00 00 (separator)
... image data

Here are the bytes of an APIC frame from a song from my library (which pic can be seen correctly):

41 50 49 43 "APIC"
00 1A C0 3C (frame size)
00 00 (flags)
00 (encoding)
69 6D 61 67 65 2F 70 6E 67 (mime type: image/png)
00 (separator, pic type ?, BOM?)
(description)
00 00 (separator)
... image data

Comparing both I realized there are 2 diffferences:

  1. encoding is set to 0
  2. pic type and BOM are missing

And indeed when I manually change and delete these bytes the picture show up on Finder. I don't understand ID3 v2.3 enough to say how to fix this the right way; if you comment on it I am happy to submit a pull request for it.

So I looked up ID3 v2.3 section 3.3, does this means iTunes simply don't support picture type and description in an APIC frame?

Does it consider a valid fix if we switch to "iTunes compatible" frame format when we found the picType = 0 and description = '' from the input?

Correction: the APIC frame understood by iTunes should be interpreted this way:

41 50 49 43 "APIC"
00 1A C0 3C (frame size)
00 00 (flags)
00 (encoding)
69 6D 61 67 65 2F 70 6E 67 (mime type: image/png)
00 (separator)
00 (pic type)
(description)
00 (separator; since desc is encoded in non-Unicode)
... image data

This should be correct because I can add whatever description into the frame and change the pic type without making the cover disappear from Finder. So the "iTunes compatible" frame format is simply an APIC frame that always encodes the description in ISO-8859-1.

What's the recommend fix here? Do we want to simply force the encoding of the description to ISO-8859-1? Or we could do something sane like check a string and give a warning or whatnot? Tell me what to do and I will submit a pull request.

Here is an alternative byte sequence that can be read by iTunes

41 50 49 43 "APIC"
00 1A C0 3C (frame size)
00 00 (flags)
01 (encoding)
69 6D 61 67 65 2F 70 6E 67 (mime type: image/png)
00 (separator)
03 (pic type)
(description with or without BOM and must not end with 00)
00 00 00 (separator?)
... image data

However I doubt this is confirm to spec since the description doesn't even have to be 2n bytes long.

Nice findings, Timothy!

I think we can add another parameter when set APIC frame which tells what encoding to use (by default it will be ISO-8859-1):

writer.setFrame('APIC', {
    type: 3,
    data: coverArrayBuffer,
    description: 'description',
    useUnicodeEncoding: true
});

And add in readme info about the issue.

By the way, I don't think many people use APIC description at all.

Would you like to work on it or I'll do it?

@egoroof Sure, I can submit a pull request with that.