GNOME Bugzilla – Bug 766704
vaapi: sharing display handle between app and vaapi elements
Last modified: 2017-07-26 13:34:27 UTC
This is to discuss how we can effectively share the display handle (X11, Wayland etc) created by the app with vaapi element. The GST_MESSAGE_NEED_CONTEXT sending by the vaapi plugin expect a GstVaapiDisplay from app if it want to share the display handle and we don't have any mechanism to do it. May be do something like gst-plugins-bad/gst-libs/gst/wayalnd ?? But unlike the older gstreamer-vaapi, we are not having any public APIs now.
Note that the wayland lib is also not public (headers are not installed).
(In reply to sreerenj from comment #0) > May be do something like gst-plugins-bad/gst-libs/gst/wayalnd ?? bad/gst-libs/gst/gl :) > But unlike the older gstreamer-vaapi, we are not having any public APIs now. Yup. At least we should expose GstVaapiDisplay. But furthermore, we should turn it into a GstObject derived class, hence we could have gobject-introspection for free.
(In reply to Tim-Philipp Müller from comment #1) > Note that the wayland lib is also not public (headers are not installed). Hm, then what is the relevance of those APIs ?
(In reply to Víctor Manuel Jáquez Leal from comment #2) > (In reply to sreerenj from comment #0) > > > May be do something like gst-plugins-bad/gst-libs/gst/wayalnd ?? > > bad/gst-libs/gst/gl :) > > > But unlike the older gstreamer-vaapi, we are not having any public APIs now. > > Yup. At least we should expose GstVaapiDisplay. But furthermore, we should > turn it into a GstObject derived class, hence we could have > gobject-introspection for free. I think we have an easy solution: vaapi elements should behave like gl elements :) If app using wayland , it set "GstWaylandDisplayHandleContextType" with wl_display, if app using x11, it set "gst.x11.display.handle" with Display etc. And just create GstVaapDisplay inside gstreamer-vaapi, so no more new APIS. Missing something?
(In reply to sreerenj from comment #4) > (In reply to Víctor Manuel Jáquez Leal from comment #2) > > Yup. At least we should expose GstVaapiDisplay. But furthermore, we should > > turn it into a GstObject derived class, hence we could have > > gobject-introspection for free. > > I think we have an easy solution: vaapi elements should behave like gl > elements :) > > If app using wayland , it set "GstWaylandDisplayHandleContextType" with > wl_display, > if app using x11, it set "gst.x11.display.handle" with Display etc. > > And just create GstVaapDisplay inside gstreamer-vaapi, so no more new APIS. > Missing something? That's what we almost have. The missing bits would be the negotiate external context (bug 766206 and bug 705821). But it won't fix, for example, bug 754820, because the application would like to have different GstVaapiDisplay in same pipeline.
(In reply to Víctor Manuel Jáquez Leal from comment #5) > (In reply to sreerenj from comment #4) > > (In reply to Víctor Manuel Jáquez Leal from comment #2) > > > Yup. At least we should expose GstVaapiDisplay. But furthermore, we should > > > turn it into a GstObject derived class, hence we could have > > > gobject-introspection for free. > > > > I think we have an easy solution: vaapi elements should behave like gl > > elements :) > > > > If app using wayland , it set "GstWaylandDisplayHandleContextType" with > > wl_display, > > if app using x11, it set "gst.x11.display.handle" with Display etc. > > > > And just create GstVaapDisplay inside gstreamer-vaapi, so no more new APIS. > > Missing something? > > That's what we almost have. The missing bits would be the negotiate > external context (bug 766206 and bug 705821). But it won't fix, for example, > bug 754820, because the application would like to have different > GstVaapiDisplay in same pipeline. Aha all the display cache stuffs back again!
GstContext is designed so you can have a different context for different bins inside your pipeline. I believe it might be useful to expose a VAAPI specific context, for APP doing VAAPI stuff externally (if that exist of course).
Created attachment 331861 [details] [review] WIP: vaapidisplay: make vaapidisplay api public I propose a patch to make vaapidisplay public.(based on patches in bug 768266) Note that this is WIP and want to hear opinions There are some issues, which need to be discussed. 1. library's name Currently, libgstvaapi.so for vaapi plugins. libgstvaapi.a for internal libraries. We should make shared library to be public, which means we should name it. In this patch, I use same name as vaapi plugins, but this should be changed. I suggest that vaapi plugins : libgstvaapielements.so vaapi shared library : libgstvaapi.so.(ver) vaapi internal library : libgstvaapi.a, same as now 2. Move to some apis in gstvaapivideocontext I move 2 apis to vaapidisplay - gst_vaapi_video_context_set_display - gst_vaapi_video_context_get_display 3. Exposed headers - gstvaapidisplay.h, gstvaapidisplay_x11.h gstvaapidisplay_wayland.h 4.Since bulid issue, I remove including libgstvaapi_priv_check.h, is this OK? 5. Need to work more about - make gir, typelib, pkgconfig, doc.
Why do we need a library with public API? Could you describe the flow, how an app would use the API?
(In reply to Tim-Philipp Müller from comment #9) > Why do we need a library with public API? Could you describe the flow, how > an app would use the API? Let me chime in. The general idea is the application will create its own GstVaapiDisplay (or multiple ones) and shared into the pipeline through the GST_MESSAGE_NEED_CONTEXT message mechanism. So we need to export an API to the user to create a GstVaapiDisplay. As far as I understand, let to user to share only the VADisplay, by using only libva library, is not enough because gstreamer-vaapi needs more context.
(In reply to Víctor Manuel Jáquez Leal from comment #10) > As far as I understand, let to user to share only the VADisplay, by using > only libva library, is not enough because gstreamer-vaapi needs more context. But, it is a good idea to confirm it first.
Created attachment 350101 [details] [review] videocontext: handle native display provided If a vaapi element is sink, it queries for native display(x11/wayland) and send need-context message if not found. In this way, vaapisink could handle native display if provided and create GstVaapiDisplay with it. And the GstVaapiDisplay should be shared via current GstContext sharing mechanism.
Created attachment 350102 [details] [review] tests: elements: add testsuite of vaapicontext
I've been working for a while on how to provide a way to share display handle especially for user. I'd been looking into using GstContext only first. As my patches, it looks working fine in normal pipeline. But I think there are still problems and we can't remove current internal display cache mechanism. Important thing is that (dec-vpp-vaapisink) has to use same VADisplay to render properly, otherwise render fails due to invalid VASurface. But as far as I see, we can't assure it in some cases, especially using multiple decodebin in one pipeline eg. the case that a pipeline has 2 (decodebin-vaapisink) pipelines just like gst-launch-1.0 filesrc ! tee name=t ! decodebin ! vaapisink t. ! decodebin ! vaapisink. Or gst-launch-1.0 filesrc ! decodebin ! vaapisink filesrc ! decodebin ! vaapisink In these cases, we can't make sure that 1st decoder pipeline shares one, and 2nd decoder pipeline shares another. And mostly fails when trying to render. So to use this way (in my patch), we shouldn't allow that kindof pipeline(just like testsuite attached) or should provide a sort of playbin including vaapidec + vpp + vaapisink.
And I believe it can be happend also in case of exposing api.
Review of attachment 331861 [details] [review]: this patch doesn't apply cleanly
(In reply to Víctor Manuel Jáquez Leal from comment #16) > Review of attachment 331861 [details] [review] [review]: > > this patch doesn't apply cleanly That is WIP patch I have uploaded 9 months ago.:) I'm going to obsolete it.
Perhaps we could handle a new context, just like "gst.x11.display.handle", but named, for example, "gst.va.display.handle" which will receive a VADisplay (not a GstVaapiDisplay object). Thus clients could create by themselves VA displays and assign them to a pipeline via GstContext.
Created attachment 354989 [details] [review] libs: display: pass GstVaapiDisplayInfo when creating with VADisplay If we want to create GstVaapiDisplay with foreign VADisplay, and render with the created GstVaapiDisplay, it also requires native display of the backend.
Created attachment 354990 [details] [review] libs: display: x11: implements gst_vaapi_display_x11_new_with_va_display Implements new api so that users could create GstVaapiDisplay with their own VADisplay and native display of the backend.
Created attachment 354991 [details] [review] videocontext: support "gst.vaapi.app.Display" context Through "gst.vaapi.app.Display" context, users can set their own VADisplay and native display of their backend. Attributes: - display : pointer of VADisplay - x11-display : pointer of X11 display (Display *), if they're using. This patch creates GstVaapidisplayX11 if information provided through "gst.vaapi.app.Display"
Created attachment 354992 [details] [review] vaapisink: remove replacing GstVaapiDisplay during rendering Replacing GstVaapiDisplay during rendering might be hiding problems at some cases, even though it's safe currently since we use cache of GstVaapidisplay. And this also means if we remove cache of GstVaapiDisplay this code might cause a problem.
Created attachment 354993 [details] [review] libs: display: remove GstVaapiDisplay cache Under current cache mechanism, we could share GstVaapiDisplay safely. But since GstContext sharing works fine, we can remove this. In addition, the cache prevented from using multiple GstVaapiDisplay. This has been killing performance due to lock of same display in vaapisink if you run multiple pipelines including vaapisink. So now, we remove GstVaapiDisplay cache.
Created attachment 354994 [details] [review] tests: elements: add testsuite of vaapi context
Created attachment 356417 [details] [review] libs: display: pass display info when foreign display When creating a GstVaapiDisplay using a foreign VADisplay, and render with that display, it also requires native display of the backend.
Created attachment 356418 [details] [review] libs: display: x11: add gst_vaapi_display_x11_new_with_va_display() Implements new API function so that users could create GstVaapiDisplay with their own VADisplay within a native display as backend.
Created attachment 356419 [details] [review] videocontext: support "gst.vaapi.app.Display" context Through "gst.vaapi.app.Display" context, users can set their own VADisplay and native display of their backend. Attributes: - display : pointer of VADisplay - x11-display : pointer of X11 display (Display *), if they're using. This patch creates GstVaapidisplayX11 if information provided through "gst.vaapi.app.Display"
Created attachment 356420 [details] [review] vaapisink: fail if surface display is different Replacing GstVaapiDisplay during rendering might be hiding problems at some cases, even though it's safe currently since we use cache of GstVaapidisplay. Play safe by failing if this happens.
Created attachment 356421 [details] [review] tests: elements: add testsuite of vaapi context Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
(In reply to Víctor Manuel Jáquez Leal from comment #27) > Created attachment 356419 [details] [review] [review] > videocontext: support "gst.vaapi.app.Display" context > > Through "gst.vaapi.app.Display" context, users can set their own VADisplay > and native display of their backend. > > Attributes: > - display : pointer of VADisplay > - x11-display : pointer of X11 display (Display *), if they're using. > > This patch creates GstVaapidisplayX11 if information provided through > "gst.vaapi.app.Display" I'm not sure if gst.vaapi.app.Display is a good name for this context. But I can't think in another.
Attachment 356417 [details] pushed as 3562122 - libs: display: pass display info when foreign display Attachment 356418 [details] pushed as d78e094 - libs: display: x11: add gst_vaapi_display_x11_new_with_va_display() Attachment 356419 [details] pushed as b8265db - videocontext: support "gst.vaapi.app.Display" context Attachment 356420 [details] pushed as 736478d - vaapisink: fail if surface display is different Attachment 356421 [details] pushed as 85856c2 - tests: elements: add testsuite of vaapi context