[FFmpeg-devel] [PATCH 1/2] avfilter/vf_cropdetect: Factorize duplicated code using a macro

[Benoit Fouet]

Hi,

Le 27/12/2014 04:35, Michael Niedermayer a écrit :
> This simplifies subsequent changes
>
> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> ---
>   libavfilter/vf_cropdetect.c |   34 ++++++++++------------------------
>   1 file changed, 10 insertions(+), 24 deletions(-)
>
> diff --git a/libavfilter/vf_cropdetect.c b/libavfilter/vf_cropdetect.c
> index 76aa7b2..fd2286d 100644
> --- a/libavfilter/vf_cropdetect.c
> +++ b/libavfilter/vf_cropdetect.c
> @@ -136,33 +136,19 @@ static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
>               s->frame_nb = 1;
>           }
>   
> -        for (y = 0; y < s->y1; y++) {
> -            if (checkline(ctx, frame->data[0] + frame->linesize[0] * y, bpp, frame->width, bpp) > s->limit) {
> -                s->y1 = y;
> -                break;
> -            }
> +#define CHECK(DST, FROM, NOEND, INC, STEP0, STEP1, LEN) \
> +        for (y = FROM; NOEND; INC) {\
> +            if (checkline(ctx, frame->data[0] + STEP0 * y, STEP1, LEN, bpp) > s->limit) {\
> +                DST = y;\
> +                break;\
> +            }\
>           }
>   

The name "CHECK" sounds a bit weird given it may also affect "DST".
I cannot come up with a good name though... Maybe something like "FIND" 
or "CHECK_AND_SET" or something like that?

> -        for (y = frame->height - 1; y > FFMAX(s->y2, s->y1); y--) {
> -            if (checkline(ctx, frame->data[0] + frame->linesize[0] * y, bpp, frame->width, bpp) > s->limit) {
> -                s->y2 = y;
> -                break;
> -            }
> -        }
> -
> -        for (y = 0; y < s->x1; y++) {
> -            if (checkline(ctx, frame->data[0] + bpp*y, frame->linesize[0], frame->height, bpp) > s->limit) {
> -                s->x1 = y;
> -                break;
> -            }
> -        }
> +        CHECK(s->y1,                 0,               y < s->y1, y++, frame->linesize[0], bpp, frame->width);
> +        CHECK(s->y2, frame->height - 1, y > FFMAX(s->y2, s->y1), y--, frame->linesize[0], bpp, frame->width);
> +        CHECK(s->x1,                 0,               y < s->x1, y++, bpp, frame->linesize[0], frame->height);
> +        CHECK(s->x2,  frame->width - 1, y > FFMAX(s->x2, s->x1), y--, bpp, frame->linesize[0], frame->height);
>   
> -        for (y = frame->width - 1; y > FFMAX(s->x2, s->x1); y--) {
> -            if (checkline(ctx, frame->data[0] + bpp*y, frame->linesize[0], frame->height, bpp) > s->limit) {
> -                s->x2 = y;
> -                break;
> -            }
> -        }
>   
>           // round x and y (up), important for yuv colorspaces
>           // make sure they stay rounded!

The remainder of the patch LGTM.

-- 
Ben