[FFmpeg-devel] [PATCH] vaapi_encode: explicitly free buffers after vaEndPicture

[Will Kelleher]

On 06/08, Mark Thompson wrote:
> On 08/06/16 21:02, Will Kelleher wrote:
> > On 06/08, Mark Thompson wrote:
> >> On 08/06/16 18:23, Will Kelleher wrote:
> >>> Hi all -
> >>>
> >>> I'm experiencing some significant heap growth when encoding with VAAPI on
> >>> my Ivy Bridge hardware.  Based on what I'm seeing from Massif, it seems like
> >>> the parameter buffers allocated in vaapi_encode_make_param_buffer are leaking.
> >>>
> >>> I came across this thread [1] that indicates that vaEndPicture might not be
> >>> freeing the param buffers like the libva documentation says it should.
> >>>
> >>> I also noticed that VLC [2] seems to explicitly call vaDestroyBuffer on the
> >>> param buffers after vaEndPicture.
> >>>
> >>> When I try that, the leak is gone.
> >>
> >> Yes, I wrote essentially the same code on observing the same issue.
> >>
> >> Unfortunately, you need a lot more machinery to do this safely - the change makes all buffer operations thread-unsafe (another thread could allocate a buffer with the ID you are about to try to destroy).  That results in needing VADisplay-global locks around pretty much everything to do with buffers (including any time the user makes use of them).
> >>
> >> I don't much like the idea of writing all the code to have locking everywhere (including in all user code talking to libavcodec), so I took the cowardly approach of doing nothing and hiding behind the documentation :/
> >>
> >> Therefore, dunno.  Maybe we should talk to the libva people about it?
> >>
> >> - Mark
> >>
> > Hmm, good point.
> > 
> > I wonder if this is why libmfx seems to open separate va displays for each encode/decode session.
> 
> libmfx on Linux isn't really a wrapper over VAAPI, it has an entirely separate proprietary driver underneath which does who knows what.
> 

That's not quite true.  It certainly has some special sauce inside, but it seems to use libva for some of it (although it is a special? fork).

When you run a libmfx encode you should see the libva init prints, like this:

libva info: VA-API version 0.35.0
libva info: va_getDriverName() returns 0
libva info: Trying to open /usr/lib/dri/i965_drv_video.so
libva info: Found init function __vaDriverInit_0_32
libva info: va_openDriver() returns 0

And I see one of those for each ffmpeg encoding that I start, unlike the vaapi encoder, which only opens libva once.

> > Unfortunately unless we can solve this, the encoder is pretty useless for any long-running encodes.
> > 
> > Talking to the libva people might help, but any fix that requires modifications to libva/libdrm is
> > less than ideal because it would require people to likely build those components from source to get
> > a recent enough version for this to work.
> 
> Yeah.  Maybe pragmatic concerns with what we have now should win here - the answer is that we obviously should do this if only the i965 driver is supported (which is mostly true already, though I've tried to not to embed it by making large assumptions like this one).
> 
> > That said... how sure are you about these thread safety concerns?  Did you witness any instability
> > when you tested your vaDestroyBuffer change?  I've been running an encode of 12 streams with this
> > patch for 17+ hours now without any issues.  I would have crashed due to OOM by now without this.
> 
> I didn't, but I never did particularly serious testing with the change (I do not currently have a use case for leaving it encoding something indefinitely).
> 
> The concern is more that any other driver someone tries to use will fall over due to this, not that the i965 one will (given the current setup, I think we believe it won't).
> 
> 
> Ok, I think I'm convinced.  We need a long comment next to it describing all of this problem, though.
> 
> Something like:
> 
>     // We free all of the parameter buffers here.
>     // This is not consistent with what VAAPI states is required; under
>     // vaDestroyBuffer, it says: "Only call this if the buffer is not
>     // going to be passed to vaRenderBuffer" [sic; vaRenderPicture].
>     // Doing this here is a pragmatic decision to preferentially support
>     // the Intel i965 back-end driver, which does not and has never
>     // freed the buffers as required by the specification - if we don't
>     // free here, memory will leak for every frame submitted when using
>     // that driver.
>     // If other drivers are supported in future, this decision may need
>     // to be revisted - if the driver really has already freed the
>     // buffer, doing so here is disaster for thread-safety because we
>     // may free buffers which have been allocated in other threads.
> 
> ?

That works for me.  I can update the patch with that warning.  Thanks!