diff mbox series

[FFmpeg-devel] avfilter/vf_program_opencl: make it possible to pass plane as argument to kernel

Message ID 20200214104134.27392-1-onemda@gmail.com
State New
Headers show
Series [FFmpeg-devel] avfilter/vf_program_opencl: make it possible to pass plane as argument to kernel
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Paul B Mahol Feb. 14, 2020, 10:41 a.m. UTC
Fixes #7190

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 doc/filters.texi                |  4 ++++
 libavfilter/vf_program_opencl.c | 17 +++++++++++++++++
 2 files changed, 21 insertions(+)

Comments

Mark Thompson Feb. 16, 2020, 8:17 p.m. UTC | #1
On 14/02/2020 10:41, Paul B Mahol wrote:
> Fixes #7190
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  doc/filters.texi                |  4 ++++
>  libavfilter/vf_program_opencl.c | 17 +++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/doc/filters.texi b/doc/filters.texi
> index b957b9302e..5bd0e44b67 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -21297,6 +21297,8 @@ Number of inputs to the filter.  Defaults to 1.
>  @item size, s
>  Size of output frames.  Defaults to the same as the first input.
>  
> +@item plane
> +Currently processed plane number is set as last kernel argument.
>  @end table
>  
>  The program source file must contain a kernel function with the given name,
> @@ -22483,6 +22485,8 @@ Pixel format to use for the generated frames.  This must be set.
>  @item rate, r
>  Number of frames generated every second.  Default value is '25'.
>  
> +@item plane
> +Currently processed plane number is set as last kernel argument.
>  @end table
>  
>  For details of how the program loading works, see the @ref{program_opencl}
> diff --git a/libavfilter/vf_program_opencl.c b/libavfilter/vf_program_opencl.c
> index ec25e931f5..3cf93464ba 100644
> --- a/libavfilter/vf_program_opencl.c
> +++ b/libavfilter/vf_program_opencl.c
> @@ -42,6 +42,7 @@ typedef struct ProgramOpenCLContext {
>      const char         *source_file;
>      const char         *kernel_name;
>      int                 nb_inputs;
> +    int                 plane_is_arg;
>      int                 width, height;
>      enum AVPixelFormat  source_format;
>      AVRational          source_rate;
> @@ -138,6 +139,16 @@ static int program_opencl_run(AVFilterContext *avctx)
>              }
>          }
>  
> +        if (ctx->plane_is_arg) {
> +            cle = clSetKernelArg(ctx->kernel, 2 + ctx->nb_inputs, sizeof(cl_int), &plane);
> +            if (cle != CL_SUCCESS) {
> +                av_log(avctx, AV_LOG_ERROR, "Failed to set kernel "
> +                       "plane argument %d: %d.\n", plane, cle);
> +                err = AVERROR_UNKNOWN;
> +                goto fail;
> +            }
> +        }
> +
>          err = ff_opencl_filter_work_size_from_image(avctx, global_work,
>                                                      output, plane, 0);
>          if (err < 0)
> @@ -348,6 +359,9 @@ static const AVOption program_opencl_options[] = {
>      { "s",      "Video size",       OFFSET(width),
>        AV_OPT_TYPE_IMAGE_SIZE,       { .str = NULL }, 0, 0, FLAGS },
>  
> +    { "plane",  "Pass currently processed plane as argument", OFFSET(plane_is_arg),
> +      AV_OPT_TYPE_BOOL,       { .i64 = 0 }, 0, 1, FLAGS },
> +
>      { NULL },
>  };
>  
> @@ -400,6 +414,9 @@ static const AVOption openclsrc_options[] = {
>      { "r",      "Video frame rate", OFFSET(source_rate),
>        AV_OPT_TYPE_VIDEO_RATE,       { .str = "25" }, 0, INT_MAX, FLAGS },
>  
> +    { "plane",  "Pass currently processed plane as argument", OFFSET(plane_is_arg),
> +      AV_OPT_TYPE_BOOL,       { .i64 = 0 }, 0, 1, FLAGS },
> +
>      { NULL },
>  };
>  
> 

Intuitively this feels like the wrong way to do it to me - that's going to become a conditional inside the kernel, which is always going to go one way or the other on any particular run.  (Do you have a specific example in mind?)

As an alternative to solve the same problem, suppose you could instead provide a list of kernel names which then get executed one for each plane?  Extending the "kernel" option could do that - provide a comma-separated list of kernels, and then run the nth kernel in the list on the nth plane.

- Mark
Paul B Mahol Feb. 16, 2020, 9:25 p.m. UTC | #2
On 2/16/20, Mark Thompson <sw@jkqxz.net> wrote:
> On 14/02/2020 10:41, Paul B Mahol wrote:
>> Fixes #7190
>>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  doc/filters.texi                |  4 ++++
>>  libavfilter/vf_program_opencl.c | 17 +++++++++++++++++
>>  2 files changed, 21 insertions(+)
>>
>> diff --git a/doc/filters.texi b/doc/filters.texi
>> index b957b9302e..5bd0e44b67 100644
>> --- a/doc/filters.texi
>> +++ b/doc/filters.texi
>> @@ -21297,6 +21297,8 @@ Number of inputs to the filter.  Defaults to 1.
>>  @item size, s
>>  Size of output frames.  Defaults to the same as the first input.
>>
>> +@item plane
>> +Currently processed plane number is set as last kernel argument.
>>  @end table
>>
>>  The program source file must contain a kernel function with the given
>> name,
>> @@ -22483,6 +22485,8 @@ Pixel format to use for the generated frames.
>> This must be set.
>>  @item rate, r
>>  Number of frames generated every second.  Default value is '25'.
>>
>> +@item plane
>> +Currently processed plane number is set as last kernel argument.
>>  @end table
>>
>>  For details of how the program loading works, see the
>> @ref{program_opencl}
>> diff --git a/libavfilter/vf_program_opencl.c
>> b/libavfilter/vf_program_opencl.c
>> index ec25e931f5..3cf93464ba 100644
>> --- a/libavfilter/vf_program_opencl.c
>> +++ b/libavfilter/vf_program_opencl.c
>> @@ -42,6 +42,7 @@ typedef struct ProgramOpenCLContext {
>>      const char         *source_file;
>>      const char         *kernel_name;
>>      int                 nb_inputs;
>> +    int                 plane_is_arg;
>>      int                 width, height;
>>      enum AVPixelFormat  source_format;
>>      AVRational          source_rate;
>> @@ -138,6 +139,16 @@ static int program_opencl_run(AVFilterContext *avctx)
>>              }
>>          }
>>
>> +        if (ctx->plane_is_arg) {
>> +            cle = clSetKernelArg(ctx->kernel, 2 + ctx->nb_inputs,
>> sizeof(cl_int), &plane);
>> +            if (cle != CL_SUCCESS) {
>> +                av_log(avctx, AV_LOG_ERROR, "Failed to set kernel "
>> +                       "plane argument %d: %d.\n", plane, cle);
>> +                err = AVERROR_UNKNOWN;
>> +                goto fail;
>> +            }
>> +        }
>> +
>>          err = ff_opencl_filter_work_size_from_image(avctx, global_work,
>>                                                      output, plane, 0);
>>          if (err < 0)
>> @@ -348,6 +359,9 @@ static const AVOption program_opencl_options[] = {
>>      { "s",      "Video size",       OFFSET(width),
>>        AV_OPT_TYPE_IMAGE_SIZE,       { .str = NULL }, 0, 0, FLAGS },
>>
>> +    { "plane",  "Pass currently processed plane as argument",
>> OFFSET(plane_is_arg),
>> +      AV_OPT_TYPE_BOOL,       { .i64 = 0 }, 0, 1, FLAGS },
>> +
>>      { NULL },
>>  };
>>
>> @@ -400,6 +414,9 @@ static const AVOption openclsrc_options[] = {
>>      { "r",      "Video frame rate", OFFSET(source_rate),
>>        AV_OPT_TYPE_VIDEO_RATE,       { .str = "25" }, 0, INT_MAX, FLAGS },
>>
>> +    { "plane",  "Pass currently processed plane as argument",
>> OFFSET(plane_is_arg),
>> +      AV_OPT_TYPE_BOOL,       { .i64 = 0 }, 0, 1, FLAGS },
>> +
>>      { NULL },
>>  };
>>
>>
>
> Intuitively this feels like the wrong way to do it to me - that's going to
> become a conditional inside the kernel, which is always going to go one way
> or the other on any particular run.  (Do you have a specific example in
> mind?)
>
> As an alternative to solve the same problem, suppose you could instead
> provide a list of kernel names which then get executed one for each plane?
> Extending the "kernel" option could do that - provide a comma-separated list
> of kernels, and then run the nth kernel in the list on the nth plane.
>

Agreed, seems better solution.
diff mbox series

Patch

diff --git a/doc/filters.texi b/doc/filters.texi
index b957b9302e..5bd0e44b67 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -21297,6 +21297,8 @@  Number of inputs to the filter.  Defaults to 1.
 @item size, s
 Size of output frames.  Defaults to the same as the first input.
 
+@item plane
+Currently processed plane number is set as last kernel argument.
 @end table
 
 The program source file must contain a kernel function with the given name,
@@ -22483,6 +22485,8 @@  Pixel format to use for the generated frames.  This must be set.
 @item rate, r
 Number of frames generated every second.  Default value is '25'.
 
+@item plane
+Currently processed plane number is set as last kernel argument.
 @end table
 
 For details of how the program loading works, see the @ref{program_opencl}
diff --git a/libavfilter/vf_program_opencl.c b/libavfilter/vf_program_opencl.c
index ec25e931f5..3cf93464ba 100644
--- a/libavfilter/vf_program_opencl.c
+++ b/libavfilter/vf_program_opencl.c
@@ -42,6 +42,7 @@  typedef struct ProgramOpenCLContext {
     const char         *source_file;
     const char         *kernel_name;
     int                 nb_inputs;
+    int                 plane_is_arg;
     int                 width, height;
     enum AVPixelFormat  source_format;
     AVRational          source_rate;
@@ -138,6 +139,16 @@  static int program_opencl_run(AVFilterContext *avctx)
             }
         }
 
+        if (ctx->plane_is_arg) {
+            cle = clSetKernelArg(ctx->kernel, 2 + ctx->nb_inputs, sizeof(cl_int), &plane);
+            if (cle != CL_SUCCESS) {
+                av_log(avctx, AV_LOG_ERROR, "Failed to set kernel "
+                       "plane argument %d: %d.\n", plane, cle);
+                err = AVERROR_UNKNOWN;
+                goto fail;
+            }
+        }
+
         err = ff_opencl_filter_work_size_from_image(avctx, global_work,
                                                     output, plane, 0);
         if (err < 0)
@@ -348,6 +359,9 @@  static const AVOption program_opencl_options[] = {
     { "s",      "Video size",       OFFSET(width),
       AV_OPT_TYPE_IMAGE_SIZE,       { .str = NULL }, 0, 0, FLAGS },
 
+    { "plane",  "Pass currently processed plane as argument", OFFSET(plane_is_arg),
+      AV_OPT_TYPE_BOOL,       { .i64 = 0 }, 0, 1, FLAGS },
+
     { NULL },
 };
 
@@ -400,6 +414,9 @@  static const AVOption openclsrc_options[] = {
     { "r",      "Video frame rate", OFFSET(source_rate),
       AV_OPT_TYPE_VIDEO_RATE,       { .str = "25" }, 0, INT_MAX, FLAGS },
 
+    { "plane",  "Pass currently processed plane as argument", OFFSET(plane_is_arg),
+      AV_OPT_TYPE_BOOL,       { .i64 = 0 }, 0, 1, FLAGS },
+
     { NULL },
 };