[PATCH v6 0/5] dcc: Create a stream for non-gl/remote clients that want to use dmabuf (v6)

[Kasireddy, Vivek]

Hi Frediano,

> >
> > For clients that cannot accept a dmabuf fd directly (such as those
> > running on a remote system), this patch series provides a way for
> > the Spice server to stream the gl/dmabuf data/buffer instead. This
> > is mostly done by enabling the creation of Gst memory using a dmabuf
> > fd as the source. This ability is useful given that dmabuf is the
> > standard mechanism for sharing buffers between various drivers and
> > userspace in many Graphics and Media usecases. Currently, this is
> > tested with Qemu and remote-viewer using the x264enc/avdec_h264
> > and msdkh264enc/dec plugins to stream the Guest/VM desktop but it
> > can be easily extended to other plugins and applications.
> >
> > Here is roughly how things work:
> > - The application (e.g, Qemu) chooses its preferred codec (a Gstreamer
> >   one) and calls gl_scanout (to update the fd) followed by gl_draw.
> > - In response, the Spice server checks to see if the client is capable
> >   of accepting a dmabuf fd directly or not. If yes, the fd is forwarded
> >   directly to the client; otherwise, a new stream is created.
> > - The Spice server then sends the dmabuf fd to the Gstreamer encoder
> >   which uses it as an input for creating an encoded buffer which is then
> >   sent to the client.
> > - Once the encoding process is done, an async completion cookie is sent
> >   to the application.
> >
> > Here is a link to the previous version that used a drawable to share
> > the dmabuf fd with the Gstreamer encoder:
> > https://lists.freedesktop.org/archives/spice-devel/2023-
> January/052948.html
> >
> > This version is tested together with following (required) patches in qemu:
> > https://gitlab.freedesktop.org/Vivek/qemu/-/commits/spice_gl_on_v2
> >
> > Changelog:
> >
> > v6: (Frediano)
> > - Properly share ownership of the dmabuf fd between stream and scanout
> > - Ensure that a newly created stream is associated with all connected clients
> >
> > v5:
> > - Addressed review comments from Frediano mainly regarding adding
> autoconf
> >   support for gstreamer-allocators dependency and not needing to access
> >   scanout as part of gl draw operation
> >
> > v4:
> > - Test with Virgl enabled
> > - Associate dmabuf's y0_top with stream's top_down variable
> >
> > v3:
> > - Updated the second patch to have a new primary surface created
> >   whenever a new stream gets created. This will avoid having to
> >   trigger primary surface creation from Qemu. And, this change
> >   also fixes the following error seen with v2:
> >   ../server/display-channel.cpp:2074:display_channel_create_surface:
> >   condition `!display->priv->surfaces[surface_id]' failed
> > - Rebase all patches on master
> >
> > v2:
> > - Update all patches to address review comments from Frediano
> > - Tested this series with msdkh264enc/dec plugins that will be added
> >   in another patch series
> >
> > Cc: Frediano Ziglio <freddy77 at gmail.com>
> > Cc: Gerd Hoffmann <kraxel at redhat.com>
> > Cc: Marc-André Lureau <marcandre.lureau at redhat.com>
> > Cc: Dongwon Kim <dongwon.kim at intel.com>
> >
> >
> > Vivek Kasireddy (5):
> >   dcc: Check to see if the client supports multiple codecs (v2)
> >   dcc: Create a stream associated with gl_draw for non-gl clients (v5)
> >   dcc-send: Encode and send gl_draw stream data to the remote client
> >     (v3)
> >   gstreamer-encoder: Add an encoder function that takes dmabuf fd as
> >     input (v3)
> >   video-stream: Don't stop a stream associated with gl_draw (v2)
> >
> >  configure.ac                     |   2 +-
> >  meson.build                      |   2 +-
> >  server/dcc-send.cpp              | 150 ++++++++++++++++++-----
> >  server/dcc.cpp                   |  36 ++++--
> >  server/dcc.h                     |   6 +
> >  server/display-channel-private.h |   1 +
> >  server/display-channel.cpp       |   1 +
> >  server/gstreamer-encoder.c       | 165 ++++++++++++++++++++-----
> >  server/video-encoder.h           |  13 ++
> >  server/video-stream.cpp          | 202 ++++++++++++++++++++++++++-----
> >  server/video-stream.h            |   5 +
> >  11 files changed, 479 insertions(+), 104 deletions(-)
> >
> 
> Hi,
>    I finally managed to try the series. Did it work on your side?
It didn't work in all cases.

> Looks like this series is still WIP/EXP/RFC quality, at this time I
> supposed it should be converging to a working state.
> By the way, it blinks a lot during boot, then when the machine
> switches to the final desktop it gets blank. At that point even if you
> close and reopen the client you get a blank screen.
Yeah, these are some of the cases where it does not seem to work.
This is mostly because the idea of sharing fd (dmabuf) ownership between
scanout and stream fails to work in a few corner cases. These can be fixed
but it requires non-trivial amount of code changes from what I can see.

I think the previous design (v4) still looks like the most reasonable thing
to do, where the fd ownership is limited only to the scanout. However, the
main concern you have expressed with this idea is that it requires us to look
at the params associated with scanout such as flags, stride, etc as part of
gl_draw, particularly when creating a new stream. I don't see any problem
with this as this is different from the local display use-case where the fd is
shared with the client and gl_draw can be called subsequently. 

Please let me know if there are any better ideas for handling this such as
stashing away these scanout params somewhere safe, etc. 

Thanks,
Vivek

> 
> Regards,
>    Frediano