[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