[FFmpeg-devel] [PATCH] VQF demuxer

[Michael Niedermayer]

On Sat, Mar 07, 2009 at 10:45:08PM +0100, Vitor Sessak wrote:
> Michael Niedermayer wrote:
>> On Sat, Mar 07, 2009 at 09:48:35PM +0100, Vitor Sessak wrote:
>>> Michael Niedermayer wrote:
>>>> On Sat, Mar 07, 2009 at 05:42:59PM +0100, Vitor Sessak wrote:
>>>>> Hi,
>>>>>
>>>>> See $subj. This is a pretty simple demuxer, the only non-standard thing 
>>>>> about it is that a frame size in bits is not always a multiple of 8. 
>>>>> I'll post in a separated thread a WIP TwinVQ decoder if anyone wants to 
>>>>> test the demuxer (I've tested it with all the samples in 
>>>>> ftp.mplayerhq.hu).
>>>> patcheck says:
>>>> divide by 2^x could use >> maybe
>>>> vqf.diff:74:+    return AVPROBE_SCORE_MAX/2;
>>> False positive
>> true
>>>> vqf.diff:223:+    int size = (c->frame_bit_len - c->remaining_bits + 
>>>> 7)/8;
>>>> vqf.diff:265:+    if ((ret = url_fseek(s->pb, (pos-7)/8 + 
>>>> s->data_offset, SEEK_SET)) < 0)
>>>> Non static with no ff_/av_ prefix
>>>> vqf.diff:246:+int vqf_read_seek(AVFormatContext *s,
>>> all fixed
>>>
>>>> vqf.diff:272:+AVInputFormat vqf_demuxer = {
>>> Very common false positive
>> Actually no, its a common bug
>
> You mean that, in libavformat/allformats.c REGISTER_MUXER(xxx) should do 
> av_register_output_format(ff_xxx_muxer) (and all muxers changed 
> accordingly)?

thanks for volunteering :)


>
>>>> [...]
>>>>> Index: libavformat/vqf.c
>>>>> ===================================================================
>>>>> --- libavformat/vqf.c	(revision 0)
>>>>> +++ libavformat/vqf.c	(revision 0)
>>>> [...]
>>> All omitted chunks fixed
>>>
>>> [...]
>>>>>> +    st->start_time = 0;
>>>>>> +
>>>>>> +    do {
>>>>>> +        int len;
>>>>>> +        chunk_tag = get_le32(s->pb);
>>>>>> +
>>>>>> +        if (chunk_tag == MKTAG('D','A','T','A'))
>>>>>> +            break;
>>>>>> +
>>>>>> +        len = get_be32(s->pb);
>>>>>> +        header_size -= 8;
>>>>>> +
>>>>>> +        switch(chunk_tag){
>>>>>> +        case MKTAG('C','O','M','M'):
>>>>>> +            st->codec->channels = get_be32(s->pb) + 1;
>>>>>> +            read_bitrate        = get_be32(s->pb);
>>>>>> +            rate_flag           = get_be32(s->pb);
>>>>>> +            url_fskip(s->pb, len-12);
>>>>>> +
>>>>>> +            st->codec->bit_rate = read_bitrate*1000;
>>>>>> +            st->codec->bits_per_coded_sample = 16;
>>>>>> +            break;
>>>>>> +        case MKTAG('N','A','M','E'):
>>>>>> +            add_metadata(s, "title"    , len, header_size);
>>>>>> +            break;
>>>>>> +        case MKTAG('(','c',')',' '):
>>>>>> +            add_metadata(s, "copyright", len, header_size);
>>>>>> +            break;
>>>>>> +        case MKTAG('A','U','T','H'):
>>>>>> +            add_metadata(s, "author"   , len, header_size);
>>>>>> +            break;
>>>>>> +        case MKTAG('A','L','B','M'):
>>>>>> +            add_metadata(s, "album"    , len, header_size);
>>>>>> +            break;
>>>>>> +        case MKTAG('T','R','C','K'):
>>>>>> +            add_metadata(s, "track"    , len, header_size);
>>>>>> +            break;
>>>>>> +        case MKTAG('C','O','M','T'):
>>>>>> +            add_metadata(s, "comment"  , len, header_size);
>>>>>> +            break;
>>>>>> +        case MKTAG('F','I','L','E'):
>>>>>> +            add_metadata(s, "filename" , len, header_size);
>>>>>> +            break;
>>>>>> +        case MKTAG('D','S','I','Z'):
>>>>>> +            add_metadata(s, "size"     , len, header_size);
>>>>>> +            break;
>>>>>> +        case MKTAG('D','A','T','E'):
>>>>>> +            add_metadata(s, "date"     , len, header_size);
>>>>>> +            break;
>>>>>> +        case MKTAG('G','E','N','R'):
>>>>>> +            add_metadata(s, "genre"    , len, header_size);
>>>>>> +            break;
>>>>>> +        default:
>>>>>> +            av_log(s, AV_LOG_ERROR, "Unknown chunk: %c%c%c%c\n",
>>>>>> +                   ((char*)&chunk_tag)[0], ((char*)&chunk_tag)[1],
>>>>>> +                   ((char*)&chunk_tag)[2], ((char*)&chunk_tag)[3]);
>>>>>> +            url_fskip(s->pb, FFMIN(len,header_size));
>>>>>> +            break;
>>>>>> +        }
>>>>>> +
>>>>>> +        header_size -= len;
>>>>>> +
>>>>>> +    } while(header_size >= 0);
>>>> infinite loop given "appropriate" len
>>> I suppose you consider making len unsigned to fix it...
>> no
>
> I understand. Now I have len signed and test for (len < 0).

better but you still miss the -8

that is  - 8 - INT_MAX still might be bad


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090307/bd3c7891/attachment.pgp>