[FFmpeg-devel] [PATCH] RoQ video encoder, take 2

[Reimar Döffinger]

Hello,
On Sun, May 27, 2007 at 12:21:03PM +0200, Vitor wrote:
> +#define SQUARE(x) ((x)*(x))

I think you are using this always on the same kinds of data, so I'd
consider making this a static inline function, at low optimization
levels this would avoid evaluating x twice.

> +typedef struct
> +{
> +    roq_cell block[4];
> +} roq_cell4;

Hmm.. Did you compare this both in code complexity and speed to just
using roq_cell *a or roq_cell a[4]?

> +typedef struct
> +{
> +    uint32_t eval_se[4];
> +
> +    uint8_t subCels[4];
> +
> +    int8_t motionX, motionY;
> +    uint8_t cbEntry;
> +} roq_subcel_evaluation_t;
> +
> +typedef struct
> +{
> +    uint32_t eval_se[3];
> +
> +    roq_subcel_evaluation_t subCels[4];
> +
> +    int8_t motionX, motionY;
> +    uint8_t cbEntry;
> +
> +    uint32_t sourceX, sourceY;
> +} cel_evaluation_t;

Unless there is a special reason not to, try using just "int" instead of
"int8_t".

> +/**
> + * 4*4*4*4 subdivide types + 3 non-subdivide main types
> + */
> +#define ROQ_MAX_POSSIBILITIES    259

Then why not just
> +#define ROQ_MAX_POSSIBILITIES    (4*4*4*4+3)

> +#define COPY_SINGLE_BLOCK(BLOCK, DATA, TOP, LEFT, LINEW, LINEWU, LINEWV)\
> +    do { \
> +        (BLOCK).y[0] = (DATA)[0][(TOP  )*(LINEW )   + LEFT    ]; \
> +        (BLOCK).y[1] = (DATA)[0][(TOP  )*(LINEW )   + LEFT + 1]; \
> +        (BLOCK).y[2] = (DATA)[0][(TOP+1)*(LINEW )   + LEFT    ]; \
> +        (BLOCK).y[3] = (DATA)[0][(TOP+1)*(LINEW )   + LEFT + 1]; \
> +        (BLOCK).u    = (DATA)[1][(TOP  )*(LINEWU)/2 + (LEFT)/2]; \
> +        (BLOCK).v    = (DATA)[2][(TOP  )*(LINEWV)/2 + (LEFT)/2]; \
> +    } while(0)

Does using a macro have a real advantage over a static inline function?
And also, since DATA is an array, why not use LINEW as an array as well
instead of split into LINEW, LINEWU and LINEWV?
I think some other macros could be converted into functions as well.

> +    /* Sort primarily by validity */
> +    if (!so1[0]->valid) {
> +        if (!so2[0]->valid)
> +            return 0;
> +        else
> +            return 1;

return !so2[0]->valid;

> +    if ((avctx->width & 0xf) || (avctx->height & 0xf)) {
> +        av_log(avctx, AV_LOG_ERROR, "Dimensions must be divisible by 16\n");
> +        return -1;
> +    }
> +
> +    if (((avctx->width)&(avctx->width-1)) ||
> +        ((avctx->height)&(avctx->height-1)))
> +        av_log(avctx, AV_LOG_ERROR, "Warning: dimensions not power of two\n");
> +
> +    if ((avctx->width > 65535) || (avctx->height > 65535))  {
> +        av_log(avctx, AV_LOG_ERROR, "Dimension must be each < 65536\n");
> +        return -1;
> +    }
> +
> +    if ((avctx->width == 0)  || (avctx->height == 0)) {
> +        av_log(avctx, AV_LOG_ERROR, "Dimensions can not be null\n");
> +        return -1;
> +    }

Lots of useless (), I also think you should probably use
avcodec_check_dimensions somewhere.

[...]
> +#define get_word(in_buffer) ((unsigned short)(in_buffer += 2, \
> +  (in_buffer[-1] << 8 | in_buffer[-2])))
> +#define get_long(in_buffer) ((unsigned long)(in_buffer += 4, \
> +  (in_buffer[-1] << 24 | in_buffer[-2] << 16 | in_buffer[-3] << 8 | in_buffer[-4])))

I don't think these are valid C.
Are these so speed critical that you can't use bytestream functions?

> +    for(i = -512; i < 512; i++)
> +        s->uiclp[i] = (i < 0 ? 0 : (i > 255 ? 255 : i));

av_clip_uint8 ?

Hmm... these seem to be there in the current code already though...

[...]
> +    AVFrame *last_frame;
> +    AVFrame *current_frame;
> +
> +    uint8_t **last_frame_data;
> +    uint8_t **current_frame_data;

Are these two really needed, can't you just use the AVFrames above as in
the decoder?
At least they should be uint8_t *last_frame_data[3]; IMO, thus avoiding
an extra malloc and indirection step on access. I don't think it matters
much if you copy 4/8 or 12/24 bytes on swapping them.

Greetings,
Reimar D?ffinger