[FFmpeg-devel] [PATCH] VQF demuxer

[Michael Niedermayer]

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

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

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- 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/8caa65a6/attachment.pgp>