On Fri, Feb 18, 2022 at 2:36 AM Marc-André Lureau
<marcandre.lur...@gmail.com> wrote:
>
> 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.
It doesn't matter even if a system has multiple context providers.
ui/headless-context.c may just choose the context provider according
to the flag a user provided.
>
>>
>> >
>> > 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.
That is exactly what I'm thinking of.
> 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)
I don't think QEMU has to provide anything fancier than the current
UIs. Users can combine QEMU and the external UI implementation like
virt-manager, which allows the UI to evolve in a separate way. The UIs
in QEMU would remain as references.
>
>
>>
>> >
>> >>
>> >>
>> >> >
>> >> >>
>> >> >> 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
