Menu

#306 APIC frame parsed incorrectly from id3v2.4 tag

1.26.x
closed-fixed
nobody
5
2021-03-27
2021-03-11
No

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.

1 Attachments

Discussion

  • Thomas Orgis

    Thomas Orgis - 2021-03-11

    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.

     
  • Thomas Orgis

    Thomas Orgis - 2021-03-14

    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:

     4.14.   Attached picture
      <Header for 'Attached picture', ID: "APIC">
       Text encoding      $xx
       MIME type          <text string> $00
       Picture type       $xx
       Description        <text string according to encoding> $00 (00)
       Picture data       <binary data>
    

    Now look at the file you provided:

    00000010  00 0b 00 00 03 54 65 73  74 20 54 72 61 63 6b 54  |.....Test TrackT|
    00000020  49 54 32 00 00 00 1a 00  00 03 41 72 74 77 6f 72  |IT2.......Artwor|
    00000030  6b 20 45 6d 62 65 64 64  65 64 20 49 44 33 5f 56  |k Embedded ID3_V|
    00000040  32 2e 34 54 58 58 58 00  00 00 1d 00 00 03 4d 50  |2.4TXXX.......MP|
    00000050  33 47 41 49 4e 5f 41 4c  42 55 4d 5f 4d 49 4e 4d  |3GAIN_ALBUM_MINM|
    00000060  41 58 00 31 35 32 2c 32  30 34 54 58 58 58 00 00  |AX.152,204TXXX..|
    00000070  00 17 00 00 03 4d 50 33  47 41 49 4e 5f 4d 49 4e  |.....MP3GAIN_MIN|
    00000080  4d 41 58 00 31 35 32 2c  32 30 34 54 58 58 58 00  |MAX.152,204TXXX.|
    00000090  00 00 19 00 00 03 4d 50  33 47 41 49 4e 5f 55 4e  |......MP3GAIN_UN|
    000000a0  44 4f 00 2d 30 31 34 2c  2d 30 31 34 2c 4e 41 50  |DO.-014,-014,NAP|
    000000b0  49 43 00 01 0c 50 00 03  00 01 0c 02 00 69 6d 61  |IC...P.......ima|
    000000c0  67 65 2f 6a 70 65 67 00  03 00 ff d8 ff 00 e0 00  |ge/jpeg.........|
    000000d0  10 4a 46 49 46 00 01 01  00 01 2c 01 2c 00 00 ff  |.JFIF.....,.,...|
    

    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
  • Leandro Nini

    Leandro Nini - 2021-03-15

    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.

     
  • Thomas Orgis

    Thomas Orgis - 2021-03-15
     
  • Thomas Orgis

    Thomas Orgis - 2021-03-15

    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.

     
  • Thomas Orgis

    Thomas Orgis - 2021-03-15

    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.

     
  • Thomas Orgis

    Thomas Orgis - 2021-03-15

    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:

    $ src/mpg123-id3dump ../../bugs/306/Test_V24.mp3 
    FILE: ../../bugs/306/Test_V24.mp3
    
    ====      ID3v1       ====
    Title: Artwork Embedded ID3_V2.4
    Artist: Test Track
    Album: 
    Year: 
    Comment: 
    Genre: 255
    ====      ID3v2       ====
    Title: Artwork Embedded ID3_V2.4
    Artist: Test Track
    
    ==== ID3v2 Raw frames ====
    TPE1 language()
     Test Track
    TIT2 language()
     Artwork Embedded ID3_V2.4
    TXXX description(MP3GAIN_ALBUM_MINMAX)
     152,204
    TXXX description(MP3GAIN_MINMAX)
     152,204
    TXXX description(MP3GAIN_UNDO)
     -014,-014,N
    APIC type(3, front cover) mime(image/jpeg) size(17908)
    
    writing ../../bugs/306/Test_V24.mp3.front_cover.jpeg
    $ md5sum ../../bugs/306/Test_V24.mp3.front_cover.jpeg
    54dba7c110ab4de1e7c0dfec28db2f4b  ../../bugs/306/Test_V24.mp3.front_cover.jpeg
    
     

    Last edit: Thomas Orgis 2021-03-15
  • Leandro Nini

    Leandro Nini - 2021-03-15

    Confirmed working, thanks!

     
  • Thomas Orgis

    Thomas Orgis - 2021-03-27
    • status: open --> closed-fixed
     

Log in to post a comment.