| Message ID | 1540792619-30383-2-git-send-email-ruiling.song@intel.com |
|---|---|
| State | New |
| Headers | show |
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++; > } >
> -----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
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
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++; }
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(-)