[FFmpeg-devel,2/4] lavfi/opencl: Handle overlay input formats correctly.

Submitted by Ruiling Song on Oct. 29, 2018, 5:18 a.m.

Details

Message ID 1540790282-29764-2-git-send-email-ruiling.song@intel.com
State Superseded
Headers show

Commit Message

Ruiling Song Oct. 29, 2018, 5:18 a.m.
The main input may have alpha channel, we just ignore it.
Also add some checks for incompatible input formats.

Signed-off-by: Ruiling Song <ruiling.song@intel.com>
---
 libavfilter/vf_overlay_opencl.c | 58 ++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 12 deletions(-)

Comments

Ruiling Song Nov. 7, 2018, 2:02 a.m.
> -----Original Message-----
> From: Song, Ruiling
> Sent: Monday, October 29, 2018 1:18 PM
> To: ffmpeg-devel@ffmpeg.org
> Cc: Song, Ruiling <ruiling.song@intel.com>
> Subject: [PATCH 2/4] lavfi/opencl: Handle overlay input formats correctly.
> 
> The main input may have alpha channel, we just ignore it.
> Also add some checks for incompatible input formats.
> 
> Signed-off-by: Ruiling Song <ruiling.song@intel.com>
> ---
>  libavfilter/vf_overlay_opencl.c | 58 ++++++++++++++++++++++++++++++++-----
> ----
>  1 file changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/libavfilter/vf_overlay_opencl.c b/libavfilter/vf_overlay_opencl.c
> index e9c8532..320c1a5 100644
> --- a/libavfilter/vf_overlay_opencl.c
> +++ b/libavfilter/vf_overlay_opencl.c
> @@ -37,7 +37,7 @@ typedef struct OverlayOpenCLContext {
> 
>      FFFrameSync      fs;
> 
> -    int              nb_planes;
> +    int              nb_color_planes;
>      int              x_subsample;
>      int              y_subsample;
>      int              alpha_separate;
> @@ -46,6 +46,22 @@ typedef struct OverlayOpenCLContext {
>      int              y_position;
>  } OverlayOpenCLContext;
> 
> +static int has_planar_alpha(const AVPixFmtDescriptor *fmt) {
> +    int nb_components;
> +    int has_alpha = !!(fmt->flags & AV_PIX_FMT_FLAG_ALPHA);
> +    if (!has_alpha) return 0;
> +
> +    nb_components = fmt->nb_components;
> +    // PAL8
> +    if (nb_components < 2) return 0;
> +
> +    if (fmt->comp[nb_components - 1].plane >
> +        fmt->comp[nb_components - 2].plane)
> +        return 1;
> +    else
> +        return 0;
> +}
> +
>  static int overlay_opencl_load(AVFilterContext *avctx,
>                                 enum AVPixelFormat main_format,
>                                 enum AVPixelFormat overlay_format)
> @@ -55,10 +71,13 @@ static int overlay_opencl_load(AVFilterContext *avctx,
>      const char *source = ff_opencl_source_overlay;
>      const char *kernel;
>      const AVPixFmtDescriptor *main_desc, *overlay_desc;
> -    int err, i, main_planes, overlay_planes;
> +    int err, i, main_planes, overlay_planes, overlay_alpha,
> +        main_planar_alpha, overlay_planar_alpha;
> 
>      main_desc    = av_pix_fmt_desc_get(main_format);
>      overlay_desc = av_pix_fmt_desc_get(overlay_format);
> +    overlay_alpha = !!(overlay_desc->flags & AV_PIX_FMT_FLAG_ALPHA);
> +    main_planar_alpha = has_planar_alpha(main_desc);
> 
>      main_planes = overlay_planes = 0;
>      for (i = 0; i < main_desc->nb_components; i++)
> @@ -68,7 +87,7 @@ static int overlay_opencl_load(AVFilterContext *avctx,
>          overlay_planes = FFMAX(overlay_planes,
>                                 overlay_desc->comp[i].plane + 1);
> 
> -    ctx->nb_planes = main_planes;
> +    ctx->nb_color_planes = main_planar_alpha ? (main_planes - 1) : main_planes;
>      ctx->x_subsample = 1 << main_desc->log2_chroma_w;
>      ctx->y_subsample = 1 << main_desc->log2_chroma_h;
> 
> @@ -80,15 +99,30 @@ static int overlay_opencl_load(AVFilterContext *avctx,
>                 ctx->x_subsample, ctx->y_subsample);
>      }
> 
> -    if (main_planes == overlay_planes) {
> -        if (main_desc->nb_components == overlay_desc->nb_components)
> -            kernel = "overlay_no_alpha";
> -        else
> -            kernel = "overlay_internal_alpha";
> +    if ((main_desc->flags & AV_PIX_FMT_FLAG_RGB) !=
> +        (overlay_desc->flags & AV_PIX_FMT_FLAG_RGB)) {
> +        av_log(avctx, AV_LOG_ERROR, "mixed YUV/RGB input formats.\n");
> +        return AVERROR(EINVAL);
> +    }
> +
> +    if (main_desc->log2_chroma_w != overlay_desc->log2_chroma_w ||
> +        main_desc->log2_chroma_h != overlay_desc->log2_chroma_h) {
> +        av_log(avctx, AV_LOG_ERROR, "incompatible chroma sub-sampling.\n");
> +        return AVERROR(EINVAL);
> +    }
> +
> +    if (!overlay_alpha) {
>          ctx->alpha_separate = 0;
> +        kernel = "overlay_no_alpha";
>      } else {
> -        kernel = "overlay_external_alpha";
> -        ctx->alpha_separate = 1;
> +        overlay_planar_alpha = has_planar_alpha(overlay_desc);
> +        if (overlay_planar_alpha) {
> +            ctx->alpha_separate = 1;
> +            kernel = "overlay_external_alpha";
> +        } else {
> +            ctx->alpha_separate = 0;
> +            kernel = "overlay_internal_alpha";
> +        }
>      }
> 
>      av_log(avctx, AV_LOG_DEBUG, "Using kernel %s.\n", kernel);
> @@ -155,7 +189,7 @@ static int overlay_opencl_blend(FFFrameSync *fs)
>          goto fail;
>      }
> 
> -    for (plane = 0; plane < ctx->nb_planes; plane++) {
> +    for (plane = 0; plane < ctx->nb_color_planes; plane++) {
>          kernel_arg = 0;
> 
>          mem = (cl_mem)output->data[plane];
> @@ -171,7 +205,7 @@ static int overlay_opencl_blend(FFFrameSync *fs)
>          kernel_arg++;
> 
>          if (ctx->alpha_separate) {
> -            mem = (cl_mem)input_overlay->data[ctx->nb_planes];
> +            mem = (cl_mem)input_overlay->data[ctx->nb_color_planes];
>              CL_SET_KERNEL_ARG(ctx->kernel, kernel_arg, cl_mem, &mem);
>              kernel_arg++;
>          }
> --
Any comments on this patch?

> 2.7.4
Zhong Li Nov. 7, 2018, 6:57 a.m.
> > -----Original Message-----

> > From: Song, Ruiling

> > Sent: Monday, October 29, 2018 1:18 PM

> > To: ffmpeg-devel@ffmpeg.org

> > Cc: Song, Ruiling <ruiling.song@intel.com>

> > Subject: [PATCH 2/4] lavfi/opencl: Handle overlay input formats correctly.

> >

> > The main input may have alpha channel, we just ignore it.

> > Also add some checks for incompatible input formats.

> >

> > Signed-off-by: Ruiling Song <ruiling.song@intel.com>

LGTM.
BTW, could the main input with alpha case be supported?
Ruiling Song Nov. 7, 2018, 8:14 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of

> Li, Zhong

> Sent: Wednesday, November 7, 2018 2:58 PM

> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH 2/4] lavfi/opencl: Handle overlay input

> formats correctly.

> 

> > > -----Original Message-----

> > > From: Song, Ruiling

> > > Sent: Monday, October 29, 2018 1:18 PM

> > > To: ffmpeg-devel@ffmpeg.org

> > > Cc: Song, Ruiling <ruiling.song@intel.com>

> > > Subject: [PATCH 2/4] lavfi/opencl: Handle overlay input formats correctly.

> > >

> > > The main input may have alpha channel, we just ignore it.

> > > Also add some checks for incompatible input formats.

> > >

> > > Signed-off-by: Ruiling Song <ruiling.song@intel.com>

> LGTM.

> BTW, could the main input with alpha case be supported?


I am not sure what kind of support do you mean?
I simply ignore the alpha channel of the main input, and do the alpha blending using the overlay alpha.
Before this patch, the filter will do it wrong if the main input has alpha channel. Now it works with this patch.

Thanks!
Ruiling
> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Zhong Li Nov. 7, 2018, 8:37 a.m.
> > > > -----Original Message-----

> > > > From: Song, Ruiling

> > > > Sent: Monday, October 29, 2018 1:18 PM

> > > > To: ffmpeg-devel@ffmpeg.org

> > > > Cc: Song, Ruiling <ruiling.song@intel.com>

> > > > Subject: [PATCH 2/4] lavfi/opencl: Handle overlay input formats

> correctly.

> > > >

> > > > The main input may have alpha channel, we just ignore it.

> > > > Also add some checks for incompatible input formats.

> > > >

> > > > Signed-off-by: Ruiling Song <ruiling.song@intel.com>

> > LGTM.

> > BTW, could the main input with alpha case be supported?

> 

> I am not sure what kind of support do you mean?

> I simply ignore the alpha channel of the main input, and do the alpha

> blending using the overlay alpha.

> Before this patch, the filter will do it wrong if the main input has alpha

> channel. Now it works with this patch.

> 

> Thanks!

> Ruiling


I mean support alpha blending with alpha channel of main input when no overlay alpha.
Ruiling Song Nov. 7, 2018, 3:32 p.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of

> Li, Zhong

> Sent: Wednesday, November 7, 2018 4:37 PM

> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH 2/4] lavfi/opencl: Handle overlay input

> formats correctly.

> 

> > > > > -----Original Message-----

> > > > > From: Song, Ruiling

> > > > > Sent: Monday, October 29, 2018 1:18 PM

> > > > > To: ffmpeg-devel@ffmpeg.org

> > > > > Cc: Song, Ruiling <ruiling.song@intel.com>

> > > > > Subject: [PATCH 2/4] lavfi/opencl: Handle overlay input formats

> > correctly.

> > > > >

> > > > > The main input may have alpha channel, we just ignore it.

> > > > > Also add some checks for incompatible input formats.

> > > > >

> > > > > Signed-off-by: Ruiling Song <ruiling.song@intel.com>

> > > LGTM.

> > > BTW, could the main input with alpha case be supported?

> >

> > I am not sure what kind of support do you mean?

> > I simply ignore the alpha channel of the main input, and do the alpha

> > blending using the overlay alpha.

> > Before this patch, the filter will do it wrong if the main input has alpha

> > channel. Now it works with this patch.

> >

> > Thanks!

> > Ruiling

> 

> I mean support alpha blending with alpha channel of main input when no overlay

> alpha.

I think I got your idea, this patch aims to fix the issues reported by Gyan.
If people really want it (blending using alpha of main input) be supported, we can add it later.
I am not sure whether this sounds ok?

Thanks!
Ruiling

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Zhong Li Nov. 8, 2018, 3:39 a.m.
> > -----Original Message-----

> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf

> > Of Li, Zhong

> > Sent: Wednesday, November 7, 2018 4:37 PM

> > To: FFmpeg development discussions and patches

> > <ffmpeg-devel@ffmpeg.org>

> > Subject: Re: [FFmpeg-devel] [PATCH 2/4] lavfi/opencl: Handle overlay

> > input formats correctly.

> >

> > > > > > -----Original Message-----

> > > > > > From: Song, Ruiling

> > > > > > Sent: Monday, October 29, 2018 1:18 PM

> > > > > > To: ffmpeg-devel@ffmpeg.org

> > > > > > Cc: Song, Ruiling <ruiling.song@intel.com>

> > > > > > Subject: [PATCH 2/4] lavfi/opencl: Handle overlay input

> > > > > > formats

> > > correctly.

> > > > > >

> > > > > > The main input may have alpha channel, we just ignore it.

> > > > > > Also add some checks for incompatible input formats.

> > > > > >

> > > > > > Signed-off-by: Ruiling Song <ruiling.song@intel.com>

> > > > LGTM.

> > > > BTW, could the main input with alpha case be supported?

> > >

> > > I am not sure what kind of support do you mean?

> > > I simply ignore the alpha channel of the main input, and do the

> > > alpha blending using the overlay alpha.

> > > Before this patch, the filter will do it wrong if the main input has

> > > alpha channel. Now it works with this patch.

> > >

> > > Thanks!

> > > Ruiling

> >

> > I mean support alpha blending with alpha channel of main input when no

> > overlay alpha.

> I think I got your idea, this patch aims to fix the issues reported by Gyan.

> If people really want it (blending using alpha of main input) be supported, we

> can add it later.

> I am not sure whether this sounds ok?

> 

> Thanks!

> Ruiling


Sure, sound good.
Ruiling Song Nov. 9, 2018, 8:34 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of

> Li, Zhong

> Sent: Thursday, November 8, 2018 11:39 AM

> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH 2/4] lavfi/opencl: Handle overlay input

> formats correctly.

> 

> > > -----Original Message-----

> > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> > Behalf

> > > Of Li, Zhong

> > > Sent: Wednesday, November 7, 2018 4:37 PM

> > > To: FFmpeg development discussions and patches

> > > <ffmpeg-devel@ffmpeg.org>

> > > Subject: Re: [FFmpeg-devel] [PATCH 2/4] lavfi/opencl: Handle overlay

> > > input formats correctly.

> > >

> > > > > > > -----Original Message-----

> > > > > > > From: Song, Ruiling

> > > > > > > Sent: Monday, October 29, 2018 1:18 PM

> > > > > > > To: ffmpeg-devel@ffmpeg.org

> > > > > > > Cc: Song, Ruiling <ruiling.song@intel.com>

> > > > > > > Subject: [PATCH 2/4] lavfi/opencl: Handle overlay input

> > > > > > > formats

> > > > correctly.

> > > > > > >

> > > > > > > The main input may have alpha channel, we just ignore it.

> > > > > > > Also add some checks for incompatible input formats.

> > > > > > >

> > > > > > > Signed-off-by: Ruiling Song <ruiling.song@intel.com>

> > > > > LGTM.

> > > > > BTW, could the main input with alpha case be supported?

> > > >

> > > > I am not sure what kind of support do you mean?

> > > > I simply ignore the alpha channel of the main input, and do the

> > > > alpha blending using the overlay alpha.

> > > > Before this patch, the filter will do it wrong if the main input has

> > > > alpha channel. Now it works with this patch.

> > > >

> > > > Thanks!

> > > > Ruiling

> > >

> > > I mean support alpha blending with alpha channel of main input when no

> > > overlay alpha.

> > I think I got your idea, this patch aims to fix the issues reported by Gyan.

> > If people really want it (blending using alpha of main input) be supported, we

> > can add it later.

> > I am not sure whether this sounds ok?

> >

> > Thanks!

> > Ruiling

> 

> Sure, sound good.


Can we merge this patch? Any objection or concern?

Ruiling
> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Zhong Li Nov. 9, 2018, 9:18 a.m.
> > > > > > > > The main input may have alpha channel, we just ignore it.

> > > > > > > > Also add some checks for incompatible input formats.

> > > > > > > >

> > > > > > > > Signed-off-by: Ruiling Song <ruiling.song@intel.com>

> > > > > > LGTM.

> > > > > > BTW, could the main input with alpha case be supported?

> > > > >

> > > > > I am not sure what kind of support do you mean?

> > > > > I simply ignore the alpha channel of the main input, and do the

> > > > > alpha blending using the overlay alpha.

> > > > > Before this patch, the filter will do it wrong if the main input

> > > > > has alpha channel. Now it works with this patch.

> > > > >

> > > > > Thanks!

> > > > > Ruiling

> > > >

> > > > I mean support alpha blending with alpha channel of main input

> > > > when no overlay alpha.

> > > I think I got your idea, this patch aims to fix the issues reported by Gyan.

> > > If people really want it (blending using alpha of main input) be

> > > supported, we can add it later.

> > > I am not sure whether this sounds ok?

> > >

> > > Thanks!

> > > Ruiling

> >

> > Sure, sound good.

> 

> Can we merge this patch? Any objection or concern?

> 

> Ruiling


Could you please provide the use case (with an alpha channel main input) to verify it?

If no body against, I prefer to merge it after verification during next a few days.

Patch hide | download patch | download mbox

diff --git a/libavfilter/vf_overlay_opencl.c b/libavfilter/vf_overlay_opencl.c
index e9c8532..320c1a5 100644
--- a/libavfilter/vf_overlay_opencl.c
+++ b/libavfilter/vf_overlay_opencl.c
@@ -37,7 +37,7 @@  typedef struct OverlayOpenCLContext {
 
     FFFrameSync      fs;
 
-    int              nb_planes;
+    int              nb_color_planes;
     int              x_subsample;
     int              y_subsample;
     int              alpha_separate;
@@ -46,6 +46,22 @@  typedef struct OverlayOpenCLContext {
     int              y_position;
 } OverlayOpenCLContext;
 
+static int has_planar_alpha(const AVPixFmtDescriptor *fmt) {
+    int nb_components;
+    int has_alpha = !!(fmt->flags & AV_PIX_FMT_FLAG_ALPHA);
+    if (!has_alpha) return 0;
+
+    nb_components = fmt->nb_components;
+    // PAL8
+    if (nb_components < 2) return 0;
+
+    if (fmt->comp[nb_components - 1].plane >
+        fmt->comp[nb_components - 2].plane)
+        return 1;
+    else
+        return 0;
+}
+
 static int overlay_opencl_load(AVFilterContext *avctx,
                                enum AVPixelFormat main_format,
                                enum AVPixelFormat overlay_format)
@@ -55,10 +71,13 @@  static int overlay_opencl_load(AVFilterContext *avctx,
     const char *source = ff_opencl_source_overlay;
     const char *kernel;
     const AVPixFmtDescriptor *main_desc, *overlay_desc;
-    int err, i, main_planes, overlay_planes;
+    int err, i, main_planes, overlay_planes, overlay_alpha,
+        main_planar_alpha, overlay_planar_alpha;
 
     main_desc    = av_pix_fmt_desc_get(main_format);
     overlay_desc = av_pix_fmt_desc_get(overlay_format);
+    overlay_alpha = !!(overlay_desc->flags & AV_PIX_FMT_FLAG_ALPHA);
+    main_planar_alpha = has_planar_alpha(main_desc);
 
     main_planes = overlay_planes = 0;
     for (i = 0; i < main_desc->nb_components; i++)
@@ -68,7 +87,7 @@  static int overlay_opencl_load(AVFilterContext *avctx,
         overlay_planes = FFMAX(overlay_planes,
                                overlay_desc->comp[i].plane + 1);
 
-    ctx->nb_planes = main_planes;
+    ctx->nb_color_planes = main_planar_alpha ? (main_planes - 1) : main_planes;
     ctx->x_subsample = 1 << main_desc->log2_chroma_w;
     ctx->y_subsample = 1 << main_desc->log2_chroma_h;
 
@@ -80,15 +99,30 @@  static int overlay_opencl_load(AVFilterContext *avctx,
                ctx->x_subsample, ctx->y_subsample);
     }
 
-    if (main_planes == overlay_planes) {
-        if (main_desc->nb_components == overlay_desc->nb_components)
-            kernel = "overlay_no_alpha";
-        else
-            kernel = "overlay_internal_alpha";
+    if ((main_desc->flags & AV_PIX_FMT_FLAG_RGB) !=
+        (overlay_desc->flags & AV_PIX_FMT_FLAG_RGB)) {
+        av_log(avctx, AV_LOG_ERROR, "mixed YUV/RGB input formats.\n");
+        return AVERROR(EINVAL);
+    }
+
+    if (main_desc->log2_chroma_w != overlay_desc->log2_chroma_w ||
+        main_desc->log2_chroma_h != overlay_desc->log2_chroma_h) {
+        av_log(avctx, AV_LOG_ERROR, "incompatible chroma sub-sampling.\n");
+        return AVERROR(EINVAL);
+    }
+
+    if (!overlay_alpha) {
         ctx->alpha_separate = 0;
+        kernel = "overlay_no_alpha";
     } else {
-        kernel = "overlay_external_alpha";
-        ctx->alpha_separate = 1;
+        overlay_planar_alpha = has_planar_alpha(overlay_desc);
+        if (overlay_planar_alpha) {
+            ctx->alpha_separate = 1;
+            kernel = "overlay_external_alpha";
+        } else {
+            ctx->alpha_separate = 0;
+            kernel = "overlay_internal_alpha";
+        }
     }
 
     av_log(avctx, AV_LOG_DEBUG, "Using kernel %s.\n", kernel);
@@ -155,7 +189,7 @@  static int overlay_opencl_blend(FFFrameSync *fs)
         goto fail;
     }
 
-    for (plane = 0; plane < ctx->nb_planes; plane++) {
+    for (plane = 0; plane < ctx->nb_color_planes; plane++) {
         kernel_arg = 0;
 
         mem = (cl_mem)output->data[plane];
@@ -171,7 +205,7 @@  static int overlay_opencl_blend(FFFrameSync *fs)
         kernel_arg++;
 
         if (ctx->alpha_separate) {
-            mem = (cl_mem)input_overlay->data[ctx->nb_planes];
+            mem = (cl_mem)input_overlay->data[ctx->nb_color_planes];
             CL_SET_KERNEL_ARG(ctx->kernel, kernel_arg, cl_mem, &mem);
             kernel_arg++;
         }