[FFmpeg-devel] [PATCH v2] libavfilter: add a gblur_vulkan filter

[Wu, Jianhua]

Andreas Rheinhardt Wrote:
> Wu Jianhua:
> > +static av_cold void init_gaussian_params(GBlurVulkanContext *s) {
> > +    if (!(s->size & 1)) {
> > +        av_log(s, AV_LOG_WARNING, "kernel size should be even\n");
> > +        s->size++;
> > +    }
> > +    if (s->sigmaV <= 0)
> > +        s->sigmaV = s->sigma;
> > +
> > +    s->kernel_size = (s->size >> 1) + 1;
> > +    s->kernel = av_mallocz(sizeof(float) * s->kernel_size);
> 
> Unchecked allocation. But it seems that it is also an unneeded
> allocation: You only use this buffer with init_gaussian_kernel() and
> immediately afterwards you copy the result to a buffer provided by
> ff_vk_map_buffers(). Can't you use this buffer directly in
> init_gaussion_kernel()? (The only potential issue I see with this is
> alignment.)
> 
> > +    kernel_def = av_asprintf("float kernel[%i];", s->kernel_size);
> 
> Unchecked allocation.
> 
> > +    buf_desc.buf_content = kernel_def;
> > +
> > +    if (!sampler)
> > +        return AVERROR_EXTERNAL;
> 
> kernel_def leaks here; move this check before its allocation.
> 
> > +        s->pl_hor = ff_vk_create_pipeline(ctx);
> > +
> > +        if (!s->pl_hor)
> > +            return AVERROR(ENOMEM);
> 
> kernel_def leaks here.
> 
> > +        s->pl_ver = ff_vk_create_pipeline(ctx);
> > +        if (!s->pl_ver)
> > +            return AVERROR(ENOMEM);
> 
> kernel_def leaks here.
> 
> > +    RET(ff_vk_create_exec_ctx(ctx, &s->exec));
> > +
> > +    s->initialized = 1;
> 
> This variable is write-only.

I  am sorry I am confused with the write-only variable here. Could you elaborate it further?

> 
> > +    ff_vk_free_buf(avctx, &s->params_buf_hor);
> > +    ff_vk_free_buf(avctx, &s->params_buf_ver);
> > +    ff_vk_filter_uninit(avctx);
> 
> Is it really safe to call this if initializing failed?
> 
> > +    tmp = ff_get_video_buffer(outlink, outlink->w, outlink->h);
> > +    if (!tmp) {
> > +        err = AVERROR(ENOMEM);
> > +        goto fail;
> > +    }
> 
> Can't you allocate the tmp frame once and just keep it?

Thanks for the careful review. Very helpful. I'll fix and resubmit the patches.

Regards,
Jianhua