[FFmpeg-devel] [PATCH] Apple RPZA encoder

[Jai Menon]

On 3/24/09, Diego Biurrun <diego at biurrun.de> wrote:
> 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
>  > >  >

[...]

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

IMHO alignment is only of concern if the code is unreadable without
the extra cosmetic spaces. I feel that the code right now is quite
readable. If you feel otherwise, please mention the relevant section
of the hunk.

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

I'll review the code again, and wait for other comments while I'm at it.

[...]

>  > --- 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 ()

As I said, for readability, but I'll remove these.

[...]

-- 
Regards,

Jai