Re: [RFC v2 2/2] spice: Add an option to forward the dmabuf directly to the encoder (v2)

[Frediano Ziglio]

Il giorno lun 30 gen 2023 alle ore 02:24 Kasireddy, Vivek
<vivek.kasireddy@intel.com> ha scritto:
>
> Hi Frediano, Gerd,
>
> >
> > Il giorno mar 24 gen 2023 alle ore 06:41 Kasireddy, Vivek
> > <vivek.kasireddy@intel.com> ha scritto:
> > >
> > > + Frediano
> > >
> > > Hi Gerd,
> > >
> > > >
> > > >   Hi,
> > > >
> > > > > Here is the flow of things from the Qemu side:
> > > > > - Call gl_scanout (to update the fd) and gl_draw_async just like
> > > > >   in the local display case.
> > > >
> > > > Ok.
> > > >
> > > > > - Additionally, create an update with the cmd set to QXL_CMD_DRAW
> > > > >   to trigger the creation of a new drawable (associated with the fd)
> > > > >   by the Spice server.
> > > > > - Wait (or block) until the Encoder is done encoding the content.
> > > > > - Unblock the pipeline once the async completion cookie is received.
> > > >
> > > > Care to explain?  For qemu it should make a difference what spice-server
> > > > does with the dma-bufs passed (local display / encode video + send to
> > > > remote).
> > > [Kasireddy, Vivek] I agree that Qemu shouldn't care what the spice-server 
> > > does
> > > with the dmabuf fds but somehow a drawable has to be created in the remote
> > client
> > > case. This is needed as most of the core functions in the server 
> > > (associated with
> > > display-channel, video-stream, encoder, etc) operate on drawables. 
> > > Therefore, I
> > > figured since Qemu already tells the server to create a drawable in the 
> > > non-gl
> > case
> > > (by creating an update that includes a QXL_CMD_DRAW cmd), the same thing
> > > can be done in the gl + remote client case as well.
> > >
> >
> > This is a hack. It's combining an invalid command in order to cause
> > some side effects on spice server but it also needs a change in spice
> > server to detect and handle this hack. QXL_CMD_DRAW is mainly bound to
> > 2D commands and should come with valid bitmap data.
> >
> > > Alternatively, we could make the server create a drawable as a response to
> > gl_scanout,
> > > when it detects a remote client. IIUC, I think this can be done but seems 
> > > rather
> > messy
> > > given that currently, the server only creates a drawable (inside
> > red_process_display)
> > > in the case of QXL_CMD_DRAW sent by Qemu/applications:
> > >         switch (ext_cmd.cmd.type) {
> > >         case QXL_CMD_DRAW: {
> > >             auto red_drawable = red_drawable_new(worker->qxl, &worker-
> > >mem_slots,
> > >                                                  ext_cmd.group_id, 
> > > ext_cmd.cmd.data,
> > >                                                  ext_cmd.flags); // 
> > > returns with 1 ref
> > >
> > >             if (red_drawable) {
> > >                 display_channel_process_draw(worker->display_channel,
> > std::move(red_drawable),
> > >                                              
> > > worker->process_display_generation);
> > >             }
> > >
> >
> > Yes, it sounds a bit long but surely better than hacking Qemu, spice
> > server and defining a new hacky ABI that will be hard to maintain and
> > understand.
> >
> > > The other option I can think of is to just not deal with drawables at all 
> > > and
> > somehow
> > > directly share the dmabuf fd with the Encoder. This solution also seems 
> > > very
> > messy
> > > and invasive to me as we'd not be able to leverage the existing APIs (in 
> > > display-
> > channel,
> > > video-stream, etc) that create and manage streams efficiently.
> > >
> >
> > Yes, that currently seems pretty long as a change but possibly the
> > most clean in future, it's up to some refactory. The Encoder does not
> > either need technically a RedDrawable, Drawable but frame data encoded
> > in a format it can manage (either raw memory data or dmabuf at the
> > moment).
> [Kasireddy, Vivek] Ok, I'll work on refactoring Spice server code to pass
> the dmabuf fd directly to the Encoder without having to creating drawables.
>
> Thanks,
> Vivek
>

Hi Vivek,
   thanks for the effort. I'll try to help although I'll be pretty
busy for a while.

I'd try (consider them only as suggestions) to edit
display_channel_gl_scanout, dcc_gl_scanout_item_new,
display_channel_gl_draw, dcc_gl_draw_item_new to handle requests from
Qemu. Specifically in dcc_gl_scanout_item_new and dcc_gl_draw_item_new
you probably want to remove the check and error for Unix sockets. As
code you wrote in Qemu you will need to create a new surface (if not
already there or if size changed) handling a new scanout. Also
probably better to create a VideoStream specifically for GL surfaces.
How to have some code shortcut to deliver the GL surface directly to
the stream? No much suggestions.
It would probably be nice to add an additional method in VideoEncoder
(video-encoder.h) to accept a GL surface instead of a SpiceBitmap.

Frediano

> >
> > > >
> > > > >  #ifdef HAVE_SPICE_GL
> > > > > +        } else if (spice_dmabuf_encode) {
> > > > > +            if (g_strcmp0(preferred_codec, "gstreamer:h264")) {
> > > > > +                error_report("dmabuf-encode=on currently only works 
> > > > > and tested"
> > > > > +                             "with gstreamer:h264");
> > > > > +                exit(1);
> > > > > +            }
> > > >
> > > > IMHO we should not hard-code todays spice-server capabilities like this.
> > > > For starters this isn't true for spice-server versions which don't (yet)
> > > > have your patches.  Also the capability might depend on hardware
> > > > support.  IMHO we need some feature negotiation between qemu and spice
> > > > here.
> > > [Kasireddy, Vivek] Ok, I can get rid of this chunk in v3. However, given 
> > > the
> > numerous
> > > features supported by the Spice server, I suspect implementing feature
> > negotiation
> > > might get really challenging. Is there any other way around this that you 
> > > can
> > think of?
> > >
> > > Thanks,
> > > Vivek
> > >
> > > >
> > > > take care,
> > > >   Gerd
> > >
> >
> > Frediano