Hi On Wed, Apr 23, 2025 at 3:46 PM Dietmar Maurer <diet...@proxmox.com> wrote: > > > On 19.4.2025 07:24 CEST Marc-André Lureau <marcandre.lur...@gmail.com> > > wrote: > > > > > > Hi > > > > On Fri, Apr 18, 2025 at 3:41 PM Dietmar Maurer <diet...@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 <diet...@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