[FFmpeg-devel] [PATCH] lavfi: edgedetect filter

[Clément Bœsch]

On Tue, Aug 07, 2012 at 08:02:47PM +0200, Nicolas George wrote:
> Le primidi 21 thermidor, an CCXX, Clément Bœsch a écrit :
> > FIXME: bump lavfi minor
> > ---
> > Canny Edge Detection, with no real tweak. Here are the results:
> > http://imgur.com/a/ujsG5
> 
> I do not know the algorithm, I can not comment on that part. Looks good,
> though.
> 

The algorithm is the kinda the more common. I added a @url
https://en.wikipedia.org/wiki/Canny_edge_detector on top of the file for
reference.

[...]
> > +static int config_props(AVFilterLink *inlink)
> > +{
> > +    AVFilterContext *ctx = inlink->dst;
> > +    EdgeDetectContext *edgedetect = ctx->priv;
> > +
> > +    edgedetect->tmpbuf     = av_malloc (inlink->w * inlink->h);
> 
> > +    edgedetect->gradients  = av_mallocz(inlink->w * inlink->h * sizeof(*edgedetect->gradients));
> 
> av_calloc(), to guard against overflows?
> 

Changed locally.

> > +    edgedetect->directions = av_malloc (inlink->w * inlink->h);
> > +    if (!edgedetect->tmpbuf || !edgedetect->gradients || !edgedetect->directions)
> > +        return AVERROR(ENOMEM);
> > +    return 0;
> > +}
> > +
> > +static void gaussian_blur(AVFilterContext *ctx, int w, int h,
> > +                                uint8_t *dst, int dst_linesize,
> > +                          const uint8_t *src, int src_linesize)
> > +{
> > +    int i, j;
> > +
> > +    memcpy(dst,                src,                w);
> > +    memcpy(dst + dst_linesize, src + src_linesize, w);
> > +    for (j = 2; j < h - 2; j++) {
> > +        dst[j*dst_linesize  ] = src[j*src_linesize  ];
> > +        dst[j*dst_linesize+1] = src[j*src_linesize+1];
> > +        for (i = 2; i < w - 2; i++) {
> > +            dst[j*dst_linesize + i] =
> > +                 ((src[(j-2)*src_linesize + (i-2)] + src[(j+2)*src_linesize + (i-2)]) * 2
> > +                + (src[(j-2)*src_linesize + (i-1)] + src[(j+2)*src_linesize + (i-1)]) * 4
> > +                + (src[(j-2)*src_linesize +  i   ] + src[(j+2)*src_linesize +  i   ]) * 5
> > +                + (src[(j-2)*src_linesize + (i+1)] + src[(j+2)*src_linesize + (i+1)]) * 4
> > +                + (src[(j-2)*src_linesize + (i+2)] + src[(j+2)*src_linesize + (i+2)]) * 2
> > +
> > +                + (src[(j-1)*src_linesize + (i-2)] + src[(j-1)*src_linesize + (i-2)]) *  4
> > +                + (src[(j-1)*src_linesize + (i-1)] + src[(j-1)*src_linesize + (i-1)]) *  9
> > +                + (src[(j-1)*src_linesize +  i   ] + src[(j-1)*src_linesize +  i   ]) * 12
> > +                + (src[(j-1)*src_linesize + (i+1)] + src[(j-1)*src_linesize + (i+1)]) *  9
> > +                + (src[(j-1)*src_linesize + (i+2)] + src[(j-1)*src_linesize + (i+2)]) *  4
> > +
> > +                + src[j*src_linesize + (i-2)] *  5
> > +                + src[j*src_linesize + (i-1)] * 12
> > +                + src[j*src_linesize +  i   ] * 15
> > +                + src[j*src_linesize + (i+1)] * 12
> > +                + src[j*src_linesize + (i+2)] *  5) / 159;
> > +        }
> > +        dst[j*dst_linesize + i  ] = src[j*src_linesize + i  ];
> > +        dst[j*dst_linesize + i+1] = src[j*src_linesize + i+1];
> > +    }
> > +    memcpy(dst +  j   *dst_linesize, src +  j   *src_linesize, w);
> > +    memcpy(dst + (j+1)*dst_linesize, src + (j+1)*src_linesize, w);
> 
> Is gcc smart enough to avoid all those multiplications by "linesize"? Just
> adding "src += linesize; dst += linesize;" would probably be more readable
> anyway.
> 

I'll look at this deeper. I'm not sure doing increment all the time will
really improve things...

> > +}
> > +
> > +enum {
> > +    DIRECTION_45UP,
> > +    DIRECTION_45DOWN,
> > +    DIRECTION_HORIZONTAL,
> > +    DIRECTION_VERTICAL,
> > +};
> > +
> > +static int get_rounded_direction(int gx, int gy)
> > +{
> > +    float tanpi8gx, tan3pi8gx;
> > +
> > +    /* reference angles:
> > +     *   tan( pi/8) = sqrt(2)-1 ~= 0.41421...
> > +     *   tan(3pi/8) = sqrt(2)+1 ~= 2.41421...
> > +     * Gy/Gx is the tangent of theta, so Gy/Gx is compared against <ref-angle>,
> > +     * or more simply Gy against <ref-angle>*Gx
> > +     */
> > +    if (gx) {
> > +        if (gx < 0) // left side, switch signs
> > +            gx = -gx, gy = -gy;
> > +        tanpi8gx  = (M_SQRT2-1) * gx;
> > +        tan3pi8gx = (M_SQRT2+1) * gx;
> > +        if (gy > -tan3pi8gx && gy < -tanpi8gx)  return DIRECTION_45UP;
> > +        if (gy > -tanpi8gx  && gy <  tanpi8gx)  return DIRECTION_HORIZONTAL;
> > +        if (gy >  tanpi8gx  && gy <  tan3pi8gx) return DIRECTION_45DOWN;
> > +    }
> > +    return DIRECTION_VERTICAL;
> > +}
> 
> A pure integer version would be nicer and more FATE-friendly. Since gx and
> gy are bounded by ±256, the extra precision is not necessary:
> 

mmh not really 256, more like [-1020;1020] afaict.

> if (gy << 16 > 158217) return ...
> 
> (16 instead of 8 if someone wants to implement higher bit depths)
> 

I'll give it a try as well.

[...]

Thanks for the review!

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120807/01c34749/attachment.asc>