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