Re: [PATCH v3 5/9] h264: new vnc option to configure h264 at server side

["Daniel P. Berrangé" <berrange@redhat.com>]

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