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

[Kasireddy, Vivek]

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 

> 
> > >
> > > >  #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