[FFmpeg-devel] [PATCH] VQF demuxer

[Vitor Sessak]

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

> 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

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

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

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