Hi
On Thu, Feb 17, 2022 at 9:25 PM Akihiko Odaki <akihiko.od...@gmail.com>
wrote:
> On Fri, Feb 18, 2022 at 2:07 AM Marc-André Lureau
> <marcandre.lur...@gmail.com> wrote:
> >
> > Hi
> >
> > On Thu, Feb 17, 2022 at 8:39 PM Akihiko Odaki <akihiko.od...@gmail.com>
> wrote:
> >>
> >> On Fri, Feb 18, 2022 at 1:12 AM Marc-André Lureau
> >> <marcandre.lur...@gmail.com> wrote:
> >> >
> >> > Hi
> >> >
> >> > On Thu, Feb 17, 2022 at 5:09 PM Akihiko Odaki <
> akihiko.od...@gmail.com> wrote:
> >> >>
> >> >> On Thu, Feb 17, 2022 at 8:58 PM <marcandre.lur...@redhat.com> wrote:
> >> >> >
> >> >> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >> >> >
> >> >> > Hi,
> >> >> >
> >> >> > In the thread "[PATCH 0/6] ui/dbus: Share one listener for a
> console", Akihiko
> >> >> > Odaki reported a number of issues with the GL and D-Bus display.
> His series
> >> >> > propose a different design, and reverting some of my previous
> generic console
> >> >> > changes to fix those issues.
> >> >> >
> >> >> > However, as I work through the issue so far, they can be solved by
> reasonable
> >> >> > simple fixes while keeping the console changes generic (not
> specific to the
> >> >> > D-Bus display backend). I belive a shared infrastructure is more
> beneficial long
> >> >> > term than having GL-specific code in the DBus code (in particular,
> the
> >> >> > egl-headless & VNC combination could potentially use it)
> >> >> >
> >> >> > Thanks a lot Akihiko for reporting the issues proposing a
> different approach!
> >> >> > Please test this alternative series and let me know of any further
> issues. My
> >> >> > understanding is that you are mainly concerned with the Cocoa
> backend, and I
> >> >> > don't have a way to test it, so please check it. If necessary, we
> may well have
> >> >> > to revert my earlier changes and go your way, eventually.
> >> >> >
> >> >> > Marc-André Lureau (12):
> >> >> > ui/console: fix crash when using gl context with non-gl listeners
> >> >> > ui/console: fix texture leak when calling
> surface_gl_create_texture()
> >> >> > ui: do not create a surface when resizing a GL scanout
> >> >> > ui/console: move check for compatible GL context
> >> >> > ui/console: move dcl compatiblity check to a callback
> >> >> > ui/console: egl-headless is compatible with non-gl listeners
> >> >> > ui/dbus: associate the DBusDisplayConsole listener with the given
> >> >> > console
> >> >> > ui/console: move console compatibility check to
> dcl_display_console()
> >> >> > ui/shader: fix potential leak of shader on error
> >> >> > ui/shader: free associated programs
> >> >> > ui/console: add a dpy_gfx_switch callback helper
> >> >> > ui/dbus: fix texture sharing
> >> >> >
> >> >> > include/ui/console.h | 19 ++++---
> >> >> > ui/dbus.h | 3 ++
> >> >> > ui/console-gl.c | 4 ++
> >> >> > ui/console.c | 119
> ++++++++++++++++++++++++++-----------------
> >> >> > ui/dbus-console.c | 27 +++++-----
> >> >> > ui/dbus-listener.c | 11 ----
> >> >> > ui/dbus.c | 33 +++++++++++-
> >> >> > ui/egl-headless.c | 17 ++++++-
> >> >> > ui/gtk.c | 18 ++++++-
> >> >> > ui/sdl2.c | 9 +++-
> >> >> > ui/shader.c | 9 +++-
> >> >> > ui/spice-display.c | 9 +++-
> >> >> > 12 files changed, 192 insertions(+), 86 deletions(-)
> >> >> >
> >> >> > --
> >> >> > 2.34.1.428.gdcc0cd074f0c
> >> >> >
> >> >> >
> >> >>
> >> >> You missed only one thing:
> >> >> >- that console_select and register_displaychangelistener may not
> call
> >> >> > dpy_gfx_switch and call dpy_gl_scanout_texture instead. It is
> >> >> > incompatible with non-OpenGL displays
> >> >>
> >> >> displaychangelistener_display_console always has to call
> >> >> dpy_gfx_switch for non-OpenGL displays, but it still doesn't.
> >> >
> >> >
> >> > Ok, would that be what you have in mind?
> >> >
> >> > --- a/ui/console.c
> >> > +++ b/ui/console.c
> >> > @@ -1122,6 +1122,10 @@ static void
> displaychangelistener_display_console(DisplayChangeListener *dcl,
> >> > } else if (con->scanout.kind == SCANOUT_SURFACE) {
> >> > dpy_gfx_create_texture(con, con->surface);
> >> > displaychangelistener_gfx_switch(dcl, con->surface);
> >> > + } else {
> >> > + /* use the fallback surface, egl-headless keeps it updated */
> >> > + assert(con->surface);
> >> > + displaychangelistener_gfx_switch(dcl, con->surface);
> >> > }
> >>
> >> It should call displaychangelistener_gfx_switch even when e.g.
> >> con->scanout.kind == SCANOUT_TEXTURE. egl-headless renders the content
> >> to the last DisplaySurface it received while con->scanout.kind ==
> >> SCANOUT_TEXTURE.
> >
> >
> > I see, egl-headless is really not a "listener".
> >
> >>
> >> >
> >> > I wish such egl-headless specific code would be there, but we would
> need more refactoring.
> >> >
> >> > I think I would rather have a backend split for GL context, like
> "-object egl-context". egl-headless-specific copy code would be handled by
> common/util code for anything that wants a pixman surface (VNC, screen
> capture, non-GL display etc).
> >> >
> >> > This split would allow sharing the context code, and introduce other
> system specific GL initialization, such as WGL etc. Right now, I doubt the
> EGL code works on anything but Linux.
> >>
> >> Sharing the context code is unlikely to happen. Usually the toolkit
> >> (GTK, SDL, or Cocoa in my fork) knows what graphics accelerator should
> >> be used and how the context should be created for a particular window.
> >> The context sharing can be achieved only for headless displays, namely
> >> dbus, egl-headless and spice. Few people would want to use them in
> >> combination.
> >
> >
> > Ok for toolkits, they usually have their own context. But ideally, qemu
> should be "headless". And the GL contexts should be working on other
> systems than EGL Linux.
> >
> > Any of the spice, vnc, dbus display etc may legitimately be fixed to
> work with WGL etc. Doing this repeatedly on the various display backends
> would be bad design.
>
> We already have ui/egl-context.c to share the code for EGL. We can
> have ui/headless-context.c or something which creates a context for
> headless but the implementation can be anything proper there. It
> doesn't require modifying ui/console.c or adding something like
> "-object egl-context".
>
Agree, as long as you have only a single context provider per system. But
that's not my experience with GL in the past. Maybe this is true today.
> >
> > Although my idea is that display servers (spice, vnc, rdp, etc) &
> various UI (gtk, cocoa, sdl, etc) should be outside of qemu. The display
> would use IPC, based on DBus if it fits the job, or something else if
> necessary. Obviously, there is still a lot of work to do to improve surface
> & texture sharing and portability, but that should be possible...
>
> Maybe we can rework the present UIs of QEMU to make them compatible
> with both in-process communication and D-Bus inter-process
> communication. If the user has a requirement incompatible with IPC
> (e.g. OpenGL on macOS), the user can opt for in-process communication.
> D-Bus would be used otherwise. (Of course that would require
> substantial effort.)
>
That should be possible, as long the IPC is very close to the inner qemu
API, we could have an IPC-based display code turned into a shared library
instead and run in process. Although I think that would limit the kind of
UI you can expect (it would be a bare display, like qemu-display today, not
something that would bring you a full user-friendly UI, virt-manager/Boxes
kind)
> >
> >>
> >>
> >> >
> >> >>
> >> >> Anything else should be addressed with this patch series. (And it
> also
> >> >> has nice fixes for shader leaks.)
> >> >
> >> >
> >> > thanks
> >> >
> >> >>
> >> >>
> >> >> cocoa doesn't have OpenGL output and egl-headless, the cause of many
> >> >> pains addressed here, does not work on macOS so you need little
> >> >> attention. I have an out-of-tree OpenGL support for cocoa but it is
> >> >> out-of-tree anyway and I can fix it anytime.
> >> >
> >> >
> >> > Great!
> >> >
> >> > btw, I suppose you checked your DBus changes against the WIP
> "qemu-display" project. What was your experience? I don't think many people
> have tried it yet. Do you think this could be made to work on macOS? at
> least the non-dmabuf support should work, as long as Gtk4 has good macOS
> support. I don't know if dmabuf or similar exist there, any idea?
> >>
> >> I tested it on Fedora. I think it would probably work on macOS but
> >> maybe require some tweaks. IOSurface is a counterpart of DMA-BUF in
> >> macOS but its situation is bad; it must be delivered via macOS's own
> >> IPC mechanisms (Mach port and XPC), but they are for server-client
> >> model and not for P2P. fileport mechanism allows to convert Mach port
> >> to file descriptor, but it is not documented. (In reality, I think all
> >> of the major browsers, Chromium, Firefox and Safari use fileport for
> >> this purpose. Apple should really document it if they use it for their
> >> app.) It is also possible to share IOSurface with a global number, but
> >> it can be brute-forced and is insecure.
> >>
> >
> > Thanks, the Gtk developers might have some clue. They have been working
> on improving macOS support, and it can use opengl now (
> https://blogs.gnome.org/chergert/2020/12/15/gtk-4-got-a-new-macos-backend-now-with-opengl/
> ).
>
> They don't need IPC for passing textures so that is a different story.
>
Yes but they have web-engine and video decoding concerns (beside
qemu/dmabuf gtk display they should be aware of). I'll try to reach
Christian about it.
thanks
> >
> >>
> >> Regards,
> >> Akihiko Odaki
> >>
> >> >
> >> >
> >> > --
> >> > Marc-André Lureau
> >
> >
> >
> > --
> > Marc-André Lureau
>
--
Marc-André Lureau
