[FFmpeg-devel] [PATCH 2/4] lavfi/opencl: Handle overlay input formats correctly.
Song, Ruiling
ruiling.song at intel.com
Wed Nov 14 03:30:38 EET 2018
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of
> Mark Thompson
> Sent: Sunday, November 11, 2018 9:55 PM
> To: ffmpeg-devel at 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 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++;
> > }
> >
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list