Re: [PATCH v5 7/7] h264: stop gstreamer pipeline before destroying, cleanup on exit

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

On Wed, Apr 30, 2025 at 09:25:24AM +0200, Dietmar Maurer wrote:
> Some encoders can hang indefinitely (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>
> ---
>  include/system/system.h |  1 +
>  include/ui/console.h    |  1 +
>  system/runstate.c       |  2 ++
>  system/vl.c             |  7 +++++++
>  ui/vnc-enc-h264.c       | 18 ++++++++++++++++++
>  ui/vnc.c                | 15 +++++++++++++++
>  6 files changed, 44 insertions(+)
> 

> diff --git a/ui/vnc.c b/ui/vnc.c
> index c707b9da37..578d9eea32 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -4368,6 +4368,21 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>      return 0;
>  }
>  
> +void vnc_cleanup(void)
> +{
> +    VncDisplay *vd;
> +    VncState *vs;
> +
> +    QTAILQ_FOREACH(vd, &vnc_displays, next) {
> +        QTAILQ_FOREACH(vs, &vd->clients, next) {
> +#ifdef CONFIG_GSTREAMER
> +            /* correctly close all h264 encoder pipelines */
> +            vnc_h264_clear(vs);
> +#endif

How is this safe to do ?

If we have an active client, we have to assume that the jobs
thread may be in the middle of processing an update, and thus
the jobs thread wil be using the h264 encoder. If we blindly
tear down the encoder I think we're liable to trigger
non-deterministic crashes in QEMU in the vnc jobs thread
accessing free'd memory.

> +        }
> +    }
> +}
> +
>  static void vnc_register_config(void)
>  {
>      qemu_add_opts(&qemu_vnc_opts);
> -- 
> 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 :|