[FFmpeg-devel] [PATCH] VQF demuxer

[Vitor Sessak]

Michael Niedermayer wrote:
> On Sat, Mar 07, 2009 at 11:30:27PM +0100, Vitor Sessak wrote:
>> 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...
> 
> looks ok i think

Applied.

-Vitor