[FFmpeg-devel] [PATCH] libavfilter/vf_convolution_opencl.c: add opencl version of libavfilter/convolution.c filter

Mark Thompson sw at jkqxz.net
Fri Mar 9 02:42:45 EET 2018


On 07/03/18 22:32, Danil Iashchenko wrote:
> ---
>  configure                           |   1 +
>  libavfilter/Makefile                |   2 +
>  libavfilter/allfilters.c            |   1 +
>  libavfilter/opencl/convolution.cl   |  80 ++++++++
>  libavfilter/opencl_source.h         |   2 +
>  libavfilter/vf_convolution_opencl.c | 374 ++++++++++++++++++++++++++++++++++++
>  6 files changed, 460 insertions(+)
>  create mode 100644 libavfilter/opencl/convolution.cl
>  create mode 100644 libavfilter/vf_convolution_opencl.c

Nice!  Works well for me on Beignet, I'll try to have a go with some other implementations in the next few days.

Review comments follow.

Thanks,

- Mark


> diff --git a/configure b/configure
> index 6916b45..7c79e20 100755
> --- a/configure
> +++ b/configure
> @@ -3212,6 +3212,7 @@ bs2b_filter_deps="libbs2b"
>  colormatrix_filter_deps="gpl"
>  convolve_filter_deps="avcodec"
>  convolve_filter_select="fft"
> +convolution_opencl_filter_deps="opencl"
>  coreimage_filter_deps="coreimage appkit"
>  coreimage_filter_extralibs="-framework OpenGL"
>  coreimagesrc_filter_deps="coreimage appkit"
> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> index 6a60836..f288f8e 100644
> --- a/libavfilter/Makefile
> +++ b/libavfilter/Makefile
> @@ -156,6 +156,8 @@ OBJS-$(CONFIG_COLORLEVELS_FILTER)            += vf_colorlevels.o
>  OBJS-$(CONFIG_COLORMATRIX_FILTER)            += vf_colormatrix.o
>  OBJS-$(CONFIG_COLORSPACE_FILTER)             += vf_colorspace.o colorspacedsp.o
>  OBJS-$(CONFIG_CONVOLUTION_FILTER)            += vf_convolution.o
> +OBJS-$(CONFIG_CONVOLUTION_OPENCL_FILTER)         += vf_convolution_opencl.o opencl.o \

Funny spacing?

> +                                                opencl/convolution.o
>  OBJS-$(CONFIG_CONVOLVE_FILTER)               += vf_convolve.o framesync.o
>  OBJS-$(CONFIG_COPY_FILTER)                   += vf_copy.o
>  OBJS-$(CONFIG_COREIMAGE_FILTER)              += vf_coreimage.o
> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
> index 9adb109..f2dc55e 100644
> --- a/libavfilter/allfilters.c
> +++ b/libavfilter/allfilters.c
> @@ -166,6 +166,7 @@ static void register_all(void)
>      REGISTER_FILTER(COLORMATRIX,    colormatrix,    vf);
>      REGISTER_FILTER(COLORSPACE,     colorspace,     vf);
>      REGISTER_FILTER(CONVOLUTION,    convolution,    vf);
> +    REGISTER_FILTER(CONVOLUTION_OPENCL, convolution_opencl, vf);
>      REGISTER_FILTER(CONVOLVE,       convolve,       vf);
>      REGISTER_FILTER(COPY,           copy,           vf);
>      REGISTER_FILTER(COREIMAGE,      coreimage,      vf);
> diff --git a/libavfilter/opencl/convolution.cl b/libavfilter/opencl/convolution.cl
> new file mode 100644
> index 0000000..5bc5839
> --- /dev/null
> +++ b/libavfilter/opencl/convolution.cl
> @@ -0,0 +1,80 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +__kernel void convolution_global(__write_only image2d_t dst,
> +                                 __read_only  image2d_t src,
> +                                 int coef_matrix_size,
> +                                 __constant float *coef_matrix,
> +                                 float div,
> +                                 float bias)
> +{
> +    const sampler_t sampler = (CLK_NORMALIZED_COORDS_FALSE | CLK_FILTER_NEAREST);
> +
> +    const int half_matrix_size = (coef_matrix_size / 2);
> +    int2 loc = (int2)(get_global_id(0), get_global_id(1));
> +    float4 convPix = (float4)(0.0f, 0.0f, 0.0f, 0.0f);
> +
> +    for (int conv_i = -half_matrix_size; conv_i <= half_matrix_size; conv_i++) {
> +        for (int conv_j = -half_matrix_size; conv_j <= half_matrix_size; conv_j++) {
> +            float4 px = read_imagef(src, sampler, loc + (int2)(conv_j, conv_i));
> +            convPix += px * coef_matrix[(conv_i+1)*coef_matrix_size+(conv_j+1)];
> +        }
> +     }
> +     write_imagef(dst, loc, convPix * div + bias);
> +}

Looks good.

> +__kernel void convolution_local(__write_only image2d_t dst,
> +                                 __read_only  image2d_t src,
> +                                 int coef_matrix_size,
> +                                 __constant float *coef_matrix,
> +                                 float div,
> +                                 float bias)
> +{
> +    const sampler_t sampler = (CLK_NORMALIZED_COORDS_FALSE |
> +                               CLK_ADDRESS_CLAMP_TO_EDGE |
> +                               CLK_FILTER_NEAREST);
> +
> +    const int block_size = 16;
> +
> +    const int block_x = get_group_id(0) * block_size;
> +    const int block_y = get_group_id(1) * block_size;
> +    const int local_x  =  get_local_id(0);
> +    const int local_y  =  get_local_id(1);
> +    const int half_matrix_size = (coef_matrix_size / 2);
> +
> +    __local float4 B[16*3*16*3];
> +
> +    for (int i = -half_matrix_size; i <= half_matrix_size; i++) {
> +            for (int j = -half_matrix_size; j <= half_matrix_size; j++) {
> +                    B[((i+1)*block_size+local_x) * (block_size*coef_matrix_size) + (j+1)*block_size+local_y] =
> +                        read_imagef(src, sampler, (int2)(block_x+local_y + (j*block_size), block_y+local_x + (i*block_size)));

This doesn't look right at all.  That's going to write to invalid negative addresses, at least.

> +            }
> +    }
> +    barrier(CLK_LOCAL_MEM_FENCE);
> +
> +    float4 convPix = (float4)(0.0f, 0.0f, 0.0f, 0.0f);
> +    for (int conv_i = -half_matrix_size; conv_i <= half_matrix_size; conv_i++) {
> +                    for (int conv_j = -half_matrix_size; conv_j <= half_matrix_size; conv_j++) {
> +                            float4 px = B[(local_x+conv_i+block_size) * (block_size*coef_matrix_size) + local_y+conv_j+block_size];
> +                            convPix += px * coef_matrix[(conv_i+1)*coef_matrix_size+(conv_j+1)];
> +                    }
> +            }
> +    barrier(CLK_LOCAL_MEM_FENCE);
> +    write_imagef(dst, (int2)(block_x+local_y, block_y+local_x), convPix * div + bias);
> +
> +}

This is never used, anyway.  Are you intending to make it work now?  If not, don't include it in this patch (it's fine to re-add it in another patch later).

> diff --git a/libavfilter/opencl_source.h b/libavfilter/opencl_source.h
> index 23cdfc6..ac2f89e 100644
> --- a/libavfilter/opencl_source.h
> +++ b/libavfilter/opencl_source.h
> @@ -22,4 +22,6 @@
>  extern const char *ff_opencl_source_overlay;
>  extern const char *ff_opencl_source_unsharp;
>  
> +extern const char *ff_opencl_source_convolution;

This list should probably be in alphabetical order like other lists, so I think put it at the start.

> +
>  #endif /* AVFILTER_OPENCL_SOURCE_H */
> diff --git a/libavfilter/vf_convolution_opencl.c b/libavfilter/vf_convolution_opencl.c
> new file mode 100644
> index 0000000..6902062
> --- /dev/null
> +++ b/libavfilter/vf_convolution_opencl.c
> @@ -0,0 +1,374 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "libavutil/common.h"
> +#include "libavutil/imgutils.h"
> +#include "libavutil/mem.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/pixdesc.h"
> +#include "libavutil/avstring.h"
> +
> +
> +#include "avfilter.h"
> +#include "internal.h"
> +#include "opencl.h"
> +#include "opencl_source.h"
> +#include "video.h"
> +
> +typedef struct ConvolutionOpenCLContext {
> +    OpenCLFilterContext ocf;
> +
> +    int              initialised;
> +    cl_kernel        kernel;
> +    cl_command_queue command_queue;
> +
> +    char *matrix_str;
> +    cl_int size;
> +
> +    cl_int matrix_length;
> +    cl_float rdiv;
> +    cl_float bias;
> +    cl_mem matrix;
> +
> +    int global;
> +
> +} ConvolutionOpenCLContext;
> +
> +
> +const float default3x3[9] = {0, 0, 0,
> +                             0, 1, 0,
> +                             0, 0, 0};
> +
> +const float default5x5[25] = {0, 0, 0, 0, 0,
> +                              0, 0, 0, 0, 0,
> +                              0, 0, 1, 0, 0,
> +                              0, 0, 0, 0, 0,
> +                              0, 0, 0, 0, 0};
> +
> +const float default7x7[49] = {0, 0, 0, 0, 0, 0, 0,
> +                              0, 0, 0, 0, 0, 0, 0,
> +                              0, 0, 0, 0, 0, 0, 0,
> +                              0, 0, 0, 1, 0, 0, 0,
> +                              0, 0, 0, 0, 0, 0, 0,
> +                              0, 0, 0, 0, 0, 0, 0,
> +                              0, 0, 0, 0, 0, 0, 0};

What do you make these for?  They values don't appear to actually get used - they do get copied into the matrix, but are then always fully overwritten.

(Also, file-local variables like this should be static.)

> +static int convolution_opencl_init(AVFilterContext *avctx)
> +{
> +    ConvolutionOpenCLContext *ctx = avctx->priv;
> +    cl_int cle;
> +    int err;
> +
> +    err = ff_opencl_filter_load_program(avctx, &ff_opencl_source_convolution, 1);
> +    if (err < 0)
> +        goto fail;
> +
> +    ctx->command_queue = clCreateCommandQueue(ctx->ocf.hwctx->context,
> +                                              ctx->ocf.hwctx->device_id,
> +                                              0, &cle);
> +    if (!ctx->command_queue) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to create OpenCL "
> +               "command queue: %d.\n", cle);
> +        err = AVERROR(EIO);
> +        goto fail;
> +    }
> +
> +    ctx->global = 1;

If you aren't intending to include the second kernel in this patch then just drop the global variable entirely.

> +
> +    ctx->kernel = clCreateKernel(ctx->ocf.program,
> +                                 ctx->global ? "convolution_global"
> +                                             : "convolution_local", &cle);
> +    if (!ctx->kernel) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to create kernel: %d.\n", cle);
> +        err = AVERROR(EIO);
> +        goto fail;
> +    }
> +
> +    ctx->initialised = 1;
> +    return 0;
> +
> +fail:
> +    if (ctx->command_queue)
> +        clReleaseCommandQueue(ctx->command_queue);
> +    if (ctx->kernel)
> +        clReleaseKernel(ctx->kernel);
> +    return err;
> +}
> +
> +
> +
> +static int convolution_opencl_make_filter_params(AVFilterContext *avctx)
> +{
> +    ConvolutionOpenCLContext *ctx = avctx->priv;
> +    float *matrix;
> +    size_t matrix_bytes;
> +    cl_mem buffer;
> +    cl_int cle;
> +    int err = 0;

This variable is not used except for the return value, might as well get right of it.

> +    char *p, *arg, *saveptr = NULL;
> +
> +    float input_matrix[49];
> +    p = ctx->matrix_str;
> +    while (ctx->matrix_length < 49) {
> +        if (!(arg = av_strtok(p, " ", &saveptr)))
> +            break;
> +        p = NULL;
> +        sscanf(arg, "%f", &input_matrix[ctx->matrix_length]);

Probably make sure that actually found a number.  It currently allows, for example, "a b c d e f g h i" as the matrix.

> +        ctx->matrix_length++;
> +    }
> +
> +    matrix_bytes = sizeof(float)*ctx->matrix_length;
> +    matrix = av_malloc(matrix_bytes);

Allocations need to be checked (return AVERROR(ENOMEM) if they ever fail).

> +
> +    if (ctx->matrix_length == 9) {
> +        ctx->size = 3;
> +        memcpy(matrix, default3x3, ctx->matrix_length);
> +    } else if (ctx->matrix_length == 25) {
> +            ctx->size = 5;
> +            memcpy(matrix, default5x5, ctx->matrix_length);
> +    } else if (ctx->matrix_length == 49) {
> +            ctx->size = 7;
> +            memcpy(matrix, default7x7, ctx->matrix_length);
> +    } else {
> +        return AVERROR(EINVAL);

There should be an error message describing why the option is invalid here.

> +    }
> +
> +    for (int i = 0; i < ctx->matrix_length; i++)

FFmpeg is still built with compilers which don't allow declaration-anywhere.  Put the "int i" at the start of the function.

> +        matrix[i] = input_matrix[i];
> +
> +
> +
> +    buffer = clCreateBuffer(ctx->ocf.hwctx->context,
> +                            CL_MEM_READ_ONLY |
> +                            CL_MEM_COPY_HOST_PTR |
> +                            CL_MEM_HOST_NO_ACCESS,
> +                            matrix_bytes, matrix, &cle);
> +    if (!buffer) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to create matrix buffer: "
> +               "%d.\n", cle);
> +        err = AVERROR(EIO);
> +        goto fail;
> +    }
> +    ctx->matrix = buffer;

Leaks the local matrix on success.

> +
> +    return err;
> +fail:
> +    av_freep(&matrix);
> +    return err;
> +
> +
> +}
> +
> +static int convolution_opencl_filter_frame(AVFilterLink *inlink, AVFrame *input)
> +{
> +    AVFilterContext    *avctx = inlink->dst;
> +    AVFilterLink     *outlink = avctx->outputs[0];
> +    ConvolutionOpenCLContext *ctx = avctx->priv;
> +    AVFrame *output = NULL;
> +    cl_int cle;
> +    size_t global_work[2];
> +    size_t local_work[2];
> +    cl_mem src, dst;
> +    int err, p;
> +
> +    av_log(ctx, AV_LOG_DEBUG, "Filter input: %s, %ux%u (%"PRId64").\n",
> +           av_get_pix_fmt_name(input->format),
> +           input->width, input->height, input->pts);
> +
> +    if (!input->hw_frames_ctx)
> +        return AVERROR(EINVAL);
> +
> +    if (!ctx->initialised) {
> +        err = convolution_opencl_init(avctx);
> +        if (err < 0)
> +            goto fail;
> +
> +        err = convolution_opencl_make_filter_params(avctx);
> +        if (err < 0)
> +            goto fail;
> +    }
> +
> +    output = ff_get_video_buffer(outlink, outlink->w, outlink->h);
> +    if (!output) {
> +        err = AVERROR(ENOMEM);
> +        goto fail;
> +    }
> +
> +    for (p = 0; p < FF_ARRAY_ELEMS(output->data); p++) {
> +        src = (cl_mem) input->data[p];
> +        dst = (cl_mem)output->data[p];
> +
> +
> +        if (!dst)
> +            break;
> +
> +        cle = clSetKernelArg(ctx->kernel, 0, sizeof(cl_mem), &dst);
> +        if (cle != CL_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to set kernel "
> +                   "destination image argument: %d.\n", cle);
> +            goto fail;
> +        }
> +        cle = clSetKernelArg(ctx->kernel, 1, sizeof(cl_mem), &src);
> +        if (cle != CL_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to set kernel "
> +                   "source image argument: %d.\n", cle);
> +            goto fail;
> +        }
> +        cle = clSetKernelArg(ctx->kernel, 2, sizeof(cl_int), &ctx->size);
> +        if (cle != CL_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to set kernel "
> +                   "matrix size argument: %d.\n", cle);
> +            goto fail;
> +        }
> +        cle = clSetKernelArg(ctx->kernel, 3, sizeof(cl_mem), &ctx->matrix);
> +        if (cle != CL_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to set kernel "
> +                   "matrix argument: %d.\n", cle);
> +            goto fail;
> +        }
> +        cle = clSetKernelArg(ctx->kernel, 4, sizeof(cl_float), &ctx->rdiv);
> +        if (cle != CL_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to set kernel "
> +                   "div argument: %d.\n", cle);
> +            goto fail;
> +        }
> +        cle = clSetKernelArg(ctx->kernel, 5, sizeof(cl_float), &ctx->bias);
> +        if (cle != CL_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to set kernel "
> +                   "bias argument: %d.\n", cle);
> +            goto fail;
> +        }
> +
> +
> +        if (ctx->global) {
> +            global_work[0] = output->width;
> +            global_work[1] = output->height;
> +        } else {
> +            global_work[0] = FFALIGN(output->width,  16);
> +            global_work[1] = FFALIGN(output->height, 16);
> +            local_work[0]  = 16;
> +            local_work[1]  = 16;
> +        }
> +
> +        av_log(avctx, AV_LOG_DEBUG, "Run kernel on plane %d "
> +               "(%"SIZE_SPECIFIER"x%"SIZE_SPECIFIER").\n",
> +               p, global_work[0], global_work[1]);
> +
> +        cle = clEnqueueNDRangeKernel(ctx->command_queue, ctx->kernel, 2, NULL,
> +                                     global_work, ctx->global ? NULL : local_work,
> +                                     0, NULL, NULL);
> +        if (cle != CL_SUCCESS) {
> +            av_log(avctx, AV_LOG_ERROR, "Failed to enqueue kernel: %d.\n",
> +                   cle);
> +            err = AVERROR(EIO);
> +            goto fail;
> +        }
> +    }
> +
> +    cle = clFinish(ctx->command_queue);
> +    if (cle != CL_SUCCESS) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to finish command queue: %d.\n",
> +               cle);
> +        err = AVERROR(EIO);
> +        goto fail;
> +    }
> +
> +    err = av_frame_copy_props(output, input);
> +    if (err < 0)
> +        goto fail;
> +
> +    av_frame_free(&input);
> +
> +    av_log(ctx, AV_LOG_DEBUG, "Filter output: %s, %ux%u (%"PRId64").\n",
> +           av_get_pix_fmt_name(output->format),
> +           output->width, output->height, output->pts);
> +
> +    return ff_filter_frame(outlink, output);
> +
> +fail:
> +    clFinish(ctx->command_queue);
> +    av_frame_free(&input);
> +    av_frame_free(&output);
> +    return err;
> +}
> +
> +static av_cold void convolution_opencl_uninit(AVFilterContext *avctx)
> +{
> +    ConvolutionOpenCLContext *ctx = avctx->priv;
> +    cl_int cle;

You should free ctx->matrix if it's set here, it's currently leaking.

> +
> +    if (ctx->kernel) {
> +        cle = clReleaseKernel(ctx->kernel);
> +        if (cle != CL_SUCCESS)
> +            av_log(avctx, AV_LOG_ERROR, "Failed to release "
> +                   "kernel: %d.\n", cle);
> +    }
> +
> +    if (ctx->command_queue) {
> +        cle = clReleaseCommandQueue(ctx->command_queue);
> +        if (cle != CL_SUCCESS)
> +            av_log(avctx, AV_LOG_ERROR, "Failed to release "
> +                   "command queue: %d.\n", cle);
> +    }
> +
> +    ff_opencl_filter_uninit(avctx);
> +}
> +
> +#define OFFSET(x) offsetof(ConvolutionOpenCLContext, x)
> +#define FLAGS (AV_OPT_FLAG_FILTERING_PARAM | AV_OPT_FLAG_VIDEO_PARAM)
> +static const AVOption convolution_opencl_options[] = {
> +    { "m",    "set matrix ",  OFFSET(matrix_str), AV_OPT_TYPE_STRING, {.str="0 0 0 0 1 0 0 0 0"}, 0, 0, FLAGS },
> +    { "rdiv", "set rdiv",     OFFSET(rdiv),      AV_OPT_TYPE_FLOAT,  {.dbl=1.0}, 0.0, INT_MAX, FLAGS},
> +    { "bias", "set bias",     OFFSET(bias),     AV_OPT_TYPE_FLOAT,  {.dbl=0.0}, 0.0, INT_MAX, FLAGS},

This applies the same matrix to all planes.  Any thoughts of being able to matrices for each plane like the existing vf_convolution filter does, and use the same option names so that switching between them is easy?  (Feel free to ignore this if you want.)

> +    { NULL }
> +};
> +
> +AVFILTER_DEFINE_CLASS(convolution_opencl);
> +
> +static const AVFilterPad convolution_opencl_inputs[] = {
> +    {
> +        .name         = "default",
> +        .type         = AVMEDIA_TYPE_VIDEO,
> +        .filter_frame = &convolution_opencl_filter_frame,
> +        .config_props = &ff_opencl_filter_config_input,
> +    },
> +    { NULL }
> +};
> +
> +static const AVFilterPad convolution_opencl_outputs[] = {
> +    {
> +        .name         = "default",
> +        .type         = AVMEDIA_TYPE_VIDEO,
> +        .config_props = &ff_opencl_filter_config_output,
> +    },
> +    { NULL }
> +};
> +
> +AVFilter ff_vf_convolution_opencl = {
> +    .name           = "convolution_opencl",
> +    .description    = NULL_IF_CONFIG_SMALL("Apply convolution mask to input video"),
> +    .priv_size      = sizeof(ConvolutionOpenCLContext),
> +    .priv_class     = &convolution_opencl_class,
> +    .init           = &ff_opencl_filter_init,
> +    .uninit         = &convolution_opencl_uninit,
> +    .query_formats  = &ff_opencl_filter_query_formats,
> +    .inputs         = convolution_opencl_inputs,
> +    .outputs        = convolution_opencl_outputs,
> +    .flags_internal = FF_FILTER_FLAG_HWFRAME_AWARE,
> +};
> 


More information about the ffmpeg-devel mailing list