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

[Mark Thompson]

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