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

[Nielkie]

On Thu, Nov 25, 2010 at 10:27 PM, Tomas H?rdin <tomas.hardin at codemill.se>wrote:

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

Done


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

I honestly don't know what it is referring to. I just removed the comment as
it looks like has been done in other implementations. (If anyone has any
other ideas...)


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

I left it in the function as that's the style in all the other filters.


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


No, then it falls back to avfilter_default_draw_slice, which passes on the
slice unfiltered. It's done the same in other filters - the idea is to wait
for end_frame and process the frame all at once, as I couldn't figure out a
good way to apply the transformation slice-by-slice.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: application/octet-stream
Size: 15013 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101126/21efc9a0/attachment.obj>