[MPlayer-dev-eng] [PATCH] libmpdemux/demux_audio: Skip ID3v2.4 tags.
Benoît Thébaudeau
benoit.thebaudeau at advansee.com
Tue Jan 17 00:40:12 CET 2012
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
More information about the MPlayer-dev-eng
mailing list