[FFmpeg-devel] [PATCH] lavfi/unsharp: fix opencl crushed on 64bit linux

[Stefano Sabatini]

On date Saturday 2013-05-04 08:09:48 +0800, Wei Gao encoded:
> 2013/5/4 Stefano Sabatini <stefasab at gmail.com>
> 
> > On date Friday 2013-05-03 15:50:59 +0800, Wei Gao encoded:
> > >
> >
> > Replace "crushed" with "crash" in the patch subject line.
> >
> > > From 767ac68cb1734b9320b615b2a8e112ebe7add102 Mon Sep 17 00:00:00 2001
> > > From: highgod0401 <highgod0401 at gmail.com>
> > > Date: Fri, 3 May 2013 15:46:57 +0800
> > > Subject: [PATCH] lavfi/unsharp: fix opencl crushed on 64bit linux
> > >
> > > ---
> > >  libavfilter/unsharp_opencl.c | 35 ++++++++++++++++++++---------------
> > >  1 file changed, 20 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/libavfilter/unsharp_opencl.c b/libavfilter/unsharp_opencl.c
> > > index e9a4c93..3de878e 100644
> > > --- a/libavfilter/unsharp_opencl.c
> > > +++ b/libavfilter/unsharp_opencl.c
> > > @@ -51,7 +51,7 @@ static int compute_mask(int step, uint32_t *mask)
> > >          ret = AVERROR(ENOMEM);
> > >          goto end;
> > >      }
> > > -    counter = av_mallocz(counter_size);
> > > +    counter = av_mallocz(sizeof(uint32_t *) * (2 * step + 1));
> > >      if (!counter) {
> > >          ret = AVERROR(ENOMEM);
> > >          goto end;
> >
> > This hunk seems correct (uh embarassing bug I overlooked).
> >
> > > @@ -88,13 +88,15 @@ static int compute_mask_matrix(cl_mem
> > cl_mask_matrix, int step_x, int step_y)
> > >  {
> > >      int i, j, ret = 0;
> > >      uint32_t *mask_matrix, *mask_x, *mask_y;
> > > -    size_t size_matrix = sizeof(uint32_t) * (2 * step_x + 1) * (2 *
> > step_y + 1);
> > > -    mask_x = av_mallocz(sizeof(uint32_t) * (2 * step_x + 1));
> > > +    size_t size_x = sizeof(uint32_t) * (2 * step_x + 1);
> > > +    size_t size_y = sizeof(uint32_t) * (2 * step_y + 1);
> > > +    size_t size_matrix = size_x * size_y;
> > > +    mask_x = av_mallocz(size_x);
> > >      if (!mask_x) {
> > >          ret = AVERROR(ENOMEM);
> > >          goto end;
> > >      }
> > > -    mask_y = av_mallocz(sizeof(uint32_t) * (2 * step_y + 1));
> > > +    mask_y = av_mallocz(size_y);
> > >      if (!mask_y) {
> > >          ret = AVERROR(ENOMEM);
> > >          goto end;
> > > @@ -200,21 +202,24 @@ int ff_opencl_apply_unsharp(AVFilterContext *ctx,
> > AVFrame *in, AVFrame *out)
> > >
> > >  int ff_opencl_unsharp_init(AVFilterContext *ctx)
> > >  {
> > > -    int ret = 0;
> > > +    int ret = 0, i;
> > >      UnsharpContext *unsharp = ctx->priv;
> > > +    size_t size_x[2], size_y[2];
> > > +    cl_mem *mask_matrix[2];
> > > +    mask_matrix[0] = &unsharp->opencl_ctx.cl_luma_mask;
> > > +    mask_matrix[1] = &unsharp->opencl_ctx.cl_chroma_mask;
> > > +    size_x[0] = sizeof(uint32_t) * (2 * unsharp->luma.steps_x + 1);
> > > +    size_x[1] = sizeof(uint32_t) * (2 * unsharp->chroma.steps_x + 1);
> > > +    size_y[0] = sizeof(uint32_t) * (2 * unsharp->luma.steps_y + 1);
> > > +    size_y[1] = sizeof(uint32_t) * (2 * unsharp->chroma.steps_y + 1);
> > >      ret = av_opencl_init(NULL);
> > >      if (ret < 0)
> > >          return ret;
> > > -    ret = av_opencl_buffer_create(&unsharp->opencl_ctx.cl_luma_mask,
> > > -                                  sizeof(uint32_t) * (2 *
> > unsharp->luma.steps_x + 1) * (2 * unsharp->luma.steps_y + 1),
> > > -                                  CL_MEM_READ_ONLY, NULL);
> > > -    if (ret < 0)
> > > -        return ret;
> > > -    ret = av_opencl_buffer_create(&unsharp->opencl_ctx.cl_chroma_mask,
> > > -                                  sizeof(uint32_t) * (2 *
> > unsharp->chroma.steps_x + 1) * (2 * unsharp->chroma.steps_y + 1),
> > > -                                  CL_MEM_READ_ONLY, NULL);
> > > -    if (ret < 0)
> > > -        return ret;
> > > +    for(i = 0; i < 2; i++) {
> >
> > Nit++: for_(
> >
> > Also this looks like unrelated refactoring, right? In this case it
> > would be better to put it in a separate patch.
> >

> Hi, thanks for your reviewing.There is still something I don't understand.
> if the size is   "sizeof(uint32_t) * (2 * unsharp->chroma.steps_x + 1) * (2
> * unsharp->chroma.steps_y + 1)", the program will crush too, so I tried to
> "sizeof(uint32_t) *  sizeof(uint32_t) * (2 * unsharp->chroma.steps_x + 1) *
> (2 * unsharp->chroma.steps_y + 1)", then the program is correct, so I use
> size_x * size_y as a matrix size.

Randomly increasing a buffer size is not OK if you can't understand
the reason. Where is it crashing exactly?

Also increasing a buffer size and refactoring code are two different
functional changes, and should go into two separate commits.
-- 
FFmpeg = Free and Free Merciful Peaceful Eretic Gorilla