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

[Michael Niedermayer]

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;



> +            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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070626/235dd33b/attachment.pgp>