[FFmpeg-devel] [PATCH] Add crop filter

[Michael Niedermayer]

On Mon, Oct 12, 2009 at 08:41:08PM +0200, Stefano Sabatini wrote:
> On date Monday 2009-10-12 09:28:29 +0200, Michael Niedermayer encoded:
> > On Sun, Oct 11, 2009 at 09:40:40PM +0200, Stefano Sabatini wrote:
> > > On date Saturday 2009-10-10 01:51:39 +0200, Michael Niedermayer encoded:
> > > > On Wed, Oct 07, 2009 at 11:50:19PM +0200, Stefano Sabatini wrote:
> > > > > On date Tuesday 2009-10-06 15:01:32 +0200, Michael Niedermayer encoded:
> > > > > > On Tue, Oct 06, 2009 at 12:58:41AM +0200, Stefano Sabatini wrote:
> > [...]
> > > +#define SET_OFFSET(x, max)                                              \
> > > +    if (x < 0)                                                          \
> > > +        x = max + x;                                                    \
> > > +    if (x < 0 || x > max-1) {                                           \
> > > +        av_log(ctx, AV_LOG_ERROR, "Value for offset %d out of the valid range 0 - %d\n", x, max-1); \
> > > +        return -1;                                                      \
> > > +    }
> > > +
> > > +static int norm_size(int x, int max, int max_contained)
> > > +{
> > > +    if (x == 0)
> > > +        x = max;
> > > +    if (x < 0)
> > > +        x = max + x;
> > > +    return av_clip(x, 1, max_contained);
> > > +}
> > > +
> > > +static int config_input(AVFilterLink *link)
> > > +{
> > > +    AVFilterContext *ctx = link->dst;
> > > +    CropContext *crop = ctx->priv;
> > > +
> > > +    SET_OFFSET(crop->x, link->w);
> > > +    SET_OFFSET(crop->y, link->h);
> > > +
> > > +    crop->w = norm_size(crop->w, link->w, link->w - crop->x);
> > > +    crop->h = norm_size(crop->h, link->h, link->h - crop->y);
> > 
> > mess
> > 
> > if(   crop->x < 0 || crop->y < 0 || crop->w < 0 || crop->h < 0
> >    || (unsigned)crop->x + (unsigned)crop->w > link->w
> >    || (unsigned)crop->y + (unsigned)crop->h > link->h)
> 
> Bah, this is missing even the original filter facility (size == 0 ->
> size = max allowed)

not at all, it just verifies that the values are correct, a size of 0
is not.
the user parameter of 0 must be changed to size before this of course
its your twisted code that tried to check validity before calculating the
values that are being checked


> and doesn't provide any hints about what the user
> is doing wrong in that case (just a laconic: somethin's wrong).

you can split that check if you think it makes sense


> 
> So specify which semantics you want implemented, that's clearly not
> what I documented in the texi file.

Iam trying to get the filters into svn and for that keeping things simple
and discussing extensions seperately is likely a good idea, that said i
dont mind the =0 case


[...]
> > [...]
> > > +static void start_frame(AVFilterLink *link, AVFilterPicRef *picref)
> > > +{
> > > +    CropContext *crop = link->dst->priv;
> > > +    AVFilterPicRef *ref2 = avfilter_ref_pic(picref, ~0);
> > > +    int i;
> > > +
> > > +    ref2->w        = crop->w;
> > > +    ref2->h        = crop->h;
> > > +
> > > +    switch (link->format) {
> > 
> > > +    case PIX_FMT_MONOWHITE:
> > > +    case PIX_FMT_MONOBLACK:
> > > +        ref2->data[0] += (crop->y * ref2->linesize[0]) + (crop->x >> 3);
> > > +        break;
> > > +
> > > +    case PIX_FMT_PAL8:
> > > +        ref2->data[0] += crop->y * ref2->linesize[0] + crop->x;
> > > +        break;
> > > +
> > > +    default:
> > > +        ref2->data[0] += crop->y * ref2->linesize[0];
> > > +        ref2->data[0] += crop->x * crop->bpp;
> > 
> > that looks factorizeable
> 
> The alternative I get is less readable though:
> 
>     ref2->data[0] += (crop->y * ref2->linesize[0]);
>     if (link->format == PIX_FMT_MONOWHITE || PIX_FMT_MONOBLACK)
>         ref2->data[0] += (crop->x >> 3);
>     else
>         ref2->data[0] += crop->x * crop->bpp;

ref2->data[0] += (crop->x * crop->bpp)>>3;

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091013/8d571e47/attachment.pgp>