[MPlayer-dev-eng] [PATCH] libmpdemux/demux_audio: Skip ID3v2.4 tags.

[Benoît Thébaudeau]

Hi Ingo, all,

> > @@ -358,9 +358,11 @@ static int demux_audio_open(demuxer_t* d
> >  	step = 4;
> >      } else if( hdr[0] == 'I' && hdr[1] == 'D' && hdr[2] == '3' &&
> >      (hdr[3] >= 2)) {
> >        int len;
> > -      stream_skip(s,2);
> > +      stream_skip(s,1);
> > +      stream_read(s,hdr,1);
> > +      len = hdr[0] & 0x10 ? 10 : 0;
> >        stream_read(s,hdr,4);
> > -      len = (hdr[0]<<21) | (hdr[1]<<14) | (hdr[2]<<7) | hdr[3];
> > +      len += (hdr[0]<<21) | (hdr[1]<<14) | (hdr[2]<<7) | hdr[3];
> >        stream_skip(s,len);
> >        step = 4;
> >      } else if( found_WAVE && hdr[0] == 'f' && hdr[1] == 'm' &&
> >      hdr[2] == 't' && hdr[3] == ' ' ) {
> 
> Why skipping 10 extra bytes? If I understand the specs correctly, the
> footer
> at the end of the file doesn't count for the ID3v2.4 size, so it's
> compatible
> to earlier ID3v2.

My reference was:
http://id3.org/id3v2.4.0-structure

"The ID3v2 tag size is the sum of the byte length of the extended
header, the padding and the frames after unsynchronisation. If a
footer is present this equals to ('total size' - 20) bytes, otherwise
('total size' - 10) bytes."

So the ID3v2 tag size from the tag header or footer means the same thing
whatever the ID3v2 revision, but as it does not take the possible footer into
account, the footer size (10 bytes) has to be added in order to be skipped:
"
     +-----------------------------+
     |      Header (10 bytes)      |
     +-----------------------------+
     |       Extended Header       |
     | (variable length, OPTIONAL) |
     +-----------------------------+
     |   Frames (variable length)  |
     +-----------------------------+
     |           Padding           |
     | (variable length, OPTIONAL) |
     +-----------------------------+
     | Footer (10 bytes, OPTIONAL) |
     +-----------------------------+
"

> > @@ -446,6 +448,17 @@ static int demux_audio_open(demuxer_t* d
> >  	g = stream_read_char(s);
> >  	demux_info_add(demuxer,"Genre",genres[g]);
> >        }
> > +      stream_seek(s,demuxer->movi_end-10);
> > +      stream_read(s,hdr,4);
> 
> I'd prefer reading into tag to better match the "TAG" check.

OK, I'll do that.

> Are "TAG" and "3DI" mutually exclusive (I don't get it clearly from
> the
> specs)?

No, they're not.

> What if both are present and "TAG" comes before "3DI"?

See "5.   Tag location". Appended ID3v2.4 tags have to be placed "before tags
from other tagging systems". In that case, the file structure will thus be:
- prepended ID3v2.4 tag:
 * header ("ID3"...),
 * contents (giving size in header/footer),
 * footer ("3DI"..., optional),
- audio,
- appended ID3v2.4 tag:
 * header ("ID3"...),
 * contents (giving size in header/footer),
 * footer ("3DI"..., mandatory: "It is REQUIRED to add a footer to an appended
   tag"),
- ID3v1 tag ("TAG"...).

> > +      if( hdr[0] == '3' && hdr[1] == 'D' && hdr[2] == 'I' &&
> > (hdr[3] >= 2)) {
> > +	int len;
> > +	stream_skip(s,1);
> > +	stream_read(s,hdr,1);
> > +	len = hdr[0] & 0x10 ? 20 : 10;
> > +	stream_read(s,hdr,4);
> > +	len += (hdr[0]<<21) | (hdr[1]<<14) | (hdr[2]<<7) | hdr[3];
> > +	demuxer->movi_end = stream_tell(s)-len;
> > +      }
> >      }
> >      if (duration && demuxer->movi_end && demuxer->movi_end >
> >      demuxer->movi_start) sh_audio->wf->nAvgBytesPerSec =
> >      (demuxer->movi_end - demuxer->movi_start) / duration;
> >      sh_audio->i_bps = sh_audio->wf->nAvgBytesPerSec;
> 
> Isn't it simply demuxer->movi_end = stream_tell(s)-4?

No. In that case, the possible I3v1 tag has already been skipped, and an
appended ID3v2.4 tag has been detected thanks to "3DI" in its footer, so the
whole tag size has to be calculated from the tag footer, then skipped by placing
demuxer->movi_end at the beginning of the appended ID3v2.4 tag.

Best regards,
Benoît