[FFmpeg-devel] [Patch] QT RLE encoder, bis

[mark cox]

On 26/06/07, Michael Niedermayer <michaelni at gmx.at> wrote:
>
> Hi
>
> On Mon, Jun 25, 2007 at 11:18:15PM +0200, Alexis Ballier wrote:
> [...]
> > >
> > >[...]
> > >> +/** Dumps frame including header */
> > >> +static int dump_frame(QtrleEncContext *s, AVFrame *p, uint8_t *buf)
> > >
> > >encode_frame()
> >
> > not sure what you meant there, I just renamed it to encode_frame.
> > Do you mean that this should be merged into qtrle_encode_frame() ?
>
> no i just meant that dump_frame() is a bad name
>
>
> >
> > >> +{
> > >> +    int i;
> > >> +    int start_line=0;
> > >> +    int end_line=s->avctx->height;
> > >> +    uint8_t *orig_buf=buf;
> > >> +
> > >> +    if(!((s->frame).key_frame)){
> > >
> > >superflous () and there are many more in the patch
> >
> > Yep, most likely that I'm too paranoid with the parser that might
> > understand it wrongly, I removed some other () aswell
>
> gcc is broken but not that broken
>
>
> >
> > >> +
> > >> +    /* save the current frame */
> > >> +    memcpy(s->previous_frame, p->data[0],
> > >avctx->height*avctx->width*s->pixel_size);
> > >
> > >this is wrong if linesize != width*pixel_size
> >
> > Indeed
> > I changed the "previous_frame" to an AVPicture, copying it with
> > av_picture_copy, I hope I got it right this time [one of the previous
> > patches used img_copy which I didn't want to use due to the attribute
> > deprecated]
> >
> > The other minor things should also be fixed in enclosed patch
> >
> >
> > In the end, the encoding time is almost divided by 2, so I just
> > removed the heuristic and the hack to switch between the optimal &
> > heuristic. Basic tests showed that the speed gain is even more
> > important with screencasts (difference between pictures is very small
> > thus not searching for every possible bulk copy helps a lot), I can
> > now do realtime encoding with this algorithm at 1024x768 at 25fps :)
>
>
> [...]
>
> > +    if (avpicture_alloc(&s->previous_frame, avctx->pix_fmt,
> avctx->width, avctx->height) < 0) {
> > +        av_log(avctx, AV_LOG_ERROR, "Error allocating picture\n");
> > +        return -1;
> > +    }
>
> why not AVCodecContext.buf_buffer()?
> no, theres not much difference for an encoder, avpicture_alloc() should
> work
> too
>
>
> [...]
> > +/**
> > + * Computes the best RLE sequence for a line
> > + */
> > +static void qtrle_dump_line(QtrleEncContext *s, AVFrame *p, int line,
> uint8_t **buf)
>
> encode_line() or something else without "dump" seems to be a better name
> dump reminds me of dumping the thing as a hex values to stderr
>
>
> [...]
> > +    uint8_t *this_line = p->data[0]+(line*p->linesize[0]) +
> (width-1)*s->pixel_size;
> > +    uint8_t *prev_line =
> s->previous_frame.data[0]+(line*p->linesize[0]) + (width-1)*s->pixel_size;
>
> superflous ()
>
>
> [...]
> > +        if((!s->frame.key_frame) && (!memcmp(this_line, prev_line,
> s->pixel_size)))
>
> superflous ()
>
>
> [...]
> > +        total_skip_cost = (s->length_table[i+skipcount] + 2);
>
> superflous ()
>
>
> [...]
> > +        if((repeatcount > 1) && ((skipcount==0) || (total_repeat_cost <
> total_skip_cost))){
>
> superflous ()
>
>
> [...]
> > +        else{
> > +            /* We cannot do neither skip nor repeat
> > +             * thus we search for the best bulk copy to do */
> > +
> > +            limit = FFMIN(width-i,MAX_RLE_BULK);
>
> the declaration could be merged (int limit = FFMIN...)
>
>
> > +
> > +            if(i==0) temp_cost = 2 + s->pixel_size;
> > +            else temp_cost = 1 + s->pixel_size;
>
> these can be aligned like:
> if(i==0) temp_cost = 2 + s->pixel_size;
> else     temp_cost = 1 + s->pixel_size;


how about

temp_cost = s->pixel_size + i ? 1 : 2  ;

> +            total_bulk_cost = s->length_table[i+1] + temp_cost;
> > +            bulkcount  = 1;
> > +
> > +            for(j=2;j<=limit;j++){
> > +                temp_cost += s->pixel_size;
> > +                if(s->length_table[i+j] + temp_cost < total_bulk_cost){
> > +                    /* We have found a better bulk copy ... */
> > +                    total_bulk_cost = s->length_table[i+j] + temp_cost;
> > +                    bulkcount = j;
> > +                }
> > +            }
>
> the loop could begin with j=1 and total_bulk_cost could be set to INT_MAX
> that would avoid the bulkcount  = 1
>
>
> [...]
> >
> +            bytestream_put_buffer(buf,this_line+(i*s->pixel_size),s->pixel_size);
>
> superflous ()
>
>
> [...]
> > +    bytestream_put_be32(&buf, 0);                     // CHUNK SIZE,
> patched later
> > +
>
> trailing whitespace is forbidden in svn
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> No human being will ever know the Truth, for even if they happen to say it
> by chance, they would not even known they had done so. -- Xenophanes
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>
>