[FFmpeg-devel] [PATCH] Apple RPZA encoder

[Michael Niedermayer]

On Tue, Mar 24, 2009 at 03:39:11PM +0530, Jai Menon wrote:
> On 3/24/09, Jai Menon <jmenon86 at gmail.com> wrote:
> > Hi,
> >
> >  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
> 
> the isom.c hunk is obviously not required, sorry for the confusion.
> correct patch attached.
[...]
> +#include "avcodec.h"
> +#include "dsputil.h"
> +#include "bitstream.h"
> +#include "assert.h"
> +
> +typedef struct RpzaContext {
> +    AVCodecContext *avctx;
> +    DSPContext dsp;
> +    AVFrame current_frame;  ///< buffer for current 24 bit source frame
> +    AVFrame prev_frame;     ///< buffer for previous 24 bit source frame
> +    PutBitContext pb;       ///< buffer for encoded frame data.
> +    int frame_width;        ///< width in pixels of source frame
> +    int frame_height;       ///< height in pixesl of source frame

> +    unsigned char *buf;

this looks a little odd as only undocumenetd field


> +    int first_frame;        ///< flag set to one when the first frame is being processed
> +                            ///< so that comparisons with previous frame data are not attempted

looks useless


[...]
> +/* 8 bit rounding constants */

not doxygen compatible

> +#define ROUND_UP        7
> +#define ROUND_NEAREST   4
> +#define ROUND_DOWN      0
> +

> +/* tuning parameters */
> +#define SIXTEEN_COLOR_THRESH        24
> +#define SKIP_FRAME_THRESH           13
> +#define START_ONE_COLOR_THRESH      8
> +#define CONTINUE_ONE_COLOR_THRESH   0

Encoders, especially such simple ones should be build to be RD optimal
in as many aspects as possible

[...]
> +static void get_colors(uint8_t *colorB, uint8_t *colorA, uint8_t color4[4][3])
> +{
> +    uint8_t step;
> +
> +    color4[0][0] = colorB[0];
> +    color4[0][1] = colorB[1];
> +    color4[0][2] = colorB[2];
> +
> +    color4[3][0] = colorA[0];
> +    color4[3][1] = colorA[1];
> +    color4[3][2] = colorA[2];
> +
> +    // red components
> +    step = (color4[3][0] - color4[0][0] + 1) / 3;

division is slow, if this is speed relevant a LUT could be used

[...]
> +/**
> + * Round a 24 bit rgb value to a 15 bit rgb value. The bias parameter
> + * specifies the rounding direction.
> + */
> +static uint16_t round_rgb24_to_rgb555(uint8_t * rgb24, int bias)
> +{
> +    uint16_t rgb555 = 0;
> +    uint32_t r, g, b;
> +
> +    r = (uint32_t)rgb24[0] + bias;
> +    g = (uint32_t)rgb24[1] + bias;
> +    b = (uint32_t)rgb24[2] + bias;
> +
> +    r = r >> 3;
> +    g = g >> 3;
> +    b = b >> 3;
> +
> +    /* clamp 0-31 */
> +    if (r > 31)
> +        r = 31;
> +
> +    if (g > 31)
> +        g = 31;
> +
> +    if (b > 31)
> +        b = 31;

this is incorrect, where it correct no clipig would be
needed

[...]
> +/**
> + * Returns the total difference between two 24 bit color values.
> + */
> +static int diff_colors(uint8_t *colorA, uint8_t *colorB)
> +{
> +    int tot;
> +    tot  = SQR(colorA[0] - colorB[0]);
> +    tot += SQR(colorA[1] - colorB[1]);
> +    tot += SQR(colorA[2] - colorB[2]);
> +    return tot;
> +}

useless intermediate var

[...]
> +    s->prev_frame.linesize[0] = pict->linesize[0];

this looks wrong

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090324/63b76f6b/attachment.pgp>