APIC frame parsed incorrectly from id3v2.4 tag
Brought to you by:
sobukus
The APIC frame from the attached example is not parsed correctly:
> mpg123-id3dump Test_V24.mp3 [...] APIC type(105, invalid type) mime( ) size(17910) mage/jpeg
while ffmpeg can extract it fine:
> ffmpeg -i Test_V24.mp3 -an -vcodec copy cover.jpg
Another sample with an id3v2.3 tag is ok.
Confirmed. Libmpg123 is giving two bytes too much in the beginning and is confused with the mime type.
Shouldn't take too long to figure out what's wrong here.
Well, I figured. And, go figure … I reckon your file is actually at fault or there is another definition
of ID3v2.4 that I am not aware of. According to
https://id3.org/id3v2.4.0-frames
this is the structure of the APIC frame:
Now look at the file you provided:
It begins with APIC (AP at the end of line 10, IC at line 11), then the rest of the 10-byte header.
At offset 0xb8, after the 0x00 0x03 sequence, there is the encoding 0x00. Then there
should follow the MIME type, but you got 4 bytes of some garbage text (well, ending with
a zero byte at least). Then follows the image/jpeg string, with the picture type 0x03
(front cover) in tow. Then a single zero byte for an empty description text. Then the actual
image data follows, beginning with 0xff 0xd8 (somewhat before the JFIF).
Without that stray 0x01 0x0c 0x02 0x00 where the MIME type should be, your image
should be extracted just fine. Is there some specification that sequence follows? Some
Additional flags? Is this just garbage?
ffmpeg does catch the correct image type of ‘Cover (front)’ … so it possibly expects that bit
of extra data or works backwards from the JFIF header.
How are you creating those image tags?
Last edit: Thomas Orgis 2021-03-14
That four extra bytes are the "Data length indicator". The flag bytes in the APIC header are set to 0x00 0x03 which , according to section 4.1.2 of the id3v2.4.0 structure document, "indicates that a data length indicator has been added to the frame" (flag p).
The file is just a test sample I downloaded from somewhere many years ago, but it looks correct to the specs.
Oh, you got me there. I missed that bit. I even have the data length flag defined as a macro, but did not implement its handling. Gross. Libmpg123 checks for encrypted and compressed frames and skips those (so far people did not complain about missing support for that).
I need to add handling of this data length value (the spec definition of the value is a bit funny).
Thanks for reporting this omission.
Also, I need to review the handling of the unsync flag.
OK, unsync is handled centrally … things would be too broken otherwise;-) It's just been some time with the code and I had to check. Now on to figuring out how the data length value should work. And how it interacts with unsync.
I fixed the parsing now. Frame flag handling needed completion for both v2.4 and 2.3. Can you verify with current svn trunk (r4789) or https://mpg123.org/snapshot ? You should get this:
Last edit: Thomas Orgis 2021-03-15
Confirmed working, thanks!