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

Album cover failed to be written into the file #42

Closed
timdream opened this issue Jul 4, 2017 · 11 comments
Closed

Album cover failed to be written into the file #42

timdream opened this issue Jul 4, 2017 · 11 comments

Comments

@timdream
Copy link
Contributor

timdream commented Jul 4, 2017

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!

@egoroof
Copy link
Owner

egoroof commented Jul 4, 2017

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.

@timdream
Copy link
Contributor Author

timdream commented Jul 4, 2017

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?

@egoroof
Copy link
Owner

egoroof commented Jul 4, 2017

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

@timdream
Copy link
Contributor Author

timdream commented Jul 5, 2017

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.

@timdream
Copy link
Contributor Author

timdream commented Jul 5, 2017

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.

@timdream
Copy link
Contributor Author

timdream commented Jul 5, 2017

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?

@timdream
Copy link
Contributor Author

timdream commented Jul 5, 2017

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.

@timdream
Copy link
Contributor Author

timdream commented Jul 5, 2017

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.

@egoroof
Copy link
Owner

egoroof commented Jul 5, 2017

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.

@egoroof
Copy link
Owner

egoroof commented Jul 5, 2017

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

@timdream
Copy link
Contributor Author

timdream commented Jul 5, 2017

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

timdream added a commit to timdream/browser-id3-writer that referenced this issue Jul 6, 2017
timdream added a commit to timdream/browser-id3-writer that referenced this issue Jul 6, 2017
There are some quirks with APIC frame parsing on macOS iTunes and Finder. When the description is encoded in Unicode and is either an empty string or with an non-Western character as the last character, iTunes will fail to parse the picture from the frame.

This fix workaround it by always use Western encoding when the description is empty, and only opt to Unicode encoding when the caller explicitly choose to with `useUnicodeEncoding: true`. README.md is updated accordingly with a warning.
timdream added a commit to timdream/browser-id3-writer that referenced this issue Jul 6, 2017
There are some quirks with APIC frame parsing on macOS iTunes and Finder. When the description is encoded in Unicode and is either an empty string or with an non-Western character as the last character, iTunes will fail to parse the picture from the frame.

This fix workaround it by always use Western encoding when the description is empty, and only opt to Unicode encoding when the caller explicitly choose to with `useUnicodeEncoding: true`. README.md is updated accordingly with a warning.
@egoroof egoroof closed this as completed in f4868b3 Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants