[FFmpeg-devel] [PATCH] DXVA2: Fix crash releasing IDirect3D9Surface's with av_buffer_default_free instead of Release

[Min]

Hi, I'm using ffmpeg dxva2 implementation (ffmpeg_dxva2.c) to render mp4
video to a DX texture.

The crash happens when closing my application, when I call to
av_frame_free() to the last frame I've created to decompress video. The
hwcontext references go to 0 and internal structs start to free its memory,
but with DXVA2FramesContext::surfaces_internal I've noticed that is an
array with IDirect3D9Surfaces's. You can see it in the file
libavutil/hwcontext_dxva2.c:169, inside function dxva2_init_pool().

    hr = IDirectXVideoAccelerationService_CreateSurface(s->service,
                                                        ctx->width,
ctx->height,

ctx->initial_pool_size - 1,
                                                        s->format,
D3DPOOL_DEFAULT, 0,

frames_hwctx->surface_type,

s->surfaces_internal, NULL);

In the same file, the function dxva2_pool_alloc() calls av_buffer_create()
with every surface created before, but the "free" function pointer
parameter is passed as NULL, and in that case av_buffer_default_free() will
be called for every surface, and that's not correct. I've created the patch
to avoid this case, and I've checked that there is no memory leaks using
debug CRT with visual studio. The surfaces are properly released in the
function dxva2_frames_uninit().

I don't know exactly how to reproduce using ffmpeg command line because
what I'm doing is integrating ffmeg in my 3d engine, but I supose that if
you decompress using dxva2 it will crash when finishing ffmpeg app, if all
the reference counting is ok and all the context and frames are freed.

Sorry if I'm wrong, but what I've seen with the visual studio debugger and
what I can see in the code makes sense for me .

Greetings
Min

2016-09-30 8:23 GMT+02:00 Hendrik Leppkes <h.leppkes at gmail.com>:

> On Fri, Sep 30, 2016 at 7:48 AM, Min <yosoymin at gmail.com> wrote:
> > diff --git a/libavutil/hwcontext_dxva2.c b/libavutil/hwcontext_dxva2.c
> > index e79254b..17d8eb5 100644
> > --- a/libavutil/hwcontext_dxva2.c
> > +++ b/libavutil/hwcontext_dxva2.c
> > @@ -101,6 +101,11 @@ static void dxva2_frames_uninit(AVHWFramesContext
> *ctx)
> >      }
> >  }
> >
> > +void dxva2_pool_free(void *opaque, uint8_t *data)
> > +{
> > +    // No need to free surfaces here, they will be Released later
> > +}
> > +
> >  static AVBufferRef *dxva2_pool_alloc(void *opaque, int size)
> >  {
> >      AVHWFramesContext      *ctx = (AVHWFramesContext*)opaque;
> > @@ -110,7 +115,7 @@ static AVBufferRef *dxva2_pool_alloc(void *opaque,
> int
> > size)
> >      if (s->nb_surfaces_used < hwctx->nb_surfaces) {
> >          s->nb_surfaces_used++;
> >          return
> > av_buffer_create((uint8_t*)s->surfaces_internal[s->nb_surfaces_used -
> 1],
> > -                                sizeof(*hwctx->surfaces), NULL, 0, 0);
> > +                                sizeof(*hwctx->surfaces),
> dxva2_pool_free,
> > 0, 0);
> >      }
> >
> >      return NULL;
>
> This is not correct and will leak memory. The buffer created here is
> not a "surface", its plain memory to hold information about a surface,
> and needs to be free'ed properly when its no longer used.
> On top of all that, DXVA2 usage through ffmpeg.c, for example, also
> doesn't crash here.
>
> So, how exactly does one reproduce the problem you're trying to fix?
>
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>