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

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

[Vivek Kasireddy]

This patch series adds options to select a preferred codec and also
to forward a dmabuf directly to the encoder module that is part of
the Spice server. Currently, gstreamer:h264 is the only combination
tested but additional work is ongoing to test other combinations. 

Tested with: -device virtio-gpu-pci,max_outputs=1,blob=true,xres=1920,yres=1080
             -spice port=3001,gl=on,disable-ticketing=on,dmabuf-encode=on,
              preferred-codec=gstreamer:h264

and remote-viewer --spice-debug spice://x.x.x.x:3001 on the client side.

Associated Spice server patches (v1) can be found here:
https://lists.freedesktop.org/archives/spice-devel/2023-January/052927.html

v2:
- Used the already available gl_scanout and gl_draw_async APIs instead
  of adding new ones to Spice.
- Improved the commit message of the second patch

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>

Vivek Kasireddy (2):
  spice: Add an option for users to provide a preferred codec
  spice: Add an option to forward the dmabuf directly to the encoder
    (v2)

 include/ui/spice-display.h |  1 +
 qemu-options.hx            | 11 +++++-
 ui/spice-core.c            | 36 ++++++++++++++++--
 ui/spice-display.c         | 75 ++++++++++++++++++++++++++++----------
 4 files changed, 100 insertions(+), 23 deletions(-)

-- 
2.37.2

[RFC v2 1/2] spice: Add an option for users to provide a preferred codec

[Vivek Kasireddy]

Giving users an option to choose a particular codec will enable
them to make an appropriate decision based on their hardware and
use-case.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 qemu-options.hx |  5 +++++
 ui/spice-core.c | 14 ++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 3aa3a2f5a3..aab8df0922 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2142,6 +2142,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
     "       [,streaming-video=[off|all|filter]][,disable-copy-paste=on|off]\n"
     "       [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
     "       [,playback-compression=[on|off]][,seamless-migration=[on|off]]\n"
+    "       [,preferred-codec=<encoder>:<codec>\n"
     "       [,gl=[on|off]][,rendernode=<file>]\n"
     "   enable spice\n"
     "   at least one of {port, tls-port} is mandatory\n",
@@ -2237,6 +2238,10 @@ SRST
     ``seamless-migration=[on|off]``
         Enable/disable spice seamless migration. Default is off.
 
+    ``preferred-codec=<encoder>:<codec>``
+        Provide the preferred codec the Spice server should use.
+        Default would be spice:mjpeg.
+
     ``gl=[on|off]``
         Enable/disable OpenGL context. Default is off.
 
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 72f8f1681c..6e00211e3a 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -469,6 +469,9 @@ static QemuOptsList qemu_spice_opts = {
         },{
             .name = "streaming-video",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "preferred-codec",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "agent-mouse",
             .type = QEMU_OPT_BOOL,
@@ -644,6 +647,7 @@ static void qemu_spice_init(void)
     char *x509_key_file = NULL,
         *x509_cert_file = NULL,
         *x509_cacert_file = NULL;
+    const char *preferred_codec = NULL;
     int port, tls_port, addr_flags;
     spice_image_compression_t compression;
     spice_wan_compression_t wan_compr;
@@ -795,6 +799,16 @@ static void qemu_spice_init(void)
         spice_server_set_streaming_video(spice_server, SPICE_STREAM_VIDEO_OFF);
     }
 
+    preferred_codec = qemu_opt_get(opts, "preferred-codec");
+    if (preferred_codec) {
+        if (spice_server_set_video_codecs(spice_server, preferred_codec)) {
+            error_report("Preferred codec name is not valid");
+            exit(1);
+        }
+    } else {
+        spice_server_set_video_codecs(spice_server, "spice:mjpeg");
+    }
+
     spice_server_set_agent_mouse
         (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
     spice_server_set_playback_compression
-- 
2.37.2

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

[Vivek Kasireddy]

This patch adds support for gl=on and port != 0. In other words,
with this option enabled, it should be possible to stream the
content associated with the dmabuf to a remote client.

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

v2:
- Use the existing gl_scanout and gl_draw_async APIs instead of
  adding new ones.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/ui/spice-display.h |  1 +
 qemu-options.hx            |  6 ++-
 ui/spice-core.c            | 22 +++++++++--
 ui/spice-display.c         | 75 ++++++++++++++++++++++++++++----------
 4 files changed, 81 insertions(+), 23 deletions(-)

diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
index e271e011da..df74f5ee9b 100644
--- a/include/ui/spice-display.h
+++ b/include/ui/spice-display.h
@@ -153,6 +153,7 @@ struct SimpleSpiceCursor {
 };
 
 extern bool spice_opengl;
+extern bool spice_dmabuf_encode;
 
 int qemu_spice_rect_is_empty(const QXLRect* r);
 void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
diff --git a/qemu-options.hx b/qemu-options.hx
index aab8df0922..3016f8a6f7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2143,7 +2143,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
     "       [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
     "       [,playback-compression=[on|off]][,seamless-migration=[on|off]]\n"
     "       [,preferred-codec=<encoder>:<codec>\n"
-    "       [,gl=[on|off]][,rendernode=<file>]\n"
+    "       [,gl=[on|off]][,rendernode=<file>][,dmabuf-encode=[on|off]]\n"
     "   enable spice\n"
     "   at least one of {port, tls-port} is mandatory\n",
     QEMU_ARCH_ALL)
@@ -2248,6 +2248,10 @@ SRST
     ``rendernode=<file>``
         DRM render node for OpenGL rendering. If not specified, it will
         pick the first available. (Since 2.9)
+
+    ``dmabuf-encode=[on|off]``
+        Forward the dmabuf directly to the encoder (Gstreamer).
+        Default is off.
 ERST
 
 DEF("portrait", 0, QEMU_OPTION_portrait,
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 6e00211e3a..c9b856b056 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -494,6 +494,9 @@ static QemuOptsList qemu_spice_opts = {
         },{
             .name = "rendernode",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "dmabuf-encode",
+            .type = QEMU_OPT_BOOL,
 #endif
         },
         { /* end of list */ }
@@ -843,11 +846,24 @@ static void qemu_spice_init(void)
     g_free(password);
 
 #ifdef HAVE_SPICE_GL
+    if (qemu_opt_get_bool(opts, "dmabuf-encode", 0)) {
+        spice_dmabuf_encode = 1;
+    }
     if (qemu_opt_get_bool(opts, "gl", 0)) {
-        if ((port != 0) || (tls_port != 0)) {
-            error_report("SPICE GL support is local-only for now and "
-                         "incompatible with -spice port/tls-port");
+        if (((port != 0) || (tls_port != 0)) && !spice_dmabuf_encode) {
+            error_report("Add dmabuf-encode=on option to enable GL streaming");
             exit(1);
+        } else if (spice_dmabuf_encode) {
+            if (port == 0 && tls_port == 0) {
+                error_report("dmabuf-encode=on is only meant to be used for "
+                             "non-local displays");
+                exit(1);
+            }
+            if (g_strcmp0(preferred_codec, "gstreamer:h264")) {
+                error_report("dmabuf-encode=on currently only works and tested"
+                             "with gstreamer:h264");
+                exit(1);
+            }
         }
         if (egl_rendernode_init(qemu_opt_get(opts, "rendernode"),
                                 DISPLAYGL_MODE_ON) != 0) {
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 494168e7fe..90ada643a2 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -28,6 +28,7 @@
 #include "ui/spice-display.h"
 
 bool spice_opengl;
+bool spice_dmabuf_encode;
 
 int qemu_spice_rect_is_empty(const QXLRect* r)
 {
@@ -117,7 +118,7 @@ void qemu_spice_wakeup(SimpleSpiceDisplay *ssd)
 }
 
 static void qemu_spice_create_one_update(SimpleSpiceDisplay *ssd,
-                                         QXLRect *rect)
+                                         QXLRect *rect, bool dmabuf)
 {
     SimpleSpiceUpdate *update;
     QXLDrawable *drawable;
@@ -168,15 +169,17 @@ static void qemu_spice_create_one_update(SimpleSpiceDisplay *ssd,
     image->bitmap.palette = 0;
     image->bitmap.format = SPICE_BITMAP_FMT_32BIT;
 
-    dest = pixman_image_create_bits(PIXMAN_LE_x8r8g8b8, bw, bh,
-                                    (void *)update->bitmap, bw * 4);
-    pixman_image_composite(PIXMAN_OP_SRC, ssd->surface, NULL, ssd->mirror,
-                           rect->left, rect->top, 0, 0,
-                           rect->left, rect->top, bw, bh);
-    pixman_image_composite(PIXMAN_OP_SRC, ssd->mirror, NULL, dest,
-                           rect->left, rect->top, 0, 0,
-                           0, 0, bw, bh);
-    pixman_image_unref(dest);
+    if (!dmabuf) {
+        dest = pixman_image_create_bits(PIXMAN_LE_x8r8g8b8, bw, bh,
+                                        (void *)update->bitmap, bw * 4);
+        pixman_image_composite(PIXMAN_OP_SRC, ssd->surface, NULL, ssd->mirror,
+                               rect->left, rect->top, 0, 0,
+                               rect->left, rect->top, bw, bh);
+        pixman_image_composite(PIXMAN_OP_SRC, ssd->mirror, NULL, dest,
+                               rect->left, rect->top, 0, 0,
+                               0, 0, bw, bh);
+        pixman_image_unref(dest);
+    }
 
     cmd->type = QXL_CMD_DRAW;
     cmd->data = (uintptr_t)drawable;
@@ -220,7 +223,7 @@ static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
                         .left   = x,
                         .right  = x + bw,
                     };
-                    qemu_spice_create_one_update(ssd, &update);
+                    qemu_spice_create_one_update(ssd, &update, false);
                     dirty_top[blk] = -1;
                 }
             } else {
@@ -241,7 +244,7 @@ static void qemu_spice_create_update(SimpleSpiceDisplay *ssd)
                 .left   = x,
                 .right  = x + bw,
             };
-            qemu_spice_create_one_update(ssd, &update);
+            qemu_spice_create_one_update(ssd, &update, false);
             dirty_top[blk] = -1;
         }
     }
@@ -848,9 +851,26 @@ static void qemu_spice_gl_block_timer(void *opaque)
     warn_report("spice: no gl-draw-done within one second");
 }
 
+static void spice_gl_create_update(SimpleSpiceDisplay *ssd,
+                                   uint32_t width, uint32_t height)
+{
+    QXLRect update = {
+        .top    = 0,
+        .bottom = height,
+        .left   = 0,
+        .right  = width,
+    };
+
+    WITH_QEMU_LOCK_GUARD(&ssd->lock) {
+        qemu_spice_create_one_update(ssd, &update, true);
+    }
+    qemu_spice_wakeup(ssd);
+}
+
 static void spice_gl_refresh(DisplayChangeListener *dcl)
 {
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
+    bool local_display = spice_dmabuf_encode ? false : true;
     uint64_t cookie;
 
     if (!ssd->ds || qemu_console_is_gl_blocked(ssd->dcl.con)) {
@@ -865,7 +885,11 @@ static void spice_gl_refresh(DisplayChangeListener *dcl)
         spice_qxl_gl_draw_async(&ssd->qxl, 0, 0,
                                 surface_width(ssd->ds),
                                 surface_height(ssd->ds),
-                                cookie);
+                                cookie, local_display);
+        if (!local_display) {
+            spice_gl_create_update(ssd, surface_width(ssd->ds),
+                                   surface_height(ssd->ds));
+        }
         ssd->gl_updates = 0;
     }
 }
@@ -891,6 +915,9 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
     }
     ssd->ds = new_surface;
     if (ssd->ds) {
+        if (spice_dmabuf_encode) {
+            qemu_spice_create_host_primary(ssd);
+        }
         surface_gl_create_texture(ssd->gls, ssd->ds);
         fd = egl_get_fd_for_texture(ssd->ds->texture,
                                     &stride, &fourcc,
@@ -909,7 +936,9 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
         spice_qxl_gl_scanout(&ssd->qxl, fd,
                              surface_width(ssd->ds),
                              surface_height(ssd->ds),
-                             stride, fourcc, false);
+                             stride, fourcc, false,
+                             spice_dmabuf_encode ? false : true);
+
         ssd->have_surface = true;
         ssd->have_scanout = false;
 
@@ -932,7 +961,8 @@ static void qemu_spice_gl_scanout_disable(DisplayChangeListener *dcl)
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
 
     trace_qemu_spice_gl_scanout_disable(ssd->qxl.id);
-    spice_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false);
+    spice_qxl_gl_scanout(&ssd->qxl, -1, 0, 0, 0, 0, false,
+                         spice_dmabuf_encode ? false : true);
     qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
     ssd->have_surface = false;
     ssd->have_scanout = false;
@@ -960,7 +990,9 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
 
     /* note: spice server will close the fd */
     spice_qxl_gl_scanout(&ssd->qxl, fd, backing_width, backing_height,
-                         stride, fourcc, y_0_top);
+                         stride, fourcc, y_0_top,
+                         spice_dmabuf_encode ? false : true);
+
     qemu_spice_gl_monitor_config(ssd, x, y, w, h);
     ssd->have_surface = false;
     ssd->have_scanout = true;
@@ -1031,6 +1063,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     EGLint stride = 0, fourcc = 0;
     bool render_cursor = false;
     bool y_0_top = false; /* FIXME */
+    bool local_display = spice_dmabuf_encode ? false : true;
     uint64_t cookie;
     int fd;
 
@@ -1072,7 +1105,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
                                             &stride, &fourcc, NULL);
                 spice_qxl_gl_scanout(&ssd->qxl, fd,
                                      dmabuf->width, dmabuf->height,
-                                     stride, fourcc, false);
+                                     stride, fourcc, false, local_display);
             }
         } else {
             trace_qemu_spice_gl_forward_dmabuf(ssd->qxl.id,
@@ -1081,7 +1114,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
             spice_qxl_gl_scanout(&ssd->qxl, dup(dmabuf->fd),
                                  dmabuf->width, dmabuf->height,
                                  dmabuf->stride, dmabuf->fourcc,
-                                 dmabuf->y0_top);
+                                 dmabuf->y0_top, local_display);
         }
         qemu_spice_gl_monitor_config(ssd, 0, 0, dmabuf->width, dmabuf->height);
         ssd->guest_dmabuf_refresh = false;
@@ -1104,7 +1137,11 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     qemu_spice_gl_block(ssd, true);
     glFlush();
     cookie = (uintptr_t)qxl_cookie_new(QXL_COOKIE_TYPE_GL_DRAW_DONE, 0);
-    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie);
+    spice_qxl_gl_draw_async(&ssd->qxl, x, y, w, h, cookie, local_display);
+    if (!local_display) {
+        spice_gl_create_update(ssd, surface_width(ssd->ds),
+                               surface_height(ssd->ds));
+    }
 }
 
 static const DisplayChangeListenerOps display_listener_gl_ops = {
-- 
2.37.2

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

[Gerd Hoffmann]

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

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

take care,
  Gerd

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

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

[Gerd Hoffmann]

  Hi,

> 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 is what I expected to happen.  This also the case when using
dma-bufs for local display.

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

I'm thinking about a simple bool, i.e. replace the current hard failure
for the remote case with a query whenever the server supports dma-buf
video encoding or not.

take care,
  Gerd

Re: [RFC v2 1/2] spice: Add an option for users to provide a preferred codec

[Frediano Ziglio]

Il giorno lun 23 gen 2023 alle ore 08:37 Vivek Kasireddy
<vivek.kasireddy@intel.com> ha scritto:
>
> Giving users an option to choose a particular codec will enable
> them to make an appropriate decision based on their hardware and
> use-case.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  qemu-options.hx |  5 +++++
>  ui/spice-core.c | 14 ++++++++++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 3aa3a2f5a3..aab8df0922 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2142,6 +2142,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
>      "       [,streaming-video=[off|all|filter]][,disable-copy-paste=on|off]\n"
>      "       [,disable-agent-file-xfer=on|off][,agent-mouse=[on|off]]\n"
>      "       [,playback-compression=[on|off]][,seamless-migration=[on|off]]\n"
> +    "       [,preferred-codec=<encoder>:<codec>\n"
>      "       [,gl=[on|off]][,rendernode=<file>]\n"
>      "   enable spice\n"
>      "   at least one of {port, tls-port} is mandatory\n",
> @@ -2237,6 +2238,10 @@ SRST
>      ``seamless-migration=[on|off]``
>          Enable/disable spice seamless migration. Default is off.
>
> +    ``preferred-codec=<encoder>:<codec>``
> +        Provide the preferred codec the Spice server should use.
> +        Default would be spice:mjpeg.
> +
>      ``gl=[on|off]``
>          Enable/disable OpenGL context. Default is off.
>
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 72f8f1681c..6e00211e3a 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -469,6 +469,9 @@ static QemuOptsList qemu_spice_opts = {
>          },{
>              .name = "streaming-video",
>              .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "preferred-codec",
> +            .type = QEMU_OPT_STRING,
>          },{
>              .name = "agent-mouse",
>              .type = QEMU_OPT_BOOL,
> @@ -644,6 +647,7 @@ static void qemu_spice_init(void)
>      char *x509_key_file = NULL,
>          *x509_cert_file = NULL,
>          *x509_cacert_file = NULL;
> +    const char *preferred_codec = NULL;
>      int port, tls_port, addr_flags;
>      spice_image_compression_t compression;
>      spice_wan_compression_t wan_compr;
> @@ -795,6 +799,16 @@ static void qemu_spice_init(void)
>          spice_server_set_streaming_video(spice_server, SPICE_STREAM_VIDEO_OFF);
>      }
>
> +    preferred_codec = qemu_opt_get(opts, "preferred-codec");
> +    if (preferred_codec) {
> +        if (spice_server_set_video_codecs(spice_server, preferred_codec)) {
> +            error_report("Preferred codec name is not valid");
> +            exit(1);
> +        }
> +    } else {
> +        spice_server_set_video_codecs(spice_server, "spice:mjpeg");

Why overriding the compiled in Spice default?
Apart from that the commit seems good to me.

> +    }
> +
>      spice_server_set_agent_mouse
>          (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
>      spice_server_set_playback_compression

Frediano

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

[Frediano Ziglio]

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

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

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

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