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

Lenny Wang lenny at multicorewareinc.com
Fri Nov 1 23:15:54 CET 2013


On Fri, Nov 1, 2013 at 2:20 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> 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
>

Newly adjusted patch based on Michael's comments.  Please review, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: compile_opencl_kernels_separately.patch
Type: application/octet-stream
Size: 23986 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131101/2cabd039/attachment.obj>


More information about the ffmpeg-devel mailing list