Some embedded cover images are not shown

[paulijar]

With the current development sources, Music app is able to show album cover images embedded in the MP3 files. This, however, does not work on all files. Especially, it never works if the image size is more than 16KB.

The root cause for this is the getID3 issue http://www.getid3.org/phpBB3/viewtopic.php?f=4&t=1930. A fix for this issue is now included in the main branch of getID3 (commit JamesHeinrich/getID3@0e6c1a3) but I'm not sure how the problem should be fixed for Music app.

Music app comes with its own version of getID3 but usually this version is not used. Instead, getID3 loaded by the core is used. On the other hand, getID3 has been already replaced with ID3Parser in the master branch of the core. ID3Parser is based on getID3 and has the same problem. I believe that core circumvents the problem by making a temporary copy of the music file before passing it to getID3 or ID3Parser.

[gnarula]

Good catch! I've noticed this too and got the same error complaining about frame size in most of my files with large sized image covers. You're right, Music uses a separate copy of getID3 but core uses a minimal version of it (ID3Parser). Great that your fix has been merged in the main branch of getID3. I think we can upgrade the dependency once they publish a release with your patch applied :)

Another approach might be to use the Preview API in core (which does get around it by making a temporary copy) but that's not a part of the public API yet unfortunately.

[gnarula]

@paulijar can you backport the changes to ID3Parser as well? I'd like to close this before pushing the next release and getID3 seems to have a long release cycle.

[paulijar]

@gnarula Okay, I'll make a PR for ID3Parser when I have time. That might be tomorrow or next week.

[paulijar]

@gnarula The fix is now included in ID3Parser release https://github.com/LukasReschke/ID3Parser/releases/tag/v0.0.3

[paulijar]

@gnarula Are you planning to integrate the new ID3Parser or should I try to do it?

[gnarula]

@paulijar I'll integrate the new ID3Parser, review the other PRs and build a new release in the next few days. Sorry for the delay, I've been a bit overloaded with exams and assignments in Uni lately.

[paulijar]

@gnarula Okay, great. While testing my fix on ID3Parser, I noticed that it produces a bit different output array structure than getID3. So that's something to take care of while making the integration.

Btw, I have done some refactoring on the update function of scanner.php on my work area. I'll try to drop in a PR about those changes tomorrow, since they may help on the ID3Parser integration.

[gnarula]

@paulijar @LukasReschke : ID3Parser doesn't seem to populate the tags key in the array, because of which getid3_lib::CopyTagsToComments() doesn't copy the id3 tags to comments array. Could either of you reproduce the same behaviour?

[paulijar]

@gnarula When earlier testing ID3Parser, I noticed that after calling getid3_lib::CopyTagsToComments() I could find the tags from $metadata["id3v2"]["comments"] instead of $metadata["comments"]. I didn't dive into reasons behind this.

I found this somewhat odd, since I've understood that the whole point of CopyTagsToComments is to merge data from id3v1 and id3v2 tags (and other metadata formats). I didn't test, what is the output if the file contains only id3v1 tags.

[gnarula]

@paulijar I think this is because ID3Parser doesn't process tags (ref https://github.com/JamesHeinrich/getID3/blob/master/getid3/getid3.php#L469). I guess id3v1 isn't used much now, but is it so rare that we can completely ignore it?

[paulijar]

@gnarula I bet there is some user somewhere who has some very old MP3 files with ID3v1 tags. But for those, very simple fallback logic should be sufficient: if the tag is not found under ["id3v2"], try also ["id3v1"] before giving up. I guess that call to CopyTagsToComments() would then be totally unnecessary.

What about APEv1 and APEv2 tags then? Those have been supported by getID3 but are not supported in ID3Parser. I wonder, how many users are screwed if we drop the APE support.

[gnarula]

@paulijar In that case, I guess it's better to wait for a new release of getID3

[paulijar]

@gnarula I don't know about the release schedule of getID3 but lately there seems to have been around 2 releases per year. The next one could be close or it may be months away.

Would it be considered bad form to make our own quick fix version of getID3? We could take our current version 1.9.8 or the most recent release 1.9.12 as baseline and cherry pick the one relevant fix on top of the baseline. After all, the license of getID3 should allow distributing also modified versions of the library. This custom version could then be replaced with 1.9.13 once that becomes available.

If we want to ship a new version of getID3, then we also need to make sure that our version of the library gets loaded. I believe that currently Music App prefers to use the getID3 library version shipped with the core if one is available.