[Ffmpeg-devel] [RFC] Fraps v2 support

Michael Niedermayer michaelni
Fri Nov 3 21:02:49 CET 2006


Hi

On Fri, Nov 03, 2006 at 03:00:51PM +0200, Kostya wrote:
> On Thu, Nov 02, 2006 at 07:52:23PM +0100, Michael Niedermayer wrote:
> > Hi
> > 
> > On Thu, Nov 02, 2006 at 08:24:46PM +0200, Kostya wrote:
> > > Please test if patch attached confirms to $subj. I have only a bit of one
> > > sample here.
> > 
> > [...]
> > 
> > > +    cur_node = 510; // for futher use in huffman decoding
> > > +    /* we have built Huffman table and are ready to decode plane */
> > > +    /* XXX this bitreader ignores every 32th bit so lavc bitreader cannot be used*/
> > 
> > hmm i can belive it ... are you sure that they ignore every 32th bit??
> > if its really true then id suggest that you use a temporary buffer and 
> > convert that 31/32bit block stream to a normal one, i guess that this 
> > with the get_vlc() code from lavc  is still faster ...
> 
> Sorry, I misunderstood it. New patch use default bitreader on bswapped() buffer
> (and ALT_BITSTREAM_READER_LE returns bits in reverse order)
> As a side effect decoder spends ~40% time less on decoding.

[...]
>  #define FPS_TAG MKTAG('F', 'P', 'S', 'x')
>  
> +/* symbol for Huffman tree node */
> +#define HNODE -1
> +
>  /**
> + * Huffman node
> + * FIXME one day this should belong to one general framework
> + */
> +typedef struct Node{
> +    int sym;
> +    int count;
> +    int n0;
> +    int n1;
> +}Node;

sym,n0,n1 dont need an int


[...]
> +/**
> + * Comparator - our nodes should ascend by count
> + * but with preserved symbol order
> + */
> +static int huff_cmp(const Node *a, const Node *b){
> +    if(a->count == b->count){
> +        return (a->sym > b->sym) ? 1 : -1;
> +    }
> +    return (a->count > b->count) ? 1 : -1;

return (a->count - b->count)*256 + a->sym - b->sym;


[...]
> +    for(i = 0; i < 256; i++){
> +        s->nodes[i].sym = i;
> +        s->nodes[i].count = LE_32(src);
> +        s->nodes[i].n0 = s->nodes[i].n1 = -1;
> +        src += 4;
> +    }
> +    size -= 1024;
> +    qsort(s->nodes, 256, sizeof(Node), huff_cmp);
> +    cur_node = 256;
> +    // FIXME how it will handle nodes with zero count?
> +    for(i = 0; i < 511; i += 2){
> +        s->nodes[cur_node].sym = HNODE;
> +        s->nodes[cur_node].count = s->nodes[i].count + s->nodes[i+1].count;
> +        s->nodes[cur_node].n0 = i;
> +        s->nodes[cur_node].n1 = i + 1;

it doesnt seem that n1 is ever changed, it will always be n0+1 so its
redundant
and n0,sym will either be -1,0..255 or 0..510,-1 so they could be merged
into one variable


> +        for(j = cur_node; j > 0; j--){
> +            if(s->nodes[j].count >= s->nodes[j - 1].count) break;
> +            tmp = s->nodes[j];
> +            s->nodes[j] = s->nodes[j - 1];
> +            s->nodes[j - 1] = tmp;

(FF)SWAP()


> +        }

there is a possible problem here, and that is that the sum of the 2 counts
can overflow, if it does then this loop will reorder too many elements 
and by doing that trash the n0/n1 pointers which then a few lines later
could lead to a stack overflow due the recursive vlc building being
run on a tree which isnt one as it contains a loop from the corruption


> +        cur_node++;

i is always 2*cur_node-512 so maybe one of them could be set based on the
other, like
int i= 2*cur_node-512; 
at the top of the loop maybe thats more readable, iam not sure though ...


[...]
> +        s->tmpbuf = av_realloc(s->tmpbuf, offs[1] - offs[0] - 1024);
> +        if(fraps2_decode_plane(s, f->data[0], f->linesize[0], avctx->width,
> +                avctx->height, buf + offs[0], offs[1] - offs[0], 0) < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Error decoding plane 0\n");
> +            return -1;
> +        }
> +        s->tmpbuf = av_realloc(s->tmpbuf, offs[2] - offs[1] - 1024);
> +        if(fraps2_decode_plane(s, f->data[1], f->linesize[1], avctx->width/2,
> +                avctx->height/2, buf + offs[1], offs[2] - offs[1], 1) < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Error decoding plane 1\n");
> +            return -1;
> +        }
> +        s->tmpbuf = av_realloc(s->tmpbuf, buf_size - offs[2] - 1024);
> +        if(fraps2_decode_plane(s, f->data[2], f->linesize[1], avctx->width/2,
> +                avctx->height/2, buf + offs[2], buf_size - offs[2], 1) < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Error decoding plane 2\n");
> +            return -1;
> +        }

code triplification, imho make a loop out of the 3

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is




More information about the ffmpeg-devel mailing list