[FFmpeg-devel] [PATCH] Add crop filter

[Michael Niedermayer]

On Fri, Oct 16, 2009 at 12:34:07AM +0200, Stefano Sabatini wrote:
> On date Thursday 2009-10-15 16:11:20 +0200, Michael Niedermayer encoded:
> > On Wed, Oct 14, 2009 at 08:49:16PM +0200, Stefano Sabatini wrote:
> > > On date Wednesday 2009-10-14 01:37:46 +0200, Michael Niedermayer encoded:
> > > > On Tue, Oct 13, 2009 at 08:47:09PM +0200, Stefano Sabatini wrote:
> > > > > On date Tuesday 2009-10-13 13:16:29 +0200, Michael Niedermayer encoded:
> > > > > > 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:
[...]
> > > +        (unsigned)crop->x + (unsigned)crop->w > link->w ||
> > > +        (unsigned)crop->y + (unsigned)crop->h > link->h) {
> > > +        av_log(ctx, AV_LOG_ERROR,
> > > +               "Output area %d:%d:%d:%d not within the input area 0:0:%d:%d\n",
> > > +               crop->x, crop->y, crop->w, crop->h, link->w, link->h);
> > > +        return -1;
> > > +    }
> > [...]
> > > +
> > > +    crop->x &= ~((1 << crop->hsub) - 1);
> > > +    crop->y &= ~((1 << crop->vsub) - 1);
> > > +    crop->w &= ~((1 << crop->hsub) - 1);
> > > +    crop->h &= ~((1 << crop->vsub) - 1);
> > > +
> > > +    av_log(link->dst, AV_LOG_INFO, "x:%d y:%d w:%d h:%d\n", crop->x, crop->y, crop->w, crop->h);
> > > +
> > > +    if (crop->w == 0 || crop->h == 0) {
> > > +        av_log(link->dst, AV_LOG_ERROR, "Size values too small\n");
> > > +        return -1;
> > > +    }
> > 
> > please move the check at the end, dont check and then change and then check
> > again. or dont round at all and rather complain that the value is not ok
> 
> Implemented as normalize+round+check, I slightly prefer to round than
> to pretend the user to do that (which implies a deep knowledge of the
> various image formats).

fine but you know that crop silently does not do what the user asks for then


[...]
> +static int config_input(AVFilterLink *link)
> +{
> +    AVFilterContext *ctx = link->dst;
> +    CropContext *crop = ctx->priv;
> +    int round_hsub, round_vsub;
> +
> +    switch (link->format) {
> +    case PIX_FMT_RGB32:
> +    case PIX_FMT_BGR32:
> +        crop->bpp = 32;
> +        break;
> +    case PIX_FMT_RGB24:
> +    case PIX_FMT_BGR24:
> +        crop->bpp = 24;
> +        break;
> +    case PIX_FMT_RGB565:
> +    case PIX_FMT_RGB555:
> +    case PIX_FMT_BGR565:
> +    case PIX_FMT_BGR555:
> +    case PIX_FMT_GRAY16BE:
> +    case PIX_FMT_GRAY16LE:
> +        crop->bpp = 16;
> +        break;
> +    case PIX_FMT_MONOWHITE:
> +    case PIX_FMT_MONOBLACK:
> +        crop->bpp  = 1;

inconsistemt spaces before =


> +        break;
> +    default:
> +        crop->bpp = 8;
> +    }
> +
> +    avcodec_get_chroma_sub_sample(link->format, &crop->hsub, &crop->vsub);
> +
> +    if (crop->w == 0)
> +        crop->w = link->w - crop->x;
> +    if (crop->h == 0)
> +        crop->h = link->h - crop->y;
> +

> +    if (link->format == PIX_FMT_MONOWHITE || link->format == PIX_FMT_MONOBLACK) {
> +        round_hsub = 3;
> +        round_vsub = 0;
> +    } else {

I think you should drop mono support, its not in SOC and i would prefer if
this does not delay this 99.9% acceptable patch for months
(its broken anyway as rounding is not correct, crop of mono should work
 with any pixel value even not *8 but i know we will fight about how to
 implement this so this would add a big delay ...)


> +        round_hsub = crop->hsub;
> +        round_vsub = crop->vsub;
> +    }
> +
> +    crop->x &= ~((1 << round_hsub) - 1);
> +    crop->y &= ~((1 << round_vsub) - 1);

> +    crop->w &= ~((1 << round_hsub) - 1);
> +    crop->h &= ~((1 << round_vsub) - 1);

i dont think these are needed


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct awnser.
-------------- 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/20091016/6a0d6d06/attachment.pgp>