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 <diet...@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 :|
