[FFmpeg-devel] [RFC] Alpha support

[Reimar Döffinger]

On Sat, Jan 17, 2009 at 03:03:12PM +0100, C?dric Schieli wrote:
> +            Y1 += lumSrc[j][i2] * lumFilter[j];\
> +            Y2 += lumSrc[j][i2+1] * lumFilter[j];\
> +            A1 += alpSrc[j][i2] * lumFilter[j];\
> +            A2 += alpSrc[j][i2+1] * lumFilter[j];\
> +        }\
> +        for (j=0; j<chrFilterSize; j++)\
> +        {\
> +            U += chrSrc[j][i] * chrFilter[j];\
> +            V += chrSrc[j][i+VOFW] * chrFilter[j];\

Align those so that the identical parts are always exactly below each
other.

> +            if (Y1>255)   Y1=255; \
> +            else if (Y1<0)Y1=0;   \
> +            if (Y2>255)   Y2=255; \
> +            else if (Y2<0)Y2=0;   \
> +            if (U>255)    U=255;  \
> +            else if (U<0) U=0;    \
> +            if (V>255)    V=255;  \
> +            else if (V<0) V=0;    \
> +            if (A1>255)   A1=255; \
> +            else if (A1<0)A1=0;   \
> +            if (A2>255)   A2=255; \
> +            else if (A2<0)A2=0;   \

av_clip_uint8

> +            if (R>=(256<<22))   R=(256<<22)-1; \
> +            else if (R<0)R=0;   \
> +            if (G>=(256<<22))   G=(256<<22)-1; \
> +            else if (G<0)G=0;   \
> +            if (B>=(256<<22))   B=(256<<22)-1; \
> +            else if (B<0)B=0;   \
> +            if (A>=(256<<22))   A=(256<<22)-1; \
> +            else if (A<0)A=0;   \

av_clip

> Index: ffmpeg/libswscale/swscale_template.c
> ===================================================================
> --- ffmpeg.orig/libswscale/swscale_template.c	2009-01-17 14:45:18.105322867 +0100
> +++ ffmpeg/libswscale/swscale_template.c	2009-01-17 14:45:26.301323127 +0100

Looks like only indentation changes?

> +#define MMX_BLENDING(areg, ash1, ash2, reg1, sh1, reg2, sh2, reg3, sh3)   \

Some comment on this macro/the meaning of the arguments would probably
be good.

> +    uint8_t *dd = dst->data[0], *ss = src->data[0];
> +    uint8_t *d, *s, *end;
> +    int w = width << 2;
> +    #if HAVE_MMX
> +        const uint8_t *mm_end;
> +    #endif

That indentation is weird.
Also I think the # of the preprocessor stuff must be in the first
column. There are a lot more places like that.

> +const char *alpha_symbols[]={

static

> +typedef struct
> +{
> +    AVEvalExpr *value;
> +    double values[POV_NULL+1];
> +} AlphaContext;

Also if you do it here, I'd consider it good style if alpha_symbols was
explicitly sized POV_NULL+1

> +static int init(AVFilterContext *ctx, const char *args, void *opaque)

av_cold?

> +AVFilter avfilter_vf_alpha =
> +{
> +    .name      = "alpha",
> +
> +    .init      = init,
> +
> +    .priv_size = sizeof(AlphaContext),
> +
> +    .query_formats = query_formats,
> +
> +    .inputs    = (AVFilterPad[]) {{ .name            = "default",
> +                                    .type            = CODEC_TYPE_VIDEO,
> +                                    .start_frame     = start_frame,
> +                                    .end_frame       = end_frame,
> +                                    .min_perms       = AV_PERM_WRITE |
> +                                                       AV_PERM_READ,
> +                                    .rej_perms       = AV_PERM_REUSE |
> +                                                       AV_PERM_REUSE2},
> +                                  { .name = NULL}},
> +    .outputs   = (AVFilterPad[]) {{ .name            = "default",
> +                                    .type            = CODEC_TYPE_VIDEO, },
> +                                  { .name = NULL}},
> +};

To avoid the same mistakes (at least I consider it as such) as for libavcodec
design: why aren't these all const?

Greetings,
Reimar D?ffinger