[FFmpeg-devel] [PATCH]opencl: compile kernels separately

Fri Nov 1 20:20:02 CET 2013

On Thu, Oct 31, 2013 at 12:32:05AM -0500, Lenny Wang wrote:
> On Wed, Oct 30, 2013 at 11:41 AM, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > On Wed, Oct 30, 2013 at 12:12:20PM -0500, Lenny Wang wrote:
> >> Thanks for your comments.
> >>
> >> I can keep the ABI intact without modifying
> >> av_opencl_register_kernel_code() but still make this patch work as
> >> intended.
> >>
> >> I can also keep the old API but since their functionality will break,
> >> I can do the "return AVERROR(ENOSYS) hack with a message" based on
> >> Michael's suggestion.
> >
> > Also, maybe you could go through your new API and check that you can
> > extend it without breaking old applications in as many places as
> > possible.
> > Things like making sure that users will use an allocation function
> > for structures for example, that means that you an always add additional
> > members at the end.
> > At least whenever it isn't much effort to do.
> > We should really avoid keeping APIs in this "experimental" state,
> > it's ok for a short while, but after a certain time it starts saying
> > "we can't be bothered to make sure our stuff actually works properly".
> 
> Attached new patch modified based on previous suggestions.
> * To keep libavfilter ABI intact, av_opencl_register_kernel_code() unchanged
> * Preserved old opencl API marked as deprecated and emit error
> messages when called
> * the new av_opencl_compile() function allows opencl components like
> filters to manage opencl kernels on their own, ocl-util now only
> provide compilation service without managing the entire kernel pool
> and related data structures
> 
> It's up to you how much longer to keep the "experimental" state or not
> at all.  I think it's pretty solid after the patch.  Thanks.

[...]
> diff --git a/libavutil/opencl.h b/libavutil/opencl.h
> index 094c108..4d720ff 100644
> --- a/libavutil/opencl.h
> +++ b/libavutil/opencl.h
> @@ -1,7 +1,8 @@
>  /*
> - * Copyright (C) 2012 Peng Gao <peng at multicorewareinc.com>
> - * Copyright (C) 2012 Li   Cao <li at multicorewareinc.com>
> - * Copyright (C) 2012 Wei  Gao <weigao at multicorewareinc.com>
> + * Copyright (C) 2012 Peng  Gao     <peng at multicorewareinc.com>
> + * Copyright (C) 2012 Li    Cao     <li at multicorewareinc.com>
> + * Copyright (C) 2012 Wei   Gao     <weigao at multicorewareinc.com>
> + * Copyright (C) 2013 Lenny Wang    <lwanghpc at gmail.com>
>   *
>   * This file is part of FFmpeg.
>   *

> @@ -41,11 +42,9 @@
>  
>  #define AV_OPENCL_KERNEL( ... )# __VA_ARGS__
>  
> -#define AV_OPENCL_MAX_KERNEL_NAME_SIZE 150
> +#define AV_OPENCL_MAX_DEVICE_NAME_SIZE 64
>  
> -#define AV_OPENCL_MAX_DEVICE_NAME_SIZE 100
> -
> -#define AV_OPENCL_MAX_PLATFORM_NAME_SIZE 100
> +#define AV_OPENCL_MAX_PLATFORM_NAME_SIZE 64

why do you decrease these, i suspect that break ABI

>  
>  typedef struct {
>      int device_type;
> @@ -67,8 +66,7 @@ typedef struct {
>  
>  typedef struct {
>      cl_command_queue command_queue;
> -    cl_kernel kernel;
> -    char kernel_name[AV_OPENCL_MAX_KERNEL_NAME_SIZE];

if you mark the fields as deprecated and place them under
#if FF_API_OLD_OPENCL
with matching code in version.h then this would not break ABI

> +    cl_program program;
>  } AVOpenCLKernelEnv;

neither the old nor the new API allows AVOpenCLKernelEnv to be
extended
I think that alone makes the new API unacceptable as the need to
extend the struct is clearly demonstrated by this.

to make it extendible no code outside may use sizeof(AVOpenCLKernelEnv)
a declaration like
AVOpenCLKernelEnv kernel_env;
effectively uses sizeof(AVOpenCLKernelEnv)

>
>  typedef struct {
> @@ -107,7 +105,6 @@ void av_opencl_free_device_list(AVOpenCLDeviceList **device_list);
>   * av_opencl_init() operation.
>   *
>   * The currently accepted options are:
> - * - build_options: set options to compile registered kernels code
>   * - platform: set index of platform in device list
>   * - device: set index of device in device list
>   *
> @@ -174,8 +171,7 @@ const char *av_opencl_errstr(cl_int status);
>  int av_opencl_register_kernel_code(const char *kernel_code);
>  
>  /**
> - * Initialize the run time OpenCL environment and compile the kernel
> - * code registered with av_opencl_register_kernel_code().
> + * Initialize the run time OpenCL environment
>   *
>   * @param ext_opencl_env external OpenCL environment, created by an
>   *                       application program, ignored if set to NULL
> @@ -190,10 +186,22 @@ int av_opencl_register_kernel_code(const char *kernel_code);
>   *                         the environment used to run the kernel
>   * @param kernel_name      kernel function name
>   * @return >=0 on success, a negative error code in case of failure
> + * @deprecated, use clCreateKernel() directly
>   */
>  int av_opencl_create_kernel(AVOpenCLKernelEnv *env, const char *kernel_name);
>  
>  /**
> + * compile specific OpenCL kernel source
> + *
> + * @param env           pointer to kernel environment
> + * @param program_name  pointer to a program name used for identification
> + * @param build_opts    pointer to a string that describes the preprocessor
> + *                      build options to be used for building the program
> + * @return >=0 on success, a negative error code in case of failure
> + */
> +int av_opencl_compile(AVOpenCLKernelEnv *env, const char *program_name, const char* build_opts);
> +
> +/**
>   * Create OpenCL buffer.
>   *
>   * The buffer is used to save the data used or created by an OpenCL
> @@ -273,6 +281,7 @@ void av_opencl_buffer_release(cl_mem *cl_buf);
>   *
>   * @param env kernel environment where the kernel object was created
>   *            with av_opencl_create_kernel()
> + * @deprecated, use clReleaseKernel() directly
>   */
>  void av_opencl_release_kernel(AVOpenCLKernelEnv *env);
>  
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 67a2acd..f25425b 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -76,7 +76,7 @@
>  
>  #define LIBAVUTIL_VERSION_MAJOR  52
>  #define LIBAVUTIL_VERSION_MINOR  48
> -#define LIBAVUTIL_VERSION_MICRO 100
> +#define LIBAVUTIL_VERSION_MICRO 101

LIBAVUTIL_VERSION_MINOR should be bumped

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131101/1e8895e7/attachment.asc>