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

[Reimar Döffinger]

On Tue, Jan 17, 2012 at 12:40:12AM +0100, Benoît Thébaudeau wrote:
> > > +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"?

Can't think of anything. "body" maybe?

> > 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?

Not particularly, whatever looks nicer :-)

> > > +    } 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?

Let's keep it with your version then, though making such somewhat
unrelated changes is not ideal (e.g. for regression bisecting -
though the code is simple enough it should not be a problem here).

> > 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.

What are those "should be"s based on?
On error, step = 4 only has the meaning "resync hdr".
Yes, it means that if the file contains ID3 followed by something we
can't parse we will have skipped a few bytes unscanned.
Seems very, very unlikely to matter.

> Does stream_skip work with negative values, or should
> stream_seek be used instead?

Neither is possible or allowed if the stream is not seekable.

> 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?

I'd say that is by mistake.