[FFmpeg-devel] [PATCH] VQF demuxer

[Vitor Sessak]

Michael Niedermayer wrote:
> 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

Yes, indeed. Forth try...

-Vitor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vqf4.diff
Type: text/x-diff
Size: 10069 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090307/f43852ea/attachment.diff>