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

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

Details

Message ID 1540792619-30383-2-git-send-email-ruiling.song@intel.com
State New
Headers show

Commit Message

Ruiling Song Oct. 29, 2018, 5:56 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

Mark Thompson Nov. 11, 2018, 1:55 p.m.
On 29/10/18 05:56, Ruiling Song wrote:
> The main input may have alpha channel, we just ignore it.

This doesn't ignore it - it leaves it uninitialised in the output, so a YUVA or GBRAP output will never write to the A plane.  I don't think that's what you're intending.

> 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;

This name change seems wrong - it includes the luma plane, which does not contain colour information.

>      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) {

{ on new line.

> +    int nb_components;
> +    int has_alpha = !!(fmt->flags & AV_PIX_FMT_FLAG_ALPHA);
> +    if (!has_alpha) return 0;

So, if the format does not not not contain alpha?  Perhaps instead write:

if (!(fmt->flags & AV_PIX_FMT_FLAG_ALPHA))
    return 0;

> +
> +    nb_components = fmt->nb_components;
> +    // PAL8
> +    if (nb_components < 2) return 0;

Check AV_PIX_FMT_FLAG_PAL instead?

> +
> +    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++;
>          }
>
Ruiling Song Nov. 14, 2018, 1:30 a.m.
> -----Original Message-----

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

> Mark Thompson

> Sent: Sunday, November 11, 2018 9:55 PM

> To: ffmpeg-devel@ffmpeg.org

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

> formats correctly.

> 

> On 29/10/18 05:56, Ruiling Song wrote:

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

> 

> This doesn't ignore it - it leaves it uninitialised in the output, so a YUVA or GBRAP

> output will never write to the A plane.  I don't think that's what you're intending.

What I wanted to say is ignoring main input alpha channel.
The question is what the user would expect the result alpha channel contains?
I don't have a clear answer to it, so I just keep it uninitialized.
Other comments make sense. Will fix them.

Thanks!
Ruiling
> 

> > 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;

> 

> This name change seems wrong - it includes the luma plane, which does not

> contain colour information.

> 

> >      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) {

> 

> { on new line.

> 

> > +    int nb_components;

> > +    int has_alpha = !!(fmt->flags & AV_PIX_FMT_FLAG_ALPHA);

> > +    if (!has_alpha) return 0;

> 

> So, if the format does not not not contain alpha?  Perhaps instead write:

> 

> if (!(fmt->flags & AV_PIX_FMT_FLAG_ALPHA))

>     return 0;

> 

> > +

> > +    nb_components = fmt->nb_components;

> > +    // PAL8

> > +    if (nb_components < 2) return 0;

> 

> Check AV_PIX_FMT_FLAG_PAL instead?

> 

> > +

> > +    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++;

> >          }

> >

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Gyan Nov. 14, 2018, 6:26 a.m.
On Wed, Nov 14, 2018 at 7:00 AM Song, Ruiling <ruiling.song@intel.com>
wrote:

>
>
> > -----Original Message-----
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of
> > Mark Thompson
> > Sent: Sunday, November 11, 2018 9:55 PM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH 2/4] lavfi/opencl: Handle overlay
> input
> > formats correctly.
> >
> > On 29/10/18 05:56, Ruiling Song wrote:
> > > The main input may have alpha channel, we just ignore it.
> >
> > This doesn't ignore it - it leaves it uninitialised in the output, so a
> YUVA or GBRAP
> > output will never write to the A plane.  I don't think that's what
> you're intending.
> What I wanted to say is ignoring main input alpha channel.
> The question is what the user would expect the result alpha channel
> contains?
> I don't have a clear answer to it, so I just keep it uninitialized.


The s/w overlay filter does this to produce the output alpha:

    main_alpha += (1-main_alpha) * overlay_alpha

Better to keep it consistent, IMHO.

Gyan

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++;
         }