[FFmpeg-devel] [PATCH] Port MPlayer 2xSaI filter to libavfilter

[Tomas Härdin]

On Thu, 2010-11-25 at 21:11 +1000, Nielkie wrote:
> On Thu, Nov 25, 2010 at 4:01 AM, Loren Merritt <lorenm at u.washington.edu>wrote:
> 
> > On Thu, 25 Nov 2010, Nielkie wrote:
> >
> >  +#define makecol(r, g, b) (r+(g<<8)+(b<<16))
> >> +
> >> +static int Init_2xSaI(enum PixelFormat pix_fmt, Super2xSaIContext *c)
> >> +{
> >> +    int minr = 0, ming = 0, minb = 0;
> >> +    int i;
> >> +
> >> +    /* Get lowest color bit */
> >> +    for (i = 0; i < 255; i++) {
> >> +        if (!minr)
> >> +            minr = makecol(i, 0, 0);
> >> +        if (!ming)
> >> +            ming = makecol(0, i, 0);
> >> +        if (!minb)
> >> +            minb = makecol(0, 0, i);
> >> +    }
> >> +
> >> +    c->colorMask = (makecol(255, 0, 0) - minr) |
> >> +                   (makecol(0, 255, 0) - ming) |
> >> +                   (makecol(0, 0, 255) - minb);
> >> +    c->lowPixelMask = minr | ming | minb;
> >> +    c->qcolorMask = (makecol(255, 0, 0) - 3 * minr) |
> >> +                    (makecol(0, 255, 0) - 3 * ming) |
> >> +                    (makecol(0, 0, 255) - 3 * minb);
> >> +    c->qlowpixelMask = (minr * 3) | (ming * 3) | (minb * 3);
> >>
> >
> > What's all this for? colorMask at this point is just a constant 0xfefefe,
> > and so on.
> >
> > Is this filter supposed to support RGBA, or RGB32? If RGBA, then masking
> > off the alpha channel when interpolating is wrong. If RGB32, then not
> > masking off the alpha channel before comparing pixel equality is wrong.
> > Either way, I don't think colorMask etc need to vary with colorspace. You
> > could just always interpolate all 4 channels.
> >
> >  +#define Q_INTERPOLATE(A, B, C, D) ((A & c->qcolorMask) >> 2) + ((B &
> >> c->qcolorMask) >> 2) \
> >> +                                + ((C & c->qcolorMask) >> 2) + ((D &
> >> c->qcolorMask) >> 2) \
> >> +                                + ((((A & c->qlowpixelMask) + (B &
> >> c->qlowpixelMask) + \
> >> +                                     (C & c->qlowpixelMask) + (D &
> >> c->qlowpixelMask)) >> 2) & c->qlowpixelMask)
> >>
> >
> > More complex than necessary when it's never actually given 4 different
> > args.
> >
> >  +AVFilter avfilter_vf_super2xsai =
> >>
> >
> > needs a .description
> >
> > --Loren Merritt
> >
> >
> Thanks, updated. Let me know if there is anything else.
> 
> diff --git a/libavfilter/vf_super2xsai.c b/libavfilter/vf_super2xsai.c
> new file mode 100644
> index 0000000..c80a71c
> --- /dev/null
> +++ b/libavfilter/vf_super2xsai.c
> @@ -0,0 +1,326 @@

> +static int Init_2xSaI(enum PixelFormat pix_fmt, Super2xSaIContext *c)
> {
> ...
> }

Roll this into config_input()?

> +static void Super2xSaI_ex(Super2xSaIContext *c,
> +                          uint8_t *src, uint32_t src_pitch,
> +                          uint8_t *dst, uint32_t dst_pitch,
> +                          uint32_t width, uint32_t height)
> +{
> ...
> +    for (y = 0; y < height; y++) {
> +        uint8_t *dst_line[2];
> +
> +        dst_line[0] = dst + dst_pitch*2*y;
> +        dst_line[1] = dst + dst_pitch*(2*y+1);
> +
> +        /* Todo: x = width - 2, x = width - 1 */

Fix it?

> ...
> +        for (x = 0; x < width; x++) {
> +
> +            /* Calculate the 4 colors for the current position using
> information from the color matrix */
> +            uint32_t product1a, product1b, product2a, product2b;
> +
> +//---------------------------------------  B0 B1 B2 B3    0  1  2  3
> +//                                         4  5* 6  S2 -> 4  5* 6  7
> +//                                         1  2  3  S1    8  9 10 11
> +//                                         A0 A1 A2 A3   12 13 14 15
> +//--------------------------------------

Weird indentation

> ...
> +
> +static int query_formats(AVFilterContext *ctx)
> +{
> +    enum PixelFormat pix_fmts[] = {
> +        PIX_FMT_RGBA, PIX_FMT_BGRA, PIX_FMT_ARGB, PIX_FMT_ABGR,
> +        PIX_FMT_RGB24, PIX_FMT_BGR24,
> +        PIX_FMT_RGB565, PIX_FMT_BGR565, PIX_FMT_RGB555,
> PIX_FMT_BGR555,
> +        PIX_FMT_NONE
> +    };
> +
> +    avfilter_set_common_formats(ctx,
> avfilter_make_format_list(pix_fmts));
> +    return 0;
> +}

static const enum PixelFormat pix_fmts[]
Also, maybe put it outside the function?

> +static void null_draw_slice(AVFilterLink *inlink, int y, int h, int
> slice_dir) { }

This seems odd. Can't you just specify NULL instead of this stub?

/Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101125/1949be80/attachment.pgp>