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

[Kasireddy, Vivek]

+ 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.

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);
            }

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.

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