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

[Benoît Thébaudeau]

Reimar,

> > +static unsigned int id3v2_tag_size(stream_t *s) {
> > +  uint8_t tag[4];
> > +  unsigned int header_footer_size;
> > +  unsigned int size;
> > +  int i;
> > +
> > +  stream_read(s,tag,2);
> > +  if(tag[0] == 0xff)
> > +    return 0;
> > +  header_footer_size = tag[1] & 0x10 ? 20 : 10;
> > +
> > +  stream_read(s,tag,4);
> > +  size = 0;
> > +  for(i = 0; i < 4; i++) {
> > +    if (tag[i] & 0x80)
> > +      return 0;
> > +    size = size << 7 | tag[i];
> > +  }
> 
> "tag" doesn't seem like really good naming to me.

OK, then what would you like? "hdr"?

> Personally I'd also rather go with either:
> 1) Read all 6 bytes at once or
> 2) Use stream_read_char to read one-by-one

OK, I'll do that. Any preference between 1 and 2?

> > +    } else if( hdr[0] == 'I' && hdr[1] == 'D' && hdr[2] == '3' &&
> > hdr[3] != 0xff &&
> > +               (len = id3v2_tag_size(s)) > 0) {
> > +      stream_skip(s,len-10);
> >        step = 4;
> 
> Why accept hdr[3] values of 0 or 1 now? Aren't they invalid, too?

http://id3.org/id3v2.4.0-structure
"An ID3v2 tag can be detected with the following pattern:
$49 44 33 yy yy xx zz zz zz zz
Where yy is less than $FF, xx is the 'flags' byte and zz is less than
$80."

It does not say that the 1st yy can not be 0 or 1, but I don't find any
information regarding ID3v2.0 or ID3v2.1. Do you think it's better to
discard these major versions as non-existing? What about the possible
future ID3v2.5 and above?

> Also when failing id3v2_tag_size has read data and thus the data in
> hdr is no longer in sync with the file position, so you must at least
> set step to 4 in that case.
> So I'm afraid it looks to me that squeezing that into the if made it
> both uglier and wrong.
> Just
> > } else if( hdr[0] == 'I' && hdr[1] == 'D' && hdr[2] == '3' &&
> > hdr[3] != 0xff) {
> >   unsigned len = id3v2_tag_size(s);
> >   if (len > 0)
> >     stream_skip(s,len-10);
> >  step = 4;
> 
> Should be fine I think.

No, that won't work either. If id3v2_tag_size fails, the tag signature is
not present, so step should be 1. It's the stream position that should be
changed in that case. Does stream_skip work with negative values, or should
stream_seek be used instead?

> > +      if( tag[0] == '3' && tag[1] == 'D' && tag[2] == 'I' &&
> > (uint8_t)tag[3] != 0xff &&
> > +          (len = id3v2_tag_size(s)) > 0) {
> > +        if(len > demuxer->movi_end) {
> > +          mp_msg(MSGT_DEMUX,MSGL_ERR,"[demux_audio] Bad ID3v2 tag
> > size: larger than stream (%u)!!!\n",len);
> > +          len = demuxer->movi_end;
> 
> First, you should check against len > demuxer->movi_end -
> demuxer->movi_start,
> because "end" before "start" is not that much better than negative
> end
> :-)
> Next, I doubt there is much point in setting it such that the file is
> detected as empty, I think most users would prefer if we did our best
> to try to play a broken file instead of pretend it is empty.
> So IMO if you detect an error set len to 0 - or maybe better 10.

OK, I'll do that. Also, the calculation of the duration uses movi_end instead
of (movi_end - movi_start) at the bottom of demuxer_audio.c. Is it on purpose?

Benoît