[FFmpeg-devel] [PATCH 1/2] libavutil/opencl: add user spec device interface

[Wei Gao]

2013/4/9 Stefano Sabatini <stefasab at gmail.com>

> On date Sunday 2013-04-07 20:10:36 +0800, Wei Gao encoded:
> >
>
> > From d8b8bd67f62d65965684b9a9d8f3b99bbb257233 Mon Sep 17 00:00:00 2001
> > From: highgod0401 <highgod0401 at gmail.com>
> > Date: Sun, 7 Apr 2013 20:05:31 +0800
> > Subject: [PATCH 1/2] add user spec device interface
> >
> > ---
> >  ffmpeg_opt.c       |  25 ++++
> >  libavutil/opencl.c | 379
> ++++++++++++++++++++++++++++++-----------------------
> >  libavutil/opencl.h |  67 +++++++++-
> >  3 files changed, 297 insertions(+), 174 deletions(-)
>
> Missing docs update in doc/ffmpeg.texi.
>
> >
> > diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
> > index 63a238d..256319f 100644
> > --- a/ffmpeg_opt.c
> > +++ b/ffmpeg_opt.c
> > @@ -41,6 +41,9 @@
> >  #include "libavutil/parseutils.h"
> >  #include "libavutil/pixdesc.h"
> >  #include "libavutil/pixfmt.h"
> > +#if CONFIG_OPENCL
> > +#include "libavutil/opencl.h"
> > +#endif
> >
> >  #define MATCH_PER_STREAM_OPT(name, type, outvar, fmtctx, st)\
> >  {\
> > @@ -316,6 +319,24 @@ static int opt_attach(void *optctx, const char
> *opt, const char *arg)
> >      o->attachments[o->nb_attachments - 1] = arg;
> >      return 0;
> >  }
>
> > +#if CONFIG_OPENCL
> > +static int opt_opencl(void *optctx, const char *opt, const char *arg)
> > +{
> > +    char *key, *value;
> > +    const char *temp = arg;
> > +    int ret = 0;
> > +    while (strlen(temp)) {
> > +        key = av_get_token(&temp, "=,:[\n");
> > +        temp++;
> > +        value = av_get_token(&temp, "=,:[\n");
> > +        temp++;
>
> buffer overread in case the string is ""
>
> Also why all this special characters? What's the format of the string
> you're supposed to parse?
>
the command like this: -openclopt buildoptions=-I.:platform=0:device=0. so
does it parsing ":" and "=" enough?

>
>
> > +    AVDictionary *options;
> >  } OpenclUtils;
>
> Note: a possibly cleaner design would be to move this to GPUEnv, and
> directly use the global GPUEnv context (which could be renamed to
> OpenCLContext).


> This would allow to expose this global context and allow the user to
> directly set options on it, rather than rely on intermediary av_opt_
> wrappers.
>
> Feel free to postpone this to another patch.
>
> will do it in the later patch

> >
> > +#define OFFSET(x) offsetof(OpenclUtils, x)
> > +
>
>
>
> > +AVOpenCLDeviceList *av_opencl_get_device_list(void)
> > +{
> > +    AVOpenCLDeviceList *device_list = NULL;
> > +    LOCK_OPENCL
> > +    device_list = av_mallocz(sizeof(AVOpenCLDeviceList));
> > +    if (!device_list) {
> > +        av_log(&openclutils, AV_LOG_ERROR,
> > +               "Could not malloc opencl device list data space\n");
> > +        return NULL;
> > +    }
> > +    if (!gpu_env.device_list.platform_num) {
> > +        if (get_device_list(&gpu_env.device_list) < 0) {
>
> > +            av_log(&openclutils, AV_LOG_ERROR,
> > +               "Could not get device list from environment\n");
>
> nit: weird indent
>
> > +            av_freep(&device_list);
> > +        }
> > +    }
> > +    *device_list = gpu_env.device_list;
>
> here you're passing global data to the application, which could be
> destroyed if the application calls av_opencl_get_device_list() again.
>
I use "gpu_env.device_list.platform_num" to check whether the device list
has been getted. I think if the platform is 0, it is OK to get the device
list again because the previous operations didn't get the list. If the
pervious operations has got the device list, it just copy the global data
to the new memory, And it can avoid user to modify the global data.

>
>
> --
> FFmpeg = Funny and Fierce Mystic Peaceless Enlightened Guru
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>