[FFmpeg-devel] [PATCH 2/3] libavutil/libavfilter: add spec opencl device APIs 20130410

Stefano Sabatini stefasab at gmail.com
Wed Apr 10 16:51:36 CEST 2013


On date Wednesday 2013-04-10 22:22:04 +0800, Wei Gao encoded:
> 2013/4/10 Stefano Sabatini <stefasab at gmail.com>
> 
> > On date Wednesday 2013-04-10 17:32:41 +0800, Wei Gao encoded:
> > >
> >
> > > From 538527370e0f4b8fc321580792384db7c9c2ab73 Mon Sep 17 00:00:00 2001
> > > From: highgod0401 <highgod0401 at gmail.com>
> > > Date: Wed, 10 Apr 2013 17:18:24 +0800
> > > Subject: [PATCH 2/3] add spec opencl device APIs 20130410
> > >
> > > ---
> > >  libavfilter/deshake_opencl.c |  5 +--
> > >  libavutil/opencl.c           | 83
> > +++++++++++++++++++++++++++-----------------
> > >  libavutil/opencl.h           | 39 +++++++++++++++++----
> > >  libavutil/version.h          |  2 +-
> > >  4 files changed, 87 insertions(+), 42 deletions(-)
> >
> > Missing docs. You could create a doc/opencl.texi file.
> 
> Can I add the opencl.texi in a later patch? thanks

@chapter OpenCL Options

When FFmpeg is configured with @code{--enable-opencl}, it is possible
to set the options to set in the global OpenCL context. The list of
supported options follows:

@table @option
@item build_options
...
@item platform_idx
...
@item device_idx
...
@end table

...

Then you need to add an include in doc/all_components.texi,
ffmpeg-utils.texi and libavutil.texi. Ideally we should merge these
files in a single utils.texi, so all libavutil doc is contained in a
single file.

> 
> >
> >
> > --
> > > 1.7.11.msysgit.1
> > --
> > FFmpeg = Funny and Fierce Majestic Puristic Earthshaking Gymnast
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >

> From 0f4bd860a835c414f1c4bda14539e60a726c0668 Mon Sep 17 00:00:00 2001
> From: highgod0401 <highgod0401 at gmail.com>
> Date: Wed, 10 Apr 2013 22:18:54 +0800
> Subject: [PATCH 2/3] add spec opencl device APIs 20130410 new
> 
> ---
>  libavfilter/deshake_opencl.c |  5 +--
>  libavutil/opencl.c           | 76 +++++++++++++++++++++++++-------------------
>  libavutil/opencl.h           | 38 ++++++++++++++++++----
>  libavutil/version.h          |  2 +-
>  4 files changed, 78 insertions(+), 43 deletions(-)
> 
> diff --git a/libavfilter/deshake_opencl.c b/libavfilter/deshake_opencl.c
> index 63d144a..0f6dcc4 100644
> --- a/libavfilter/deshake_opencl.c
> +++ b/libavfilter/deshake_opencl.c
> @@ -98,10 +98,7 @@ int ff_opencl_deshake_init(AVFilterContext *ctx)
>  {
>      int ret = 0;
>      DeshakeContext *deshake = ctx->priv;
> -    AVDictionary *options = NULL;
> -    av_dict_set(&options, "build_options", "-I.", 0);
> -    ret = av_opencl_init(options, NULL);
> -    av_dict_free(&options);
> +    ret = av_opencl_init(NULL);
>      if (ret < 0)
>          return ret;
>      deshake->opencl_ctx.matrix_size = MATRIX_SIZE;
> diff --git a/libavutil/opencl.c b/libavutil/opencl.c
> index 2e5797e..ddcbf50 100644
> --- a/libavutil/opencl.c
> +++ b/libavutil/opencl.c
> @@ -24,6 +24,7 @@
>  #include "avstring.h"
>  #include "log.h"
>  #include "avassert.h"
> +#include "opt.h"
>  
>  #if HAVE_PTHREADS
>  
> @@ -73,10 +74,23 @@ typedef struct {
>      const AVClass *class;
>      int log_offset;
>      void *log_ctx;
> +    int init_flag;
> +    int platform_idx;
> +    int device_idx;
> +    char *build_options;
>  } OpenclUtils;
>  
> +#define OFFSET(x) offsetof(OpenclUtils, x)
> +
> +static const AVOption opencl_options[] = {
> +     { "platform_idx",        "set platform index value",  OFFSET(platform_idx),  AV_OPT_TYPE_INT,    {.i64=-1}, -1, INT_MAX},
> +     { "device_idx",          "set device index value",    OFFSET(device_idx),    AV_OPT_TYPE_INT,    {.i64=-1}, -1, INT_MAX},
> +     { "build_options",   "build options of opencl",   OFFSET(build_options), AV_OPT_TYPE_STRING, {.str="-I."},  CHAR_MIN, CHAR_MAX},

nit: weird align

> +};
> +
>  static const AVClass openclutils_class = {
>      .class_name                = "OPENCLUTILS",
> +    .option                    = opencl_options,
>      .item_name                 = av_default_item_name,
>      .version                   = LIBAVUTIL_VERSION_INT,
>      .log_level_offset_offset   = offsetof(OpenclUtils, log_offset),
> @@ -311,6 +325,29 @@ void av_opencl_free_device_list(AVOpenCLDeviceList **device_list)
>      av_freep(device_list);
>  }
>  
> +int av_opencl_set_option(const char *key, const char *val)
> +{
> +    int ret = 0;
> +    LOCK_OPENCL
> +    if (!openclutils.init_flag) {
> +        av_opt_set_defaults(&openclutils);
> +        openclutils.init_flag = 1;
> +    }
> +    ret = av_opt_set(&openclutils, key, val, 0);
> +    UNLOCK_OPENCL
> +    return ret;
> +}
> +
> +

Nit+++: just one empty line is enough

> +int av_opencl_get_option(const char *key, uint8_t **out_val)
> +{
> +    int ret = 0;
> +    LOCK_OPENCL
> +    ret = av_opt_get(&openclutils, key, 0, out_val);
> +    UNLOCK_OPENCL
> +    return ret;
> +}
> +
>  AVOpenCLExternalEnv *av_opencl_alloc_external_env(void)
>  {
>      AVOpenCLExternalEnv *ext = av_mallocz(sizeof(AVOpenCLExternalEnv));
> @@ -561,47 +598,22 @@ end:
>      return ret;
>  }
>  
> -int av_opencl_init(AVDictionary *options, AVOpenCLExternalEnv *ext_opencl_env)
> +int av_opencl_init(AVOpenCLExternalEnv *ext_opencl_env)
>  {
>      int ret = 0;
> -    AVDictionaryEntry *opt_build_entry;
> -    AVDictionaryEntry *opt_platform_entry;
> -    AVDictionaryEntry *opt_device_entry;
> -    char *pos;
> -    AVOpenCLDeviceList *device_list;
>      LOCK_OPENCL
>      if (!gpu_env.init_count) {
> -        opt_platform_entry = av_dict_get(options, "platform_idx", NULL, 0);
> -        opt_device_entry   = av_dict_get(options, "device_idx", NULL, 0);
> -        /* initialize devices, context, command_queue */
> -        gpu_env.platform_idx = -1;
> -        gpu_env.device_idx = -1;
> -        if (opt_platform_entry) {
> -            gpu_env.platform_idx = strtol(opt_platform_entry->value, &pos, 10);
> -            if (pos == opt_platform_entry->value) {
> -                av_log(&openclutils, AV_LOG_ERROR, "Platform index should be a number\n");
> -                ret = AVERROR(EINVAL);
> -                goto end;
> -            }
> -        }
> -        if (opt_device_entry) {
> -            gpu_env.device_idx = strtol(opt_device_entry->value, &pos, 10);
> -            if (pos == opt_platform_entry->value) {
> -                av_log(&openclutils, AV_LOG_ERROR, "Device index should be a number\n");
> -                ret = AVERROR(EINVAL);
> -                goto end;
> -            }

> +        if (!openclutils.init_flag) {
> +            av_opt_set_defaults(&openclutils);
> +            openclutils.init_flag = 1;
>          }
> +        gpu_env.device_idx   = openclutils.device_idx;
> +        gpu_env.platform_idx = openclutils.platform_idx;
>          ret = init_opencl_env(&gpu_env, ext_opencl_env);
>          if (ret < 0)
>              goto end;
>      }
> -    /*initialize program, kernel_name, kernel_count*/
> -    opt_build_entry = av_dict_get(options, "build_options", NULL, 0);
> -    if (opt_build_entry)
> -        ret = compile_kernel_file(&gpu_env, opt_build_entry->value);
> -    else
> -        ret = compile_kernel_file(&gpu_env, NULL);
> +    ret = compile_kernel_file(&gpu_env, openclutils.build_options);
>      if (ret < 0)
>          goto end;
>      if (gpu_env.kernel_code_count <= 0) {

We should probably free openclutils with av_opt_free() at some point,
but I can't see a safe way to do it. Ideally if you set it once in the
global context, build_options should not be reset when doing
av_opencl_init() again. A possibility would be to have a dedicated
function av_opencl_free() to free global data, but looks somehow
awkward for the user.

You may add a note somewhere in the code:
FIXME: free openclutils context

unless someone has a better idea.

> diff --git a/libavutil/opencl.h b/libavutil/opencl.h
> index a34b1b7..1dce028 100644
> --- a/libavutil/opencl.h
> +++ b/libavutil/opencl.h
> @@ -97,6 +97,37 @@ int av_opencl_get_device_list(AVOpenCLDeviceList **device_list);
>  void av_opencl_free_device_list(AVOpenCLDeviceList **device_list);
>  
>  /**
> + * Set option in the global OpenCL context.
> + *
> + * This options affect the operation performed by the next
> + * 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
> + *
> + * See reference "OpenCL Specification Version: 1.2 chapter 5.6.4".
> + *
> + * @param key                 option key
> + * @param val                 option value
> + * @return >=0 on success, a negative error code in case of failure
> + * @see av_opencl_get_option()
> + */
> +int av_opencl_set_option(const char *key, const char *val);
> +
> +/**
> + * Get option value from the global OpenCL context.
> + *
> + * @param key        option key
> + * @param out_val  pointer to location where option value will be
> + *                         written, must be freed with av_freep()
> + * @return  >=0 on success, a negative error code in case of failure
> + * @see av_opencl_set_option()
> + */
> +int av_opencl_get_option(const char *key, uint8_t **out_val);
> +
> +/**
>   * Allocate OpenCL external environment.
>   *
>   * It must be freed with av_opencl_free_external_env().
> @@ -128,16 +159,11 @@ 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().
>   *
> - * Currently, the only accepted option is "build_options", used to set
> - * options to compile registered kernels code. See reference "OpenCL
> - * Specification Version: 1.2 chapter 5.6.4".
> - *
> - * @param options        dictionary of key/value options
>   * @param ext_opencl_env external OpenCL environment, created by an
>   *                       application program, ignored if set to NULL
>   * @return >=0 on success, a negative error code in case of failure
>   */
> - int av_opencl_init(AVDictionary *options, AVOpenCLExternalEnv *ext_opencl_env);
> + int av_opencl_init(AVOpenCLExternalEnv *ext_opencl_env);

LGTM otherwise, thanks.
-- 
FFmpeg = Frenzy and Fostering Minimal Pitiless Eager Gymnast


More information about the ffmpeg-devel mailing list