* [PATCH v3 1/9] new configure option to enable gstreamer
2025-04-18 11:29 [PATCH v3 0/9] Add VNC Open H.264 Encoding Dietmar Maurer
@ 2025-04-18 11:29 ` Dietmar Maurer
2025-04-19 5:11 ` Marc-André Lureau
2025-04-23 12:14 ` Daniel P. Berrangé
2025-04-18 11:29 ` [PATCH v3 2/9] add vnc h264 encoder Dietmar Maurer
` (7 subsequent siblings)
8 siblings, 2 replies; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-18 11:29 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: Dietmar Maurer
GStreamer is required to implement H264 encoding for VNC. Please note
that QEMU already depends on this library when you enable Spice.
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
meson.build | 10 ++++++++++
meson_options.txt | 2 ++
scripts/meson-buildoptions.sh | 5 ++++-
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 41f68d3806..28ca37855a 100644
--- a/meson.build
+++ b/meson.build
@@ -1348,6 +1348,14 @@ if not get_option('zstd').auto() or have_block
required: get_option('zstd'),
method: 'pkg-config')
endif
+
+gstreamer = not_found
+if not get_option('gstreamer').auto() or have_block
+ gstreamer = dependency('gstreamer-1.0 gstreamer-base-1.0', version: '>=1.22.0',
+ required: get_option('gstreamer'),
+ method: 'pkg-config')
+endif
+
qpl = not_found
if not get_option('qpl').auto() or have_system
qpl = dependency('qpl', version: '>=1.5.0',
@@ -2563,6 +2571,7 @@ config_host_data.set('CONFIG_MALLOC_TRIM', has_malloc_trim)
config_host_data.set('CONFIG_STATX', has_statx)
config_host_data.set('CONFIG_STATX_MNT_ID', has_statx_mnt_id)
config_host_data.set('CONFIG_ZSTD', zstd.found())
+config_host_data.set('CONFIG_GSTREAMER', gstreamer.found())
config_host_data.set('CONFIG_QPL', qpl.found())
config_host_data.set('CONFIG_UADK', uadk.found())
config_host_data.set('CONFIG_QATZIP', qatzip.found())
@@ -4836,6 +4845,7 @@ summary_info += {'snappy support': snappy}
summary_info += {'bzip2 support': libbzip2}
summary_info += {'lzfse support': liblzfse}
summary_info += {'zstd support': zstd}
+summary_info += {'gstreamer support': gstreamer}
summary_info += {'Query Processing Library support': qpl}
summary_info += {'UADK Library support': uadk}
summary_info += {'qatzip support': qatzip}
diff --git a/meson_options.txt b/meson_options.txt
index 59d973bca0..11cd132be5 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -254,6 +254,8 @@ option('vnc_sasl', type : 'feature', value : 'auto',
description: 'SASL authentication for VNC server')
option('vte', type : 'feature', value : 'auto',
description: 'vte support for the gtk UI')
+option('gstreamer', type : 'feature', value : 'auto',
+ description: 'for VNC H.264 encoding with gstreamer')
# GTK Clipboard implementation is disabled by default, since it may cause hangs
# of the guest VCPUs. See gitlab issue 1150:
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 3e8e00852b..b0c273d61e 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -229,6 +229,7 @@ meson_options_help() {
printf "%s\n" ' Xen PCI passthrough support'
printf "%s\n" ' xkbcommon xkbcommon support'
printf "%s\n" ' zstd zstd compression support'
+ printf "%s\n" ' gstreamer gstreamer support (H264 for VNC)'
}
_meson_option_parse() {
case $1 in
@@ -581,6 +582,8 @@ _meson_option_parse() {
--disable-xkbcommon) printf "%s" -Dxkbcommon=disabled ;;
--enable-zstd) printf "%s" -Dzstd=enabled ;;
--disable-zstd) printf "%s" -Dzstd=disabled ;;
- *) return 1 ;;
+ --enable-gstreamer) printf "%s" -Dgstreamer=enabled ;;
+ --disable-gstreamer) printf "%s" -Dgstreamer=disabled ;;
+ *) return 1 ;;
esac
}
--
2.39.5
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 1/9] new configure option to enable gstreamer
2025-04-18 11:29 ` [PATCH v3 1/9] new configure option to enable gstreamer Dietmar Maurer
@ 2025-04-19 5:11 ` Marc-André Lureau
2025-04-23 12:14 ` Daniel P. Berrangé
1 sibling, 0 replies; 47+ messages in thread
From: Marc-André Lureau @ 2025-04-19 5:11 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: qemu-devel
On Fri, Apr 18, 2025 at 3:51 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
>
> GStreamer is required to implement H264 encoding for VNC. Please note
> that QEMU already depends on this library when you enable Spice.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> meson.build | 10 ++++++++++
> meson_options.txt | 2 ++
> scripts/meson-buildoptions.sh | 5 ++++-
> 3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 41f68d3806..28ca37855a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1348,6 +1348,14 @@ if not get_option('zstd').auto() or have_block
> required: get_option('zstd'),
> method: 'pkg-config')
> endif
> +
> +gstreamer = not_found
> +if not get_option('gstreamer').auto() or have_block
(from v2 review)
rather "or have_system"
otherwise,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> + gstreamer = dependency('gstreamer-1.0 gstreamer-base-1.0', version: '>=1.22.0',
> + required: get_option('gstreamer'),
> + method: 'pkg-config')
> +endif
> +
> qpl = not_found
> if not get_option('qpl').auto() or have_system
> qpl = dependency('qpl', version: '>=1.5.0',
> @@ -2563,6 +2571,7 @@ config_host_data.set('CONFIG_MALLOC_TRIM', has_malloc_trim)
> config_host_data.set('CONFIG_STATX', has_statx)
> config_host_data.set('CONFIG_STATX_MNT_ID', has_statx_mnt_id)
> config_host_data.set('CONFIG_ZSTD', zstd.found())
> +config_host_data.set('CONFIG_GSTREAMER', gstreamer.found())
> config_host_data.set('CONFIG_QPL', qpl.found())
> config_host_data.set('CONFIG_UADK', uadk.found())
> config_host_data.set('CONFIG_QATZIP', qatzip.found())
> @@ -4836,6 +4845,7 @@ summary_info += {'snappy support': snappy}
> summary_info += {'bzip2 support': libbzip2}
> summary_info += {'lzfse support': liblzfse}
> summary_info += {'zstd support': zstd}
> +summary_info += {'gstreamer support': gstreamer}
> summary_info += {'Query Processing Library support': qpl}
> summary_info += {'UADK Library support': uadk}
> summary_info += {'qatzip support': qatzip}
> diff --git a/meson_options.txt b/meson_options.txt
> index 59d973bca0..11cd132be5 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -254,6 +254,8 @@ option('vnc_sasl', type : 'feature', value : 'auto',
> description: 'SASL authentication for VNC server')
> option('vte', type : 'feature', value : 'auto',
> description: 'vte support for the gtk UI')
> +option('gstreamer', type : 'feature', value : 'auto',
> + description: 'for VNC H.264 encoding with gstreamer')
>
> # GTK Clipboard implementation is disabled by default, since it may cause hangs
> # of the guest VCPUs. See gitlab issue 1150:
> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
> index 3e8e00852b..b0c273d61e 100644
> --- a/scripts/meson-buildoptions.sh
> +++ b/scripts/meson-buildoptions.sh
> @@ -229,6 +229,7 @@ meson_options_help() {
> printf "%s\n" ' Xen PCI passthrough support'
> printf "%s\n" ' xkbcommon xkbcommon support'
> printf "%s\n" ' zstd zstd compression support'
> + printf "%s\n" ' gstreamer gstreamer support (H264 for VNC)'
> }
> _meson_option_parse() {
> case $1 in
> @@ -581,6 +582,8 @@ _meson_option_parse() {
> --disable-xkbcommon) printf "%s" -Dxkbcommon=disabled ;;
> --enable-zstd) printf "%s" -Dzstd=enabled ;;
> --disable-zstd) printf "%s" -Dzstd=disabled ;;
> - *) return 1 ;;
> + --enable-gstreamer) printf "%s" -Dgstreamer=enabled ;;
> + --disable-gstreamer) printf "%s" -Dgstreamer=disabled ;;
> + *) return 1 ;;
> esac
> }
> --
> 2.39.5
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 1/9] new configure option to enable gstreamer
2025-04-18 11:29 ` [PATCH v3 1/9] new configure option to enable gstreamer Dietmar Maurer
2025-04-19 5:11 ` Marc-André Lureau
@ 2025-04-23 12:14 ` Daniel P. Berrangé
1 sibling, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-23 12:14 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Fri, Apr 18, 2025 at 01:29:45PM +0200, Dietmar Maurer wrote:
> GStreamer is required to implement H264 encoding for VNC. Please note
> that QEMU already depends on this library when you enable Spice.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> meson.build | 10 ++++++++++
> meson_options.txt | 2 ++
> scripts/meson-buildoptions.sh | 5 ++++-
> 3 files changed, 16 insertions(+), 1 deletion(-)
> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
> index 3e8e00852b..b0c273d61e 100644
> --- a/scripts/meson-buildoptions.sh
> +++ b/scripts/meson-buildoptions.sh
> @@ -229,6 +229,7 @@ meson_options_help() {
> printf "%s\n" ' Xen PCI passthrough support'
> printf "%s\n" ' xkbcommon xkbcommon support'
> printf "%s\n" ' zstd zstd compression support'
> + printf "%s\n" ' gstreamer gstreamer support (H264 for VNC)'
> }
> _meson_option_parse() {
> case $1 in
> @@ -581,6 +582,8 @@ _meson_option_parse() {
> --disable-xkbcommon) printf "%s" -Dxkbcommon=disabled ;;
> --enable-zstd) printf "%s" -Dzstd=enabled ;;
> --disable-zstd) printf "%s" -Dzstd=disabled ;;
> - *) return 1 ;;
> + --enable-gstreamer) printf "%s" -Dgstreamer=enabled ;;
> + --disable-gstreamer) printf "%s" -Dgstreamer=disabled ;;
> + *) return 1 ;;
This has broken the indent of the existing code.
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 2/9] add vnc h264 encoder
2025-04-18 11:29 [PATCH v3 0/9] Add VNC Open H.264 Encoding Dietmar Maurer
2025-04-18 11:29 ` [PATCH v3 1/9] new configure option to enable gstreamer Dietmar Maurer
@ 2025-04-18 11:29 ` Dietmar Maurer
2025-04-19 5:24 ` Marc-André Lureau
` (3 more replies)
2025-04-18 11:29 ` [PATCH v3 3/9] vnc: h264: send additional frames after the display is clean Dietmar Maurer
` (6 subsequent siblings)
8 siblings, 4 replies; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-18 11:29 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: Dietmar Maurer
This patch implements H264 support for VNC. The RFB protocol
extension is defined in:
https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#open-h-264-encoding
Currently the Gstreamer x264enc plugin (software encoder) is used
to encode the video stream.
The gstreamer pipe is:
appsrc -> videoconvert -> x264enc -> appsink
Note: videoconvert is required for RGBx to YUV420 conversion.
The code still use the VNC server framebuffer change detection,
and only encodes and sends video frames if there are changes.
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
ui/meson.build | 1 +
ui/vnc-enc-h264.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++
ui/vnc-jobs.c | 49 +++++---
ui/vnc.c | 21 ++++
ui/vnc.h | 21 ++++
5 files changed, 359 insertions(+), 15 deletions(-)
create mode 100644 ui/vnc-enc-h264.c
diff --git a/ui/meson.build b/ui/meson.build
index 35fb04cadf..34f1f33699 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -46,6 +46,7 @@ vnc_ss.add(files(
))
vnc_ss.add(zlib, jpeg)
vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
+vnc_ss.add(when: gstreamer, if_true: files('vnc-enc-h264.c'))
system_ss.add_all(when: [vnc, pixman], if_true: vnc_ss)
system_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
new file mode 100644
index 0000000000..3abe6a1528
--- /dev/null
+++ b/ui/vnc-enc-h264.c
@@ -0,0 +1,282 @@
+/*
+ * QEMU VNC display driver: hextile encoding
+ *
+ * Copyright (C) 2025 Proxmox Server Solutions GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "vnc.h"
+
+#include <gst/gst.h>
+
+static void destroy_encoder_context(VncState *vs)
+{
+ gst_clear_object(&vs->h264->source);
+ gst_clear_object(&vs->h264->convert);
+ gst_clear_object(&vs->h264->gst_encoder);
+ gst_clear_object(&vs->h264->sink);
+ gst_clear_object(&vs->h264->pipeline);
+}
+
+static bool create_encoder_context(VncState *vs, int w, int h)
+{
+ g_autoptr(GstCaps) source_caps = NULL;
+ GstStateChangeReturn state_change_ret;
+
+ g_assert(vs->h264 != NULL);
+
+ if (vs->h264->sink) {
+ if (w != vs->h264->width || h != vs->h264->height) {
+ destroy_encoder_context(vs);
+ }
+ }
+
+ if (vs->h264->sink) {
+ return TRUE;
+ }
+
+ vs->h264->width = w;
+ vs->h264->height = h;
+
+ vs->h264->source = gst_element_factory_make("appsrc", "source");
+ if (!vs->h264->source) {
+ VNC_DEBUG("Could not create gst source\n");
+ goto error;
+ }
+
+ vs->h264->convert = gst_element_factory_make("videoconvert", "convert");
+ if (!vs->h264->convert) {
+ VNC_DEBUG("Could not create gst convert element\n");
+ goto error;
+ }
+
+ vs->h264->gst_encoder = gst_element_factory_make("x264enc", "gst-encoder");
+ if (!vs->h264->gst_encoder) {
+ VNC_DEBUG("Could not create gst x264 encoder\n");
+ goto error;
+ }
+
+ g_object_set(
+ vs->h264->gst_encoder,
+ "tune", 4, /* zerolatency */
+ /*
+ * fix for zerolatency with novnc (without, noVNC displays
+ * green stripes)
+ */
+ "threads", 1,
+ "pass", 5, /* Constant Quality */
+ "quantizer", 26,
+ /* avoid access unit delimiters (Nal Unit Type 9) - not required */
+ "aud", false,
+ NULL);
+
+ vs->h264->sink = gst_element_factory_make("appsink", "sink");
+ if (!vs->h264->sink) {
+ VNC_DEBUG("Could not create gst sink\n");
+ goto error;
+ }
+
+ vs->h264->pipeline = gst_pipeline_new("vnc-h264-pipeline");
+ if (!vs->h264->pipeline) {
+ VNC_DEBUG("Could not create gst pipeline\n");
+ goto error;
+ }
+
+ gst_object_ref(vs->h264->source);
+ if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->source)) {
+ gst_object_unref(vs->h264->source);
+ VNC_DEBUG("Could not add source to gst pipeline\n");
+ goto error;
+ }
+
+ gst_object_ref(vs->h264->convert);
+ if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->convert)) {
+ gst_object_unref(vs->h264->convert);
+ VNC_DEBUG("Could not add convert to gst pipeline\n");
+ goto error;
+ }
+
+ gst_object_ref(vs->h264->gst_encoder);
+ if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->gst_encoder)) {
+ gst_object_unref(vs->h264->gst_encoder);
+ VNC_DEBUG("Could not add encoder to gst pipeline\n");
+ goto error;
+ }
+
+ gst_object_ref(vs->h264->sink);
+ if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->sink)) {
+ gst_object_unref(vs->h264->sink);
+ VNC_DEBUG("Could not add sink to gst pipeline\n");
+ goto error;
+ }
+
+ source_caps = gst_caps_new_simple(
+ "video/x-raw",
+ "format", G_TYPE_STRING, "BGRx",
+ "framerate", GST_TYPE_FRACTION, 33, 1,
+ "width", G_TYPE_INT, w,
+ "height", G_TYPE_INT, h,
+ NULL);
+
+ if (!source_caps) {
+ VNC_DEBUG("Could not create source caps filter\n");
+ goto error;
+ }
+
+ g_object_set(vs->h264->source, "caps", source_caps, NULL);
+
+ if (gst_element_link_many(
+ vs->h264->source,
+ vs->h264->convert,
+ vs->h264->gst_encoder,
+ vs->h264->sink,
+ NULL
+ ) != TRUE) {
+ VNC_DEBUG("Elements could not be linked.\n");
+ goto error;
+ }
+
+ /* Start playing */
+ state_change_ret = gst_element_set_state(
+ vs->h264->pipeline, GST_STATE_PLAYING);
+
+ if (state_change_ret == GST_STATE_CHANGE_FAILURE) {
+ VNC_DEBUG("Unable to set the pipeline to the playing state.\n");
+ goto error;
+ }
+
+ return TRUE;
+
+ error:
+ destroy_encoder_context(vs);
+ return FALSE;
+}
+
+bool vnc_h264_encoder_init(VncState *vs)
+{
+ g_assert(vs->h264 == NULL);
+
+ vs->h264 = g_new0(VncH264, 1);
+
+ return true;
+}
+
+/*
+ * Returns the number of generated framebuffer updates,
+ * or -1 in case of errors
+ */
+int vnc_h264_send_framebuffer_update(
+ VncState *vs,
+ int _x,
+ int _y,
+ int _w,
+ int _h
+) {
+ int n = 0;
+ int rdb_h264_flags = 0;
+ int width, height;
+ uint8_t *src_data_ptr = NULL;
+ size_t src_data_size;
+ GstFlowReturn flow_ret = GST_FLOW_ERROR;
+ GstBuffer *src_buffer = NULL;
+
+ g_assert(vs->h264 != NULL);
+ g_assert(vs->vd != NULL);
+ g_assert(vs->vd->server != NULL);
+
+ width = pixman_image_get_width(vs->vd->server);
+ height = pixman_image_get_height(vs->vd->server);
+
+ g_assert(width == vs->client_width);
+ g_assert(height == vs->client_height);
+
+ if (vs->h264->sink) {
+ if (width != vs->h264->width || height != vs->h264->height) {
+ rdb_h264_flags = 2;
+ }
+ } else {
+ rdb_h264_flags = 2;
+ }
+
+ if (!create_encoder_context(vs, width, height)) {
+ VNC_DEBUG("Create encoder context failed\n");
+ return -1;
+ }
+
+ g_assert(vs->h264->sink != NULL);
+
+ src_data_ptr = vnc_server_fb_ptr(vs->vd, 0, 0);
+ src_data_size = width * height * VNC_SERVER_FB_BYTES;
+
+ src_buffer = gst_buffer_new_wrapped_full(
+ 0, src_data_ptr, src_data_size, 0, src_data_size, NULL, NULL);
+
+ g_signal_emit_by_name(
+ vs->h264->source, "push-buffer", src_buffer, &flow_ret);
+
+ if (flow_ret != GST_FLOW_OK) {
+ VNC_DEBUG("gst appsrc push buffer failed\n");
+ return -1;
+ }
+
+ do {
+ GstSample *sample = NULL;
+ GstMapInfo map;
+ GstBuffer *out_buffer;
+
+ /* Retrieve the buffer */
+ g_signal_emit_by_name(vs->h264->sink, "try-pull-sample", 0, &sample);
+ if (!sample) {
+ break;
+ }
+ out_buffer = gst_sample_get_buffer(sample);
+ if (gst_buffer_map(out_buffer, &map, 0)) {
+ vnc_framebuffer_update(vs, 0, 0, width, height, VNC_ENCODING_H264);
+ vnc_write_s32(vs, map.size); /* write data length */
+ vnc_write_s32(vs, rdb_h264_flags); /* write flags */
+ rdb_h264_flags = 0;
+
+ VNC_DEBUG("GST vnc_h264_update send %ld\n", map.size);
+
+ vnc_write(vs, map.data, map.size);
+
+ gst_buffer_unmap(out_buffer, &map);
+
+ n += 1;
+ } else {
+ VNC_DEBUG("unable to map sample\n");
+ }
+ gst_sample_unref(sample);
+ } while (true);
+
+ return n;
+}
+
+void vnc_h264_clear(VncState *vs)
+{
+ if (!vs->h264) {
+ return;
+ }
+
+ destroy_encoder_context(vs);
+
+ g_clear_pointer(&vs->h264, g_free);
+}
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index fcca7ec632..853a547d9a 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -193,6 +193,7 @@ static void vnc_async_encoding_start(VncState *orig, VncState *local)
local->zlib = orig->zlib;
local->hextile = orig->hextile;
local->zrle = orig->zrle;
+ local->h264 = orig->h264;
local->client_width = orig->client_width;
local->client_height = orig->client_height;
}
@@ -204,6 +205,7 @@ static void vnc_async_encoding_end(VncState *orig, VncState *local)
orig->zlib = local->zlib;
orig->hextile = local->hextile;
orig->zrle = local->zrle;
+ orig->h264 = local->h264;
orig->lossy_rect = local->lossy_rect;
}
@@ -284,25 +286,42 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
vnc_write_u16(&vs, 0);
vnc_lock_display(job->vs->vd);
- QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
- int n;
-
- if (job->vs->ioc == NULL) {
- vnc_unlock_display(job->vs->vd);
- /* Copy persistent encoding data */
- vnc_async_encoding_end(job->vs, &vs);
- goto disconnected;
- }
- if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) {
- n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y,
- entry->rect.w, entry->rect.h);
+ if (vs.vnc_encoding == VNC_ENCODING_H264) {
+ int width = pixman_image_get_width(vs.vd->server);
+ int height = pixman_image_get_height(vs.vd->server);
+ int n = vnc_send_framebuffer_update(&vs, 0, 0, width, height);
+ if (n >= 0) {
+ n_rectangles += n;
+ }
+ QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
+ g_free(entry);
+ }
+ } else {
+ QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
+ int n;
+
+ if (job->vs->ioc == NULL) {
+ vnc_unlock_display(job->vs->vd);
+ /* Copy persistent encoding data */
+ vnc_async_encoding_end(job->vs, &vs);
+ goto disconnected;
+ }
- if (n >= 0) {
- n_rectangles += n;
+ if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) {
+ n = vnc_send_framebuffer_update(
+ &vs,
+ entry->rect.x,
+ entry->rect.y,
+ entry->rect.w,
+ entry->rect.h);
+
+ if (n >= 0) {
+ n_rectangles += n;
+ }
}
+ g_free(entry);
}
- g_free(entry);
}
trace_vnc_job_nrects(&vs, job, n_rectangles);
vnc_unlock_display(job->vs->vd);
diff --git a/ui/vnc.c b/ui/vnc.c
index 9241caaad9..aed25b0183 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -972,6 +972,9 @@ int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
case VNC_ENCODING_ZYWRLE:
n = vnc_zywrle_send_framebuffer_update(vs, x, y, w, h);
break;
+ case VNC_ENCODING_H264:
+ n = vnc_h264_send_framebuffer_update(vs, x, y, w, h);
+ break;
default:
vnc_framebuffer_update(vs, x, y, w, h, VNC_ENCODING_RAW);
n = vnc_raw_send_framebuffer_update(vs, x, y, w, h);
@@ -1326,6 +1329,10 @@ void vnc_disconnect_finish(VncState *vs)
vnc_tight_clear(vs);
vnc_zrle_clear(vs);
+#ifdef CONFIG_GSTREAMER
+ vnc_h264_clear(vs);
+#endif
+
#ifdef CONFIG_VNC_SASL
vnc_sasl_client_cleanup(vs);
#endif /* CONFIG_VNC_SASL */
@@ -2181,6 +2188,16 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
vnc_set_feature(vs, VNC_FEATURE_ZYWRLE);
vs->vnc_encoding = enc;
break;
+#ifdef CONFIG_GSTREAMER
+ case VNC_ENCODING_H264:
+ if (vnc_h264_encoder_init(vs)) {
+ vnc_set_feature(vs, VNC_FEATURE_H264);
+ vs->vnc_encoding = enc;
+ } else {
+ VNC_DEBUG("vnc_h264_encoder_init failed\n");
+ }
+ break;
+#endif
case VNC_ENCODING_DESKTOPRESIZE:
vnc_set_feature(vs, VNC_FEATURE_RESIZE);
break;
@@ -4291,6 +4308,10 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
Error *local_err = NULL;
char *id = (char *)qemu_opts_id(opts);
+#ifdef CONFIG_GSTREAMER
+ gst_init(NULL, NULL);
+#endif
+
assert(id);
vnc_display_init(id, &local_err);
if (local_err) {
diff --git a/ui/vnc.h b/ui/vnc.h
index acc53a2cc1..a0d336738d 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -46,6 +46,10 @@
#include "vnc-enc-zrle.h"
#include "ui/kbd-state.h"
+#ifdef CONFIG_GSTREAMER
+#include <gst/gst.h>
+#endif
+
// #define _VNC_DEBUG 1
#ifdef _VNC_DEBUG
@@ -231,6 +235,14 @@ typedef struct VncZywrle {
int buf[VNC_ZRLE_TILE_WIDTH * VNC_ZRLE_TILE_HEIGHT];
} VncZywrle;
+#ifdef CONFIG_GSTREAMER
+typedef struct VncH264 {
+ GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
+ size_t width;
+ size_t height;
+} VncH264;
+#endif
+
struct VncRect
{
int x;
@@ -344,6 +356,9 @@ struct VncState
VncHextile hextile;
VncZrle *zrle;
VncZywrle zywrle;
+#ifdef CONFIG_GSTREAMER
+ VncH264 *h264;
+#endif
Notifier mouse_mode_notifier;
@@ -404,6 +419,7 @@ enum {
#define VNC_ENCODING_TRLE 0x0000000f
#define VNC_ENCODING_ZRLE 0x00000010
#define VNC_ENCODING_ZYWRLE 0x00000011
+#define VNC_ENCODING_H264 0x00000032 /* 50 */
#define VNC_ENCODING_COMPRESSLEVEL0 0xFFFFFF00 /* -256 */
#define VNC_ENCODING_QUALITYLEVEL0 0xFFFFFFE0 /* -32 */
#define VNC_ENCODING_XCURSOR 0xFFFFFF10 /* -240 */
@@ -464,6 +480,7 @@ enum VncFeatures {
VNC_FEATURE_XVP,
VNC_FEATURE_CLIPBOARD_EXT,
VNC_FEATURE_AUDIO,
+ VNC_FEATURE_H264,
};
@@ -625,6 +642,10 @@ int vnc_zrle_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
int vnc_zywrle_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
void vnc_zrle_clear(VncState *vs);
+bool vnc_h264_encoder_init(VncState *vs);
+int vnc_h264_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
+void vnc_h264_clear(VncState *vs);
+
/* vnc-clipboard.c */
void vnc_server_cut_text_caps(VncState *vs);
void vnc_client_cut_text(VncState *vs, size_t len, uint8_t *text);
--
2.39.5
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-18 11:29 ` [PATCH v3 2/9] add vnc h264 encoder Dietmar Maurer
@ 2025-04-19 5:24 ` Marc-André Lureau
2025-04-23 11:46 ` Dietmar Maurer
2025-04-23 12:10 ` Daniel P. Berrangé
` (2 subsequent siblings)
3 siblings, 1 reply; 47+ messages in thread
From: Marc-André Lureau @ 2025-04-19 5:24 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: qemu-devel
Hi
On Fri, Apr 18, 2025 at 3:41 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
>
> This patch implements H264 support for VNC. The RFB protocol
> extension is defined in:
>
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#open-h-264-encoding
>
> Currently the Gstreamer x264enc plugin (software encoder) is used
> to encode the video stream.
>
> The gstreamer pipe is:
>
> appsrc -> videoconvert -> x264enc -> appsink
>
> Note: videoconvert is required for RGBx to YUV420 conversion.
>
> The code still use the VNC server framebuffer change detection,
> and only encodes and sends video frames if there are changes.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/meson.build | 1 +
> ui/vnc-enc-h264.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++
> ui/vnc-jobs.c | 49 +++++---
> ui/vnc.c | 21 ++++
> ui/vnc.h | 21 ++++
> 5 files changed, 359 insertions(+), 15 deletions(-)
> create mode 100644 ui/vnc-enc-h264.c
>
> diff --git a/ui/meson.build b/ui/meson.build
> index 35fb04cadf..34f1f33699 100644
> --- a/ui/meson.build
> +++ b/ui/meson.build
> @@ -46,6 +46,7 @@ vnc_ss.add(files(
> ))
> vnc_ss.add(zlib, jpeg)
> vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
> +vnc_ss.add(when: gstreamer, if_true: files('vnc-enc-h264.c'))
> system_ss.add_all(when: [vnc, pixman], if_true: vnc_ss)
> system_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
>
> diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> new file mode 100644
> index 0000000000..3abe6a1528
> --- /dev/null
> +++ b/ui/vnc-enc-h264.c
> @@ -0,0 +1,282 @@
> +/*
> + * QEMU VNC display driver: hextile encoding
> + *
> + * Copyright (C) 2025 Proxmox Server Solutions GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "vnc.h"
> +
> +#include <gst/gst.h>
> +
> +static void destroy_encoder_context(VncState *vs)
> +{
> + gst_clear_object(&vs->h264->source);
> + gst_clear_object(&vs->h264->convert);
> + gst_clear_object(&vs->h264->gst_encoder);
> + gst_clear_object(&vs->h264->sink);
> + gst_clear_object(&vs->h264->pipeline);
> +}
> +
> +static bool create_encoder_context(VncState *vs, int w, int h)
> +{
> + g_autoptr(GstCaps) source_caps = NULL;
> + GstStateChangeReturn state_change_ret;
> +
> + g_assert(vs->h264 != NULL);
> +
> + if (vs->h264->sink) {
> + if (w != vs->h264->width || h != vs->h264->height) {
> + destroy_encoder_context(vs);
> + }
> + }
> +
> + if (vs->h264->sink) {
> + return TRUE;
> + }
I suggest to move that condition in the previous block (.. else {
return TRUE; }) for readibility
> +
> + vs->h264->width = w;
> + vs->h264->height = h;
> +
> + vs->h264->source = gst_element_factory_make("appsrc", "source");
> + if (!vs->h264->source) {
> + VNC_DEBUG("Could not create gst source\n");
> + goto error;
> + }
> +
> + vs->h264->convert = gst_element_factory_make("videoconvert", "convert");
> + if (!vs->h264->convert) {
> + VNC_DEBUG("Could not create gst convert element\n");
> + goto error;
> + }
> +
> + vs->h264->gst_encoder = gst_element_factory_make("x264enc", "gst-encoder");
> + if (!vs->h264->gst_encoder) {
> + VNC_DEBUG("Could not create gst x264 encoder\n");
> + goto error;
> + }
> +
> + g_object_set(
> + vs->h264->gst_encoder,
> + "tune", 4, /* zerolatency */
> + /*
> + * fix for zerolatency with novnc (without, noVNC displays
> + * green stripes)
> + */
> + "threads", 1,
> + "pass", 5, /* Constant Quality */
> + "quantizer", 26,
> + /* avoid access unit delimiters (Nal Unit Type 9) - not required */
> + "aud", false,
> + NULL);
> +
> + vs->h264->sink = gst_element_factory_make("appsink", "sink");
> + if (!vs->h264->sink) {
> + VNC_DEBUG("Could not create gst sink\n");
> + goto error;
> + }
> +
> + vs->h264->pipeline = gst_pipeline_new("vnc-h264-pipeline");
> + if (!vs->h264->pipeline) {
> + VNC_DEBUG("Could not create gst pipeline\n");
> + goto error;
> + }
> +
> + gst_object_ref(vs->h264->source);
> + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->source)) {
> + gst_object_unref(vs->h264->source);
I think it's a bit unnecessary to ref/unref each element, this is not
typically what the gst API recommends. See for ex
https://gstreamer.freedesktop.org/documentation/tutorials/basic/dynamic-pipelines.html?gi-language=c.
They rely on weak pointers.
> + VNC_DEBUG("Could not add source to gst pipeline\n");
> + goto error;
> + }
> +
> + gst_object_ref(vs->h264->convert);
> + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->convert)) {
Can you use gst_bin_add_many() ?
> + gst_object_unref(vs->h264->convert);
> + VNC_DEBUG("Could not add convert to gst pipeline\n");
> + goto error;
> + }
> +
> + gst_object_ref(vs->h264->gst_encoder);
> + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->gst_encoder)) {
> + gst_object_unref(vs->h264->gst_encoder);
> + VNC_DEBUG("Could not add encoder to gst pipeline\n");
> + goto error;
> + }
> +
> + gst_object_ref(vs->h264->sink);
> + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->sink)) {
> + gst_object_unref(vs->h264->sink);
> + VNC_DEBUG("Could not add sink to gst pipeline\n");
> + goto error;
> + }
> +
> + source_caps = gst_caps_new_simple(
> + "video/x-raw",
> + "format", G_TYPE_STRING, "BGRx",
> + "framerate", GST_TYPE_FRACTION, 33, 1,
> + "width", G_TYPE_INT, w,
> + "height", G_TYPE_INT, h,
> + NULL);
> +
> + if (!source_caps) {
> + VNC_DEBUG("Could not create source caps filter\n");
> + goto error;
> + }
> +
> + g_object_set(vs->h264->source, "caps", source_caps, NULL);
> +
> + if (gst_element_link_many(
> + vs->h264->source,
> + vs->h264->convert,
> + vs->h264->gst_encoder,
> + vs->h264->sink,
> + NULL
> + ) != TRUE) {
> + VNC_DEBUG("Elements could not be linked.\n");
> + goto error;
> + }
> +
> + /* Start playing */
> + state_change_ret = gst_element_set_state(
> + vs->h264->pipeline, GST_STATE_PLAYING);
> +
> + if (state_change_ret == GST_STATE_CHANGE_FAILURE) {
> + VNC_DEBUG("Unable to set the pipeline to the playing state.\n");
> + goto error;
> + }
> +
> + return TRUE;
> +
> + error:
> + destroy_encoder_context(vs);
> + return FALSE;
> +}
> +
> +bool vnc_h264_encoder_init(VncState *vs)
> +{
> + g_assert(vs->h264 == NULL);
> +
> + vs->h264 = g_new0(VncH264, 1);
> +
> + return true;
> +}
> +
> +/*
> + * Returns the number of generated framebuffer updates,
> + * or -1 in case of errors
> + */
> +int vnc_h264_send_framebuffer_update(
> + VncState *vs,
> + int _x,
> + int _y,
> + int _w,
> + int _h
> +) {
> + int n = 0;
> + int rdb_h264_flags = 0;
> + int width, height;
> + uint8_t *src_data_ptr = NULL;
> + size_t src_data_size;
> + GstFlowReturn flow_ret = GST_FLOW_ERROR;
> + GstBuffer *src_buffer = NULL;
> +
> + g_assert(vs->h264 != NULL);
> + g_assert(vs->vd != NULL);
> + g_assert(vs->vd->server != NULL);
> +
> + width = pixman_image_get_width(vs->vd->server);
> + height = pixman_image_get_height(vs->vd->server);
> +
> + g_assert(width == vs->client_width);
> + g_assert(height == vs->client_height);
> +
> + if (vs->h264->sink) {
> + if (width != vs->h264->width || height != vs->h264->height) {
> + rdb_h264_flags = 2;
> + }
> + } else {
> + rdb_h264_flags = 2;
> + }
> +
> + if (!create_encoder_context(vs, width, height)) {
> + VNC_DEBUG("Create encoder context failed\n");
> + return -1;
> + }
> +
> + g_assert(vs->h264->sink != NULL);
> +
> + src_data_ptr = vnc_server_fb_ptr(vs->vd, 0, 0);
> + src_data_size = width * height * VNC_SERVER_FB_BYTES;
> +
> + src_buffer = gst_buffer_new_wrapped_full(
> + 0, src_data_ptr, src_data_size, 0, src_data_size, NULL, NULL);
> +
> + g_signal_emit_by_name(
> + vs->h264->source, "push-buffer", src_buffer, &flow_ret);
> +
> + if (flow_ret != GST_FLOW_OK) {
> + VNC_DEBUG("gst appsrc push buffer failed\n");
> + return -1;
> + }
> +
> + do {
> + GstSample *sample = NULL;
> + GstMapInfo map;
> + GstBuffer *out_buffer;
> +
> + /* Retrieve the buffer */
> + g_signal_emit_by_name(vs->h264->sink, "try-pull-sample", 0, &sample);
> + if (!sample) {
> + break;
> + }
> + out_buffer = gst_sample_get_buffer(sample);
> + if (gst_buffer_map(out_buffer, &map, 0)) {
> + vnc_framebuffer_update(vs, 0, 0, width, height, VNC_ENCODING_H264);
> + vnc_write_s32(vs, map.size); /* write data length */
> + vnc_write_s32(vs, rdb_h264_flags); /* write flags */
> + rdb_h264_flags = 0;
> +
> + VNC_DEBUG("GST vnc_h264_update send %ld\n", map.size);
> +
> + vnc_write(vs, map.data, map.size);
> +
> + gst_buffer_unmap(out_buffer, &map);
> +
> + n += 1;
> + } else {
> + VNC_DEBUG("unable to map sample\n");
> + }
> + gst_sample_unref(sample);
> + } while (true);
> +
> + return n;
> +}
> +
> +void vnc_h264_clear(VncState *vs)
> +{
> + if (!vs->h264) {
> + return;
> + }
unnecessary
> +
> + destroy_encoder_context(vs);
> +
> + g_clear_pointer(&vs->h264, g_free);
> +}
> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> index fcca7ec632..853a547d9a 100644
> --- a/ui/vnc-jobs.c
> +++ b/ui/vnc-jobs.c
> @@ -193,6 +193,7 @@ static void vnc_async_encoding_start(VncState *orig, VncState *local)
> local->zlib = orig->zlib;
> local->hextile = orig->hextile;
> local->zrle = orig->zrle;
> + local->h264 = orig->h264;
> local->client_width = orig->client_width;
> local->client_height = orig->client_height;
> }
> @@ -204,6 +205,7 @@ static void vnc_async_encoding_end(VncState *orig, VncState *local)
> orig->zlib = local->zlib;
> orig->hextile = local->hextile;
> orig->zrle = local->zrle;
> + orig->h264 = local->h264;
> orig->lossy_rect = local->lossy_rect;
> }
>
> @@ -284,25 +286,42 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
> vnc_write_u16(&vs, 0);
>
> vnc_lock_display(job->vs->vd);
> - QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> - int n;
> -
> - if (job->vs->ioc == NULL) {
> - vnc_unlock_display(job->vs->vd);
> - /* Copy persistent encoding data */
> - vnc_async_encoding_end(job->vs, &vs);
> - goto disconnected;
> - }
>
> - if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) {
> - n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y,
> - entry->rect.w, entry->rect.h);
> + if (vs.vnc_encoding == VNC_ENCODING_H264) {
> + int width = pixman_image_get_width(vs.vd->server);
> + int height = pixman_image_get_height(vs.vd->server);
> + int n = vnc_send_framebuffer_update(&vs, 0, 0, width, height);
> + if (n >= 0) {
> + n_rectangles += n;
> + }
> + QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> + g_free(entry);
> + }
> + } else {
> + QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> + int n;
> +
> + if (job->vs->ioc == NULL) {
> + vnc_unlock_display(job->vs->vd);
> + /* Copy persistent encoding data */
> + vnc_async_encoding_end(job->vs, &vs);
> + goto disconnected;
> + }
>
> - if (n >= 0) {
> - n_rectangles += n;
> + if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) {
> + n = vnc_send_framebuffer_update(
> + &vs,
> + entry->rect.x,
> + entry->rect.y,
> + entry->rect.w,
> + entry->rect.h);
> +
> + if (n >= 0) {
> + n_rectangles += n;
> + }
> }
> + g_free(entry);
> }
> - g_free(entry);
> }
> trace_vnc_job_nrects(&vs, job, n_rectangles);
> vnc_unlock_display(job->vs->vd);
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 9241caaad9..aed25b0183 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -972,6 +972,9 @@ int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
> case VNC_ENCODING_ZYWRLE:
> n = vnc_zywrle_send_framebuffer_update(vs, x, y, w, h);
> break;
> + case VNC_ENCODING_H264:
> + n = vnc_h264_send_framebuffer_update(vs, x, y, w, h);
> + break;
> default:
> vnc_framebuffer_update(vs, x, y, w, h, VNC_ENCODING_RAW);
> n = vnc_raw_send_framebuffer_update(vs, x, y, w, h);
> @@ -1326,6 +1329,10 @@ void vnc_disconnect_finish(VncState *vs)
> vnc_tight_clear(vs);
> vnc_zrle_clear(vs);
>
> +#ifdef CONFIG_GSTREAMER
> + vnc_h264_clear(vs);
> +#endif
> +
> #ifdef CONFIG_VNC_SASL
> vnc_sasl_client_cleanup(vs);
> #endif /* CONFIG_VNC_SASL */
> @@ -2181,6 +2188,16 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
> vnc_set_feature(vs, VNC_FEATURE_ZYWRLE);
> vs->vnc_encoding = enc;
> break;
> +#ifdef CONFIG_GSTREAMER
> + case VNC_ENCODING_H264:
> + if (vnc_h264_encoder_init(vs)) {
> + vnc_set_feature(vs, VNC_FEATURE_H264);
> + vs->vnc_encoding = enc;
> + } else {
> + VNC_DEBUG("vnc_h264_encoder_init failed\n");
> + }
> + break;
> +#endif
> case VNC_ENCODING_DESKTOPRESIZE:
> vnc_set_feature(vs, VNC_FEATURE_RESIZE);
> break;
> @@ -4291,6 +4308,10 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> Error *local_err = NULL;
> char *id = (char *)qemu_opts_id(opts);
>
> +#ifdef CONFIG_GSTREAMER
> + gst_init(NULL, NULL);
> +#endif
> +
> assert(id);
> vnc_display_init(id, &local_err);
> if (local_err) {
> diff --git a/ui/vnc.h b/ui/vnc.h
> index acc53a2cc1..a0d336738d 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -46,6 +46,10 @@
> #include "vnc-enc-zrle.h"
> #include "ui/kbd-state.h"
>
> +#ifdef CONFIG_GSTREAMER
> +#include <gst/gst.h>
> +#endif
> +
> // #define _VNC_DEBUG 1
>
> #ifdef _VNC_DEBUG
> @@ -231,6 +235,14 @@ typedef struct VncZywrle {
> int buf[VNC_ZRLE_TILE_WIDTH * VNC_ZRLE_TILE_HEIGHT];
> } VncZywrle;
>
> +#ifdef CONFIG_GSTREAMER
> +typedef struct VncH264 {
> + GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
> + size_t width;
> + size_t height;
> +} VncH264;
> +#endif
> +
> struct VncRect
> {
> int x;
> @@ -344,6 +356,9 @@ struct VncState
> VncHextile hextile;
> VncZrle *zrle;
> VncZywrle zywrle;
> +#ifdef CONFIG_GSTREAMER
> + VncH264 *h264;
> +#endif
>
> Notifier mouse_mode_notifier;
>
> @@ -404,6 +419,7 @@ enum {
> #define VNC_ENCODING_TRLE 0x0000000f
> #define VNC_ENCODING_ZRLE 0x00000010
> #define VNC_ENCODING_ZYWRLE 0x00000011
> +#define VNC_ENCODING_H264 0x00000032 /* 50 */
> #define VNC_ENCODING_COMPRESSLEVEL0 0xFFFFFF00 /* -256 */
> #define VNC_ENCODING_QUALITYLEVEL0 0xFFFFFFE0 /* -32 */
> #define VNC_ENCODING_XCURSOR 0xFFFFFF10 /* -240 */
> @@ -464,6 +480,7 @@ enum VncFeatures {
> VNC_FEATURE_XVP,
> VNC_FEATURE_CLIPBOARD_EXT,
> VNC_FEATURE_AUDIO,
> + VNC_FEATURE_H264,
> };
>
>
> @@ -625,6 +642,10 @@ int vnc_zrle_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> int vnc_zywrle_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> void vnc_zrle_clear(VncState *vs);
>
> +bool vnc_h264_encoder_init(VncState *vs);
> +int vnc_h264_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> +void vnc_h264_clear(VncState *vs);
> +
> /* vnc-clipboard.c */
> void vnc_server_cut_text_caps(VncState *vs);
> void vnc_client_cut_text(VncState *vs, size_t len, uint8_t *text);
> --
> 2.39.5
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-19 5:24 ` Marc-André Lureau
@ 2025-04-23 11:46 ` Dietmar Maurer
2025-04-23 11:57 ` Marc-André Lureau
0 siblings, 1 reply; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-23 11:46 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
> On 19.4.2025 07:24 CEST Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
>
> Hi
>
> On Fri, Apr 18, 2025 at 3:41 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
> >
> > This patch implements H264 support for VNC. The RFB protocol
> > extension is defined in:
> >
> > https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#open-h-264-encoding
> >
> > Currently the Gstreamer x264enc plugin (software encoder) is used
> > to encode the video stream.
> >
> > The gstreamer pipe is:
> >
> > appsrc -> videoconvert -> x264enc -> appsink
> >
> > Note: videoconvert is required for RGBx to YUV420 conversion.
> >
> > The code still use the VNC server framebuffer change detection,
> > and only encodes and sends video frames if there are changes.
> >
> > Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> > ---
> > ui/meson.build | 1 +
> > ui/vnc-enc-h264.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++
> > ui/vnc-jobs.c | 49 +++++---
> > ui/vnc.c | 21 ++++
> > ui/vnc.h | 21 ++++
> > 5 files changed, 359 insertions(+), 15 deletions(-)
> > create mode 100644 ui/vnc-enc-h264.c
> >
> > diff --git a/ui/meson.build b/ui/meson.build
> > index 35fb04cadf..34f1f33699 100644
> > --- a/ui/meson.build
> > +++ b/ui/meson.build
> > @@ -46,6 +46,7 @@ vnc_ss.add(files(
> > ))
> > vnc_ss.add(zlib, jpeg)
> > vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
> > +vnc_ss.add(when: gstreamer, if_true: files('vnc-enc-h264.c'))
> > system_ss.add_all(when: [vnc, pixman], if_true: vnc_ss)
> > system_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
> >
> > diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> > new file mode 100644
> > index 0000000000..3abe6a1528
> > --- /dev/null
> > +++ b/ui/vnc-enc-h264.c
> > @@ -0,0 +1,282 @@
> > +/*
> > + * QEMU VNC display driver: hextile encoding
> > + *
> > + * Copyright (C) 2025 Proxmox Server Solutions GmbH
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to deal
> > + * in the Software without restriction, including without limitation the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "vnc.h"
> > +
> > +#include <gst/gst.h>
> > +
> > +static void destroy_encoder_context(VncState *vs)
> > +{
> > + gst_clear_object(&vs->h264->source);
> > + gst_clear_object(&vs->h264->convert);
> > + gst_clear_object(&vs->h264->gst_encoder);
> > + gst_clear_object(&vs->h264->sink);
> > + gst_clear_object(&vs->h264->pipeline);
> > +}
> > +
> > +static bool create_encoder_context(VncState *vs, int w, int h)
> > +{
> > + g_autoptr(GstCaps) source_caps = NULL;
> > + GstStateChangeReturn state_change_ret;
> > +
> > + g_assert(vs->h264 != NULL);
> > +
> > + if (vs->h264->sink) {
> > + if (w != vs->h264->width || h != vs->h264->height) {
> > + destroy_encoder_context(vs);
> > + }
> > + }
> > +
> > + if (vs->h264->sink) {
> > + return TRUE;
> > + }
>
> I suggest to move that condition in the previous block (.. else {
> return TRUE; }) for readibility
Sorry, but this would change the semantics totally.
Please note that destroy_encoder_context() can set
sink to NULL.
>
> > +
> > + vs->h264->width = w;
> > + vs->h264->height = h;
> > +
> > + vs->h264->source = gst_element_factory_make("appsrc", "source");
> > + if (!vs->h264->source) {
> > + VNC_DEBUG("Could not create gst source\n");
> > + goto error;
> > + }
> > +
> > + vs->h264->convert = gst_element_factory_make("videoconvert", "convert");
> > + if (!vs->h264->convert) {
> > + VNC_DEBUG("Could not create gst convert element\n");
> > + goto error;
> > + }
> > +
> > + vs->h264->gst_encoder = gst_element_factory_make("x264enc", "gst-encoder");
> > + if (!vs->h264->gst_encoder) {
> > + VNC_DEBUG("Could not create gst x264 encoder\n");
> > + goto error;
> > + }
> > +
> > + g_object_set(
> > + vs->h264->gst_encoder,
> > + "tune", 4, /* zerolatency */
> > + /*
> > + * fix for zerolatency with novnc (without, noVNC displays
> > + * green stripes)
> > + */
> > + "threads", 1,
> > + "pass", 5, /* Constant Quality */
> > + "quantizer", 26,
> > + /* avoid access unit delimiters (Nal Unit Type 9) - not required */
> > + "aud", false,
> > + NULL);
> > +
> > + vs->h264->sink = gst_element_factory_make("appsink", "sink");
> > + if (!vs->h264->sink) {
> > + VNC_DEBUG("Could not create gst sink\n");
> > + goto error;
> > + }
> > +
> > + vs->h264->pipeline = gst_pipeline_new("vnc-h264-pipeline");
> > + if (!vs->h264->pipeline) {
> > + VNC_DEBUG("Could not create gst pipeline\n");
> > + goto error;
> > + }
> > +
> > + gst_object_ref(vs->h264->source);
> > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->source)) {
> > + gst_object_unref(vs->h264->source);
>
> I think it's a bit unnecessary to ref/unref each element, this is not
> typically what the gst API recommends. See for ex
> https://gstreamer.freedesktop.org/documentation/tutorials/basic/dynamic-pipelines.html?gi-language=c.
> They rely on weak pointers.
I cant see whats better there, or whats exactly the problem with correct reference counting?
> > + VNC_DEBUG("Could not add source to gst pipeline\n");
> > + goto error;
> > + }
> > +
> > + gst_object_ref(vs->h264->convert);
> > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->convert)) {
>
> Can you use gst_bin_add_many() ?
will try to use that.
>
> > + gst_object_unref(vs->h264->convert);
> > + VNC_DEBUG("Could not add convert to gst pipeline\n");
> > + goto error;
> > + }
> > +
> > + gst_object_ref(vs->h264->gst_encoder);
> > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->gst_encoder)) {
> > + gst_object_unref(vs->h264->gst_encoder);
> > + VNC_DEBUG("Could not add encoder to gst pipeline\n");
> > + goto error;
> > + }
> > +
> > + gst_object_ref(vs->h264->sink);
> > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->sink)) {
> > + gst_object_unref(vs->h264->sink);
> > + VNC_DEBUG("Could not add sink to gst pipeline\n");
> > + goto error;
> > + }
> > +
> > + source_caps = gst_caps_new_simple(
> > + "video/x-raw",
> > + "format", G_TYPE_STRING, "BGRx",
> > + "framerate", GST_TYPE_FRACTION, 33, 1,
> > + "width", G_TYPE_INT, w,
> > + "height", G_TYPE_INT, h,
> > + NULL);
> > +
> > + if (!source_caps) {
> > + VNC_DEBUG("Could not create source caps filter\n");
> > + goto error;
> > + }
> > +
> > + g_object_set(vs->h264->source, "caps", source_caps, NULL);
> > +
> > + if (gst_element_link_many(
> > + vs->h264->source,
> > + vs->h264->convert,
> > + vs->h264->gst_encoder,
> > + vs->h264->sink,
> > + NULL
> > + ) != TRUE) {
> > + VNC_DEBUG("Elements could not be linked.\n");
> > + goto error;
> > + }
> > +
> > + /* Start playing */
> > + state_change_ret = gst_element_set_state(
> > + vs->h264->pipeline, GST_STATE_PLAYING);
> > +
> > + if (state_change_ret == GST_STATE_CHANGE_FAILURE) {
> > + VNC_DEBUG("Unable to set the pipeline to the playing state.\n");
> > + goto error;
> > + }
> > +
> > + return TRUE;
> > +
> > + error:
> > + destroy_encoder_context(vs);
> > + return FALSE;
> > +}
> > +
> > +bool vnc_h264_encoder_init(VncState *vs)
> > +{
> > + g_assert(vs->h264 == NULL);
> > +
> > + vs->h264 = g_new0(VncH264, 1);
> > +
> > + return true;
> > +}
> > +
> > +/*
> > + * Returns the number of generated framebuffer updates,
> > + * or -1 in case of errors
> > + */
> > +int vnc_h264_send_framebuffer_update(
> > + VncState *vs,
> > + int _x,
> > + int _y,
> > + int _w,
> > + int _h
> > +) {
> > + int n = 0;
> > + int rdb_h264_flags = 0;
> > + int width, height;
> > + uint8_t *src_data_ptr = NULL;
> > + size_t src_data_size;
> > + GstFlowReturn flow_ret = GST_FLOW_ERROR;
> > + GstBuffer *src_buffer = NULL;
> > +
> > + g_assert(vs->h264 != NULL);
> > + g_assert(vs->vd != NULL);
> > + g_assert(vs->vd->server != NULL);
> > +
> > + width = pixman_image_get_width(vs->vd->server);
> > + height = pixman_image_get_height(vs->vd->server);
> > +
> > + g_assert(width == vs->client_width);
> > + g_assert(height == vs->client_height);
> > +
> > + if (vs->h264->sink) {
> > + if (width != vs->h264->width || height != vs->h264->height) {
> > + rdb_h264_flags = 2;
> > + }
> > + } else {
> > + rdb_h264_flags = 2;
> > + }
> > +
> > + if (!create_encoder_context(vs, width, height)) {
> > + VNC_DEBUG("Create encoder context failed\n");
> > + return -1;
> > + }
> > +
> > + g_assert(vs->h264->sink != NULL);
> > +
> > + src_data_ptr = vnc_server_fb_ptr(vs->vd, 0, 0);
> > + src_data_size = width * height * VNC_SERVER_FB_BYTES;
> > +
> > + src_buffer = gst_buffer_new_wrapped_full(
> > + 0, src_data_ptr, src_data_size, 0, src_data_size, NULL, NULL);
> > +
> > + g_signal_emit_by_name(
> > + vs->h264->source, "push-buffer", src_buffer, &flow_ret);
> > +
> > + if (flow_ret != GST_FLOW_OK) {
> > + VNC_DEBUG("gst appsrc push buffer failed\n");
> > + return -1;
> > + }
> > +
> > + do {
> > + GstSample *sample = NULL;
> > + GstMapInfo map;
> > + GstBuffer *out_buffer;
> > +
> > + /* Retrieve the buffer */
> > + g_signal_emit_by_name(vs->h264->sink, "try-pull-sample", 0, &sample);
> > + if (!sample) {
> > + break;
> > + }
> > + out_buffer = gst_sample_get_buffer(sample);
> > + if (gst_buffer_map(out_buffer, &map, 0)) {
> > + vnc_framebuffer_update(vs, 0, 0, width, height, VNC_ENCODING_H264);
> > + vnc_write_s32(vs, map.size); /* write data length */
> > + vnc_write_s32(vs, rdb_h264_flags); /* write flags */
> > + rdb_h264_flags = 0;
> > +
> > + VNC_DEBUG("GST vnc_h264_update send %ld\n", map.size);
> > +
> > + vnc_write(vs, map.data, map.size);
> > +
> > + gst_buffer_unmap(out_buffer, &map);
> > +
> > + n += 1;
> > + } else {
> > + VNC_DEBUG("unable to map sample\n");
> > + }
> > + gst_sample_unref(sample);
> > + } while (true);
> > +
> > + return n;
> > +}
> > +
> > +void vnc_h264_clear(VncState *vs)
> > +{
> > + if (!vs->h264) {
> > + return;
> > + }
>
> unnecessary
This is required. For example if you disable h264, vs->h264 is
always NULL, and we unconditionally call vnc_h264_clear().
Why do you think this is unnecessary?
>
> > +
> > + destroy_encoder_context(vs);
> > +
> > + g_clear_pointer(&vs->h264, g_free);
> > +}
> > diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> > index fcca7ec632..853a547d9a 100644
> > --- a/ui/vnc-jobs.c
> > +++ b/ui/vnc-jobs.c
> > @@ -193,6 +193,7 @@ static void vnc_async_encoding_start(VncState *orig, VncState *local)
> > local->zlib = orig->zlib;
> > local->hextile = orig->hextile;
> > local->zrle = orig->zrle;
> > + local->h264 = orig->h264;
> > local->client_width = orig->client_width;
> > local->client_height = orig->client_height;
> > }
> > @@ -204,6 +205,7 @@ static void vnc_async_encoding_end(VncState *orig, VncState *local)
> > orig->zlib = local->zlib;
> > orig->hextile = local->hextile;
> > orig->zrle = local->zrle;
> > + orig->h264 = local->h264;
> > orig->lossy_rect = local->lossy_rect;
> > }
> >
> > @@ -284,25 +286,42 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
> > vnc_write_u16(&vs, 0);
> >
> > vnc_lock_display(job->vs->vd);
> > - QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> > - int n;
> > -
> > - if (job->vs->ioc == NULL) {
> > - vnc_unlock_display(job->vs->vd);
> > - /* Copy persistent encoding data */
> > - vnc_async_encoding_end(job->vs, &vs);
> > - goto disconnected;
> > - }
> >
> > - if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) {
> > - n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y,
> > - entry->rect.w, entry->rect.h);
> > + if (vs.vnc_encoding == VNC_ENCODING_H264) {
> > + int width = pixman_image_get_width(vs.vd->server);
> > + int height = pixman_image_get_height(vs.vd->server);
> > + int n = vnc_send_framebuffer_update(&vs, 0, 0, width, height);
> > + if (n >= 0) {
> > + n_rectangles += n;
> > + }
> > + QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> > + g_free(entry);
> > + }
> > + } else {
> > + QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> > + int n;
> > +
> > + if (job->vs->ioc == NULL) {
> > + vnc_unlock_display(job->vs->vd);
> > + /* Copy persistent encoding data */
> > + vnc_async_encoding_end(job->vs, &vs);
> > + goto disconnected;
> > + }
> >
> > - if (n >= 0) {
> > - n_rectangles += n;
> > + if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) {
> > + n = vnc_send_framebuffer_update(
> > + &vs,
> > + entry->rect.x,
> > + entry->rect.y,
> > + entry->rect.w,
> > + entry->rect.h);
> > +
> > + if (n >= 0) {
> > + n_rectangles += n;
> > + }
> > }
> > + g_free(entry);
> > }
> > - g_free(entry);
> > }
> > trace_vnc_job_nrects(&vs, job, n_rectangles);
> > vnc_unlock_display(job->vs->vd);
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index 9241caaad9..aed25b0183 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -972,6 +972,9 @@ int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
> > case VNC_ENCODING_ZYWRLE:
> > n = vnc_zywrle_send_framebuffer_update(vs, x, y, w, h);
> > break;
> > + case VNC_ENCODING_H264:
> > + n = vnc_h264_send_framebuffer_update(vs, x, y, w, h);
> > + break;
> > default:
> > vnc_framebuffer_update(vs, x, y, w, h, VNC_ENCODING_RAW);
> > n = vnc_raw_send_framebuffer_update(vs, x, y, w, h);
> > @@ -1326,6 +1329,10 @@ void vnc_disconnect_finish(VncState *vs)
> > vnc_tight_clear(vs);
> > vnc_zrle_clear(vs);
> >
> > +#ifdef CONFIG_GSTREAMER
> > + vnc_h264_clear(vs);
> > +#endif
> > +
> > #ifdef CONFIG_VNC_SASL
> > vnc_sasl_client_cleanup(vs);
> > #endif /* CONFIG_VNC_SASL */
> > @@ -2181,6 +2188,16 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
> > vnc_set_feature(vs, VNC_FEATURE_ZYWRLE);
> > vs->vnc_encoding = enc;
> > break;
> > +#ifdef CONFIG_GSTREAMER
> > + case VNC_ENCODING_H264:
> > + if (vnc_h264_encoder_init(vs)) {
> > + vnc_set_feature(vs, VNC_FEATURE_H264);
> > + vs->vnc_encoding = enc;
> > + } else {
> > + VNC_DEBUG("vnc_h264_encoder_init failed\n");
> > + }
> > + break;
> > +#endif
> > case VNC_ENCODING_DESKTOPRESIZE:
> > vnc_set_feature(vs, VNC_FEATURE_RESIZE);
> > break;
> > @@ -4291,6 +4308,10 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> > Error *local_err = NULL;
> > char *id = (char *)qemu_opts_id(opts);
> >
> > +#ifdef CONFIG_GSTREAMER
> > + gst_init(NULL, NULL);
> > +#endif
> > +
> > assert(id);
> > vnc_display_init(id, &local_err);
> > if (local_err) {
> > diff --git a/ui/vnc.h b/ui/vnc.h
> > index acc53a2cc1..a0d336738d 100644
> > --- a/ui/vnc.h
> > +++ b/ui/vnc.h
> > @@ -46,6 +46,10 @@
> > #include "vnc-enc-zrle.h"
> > #include "ui/kbd-state.h"
> >
> > +#ifdef CONFIG_GSTREAMER
> > +#include <gst/gst.h>
> > +#endif
> > +
> > // #define _VNC_DEBUG 1
> >
> > #ifdef _VNC_DEBUG
> > @@ -231,6 +235,14 @@ typedef struct VncZywrle {
> > int buf[VNC_ZRLE_TILE_WIDTH * VNC_ZRLE_TILE_HEIGHT];
> > } VncZywrle;
> >
> > +#ifdef CONFIG_GSTREAMER
> > +typedef struct VncH264 {
> > + GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
> > + size_t width;
> > + size_t height;
> > +} VncH264;
> > +#endif
> > +
> > struct VncRect
> > {
> > int x;
> > @@ -344,6 +356,9 @@ struct VncState
> > VncHextile hextile;
> > VncZrle *zrle;
> > VncZywrle zywrle;
> > +#ifdef CONFIG_GSTREAMER
> > + VncH264 *h264;
> > +#endif
> >
> > Notifier mouse_mode_notifier;
> >
> > @@ -404,6 +419,7 @@ enum {
> > #define VNC_ENCODING_TRLE 0x0000000f
> > #define VNC_ENCODING_ZRLE 0x00000010
> > #define VNC_ENCODING_ZYWRLE 0x00000011
> > +#define VNC_ENCODING_H264 0x00000032 /* 50 */
> > #define VNC_ENCODING_COMPRESSLEVEL0 0xFFFFFF00 /* -256 */
> > #define VNC_ENCODING_QUALITYLEVEL0 0xFFFFFFE0 /* -32 */
> > #define VNC_ENCODING_XCURSOR 0xFFFFFF10 /* -240 */
> > @@ -464,6 +480,7 @@ enum VncFeatures {
> > VNC_FEATURE_XVP,
> > VNC_FEATURE_CLIPBOARD_EXT,
> > VNC_FEATURE_AUDIO,
> > + VNC_FEATURE_H264,
> > };
> >
> >
> > @@ -625,6 +642,10 @@ int vnc_zrle_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> > int vnc_zywrle_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> > void vnc_zrle_clear(VncState *vs);
> >
> > +bool vnc_h264_encoder_init(VncState *vs);
> > +int vnc_h264_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> > +void vnc_h264_clear(VncState *vs);
> > +
> > /* vnc-clipboard.c */
> > void vnc_server_cut_text_caps(VncState *vs);
> > void vnc_client_cut_text(VncState *vs, size_t len, uint8_t *text);
> > --
> > 2.39.5
> >
> >
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-23 11:46 ` Dietmar Maurer
@ 2025-04-23 11:57 ` Marc-André Lureau
2025-04-24 6:19 ` Dietmar Maurer
2025-04-24 9:28 ` Dietmar Maurer
0 siblings, 2 replies; 47+ messages in thread
From: Marc-André Lureau @ 2025-04-23 11:57 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: qemu-devel
Hi
On Wed, Apr 23, 2025 at 3:46 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
>
> > On 19.4.2025 07:24 CEST Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >
> >
> > Hi
> >
> > On Fri, Apr 18, 2025 at 3:41 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
> > >
> > > This patch implements H264 support for VNC. The RFB protocol
> > > extension is defined in:
> > >
> > > https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#open-h-264-encoding
> > >
> > > Currently the Gstreamer x264enc plugin (software encoder) is used
> > > to encode the video stream.
> > >
> > > The gstreamer pipe is:
> > >
> > > appsrc -> videoconvert -> x264enc -> appsink
> > >
> > > Note: videoconvert is required for RGBx to YUV420 conversion.
> > >
> > > The code still use the VNC server framebuffer change detection,
> > > and only encodes and sends video frames if there are changes.
> > >
> > > Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> > > ---
> > > ui/meson.build | 1 +
> > > ui/vnc-enc-h264.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++
> > > ui/vnc-jobs.c | 49 +++++---
> > > ui/vnc.c | 21 ++++
> > > ui/vnc.h | 21 ++++
> > > 5 files changed, 359 insertions(+), 15 deletions(-)
> > > create mode 100644 ui/vnc-enc-h264.c
> > >
> > > diff --git a/ui/meson.build b/ui/meson.build
> > > index 35fb04cadf..34f1f33699 100644
> > > --- a/ui/meson.build
> > > +++ b/ui/meson.build
> > > @@ -46,6 +46,7 @@ vnc_ss.add(files(
> > > ))
> > > vnc_ss.add(zlib, jpeg)
> > > vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
> > > +vnc_ss.add(when: gstreamer, if_true: files('vnc-enc-h264.c'))
> > > system_ss.add_all(when: [vnc, pixman], if_true: vnc_ss)
> > > system_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
> > >
> > > diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> > > new file mode 100644
> > > index 0000000000..3abe6a1528
> > > --- /dev/null
> > > +++ b/ui/vnc-enc-h264.c
> > > @@ -0,0 +1,282 @@
> > > +/*
> > > + * QEMU VNC display driver: hextile encoding
> > > + *
> > > + * Copyright (C) 2025 Proxmox Server Solutions GmbH
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > > + * of this software and associated documentation files (the "Software"), to deal
> > > + * in the Software without restriction, including without limitation the rights
> > > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> > > + * copies of the Software, and to permit persons to whom the Software is
> > > + * furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice shall be included in
> > > + * all copies or substantial portions of the Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> > > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> > > + * THE SOFTWARE.
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "vnc.h"
> > > +
> > > +#include <gst/gst.h>
> > > +
> > > +static void destroy_encoder_context(VncState *vs)
> > > +{
> > > + gst_clear_object(&vs->h264->source);
> > > + gst_clear_object(&vs->h264->convert);
> > > + gst_clear_object(&vs->h264->gst_encoder);
> > > + gst_clear_object(&vs->h264->sink);
> > > + gst_clear_object(&vs->h264->pipeline);
> > > +}
> > > +
> > > +static bool create_encoder_context(VncState *vs, int w, int h)
> > > +{
> > > + g_autoptr(GstCaps) source_caps = NULL;
> > > + GstStateChangeReturn state_change_ret;
> > > +
> > > + g_assert(vs->h264 != NULL);
> > > +
> > > + if (vs->h264->sink) {
> > > + if (w != vs->h264->width || h != vs->h264->height) {
> > > + destroy_encoder_context(vs);
> > > + }
> > > + }
> > > +
> > > + if (vs->h264->sink) {
> > > + return TRUE;
> > > + }
> >
> > I suggest to move that condition in the previous block (.. else {
> > return TRUE; }) for readibility
>
> Sorry, but this would change the semantics totally.
> Please note that destroy_encoder_context() can set
> sink to NULL.
It's not very readable, ..
if (vs->h264->sink) {
if (w != vs->h264->width || h != vs->h264->height) {
destroy_encoder_context(vs);
} else {
return TRUE;
}
}
or even better:
if (vs->h264->sink && w == vs->h264->width && h == vs->h264->height) {
return TRUE;
}
destroy_encoder_context(vs);
...
>
> >
> > > +
> > > + vs->h264->width = w;
> > > + vs->h264->height = h;
> > > +
> > > + vs->h264->source = gst_element_factory_make("appsrc", "source");
> > > + if (!vs->h264->source) {
> > > + VNC_DEBUG("Could not create gst source\n");
> > > + goto error;
> > > + }
> > > +
> > > + vs->h264->convert = gst_element_factory_make("videoconvert", "convert");
> > > + if (!vs->h264->convert) {
> > > + VNC_DEBUG("Could not create gst convert element\n");
> > > + goto error;
> > > + }
> > > +
> > > + vs->h264->gst_encoder = gst_element_factory_make("x264enc", "gst-encoder");
> > > + if (!vs->h264->gst_encoder) {
> > > + VNC_DEBUG("Could not create gst x264 encoder\n");
> > > + goto error;
> > > + }
> > > +
> > > + g_object_set(
> > > + vs->h264->gst_encoder,
> > > + "tune", 4, /* zerolatency */
> > > + /*
> > > + * fix for zerolatency with novnc (without, noVNC displays
> > > + * green stripes)
> > > + */
> > > + "threads", 1,
> > > + "pass", 5, /* Constant Quality */
> > > + "quantizer", 26,
> > > + /* avoid access unit delimiters (Nal Unit Type 9) - not required */
> > > + "aud", false,
> > > + NULL);
> > > +
> > > + vs->h264->sink = gst_element_factory_make("appsink", "sink");
> > > + if (!vs->h264->sink) {
> > > + VNC_DEBUG("Could not create gst sink\n");
> > > + goto error;
> > > + }
> > > +
> > > + vs->h264->pipeline = gst_pipeline_new("vnc-h264-pipeline");
> > > + if (!vs->h264->pipeline) {
> > > + VNC_DEBUG("Could not create gst pipeline\n");
> > > + goto error;
> > > + }
> > > +
> > > + gst_object_ref(vs->h264->source);
> > > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->source)) {
> > > + gst_object_unref(vs->h264->source);
> >
> > I think it's a bit unnecessary to ref/unref each element, this is not
> > typically what the gst API recommends. See for ex
> > https://gstreamer.freedesktop.org/documentation/tutorials/basic/dynamic-pipelines.html?gi-language=c.
> > They rely on weak pointers.
>
> I cant see whats better there, or whats exactly the problem with correct reference counting?
It's about readability, simplicity and perhaps correctness. It's
better to use gstreamer API the way it is intended to.
>
> > > + VNC_DEBUG("Could not add source to gst pipeline\n");
> > > + goto error;
> > > + }
> > > +
> > > + gst_object_ref(vs->h264->convert);
> > > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->convert)) {
> >
> > Can you use gst_bin_add_many() ?
>
> will try to use that.
thanks
>
> >
> > > + gst_object_unref(vs->h264->convert);
> > > + VNC_DEBUG("Could not add convert to gst pipeline\n");
> > > + goto error;
> > > + }
> > > +
> > > + gst_object_ref(vs->h264->gst_encoder);
> > > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->gst_encoder)) {
> > > + gst_object_unref(vs->h264->gst_encoder);
> > > + VNC_DEBUG("Could not add encoder to gst pipeline\n");
> > > + goto error;
> > > + }
> > > +
> > > + gst_object_ref(vs->h264->sink);
> > > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->sink)) {
> > > + gst_object_unref(vs->h264->sink);
> > > + VNC_DEBUG("Could not add sink to gst pipeline\n");
> > > + goto error;
> > > + }
> > > +
> > > + source_caps = gst_caps_new_simple(
> > > + "video/x-raw",
> > > + "format", G_TYPE_STRING, "BGRx",
> > > + "framerate", GST_TYPE_FRACTION, 33, 1,
> > > + "width", G_TYPE_INT, w,
> > > + "height", G_TYPE_INT, h,
> > > + NULL);
> > > +
> > > + if (!source_caps) {
> > > + VNC_DEBUG("Could not create source caps filter\n");
> > > + goto error;
> > > + }
> > > +
> > > + g_object_set(vs->h264->source, "caps", source_caps, NULL);
> > > +
> > > + if (gst_element_link_many(
> > > + vs->h264->source,
> > > + vs->h264->convert,
> > > + vs->h264->gst_encoder,
> > > + vs->h264->sink,
> > > + NULL
> > > + ) != TRUE) {
> > > + VNC_DEBUG("Elements could not be linked.\n");
> > > + goto error;
> > > + }
> > > +
> > > + /* Start playing */
> > > + state_change_ret = gst_element_set_state(
> > > + vs->h264->pipeline, GST_STATE_PLAYING);
> > > +
> > > + if (state_change_ret == GST_STATE_CHANGE_FAILURE) {
> > > + VNC_DEBUG("Unable to set the pipeline to the playing state.\n");
> > > + goto error;
> > > + }
> > > +
> > > + return TRUE;
> > > +
> > > + error:
> > > + destroy_encoder_context(vs);
> > > + return FALSE;
> > > +}
> > > +
> > > +bool vnc_h264_encoder_init(VncState *vs)
> > > +{
> > > + g_assert(vs->h264 == NULL);
> > > +
> > > + vs->h264 = g_new0(VncH264, 1);
> > > +
> > > + return true;
> > > +}
> > > +
> > > +/*
> > > + * Returns the number of generated framebuffer updates,
> > > + * or -1 in case of errors
> > > + */
> > > +int vnc_h264_send_framebuffer_update(
> > > + VncState *vs,
> > > + int _x,
> > > + int _y,
> > > + int _w,
> > > + int _h
> > > +) {
> > > + int n = 0;
> > > + int rdb_h264_flags = 0;
> > > + int width, height;
> > > + uint8_t *src_data_ptr = NULL;
> > > + size_t src_data_size;
> > > + GstFlowReturn flow_ret = GST_FLOW_ERROR;
> > > + GstBuffer *src_buffer = NULL;
> > > +
> > > + g_assert(vs->h264 != NULL);
> > > + g_assert(vs->vd != NULL);
> > > + g_assert(vs->vd->server != NULL);
> > > +
> > > + width = pixman_image_get_width(vs->vd->server);
> > > + height = pixman_image_get_height(vs->vd->server);
> > > +
> > > + g_assert(width == vs->client_width);
> > > + g_assert(height == vs->client_height);
> > > +
> > > + if (vs->h264->sink) {
> > > + if (width != vs->h264->width || height != vs->h264->height) {
> > > + rdb_h264_flags = 2;
> > > + }
> > > + } else {
> > > + rdb_h264_flags = 2;
> > > + }
> > > +
> > > + if (!create_encoder_context(vs, width, height)) {
> > > + VNC_DEBUG("Create encoder context failed\n");
> > > + return -1;
> > > + }
> > > +
> > > + g_assert(vs->h264->sink != NULL);
> > > +
> > > + src_data_ptr = vnc_server_fb_ptr(vs->vd, 0, 0);
> > > + src_data_size = width * height * VNC_SERVER_FB_BYTES;
> > > +
> > > + src_buffer = gst_buffer_new_wrapped_full(
> > > + 0, src_data_ptr, src_data_size, 0, src_data_size, NULL, NULL);
> > > +
> > > + g_signal_emit_by_name(
> > > + vs->h264->source, "push-buffer", src_buffer, &flow_ret);
> > > +
> > > + if (flow_ret != GST_FLOW_OK) {
> > > + VNC_DEBUG("gst appsrc push buffer failed\n");
> > > + return -1;
> > > + }
> > > +
> > > + do {
> > > + GstSample *sample = NULL;
> > > + GstMapInfo map;
> > > + GstBuffer *out_buffer;
> > > +
> > > + /* Retrieve the buffer */
> > > + g_signal_emit_by_name(vs->h264->sink, "try-pull-sample", 0, &sample);
> > > + if (!sample) {
> > > + break;
> > > + }
> > > + out_buffer = gst_sample_get_buffer(sample);
> > > + if (gst_buffer_map(out_buffer, &map, 0)) {
> > > + vnc_framebuffer_update(vs, 0, 0, width, height, VNC_ENCODING_H264);
> > > + vnc_write_s32(vs, map.size); /* write data length */
> > > + vnc_write_s32(vs, rdb_h264_flags); /* write flags */
> > > + rdb_h264_flags = 0;
> > > +
> > > + VNC_DEBUG("GST vnc_h264_update send %ld\n", map.size);
> > > +
> > > + vnc_write(vs, map.data, map.size);
> > > +
> > > + gst_buffer_unmap(out_buffer, &map);
> > > +
> > > + n += 1;
> > > + } else {
> > > + VNC_DEBUG("unable to map sample\n");
> > > + }
> > > + gst_sample_unref(sample);
> > > + } while (true);
> > > +
> > > + return n;
> > > +}
> > > +
> > > +void vnc_h264_clear(VncState *vs)
> > > +{
> > > + if (!vs->h264) {
> > > + return;
> > > + }
> >
> > unnecessary
>
> This is required. For example if you disable h264, vs->h264 is
> always NULL, and we unconditionally call vnc_h264_clear().
>
> Why do you think this is unnecessary?
and there are already checks for NULL, no need to do it twice, do it
where it is actually necessary.
>
> >
> > > +
> > > + destroy_encoder_context(vs);
> > > +
> > > + g_clear_pointer(&vs->h264, g_free);
> > > +}
> > > diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> > > index fcca7ec632..853a547d9a 100644
> > > --- a/ui/vnc-jobs.c
> > > +++ b/ui/vnc-jobs.c
> > > @@ -193,6 +193,7 @@ static void vnc_async_encoding_start(VncState *orig, VncState *local)
> > > local->zlib = orig->zlib;
> > > local->hextile = orig->hextile;
> > > local->zrle = orig->zrle;
> > > + local->h264 = orig->h264;
> > > local->client_width = orig->client_width;
> > > local->client_height = orig->client_height;
> > > }
> > > @@ -204,6 +205,7 @@ static void vnc_async_encoding_end(VncState *orig, VncState *local)
> > > orig->zlib = local->zlib;
> > > orig->hextile = local->hextile;
> > > orig->zrle = local->zrle;
> > > + orig->h264 = local->h264;
> > > orig->lossy_rect = local->lossy_rect;
> > > }
> > >
> > > @@ -284,25 +286,42 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
> > > vnc_write_u16(&vs, 0);
> > >
> > > vnc_lock_display(job->vs->vd);
> > > - QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> > > - int n;
> > > -
> > > - if (job->vs->ioc == NULL) {
> > > - vnc_unlock_display(job->vs->vd);
> > > - /* Copy persistent encoding data */
> > > - vnc_async_encoding_end(job->vs, &vs);
> > > - goto disconnected;
> > > - }
> > >
> > > - if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) {
> > > - n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y,
> > > - entry->rect.w, entry->rect.h);
> > > + if (vs.vnc_encoding == VNC_ENCODING_H264) {
> > > + int width = pixman_image_get_width(vs.vd->server);
> > > + int height = pixman_image_get_height(vs.vd->server);
> > > + int n = vnc_send_framebuffer_update(&vs, 0, 0, width, height);
> > > + if (n >= 0) {
> > > + n_rectangles += n;
> > > + }
> > > + QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> > > + g_free(entry);
> > > + }
> > > + } else {
> > > + QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> > > + int n;
> > > +
> > > + if (job->vs->ioc == NULL) {
> > > + vnc_unlock_display(job->vs->vd);
> > > + /* Copy persistent encoding data */
> > > + vnc_async_encoding_end(job->vs, &vs);
> > > + goto disconnected;
> > > + }
> > >
> > > - if (n >= 0) {
> > > - n_rectangles += n;
> > > + if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) {
> > > + n = vnc_send_framebuffer_update(
> > > + &vs,
> > > + entry->rect.x,
> > > + entry->rect.y,
> > > + entry->rect.w,
> > > + entry->rect.h);
> > > +
> > > + if (n >= 0) {
> > > + n_rectangles += n;
> > > + }
> > > }
> > > + g_free(entry);
> > > }
> > > - g_free(entry);
> > > }
> > > trace_vnc_job_nrects(&vs, job, n_rectangles);
> > > vnc_unlock_display(job->vs->vd);
> > > diff --git a/ui/vnc.c b/ui/vnc.c
> > > index 9241caaad9..aed25b0183 100644
> > > --- a/ui/vnc.c
> > > +++ b/ui/vnc.c
> > > @@ -972,6 +972,9 @@ int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
> > > case VNC_ENCODING_ZYWRLE:
> > > n = vnc_zywrle_send_framebuffer_update(vs, x, y, w, h);
> > > break;
> > > + case VNC_ENCODING_H264:
> > > + n = vnc_h264_send_framebuffer_update(vs, x, y, w, h);
> > > + break;
> > > default:
> > > vnc_framebuffer_update(vs, x, y, w, h, VNC_ENCODING_RAW);
> > > n = vnc_raw_send_framebuffer_update(vs, x, y, w, h);
> > > @@ -1326,6 +1329,10 @@ void vnc_disconnect_finish(VncState *vs)
> > > vnc_tight_clear(vs);
> > > vnc_zrle_clear(vs);
> > >
> > > +#ifdef CONFIG_GSTREAMER
> > > + vnc_h264_clear(vs);
> > > +#endif
> > > +
> > > #ifdef CONFIG_VNC_SASL
> > > vnc_sasl_client_cleanup(vs);
> > > #endif /* CONFIG_VNC_SASL */
> > > @@ -2181,6 +2188,16 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
> > > vnc_set_feature(vs, VNC_FEATURE_ZYWRLE);
> > > vs->vnc_encoding = enc;
> > > break;
> > > +#ifdef CONFIG_GSTREAMER
> > > + case VNC_ENCODING_H264:
> > > + if (vnc_h264_encoder_init(vs)) {
> > > + vnc_set_feature(vs, VNC_FEATURE_H264);
> > > + vs->vnc_encoding = enc;
> > > + } else {
> > > + VNC_DEBUG("vnc_h264_encoder_init failed\n");
> > > + }
> > > + break;
> > > +#endif
> > > case VNC_ENCODING_DESKTOPRESIZE:
> > > vnc_set_feature(vs, VNC_FEATURE_RESIZE);
> > > break;
> > > @@ -4291,6 +4308,10 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> > > Error *local_err = NULL;
> > > char *id = (char *)qemu_opts_id(opts);
> > >
> > > +#ifdef CONFIG_GSTREAMER
> > > + gst_init(NULL, NULL);
> > > +#endif
> > > +
> > > assert(id);
> > > vnc_display_init(id, &local_err);
> > > if (local_err) {
> > > diff --git a/ui/vnc.h b/ui/vnc.h
> > > index acc53a2cc1..a0d336738d 100644
> > > --- a/ui/vnc.h
> > > +++ b/ui/vnc.h
> > > @@ -46,6 +46,10 @@
> > > #include "vnc-enc-zrle.h"
> > > #include "ui/kbd-state.h"
> > >
> > > +#ifdef CONFIG_GSTREAMER
> > > +#include <gst/gst.h>
> > > +#endif
> > > +
> > > // #define _VNC_DEBUG 1
> > >
> > > #ifdef _VNC_DEBUG
> > > @@ -231,6 +235,14 @@ typedef struct VncZywrle {
> > > int buf[VNC_ZRLE_TILE_WIDTH * VNC_ZRLE_TILE_HEIGHT];
> > > } VncZywrle;
> > >
> > > +#ifdef CONFIG_GSTREAMER
> > > +typedef struct VncH264 {
> > > + GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
> > > + size_t width;
> > > + size_t height;
> > > +} VncH264;
> > > +#endif
> > > +
> > > struct VncRect
> > > {
> > > int x;
> > > @@ -344,6 +356,9 @@ struct VncState
> > > VncHextile hextile;
> > > VncZrle *zrle;
> > > VncZywrle zywrle;
> > > +#ifdef CONFIG_GSTREAMER
> > > + VncH264 *h264;
> > > +#endif
> > >
> > > Notifier mouse_mode_notifier;
> > >
> > > @@ -404,6 +419,7 @@ enum {
> > > #define VNC_ENCODING_TRLE 0x0000000f
> > > #define VNC_ENCODING_ZRLE 0x00000010
> > > #define VNC_ENCODING_ZYWRLE 0x00000011
> > > +#define VNC_ENCODING_H264 0x00000032 /* 50 */
> > > #define VNC_ENCODING_COMPRESSLEVEL0 0xFFFFFF00 /* -256 */
> > > #define VNC_ENCODING_QUALITYLEVEL0 0xFFFFFFE0 /* -32 */
> > > #define VNC_ENCODING_XCURSOR 0xFFFFFF10 /* -240 */
> > > @@ -464,6 +480,7 @@ enum VncFeatures {
> > > VNC_FEATURE_XVP,
> > > VNC_FEATURE_CLIPBOARD_EXT,
> > > VNC_FEATURE_AUDIO,
> > > + VNC_FEATURE_H264,
> > > };
> > >
> > >
> > > @@ -625,6 +642,10 @@ int vnc_zrle_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> > > int vnc_zywrle_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> > > void vnc_zrle_clear(VncState *vs);
> > >
> > > +bool vnc_h264_encoder_init(VncState *vs);
> > > +int vnc_h264_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> > > +void vnc_h264_clear(VncState *vs);
> > > +
> > > /* vnc-clipboard.c */
> > > void vnc_server_cut_text_caps(VncState *vs);
> > > void vnc_client_cut_text(VncState *vs, size_t len, uint8_t *text);
> > > --
> > > 2.39.5
> > >
> > >
> >
> >
> > --
> > Marc-André Lureau
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-23 11:57 ` Marc-André Lureau
@ 2025-04-24 6:19 ` Dietmar Maurer
2025-04-24 8:32 ` Daniel P. Berrangé
2025-04-24 9:28 ` Dietmar Maurer
1 sibling, 1 reply; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-24 6:19 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
> > > > + VNC_DEBUG("Could not add source to gst pipeline\n");
> > > > + goto error;
> > > > + }
> > > > +
> > > > + gst_object_ref(vs->h264->convert);
> > > > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->convert)) {
> > >
> > > Can you use gst_bin_add_many() ?
> >
> > will try to use that.
I really struggle to use those functions. Documentation states
that gst_bin_add() can fail, but gst_bin_add_many() simply ignores
the results of gst_bin_add() (explicitly stated in the docs)?
Do you really want to use gst_bin_add_many() anyways?
- Dietmar
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-23 11:57 ` Marc-André Lureau
2025-04-24 6:19 ` Dietmar Maurer
@ 2025-04-24 9:28 ` Dietmar Maurer
2025-04-24 9:34 ` Daniel P. Berrangé
1 sibling, 1 reply; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-24 9:28 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
> > > > +void vnc_h264_clear(VncState *vs)
> > > > +{
> > > > + if (!vs->h264) {
> > > > + return;
> > > > + }
> > >
> > > unnecessary
> >
> > This is required. For example if you disable h264, vs->h264 is
> > always NULL, and we unconditionally call vnc_h264_clear().
> >
> > Why do you think this is unnecessary?
>
> and there are already checks for NULL, no need to do it twice, do it
> where it is actually necessary.
There is no check in destroy_encoder_context(), so this will generate a core dump.
So what do you mean by "where it is actually necessary"?
The final code looks like:
void vnc_h264_clear(VncState *vs)
{
// Assume we remove this check ...
// if (!vs->h264) {
// return;
//}
// will trigger a core dump
notifier_remove(&vs->h264->shutdown_notifier);
// will trigger a core dump
destroy_encoder_context(vs->h264);
// will trigger a core dump
g_free(vs->h264->encoder_name);
g_clear_pointer(&vs->h264, g_free);
}
Where do you want the check for NULL exactly? At the call site?
- Dietmar
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-24 9:28 ` Dietmar Maurer
@ 2025-04-24 9:34 ` Daniel P. Berrangé
0 siblings, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-24 9:34 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: Marc-André Lureau, qemu-devel
On Thu, Apr 24, 2025 at 11:28:34AM +0200, Dietmar Maurer wrote:
> > > > > +void vnc_h264_clear(VncState *vs)
> > > > > +{
> > > > > + if (!vs->h264) {
> > > > > + return;
> > > > > + }
> > > >
> > > > unnecessary
> > >
> > > This is required. For example if you disable h264, vs->h264 is
> > > always NULL, and we unconditionally call vnc_h264_clear().
> > >
> > > Why do you think this is unnecessary?
> >
> > and there are already checks for NULL, no need to do it twice, do it
> > where it is actually necessary.
>
> There is no check in destroy_encoder_context(), so this will generate a core dump.
>
> So what do you mean by "where it is actually necessary"?
>
> The final code looks like:
>
> void vnc_h264_clear(VncState *vs)
> {
> // Assume we remove this check ...
> // if (!vs->h264) {
> // return;
> //}
>
> // will trigger a core dump
> notifier_remove(&vs->h264->shutdown_notifier);
>
> // will trigger a core dump
> destroy_encoder_context(vs->h264);
> // will trigger a core dump
> g_free(vs->h264->encoder_name);
>
> g_clear_pointer(&vs->h264, g_free);
> }
>
> Where do you want the check for NULL exactly? At the call site?
IMHO checking in vnc_h264_clear is correct - it is the expected pattern
that deallocation functions accept NULL and act as a no-op in that case.
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-18 11:29 ` [PATCH v3 2/9] add vnc h264 encoder Dietmar Maurer
2025-04-19 5:24 ` Marc-André Lureau
@ 2025-04-23 12:10 ` Daniel P. Berrangé
2025-04-23 12:25 ` Daniel P. Berrangé
2025-04-24 16:39 ` Daniel P. Berrangé
3 siblings, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-23 12:10 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Fri, Apr 18, 2025 at 01:29:46PM +0200, Dietmar Maurer wrote:
> This patch implements H264 support for VNC. The RFB protocol
> extension is defined in:
>
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#open-h-264-encoding
>
> Currently the Gstreamer x264enc plugin (software encoder) is used
> to encode the video stream.
>
> The gstreamer pipe is:
>
> appsrc -> videoconvert -> x264enc -> appsink
>
> Note: videoconvert is required for RGBx to YUV420 conversion.
>
> The code still use the VNC server framebuffer change detection,
> and only encodes and sends video frames if there are changes.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/meson.build | 1 +
> ui/vnc-enc-h264.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++
> ui/vnc-jobs.c | 49 +++++---
> ui/vnc.c | 21 ++++
> ui/vnc.h | 21 ++++
> 5 files changed, 359 insertions(+), 15 deletions(-)
> create mode 100644 ui/vnc-enc-h264.c
>
> diff --git a/ui/meson.build b/ui/meson.build
> index 35fb04cadf..34f1f33699 100644
> --- a/ui/meson.build
> +++ b/ui/meson.build
> @@ -46,6 +46,7 @@ vnc_ss.add(files(
> ))
> vnc_ss.add(zlib, jpeg)
> vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
> +vnc_ss.add(when: gstreamer, if_true: files('vnc-enc-h264.c'))
> system_ss.add_all(when: [vnc, pixman], if_true: vnc_ss)
> system_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
>
> diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> new file mode 100644
> index 0000000000..3abe6a1528
> --- /dev/null
> +++ b/ui/vnc-enc-h264.c
> @@ -0,0 +1,282 @@
> +/*
> + * QEMU VNC display driver: hextile encoding
> + *
> + * Copyright (C) 2025 Proxmox Server Solutions GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
New code is expected to use SPDX-License-Identifier, without this
boilerplate text. We'd also expect it to be GPL-2.0-or-later, and
if this is not possible for some reason (eg derived from pre-existing
code), it should be justified in the commit message.
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-18 11:29 ` [PATCH v3 2/9] add vnc h264 encoder Dietmar Maurer
2025-04-19 5:24 ` Marc-André Lureau
2025-04-23 12:10 ` Daniel P. Berrangé
@ 2025-04-23 12:25 ` Daniel P. Berrangé
2025-04-24 7:32 ` Dietmar Maurer
2025-04-24 10:39 ` Dietmar Maurer
2025-04-24 16:39 ` Daniel P. Berrangé
3 siblings, 2 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-23 12:25 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Fri, Apr 18, 2025 at 01:29:46PM +0200, Dietmar Maurer wrote:
> This patch implements H264 support for VNC. The RFB protocol
> extension is defined in:
>
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#open-h-264-encoding
>
> Currently the Gstreamer x264enc plugin (software encoder) is used
> to encode the video stream.
>
> The gstreamer pipe is:
>
> appsrc -> videoconvert -> x264enc -> appsink
>
> Note: videoconvert is required for RGBx to YUV420 conversion.
>
> The code still use the VNC server framebuffer change detection,
> and only encodes and sends video frames if there are changes.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/meson.build | 1 +
> ui/vnc-enc-h264.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++
> ui/vnc-jobs.c | 49 +++++---
> ui/vnc.c | 21 ++++
> ui/vnc.h | 21 ++++
> 5 files changed, 359 insertions(+), 15 deletions(-)
> create mode 100644 ui/vnc-enc-h264.c
> +static bool create_encoder_context(VncState *vs, int w, int h)
> +{
> + g_autoptr(GstCaps) source_caps = NULL;
> + GstStateChangeReturn state_change_ret;
> +
> + g_assert(vs->h264 != NULL);
> +
> + if (vs->h264->sink) {
> + if (w != vs->h264->width || h != vs->h264->height) {
> + destroy_encoder_context(vs);
> + }
> + }
> +
> + if (vs->h264->sink) {
> + return TRUE;
> + }
> +
> + vs->h264->width = w;
> + vs->h264->height = h;
> +
> + vs->h264->source = gst_element_factory_make("appsrc", "source");
> + if (!vs->h264->source) {
> + VNC_DEBUG("Could not create gst source\n");
> + goto error;
> + }
> +
> + vs->h264->convert = gst_element_factory_make("videoconvert", "convert");
> + if (!vs->h264->convert) {
> + VNC_DEBUG("Could not create gst convert element\n");
> + goto error;
> + }
> +
> + vs->h264->gst_encoder = gst_element_factory_make("x264enc", "gst-encoder");
> + if (!vs->h264->gst_encoder) {
> + VNC_DEBUG("Could not create gst x264 encoder\n");
> + goto error;
> + }
> +
> + g_object_set(
> + vs->h264->gst_encoder,
> + "tune", 4, /* zerolatency */
> + /*
> + * fix for zerolatency with novnc (without, noVNC displays
> + * green stripes)
> + */
> + "threads", 1,
It seems a bit dubious for QEMU to workaround VNC client bugs, as our
server should be client agnostic. Shouldn't this flaw be fixed in noVNC
instead.
> + "pass", 5, /* Constant Quality */
> + "quantizer", 26,
> + /* avoid access unit delimiters (Nal Unit Type 9) - not required */
> + "aud", false,
> + NULL);
> +
> + vs->h264->sink = gst_element_factory_make("appsink", "sink");
> + if (!vs->h264->sink) {
> + VNC_DEBUG("Could not create gst sink\n");
> + goto error;
> + }
> +
> + vs->h264->pipeline = gst_pipeline_new("vnc-h264-pipeline");
> + if (!vs->h264->pipeline) {
> + VNC_DEBUG("Could not create gst pipeline\n");
> + goto error;
> + }
> +
> + gst_object_ref(vs->h264->source);
> + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->source)) {
> + gst_object_unref(vs->h264->source);
> + VNC_DEBUG("Could not add source to gst pipeline\n");
> + goto error;
> + }
If you put the gst_object_ref call after sucessfully calling
gst_bin_add, then it wouldn't need the gst_object_unref call
on failure. Repeated many times below.
> +
> + gst_object_ref(vs->h264->convert);
> + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->convert)) {
> + gst_object_unref(vs->h264->convert);
> + VNC_DEBUG("Could not add convert to gst pipeline\n");
> + goto error;
> + }
> +
> + gst_object_ref(vs->h264->gst_encoder);
> + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->gst_encoder)) {
> + gst_object_unref(vs->h264->gst_encoder);
> + VNC_DEBUG("Could not add encoder to gst pipeline\n");
> + goto error;
> + }
> +
> + gst_object_ref(vs->h264->sink);
> + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->sink)) {
> + gst_object_unref(vs->h264->sink);
> + VNC_DEBUG("Could not add sink to gst pipeline\n");
> + goto error;
> + }
> +
> + source_caps = gst_caps_new_simple(
> + "video/x-raw",
> + "format", G_TYPE_STRING, "BGRx",
> + "framerate", GST_TYPE_FRACTION, 33, 1,
> + "width", G_TYPE_INT, w,
> + "height", G_TYPE_INT, h,
> + NULL);
> +
> + if (!source_caps) {
> + VNC_DEBUG("Could not create source caps filter\n");
> + goto error;
> + }
> +
> + g_object_set(vs->h264->source, "caps", source_caps, NULL);
> +
> + if (gst_element_link_many(
> + vs->h264->source,
> + vs->h264->convert,
> + vs->h264->gst_encoder,
> + vs->h264->sink,
> + NULL
> + ) != TRUE) {
> + VNC_DEBUG("Elements could not be linked.\n");
> + goto error;
> + }
> +
> + /* Start playing */
> + state_change_ret = gst_element_set_state(
> + vs->h264->pipeline, GST_STATE_PLAYING);
> +
> + if (state_change_ret == GST_STATE_CHANGE_FAILURE) {
> + VNC_DEBUG("Unable to set the pipeline to the playing state.\n");
> + goto error;
> + }
> +
> + return TRUE;
> +
> + error:
> + destroy_encoder_context(vs);
> + return FALSE;
> +}
> +
> +/*
> + * Returns the number of generated framebuffer updates,
> + * or -1 in case of errors
> + */
> +int vnc_h264_send_framebuffer_update(
> + VncState *vs,
> + int _x,
> + int _y,
> + int _w,
> + int _h
This line wrapping of params is not needed, put it all
on one line. Also don't use a leading underscore on
the params. Or indeed why have these params at all
if they're not going to be used ?
> +) {
> + int n = 0;
> + int rdb_h264_flags = 0;
> + int width, height;
> + uint8_t *src_data_ptr = NULL;
> + size_t src_data_size;
> + GstFlowReturn flow_ret = GST_FLOW_ERROR;
> + GstBuffer *src_buffer = NULL;
> +
> + g_assert(vs->h264 != NULL);
> + g_assert(vs->vd != NULL);
> + g_assert(vs->vd->server != NULL);
> +
> + width = pixman_image_get_width(vs->vd->server);
> + height = pixman_image_get_height(vs->vd->server);
> +
> + g_assert(width == vs->client_width);
> + g_assert(height == vs->client_height);
> +
> + if (vs->h264->sink) {
> + if (width != vs->h264->width || height != vs->h264->height) {
> + rdb_h264_flags = 2;
> + }
> + } else {
> + rdb_h264_flags = 2;
> + }
> +
> + if (!create_encoder_context(vs, width, height)) {
> + VNC_DEBUG("Create encoder context failed\n");
> + return -1;
> + }
> +
> + g_assert(vs->h264->sink != NULL);
> +
> + src_data_ptr = vnc_server_fb_ptr(vs->vd, 0, 0);
> + src_data_size = width * height * VNC_SERVER_FB_BYTES;
> +
> + src_buffer = gst_buffer_new_wrapped_full(
> + 0, src_data_ptr, src_data_size, 0, src_data_size, NULL, NULL);
> +
> + g_signal_emit_by_name(
> + vs->h264->source, "push-buffer", src_buffer, &flow_ret);
> +
> + if (flow_ret != GST_FLOW_OK) {
> + VNC_DEBUG("gst appsrc push buffer failed\n");
> + return -1;
> + }
> +
> + do {
> + GstSample *sample = NULL;
> + GstMapInfo map;
> + GstBuffer *out_buffer;
> +
> + /* Retrieve the buffer */
> + g_signal_emit_by_name(vs->h264->sink, "try-pull-sample", 0, &sample);
> + if (!sample) {
> + break;
> + }
> + out_buffer = gst_sample_get_buffer(sample);
> + if (gst_buffer_map(out_buffer, &map, 0)) {
> + vnc_framebuffer_update(vs, 0, 0, width, height, VNC_ENCODING_H264);
> + vnc_write_s32(vs, map.size); /* write data length */
> + vnc_write_s32(vs, rdb_h264_flags); /* write flags */
> + rdb_h264_flags = 0;
> +
> + VNC_DEBUG("GST vnc_h264_update send %ld\n", map.size);
> +
> + vnc_write(vs, map.data, map.size);
> +
> + gst_buffer_unmap(out_buffer, &map);
> +
> + n += 1;
> + } else {
> + VNC_DEBUG("unable to map sample\n");
> + }
> + gst_sample_unref(sample);
> + } while (true);
> +
> + return n;
> +}
> +
> +void vnc_h264_clear(VncState *vs)
> +{
> + if (!vs->h264) {
> + return;
> + }
> +
> + destroy_encoder_context(vs);
> +
> + g_clear_pointer(&vs->h264, g_free);
> +}
> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
> index fcca7ec632..853a547d9a 100644
> --- a/ui/vnc-jobs.c
> +++ b/ui/vnc-jobs.c
> @@ -193,6 +193,7 @@ static void vnc_async_encoding_start(VncState *orig, VncState *local)
> local->zlib = orig->zlib;
> local->hextile = orig->hextile;
> local->zrle = orig->zrle;
> + local->h264 = orig->h264;
> local->client_width = orig->client_width;
> local->client_height = orig->client_height;
> }
> @@ -204,6 +205,7 @@ static void vnc_async_encoding_end(VncState *orig, VncState *local)
> orig->zlib = local->zlib;
> orig->hextile = local->hextile;
> orig->zrle = local->zrle;
> + orig->h264 = local->h264;
> orig->lossy_rect = local->lossy_rect;
> }
>
> @@ -284,25 +286,42 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
> vnc_write_u16(&vs, 0);
>
> vnc_lock_display(job->vs->vd);
> - QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> - int n;
> -
> - if (job->vs->ioc == NULL) {
> - vnc_unlock_display(job->vs->vd);
> - /* Copy persistent encoding data */
> - vnc_async_encoding_end(job->vs, &vs);
> - goto disconnected;
> - }
>
> - if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) {
> - n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y,
> - entry->rect.w, entry->rect.h);
> + if (vs.vnc_encoding == VNC_ENCODING_H264) {
Having an encoding branch in this area of code looks wierd...
> + int width = pixman_image_get_width(vs.vd->server);
> + int height = pixman_image_get_height(vs.vd->server);
> + int n = vnc_send_framebuffer_update(&vs, 0, 0, width, height);
> + if (n >= 0) {
> + n_rectangles += n;
> + }
> + QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> + g_free(entry);
> + }
... and if we're not using the rectangles array, then it feels like
we should not have populated that data to begin with.
If you avoid populating the rectangle array, then the code could be
made independant of encoding. ie instead of checking encoding, check
whether the rectangles list is empty or not. ie use an empty rects
list to decide whether we're doing a full screen update or a regionwise
update.
> + } else {
> + QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) {
> + int n;
> +
> + if (job->vs->ioc == NULL) {
> + vnc_unlock_display(job->vs->vd);
> + /* Copy persistent encoding data */
> + vnc_async_encoding_end(job->vs, &vs);
> + goto disconnected;
> + }
>
> - if (n >= 0) {
> - n_rectangles += n;
> + if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) {
> + n = vnc_send_framebuffer_update(
> + &vs,
> + entry->rect.x,
> + entry->rect.y,
> + entry->rect.w,
> + entry->rect.h);
> +
> + if (n >= 0) {
> + n_rectangles += n;
> + }
> }
> + g_free(entry);
> }
> - g_free(entry);
> }
> trace_vnc_job_nrects(&vs, job, n_rectangles);
> vnc_unlock_display(job->vs->vd);
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-23 12:25 ` Daniel P. Berrangé
@ 2025-04-24 7:32 ` Dietmar Maurer
2025-04-24 8:43 ` Dietmar Maurer
2025-04-24 10:39 ` Dietmar Maurer
1 sibling, 1 reply; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-24 7:32 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: marcandre.lureau, qemu-devel
> > + gst_object_ref(vs->h264->source);
> > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->source)) {
> > + gst_object_unref(vs->h264->source);
> > + VNC_DEBUG("Could not add source to gst pipeline\n");
> > + goto error;
> > + }
>
> If you put the gst_object_ref call after sucessfully calling
> gst_bin_add, then it wouldn't need the gst_object_unref call
> on failure. Repeated many times below.
Gstreamer docs claims that gst_bin_add() takes ownership of the element. So I assumed that it unref the element in case of error.
If I do not ref the object before, this would free the object too early.
But a look at the source code of gstbin.c reveals that it does
not unref the element in case of errors, so your suggestion works.
I will change that in the next version...
- Dietmar
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-24 7:32 ` Dietmar Maurer
@ 2025-04-24 8:43 ` Dietmar Maurer
2025-04-24 8:58 ` Daniel P. Berrangé
0 siblings, 1 reply; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-24 8:43 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: marcandre.lureau, qemu-devel
> On 24.4.2025 09:32 CEST Dietmar Maurer <dietmar@proxmox.com> wrote:
>
>
> > > + gst_object_ref(vs->h264->source);
> > > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->source)) {
> > > + gst_object_unref(vs->h264->source);
> > > + VNC_DEBUG("Could not add source to gst pipeline\n");
> > > + goto error;
> > > + }
> >
> > If you put the gst_object_ref call after sucessfully calling
> > gst_bin_add, then it wouldn't need the gst_object_unref call
> > on failure. Repeated many times below.
>
> Gstreamer docs claims that gst_bin_add() takes ownership of the element. So I assumed that it unref the element in case of error.
> If I do not ref the object before, this would free the object too early.
>
> But a look at the source code of gstbin.c reveals that it does
> not unref the element in case of errors, so your suggestion works.
> I will change that in the next version...
From the gstreamer docs about refcounting and gst_bin_add:
https://gstreamer.freedesktop.org/documentation/additional/design/MT-refcounting.html?gi-language=c#refcounting1
> As soon as this function is called in a Bin, the element passed as an argument is owned by the bin and you are not allowed to access it anymore without taking a _ref() before adding it to the bin.
This clearly states we should taking a _ref() before adding it to the bin?
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-24 8:43 ` Dietmar Maurer
@ 2025-04-24 8:58 ` Daniel P. Berrangé
0 siblings, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-24 8:58 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Thu, Apr 24, 2025 at 10:43:21AM +0200, Dietmar Maurer wrote:
>
> > On 24.4.2025 09:32 CEST Dietmar Maurer <dietmar@proxmox.com> wrote:
> >
> >
> > > > + gst_object_ref(vs->h264->source);
> > > > + if (!gst_bin_add(GST_BIN(vs->h264->pipeline), vs->h264->source)) {
> > > > + gst_object_unref(vs->h264->source);
> > > > + VNC_DEBUG("Could not add source to gst pipeline\n");
> > > > + goto error;
> > > > + }
> > >
> > > If you put the gst_object_ref call after sucessfully calling
> > > gst_bin_add, then it wouldn't need the gst_object_unref call
> > > on failure. Repeated many times below.
> >
> > Gstreamer docs claims that gst_bin_add() takes ownership of the element. So I assumed that it unref the element in case of error.
> > If I do not ref the object before, this would free the object too early.
> >
> > But a look at the source code of gstbin.c reveals that it does
> > not unref the element in case of errors, so your suggestion works.
> > I will change that in the next version...
>
> From the gstreamer docs about refcounting and gst_bin_add:
>
> https://gstreamer.freedesktop.org/documentation/additional/design/MT-refcounting.html?gi-language=c#refcounting1
>
> > As soon as this function is called in a Bin, the element passed as an argument is owned by the bin and you are not allowed to access it anymore without taking a _ref() before adding it to the bin.
>
> This clearly states we should taking a _ref() before adding it to the bin?
Yes, you are correct, ignore my suggestion.
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-23 12:25 ` Daniel P. Berrangé
2025-04-24 7:32 ` Dietmar Maurer
@ 2025-04-24 10:39 ` Dietmar Maurer
2025-04-24 10:45 ` Daniel P. Berrangé
1 sibling, 1 reply; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-24 10:39 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: marcandre.lureau, qemu-devel
> > + g_object_set(
> > + vs->h264->gst_encoder,
> > + "tune", 4, /* zerolatency */
> > + /*
> > + * fix for zerolatency with novnc (without, noVNC displays
> > + * green stripes)
> > + */
> > + "threads", 1,
>
> It seems a bit dubious for QEMU to workaround VNC client bugs, as our
> server should be client agnostic. Shouldn't this flaw be fixed in noVNC
> instead.
To be more specific, it is either a x264 encoder bug, or a web
browser (VideoEncoder api) bug. You can reproduce it with noVNC.
Form what I found out, newer versions of x264 do not use the problematic mode at all (but we want to support older versions).
- Dietmar
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-24 10:39 ` Dietmar Maurer
@ 2025-04-24 10:45 ` Daniel P. Berrangé
2025-04-24 11:01 ` Dietmar Maurer
0 siblings, 1 reply; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-24 10:45 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Thu, Apr 24, 2025 at 12:39:22PM +0200, Dietmar Maurer wrote:
> > > + g_object_set(
> > > + vs->h264->gst_encoder,
> > > + "tune", 4, /* zerolatency */
> > > + /*
> > > + * fix for zerolatency with novnc (without, noVNC displays
> > > + * green stripes)
> > > + */
> > > + "threads", 1,
> >
> > It seems a bit dubious for QEMU to workaround VNC client bugs, as our
> > server should be client agnostic. Shouldn't this flaw be fixed in noVNC
> > instead.
>
> To be more specific, it is either a x264 encoder bug, or a web
> browser (VideoEncoder api) bug. You can reproduce it with noVNC.
>
> Form what I found out, newer versions of x264 do not use the problematic mode at all (but we want to support older versions).
Do you have examples of versions which are fixed vs broken ? It would be
desirable to record something, so we know in future when we can potentially
remove the workaround.
NB, QEMU only aims to support the 2 most recent releases of major distros,
so we don't need to care about arbitrarily old versions of x264
https://www.qemu.org/docs/master/about/build-platforms.html
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-24 10:45 ` Daniel P. Berrangé
@ 2025-04-24 11:01 ` Dietmar Maurer
0 siblings, 0 replies; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-24 11:01 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: marcandre.lureau, qemu-devel
> > To be more specific, it is either a x264 encoder bug, or a web
> > browser (VideoEncoder api) bug. You can reproduce it with noVNC.
> >
> > Form what I found out, newer versions of x264 do not use the problematic mode at all (but we want to support older versions).
>
> Do you have examples of versions which are fixed vs broken ? It would be
> desirable to record something, so we know in future when we can potentially
> remove the workaround.
Sorry, did not had time to debug that further ...
We are currently preparing the update to Debian Trixie, and will
test again with those versions...
> NB, QEMU only aims to support the 2 most recent releases of major distros,
> so we don't need to care about arbitrarily old versions of x264
Debian bookworm is the latest stable Debian (not an old version).
- Dietmar
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 2/9] add vnc h264 encoder
2025-04-18 11:29 ` [PATCH v3 2/9] add vnc h264 encoder Dietmar Maurer
` (2 preceding siblings ...)
2025-04-23 12:25 ` Daniel P. Berrangé
@ 2025-04-24 16:39 ` Daniel P. Berrangé
3 siblings, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-24 16:39 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Fri, Apr 18, 2025 at 01:29:46PM +0200, Dietmar Maurer wrote:
> This patch implements H264 support for VNC. The RFB protocol
> extension is defined in:
>
> https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#open-h-264-encoding
>
> Currently the Gstreamer x264enc plugin (software encoder) is used
> to encode the video stream.
>
> The gstreamer pipe is:
>
> appsrc -> videoconvert -> x264enc -> appsink
>
> Note: videoconvert is required for RGBx to YUV420 conversion.
>
> The code still use the VNC server framebuffer change detection,
> and only encodes and sends video frames if there are changes.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/meson.build | 1 +
> ui/vnc-enc-h264.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++
> ui/vnc-jobs.c | 49 +++++---
> ui/vnc.c | 21 ++++
> ui/vnc.h | 21 ++++
> 5 files changed, 359 insertions(+), 15 deletions(-)
> create mode 100644 ui/vnc-enc-h264.c
>
> diff --git a/ui/meson.build b/ui/meson.build
> index 35fb04cadf..34f1f33699 100644
> --- a/ui/meson.build
> +++ b/ui/meson.build
> @@ -46,6 +46,7 @@ vnc_ss.add(files(
> ))
> vnc_ss.add(zlib, jpeg)
> vnc_ss.add(when: sasl, if_true: files('vnc-auth-sasl.c'))
> +vnc_ss.add(when: gstreamer, if_true: files('vnc-enc-h264.c'))
> system_ss.add_all(when: [vnc, pixman], if_true: vnc_ss)
> system_ss.add(when: vnc, if_false: files('vnc-stubs.c'))
>
> diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> new file mode 100644
> index 0000000000..3abe6a1528
> --- /dev/null
> +++ b/ui/vnc-enc-h264.c
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 9241caaad9..aed25b0183 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -972,6 +972,9 @@ int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
> case VNC_ENCODING_ZYWRLE:
> n = vnc_zywrle_send_framebuffer_update(vs, x, y, w, h);
> break;
> + case VNC_ENCODING_H264:
> + n = vnc_h264_send_framebuffer_update(vs, x, y, w, h);
> + break;
Needs protecting with #ifdef CONFIG_GSTREAMER otherwise I'd
expect a linker error
> default:
> vnc_framebuffer_update(vs, x, y, w, h, VNC_ENCODING_RAW);
> n = vnc_raw_send_framebuffer_update(vs, x, y, w, h);
> @@ -1326,6 +1329,10 @@ void vnc_disconnect_finish(VncState *vs)
> vnc_tight_clear(vs);
> vnc_zrle_clear(vs);
>
> +#ifdef CONFIG_GSTREAMER
> + vnc_h264_clear(vs);
> +#endif
> +
> #ifdef CONFIG_VNC_SASL
> vnc_sasl_client_cleanup(vs);
> #endif /* CONFIG_VNC_SASL */
> @@ -2181,6 +2188,16 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
> vnc_set_feature(vs, VNC_FEATURE_ZYWRLE);
> vs->vnc_encoding = enc;
> break;
> +#ifdef CONFIG_GSTREAMER
> + case VNC_ENCODING_H264:
> + if (vnc_h264_encoder_init(vs)) {
> + vnc_set_feature(vs, VNC_FEATURE_H264);
> + vs->vnc_encoding = enc;
> + } else {
> + VNC_DEBUG("vnc_h264_encoder_init failed\n");
> + }
> + break;
> +#endif
> case VNC_ENCODING_DESKTOPRESIZE:
> vnc_set_feature(vs, VNC_FEATURE_RESIZE);
> break;
> @@ -4291,6 +4308,10 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> Error *local_err = NULL;
> char *id = (char *)qemu_opts_id(opts);
>
> +#ifdef CONFIG_GSTREAMER
> + gst_init(NULL, NULL);
> +#endif
> +
> assert(id);
> vnc_display_init(id, &local_err);
> if (local_err) {
> diff --git a/ui/vnc.h b/ui/vnc.h
> index acc53a2cc1..a0d336738d 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -46,6 +46,10 @@
> #include "vnc-enc-zrle.h"
> #include "ui/kbd-state.h"
>
> +#ifdef CONFIG_GSTREAMER
> +#include <gst/gst.h>
> +#endif
> +
> // #define _VNC_DEBUG 1
>
> #ifdef _VNC_DEBUG
> @@ -231,6 +235,14 @@ typedef struct VncZywrle {
> int buf[VNC_ZRLE_TILE_WIDTH * VNC_ZRLE_TILE_HEIGHT];
> } VncZywrle;
>
> +#ifdef CONFIG_GSTREAMER
> +typedef struct VncH264 {
> + GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
> + size_t width;
> + size_t height;
> +} VncH264;
> +#endif
> +
> struct VncRect
> {
> int x;
> @@ -344,6 +356,9 @@ struct VncState
> VncHextile hextile;
> VncZrle *zrle;
> VncZywrle zywrle;
> +#ifdef CONFIG_GSTREAMER
> + VncH264 *h264;
> +#endif
>
> Notifier mouse_mode_notifier;
>
> @@ -404,6 +419,7 @@ enum {
> #define VNC_ENCODING_TRLE 0x0000000f
> #define VNC_ENCODING_ZRLE 0x00000010
> #define VNC_ENCODING_ZYWRLE 0x00000011
> +#define VNC_ENCODING_H264 0x00000032 /* 50 */
> #define VNC_ENCODING_COMPRESSLEVEL0 0xFFFFFF00 /* -256 */
> #define VNC_ENCODING_QUALITYLEVEL0 0xFFFFFFE0 /* -32 */
> #define VNC_ENCODING_XCURSOR 0xFFFFFF10 /* -240 */
> @@ -464,6 +480,7 @@ enum VncFeatures {
> VNC_FEATURE_XVP,
> VNC_FEATURE_CLIPBOARD_EXT,
> VNC_FEATURE_AUDIO,
> + VNC_FEATURE_H264,
> };
>
>
> @@ -625,6 +642,10 @@ int vnc_zrle_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> int vnc_zywrle_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> void vnc_zrle_clear(VncState *vs);
>
> +bool vnc_h264_encoder_init(VncState *vs);
> +int vnc_h264_send_framebuffer_update(VncState *vs, int x, int y, int w, int h);
> +void vnc_h264_clear(VncState *vs);
> +
> /* vnc-clipboard.c */
> void vnc_server_cut_text_caps(VncState *vs);
> void vnc_client_cut_text(VncState *vs, size_t len, uint8_t *text);
> --
> 2.39.5
>
>
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 3/9] vnc: h264: send additional frames after the display is clean
2025-04-18 11:29 [PATCH v3 0/9] Add VNC Open H.264 Encoding Dietmar Maurer
2025-04-18 11:29 ` [PATCH v3 1/9] new configure option to enable gstreamer Dietmar Maurer
2025-04-18 11:29 ` [PATCH v3 2/9] add vnc h264 encoder Dietmar Maurer
@ 2025-04-18 11:29 ` Dietmar Maurer
2025-04-19 5:26 ` Marc-André Lureau
2025-04-23 12:39 ` Daniel P. Berrangé
2025-04-18 11:29 ` [PATCH v3 4/9] h264: search for available h264 encoder Dietmar Maurer
` (5 subsequent siblings)
8 siblings, 2 replies; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-18 11:29 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: Dietmar Maurer
The H264 implementation only sends frames when it detects changes in
the server's framebuffer. This leads to artifacts when there are no
further changes, as the internal H264 encoder may still contain data.
This patch modifies the code to send a few additional frames in such
situations to flush the H264 encoder data.
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
ui/vnc.c | 25 ++++++++++++++++++++++++-
ui/vnc.h | 3 +++
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index aed25b0183..badc7912c0 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3239,7 +3239,30 @@ static void vnc_refresh(DisplayChangeListener *dcl)
vnc_unlock_display(vd);
QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
- rects += vnc_update_client(vs, has_dirty);
+ int client_dirty = has_dirty;
+ if (vs->h264) {
+ if (client_dirty) {
+ vs->h264->keep_dirty = VNC_H264_KEEP_DIRTY;
+ } else {
+ if (vs->h264->keep_dirty > 0) {
+ client_dirty = 1;
+ vs->h264->keep_dirty--;
+ }
+ }
+ }
+
+ int count = vnc_update_client(vs, client_dirty);
+ rects += count;
+
+ if (vs->h264 && !count && vs->h264->keep_dirty) {
+ VncJob *job = vnc_job_new(vs);
+ int height = pixman_image_get_height(vd->server);
+ int width = pixman_image_get_width(vd->server);
+ vs->job_update = vs->update;
+ vs->update = VNC_STATE_UPDATE_NONE;
+ vnc_job_add_rect(job, 0, 0, width, height);
+ vnc_job_push(job);
+ }
/* vs might be free()ed here */
}
diff --git a/ui/vnc.h b/ui/vnc.h
index a0d336738d..a5ea134de8 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -236,10 +236,13 @@ typedef struct VncZywrle {
} VncZywrle;
#ifdef CONFIG_GSTREAMER
+/* Number of frames we send after the display is clean. */
+#define VNC_H264_KEEP_DIRTY 10
typedef struct VncH264 {
GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
size_t width;
size_t height;
+ guint keep_dirty;
} VncH264;
#endif
--
2.39.5
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 3/9] vnc: h264: send additional frames after the display is clean
2025-04-18 11:29 ` [PATCH v3 3/9] vnc: h264: send additional frames after the display is clean Dietmar Maurer
@ 2025-04-19 5:26 ` Marc-André Lureau
2025-04-23 12:39 ` Daniel P. Berrangé
1 sibling, 0 replies; 47+ messages in thread
From: Marc-André Lureau @ 2025-04-19 5:26 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: qemu-devel
On Fri, Apr 18, 2025 at 3:54 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
>
> The H264 implementation only sends frames when it detects changes in
> the server's framebuffer. This leads to artifacts when there are no
> further changes, as the internal H264 encoder may still contain data.
>
> This patch modifies the code to send a few additional frames in such
> situations to flush the H264 encoder data.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
(from v1)
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> ui/vnc.c | 25 ++++++++++++++++++++++++-
> ui/vnc.h | 3 +++
> 2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index aed25b0183..badc7912c0 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3239,7 +3239,30 @@ static void vnc_refresh(DisplayChangeListener *dcl)
> vnc_unlock_display(vd);
>
> QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
> - rects += vnc_update_client(vs, has_dirty);
> + int client_dirty = has_dirty;
> + if (vs->h264) {
> + if (client_dirty) {
> + vs->h264->keep_dirty = VNC_H264_KEEP_DIRTY;
> + } else {
> + if (vs->h264->keep_dirty > 0) {
> + client_dirty = 1;
> + vs->h264->keep_dirty--;
> + }
> + }
> + }
> +
> + int count = vnc_update_client(vs, client_dirty);
> + rects += count;
> +
> + if (vs->h264 && !count && vs->h264->keep_dirty) {
> + VncJob *job = vnc_job_new(vs);
> + int height = pixman_image_get_height(vd->server);
> + int width = pixman_image_get_width(vd->server);
> + vs->job_update = vs->update;
> + vs->update = VNC_STATE_UPDATE_NONE;
> + vnc_job_add_rect(job, 0, 0, width, height);
> + vnc_job_push(job);
> + }
> /* vs might be free()ed here */
> }
>
> diff --git a/ui/vnc.h b/ui/vnc.h
> index a0d336738d..a5ea134de8 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -236,10 +236,13 @@ typedef struct VncZywrle {
> } VncZywrle;
>
> #ifdef CONFIG_GSTREAMER
> +/* Number of frames we send after the display is clean. */
> +#define VNC_H264_KEEP_DIRTY 10
> typedef struct VncH264 {
> GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
> size_t width;
> size_t height;
> + guint keep_dirty;
> } VncH264;
> #endif
>
> --
> 2.39.5
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 3/9] vnc: h264: send additional frames after the display is clean
2025-04-18 11:29 ` [PATCH v3 3/9] vnc: h264: send additional frames after the display is clean Dietmar Maurer
2025-04-19 5:26 ` Marc-André Lureau
@ 2025-04-23 12:39 ` Daniel P. Berrangé
1 sibling, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-23 12:39 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Fri, Apr 18, 2025 at 01:29:47PM +0200, Dietmar Maurer wrote:
> The H264 implementation only sends frames when it detects changes in
> the server's framebuffer. This leads to artifacts when there are no
> further changes, as the internal H264 encoder may still contain data.
>
> This patch modifies the code to send a few additional frames in such
> situations to flush the H264 encoder data.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/vnc.c | 25 ++++++++++++++++++++++++-
> ui/vnc.h | 3 +++
> 2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index aed25b0183..badc7912c0 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3239,7 +3239,30 @@ static void vnc_refresh(DisplayChangeListener *dcl)
> vnc_unlock_display(vd);
>
> QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
> - rects += vnc_update_client(vs, has_dirty);
> + int client_dirty = has_dirty;
> + if (vs->h264) {
> + if (client_dirty) {
> + vs->h264->keep_dirty = VNC_H264_KEEP_DIRTY;
> + } else {
> + if (vs->h264->keep_dirty > 0) {
> + client_dirty = 1;
> + vs->h264->keep_dirty--;
> + }
> + }
> + }
> +
> + int count = vnc_update_client(vs, client_dirty);
> + rects += count;
> +
> + if (vs->h264 && !count && vs->h264->keep_dirty) {
> + VncJob *job = vnc_job_new(vs);
> + int height = pixman_image_get_height(vd->server);
> + int width = pixman_image_get_width(vd->server);
> + vs->job_update = vs->update;
> + vs->update = VNC_STATE_UPDATE_NONE;
> + vnc_job_add_rect(job, 0, 0, width, height);
> + vnc_job_push(job);
> + }
> /* vs might be free()ed here */
This comment is telling you that 'vnc_update_client' may have
released 'vs', and yet this code is referencing 'vs->...'
and so will be liable to crash.
> }
>
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 4/9] h264: search for available h264 encoder
2025-04-18 11:29 [PATCH v3 0/9] Add VNC Open H.264 Encoding Dietmar Maurer
` (2 preceding siblings ...)
2025-04-18 11:29 ` [PATCH v3 3/9] vnc: h264: send additional frames after the display is clean Dietmar Maurer
@ 2025-04-18 11:29 ` Dietmar Maurer
2025-04-23 12:43 ` Daniel P. Berrangé
2025-04-24 10:30 ` Daniel P. Berrangé
2025-04-18 11:29 ` [PATCH v3 5/9] h264: new vnc option to configure h264 at server side Dietmar Maurer
` (4 subsequent siblings)
8 siblings, 2 replies; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-18 11:29 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: Dietmar Maurer
The search list is currently hardcoded to: ["x264enc", "openh264enc"]
x264enc: is probably the best available software encoder
openh264enc: lower quality, but available on more systems.
We restrict encoders to a known list because each encoder requires
fine tuning to get reasonable/usable results.
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
ui/vnc-enc-h264.c | 89 +++++++++++++++++++++++++++++++++++++++--------
ui/vnc.h | 1 +
2 files changed, 75 insertions(+), 15 deletions(-)
diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
index 3abe6a1528..047f4a3128 100644
--- a/ui/vnc-enc-h264.c
+++ b/ui/vnc-enc-h264.c
@@ -27,6 +27,68 @@
#include <gst/gst.h>
+const char *encoder_list[] = { "x264enc", "openh264enc", NULL };
+
+static const char *get_available_encoder(void)
+{
+ int i = 0;
+ do {
+ const char *encoder_name = encoder_list[i];
+ if (encoder_name == NULL) {
+ break;
+ }
+ GstElement *element = gst_element_factory_make(
+ encoder_name, "video-encoder");
+ if (element != NULL) {
+ gst_object_unref(element);
+ return encoder_name;
+ }
+ i = i + 1;
+ } while (true);
+
+ return NULL;
+}
+
+static GstElement *create_encoder(const char *encoder_name)
+{
+ GstElement *encoder = gst_element_factory_make(
+ encoder_name, "video-encoder");
+ if (!encoder) {
+ VNC_DEBUG("Could not create gst '%s' video encoder\n", encoder_name);
+ return NULL;
+ }
+
+ if (!strcmp(encoder_name, "x264enc")) {
+ g_object_set(
+ encoder,
+ "tune", 4, /* zerolatency */
+ /*
+ * fix for zerolatency with novnc (without,
+ * noVNC displays green stripes)
+ */
+ "threads", 1,
+ "pass", 5, /* Constant Quality */
+ "quantizer", 26,
+ /* avoid access unit delimiters (Nal Unit Type 9) - not required */
+ "aud", false,
+ NULL);
+ } else if (!strcmp(encoder_name, "openh264enc")) {
+ g_object_set(
+ encoder,
+ "usage-type", 1, /* screen content */
+ "complexity", 0, /* low, high speed */
+ "rate-control", 0, /* quality mode */
+ "qp-min", 20,
+ "qp-max", 27,
+ NULL);
+ } else {
+ VNC_DEBUG("Unknown H264 encoder name '%s' - not setting any properties",
+ encoder_name);
+ }
+
+ return encoder;
+}
+
static void destroy_encoder_context(VncState *vs)
{
gst_clear_object(&vs->h264->source);
@@ -68,26 +130,12 @@ static bool create_encoder_context(VncState *vs, int w, int h)
goto error;
}
- vs->h264->gst_encoder = gst_element_factory_make("x264enc", "gst-encoder");
+ vs->h264->gst_encoder = create_encoder(vs->h264->encoder_name);
if (!vs->h264->gst_encoder) {
VNC_DEBUG("Could not create gst x264 encoder\n");
goto error;
}
- g_object_set(
- vs->h264->gst_encoder,
- "tune", 4, /* zerolatency */
- /*
- * fix for zerolatency with novnc (without, noVNC displays
- * green stripes)
- */
- "threads", 1,
- "pass", 5, /* Constant Quality */
- "quantizer", 26,
- /* avoid access unit delimiters (Nal Unit Type 9) - not required */
- "aud", false,
- NULL);
-
vs->h264->sink = gst_element_factory_make("appsink", "sink");
if (!vs->h264->sink) {
VNC_DEBUG("Could not create gst sink\n");
@@ -172,9 +220,20 @@ static bool create_encoder_context(VncState *vs, int w, int h)
bool vnc_h264_encoder_init(VncState *vs)
{
+ const char *encoder_name;
+
g_assert(vs->h264 == NULL);
+ encoder_name = get_available_encoder();
+ if (encoder_name == NULL) {
+ VNC_DEBUG("No H264 encoder available.\n");
+ return -1;
+ }
+
vs->h264 = g_new0(VncH264, 1);
+ vs->h264->encoder_name = encoder_name;
+
+ VNC_DEBUG("Allow H264 using encoder '%s`\n", encoder_name);
return true;
}
diff --git a/ui/vnc.h b/ui/vnc.h
index a5ea134de8..e97276349e 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -239,6 +239,7 @@ typedef struct VncZywrle {
/* Number of frames we send after the display is clean. */
#define VNC_H264_KEEP_DIRTY 10
typedef struct VncH264 {
+ const char *encoder_name;
GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
size_t width;
size_t height;
--
2.39.5
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 4/9] h264: search for available h264 encoder
2025-04-18 11:29 ` [PATCH v3 4/9] h264: search for available h264 encoder Dietmar Maurer
@ 2025-04-23 12:43 ` Daniel P. Berrangé
2025-04-24 10:30 ` Daniel P. Berrangé
1 sibling, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-23 12:43 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Fri, Apr 18, 2025 at 01:29:48PM +0200, Dietmar Maurer wrote:
> The search list is currently hardcoded to: ["x264enc", "openh264enc"]
>
> x264enc: is probably the best available software encoder
> openh264enc: lower quality, but available on more systems.
>
> We restrict encoders to a known list because each encoder requires
> fine tuning to get reasonable/usable results.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/vnc-enc-h264.c | 89 +++++++++++++++++++++++++++++++++++++++--------
> ui/vnc.h | 1 +
> 2 files changed, 75 insertions(+), 15 deletions(-)
>
> diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> index 3abe6a1528..047f4a3128 100644
> --- a/ui/vnc-enc-h264.c
> +++ b/ui/vnc-enc-h264.c
> @@ -27,6 +27,68 @@
>
> #include <gst/gst.h>
>
> +const char *encoder_list[] = { "x264enc", "openh264enc", NULL };
> +
> +static const char *get_available_encoder(void)
> +{
> + int i = 0;
> + do {
> + const char *encoder_name = encoder_list[i];
> + if (encoder_name == NULL) {
> + break;
> + }
> + GstElement *element = gst_element_factory_make(
> + encoder_name, "video-encoder");
> + if (element != NULL) {
> + gst_object_unref(element);
> + return encoder_name;
> + }
> + i = i + 1;
> + } while (true);
This while loop is incredibly verbose as written.
for (int i = 0; i < G_N_ELEMENTS(encoder_list); i++) {
GstElement *element = gst_element_factory_make(
encoder_list[i], "video-encoder");
if (element != NULL) {
gst_object_unref(element);
return encoder_list[i];
}
}
and get rid of the trailing "NULL" in encoder_list as it
isn't required
> +
> + return NULL;
> +}
> +
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 4/9] h264: search for available h264 encoder
2025-04-18 11:29 ` [PATCH v3 4/9] h264: search for available h264 encoder Dietmar Maurer
2025-04-23 12:43 ` Daniel P. Berrangé
@ 2025-04-24 10:30 ` Daniel P. Berrangé
1 sibling, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-24 10:30 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Fri, Apr 18, 2025 at 01:29:48PM +0200, Dietmar Maurer wrote:
> The search list is currently hardcoded to: ["x264enc", "openh264enc"]
>
> x264enc: is probably the best available software encoder
> openh264enc: lower quality, but available on more systems.
>
> We restrict encoders to a known list because each encoder requires
> fine tuning to get reasonable/usable results.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/vnc-enc-h264.c | 89 +++++++++++++++++++++++++++++++++++++++--------
> ui/vnc.h | 1 +
> 2 files changed, 75 insertions(+), 15 deletions(-)
>
> diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> index 3abe6a1528..047f4a3128 100644
> --- a/ui/vnc-enc-h264.c
> +++ b/ui/vnc-enc-h264.c
> @@ -172,9 +220,20 @@ static bool create_encoder_context(VncState *vs, int w, int h)
>
> bool vnc_h264_encoder_init(VncState *vs)
> {
> + const char *encoder_name;
> +
> g_assert(vs->h264 == NULL);
>
> + encoder_name = get_available_encoder();
> + if (encoder_name == NULL) {
> + VNC_DEBUG("No H264 encoder available.\n");
> + return -1;
This method has return type 'bool', not 'int', this needs
to be 'false'.
> + }
> +
> vs->h264 = g_new0(VncH264, 1);
> + vs->h264->encoder_name = encoder_name;
> +
> + VNC_DEBUG("Allow H264 using encoder '%s`\n", encoder_name);
>
> return true;
> }
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 5/9] h264: new vnc option to configure h264 at server side
2025-04-18 11:29 [PATCH v3 0/9] Add VNC Open H.264 Encoding Dietmar Maurer
` (3 preceding siblings ...)
2025-04-18 11:29 ` [PATCH v3 4/9] h264: search for available h264 encoder Dietmar Maurer
@ 2025-04-18 11:29 ` Dietmar Maurer
2025-04-21 10:06 ` Marc-André Lureau
2025-04-23 12:47 ` Daniel P. Berrangé
2025-04-18 11:29 ` [PATCH v3 6/9] h264: add hardware encoders Dietmar Maurer
` (3 subsequent siblings)
8 siblings, 2 replies; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-18 11:29 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: Dietmar Maurer
Values can be 'on', 'off', or a space sparated list of
allowed gstreamer encoders.
- on: automatically select the encoder
- off: disbale h264
- encoder-list: select first available encoder from that list.
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
ui/vnc-enc-h264.c | 30 ++++++++++++++++++++++--------
ui/vnc.c | 25 ++++++++++++++++++++-----
ui/vnc.h | 6 +++++-
3 files changed, 47 insertions(+), 14 deletions(-)
diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
index 047f4a3128..0f89cafbf6 100644
--- a/ui/vnc-enc-h264.c
+++ b/ui/vnc-enc-h264.c
@@ -27,13 +27,21 @@
#include <gst/gst.h>
-const char *encoder_list[] = { "x264enc", "openh264enc", NULL };
-
-static const char *get_available_encoder(void)
+static char *get_available_encoder(const char *encoder_list)
{
+ g_assert(encoder_list != NULL);
+
+ if (!strcmp(encoder_list, "")) {
+ /* use default list */
+ encoder_list = "x264enc openh264enc";
+ }
+
+ char *ret = NULL;
+ char **encoder_array = g_strsplit(encoder_list, " ", -1);
+
int i = 0;
do {
- const char *encoder_name = encoder_list[i];
+ const char *encoder_name = encoder_array[i];
if (encoder_name == NULL) {
break;
}
@@ -41,12 +49,15 @@ static const char *get_available_encoder(void)
encoder_name, "video-encoder");
if (element != NULL) {
gst_object_unref(element);
- return encoder_name;
+ ret = strdup(encoder_name);
+ break;
}
i = i + 1;
} while (true);
- return NULL;
+ g_strfreev(encoder_array);
+
+ return ret;
}
static GstElement *create_encoder(const char *encoder_name)
@@ -220,11 +231,13 @@ static bool create_encoder_context(VncState *vs, int w, int h)
bool vnc_h264_encoder_init(VncState *vs)
{
- const char *encoder_name;
+ char *encoder_name;
g_assert(vs->h264 == NULL);
+ g_assert(vs->vd != NULL);
+ g_assert(vs->vd->h264_encoder_list != NULL);
- encoder_name = get_available_encoder();
+ encoder_name = get_available_encoder(vs->vd->h264_encoder_list);
if (encoder_name == NULL) {
VNC_DEBUG("No H264 encoder available.\n");
return -1;
@@ -336,6 +349,7 @@ void vnc_h264_clear(VncState *vs)
}
destroy_encoder_context(vs);
+ g_free(vs->h264->encoder_name);
g_clear_pointer(&vs->h264, g_free);
}
diff --git a/ui/vnc.c b/ui/vnc.c
index badc7912c0..feab4c0043 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2190,11 +2190,11 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
break;
#ifdef CONFIG_GSTREAMER
case VNC_ENCODING_H264:
- if (vnc_h264_encoder_init(vs)) {
- vnc_set_feature(vs, VNC_FEATURE_H264);
- vs->vnc_encoding = enc;
- } else {
- VNC_DEBUG("vnc_h264_encoder_init failed\n");
+ if (vs->vd->h264_encoder_list != NULL) { /* if h264 is enabled */
+ if (vnc_h264_encoder_init(vs)) {
+ vnc_set_feature(vs, VNC_FEATURE_H264);
+ vs->vnc_encoding = enc;
+ }
}
break;
#endif
@@ -3634,6 +3634,9 @@ static QemuOptsList qemu_vnc_opts = {
},{
.name = "power-control",
.type = QEMU_OPT_BOOL,
+ },{
+ .name = "h264",
+ .type = QEMU_OPT_STRING,
},
{ /* end of list */ }
},
@@ -4196,6 +4199,18 @@ void vnc_display_open(const char *id, Error **errp)
}
#endif
+#ifdef CONFIG_GSTREAMER
+ const char *h264_opt = qemu_opt_get(opts, "h264");
+ if (!strcmp(h264_opt, "off")) {
+ vd->h264_encoder_list = NULL; /* disable h264 */
+ } else if (!strcmp(h264_opt, "on")) {
+ vd->h264_encoder_list = ""; /* use default encoder list */
+ } else {
+ /* assume this is a list of endiers */
+ vd->h264_encoder_list = h264_opt;
+ }
+#endif
+
if (vnc_display_setup_auth(&vd->auth, &vd->subauth,
vd->tlscreds, password,
sasl, false, errp) < 0) {
diff --git a/ui/vnc.h b/ui/vnc.h
index e97276349e..789b18806b 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -188,6 +188,10 @@ struct VncDisplay
VncDisplaySASL sasl;
#endif
+#ifdef CONFIG_GSTREAMER
+ const char *h264_encoder_list;
+#endif
+
AudioState *audio_state;
};
@@ -239,7 +243,7 @@ typedef struct VncZywrle {
/* Number of frames we send after the display is clean. */
#define VNC_H264_KEEP_DIRTY 10
typedef struct VncH264 {
- const char *encoder_name;
+ char *encoder_name;
GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
size_t width;
size_t height;
--
2.39.5
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 5/9] h264: new vnc option to configure h264 at server side
2025-04-18 11:29 ` [PATCH v3 5/9] h264: new vnc option to configure h264 at server side Dietmar Maurer
@ 2025-04-21 10:06 ` Marc-André Lureau
2025-04-23 12:47 ` Daniel P. Berrangé
1 sibling, 0 replies; 47+ messages in thread
From: Marc-André Lureau @ 2025-04-21 10:06 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: qemu-devel
Hi
On Fri, Apr 18, 2025 at 3:30 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
>
> Values can be 'on', 'off', or a space sparated list of
separated
> allowed gstreamer encoders.
>
> - on: automatically select the encoder
'auto' then (default?)
> - off: disbale h264
disable
'auto'/'off' but there should be 'on' too.
> - encoder-list: select first available encoder from that list.
This should be a different option.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/vnc-enc-h264.c | 30 ++++++++++++++++++++++--------
> ui/vnc.c | 25 ++++++++++++++++++++-----
> ui/vnc.h | 6 +++++-
> 3 files changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> index 047f4a3128..0f89cafbf6 100644
> --- a/ui/vnc-enc-h264.c
> +++ b/ui/vnc-enc-h264.c
> @@ -27,13 +27,21 @@
>
> #include <gst/gst.h>
>
> -const char *encoder_list[] = { "x264enc", "openh264enc", NULL };
> -
> -static const char *get_available_encoder(void)
> +static char *get_available_encoder(const char *encoder_list)
> {
> + g_assert(encoder_list != NULL);
> +
> + if (!strcmp(encoder_list, "")) {
> + /* use default list */
> + encoder_list = "x264enc openh264enc";
> + }
> +
> + char *ret = NULL;
> + char **encoder_array = g_strsplit(encoder_list, " ", -1);
> +
> int i = 0;
> do {
> - const char *encoder_name = encoder_list[i];
> + const char *encoder_name = encoder_array[i];
> if (encoder_name == NULL) {
> break;
> }
> @@ -41,12 +49,15 @@ static const char *get_available_encoder(void)
> encoder_name, "video-encoder");
> if (element != NULL) {
> gst_object_unref(element);
> - return encoder_name;
> + ret = strdup(encoder_name);
> + break;
> }
> i = i + 1;
> } while (true);
>
> - return NULL;
> + g_strfreev(encoder_array);
> +
> + return ret;
> }
>
> static GstElement *create_encoder(const char *encoder_name)
> @@ -220,11 +231,13 @@ static bool create_encoder_context(VncState *vs, int w, int h)
>
> bool vnc_h264_encoder_init(VncState *vs)
> {
> - const char *encoder_name;
> + char *encoder_name;
>
> g_assert(vs->h264 == NULL);
> + g_assert(vs->vd != NULL);
> + g_assert(vs->vd->h264_encoder_list != NULL);
>
> - encoder_name = get_available_encoder();
> + encoder_name = get_available_encoder(vs->vd->h264_encoder_list);
> if (encoder_name == NULL) {
> VNC_DEBUG("No H264 encoder available.\n");
> return -1;
> @@ -336,6 +349,7 @@ void vnc_h264_clear(VncState *vs)
> }
>
> destroy_encoder_context(vs);
> + g_free(vs->h264->encoder_name);
>
> g_clear_pointer(&vs->h264, g_free);
> }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index badc7912c0..feab4c0043 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2190,11 +2190,11 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
> break;
> #ifdef CONFIG_GSTREAMER
> case VNC_ENCODING_H264:
> - if (vnc_h264_encoder_init(vs)) {
> - vnc_set_feature(vs, VNC_FEATURE_H264);
> - vs->vnc_encoding = enc;
> - } else {
> - VNC_DEBUG("vnc_h264_encoder_init failed\n");
> + if (vs->vd->h264_encoder_list != NULL) { /* if h264 is enabled */
> + if (vnc_h264_encoder_init(vs)) {
> + vnc_set_feature(vs, VNC_FEATURE_H264);
> + vs->vnc_encoding = enc;
> + }
> }
> break;
> #endif
> @@ -3634,6 +3634,9 @@ static QemuOptsList qemu_vnc_opts = {
> },{
> .name = "power-control",
> .type = QEMU_OPT_BOOL,
> + },{
> + .name = "h264",
> + .type = QEMU_OPT_STRING,
> },
> { /* end of list */ }
> },
> @@ -4196,6 +4199,18 @@ void vnc_display_open(const char *id, Error **errp)
> }
> #endif
>
> +#ifdef CONFIG_GSTREAMER
> + const char *h264_opt = qemu_opt_get(opts, "h264");
> + if (!strcmp(h264_opt, "off")) {
> + vd->h264_encoder_list = NULL; /* disable h264 */
> + } else if (!strcmp(h264_opt, "on")) {
> + vd->h264_encoder_list = ""; /* use default encoder list */
> + } else {
> + /* assume this is a list of endiers */
> + vd->h264_encoder_list = h264_opt;
> + }
> +#endif
> +
> if (vnc_display_setup_auth(&vd->auth, &vd->subauth,
> vd->tlscreds, password,
> sasl, false, errp) < 0) {
> diff --git a/ui/vnc.h b/ui/vnc.h
> index e97276349e..789b18806b 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -188,6 +188,10 @@ struct VncDisplay
> VncDisplaySASL sasl;
> #endif
>
> +#ifdef CONFIG_GSTREAMER
> + const char *h264_encoder_list;
> +#endif
> +
> AudioState *audio_state;
> };
>
> @@ -239,7 +243,7 @@ typedef struct VncZywrle {
> /* Number of frames we send after the display is clean. */
> #define VNC_H264_KEEP_DIRTY 10
> typedef struct VncH264 {
> - const char *encoder_name;
> + char *encoder_name;
> GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
> size_t width;
> size_t height;
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 5/9] h264: new vnc option to configure h264 at server side
2025-04-18 11:29 ` [PATCH v3 5/9] h264: new vnc option to configure h264 at server side Dietmar Maurer
2025-04-21 10:06 ` Marc-André Lureau
@ 2025-04-23 12:47 ` Daniel P. Berrangé
2025-04-25 8:02 ` Dietmar Maurer
1 sibling, 1 reply; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-23 12:47 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Fri, Apr 18, 2025 at 01:29:49PM +0200, Dietmar Maurer wrote:
> Values can be 'on', 'off', or a space sparated list of
> allowed gstreamer encoders.
space separated list values on the command line is incredibly unpleasant
syntax as it requires quoting the args.
>
> - on: automatically select the encoder
> - off: disbale h264
> - encoder-list: select first available encoder from that list.
Overloading one config option to both turn h264 on/off,
an choose encoding is not very desirable.
IMHO there should be a "h264=<bool>" option to turn it
on/off, and a separate flag to choose the encoder.
Do we even need to give a list of encoders, as opposed
to just choosing the desired encoder ?
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/vnc-enc-h264.c | 30 ++++++++++++++++++++++--------
> ui/vnc.c | 25 ++++++++++++++++++++-----
> ui/vnc.h | 6 +++++-
> 3 files changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> index 047f4a3128..0f89cafbf6 100644
> --- a/ui/vnc-enc-h264.c
> +++ b/ui/vnc-enc-h264.c
> @@ -27,13 +27,21 @@
>
> #include <gst/gst.h>
>
> -const char *encoder_list[] = { "x264enc", "openh264enc", NULL };
> -
> -static const char *get_available_encoder(void)
> +static char *get_available_encoder(const char *encoder_list)
> {
> + g_assert(encoder_list != NULL);
> +
> + if (!strcmp(encoder_list, "")) {
> + /* use default list */
> + encoder_list = "x264enc openh264enc";
> + }
> +
> + char *ret = NULL;
> + char **encoder_array = g_strsplit(encoder_list, " ", -1);
> +
> int i = 0;
> do {
> - const char *encoder_name = encoder_list[i];
> + const char *encoder_name = encoder_array[i];
> if (encoder_name == NULL) {
> break;
> }
> @@ -41,12 +49,15 @@ static const char *get_available_encoder(void)
> encoder_name, "video-encoder");
> if (element != NULL) {
> gst_object_unref(element);
> - return encoder_name;
> + ret = strdup(encoder_name);
> + break;
> }
> i = i + 1;
> } while (true);
>
> - return NULL;
> + g_strfreev(encoder_array);
> +
> + return ret;
> }
>
> static GstElement *create_encoder(const char *encoder_name)
> @@ -220,11 +231,13 @@ static bool create_encoder_context(VncState *vs, int w, int h)
>
> bool vnc_h264_encoder_init(VncState *vs)
> {
> - const char *encoder_name;
> + char *encoder_name;
>
> g_assert(vs->h264 == NULL);
> + g_assert(vs->vd != NULL);
> + g_assert(vs->vd->h264_encoder_list != NULL);
>
> - encoder_name = get_available_encoder();
> + encoder_name = get_available_encoder(vs->vd->h264_encoder_list);
> if (encoder_name == NULL) {
> VNC_DEBUG("No H264 encoder available.\n");
> return -1;
> @@ -336,6 +349,7 @@ void vnc_h264_clear(VncState *vs)
> }
>
> destroy_encoder_context(vs);
> + g_free(vs->h264->encoder_name);
>
> g_clear_pointer(&vs->h264, g_free);
> }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index badc7912c0..feab4c0043 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2190,11 +2190,11 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
> break;
> #ifdef CONFIG_GSTREAMER
> case VNC_ENCODING_H264:
> - if (vnc_h264_encoder_init(vs)) {
> - vnc_set_feature(vs, VNC_FEATURE_H264);
> - vs->vnc_encoding = enc;
> - } else {
> - VNC_DEBUG("vnc_h264_encoder_init failed\n");
> + if (vs->vd->h264_encoder_list != NULL) { /* if h264 is enabled */
> + if (vnc_h264_encoder_init(vs)) {
> + vnc_set_feature(vs, VNC_FEATURE_H264);
> + vs->vnc_encoding = enc;
> + }
> }
> break;
> #endif
> @@ -3634,6 +3634,9 @@ static QemuOptsList qemu_vnc_opts = {
> },{
> .name = "power-control",
> .type = QEMU_OPT_BOOL,
> + },{
> + .name = "h264",
> + .type = QEMU_OPT_STRING,
> },
> { /* end of list */ }
> },
> @@ -4196,6 +4199,18 @@ void vnc_display_open(const char *id, Error **errp)
> }
> #endif
>
> +#ifdef CONFIG_GSTREAMER
> + const char *h264_opt = qemu_opt_get(opts, "h264");
> + if (!strcmp(h264_opt, "off")) {
> + vd->h264_encoder_list = NULL; /* disable h264 */
> + } else if (!strcmp(h264_opt, "on")) {
> + vd->h264_encoder_list = ""; /* use default encoder list */
> + } else {
> + /* assume this is a list of endiers */
> + vd->h264_encoder_list = h264_opt;
> + }
> +#endif
> +
> if (vnc_display_setup_auth(&vd->auth, &vd->subauth,
> vd->tlscreds, password,
> sasl, false, errp) < 0) {
> diff --git a/ui/vnc.h b/ui/vnc.h
> index e97276349e..789b18806b 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -188,6 +188,10 @@ struct VncDisplay
> VncDisplaySASL sasl;
> #endif
>
> +#ifdef CONFIG_GSTREAMER
> + const char *h264_encoder_list;
> +#endif
> +
> AudioState *audio_state;
> };
>
> @@ -239,7 +243,7 @@ typedef struct VncZywrle {
> /* Number of frames we send after the display is clean. */
> #define VNC_H264_KEEP_DIRTY 10
> typedef struct VncH264 {
> - const char *encoder_name;
> + char *encoder_name;
> GstElement *pipeline, *source, *gst_encoder, *sink, *convert;
> size_t width;
> size_t height;
> --
> 2.39.5
>
>
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 5/9] h264: new vnc option to configure h264 at server side
2025-04-23 12:47 ` Daniel P. Berrangé
@ 2025-04-25 8:02 ` Dietmar Maurer
0 siblings, 0 replies; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-25 8:02 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: marcandre.lureau, qemu-devel
> Overloading one config option to both turn h264 on/off,
> an choose encoding is not very desirable.
>
> IMHO there should be a "h264=<bool>" option to turn it
> on/off, and a separate flag to choose the encoder.
will do that.
> Do we even need to give a list of encoders, as opposed
> to just choosing the desired encoder ?
How do you know if the encoder is available? If we use a list,
we can simply loop through and use the first available. This way
we can also test for (and prefer) hardware encoder...
- Dietmar
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 6/9] h264: add hardware encoders
2025-04-18 11:29 [PATCH v3 0/9] Add VNC Open H.264 Encoding Dietmar Maurer
` (4 preceding siblings ...)
2025-04-18 11:29 ` [PATCH v3 5/9] h264: new vnc option to configure h264 at server side Dietmar Maurer
@ 2025-04-18 11:29 ` Dietmar Maurer
2025-04-23 12:49 ` Daniel P. Berrangé
2025-04-18 11:29 ` [PATCH v3 7/9] h264: do not reduce vnc update speed while we have an active h264 stream Dietmar Maurer
` (2 subsequent siblings)
8 siblings, 1 reply; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-18 11:29 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: Dietmar Maurer
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
ui/vnc-enc-h264.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
index 0f89cafbf6..840674dbdb 100644
--- a/ui/vnc-enc-h264.c
+++ b/ui/vnc-enc-h264.c
@@ -29,15 +29,17 @@
static char *get_available_encoder(const char *encoder_list)
{
+ char *ret = NULL;
+ char **encoder_array = NULL;
+
g_assert(encoder_list != NULL);
if (!strcmp(encoder_list, "")) {
/* use default list */
- encoder_list = "x264enc openh264enc";
+ encoder_list = "nvh264enc vaapih264enc x264enc openh264enc";
}
- char *ret = NULL;
- char **encoder_array = g_strsplit(encoder_list, " ", -1);
+ encoder_array = g_strsplit(encoder_list, " ", -1);
int i = 0;
do {
@@ -69,7 +71,19 @@ static GstElement *create_encoder(const char *encoder_name)
return NULL;
}
- if (!strcmp(encoder_name, "x264enc")) {
+ if (!strcmp(encoder_name, "nvh264enc")) {
+ g_object_set(
+ encoder,
+ "preset", 8, /* p1 - fastest */
+ "multi-pass", 1, /* multipass disabled */
+ "tune", 2, /* low latency */
+ "zerolatency", true, /* low latency */
+ /* avoid access unit delimiters (Nal Unit Type 9) - not required */
+ "aud", false,
+ NULL);
+ } else if (!strcmp(encoder_name, "vaapih264enc")) {
+ g_object_set(encoder, "tune", 1, NULL); /* high compression */
+ } else if (!strcmp(encoder_name, "x264enc")) {
g_object_set(
encoder,
"tune", 4, /* zerolatency */
--
2.39.5
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 6/9] h264: add hardware encoders
2025-04-18 11:29 ` [PATCH v3 6/9] h264: add hardware encoders Dietmar Maurer
@ 2025-04-23 12:49 ` Daniel P. Berrangé
0 siblings, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-23 12:49 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Fri, Apr 18, 2025 at 01:29:50PM +0200, Dietmar Maurer wrote:
....could at lesat explain what the chosen encoders are..
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/vnc-enc-h264.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> index 0f89cafbf6..840674dbdb 100644
> --- a/ui/vnc-enc-h264.c
> +++ b/ui/vnc-enc-h264.c
> @@ -29,15 +29,17 @@
>
> static char *get_available_encoder(const char *encoder_list)
> {
> + char *ret = NULL;
> + char **encoder_array = NULL;
> +
> g_assert(encoder_list != NULL);
>
> if (!strcmp(encoder_list, "")) {
> /* use default list */
> - encoder_list = "x264enc openh264enc";
> + encoder_list = "nvh264enc vaapih264enc x264enc openh264enc";
> }
>
> - char *ret = NULL;
> - char **encoder_array = g_strsplit(encoder_list, " ", -1);
> + encoder_array = g_strsplit(encoder_list, " ", -1);
>
> int i = 0;
> do {
> @@ -69,7 +71,19 @@ static GstElement *create_encoder(const char *encoder_name)
> return NULL;
> }
>
> - if (!strcmp(encoder_name, "x264enc")) {
> + if (!strcmp(encoder_name, "nvh264enc")) {
> + g_object_set(
> + encoder,
> + "preset", 8, /* p1 - fastest */
> + "multi-pass", 1, /* multipass disabled */
> + "tune", 2, /* low latency */
> + "zerolatency", true, /* low latency */
> + /* avoid access unit delimiters (Nal Unit Type 9) - not required */
> + "aud", false,
> + NULL);
> + } else if (!strcmp(encoder_name, "vaapih264enc")) {
> + g_object_set(encoder, "tune", 1, NULL); /* high compression */
> + } else if (!strcmp(encoder_name, "x264enc")) {
> g_object_set(
> encoder,
> "tune", 4, /* zerolatency */
Feels like this patch could jsut be folded into patch 4 that first
introduces this method.
> --
> 2.39.5
>
>
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 7/9] h264: do not reduce vnc update speed while we have an active h264 stream
2025-04-18 11:29 [PATCH v3 0/9] Add VNC Open H.264 Encoding Dietmar Maurer
` (5 preceding siblings ...)
2025-04-18 11:29 ` [PATCH v3 6/9] h264: add hardware encoders Dietmar Maurer
@ 2025-04-18 11:29 ` Dietmar Maurer
2025-04-23 12:50 ` Daniel P. Berrangé
2025-04-18 11:29 ` [PATCH v3 8/9] vnc: initialize gst during argument processing Dietmar Maurer
2025-04-18 11:29 ` [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context Dietmar Maurer
8 siblings, 1 reply; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-18 11:29 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: Dietmar Maurer
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
ui/vnc.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index feab4c0043..6db03a1550 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3222,6 +3222,7 @@ static void vnc_refresh(DisplayChangeListener *dcl)
VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
VncState *vs, *vn;
int has_dirty, rects = 0;
+ bool keep_dirty = false;
if (QTAILQ_EMPTY(&vd->clients)) {
update_displaychangelistener(&vd->dcl, VNC_REFRESH_INTERVAL_MAX);
@@ -3249,6 +3250,9 @@ static void vnc_refresh(DisplayChangeListener *dcl)
vs->h264->keep_dirty--;
}
}
+ if (vs->h264->keep_dirty > 0) {
+ keep_dirty = true;
+ }
}
int count = vnc_update_client(vs, client_dirty);
@@ -3266,7 +3270,7 @@ static void vnc_refresh(DisplayChangeListener *dcl)
/* vs might be free()ed here */
}
- if (has_dirty && rects) {
+ if ((has_dirty && rects) || keep_dirty) {
vd->dcl.update_interval /= 2;
if (vd->dcl.update_interval < VNC_REFRESH_INTERVAL_BASE) {
vd->dcl.update_interval = VNC_REFRESH_INTERVAL_BASE;
--
2.39.5
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 7/9] h264: do not reduce vnc update speed while we have an active h264 stream
2025-04-18 11:29 ` [PATCH v3 7/9] h264: do not reduce vnc update speed while we have an active h264 stream Dietmar Maurer
@ 2025-04-23 12:50 ` Daniel P. Berrangé
0 siblings, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-23 12:50 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Fri, Apr 18, 2025 at 01:29:51PM +0200, Dietmar Maurer wrote:
...the $SUBJECT says what it does, but the comment message ought
to describe why it is doing this too.
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/vnc.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index feab4c0043..6db03a1550 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3222,6 +3222,7 @@ static void vnc_refresh(DisplayChangeListener *dcl)
> VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
> VncState *vs, *vn;
> int has_dirty, rects = 0;
> + bool keep_dirty = false;
>
> if (QTAILQ_EMPTY(&vd->clients)) {
> update_displaychangelistener(&vd->dcl, VNC_REFRESH_INTERVAL_MAX);
> @@ -3249,6 +3250,9 @@ static void vnc_refresh(DisplayChangeListener *dcl)
> vs->h264->keep_dirty--;
> }
> }
> + if (vs->h264->keep_dirty > 0) {
> + keep_dirty = true;
> + }
> }
>
> int count = vnc_update_client(vs, client_dirty);
> @@ -3266,7 +3270,7 @@ static void vnc_refresh(DisplayChangeListener *dcl)
> /* vs might be free()ed here */
> }
>
> - if (has_dirty && rects) {
> + if ((has_dirty && rects) || keep_dirty) {
> vd->dcl.update_interval /= 2;
> if (vd->dcl.update_interval < VNC_REFRESH_INTERVAL_BASE) {
> vd->dcl.update_interval = VNC_REFRESH_INTERVAL_BASE;
> --
> 2.39.5
>
>
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 8/9] vnc: initialize gst during argument processing
2025-04-18 11:29 [PATCH v3 0/9] Add VNC Open H.264 Encoding Dietmar Maurer
` (6 preceding siblings ...)
2025-04-18 11:29 ` [PATCH v3 7/9] h264: do not reduce vnc update speed while we have an active h264 stream Dietmar Maurer
@ 2025-04-18 11:29 ` Dietmar Maurer
2025-04-21 10:09 ` Marc-André Lureau
2025-04-23 12:52 ` Daniel P. Berrangé
2025-04-18 11:29 ` [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context Dietmar Maurer
8 siblings, 2 replies; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-18 11:29 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: Dietmar Maurer
So that we can set --gst- options on the qemu command line.
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
system/vl.c | 8 ++++++++
ui/vnc.c | 4 ----
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/system/vl.c b/system/vl.c
index ec93988a03..c7fff02da2 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -140,6 +140,10 @@
#include "qemu/guest-random.h"
#include "qemu/keyval.h"
+#ifdef CONFIG_GSTREAMER
+#include <gst/gst.h>
+#endif
+
#define MAX_VIRTIO_CONSOLES 1
typedef struct BlockdevOptionsQueueEntry {
@@ -2848,6 +2852,10 @@ void qemu_init(int argc, char **argv)
bool userconfig = true;
FILE *vmstate_dump_file = NULL;
+#ifdef CONFIG_GSTREAMER
+ gst_init(&argc, &argv);
+#endif
+
qemu_add_opts(&qemu_drive_opts);
qemu_add_drive_opts(&qemu_legacy_drive_opts);
qemu_add_drive_opts(&qemu_common_drive_opts);
diff --git a/ui/vnc.c b/ui/vnc.c
index 6db03a1550..8f6287e2e6 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -4350,10 +4350,6 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
Error *local_err = NULL;
char *id = (char *)qemu_opts_id(opts);
-#ifdef CONFIG_GSTREAMER
- gst_init(NULL, NULL);
-#endif
-
assert(id);
vnc_display_init(id, &local_err);
if (local_err) {
--
2.39.5
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 8/9] vnc: initialize gst during argument processing
2025-04-18 11:29 ` [PATCH v3 8/9] vnc: initialize gst during argument processing Dietmar Maurer
@ 2025-04-21 10:09 ` Marc-André Lureau
2025-04-23 12:52 ` Daniel P. Berrangé
1 sibling, 0 replies; 47+ messages in thread
From: Marc-André Lureau @ 2025-04-21 10:09 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: qemu-devel
Hi
On Fri, Apr 18, 2025 at 3:30 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
>
> So that we can set --gst- options on the qemu command line.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
Nice, could you move that up in the patch series? As a second patch
after linking gst seems fitting.
> ---
> system/vl.c | 8 ++++++++
> ui/vnc.c | 4 ----
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/system/vl.c b/system/vl.c
> index ec93988a03..c7fff02da2 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -140,6 +140,10 @@
> #include "qemu/guest-random.h"
> #include "qemu/keyval.h"
>
> +#ifdef CONFIG_GSTREAMER
> +#include <gst/gst.h>
> +#endif
> +
> #define MAX_VIRTIO_CONSOLES 1
>
> typedef struct BlockdevOptionsQueueEntry {
> @@ -2848,6 +2852,10 @@ void qemu_init(int argc, char **argv)
> bool userconfig = true;
> FILE *vmstate_dump_file = NULL;
>
> +#ifdef CONFIG_GSTREAMER
> + gst_init(&argc, &argv);
> +#endif
> +
> qemu_add_opts(&qemu_drive_opts);
> qemu_add_drive_opts(&qemu_legacy_drive_opts);
> qemu_add_drive_opts(&qemu_common_drive_opts);
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 6db03a1550..8f6287e2e6 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -4350,10 +4350,6 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> Error *local_err = NULL;
> char *id = (char *)qemu_opts_id(opts);
>
> -#ifdef CONFIG_GSTREAMER
> - gst_init(NULL, NULL);
> -#endif
> -
> assert(id);
> vnc_display_init(id, &local_err);
> if (local_err) {
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 8/9] vnc: initialize gst during argument processing
2025-04-18 11:29 ` [PATCH v3 8/9] vnc: initialize gst during argument processing Dietmar Maurer
2025-04-21 10:09 ` Marc-André Lureau
@ 2025-04-23 12:52 ` Daniel P. Berrangé
1 sibling, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-23 12:52 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel, armbru
On Fri, Apr 18, 2025 at 01:29:52PM +0200, Dietmar Maurer wrote:
> So that we can set --gst- options on the qemu command line.
I don't think this is desirable. It exposes a bunch of arbitrary
command line arguments that QEMU has no visibility or control
over. This will complicate our long term work / goal of moving all
QEMU configuration to QMP.
CC Markus for a 2nd opinion as our command line modelling / QMP
expert.
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> system/vl.c | 8 ++++++++
> ui/vnc.c | 4 ----
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/system/vl.c b/system/vl.c
> index ec93988a03..c7fff02da2 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -140,6 +140,10 @@
> #include "qemu/guest-random.h"
> #include "qemu/keyval.h"
>
> +#ifdef CONFIG_GSTREAMER
> +#include <gst/gst.h>
> +#endif
> +
> #define MAX_VIRTIO_CONSOLES 1
>
> typedef struct BlockdevOptionsQueueEntry {
> @@ -2848,6 +2852,10 @@ void qemu_init(int argc, char **argv)
> bool userconfig = true;
> FILE *vmstate_dump_file = NULL;
>
> +#ifdef CONFIG_GSTREAMER
> + gst_init(&argc, &argv);
> +#endif
> +
> qemu_add_opts(&qemu_drive_opts);
> qemu_add_drive_opts(&qemu_legacy_drive_opts);
> qemu_add_drive_opts(&qemu_common_drive_opts);
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 6db03a1550..8f6287e2e6 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -4350,10 +4350,6 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> Error *local_err = NULL;
> char *id = (char *)qemu_opts_id(opts);
>
> -#ifdef CONFIG_GSTREAMER
> - gst_init(NULL, NULL);
> -#endif
> -
> assert(id);
> vnc_display_init(id, &local_err);
> if (local_err) {
> --
> 2.39.5
>
>
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context
2025-04-18 11:29 [PATCH v3 0/9] Add VNC Open H.264 Encoding Dietmar Maurer
` (7 preceding siblings ...)
2025-04-18 11:29 ` [PATCH v3 8/9] vnc: initialize gst during argument processing Dietmar Maurer
@ 2025-04-18 11:29 ` Dietmar Maurer
2025-04-21 10:14 ` Marc-André Lureau
2025-04-23 12:57 ` Daniel P. Berrangé
8 siblings, 2 replies; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-18 11:29 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: Dietmar Maurer
Some encoders can hang indefinetly (i.e. nvh264enc) if
the pipeline is not stopped before it is destroyed
(Observed on Debian bookworm).
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
ui/vnc-enc-h264.c | 50 ++++++++++++++++++++++++++++++++++++++---------
ui/vnc.h | 1 +
2 files changed, 42 insertions(+), 9 deletions(-)
diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
index 840674dbdb..9dbfba3a16 100644
--- a/ui/vnc-enc-h264.c
+++ b/ui/vnc-enc-h264.c
@@ -23,6 +23,8 @@
*/
#include "qemu/osdep.h"
+#include "system/runstate.h"
+
#include "vnc.h"
#include <gst/gst.h>
@@ -114,13 +116,33 @@ static GstElement *create_encoder(const char *encoder_name)
return encoder;
}
-static void destroy_encoder_context(VncState *vs)
+static void destroy_encoder_context(VncH264 *h264)
{
- gst_clear_object(&vs->h264->source);
- gst_clear_object(&vs->h264->convert);
- gst_clear_object(&vs->h264->gst_encoder);
- gst_clear_object(&vs->h264->sink);
- gst_clear_object(&vs->h264->pipeline);
+ GstStateChangeReturn state_change_ret;
+
+ g_assert(h264 != NULL);
+
+ VNC_DEBUG("Destroy h264 context.\n");
+
+ /*
+ * Some encoders can hang indefinetly (i.e. nvh264enc) if
+ * the pipeline is not stopped before it is destroyed
+ * (Observed on Debian bookworm).
+ */
+ if (h264->pipeline != NULL) {
+ state_change_ret = gst_element_set_state(
+ h264->pipeline, GST_STATE_NULL);
+
+ if (state_change_ret == GST_STATE_CHANGE_FAILURE) {
+ VNC_DEBUG("Unable to stop the GST pipeline\n");
+ }
+ }
+
+ gst_clear_object(&h264->source);
+ gst_clear_object(&h264->convert);
+ gst_clear_object(&h264->gst_encoder);
+ gst_clear_object(&h264->sink);
+ gst_clear_object(&h264->pipeline);
}
static bool create_encoder_context(VncState *vs, int w, int h)
@@ -132,7 +154,7 @@ static bool create_encoder_context(VncState *vs, int w, int h)
if (vs->h264->sink) {
if (w != vs->h264->width || h != vs->h264->height) {
- destroy_encoder_context(vs);
+ destroy_encoder_context(vs->h264);
}
}
@@ -239,10 +261,16 @@ static bool create_encoder_context(VncState *vs, int w, int h)
return TRUE;
error:
- destroy_encoder_context(vs);
+ destroy_encoder_context(vs->h264);
return FALSE;
}
+static void shutdown_h264(Notifier *n, void *opaque)
+{
+ VncH264 *h264 = container_of(n, VncH264, shutdown_notifier);
+ destroy_encoder_context(h264);
+}
+
bool vnc_h264_encoder_init(VncState *vs)
{
char *encoder_name;
@@ -259,6 +287,8 @@ bool vnc_h264_encoder_init(VncState *vs)
vs->h264 = g_new0(VncH264, 1);
vs->h264->encoder_name = encoder_name;
+ vs->h264->shutdown_notifier.notify = shutdown_h264;
+ qemu_register_shutdown_notifier(&vs->h264->shutdown_notifier);
VNC_DEBUG("Allow H264 using encoder '%s`\n", encoder_name);
@@ -362,7 +392,9 @@ void vnc_h264_clear(VncState *vs)
return;
}
- destroy_encoder_context(vs);
+ notifier_remove(&vs->h264->shutdown_notifier);
+
+ destroy_encoder_context(vs->h264);
g_free(vs->h264->encoder_name);
g_clear_pointer(&vs->h264, g_free);
diff --git a/ui/vnc.h b/ui/vnc.h
index 789b18806b..ea52085b19 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -248,6 +248,7 @@ typedef struct VncH264 {
size_t width;
size_t height;
guint keep_dirty;
+ Notifier shutdown_notifier;
} VncH264;
#endif
--
2.39.5
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context
2025-04-18 11:29 ` [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context Dietmar Maurer
@ 2025-04-21 10:14 ` Marc-André Lureau
2025-04-22 6:35 ` Dietmar Maurer
2025-04-23 12:57 ` Daniel P. Berrangé
1 sibling, 1 reply; 47+ messages in thread
From: Marc-André Lureau @ 2025-04-21 10:14 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: qemu-devel
Hi
On Fri, Apr 18, 2025 at 3:30 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
>
> Some encoders can hang indefinetly (i.e. nvh264enc) if
indefinitely
> the pipeline is not stopped before it is destroyed
> (Observed on Debian bookworm).
but why do you need the extra shutdown notifier?
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/vnc-enc-h264.c | 50 ++++++++++++++++++++++++++++++++++++++---------
> ui/vnc.h | 1 +
> 2 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> index 840674dbdb..9dbfba3a16 100644
> --- a/ui/vnc-enc-h264.c
> +++ b/ui/vnc-enc-h264.c
> @@ -23,6 +23,8 @@
> */
>
> #include "qemu/osdep.h"
> +#include "system/runstate.h"
> +
> #include "vnc.h"
>
> #include <gst/gst.h>
> @@ -114,13 +116,33 @@ static GstElement *create_encoder(const char *encoder_name)
> return encoder;
> }
>
> -static void destroy_encoder_context(VncState *vs)
> +static void destroy_encoder_context(VncH264 *h264)
> {
> - gst_clear_object(&vs->h264->source);
> - gst_clear_object(&vs->h264->convert);
> - gst_clear_object(&vs->h264->gst_encoder);
> - gst_clear_object(&vs->h264->sink);
> - gst_clear_object(&vs->h264->pipeline);
> + GstStateChangeReturn state_change_ret;
> +
> + g_assert(h264 != NULL);
> +
> + VNC_DEBUG("Destroy h264 context.\n");
> +
> + /*
> + * Some encoders can hang indefinetly (i.e. nvh264enc) if
> + * the pipeline is not stopped before it is destroyed
> + * (Observed on Debian bookworm).
> + */
> + if (h264->pipeline != NULL) {
> + state_change_ret = gst_element_set_state(
> + h264->pipeline, GST_STATE_NULL);
> +
> + if (state_change_ret == GST_STATE_CHANGE_FAILURE) {
> + VNC_DEBUG("Unable to stop the GST pipeline\n");
> + }
> + }
> +
> + gst_clear_object(&h264->source);
> + gst_clear_object(&h264->convert);
> + gst_clear_object(&h264->gst_encoder);
> + gst_clear_object(&h264->sink);
> + gst_clear_object(&h264->pipeline);
> }
>
> static bool create_encoder_context(VncState *vs, int w, int h)
> @@ -132,7 +154,7 @@ static bool create_encoder_context(VncState *vs, int w, int h)
>
> if (vs->h264->sink) {
> if (w != vs->h264->width || h != vs->h264->height) {
> - destroy_encoder_context(vs);
> + destroy_encoder_context(vs->h264);
> }
> }
>
> @@ -239,10 +261,16 @@ static bool create_encoder_context(VncState *vs, int w, int h)
> return TRUE;
>
> error:
> - destroy_encoder_context(vs);
> + destroy_encoder_context(vs->h264);
> return FALSE;
> }
>
> +static void shutdown_h264(Notifier *n, void *opaque)
> +{
> + VncH264 *h264 = container_of(n, VncH264, shutdown_notifier);
> + destroy_encoder_context(h264);
> +}
> +
> bool vnc_h264_encoder_init(VncState *vs)
> {
> char *encoder_name;
> @@ -259,6 +287,8 @@ bool vnc_h264_encoder_init(VncState *vs)
>
> vs->h264 = g_new0(VncH264, 1);
> vs->h264->encoder_name = encoder_name;
> + vs->h264->shutdown_notifier.notify = shutdown_h264;
> + qemu_register_shutdown_notifier(&vs->h264->shutdown_notifier);
>
> VNC_DEBUG("Allow H264 using encoder '%s`\n", encoder_name);
>
> @@ -362,7 +392,9 @@ void vnc_h264_clear(VncState *vs)
> return;
> }
>
> - destroy_encoder_context(vs);
> + notifier_remove(&vs->h264->shutdown_notifier);
> +
> + destroy_encoder_context(vs->h264);
> g_free(vs->h264->encoder_name);
>
> g_clear_pointer(&vs->h264, g_free);
> diff --git a/ui/vnc.h b/ui/vnc.h
> index 789b18806b..ea52085b19 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -248,6 +248,7 @@ typedef struct VncH264 {
> size_t width;
> size_t height;
> guint keep_dirty;
> + Notifier shutdown_notifier;
> } VncH264;
> #endif
>
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context
2025-04-21 10:14 ` Marc-André Lureau
@ 2025-04-22 6:35 ` Dietmar Maurer
2025-04-22 6:39 ` Marc-André Lureau
2025-04-23 12:58 ` Daniel P. Berrangé
0 siblings, 2 replies; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-22 6:35 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
> On Fri, Apr 18, 2025 at 3:30 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
> >
> > Some encoders can hang indefinetly (i.e. nvh264enc) if
>
> indefinitely
>
> > the pipeline is not stopped before it is destroyed
> > (Observed on Debian bookworm).
>
> but why do you need the extra shutdown notifier?
Because Qemu does not close open VNC connections on shutdown.
and if the VNC connection is open, the h264 pipeline is still active,
which cause the Qemu process to hang (CTRL-C does not stop it, only kill -9)
- Dietmar
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context
2025-04-22 6:35 ` Dietmar Maurer
@ 2025-04-22 6:39 ` Marc-André Lureau
2025-04-22 7:03 ` Dietmar Maurer
2025-04-23 12:58 ` Daniel P. Berrangé
1 sibling, 1 reply; 47+ messages in thread
From: Marc-André Lureau @ 2025-04-22 6:39 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: qemu-devel
Hi
On Tue, Apr 22, 2025 at 10:37 AM Dietmar Maurer <dietmar@proxmox.com> wrote:
>
> > On Fri, Apr 18, 2025 at 3:30 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
> > >
> > > Some encoders can hang indefinetly (i.e. nvh264enc) if
> >
> > indefinitely
> >
> > > the pipeline is not stopped before it is destroyed
> > > (Observed on Debian bookworm).
> >
> > but why do you need the extra shutdown notifier?
>
> Because Qemu does not close open VNC connections on shutdown.
> and if the VNC connection is open, the h264 pipeline is still active,
> which cause the Qemu process to hang (CTRL-C does not stop it, only kill -9)
Given that h264 code depends on VNC state, can you make VNC
close/clean the connection instead?
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context
2025-04-22 6:39 ` Marc-André Lureau
@ 2025-04-22 7:03 ` Dietmar Maurer
2025-04-22 7:07 ` Marc-André Lureau
0 siblings, 1 reply; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-22 7:03 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
> On 22.4.2025 08:39 CEST Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
>
> Hi
>
> On Tue, Apr 22, 2025 at 10:37 AM Dietmar Maurer <dietmar@proxmox.com> wrote:
> >
> > > On Fri, Apr 18, 2025 at 3:30 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
> > > >
> > > > Some encoders can hang indefinetly (i.e. nvh264enc) if
> > >
> > > indefinitely
> > >
> > > > the pipeline is not stopped before it is destroyed
> > > > (Observed on Debian bookworm).
> > >
> > > but why do you need the extra shutdown notifier?
> >
> > Because Qemu does not close open VNC connections on shutdown.
> > and if the VNC connection is open, the h264 pipeline is still active,
> > which cause the Qemu process to hang (CTRL-C does not stop it, only kill -9)
>
> Given that h264 code depends on VNC state, can you make VNC
> close/clean the connection instead?
For all open VNC connections?
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context
2025-04-22 7:03 ` Dietmar Maurer
@ 2025-04-22 7:07 ` Marc-André Lureau
2025-04-22 7:17 ` Dietmar Maurer
0 siblings, 1 reply; 47+ messages in thread
From: Marc-André Lureau @ 2025-04-22 7:07 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: qemu-devel
Hi
On Tue, Apr 22, 2025 at 11:03 AM Dietmar Maurer <dietmar@proxmox.com> wrote:
>
>
> > On 22.4.2025 08:39 CEST Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> >
> >
> > Hi
> >
> > On Tue, Apr 22, 2025 at 10:37 AM Dietmar Maurer <dietmar@proxmox.com> wrote:
> > >
> > > > On Fri, Apr 18, 2025 at 3:30 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
> > > > >
> > > > > Some encoders can hang indefinetly (i.e. nvh264enc) if
> > > >
> > > > indefinitely
> > > >
> > > > > the pipeline is not stopped before it is destroyed
> > > > > (Observed on Debian bookworm).
> > > >
> > > > but why do you need the extra shutdown notifier?
> > >
> > > Because Qemu does not close open VNC connections on shutdown.
> > > and if the VNC connection is open, the h264 pipeline is still active,
> > > which cause the Qemu process to hang (CTRL-C does not stop it, only kill -9)
> >
> > Given that h264 code depends on VNC state, can you make VNC
> > close/clean the connection instead?
>
> For all open VNC connections?
>
Yes, we should have a cleanup path instead of shutdown notifiers.
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context
2025-04-22 7:07 ` Marc-André Lureau
@ 2025-04-22 7:17 ` Dietmar Maurer
0 siblings, 0 replies; 47+ messages in thread
From: Dietmar Maurer @ 2025-04-22 7:17 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
> On 22.4.2025 09:07 CEST Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> > >
> > > Given that h264 code depends on VNC state, can you make VNC
> > > close/clean the connection instead?
> >
> > For all open VNC connections?
> >
>
> Yes, we should have a cleanup path instead of shutdown notifiers.
I am finally confused. How do I close/clean the VNC connection without a shutdown notifier?
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context
2025-04-22 6:35 ` Dietmar Maurer
2025-04-22 6:39 ` Marc-André Lureau
@ 2025-04-23 12:58 ` Daniel P. Berrangé
1 sibling, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-23 12:58 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: Marc-André Lureau, qemu-devel
On Tue, Apr 22, 2025 at 08:35:52AM +0200, Dietmar Maurer wrote:
> > On Fri, Apr 18, 2025 at 3:30 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
> > >
> > > Some encoders can hang indefinetly (i.e. nvh264enc) if
> >
> > indefinitely
> >
> > > the pipeline is not stopped before it is destroyed
> > > (Observed on Debian bookworm).
> >
> > but why do you need the extra shutdown notifier?
>
> Because Qemu does not close open VNC connections on shutdown.
> and if the VNC connection is open, the h264 pipeline is still active,
> which cause the Qemu process to hang (CTRL-C does not stop it, only kill -9)
This is surely liable to be a race condition that causes crashes. The still
open VNC connection may still reference the encoder that you're destroying,
allowing the VNC worker thread to potentially access free'd memory.
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context
2025-04-18 11:29 ` [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context Dietmar Maurer
2025-04-21 10:14 ` Marc-André Lureau
@ 2025-04-23 12:57 ` Daniel P. Berrangé
1 sibling, 0 replies; 47+ messages in thread
From: Daniel P. Berrangé @ 2025-04-23 12:57 UTC (permalink / raw)
To: Dietmar Maurer; +Cc: marcandre.lureau, qemu-devel
On Fri, Apr 18, 2025 at 01:29:53PM +0200, Dietmar Maurer wrote:
> Some encoders can hang indefinetly (i.e. nvh264enc) if
> the pipeline is not stopped before it is destroyed
> (Observed on Debian bookworm).
>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
> ui/vnc-enc-h264.c | 50 ++++++++++++++++++++++++++++++++++++++---------
> ui/vnc.h | 1 +
> 2 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> index 840674dbdb..9dbfba3a16 100644
> --- a/ui/vnc-enc-h264.c
> +++ b/ui/vnc-enc-h264.c
> @@ -23,6 +23,8 @@
> */
>
> #include "qemu/osdep.h"
> +#include "system/runstate.h"
> +
> #include "vnc.h"
>
> #include <gst/gst.h>
> @@ -114,13 +116,33 @@ static GstElement *create_encoder(const char *encoder_name)
> return encoder;
> }
>
> -static void destroy_encoder_context(VncState *vs)
> +static void destroy_encoder_context(VncH264 *h264)
> {
> - gst_clear_object(&vs->h264->source);
> - gst_clear_object(&vs->h264->convert);
> - gst_clear_object(&vs->h264->gst_encoder);
> - gst_clear_object(&vs->h264->sink);
> - gst_clear_object(&vs->h264->pipeline);
> + GstStateChangeReturn state_change_ret;
> +
> + g_assert(h264 != NULL);
> +
> + VNC_DEBUG("Destroy h264 context.\n");
> +
> + /*
> + * Some encoders can hang indefinetly (i.e. nvh264enc) if
> + * the pipeline is not stopped before it is destroyed
> + * (Observed on Debian bookworm).
> + */
> + if (h264->pipeline != NULL) {
> + state_change_ret = gst_element_set_state(
> + h264->pipeline, GST_STATE_NULL);
> +
> + if (state_change_ret == GST_STATE_CHANGE_FAILURE) {
> + VNC_DEBUG("Unable to stop the GST pipeline\n");
> + }
> + }
> +
> + gst_clear_object(&h264->source);
> + gst_clear_object(&h264->convert);
> + gst_clear_object(&h264->gst_encoder);
> + gst_clear_object(&h264->sink);
> + gst_clear_object(&h264->pipeline);
> }
>
> static bool create_encoder_context(VncState *vs, int w, int h)
> @@ -132,7 +154,7 @@ static bool create_encoder_context(VncState *vs, int w, int h)
>
> if (vs->h264->sink) {
> if (w != vs->h264->width || h != vs->h264->height) {
> - destroy_encoder_context(vs);
> + destroy_encoder_context(vs->h264);
> }
> }
>
> @@ -239,10 +261,16 @@ static bool create_encoder_context(VncState *vs, int w, int h)
> return TRUE;
>
> error:
> - destroy_encoder_context(vs);
> + destroy_encoder_context(vs->h264);
> return FALSE;
> }
>
> +static void shutdown_h264(Notifier *n, void *opaque)
> +{
> + VncH264 *h264 = container_of(n, VncH264, shutdown_notifier);
> + destroy_encoder_context(h264);
> +}
> +
> bool vnc_h264_encoder_init(VncState *vs)
> {
> char *encoder_name;
> @@ -259,6 +287,8 @@ bool vnc_h264_encoder_init(VncState *vs)
>
> vs->h264 = g_new0(VncH264, 1);
> vs->h264->encoder_name = encoder_name;
> + vs->h264->shutdown_notifier.notify = shutdown_h264;
> + qemu_register_shutdown_notifier(&vs->h264->shutdown_notifier);
This is broken - in some configurations, when a guest shutdown is
actioned, QEMU will merely pause guest CPU execution. The mgmt
app can then reset the machine and resume CPU execution. IOW you
can't assume QEMU is about to exit from a shutdown notifier.
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 :|
^ permalink raw reply [flat|nested] 47+ messages in thread