On 09/06/16 01:10, Will Kelleher wrote: > 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!
I've updated my own version with more explanation and sent here: <https://lists.libav.org/pipermail/libav-devel/2016-June/077479.html>. It also adds a warning on startup if we see another back-end driver, because it may (should?) implement different behaviour, and it would be best if the user is aware that there could be a problem. Thanks, - Mark _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
