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

}

     return NULL;

On Fri, Sep 30, 2016 at 7:48 AM, Min <yosoymin@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

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@gmail.com>:

> On Fri, Sep 30, 2016 at 7:48 AM, Min <yosoymin@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Please don't top post on the mailing list.

On Tue, Oct 4, 2016 at 8:20 AM, Min <yosoymin@gmail.com> wrote:
> 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().

The surfaces are always released, this is about memory, not surfaces.
It sounds to me like your copy of the implementation here is just
flawed, and you need to increase the refcount of the pool somewhere.

The hwcontext implementation seems fine. The free is required or it'll
leak memory.

>
> 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.
>

the ffmpeg app does not crash when you use dxva2 decoding.

- Hendrik