[vlc-devel] [PATCH 1/3] window: add has_shared_egl info flag
Alexandre Janniaux
ajanni at videolabs.io
Thu Oct 3 14:35:32 CEST 2019
Hi,
Thank you for your quite complete review, comments inline.
On Wed, Oct 02, 2019 at 11:18:47AM +0300, Rémi Denis-Courmont wrote:
> Hi,
>
> You wrote already in the previous thread that the window consumer should not have to care about such parameter.
It's a bit wider than what I wrote but I agree with this too.
>If so, it's not a question of how the flag is passed - it should not be passed, and the provider should not use a non-refcounted EGL, and/or not provide a window.
I could pass VOUT_WINDOW_TYPE_WAYLAND_EGLNOTSHARED instead but it felt a
lot more cumbersome to use than an info flag that the window consumer
can just ignore if it is not concerned. The window provider can provide
the wrong window type as well, so it was solving nothing more and put
the risk on all the window (wayland) implementation (which are multiple)
and the window consumers (opengl, wayland, vulkan) instead of putting
these issues only on users that need them (egl, window provider with
shared egl).
> It's not entirely clear how this works if a legacy GL WSI is used. On X11 it might work regardless of GLX and EGL because different Display* are allocated, but is that guaranteed? What about other windowing systems?
Indeed, the original issue stems from the fact that you must share the
same `wl_display` instance between the interface and the video. Maybe
foreign surface support would solve the issue in a later release, when
available.
However, I don't claim neither solving all GL WSI issues nor finding a
way to unify this solution for all of them. It's a far wider tasks that
need a lot more specialized skill than I can provide for now unfortunately.
This is the exact same reason why we splitted video callbacks and I would
prefer doing the same, then unify, instead of trying to half-do this on
targets that might not need it.
> It's also questionable if the provider can compute the flag at all. I don't know to what extent Qt guarantees that EGL is used but in general, LibVLC bindings or other UI framework might not provide the necessary info and/or guarantees. In other words, on second thought, this is not a boolean, but a tristate: EGL not used, EGL used, and unknown.
Nice point, but in any case your EGL GL provider can only do two things:
+ if the underlying implementation supports display_reference, or if the
window implementation doesn't use a shared display/EGLDisplay, terminate.
+ in any other case, don't terminate.
EGL unknown is basically the first case in the current implementation.
For the toolkits:
+ eglQueryDisplayAttribKHR allows tracking this attribute, as referenced
by the extension publication:
https://www.khronos.org/registry/EGL/extensions/KHR/EGL_KHR_display_reference.txt
+ eglGetCurrentDisplay allows you to get the display of the currently
binded context.
So you can determine this in a render command. If the toolkit doesn't
support this in this later case, too bad, we can't really do any
toolkit cooperation on a shared state if we can't have feedback.
For the LibVLC case, yes it's an issue and was the original reason
of the usage of vlc_variable. But as you rightfully pointed out in
#16106, this will be trivially fixed once the window provider callbacks
are available. And the former are exactly why I'm trying to provide
this patch, so as to provide a first video embedding PoC using Wayland,
which is already working.
As this issue would happen with usage that are not currently supported,
meaning mainly wayland integration in LibVLC for the short term, I don't
think this is a blocking issue.
>
> With that, there are three options, make it the responsibility of the GL provider, or the window provider, or go with a tristate. In the earlier case, it means refuse to use EGL on affected windowing systems unless reference counting is available, i.e. LibVLC playing it safe.
>
> In the second case, it means the window provider has to ensure reference counting is available, otherwise fail safe, i.e. make it someone else's, LibVLC app developers' problem (not safe IMO).
>
> In the third case, we essentially whitelist window providers with which EGL can be used without reference counting. But we don't fix the ugly entanglement between provider and consumer.
The first point will never happen. I don't even have reference counting
available on my computer, with intel drivers, and I use wayland so
I have no other alternatives. We can't just break opengl for everybody
and we need to provide video integration without relying on hacks.
The third point has been discussed above. It doesn't solve any issue
and doesn't bring additional value in my understanding.
The second point is what is done in the current version, but it's missing
the window provider callbacks to be complete, which are coming right after
the patches are merged. Yes, clients and especially LibVLC app developers
have to be aware of EGL, but I just don't see, even theoretically, how
you can cooperatively share a state with one of the side being unaware
of this. At least on Android or if you're using reference counting in
your application EGL display, you will never have issues as it is expected
to be different EGLdisplays, so it is made to improve over years instead
of being a full deprecated behaviour.
I hope I've answer all your remarks and detailled why I don't think
changing these patches, that still uses your remarks from the first
thread, is a good idea.
Regards,
--
Alexandre Janniaux
Videolabs
>
> Le 26 septembre 2019 12:04:39 GMT+03:00, Alexandre Janniaux <ajanni at videolabs.io> a écrit :
> >UI integration of the video might need shared display, for instance
> >with
> >wayland. When the display connection is shared, calling eglTerminate
> >will destroy every other context. The `has_shared_egl` flag prevents
> >this by notifying the possible EGL clients of the window that they
> >should not terminate the EGLDisplay themselves.
> >---
> > include/vlc_vout_window.h | 15 +++++++++++++++
> > src/video_output/window.c | 1 +
> > 2 files changed, 16 insertions(+)
> >
> >diff --git a/include/vlc_vout_window.h b/include/vlc_vout_window.h
> >index 9116fa65614..10cdc410e51 100644
> >--- a/include/vlc_vout_window.h
> >+++ b/include/vlc_vout_window.h
> >@@ -381,6 +381,21 @@ typedef struct vout_window_t {
> > struct {
> > bool has_double_click; /**< Whether double click events are sent,
> > or need to be emulated */
> >+ /**
> >+ * Define whether EGL is used elsewhere within the same
> >display.
> >+ *
> >+ * When this flag is set to `true`, the underlying opengl
> >providers
> >+ * won't terminate EGLDisplay, so the other clients are
> >expected to
> >+ * terminate the EGLDisplay after all VLC OpenGL providers
> >linked to
> >+ * this EGLDisplay have been closed.
> >+ *
> >+ * \remark {
> >+ * The main reason for this flags is to allow embedding a
> >video into
> >+ * a client UI which would be using EGL, whenever the window
> >display
> >+ * needs to be the same between both the UI and the video.
> >+ * In particular, it would happen with Wayland and a QML
> >client. }
> >+ * */
> >+ bool has_shared_egl;
> > } info;
> >
> > /* Private place holder for the vout_window_t module (optional)
> >diff --git a/src/video_output/window.c b/src/video_output/window.c
> >index 16ebf9ef5a2..363f5f21854 100644
> >--- a/src/video_output/window.c
> >+++ b/src/video_output/window.c
> >@@ -67,6 +67,7 @@ vout_window_t *vout_window_New(vlc_object_t *obj,
> >const char *module,
> >
> > memset(&window->handle, 0, sizeof(window->handle));
> > window->info.has_double_click = false;
> >+ window->info.has_shared_egl = false;
> > window->sys = NULL;
> > assert(owner != NULL);
> > window->owner = *owner;
> >--
> >2.23.0
> >_______________________________________________
> >vlc-devel mailing list
> >To unsubscribe or modify your subscription options:
> >https://mailman.videolan.org/listinfo/vlc-devel
>
> --
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list