[Bug 779145] New: dmabuf export from vaapi encoders fails

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

Bug ID: 779145
Summary: dmabuf export from vaapi encoders fails
Classification: Platform
Product: GStreamer
Version: git master
OS: Linux
Status: NEW
Severity: normal
Priority: Normal
Component: gstreamer-vaapi
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@intel.com
QA Contact: gstreamer-***@lists.freedesktop.org
CC: ***@gmail.com, ***@igalia.com
GNOME version: ---

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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

--- Comment #1 from Scott D Phillips <***@intel.com> ---
Created attachment 346591
--> https://bugzilla.gnome.org/attachment.cgi?id=346591&action=edit
[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

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(-)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

Julien Isorce <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

Hyunjun Ko <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@igalia.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

--- Comment #2 from Scott D Phillips <***@intel.com> ---
Created attachment 347176
--> https://bugzilla.gnome.org/attachment.cgi?id=347176&action=edit
[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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

--- Comment #3 from Scott D Phillips <***@intel.com> ---
(In reply to Scott D Phillips from comment #2)

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.

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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346591|none |needs-work
status| |

--- Comment #4 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 346591:
--> (https://bugzilla.gnome.org/review?bug=779145&attachment=346591)

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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346591|none |needs-work
status| |

--- Comment #4 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 346591:
--> (https://bugzilla.gnome.org/review?bug=779145&attachment=346591)

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.

--- Comment #5 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 346591:
--> (https://bugzilla.gnome.org/review?bug=779145&attachment=346591)

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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #347176|none |reviewed
status| |

--- Comment #6 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Review of attachment 347176:
--> (https://bugzilla.gnome.org/review?bug=779145&attachment=347176)

lgtm

@Nicolas, @Sebastian what do you thing about this new API to fd memory?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

--- Comment #7 from Scott D Phillips <***@intel.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #6)

Post by "GStreamer" (GNOME Bugzilla)
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@ndufresne.ca

--- Comment #8 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
(In reply to Scott D Phillips from comment #3)

Post by "GStreamer" (GNOME Bugzilla)
(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.

Bad idea flag here. This will break all the caching we have implemented for
zero-copy path with DMABuf.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

--- Comment #9 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #347176|reviewed |needs-work
status| |

--- Comment #10 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Review of attachment 347176:
--> (https://bugzilla.gnome.org/review?bug=779145&attachment=347176)

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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

--- Comment #11 from Scott D Phillips <***@intel.com> ---
(In reply to Nicolas Dufresne (stormer) from comment #9)

Post by "GStreamer" (GNOME Bugzilla)
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

--- Comment #12 from Scott D Phillips <***@intel.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #5)

Post by "GStreamer" (GNOME Bugzilla)
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

--- Comment #13 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

--- Comment #14 from Scott D Phillips <***@intel.com> ---
(In reply to Nicolas Dufresne (stormer) from comment #13)

Post by "GStreamer" (GNOME Bugzilla)
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Whiteboard| |P1

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779145

GStreamer system administrator <***@gstreamer.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |OBSOLETE

--- Comment #15 from GStreamer system administrator <***@gstreamer.net> ---
-- 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.