[FFmpeg-devel] [PATCH] VQF demuxer

[Michael Niedermayer]

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


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


>
>>> +
>>> +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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Thouse who are best at talking, realize last or never when they are wrong.
-------------- 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/2ca2e140/attachment.pgp>