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

[Wei Gao]

2013/5/4 Stefano Sabatini <stefasab at gmail.com>

> 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?
>
The number of x row element is (2 * step_x + 1), the number of y row
element is (2 * step_y + 1), so, the matrix element number is (2 * step_x +
1) * (2 * step_y + 1), and if the size of element is sizeof(uint32_t), then
the x size is sizeof(uint32_t) * (2 * step_x + 1), the y size
is sizeof(uint32_t) * (2 * step_y + 1), the matrix size is sizeof(unit32_t)
* (2 * step_x + 1) * (2 * step_y + 1), I think it is right, but if I set
the matrix size to sizeof(unit32_t) * (2 * step_x + 1) * (2 * step_y + 1),
the program is not stable, crush some times when free buffer.

>
> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>