GNOME Bugzilla – Bug 779145
dmabuf export from vaapi encoders fails
Last modified: 2018-11-03 15:48:45 UTC
Example pipeline: v4l2src io-mode=dmabuf-import ! video/x-raw,format=YUY2 ! vaapijpegenc ! fakesink The cause here is that the vaSurface's provided by vaapijpegenc still have the derived image existing. This causes vaBeginPicture to fail with a "surface busy" error.
Created attachment 346591 [details] [review] [PATCH] vaapivideomemory: dmabuf: destroy derived imagein al in all cases The derived image will cause vaBeginPicture to fail for both upstream and downstream cases, so change the dmabuf creation to destroy the derived image in both cases. https://bugzilla.gnome.org/show_bug.cgi?id=779145 --- Note that this logic conflicts with the defined usage of buffer handles in the VA-API, which states: > Besides, no additional operation is allowed on any of the buffer > parent object until vaReleaseBufferHandle() is called. e.g. > decoding into a VA surface backed with the supplied VA buffer > object buf_id would fail with a VA_STATUS_ERROR_SURFACE_BUSY > error. Obviously the intel-vaapi-driver is not currently behaving this way, but the dmabuf allocator here should be adapted later to work once the intel-vaapi-driver begins conforming to this defined restriction. gst/vaapi/gstvaapivideomemory.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
Created attachment 347176 [details] [review] [PATCH gst-plugins-base] fdmemory: Virtually dispatch get_fd through the allocator Change get_fd to be dispatched to the mem's allocator. This allows subclasses of fdmemory to manipulate the fd throughout the memory's lifetime in case the fd needs to come and go while the backing memory itself lives.
(In reply to Scott D Phillips from comment #2) > Created attachment 347176 [details] [review] [review] > [PATCH gst-plugins-base] fdmemory: Virtually dispatch get_fd through the > allocator > > Change get_fd to be dispatched to the mem's allocator. This allows > subclasses of fdmemory to manipulate the fd throughout the > memory's lifetime in case the fd needs to come and go while the > backing memory itself lives. This is some groundwork for changing the vaapi allocator to support releasing the dmabuf fd before doing a vaBeginPicture. In this idea, the vaapi allocator would inherit from dmabuf_allocator, but provide its own gstmemory which is back by a VASurface and only holds a dmabuf fd transiently.
Review of attachment 346591 [details] [review]: I have just tested the patch with this pipeline gst-launch-1.0 v4l2src io-mode=dmabuf-import ! vaapipostproc ! vaapisink And surprisingly it works (it's an integrated camera in my laptop). Honestly, I didn't expect it. I said this, because sometime ago, Julien wrote another approach to this on commit 1dbcc8a0e199f2da6a0ab8e949f13341916128a3 and I had to revert it (commit 7472826a3633d803d55def32dee58eb8d318e3ae) because it leaked file descriptors. But with your patch there is no leakage. Said this, in my opinion, the correct patch should be aligned the Julien approach because we could avoid this gst_vaapi_buffer_proxy_release_data() call.
Review of attachment 347176 [details] [review]: lgtm @Nicolas, @Sebastian what do you thing about this new API to fd memory?
(In reply to Víctor Manuel Jáquez Leal from comment #6) > Review of attachment 347176 [details] [review] [review]: > > lgtm > > @Nicolas, @Sebastian what do you thing about this new API to fd memory? Looking at the patch again, I guess backwards compatibility would be a problem because there's no reserved space in the allocator object.
(In reply to Scott D Phillips from comment #3) > (In reply to Scott D Phillips from comment #2) > > Created attachment 347176 [details] [review] [review] [review] > > [PATCH gst-plugins-base] fdmemory: Virtually dispatch get_fd through the > > allocator > > > > Change get_fd to be dispatched to the mem's allocator. This allows > > subclasses of fdmemory to manipulate the fd throughout the > > memory's lifetime in case the fd needs to come and go while the > > backing memory itself lives. > > This is some groundwork for changing the vaapi allocator to support > releasing the dmabuf fd before doing a vaBeginPicture. In this idea, the > vaapi allocator would inherit from dmabuf_allocator, but provide its own > gstmemory which is back by a VASurface and only holds a dmabuf fd > transiently. Bad idea flag here. This will break all the caching we have implemented for zero-copy path with DMABuf.
Problem would be: VAAPI produce a dmabuf glupload create a ELGImage and cache it (qdata on the GstMemory) glimagesink renders (all fine so far) The GstBuffer goes back to VAAPI VAAPI replace the FD glupload pick the cached EGLImage glimagesink renders from the wrong FD My take, if you need to change FD, just grab another GstMemory object.
Review of attachment 347176 [details] [review]: First, virtual func should be in the class structure, I'm not sure how you manage to access "allocator" instance from the class init here. Then, indeed, that's an ABI break, and it's unfortunate, there is nothing we can do about it now. This is just not an option.
(In reply to Nicolas Dufresne (stormer) from comment #9) > Problem would be: > > VAAPI produce a dmabuf > glupload create a ELGImage and cache it (qdata on the GstMemory) > glimagesink renders (all fine so far) > The GstBuffer goes back to VAAPI > VAAPI replace the FD > glupload pick the cached EGLImage > glimagesink renders from the wrong FD > > My take, if you need to change FD, just grab another GstMemory object. Right, the way the VA-API is defined right now, this is exactly what you need to do. The API doesn't make any promises about the stability of the underlying storage of a VASurface. I think we're going to need to make some changes to the promised behavior around dmabufs in the VA-API to get it to be a good citizen where the importer doesn't need to re-import the dmabuf per-frame. This is all based on the "letter of the law" in va.h, intel-vaapi-driver for example isn't currently swapping around the underlying bo, so the dmabuf fd happens to continue referring to the same VASurface. We just need to make this explicit in the API. That will totally remove the need for swapping around the fd in an FdMemory which turns out to be quite a bad idea.
(In reply to Víctor Manuel Jáquez Leal from comment #5) > Review of attachment 346591 [details] [review] [review]: > > I have just tested the patch with this pipeline > > gst-launch-1.0 v4l2src io-mode=dmabuf-import ! vaapipostproc ! vaapisink > > And surprisingly it works (it's an integrated camera in my laptop). > Honestly, I didn't expect it. > > I said this, because sometime ago, Julien wrote another approach to this on > commit 1dbcc8a0e199f2da6a0ab8e949f13341916128a3 and I had to revert it > (commit 7472826a3633d803d55def32dee58eb8d318e3ae) because it leaked file > descriptors. But with your patch there is no leakage. > > Said this, in my opinion, the correct patch should be aligned the Julien > approach because we could avoid this gst_vaapi_buffer_proxy_release_data() > call. I think, boiling it down, what we need to do right now in all cases is: DeriveImage AcquireBufferHandle dup(fd) ReleaseBufferHandle DestroyImage There's no need to have an object tracking the lifetime of the buffer or the image because we know we always want to nuke them right away when getting a dmabuf fd. So maybe it would be better to add a method to VASurface: gint gst_vaapi_surface_get_dmabuf_fd (GstVaapiSurface * surface); that does those steps. Then the only thing missing is what I mentioned above, the promise in VA-API that the dambuf fd will refer to the VASurface's storage as long as the surface lives.
I'm surprise we need the dup() step. Can you explain ? It done at allocation time, so not to worry, but I'm wondering if there isn't a way around.
(In reply to Nicolas Dufresne (stormer) from comment #13) > I'm surprise we need the dup() step. Can you explain ? It done at allocation > time, so not to worry, but I'm wondering if there isn't a way around. Currently vaReleaseBufferHandle does a close(fd). The set of changes that I'm envisioning for VA-API for better dmabuf behavior will include moving this close() from vaReleaseBufferHandle to vaDestroySurface.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer-vaapi/issues/48.