[PATCH v5 0/7] ui/spice: Enable gl=on option for non-local or remote clients

[PATCH v5 0/7] ui/spice: Enable gl=on option for non-local or remote clients

[Vivek Kasireddy]

To address the limitation that this option is incompatible with
remote clients, this patch series adds an option to select a
preferred codec and also enable gl=on option for clients that
are connected via the network. In other words, with this option
enabled (and the below linked Spice series merged), it would be
possible to have Qemu share a dmabuf fd with Spice, which would
then forward it to a hardware or software based encoder and
eventually send the data associated with the fd to a client that
could be located on a different machine.

Essentially, this patch series provides a hardware accelerated,
opensource VDI option for users using Qemu and Spice by leveraging
the iGPU/dGPU on the host machine to encode the Guest FB via the
Gstreamer framework.

v4 -> v5 (suggestions from Marc-André):
- Fix the errors (mostly 80 chars limit violations) identified by
  scripts/checkpatch.pl
- Rename the globals to have a spice_ prefix for consistency
- Rename MAX_REFRESH_RATE to DEFAULT_MAX_REFRESH_RATE
- Added comments to explain how/when the gl_draw request is submitted
  to spice server in the remote clients case
- Fix the mem_obj leak that would occur when the associated texture
  is destroyed or when an error is encountered while creating a
  texture from an fd (Dmitry and Michael)
- Merged Michael's patch to fix the mem_obj leak into this series and
  added his Co-developed-by tag to the relevant patches

v3 -> v4 (suggestions from Marc-André):
- Add a new parameter to make max_refresh_rate configurable
- Have surface_gl_create_texture_from_fd() return bool after checking
  for errors
- Remove the check for PIXMAN_r5g6b5() in spice_gl_replace_fd_texture()
- Report errors in spice_gl_replace_fd_texture() when someting fails
- Use glGetError() correctly by adding an additional (dummy) call
  before checking for actual errors (Dmitry)
- Add a new patch to check fd values in egl_dmabuf_export_texture()
- Rebase on Qemu master

v2 -> v3:
- Check for errors after invoking glImportMemoryFdEXT() using
  glGetError() and report the error to user (Dmitry)

v1 -> v2:
- Replace the option name preferred-codec with video-codecs (Marc-André)
- Add a warning when an fd cannot be created from texture (Marc-André)
- Add a new patch to blit the scanout texture into a linear one to
  make it work with virgl
- Rebased and tested against the latest Spice master

Tested with the following Qemu parameters:
-device virtio-vga,max_outputs=1,xres=1920,yres=1080,blob=true
-spice port=3001,gl=on,disable-ticketing=on,video-codecs=gstreamer:h264

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

Associated Spice server MR (merged):
https://gitlab.freedesktop.org/spice/spice/-/merge_requests/229

---
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Alex Bennée <alex.bennee@linaro.org>

Vivek Kasireddy (7):
  ui/egl-helpers: Error check the fds in egl_dmabuf_export_texture()
  ui/spice: Add an option for users to provide a preferred codec
  ui/spice: Enable gl=on option for non-local or remote clients
  ui/spice: Add an option to submit gl_draw requests at fixed rate
  ui/console-gl: Add a helper to create a texture with linear memory
    layout
  ui/spice: Create a new texture with linear layout when gl=on is
    enabled
  ui/spice: Blit the scanout texture if its memory layout is not linear

 include/ui/console.h       |   3 +
 include/ui/spice-display.h |   5 +
 include/ui/surface.h       |   1 +
 qemu-options.hx            |  10 ++
 ui/console-gl.c            |  54 +++++++++
 ui/egl-helpers.c           |   6 +
 ui/spice-core.c            |  28 +++++
 ui/spice-display.c         | 226 ++++++++++++++++++++++++++++++++++---
 8 files changed, 317 insertions(+), 16 deletions(-)

-- 
2.49.0

[PATCH v5 1/7] ui/egl-helpers: Error check the fds in egl_dmabuf_export_texture()

[Vivek Kasireddy]

While trying to export and obtain fds associated with a texture, it
is possible that the fds returned after eglExportDMABUFImageMESA()
call have error values. Therefore, we need to evaluate the value of
all fds and return false if any of them are negative.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 ui/egl-helpers.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 5503a795e4..e3f2872cc1 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -295,6 +295,7 @@ bool egl_dmabuf_export_texture(uint32_t tex_id, int *fd, EGLint *offset,
 {
     EGLImageKHR image;
     EGLuint64KHR modifiers[DMABUF_MAX_PLANES];
+    int i;
 
     image = eglCreateImageKHR(qemu_egl_display, eglGetCurrentContext(),
                               EGL_GL_TEXTURE_2D_KHR,
@@ -314,6 +315,11 @@ bool egl_dmabuf_export_texture(uint32_t tex_id, int *fd, EGLint *offset,
         *modifier = modifiers[0];
     }
 
+    for (i = 0; i < *num_planes; i++) {
+        if (fd[i] < 0) {
+            return false;
+        }
+    }
     return true;
 }
 
-- 
2.49.0

[PATCH v5 2/7] ui/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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 qemu-options.hx |  5 +++++
 ui/spice-core.c | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 7eb8e02b4b..fcddb583c9 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2281,6 +2281,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"
+    "       [,video-codecs=<encoder>:<codec>\n"
     "       [,gl=[on|off]][,rendernode=<file>]\n"
     "                enable spice\n"
     "                at least one of {port, tls-port} is mandatory\n",
@@ -2369,6 +2370,10 @@ SRST
     ``seamless-migration=[on|off]``
         Enable/disable spice seamless migration. Default is off.
 
+    ``video-codecs=<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 0326c63bec..907b0e9a81 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -488,6 +488,9 @@ static QemuOptsList qemu_spice_opts = {
         },{
             .name = "streaming-video",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "video-codecs",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "agent-mouse",
             .type = QEMU_OPT_BOOL,
@@ -662,6 +665,7 @@ static void qemu_spice_init(void)
     char *x509_key_file = NULL,
         *x509_cert_file = NULL,
         *x509_cacert_file = NULL;
+    const char *video_codecs = NULL;
     int port, tls_port, addr_flags;
     spice_image_compression_t compression;
     spice_wan_compression_t wan_compr;
@@ -801,6 +805,14 @@ static void qemu_spice_init(void)
         spice_server_set_streaming_video(spice_server, SPICE_STREAM_VIDEO_OFF);
     }
 
+    video_codecs = qemu_opt_get(opts, "video-codecs");
+    if (video_codecs) {
+        if (spice_server_set_video_codecs(spice_server, video_codecs)) {
+            error_report("invalid video codecs");
+            exit(1);
+        }
+    }
+
     spice_server_set_agent_mouse
         (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
     spice_server_set_playback_compression
-- 
2.49.0

[PATCH v5 3/7] ui/spice: Enable gl=on option for non-local or remote clients

[Vivek Kasireddy]

Newer versions of Spice server should be able to accept dmabuf
fds from Qemu for clients that are connected via the network.
In other words, when this option is enabled, Qemu would share
a dmabuf fd with Spice which would encode and send the data
associated with the fd to a client that could be located on
a different machine.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/ui/spice-display.h | 1 +
 ui/spice-core.c            | 4 ++++
 ui/spice-display.c         | 1 +
 3 files changed, 6 insertions(+)

diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
index e1a9b36185..6c55f38c8b 100644
--- a/include/ui/spice-display.h
+++ b/include/ui/spice-display.h
@@ -151,6 +151,7 @@ struct SimpleSpiceCursor {
 };
 
 extern bool spice_opengl;
+extern bool spice_remote_client;
 
 int qemu_spice_rect_is_empty(const QXLRect* r);
 void qemu_spice_rect_union(QXLRect *dest, const QXLRect *r);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 907b0e9a81..f92dd55392 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -848,9 +848,13 @@ static void qemu_spice_init(void)
 #ifdef HAVE_SPICE_GL
     if (qemu_opt_get_bool(opts, "gl", 0)) {
         if ((port != 0) || (tls_port != 0)) {
+#if SPICE_SERVER_VERSION >= 0x000f03 /* release 0.15.3 */
+            spice_remote_client = 1;
+#else
             error_report("SPICE GL support is local-only for now and "
                          "incompatible with -spice port/tls-port");
             exit(1);
+#endif
         }
         egl_init(qemu_opt_get(opts, "rendernode"), DISPLAY_GL_MODE_ON, &error_fatal);
         spice_opengl = 1;
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 9c39d2c5c8..0fb72f6d6f 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -31,6 +31,7 @@
 #include "standard-headers/drm/drm_fourcc.h"
 
 bool spice_opengl;
+bool spice_remote_client;
 
 int qemu_spice_rect_is_empty(const QXLRect* r)
 {
-- 
2.49.0

[PATCH v5 4/7] ui/spice: Add an option to submit gl_draw requests at fixed rate

[Vivek Kasireddy]

In the specific case where the display layer (virtio-gpu) is using
dmabuf, and if remote clients are enabled (-spice gl=on,port=xxxx),
it makes sense to limit the maximum (streaming) rate (refresh rate)
to a fixed value using the GUI refresh timer. Otherwise, the updates
or gl_draw requests would be sent as soon as the Guest submits a new
frame which is not optimal as it would lead to increased network
traffic and wastage of GPU cycles if the frames get dropped.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/ui/spice-display.h |  1 +
 qemu-options.hx            |  5 +++
 ui/spice-core.c            | 12 ++++++++
 ui/spice-display.c         | 62 ++++++++++++++++++++++++++++++++------
 4 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
index 6c55f38c8b..9bdde78266 100644
--- a/include/ui/spice-display.h
+++ b/include/ui/spice-display.h
@@ -152,6 +152,7 @@ struct SimpleSpiceCursor {
 
 extern bool spice_opengl;
 extern bool spice_remote_client;
+extern int spice_max_refresh_rate;
 
 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 fcddb583c9..98af43953d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2282,6 +2282,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"
     "       [,video-codecs=<encoder>:<codec>\n"
+    "       [,max-refresh-rate=rate\n"
     "       [,gl=[on|off]][,rendernode=<file>]\n"
     "                enable spice\n"
     "                at least one of {port, tls-port} is mandatory\n",
@@ -2374,6 +2375,10 @@ SRST
         Provide the preferred codec the Spice server should use.
         Default would be spice:mjpeg.
 
+    ``max-refresh-rate=rate``
+        Provide the maximum refresh rate (or FPS) at which the encoding
+        requests should be sent to the Spice server. Default would be 30.
+
     ``gl=[on|off]``
         Enable/disable OpenGL context. Default is off.
 
diff --git a/ui/spice-core.c b/ui/spice-core.c
index f92dd55392..6576bfd20d 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -56,6 +56,8 @@ struct SpiceTimer {
     QEMUTimer *timer;
 };
 
+#define DEFAULT_MAX_REFRESH_RATE 30
+
 static SpiceTimer *timer_add(SpiceTimerFunc func, void *opaque)
 {
     SpiceTimer *timer;
@@ -491,6 +493,9 @@ static QemuOptsList qemu_spice_opts = {
         },{
             .name = "video-codecs",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "max-refresh-rate",
+            .type = QEMU_OPT_NUMBER,
         },{
             .name = "agent-mouse",
             .type = QEMU_OPT_BOOL,
@@ -813,6 +818,13 @@ static void qemu_spice_init(void)
         }
     }
 
+    spice_max_refresh_rate = qemu_opt_get_number(opts, "max-refresh-rate",
+                                                 DEFAULT_MAX_REFRESH_RATE);
+    if (spice_max_refresh_rate <= 0) {
+        error_report("max refresh rate/fps is invalid");
+        exit(1);
+    }
+
     spice_server_set_agent_mouse
         (spice_server, qemu_opt_get_bool(opts, "agent-mouse", 1));
     spice_server_set_playback_compression
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 0fb72f6d6f..e409b6bdb2 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -32,6 +32,7 @@
 
 bool spice_opengl;
 bool spice_remote_client;
+int spice_max_refresh_rate;
 
 int qemu_spice_rect_is_empty(const QXLRect* r)
 {
@@ -844,12 +845,32 @@ static void qemu_spice_gl_block_timer(void *opaque)
     warn_report("spice: no gl-draw-done within one second");
 }
 
+static void spice_gl_draw(SimpleSpiceDisplay *ssd,
+                           uint32_t x, uint32_t y, uint32_t w, uint32_t h)
+{
+    uint64_t cookie;
+
+    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);
+}
+
 static void spice_gl_refresh(DisplayChangeListener *dcl)
 {
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
-    uint64_t cookie;
 
-    if (!ssd->ds || qemu_console_is_gl_blocked(ssd->dcl.con)) {
+    if (!ssd->ds) {
+        return;
+    }
+
+    if (qemu_console_is_gl_blocked(ssd->dcl.con)) {
+        if (spice_remote_client && ssd->gl_updates && ssd->have_scanout) {
+            glFlush();
+            spice_gl_draw(ssd, 0, 0,
+                          surface_width(ssd->ds), surface_height(ssd->ds));
+            ssd->gl_updates = 0;
+            /* E.g, to achieve 60 FPS, update_interval needs to be ~16.66 ms */
+            dcl->update_interval = 1000 / spice_max_refresh_rate;
+        }
         return;
     }
 
@@ -857,11 +878,8 @@ static void spice_gl_refresh(DisplayChangeListener *dcl)
     if (ssd->gl_updates && ssd->have_surface) {
         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, 0, 0,
-                                surface_width(ssd->ds),
-                                surface_height(ssd->ds),
-                                cookie);
+        spice_gl_draw(ssd, 0, 0,
+                      surface_width(ssd->ds), surface_height(ssd->ds));
         ssd->gl_updates = 0;
     }
 }
@@ -954,6 +972,20 @@ 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);
+
+    /*
+     * We need to check for the case of "lost" updates, where a gl_draw
+     * was not submitted because the timer did not get a chance to run.
+     * One case where this happens is when the Guest VM is getting
+     * rebooted. If the console is blocked in this situation, we need
+     * to unblock it. Otherwise, newer updates would not take effect.
+     */
+    if (qemu_console_is_gl_blocked(ssd->dcl.con)) {
+        if (spice_remote_client && ssd->gl_updates && ssd->have_scanout) {
+            ssd->gl_updates = 0;
+            qemu_spice_gl_block(ssd, false);
+        }
+    }
     spice_server_gl_scanout(&ssd->qxl, NULL, 0, 0, NULL, NULL, 0, DRM_FORMAT_INVALID,
                             DRM_FORMAT_MOD_INVALID, false);
     qemu_spice_gl_monitor_config(ssd, 0, 0, 0, 0);
@@ -1061,7 +1093,6 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     EGLint fourcc = 0;
     bool render_cursor = false;
     bool y_0_top = false; /* FIXME */
-    uint64_t cookie;
     uint32_t width, height, texture;
 
     if (!ssd->have_scanout) {
@@ -1159,8 +1190,19 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     trace_qemu_spice_gl_update(ssd->qxl.id, w, h, x, y);
     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);
+
+    /*
+     * In the case of remote clients, the submission of gl_draw request is
+     * deferred here, so that it can be submitted later (to spice server)
+     * from spice_gl_refresh() timer callback. This is done to ensure that
+     * Guest updates are submitted at a steady rate (e.g. 60 FPS) instead
+     * of submitting them arbitrarily.
+     */
+    if (spice_remote_client) {
+        ssd->gl_updates++;
+    } else {
+        spice_gl_draw(ssd, x, y, w, h);
+    }
 }
 
 static const DisplayChangeListenerOps display_listener_gl_ops = {
-- 
2.49.0

[PATCH v5 5/7] ui/console-gl: Add a helper to create a texture with linear memory layout

[Vivek Kasireddy]

There are cases where we do not want the memory layout of a texture to
be tiled as the component processing the texture would not know how to
de-tile either via software or hardware. Therefore, ensuring that the
memory backing the texture has a linear layout is absolutely necessary
in these situations.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Co-developed-by: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/ui/console.h |  3 +++
 ui/console-gl.c      | 48 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index 46b3128185..98feaa58bd 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -422,6 +422,9 @@ bool console_gl_check_format(DisplayChangeListener *dcl,
                              pixman_format_code_t format);
 void surface_gl_create_texture(QemuGLShader *gls,
                                DisplaySurface *surface);
+bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
+                                       int fd, GLuint *texture,
+                                       GLuint *mem_obj);
 void surface_gl_update_texture(QemuGLShader *gls,
                                DisplaySurface *surface,
                                int x, int y, int w, int h);
diff --git a/ui/console-gl.c b/ui/console-gl.c
index 103b954017..afb36dba64 100644
--- a/ui/console-gl.c
+++ b/ui/console-gl.c
@@ -25,6 +25,7 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "ui/console.h"
 #include "ui/shader.h"
 
@@ -96,6 +97,53 @@ void surface_gl_create_texture(QemuGLShader *gls,
     glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
 }
 
+bool surface_gl_create_texture_from_fd(DisplaySurface *surface,
+                                       int fd, GLuint *texture,
+                                       GLuint *mem_obj)
+{
+    unsigned long size = surface_stride(surface) * surface_height(surface);
+    GLenum err = glGetError();
+    *texture = 0;
+    *mem_obj = 0;
+
+    if (!epoxy_has_gl_extension("GL_EXT_memory_object") ||
+        !epoxy_has_gl_extension("GL_EXT_memory_object_fd")) {
+        error_report("spice: required OpenGL extensions not supported: "
+                     "GL_EXT_memory_object and GL_EXT_memory_object_fd");
+        return false;
+    }
+
+#ifdef GL_EXT_memory_object_fd
+    glCreateMemoryObjectsEXT(1, mem_obj);
+    glImportMemoryFdEXT(*mem_obj, size, GL_HANDLE_TYPE_OPAQUE_FD_EXT, fd);
+
+    err = glGetError();
+    if (err != GL_NO_ERROR) {
+        error_report("spice: cannot import memory object from fd");
+        goto cleanup_mem;
+    }
+
+    glGenTextures(1, texture);
+    glBindTexture(GL_TEXTURE_2D, *texture);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_TILING_EXT, GL_LINEAR_TILING_EXT);
+    glTexStorageMem2DEXT(GL_TEXTURE_2D, 1, GL_RGBA8, surface_width(surface),
+                         surface_height(surface), *mem_obj, 0);
+    err = glGetError();
+    if (err != GL_NO_ERROR) {
+        error_report("spice: cannot create texture from memory object");
+        goto cleanup_tex_and_mem;
+    }
+    return true;
+
+cleanup_tex_and_mem:
+    glDeleteTextures(1, texture);
+cleanup_mem:
+    glDeleteMemoryObjectsEXT(1, mem_obj);
+
+#endif
+    return false;
+}
+
 void surface_gl_update_texture(QemuGLShader *gls,
                                DisplaySurface *surface,
                                int x, int y, int w, int h)
-- 
2.49.0

[PATCH v5 6/7] ui/spice: Create a new texture with linear layout when gl=on is enabled

[Vivek Kasireddy]

Since most encoders/decoders (invoked by Spice) may not work properly
with tiled memory associated with a texture, we need to create another
texture that has linear memory layout and use that instead.

Note that, there does not seem to be a direct way to indicate to the
GL implementation that a texture's backing memory needs to be linear.
Instead, we have to do it in a roundabout way where we need to first
create a tiled texture and import that as a memory object to create
a new texture that has a linear memory layout.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Co-developed-by: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/ui/surface.h |  1 +
 ui/console-gl.c      |  6 ++++
 ui/spice-display.c   | 82 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+)

diff --git a/include/ui/surface.h b/include/ui/surface.h
index f16f7be8be..006b1986bb 100644
--- a/include/ui/surface.h
+++ b/include/ui/surface.h
@@ -22,6 +22,7 @@ typedef struct DisplaySurface {
     GLenum glformat;
     GLenum gltype;
     GLuint texture;
+    GLuint mem_obj;
 #endif
     qemu_pixman_shareable share_handle;
     uint32_t share_handle_offset;
diff --git a/ui/console-gl.c b/ui/console-gl.c
index afb36dba64..403fc36fbd 100644
--- a/ui/console-gl.c
+++ b/ui/console-gl.c
@@ -184,6 +184,12 @@ void surface_gl_destroy_texture(QemuGLShader *gls,
     }
     glDeleteTextures(1, &surface->texture);
     surface->texture = 0;
+#ifdef GL_EXT_memory_object_fd
+    if (surface->mem_obj) {
+        glDeleteMemoryObjectsEXT(1, &surface->mem_obj);
+        surface->mem_obj = 0;
+    }
+#endif
 }
 
 void surface_gl_setup_viewport(QemuGLShader *gls,
diff --git a/ui/spice-display.c b/ui/spice-display.c
index e409b6bdb2..854a97c198 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -893,6 +893,81 @@ static void spice_gl_update(DisplayChangeListener *dcl,
     ssd->gl_updates++;
 }
 
+static bool spice_gl_replace_fd_texture(SimpleSpiceDisplay *ssd,
+                                        int *fds, uint64_t *modifier,
+                                        int *num_planes)
+{
+    uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
+    GLuint texture;
+    GLuint mem_obj;
+    int fourcc;
+    bool ret;
+
+    if (!spice_remote_client) {
+        return true;
+    }
+
+    if (*modifier == DRM_FORMAT_MOD_LINEAR) {
+        return true;
+    }
+
+    if (*num_planes > 1) {
+        error_report("spice: cannot replace texture with multiple planes");
+        return false;
+    }
+
+    /*
+     * We really want to ensure that the memory layout of the texture
+     * is linear; otherwise, the encoder's output may show corruption.
+     */
+    if (!surface_gl_create_texture_from_fd(ssd->ds, fds[0], &texture,
+                                           &mem_obj)) {
+        error_report("spice: cannot create new texture from fd");
+        return false;
+    }
+
+    /*
+     * A successful return after glImportMemoryFdEXT() means that
+     * the ownership of fd has been passed to GL. In other words,
+     * the fd we got above should not be used anymore.
+     */
+    ret = egl_dmabuf_export_texture(texture,
+                                    fds,
+                                    (EGLint *)offsets,
+                                    (EGLint *)strides,
+                                    &fourcc,
+                                    num_planes,
+                                    modifier);
+    if (!ret) {
+        glDeleteTextures(1, &texture);
+#ifdef GL_EXT_memory_object_fd
+        glDeleteMemoryObjectsEXT(1, &mem_obj);
+#endif
+
+        /*
+         * Since we couldn't export our newly create texture (or create,
+         * an fd associated with it) we need to backtrack and try to
+         * recreate the fd associated with the original texture.
+         */
+        ret = egl_dmabuf_export_texture(ssd->ds->texture,
+                                        fds,
+                                        (EGLint *)offsets,
+                                        (EGLint *)strides,
+                                        &fourcc,
+                                        num_planes,
+                                        modifier);
+        if (!ret) {
+            surface_gl_destroy_texture(ssd->gls, ssd->ds);
+            warn_report("spice: no texture available to display");
+        }
+    } else {
+        surface_gl_destroy_texture(ssd->gls, ssd->ds);
+        ssd->ds->texture = texture;
+        ssd->ds->mem_obj = mem_obj;
+    }
+    return ret;
+}
+
 static void spice_server_gl_scanout(QXLInstance *qxl,
                                     const int *fd,
                                     uint32_t width, uint32_t height,
@@ -917,6 +992,7 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
                             struct DisplaySurface *new_surface)
 {
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
+    bool ret;
 
     if (ssd->ds) {
         surface_gl_destroy_texture(ssd->gls, ssd->ds);
@@ -939,6 +1015,12 @@ static void spice_gl_switch(DisplayChangeListener *dcl,
             return;
         }
 
+        ret = spice_gl_replace_fd_texture(ssd, fd, &modifier, &num_planes);
+        if (!ret) {
+            surface_gl_destroy_texture(ssd->gls, ssd->ds);
+            return;
+        }
+
         trace_qemu_spice_gl_surface(ssd->qxl.id,
                                     surface_width(ssd->ds),
                                     surface_height(ssd->ds),
-- 
2.49.0

[PATCH v5 7/7] ui/spice: Blit the scanout texture if its memory layout is not linear

[Vivek Kasireddy]

In cases where the scanout buffer is provided as a texture (e.g. Virgl)
we need to check to see if it has a linear memory layout or not. If
it doesn't have a linear layout, then blitting it onto the texture
associated with the display surface (which already has a linear layout)
seems to ensure that there is no corruption seen regardless of which
encoder or decoder is used.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Frediano Ziglio <freddy77@gmail.com>
Cc: Dongwon Kim <dongwon.kim@intel.com>
Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 include/ui/spice-display.h |  3 ++
 ui/spice-display.c         | 81 +++++++++++++++++++++++++++++++++++---
 2 files changed, 78 insertions(+), 6 deletions(-)

diff --git a/include/ui/spice-display.h b/include/ui/spice-display.h
index 9bdde78266..690ece7380 100644
--- a/include/ui/spice-display.h
+++ b/include/ui/spice-display.h
@@ -132,6 +132,9 @@ struct SimpleSpiceDisplay {
     egl_fb guest_fb;
     egl_fb blit_fb;
     egl_fb cursor_fb;
+    bool backing_y_0_top;
+    bool blit_scanout_texture;
+    bool new_scanout_texture;
     bool have_hot;
 #endif
 };
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 854a97c198..9ce622cefc 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1086,7 +1086,7 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
 {
     SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
     EGLint offset[DMABUF_MAX_PLANES], stride[DMABUF_MAX_PLANES], fourcc = 0;
-    int fd[DMABUF_MAX_PLANES], num_planes;
+    int fd[DMABUF_MAX_PLANES], num_planes, i;
     uint64_t modifier;
 
     assert(tex_id);
@@ -1098,11 +1098,26 @@ static void qemu_spice_gl_scanout_texture(DisplayChangeListener *dcl,
 
     trace_qemu_spice_gl_scanout_texture(ssd->qxl.id, w, h, fourcc);
 
-    /* note: spice server will close the fd */
-    spice_server_gl_scanout(&ssd->qxl, fd, backing_width, backing_height,
-                            (uint32_t *)offset, (uint32_t *)stride, num_planes,
-                            fourcc, modifier, y_0_top);
-    qemu_spice_gl_monitor_config(ssd, x, y, w, h);
+    if (spice_remote_client && modifier != DRM_FORMAT_MOD_LINEAR) {
+        egl_fb_destroy(&ssd->guest_fb);
+        egl_fb_setup_for_tex(&ssd->guest_fb,
+                             backing_width, backing_height,
+                             tex_id, false);
+        ssd->backing_y_0_top = y_0_top;
+        ssd->blit_scanout_texture = true;
+        ssd->new_scanout_texture = true;
+
+        for (i = 0; i < num_planes; i++) {
+            close(fd[i]);
+        }
+    } else {
+        /* note: spice server will close the fd */
+        spice_server_gl_scanout(&ssd->qxl, fd, backing_width, backing_height,
+                                (uint32_t *)offset, (uint32_t *)stride,
+                                num_planes, fourcc, modifier, y_0_top);
+        qemu_spice_gl_monitor_config(ssd, x, y, w, h);
+    }
+
     ssd->have_surface = false;
     ssd->have_scanout = true;
 }
@@ -1168,6 +1183,50 @@ static void qemu_spice_gl_release_dmabuf(DisplayChangeListener *dcl,
     egl_dmabuf_release_texture(dmabuf);
 }
 
+static bool spice_gl_blit_scanout_texture(SimpleSpiceDisplay *ssd,
+                                          egl_fb *scanout_tex_fb)
+{
+    uint32_t offsets[DMABUF_MAX_PLANES], strides[DMABUF_MAX_PLANES];
+    int fds[DMABUF_MAX_PLANES], num_planes, fourcc;
+    uint64_t modifier;
+    bool ret;
+
+    egl_fb_destroy(scanout_tex_fb);
+    egl_fb_setup_for_tex(scanout_tex_fb,
+                         surface_width(ssd->ds), surface_height(ssd->ds),
+                         ssd->ds->texture, false);
+    egl_fb_blit(scanout_tex_fb, &ssd->guest_fb, false);
+    glFlush();
+
+    if (!ssd->new_scanout_texture) {
+        return true;
+    }
+
+    ret = egl_dmabuf_export_texture(ssd->ds->texture,
+                                    fds,
+                                    (EGLint *)offsets,
+                                    (EGLint *)strides,
+                                    &fourcc,
+                                    &num_planes,
+                                    &modifier);
+    if (!ret) {
+        error_report("spice: failed to get fd for texture");
+        return false;
+    }
+
+    spice_server_gl_scanout(&ssd->qxl, fds,
+                            surface_width(ssd->ds),
+                            surface_height(ssd->ds),
+                            (uint32_t *)offsets, (uint32_t *)strides,
+                            num_planes, fourcc, modifier,
+                            ssd->backing_y_0_top);
+    qemu_spice_gl_monitor_config(ssd, 0, 0,
+                                 surface_width(ssd->ds),
+                                 surface_height(ssd->ds));
+    ssd->new_scanout_texture = false;
+    return true;
+}
+
 static void qemu_spice_gl_update(DisplayChangeListener *dcl,
                                  uint32_t x, uint32_t y, uint32_t w, uint32_t h)
 {
@@ -1175,6 +1234,7 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
     EGLint fourcc = 0;
     bool render_cursor = false;
     bool y_0_top = false; /* FIXME */
+    bool ret;
     uint32_t width, height, texture;
 
     if (!ssd->have_scanout) {
@@ -1269,6 +1329,15 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
         glFlush();
     }
 
+    if (spice_remote_client && ssd->blit_scanout_texture) {
+        egl_fb scanout_tex_fb;
+
+        ret = spice_gl_blit_scanout_texture(ssd, &scanout_tex_fb);
+        if (!ret) {
+            return;
+        }
+    }
+
     trace_qemu_spice_gl_update(ssd->qxl.id, w, h, x, y);
     qemu_spice_gl_block(ssd, true);
     glFlush();
-- 
2.49.0

Re: [PATCH v5 0/7] ui/spice: Enable gl=on option for non-local or remote clients

[Marc-André Lureau]

Hi Vivek

On Thu, May 29, 2025 at 9:16 AM Vivek Kasireddy
<vivek.kasireddy@intel.com> wrote:
>
> To address the limitation that this option is incompatible with
> remote clients, this patch series adds an option to select a
> preferred codec and also enable gl=on option for clients that
> are connected via the network. In other words, with this option
> enabled (and the below linked Spice series merged), it would be
> possible to have Qemu share a dmabuf fd with Spice, which would
> then forward it to a hardware or software based encoder and
> eventually send the data associated with the fd to a client that
> could be located on a different machine.
>
> Essentially, this patch series provides a hardware accelerated,
> opensource VDI option for users using Qemu and Spice by leveraging
> the iGPU/dGPU on the host machine to encode the Guest FB via the
> Gstreamer framework.
>

I tested the series on fedora with intel-media-driver installed from rpmfusion.

Without explicit video-codecs argument I get:
qemu-system-x86_64: warning: Spice:
../server/dcc-send.cpp:1780:red_marshall_gl_draw_stream: No video
encoder available for this stream
qemu-system-x86_64: warning: spice: no gl-draw-done within one second

If I specify video-codecs=gstreamer:h264, then it seems to work fine.

I wish all of this would be better documented or more explicit, as
each step took me a while to figure out (why it didn't pick a
compatible encoder, what was the argument for video-codecs, why gst
didn't support h264enc, where to find the encoder, ...). I am not even
sure I am doing all this  correctly. Maybe there should be some
docs/interop/spice.rst to document qemu -spice and video-codecs
usages? (and move docs/spice-port-fqdn.txt content there too)

I suppose the issue with better default for video-codecs is on
spice-server side, so for this series:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> v4  -> v5 (suggestions from Marc-André):
> - Fix the errors (mostly 80 chars limit violations) identified by
>   scripts/checkpatch.pl
> - Rename the globals to have a spice_ prefix for consistency
> - Rename MAX_REFRESH_RATE to DEFAULT_MAX_REFRESH_RATE
> - Added comments to explain how/when the gl_draw request is submitted
>   to spice server in the remote clients case
> - Fix the mem_obj leak that would occur when the associated texture
>   is destroyed or when an error is encountered while creating a
>   texture from an fd (Dmitry and Michael)
> - Merged Michael's patch to fix the mem_obj leak into this series and
>   added his Co-developed-by tag to the relevant patches
>
> v3 -> v4 (suggestions from Marc-André):
> - Add a new parameter to make max_refresh_rate configurable
> - Have surface_gl_create_texture_from_fd() return bool after checking
>   for errors
> - Remove the check for PIXMAN_r5g6b5() in spice_gl_replace_fd_texture()
> - Report errors in spice_gl_replace_fd_texture() when someting fails
> - Use glGetError() correctly by adding an additional (dummy) call
>   before checking for actual errors (Dmitry)
> - Add a new patch to check fd values in egl_dmabuf_export_texture()
> - Rebase on Qemu master
>
> v2 -> v3:
> - Check for errors after invoking glImportMemoryFdEXT() using
>   glGetError() and report the error to user (Dmitry)
>
> v1 -> v2:
> - Replace the option name preferred-codec with video-codecs (Marc-André)
> - Add a warning when an fd cannot be created from texture (Marc-André)
> - Add a new patch to blit the scanout texture into a linear one to
>   make it work with virgl
> - Rebased and tested against the latest Spice master
>
> Tested with the following Qemu parameters:
> -device virtio-vga,max_outputs=1,xres=1920,yres=1080,blob=true
> -spice port=3001,gl=on,disable-ticketing=on,video-codecs=gstreamer:h264
>
> and remote-viewer --spice-debug spice://x.x.x.x:3001 on the client side.
>
> Associated Spice server MR (merged):
> https://gitlab.freedesktop.org/spice/spice/-/merge_requests/229
>
> ---
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Cc: Frediano Ziglio <freddy77@gmail.com>
> Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Cc: Alex Bennée <alex.bennee@linaro.org>
>
> Vivek Kasireddy (7):
>   ui/egl-helpers: Error check the fds in egl_dmabuf_export_texture()
>   ui/spice: Add an option for users to provide a preferred codec
>   ui/spice: Enable gl=on option for non-local or remote clients
>   ui/spice: Add an option to submit gl_draw requests at fixed rate
>   ui/console-gl: Add a helper to create a texture with linear memory
>     layout
>   ui/spice: Create a new texture with linear layout when gl=on is
>     enabled
>   ui/spice: Blit the scanout texture if its memory layout is not linear
>
>  include/ui/console.h       |   3 +
>  include/ui/spice-display.h |   5 +
>  include/ui/surface.h       |   1 +
>  qemu-options.hx            |  10 ++
>  ui/console-gl.c            |  54 +++++++++
>  ui/egl-helpers.c           |   6 +
>  ui/spice-core.c            |  28 +++++
>  ui/spice-display.c         | 226 ++++++++++++++++++++++++++++++++++---
>  8 files changed, 317 insertions(+), 16 deletions(-)
>
> --
> 2.49.0
>
>


-- 
Marc-André Lureau

RE: [PATCH v5 0/7] ui/spice: Enable gl=on option for non-local or remote clients

[Kasireddy, Vivek]

Hi Marc-André,

> Subject: Re: [PATCH v5 0/7] ui/spice: Enable gl=on option for non-local or
> remote clients
> 
> >
> > To address the limitation that this option is incompatible with
> > remote clients, this patch series adds an option to select a
> > preferred codec and also enable gl=on option for clients that
> > are connected via the network. In other words, with this option
> > enabled (and the below linked Spice series merged), it would be
> > possible to have Qemu share a dmabuf fd with Spice, which would
> > then forward it to a hardware or software based encoder and
> > eventually send the data associated with the fd to a client that
> > could be located on a different machine.
> >
> > Essentially, this patch series provides a hardware accelerated,
> > opensource VDI option for users using Qemu and Spice by leveraging
> > the iGPU/dGPU on the host machine to encode the Guest FB via the
> > Gstreamer framework.
> >
> 
> I tested the series on fedora with intel-media-driver installed from rpmfusion.
> 
> Without explicit video-codecs argument I get:
> qemu-system-x86_64: warning: Spice:
> ../server/dcc-send.cpp:1780:red_marshall_gl_draw_stream: No video
> encoder available for this stream
> qemu-system-x86_64: warning: spice: no gl-draw-done within one second
> 
> If I specify video-codecs=gstreamer:h264, then it seems to work fine.
> 
> I wish all of this would be better documented or more explicit, as
Do you think it makes sense to use "gstreamer:h264" as default if the user
chose -spice gl=on but did not specify any video-codecs explicitly?

> each step took me a while to figure out (why it didn't pick a
> compatible encoder, what was the argument for video-codecs, why gst
> didn't support h264enc, where to find the encoder, ...). I am not even
> sure I am doing all this  correctly. Maybe there should be some
> docs/interop/spice.rst to document qemu -spice and video-codecs
> usages? (and move docs/spice-port-fqdn.txt content there too)
The codecs negotiation between Qemu and Spice can definitely be improved
as we test/add newer codecs such as AV1, etc. As far as documentation
is concerned, I'd like to do it in a follow up series if it is ok with you since
it might delay this series given that it would probably require few more
revisions/versions to get it right.

> 
> I suppose the issue with better default for video-codecs is on
> spice-server side, so for this series:
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Thank you so much!

Thanks,
Vivek

> 
> 
> > v4  -> v5 (suggestions from Marc-André):
> > - Fix the errors (mostly 80 chars limit violations) identified by
> >   scripts/checkpatch.pl
> > - Rename the globals to have a spice_ prefix for consistency
> > - Rename MAX_REFRESH_RATE to DEFAULT_MAX_REFRESH_RATE
> > - Added comments to explain how/when the gl_draw request is submitted
> >   to spice server in the remote clients case
> > - Fix the mem_obj leak that would occur when the associated texture
> >   is destroyed or when an error is encountered while creating a
> >   texture from an fd (Dmitry and Michael)
> > - Merged Michael's patch to fix the mem_obj leak into this series and
> >   added his Co-developed-by tag to the relevant patches
> >
> > v3 -> v4 (suggestions from Marc-André):
> > - Add a new parameter to make max_refresh_rate configurable
> > - Have surface_gl_create_texture_from_fd() return bool after checking
> >   for errors
> > - Remove the check for PIXMAN_r5g6b5() in spice_gl_replace_fd_texture()
> > - Report errors in spice_gl_replace_fd_texture() when someting fails
> > - Use glGetError() correctly by adding an additional (dummy) call
> >   before checking for actual errors (Dmitry)
> > - Add a new patch to check fd values in egl_dmabuf_export_texture()
> > - Rebase on Qemu master
> >
> > v2 -> v3:
> > - Check for errors after invoking glImportMemoryFdEXT() using
> >   glGetError() and report the error to user (Dmitry)
> >
> > v1 -> v2:
> > - Replace the option name preferred-codec with video-codecs (Marc-André)
> > - Add a warning when an fd cannot be created from texture (Marc-André)
> > - Add a new patch to blit the scanout texture into a linear one to
> >   make it work with virgl
> > - Rebased and tested against the latest Spice master
> >
> > Tested with the following Qemu parameters:
> > -device virtio-vga,max_outputs=1,xres=1920,yres=1080,blob=true
> > -spice port=3001,gl=on,disable-ticketing=on,video-codecs=gstreamer:h264
> >
> > and remote-viewer --spice-debug spice://x.x.x.x:3001 on the client side.
> >
> > Associated Spice server MR (merged):
> > https://gitlab.freedesktop.org/spice/spice/-/merge_requests/229
> >
> > ---
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > Cc: Frediano Ziglio <freddy77@gmail.com>
> > Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
> > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > Cc: Alex Bennée <alex.bennee@linaro.org>
> >
> > Vivek Kasireddy (7):
> >   ui/egl-helpers: Error check the fds in egl_dmabuf_export_texture()
> >   ui/spice: Add an option for users to provide a preferred codec
> >   ui/spice: Enable gl=on option for non-local or remote clients
> >   ui/spice: Add an option to submit gl_draw requests at fixed rate
> >   ui/console-gl: Add a helper to create a texture with linear memory
> >     layout
> >   ui/spice: Create a new texture with linear layout when gl=on is
> >     enabled
> >   ui/spice: Blit the scanout texture if its memory layout is not linear
> >
> >  include/ui/console.h       |   3 +
> >  include/ui/spice-display.h |   5 +
> >  include/ui/surface.h       |   1 +
> >  qemu-options.hx            |  10 ++
> >  ui/console-gl.c            |  54 +++++++++
> >  ui/egl-helpers.c           |   6 +
> >  ui/spice-core.c            |  28 +++++
> >  ui/spice-display.c         | 226 ++++++++++++++++++++++++++++++++++---
> >  8 files changed, 317 insertions(+), 16 deletions(-)
> >
> > --
> > 2.49.0
> >
> >
> 
> 
> --
> Marc-André Lureau

Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec

[Daniel P. Berrangé]

On Wed, May 28, 2025 at 10:11:13PM -0700, Vivek Kasireddy wrote:
> 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Cc: Frediano Ziglio <freddy77@gmail.com>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  qemu-options.hx |  5 +++++
>  ui/spice-core.c | 12 ++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 7eb8e02b4b..fcddb583c9 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2281,6 +2281,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"
> +    "       [,video-codecs=<encoder>:<codec>\n"
>      "       [,gl=[on|off]][,rendernode=<file>]\n"
>      "                enable spice\n"
>      "                at least one of {port, tls-port} is mandatory\n",
> @@ -2369,6 +2370,10 @@ SRST
>      ``seamless-migration=[on|off]``
>          Enable/disable spice seamless migration. Default is off.
>  
> +    ``video-codecs=<encoder>:<codec>``
> +        Provide the preferred codec the Spice server should use.
> +        Default would be spice:mjpeg.

This looks like two distinct settings overloaded into one command
line parameter, which is a design anti-pattern.

Why can't this be done as separate parameters

   video-encoder=<blah>
   video-codec=<blah>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v5 0/7] ui/spice: Enable gl=on option for non-local or remote clients

[Alex Bennée]

Vivek Kasireddy <vivek.kasireddy@intel.com> writes:

> To address the limitation that this option is incompatible with
> remote clients, this patch series adds an option to select a
> preferred codec and also enable gl=on option for clients that
> are connected via the network. In other words, with this option
> enabled (and the below linked Spice series merged), it would be
> possible to have Qemu share a dmabuf fd with Spice, which would
> then forward it to a hardware or software based encoder and
> eventually send the data associated with the fd to a client that
> could be located on a different machine.
>
> Essentially, this patch series provides a hardware accelerated,
> opensource VDI option for users using Qemu and Spice by leveraging
> the iGPU/dGPU on the host machine to encode the Guest FB via the
> Gstreamer framework.
>
> v4 -> v5 (suggestions from Marc-André):
> - Fix the errors (mostly 80 chars limit violations) identified by
>   scripts/checkpatch.pl
> - Rename the globals to have a spice_ prefix for consistency
> - Rename MAX_REFRESH_RATE to DEFAULT_MAX_REFRESH_RATE
> - Added comments to explain how/when the gl_draw request is submitted
>   to spice server in the remote clients case
> - Fix the mem_obj leak that would occur when the associated texture
>   is destroyed or when an error is encountered while creating a
>   texture from an fd (Dmitry and Michael)
> - Merged Michael's patch to fix the mem_obj leak into this series and
>   added his Co-developed-by tag to the relevant patches
>
> v3 -> v4 (suggestions from Marc-André):
> - Add a new parameter to make max_refresh_rate configurable
> - Have surface_gl_create_texture_from_fd() return bool after checking
>   for errors
> - Remove the check for PIXMAN_r5g6b5() in spice_gl_replace_fd_texture()
> - Report errors in spice_gl_replace_fd_texture() when someting fails
> - Use glGetError() correctly by adding an additional (dummy) call
>   before checking for actual errors (Dmitry)
> - Add a new patch to check fd values in egl_dmabuf_export_texture()
> - Rebase on Qemu master
>
> v2 -> v3:
> - Check for errors after invoking glImportMemoryFdEXT() using
>   glGetError() and report the error to user (Dmitry)
>
> v1 -> v2:
> - Replace the option name preferred-codec with video-codecs (Marc-André)
> - Add a warning when an fd cannot be created from texture (Marc-André)
> - Add a new patch to blit the scanout texture into a linear one to
>   make it work with virgl
> - Rebased and tested against the latest Spice master
>
> Tested with the following Qemu parameters:
> -device virtio-vga,max_outputs=1,xres=1920,yres=1080,blob=true
> -spice port=3001,gl=on,disable-ticketing=on,video-codecs=gstreamer:h264
>
> and remote-viewer --spice-debug spice://x.x.x.x:3001 on the client side.
>
> Associated Spice server MR (merged):
> https://gitlab.freedesktop.org/spice/spice/-/merge_requests/229
>
> ---
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Cc: Frediano Ziglio <freddy77@gmail.com>
> Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Cc: Alex Bennée <alex.bennee@linaro.org>
>
> Vivek Kasireddy (7):
>   ui/egl-helpers: Error check the fds in egl_dmabuf_export_texture()
>   ui/spice: Add an option for users to provide a preferred codec
>   ui/spice: Enable gl=on option for non-local or remote clients
>   ui/spice: Add an option to submit gl_draw requests at fixed rate
>   ui/console-gl: Add a helper to create a texture with linear memory
>     layout
>   ui/spice: Create a new texture with linear layout when gl=on is
>     enabled
>   ui/spice: Blit the scanout texture if its memory layout is not linear
>
>  include/ui/console.h       |   3 +
>  include/ui/spice-display.h |   5 +
>  include/ui/surface.h       |   1 +
>  qemu-options.hx            |  10 ++

Aside from the qemu-options we don't see a lot about SPICE in the docs.
Could we add a section to cover the configuration or maybe just expand
system/devices/virtio-gpu with information about how acceleration works
with remote SPICE viewers?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

RE: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec

[Kasireddy, Vivek]

Hi Daniel,

> Subject: Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a
> preferred codec
> 
> On Wed, May 28, 2025 at 10:11:13PM -0700, Vivek Kasireddy wrote:
> > 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > Cc: Frediano Ziglio <freddy77@gmail.com>
> > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > ---
> >  qemu-options.hx |  5 +++++
> >  ui/spice-core.c | 12 ++++++++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 7eb8e02b4b..fcddb583c9 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -2281,6 +2281,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"
> > +    "       [,video-codecs=<encoder>:<codec>\n"
> >      "       [,gl=[on|off]][,rendernode=<file>]\n"
> >      "                enable spice\n"
> >      "                at least one of {port, tls-port} is mandatory\n",
> > @@ -2369,6 +2370,10 @@ SRST
> >      ``seamless-migration=[on|off]``
> >          Enable/disable spice seamless migration. Default is off.
> >
> > +    ``video-codecs=<encoder>:<codec>``
> > +        Provide the preferred codec the Spice server should use.
> > +        Default would be spice:mjpeg.
> 
> This looks like two distinct settings overloaded into one command
> line parameter, which is a design anti-pattern.
> 
> Why can't this be done as separate parameters
The Spice server API used by Qemu (spice_server_set_video_codecs)
to set the preferred codec requires the video-codecs string to be in
encoder:codec format. AFAIK, there is no other option or API to set
the encoder and codec values separately.

Thanks,
Vivek

> 
>    video-encoder=<blah>
>    video-codec=<blah>
> 
> 
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange
> :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange
> :|

Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec

[Daniel P. Berrangé]

On Fri, Jun 06, 2025 at 06:10:31AM +0000, Kasireddy, Vivek wrote:
> Hi Daniel,
> 
> > Subject: Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a
> > preferred codec
> > 
> > On Wed, May 28, 2025 at 10:11:13PM -0700, Vivek Kasireddy wrote:
> > > 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > > Cc: Frediano Ziglio <freddy77@gmail.com>
> > > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > > Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
> > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > ---
> > >  qemu-options.hx |  5 +++++
> > >  ui/spice-core.c | 12 ++++++++++++
> > >  2 files changed, 17 insertions(+)
> > >
> > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > index 7eb8e02b4b..fcddb583c9 100644
> > > --- a/qemu-options.hx
> > > +++ b/qemu-options.hx
> > > @@ -2281,6 +2281,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"
> > > +    "       [,video-codecs=<encoder>:<codec>\n"
> > >      "       [,gl=[on|off]][,rendernode=<file>]\n"
> > >      "                enable spice\n"
> > >      "                at least one of {port, tls-port} is mandatory\n",
> > > @@ -2369,6 +2370,10 @@ SRST
> > >      ``seamless-migration=[on|off]``
> > >          Enable/disable spice seamless migration. Default is off.
> > >
> > > +    ``video-codecs=<encoder>:<codec>``
> > > +        Provide the preferred codec the Spice server should use.
> > > +        Default would be spice:mjpeg.
> > 
> > This looks like two distinct settings overloaded into one command
> > line parameter, which is a design anti-pattern.
> > 
> > Why can't this be done as separate parameters
> The Spice server API used by Qemu (spice_server_set_video_codecs)
> to set the preferred codec requires the video-codecs string to be in
> encoder:codec format. AFAIK, there is no other option or API to set
> the encoder and codec values separately.

QEMU can accept the separate parameters and format them into the string
format that the spice API requires so our public API is not impacted
by this spice design choice.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3429 bytes --]

Hi

On Fri, Jun 6, 2025 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, Jun 06, 2025 at 06:10:31AM +0000, Kasireddy, Vivek wrote:
> > Hi Daniel,
> >
> > > Subject: Re: [PATCH v5 2/7] ui/spice: Add an option for users to
> provide a
> > > preferred codec
> > >
> > > On Wed, May 28, 2025 at 10:11:13PM -0700, Vivek Kasireddy wrote:
> > > > 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > > > Cc: Frediano Ziglio <freddy77@gmail.com>
> > > > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > > > Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
> > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > > ---
> > > >  qemu-options.hx |  5 +++++
> > > >  ui/spice-core.c | 12 ++++++++++++
> > > >  2 files changed, 17 insertions(+)
> > > >
> > > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > > index 7eb8e02b4b..fcddb583c9 100644
> > > > --- a/qemu-options.hx
> > > > +++ b/qemu-options.hx
> > > > @@ -2281,6 +2281,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"
> > > > +    "       [,video-codecs=<encoder>:<codec>\n"
> > > >      "       [,gl=[on|off]][,rendernode=<file>]\n"
> > > >      "                enable spice\n"
> > > >      "                at least one of {port, tls-port} is
> mandatory\n",
> > > > @@ -2369,6 +2370,10 @@ SRST
> > > >      ``seamless-migration=[on|off]``
> > > >          Enable/disable spice seamless migration. Default is off.
> > > >
> > > > +    ``video-codecs=<encoder>:<codec>``
> > > > +        Provide the preferred codec the Spice server should use.
> > > > +        Default would be spice:mjpeg.
> > >
> > > This looks like two distinct settings overloaded into one command
> > > line parameter, which is a design anti-pattern.
> > >
> > > Why can't this be done as separate parameters
> > The Spice server API used by Qemu (spice_server_set_video_codecs)
> > to set the preferred codec requires the video-codecs string to be in
> > encoder:codec format. AFAIK, there is no other option or API to set
> > the encoder and codec values separately.
>
> QEMU can accept the separate parameters and format them into the string
> format that the spice API requires so our public API is not impacted
> by this spice design choice.
>
>
Apparently you cannot mix and match freely, it has a rather fixed set of
actually working values.

See here and related code:
https://gitlab.freedesktop.org/spice/spice/-/blob/master/server/reds.cpp?ref_type=heads#L3468

Tbh, I don't think the encoder matters much, and I don't know why it was
decided to associate it with video codec names.

Maybe the spice API should provide a simpler form: accept only codec names.

In the meantime, qemu should perhaps add the "working" encoder prefixes
(spice: for mjpeg, gstreamer: for others)  itself and not expose any extra
option to the user?

[-- Attachment #2: Type: text/html, Size: 5217 bytes --]

Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec

[Daniel P. Berrangé]

On Tue, Jun 10, 2025 at 03:30:24PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Jun 6, 2025 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Fri, Jun 06, 2025 at 06:10:31AM +0000, Kasireddy, Vivek wrote:
> > > Hi Daniel,
> > >
> > > > Subject: Re: [PATCH v5 2/7] ui/spice: Add an option for users to
> > provide a
> > > > preferred codec
> > > >
> > > > On Wed, May 28, 2025 at 10:11:13PM -0700, Vivek Kasireddy wrote:
> > > > > 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > > > > Cc: Frediano Ziglio <freddy77@gmail.com>
> > > > > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > > > > Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
> > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > > > ---
> > > > >  qemu-options.hx |  5 +++++
> > > > >  ui/spice-core.c | 12 ++++++++++++
> > > > >  2 files changed, 17 insertions(+)
> > > > >
> > > > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > > > index 7eb8e02b4b..fcddb583c9 100644
> > > > > --- a/qemu-options.hx
> > > > > +++ b/qemu-options.hx
> > > > > @@ -2281,6 +2281,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"
> > > > > +    "       [,video-codecs=<encoder>:<codec>\n"
> > > > >      "       [,gl=[on|off]][,rendernode=<file>]\n"
> > > > >      "                enable spice\n"
> > > > >      "                at least one of {port, tls-port} is
> > mandatory\n",
> > > > > @@ -2369,6 +2370,10 @@ SRST
> > > > >      ``seamless-migration=[on|off]``
> > > > >          Enable/disable spice seamless migration. Default is off.
> > > > >
> > > > > +    ``video-codecs=<encoder>:<codec>``
> > > > > +        Provide the preferred codec the Spice server should use.
> > > > > +        Default would be spice:mjpeg.
> > > >
> > > > This looks like two distinct settings overloaded into one command
> > > > line parameter, which is a design anti-pattern.
> > > >
> > > > Why can't this be done as separate parameters
> > > The Spice server API used by Qemu (spice_server_set_video_codecs)
> > > to set the preferred codec requires the video-codecs string to be in
> > > encoder:codec format. AFAIK, there is no other option or API to set
> > > the encoder and codec values separately.
> >
> > QEMU can accept the separate parameters and format them into the string
> > format that the spice API requires so our public API is not impacted
> > by this spice design choice.
> >
> >
> Apparently you cannot mix and match freely, it has a rather fixed set of
> actually working values.
> 
> See here and related code:
> https://gitlab.freedesktop.org/spice/spice/-/blob/master/server/reds.cpp?ref_type=heads#L3468

That's just showing the built-in defaults - the parsing code is
not enforcing any constraints. The impl though cleary only allows
'mjpeg' with 'spice':

  https://gitlab.freedesktop.org/spice/spice/-/blob/master/server/mjpeg-encoder.c#L1371

> Tbh, I don't think the encoder matters much, and I don't know why it was
> decided to associate it with video codec names.

AFAICT the only way in which the encoder matters is to distinguish the
built-in "mjpeg" impl from the gstreamer "mjpeg" coder.

> Maybe the spice API should provide a simpler form: accept only codec names.
> 
> In the meantime, qemu should perhaps add the "working" encoder prefixes
> (spice: for mjpeg, gstreamer: for others)  itself and not expose any extra
> option to the user?

Ths question is whether we need to be able to request the gstreamer
'mjpeg' impl ?

If we do, and we also assume that 'spice' will never gain any more codec
impls as built-ins, we could do

  builtin, mjpeg, vp8, vp9, h264

where 'builtin' is the standard mjpeg encoder ?

Alternatively we could just go with

  mjpeg, vp8, vp9, h264

and in the unlikely event we need to be able to skip the built-in mjpeg,
we could add  a boolean 'prefer-gstreamer=on|off'

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec

[Michael Scherle]

On 10.06.25 14:20, Daniel P. Berrangé wrote:
> On Tue, Jun 10, 2025 at 03:30:24PM +0400, Marc-André Lureau wrote:
>> Hi
>>
>> On Fri, Jun 6, 2025 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com>
>> wrote:
>>
>> Apparently you cannot mix and match freely, it has a rather fixed set of
>> actually working values.
>>
>> See here and related code:
>> https://gitlab.freedesktop.org/spice/spice/-/blob/master/server/reds.cpp?ref_type=heads#L3468
> 
> That's just showing the built-in defaults - the parsing code is
> not enforcing any constraints. The impl though cleary only allows
> 'mjpeg' with 'spice':
> 
>    https://gitlab.freedesktop.org/spice/spice/-/blob/master/server/mjpeg-encoder.c#L1371
> 
>> Tbh, I don't think the encoder matters much, and I don't know why it was
>> decided to associate it with video codec names.
> 
> AFAICT the only way in which the encoder matters is to distinguish the
> built-in "mjpeg" impl from the gstreamer "mjpeg" coder.
> 
>> Maybe the spice API should provide a simpler form: accept only codec names.
>>
>> In the meantime, qemu should perhaps add the "working" encoder prefixes
>> (spice: for mjpeg, gstreamer: for others)  itself and not expose any extra
>> option to the user?
> 
> Ths question is whether we need to be able to request the gstreamer
> 'mjpeg' impl ?
> 
> If we do, and we also assume that 'spice' will never gain any more codec
> impls as built-ins, we could do
> 
>    builtin, mjpeg, vp8, vp9, h264

With 'spice' you mean only that the builtin will not get any more codecs 
right? because I want to do an MR for spice gstreamer to add more 
codecs, like av1.

Also It might be useful to set more parameters in future, like chroma 
sub-sampling or which GStreamer plugin is wanted like: msdk, va, vaapi....

Another point is that it can actually be a list of codecs like:
gstreamer:vp8;gstreamer:h264;spice:mjpeg

Which is maybe not clear from the current documentation, which might 
rather be:
[,video-codecs=<encoder>:<codec>[;...]]

This is useful the specify the allowed codecs and also set the priority.


> 
> where 'builtin' is the standard mjpeg encoder ?
> 
> Alternatively we could just go with
> 
>    mjpeg, vp8, vp9, h264
> 
> and in the unlikely event we need to be able to skip the built-in mjpeg,
> we could add  a boolean 'prefer-gstreamer=on|off'
> 
> With regards,
> Daniel

Greetings,
Michael

RE: [PATCH v5 2/7] ui/spice: Add an option for users to provide a preferred codec

[Kasireddy, Vivek]

Hi Daniel,

> Subject: Re: [PATCH v5 2/7] ui/spice: Add an option for users to provide a
> preferred codec
> 
> On Tue, Jun 10, 2025 at 03:30:24PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Fri, Jun 6, 2025 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Fri, Jun 06, 2025 at 06:10:31AM +0000, Kasireddy, Vivek wrote:
> > > > Hi Daniel,
> > > >
> > > > > Subject: Re: [PATCH v5 2/7] ui/spice: Add an option for users to
> > > provide a
> > > > > preferred codec
> > > > >
> > > > > On Wed, May 28, 2025 at 10:11:13PM -0700, Vivek Kasireddy wrote:
> > > > > > 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: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> > > > > > Cc: Frediano Ziglio <freddy77@gmail.com>
> > > > > > Cc: Dongwon Kim <dongwon.kim@intel.com>
> > > > > > Cc: Michael Scherle <michael.scherle@rz.uni-freiburg.de>
> > > > > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> > > > > > ---
> > > > > >  qemu-options.hx |  5 +++++
> > > > > >  ui/spice-core.c | 12 ++++++++++++
> > > > > >  2 files changed, 17 insertions(+)
> > > > > >
> > > > > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > > > > index 7eb8e02b4b..fcddb583c9 100644
> > > > > > --- a/qemu-options.hx
> > > > > > +++ b/qemu-options.hx
> > > > > > @@ -2281,6 +2281,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"
> > > > > > +    "       [,video-codecs=<encoder>:<codec>\n"
> > > > > >      "       [,gl=[on|off]][,rendernode=<file>]\n"
> > > > > >      "                enable spice\n"
> > > > > >      "                at least one of {port, tls-port} is
> > > mandatory\n",
> > > > > > @@ -2369,6 +2370,10 @@ SRST
> > > > > >      ``seamless-migration=[on|off]``
> > > > > >          Enable/disable spice seamless migration. Default is off.
> > > > > >
> > > > > > +    ``video-codecs=<encoder>:<codec>``
> > > > > > +        Provide the preferred codec the Spice server should use.
> > > > > > +        Default would be spice:mjpeg.
> > > > >
> > > > > This looks like two distinct settings overloaded into one command
> > > > > line parameter, which is a design anti-pattern.
> > > > >
> > > > > Why can't this be done as separate parameters
> > > > The Spice server API used by Qemu (spice_server_set_video_codecs)
> > > > to set the preferred codec requires the video-codecs string to be in
> > > > encoder:codec format. AFAIK, there is no other option or API to set
> > > > the encoder and codec values separately.
> > >
> > > QEMU can accept the separate parameters and format them into the
> string
> > > format that the spice API requires so our public API is not impacted
> > > by this spice design choice.
> > >
> > >
> > Apparently you cannot mix and match freely, it has a rather fixed set of
> > actually working values.
> >
> > See here and related code:
> > https://gitlab.freedesktop.org/spice/spice/-
> /blob/master/server/reds.cpp?ref_type=heads#L3468
> 
> That's just showing the built-in defaults - the parsing code is
> not enforcing any constraints. The impl though cleary only allows
> 'mjpeg' with 'spice':
> 
>   https://gitlab.freedesktop.org/spice/spice/-/blob/master/server/mjpeg-
> encoder.c#L1371
> 
> > Tbh, I don't think the encoder matters much, and I don't know why it was
> > decided to associate it with video codec names.
> 
> AFAICT the only way in which the encoder matters is to distinguish the
> built-in "mjpeg" impl from the gstreamer "mjpeg" coder.
> 
> > Maybe the spice API should provide a simpler form: accept only codec
> names.
> >
> > In the meantime, qemu should perhaps add the "working" encoder prefixes
> > (spice: for mjpeg, gstreamer: for others)  itself and not expose any extra
> > option to the user?
> 
> Ths question is whether we need to be able to request the gstreamer
> 'mjpeg' impl ?
> 
> If we do, and we also assume that 'spice' will never gain any more codec
> impls as built-ins, we could do
> 
>   builtin, mjpeg, vp8, vp9, h264
> 
> where 'builtin' is the standard mjpeg encoder ?
> 
> Alternatively we could just go with
> 
>   mjpeg, vp8, vp9, h264
> 
> and in the unlikely event we need to be able to skip the built-in mjpeg,
> we could add  a boolean 'prefer-gstreamer=on|off'
This patch series is intended to be used only with the "gstreamer" encoder
type. So, I guess Qemu can just accept video-codecs=<codec> where codec
could be any of mjpeg, vp8, vp9, h264 and we could then invoke the Spice API
spice_server_set_video_codecs() with the appropriately formatted codecs
string (i.e, gstreamer:<codec>).

Thanks,
Vivek

> 
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|