[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"?
>
> 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 :-)

OK. I went for 2 because it kills the variable naming discussion and it
reduces the number of dropped bytes in case of failure.

> > > > +    } 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).

OK.

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

Yes, my concern was these dropped bytes, hence those "should be"s.

> > 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, then the choice is easy.

See my updated patch attached. It also takes into account the extra ID3
header check Ingo wanted, as well as what you said regarding uint8_t.

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

I'll send a new patch for that.

Benoît
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1-demux-audio-skip-id3v2.4.patch
Type: text/x-patch
Size: 3473 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20120117/c7cd9fe1/attachment.bin>