[FFmpeg-devel] [PATCH] VQF demuxer

[Vitor Sessak]

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)?

>>> [...]
>>>> 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).

>>>> +static int vqf_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>> +{
>>>> +    VqfContext *c = s->priv_data;
>>>> +    int ret;
>>>> +    int size = (c->frame_bit_len - c->remaining_bits + 7)/8;
>>>> +
>>>> +    pkt->pos = url_ftell(s->pb);
>>>> +    pkt->stream_index = 0;
>>>> +
>>>> +    if (av_new_packet(pkt, size+2) < 0)
>>>> +        return AVERROR(EIO);
>>>> +
>>>> +    pkt->data[0] = 8 - c->remaining_bits; // Number of bits to skip
>>>> +    pkt->data[1] = c->last_frame_bits;
>>> wastes 7bit ;)
>>> you could avoid this (iam not suggesting you actually do ..)
>>> by filling the bits to skip by
>>> 1   (for 1)
>>> 01  (for 2)
>>> ...
>>> 00000001 (for 8)
>> I also thought about this for a moment, but didn't find a cleaner way to do 
>> it (and a single byte is cheap ;)
>>
>> I've also added CODEC_ID_TWINVQ to avcodec.h in this new patch, so it 
>> compiles cleanly.
> [...]
>> +static void add_metadata(AVFormatContext *s, const char *tag, int tag_len,
>> +                         int remaining)
>> +{
>> +    char buf[2048];
>> +    int len = FFMIN3(tag_len, remaining, sizeof(buf) - 1);
>> +
>> +    if (len != tag_len)
>> +        av_log(s, AV_LOG_ERROR, "Warning: truncating metadata!\n");
>> +
>> +    get_buffer(s->pb, buf, len);
>> +    buf[len] = 0;
> 
> len can end negative here

Here it is useful to have it unsigned...

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