[FFmpeg-devel] [PATCH] Apple RPZA encoder

[Diego Biurrun]

On Tue, Mar 24, 2009 at 02:08:44PM +0530, Jai Menon wrote:
> On 3/24/09, Diego Biurrun <diego at biurrun.de> wrote:
> > On Tue, Mar 24, 2009 at 10:33:19AM +0530, Jai Menon wrote:
> >  >
> >  > Attached patch is a cleaned up version of the original one posted by
> >  > Todd Kirby [1].
> >  > And, yeah, its a gsoc qualification task :)
> >  >
> >  > [1] http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2005-June/001673.html
> >  >
> >
> > > --- libavcodec/rpzaenc.c      (revision 0)
> >  > +++ libavcodec/rpzaenc.c      (revision 0)
> >  > @@ -0,0 +1,848 @@
> >  > +/*
> >  > + * Quicktime RPZA Video Encoder.
> >
> >  QuickTime RPZA video encoder

unchanged

> >  > + * QT RPZA Video Encoder
> >
> >  QuickTime RPZA video encoder
> 
> fixed

No.

> >  > +typedef enum { BLUE,
> >  > +               GREEN,
> >  > +               RED,
> >  > +} channel_offset;
> >
> >  ditto

unchanged

> >  > +    if (r > 31) {
> >  > +        r = 31;
> >  > +    }
> >  > +    if (g > 31) {
> >  > +        g = 31;
> >  > +    }
> >  > +    if (b > 31) {
> >  > +        b = 31;
> >  > +    }
> >
> >  useless {}, same below
> 
> removed

but many more remain

> >  > +    bi.image_width = s->frame_width;
> >  > +    bi.image_height = s->frame_height;
> >  > +    bi.rowstride = pict->linesize[0];
> >  > +
> >  > +    bi.blocks_per_row = (s->frame_width + 3) >> 2;
> >
> >  align
> 
> aligned

But many more opportunities for alignment exist throughout your patch..

> revised patch attached

This revised patch leaves a few things to be desired.  You are expected
not only to address the issues pointed out, but also to look for similar
issues in the rest of your code.


> --- Changelog	(revision 18140)
> +++ Changelog	(working copy)
> @@ -6,9 +6,9 @@
>  - Alpha channel scaler
>  - PCX encoder
> +- QuickTime RPZA video encoder
>  
>  
> -
>  version 0.5:

unwanted cosmetics

> --- libavcodec/rpzaenc.c	(revision 0)
> +++ libavcodec/rpzaenc.c	(revision 0)
> @@ -0,0 +1,848 @@
> +            min_r = FFMIN(block_ptr[(x * PIXELSTRIDE) + 2], min_r);
> +            min_g = FFMIN(block_ptr[(x * PIXELSTRIDE) + 1], min_g);
> +            min_b = FFMIN(block_ptr[(x * PIXELSTRIDE) + 0], min_b);
> +
> +            max_r = FFMAX(block_ptr[(x * PIXELSTRIDE) + 2], max_r);
> +            max_g = FFMAX(block_ptr[(x * PIXELSTRIDE) + 1], max_g);
> +            max_b = FFMAX(block_ptr[(x * PIXELSTRIDE) + 0], max_b);

possibly superfluous ()

> +AVCodec rpza_encoder = {
> +    .long_name = NULL_IF_CONFIG_SMALL("QuickTime RPZA"),

not the same as the decoder

Diego