[MPlayer-dev-eng] [PATCH] libmpdemux/demux_audio: Skip ID3v2.4 tags.
Reimar Döffinger
Reimar.Doeffinger at gmx.de
Mon Jan 16 23:23:42 CET 2012
On Mon, Jan 16, 2012 at 10:21:28PM +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.
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
> + } 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?
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.
> + 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.
More information about the MPlayer-dev-eng
mailing list