[FFmpeg-devel] [RFC] Alpha support

[Cédric Schieli]

Hello,

2009/1/17 Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>

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

Fixed. This code was copied from swscale.c, so it has to be fixed there
also.


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

Fixed. Same as above.


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

Fixed. Same as above.


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

Yes, I've split the re-indentation from previous patch to make it more
readable.


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

Hopefully fixed.


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

Fixed.


>
> > +const char *alpha_symbols[]={
>
> static
>

Fixed.


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

Fixed.


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

None of the current filters does it. IMHO yes, but I'll wait for a
consensus.


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

Same as previous.


>
> Greetings,
> Reimar D?ffinger
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>


Regards,
C?dric Schieli
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sws_use_alpha.patch
Type: text/x-patch
Size: 45806 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090117/7fa6523c/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: av_picture_blend.patch
Type: text/x-patch
Size: 11994 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090117/7fa6523c/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vf_alpha.patch
Type: text/x-patch
Size: 7265 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090117/7fa6523c/attachment-0002.bin>