[vlc-devel] [PATCH 1/3] window: add has_shared_egl info flag

[Alexandre Janniaux]

Hi,

On Thu, Oct 03, 2019 at 08:39:27PM +0300, Rémi Denis-Courmont wrote:
> Le torstaina 3. lokakuuta 2019, 15.35.32 EEST Alexandre Janniaux a écrit :
> > > 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.
>
> I'm not convinced that we can assume that EGL matches displays by pointer
> values. That looks like an unguaranteed implementation detail.
>

If you're not convinced, I invite you to read the documentation of
eglGetPlatformDisplay.

>  eglGetPlatformDisplay obtains an EGL display connection for the specified
> platform and native_display.

If you fail on matching native_display with a pointer type, I invite you to
read the related platform documentations. The point giving the same return
object is:

>  Multiple calls made to eglGetPlatformDisplay with the same parameters will
> return the same EGLDisplay handle.

It doesn't clearly tell "multiple call made to eglGetPlaformDisplay with
different parameters gives different values" but as explained after, it would be
very unconventional and borderline to have the same EGLDisplay for different
display connection. You can also check eglGetDisplay documentation which
cryptically says:

> As for eglGetPlatformDisplay, EGL considers the returned EGLDisplay as
> belonging to the same platform as display_id. However, the set of platforms to
> which display_id is permitted to belong, as well as the actual type of
> display_id, are implementation-specific.

The display connection represents the resource allocator. That is what a display
connection is for a client. The fact that X11 is transparent and wayland doesn't
define behaviour consistency between clients, and that an EGLDisplay binds on a
native window display connection litterally means that if your arguments are
different, you cannot easily ensure whether they points to the same resource or
not without relying painful hacks, and nobody in the open source world is
writing painful hacks just for the sake of breaking an API for a production
software.

Some custom platforms might do this kind of atrocity but honestly, I don't care
about supporting edge case. If you want to, please do so but it's out of the
scope of this patch. This is the current expected behaviour for X11 and Wayland.

> > > 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.
>
> + fail safe

I don't get what you mean by fail safe. Do you mean leaking memory in the
default case ? I won't agree with this, as the default terminate behaviour would
only fail MakeCurrent and not imply undefined behaviour.

> > EGL unknown is basically the first case in the current implementation.
>
> No. The current implementation assumes that the EGL (reference) is *known* not
> to be shared, i.e. it is not shared and/or it is reference-counted.

What do you expect for the unknown state ? leaking memory in the situation that
the client doesn't use EGL ?

> > 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_referenc
> > e.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.
>
> I don't follow how that determines whether Qt uses EGL or not, TBH.
>
> If creating a reference-counted EGL fails, then we still need to know if EGL
> is shared. In other words, we still need to know if it is used by the window
> provider (e.g. Qt GUI), by another EGL plugin instance for a different surface
> of the same display (e.g. other video track), or whatever else.

You can get the context, query the display, and check whether it is reference
counted. There are litteraly one function for each of those action, and a
failure at any level means that Qt is not using EGL at all. A negative result on
the last action means it is using an EGL implementation which is not using the
tracking attribute. Then, as explained quite clearly in the display_reference
document from Khronos, it depends whether you're on Android or not.

>
> > 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.
>
> I never pointed that out and, as a matter of facts, I don't know how it helps.
>
> I just pointed out that I am waiting for the player window allocation callback
> before I start writing the Wayland embedded window provider and its LibVLC
> frontend. And that's really only because I don't want to write throw-away code
> in the mean time.

Well, that's exactly the same issue, you have to provide the same resource
allocator, so you can provide this kind of information too.

> > 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.
>
> Well... does this not affect multiple non-embedded video tracks too? That's
> already supposed to be supported because I'd always assumed that EGL was
> reference-counted. But now it seems that that was wrong.

It's not, because of how eglGetPlatformDisplay works. You have one display
connection per output (=per vout thread).

Pierre already pinpointed the fact that EGL wasn't reference-counted in the
first Qt refactor review, because he needed it for running an opengl integration
of the video into Qt.

I also have other cases in mind which will actually need this reference
counting, and wrote the needed addition to add internal refcounting for EGL
vlc_gl provider, but as there is currently no need for this, it's sleeping on a
branch.

> > > 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.
>
> That third option does not make sense if there are multiple video tracks, so
> I'll pretend I never iuntroduced it from now on.

As above, eglGetPlatformDisplay doesn't work like this and we currently don't
support harmful use cases.

>
> > 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.
>
> Unfortunately, the second option has the exact same limitation. It's just that
> the dependency on reference counting is checked in a different, and IMO less
> adequate, component: the window provider instead of the GL context provider.
>
> As you pointed out earlier on, the window provider might not know anything
> about EGL. Indeed the XDG shell SSD window provider does not.

As above, eglGetPlatformDisplay doesn't work like this and XDG-shell doesn't use
EGL so this makes no sense.

I hope I've cleared your last doubts and that we can finally reach a
constructive review now that I pinpointed some of the shadow you had on EGL.
If you still have some, please highlight them out of context, and I'll try to
make sure I can gather all the elements to build a correct understanding of it
somewhere for the mailing list before we can continue the discussion.
I would appreciate that as answering assumings is quite time consuming, my free
time is limited, and as you rightfully pointed out, we are missing review time.

Regards,
--
Alexandre Janniaux
Videolabs