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

Dietmar Maurer posted 7 patches 7 months, 3 weeks ago
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
[PATCH v5 7/7] h264: stop gstreamer pipeline before destroying, cleanup on exit
Posted by Dietmar Maurer 7 months, 3 weeks ago
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/include/system/system.h b/include/system/system.h
index a7effe7dfd..9226e60050 100644
--- a/include/system/system.h
+++ b/include/system/system.h
@@ -109,6 +109,7 @@ bool defaults_enabled(void);
 void qemu_init(int argc, char **argv);
 int qemu_main_loop(void);
 void qemu_cleanup(int);
+void qemu_cleanup_displays(void);
 
 extern QemuOptsList qemu_legacy_drive_opts;
 extern QemuOptsList qemu_common_drive_opts;
diff --git a/include/ui/console.h b/include/ui/console.h
index 46b3128185..ff46e9fe98 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -458,6 +458,7 @@ int vnc_display_password(const char *id, const char *password);
 int vnc_display_pw_expire(const char *id, time_t expires);
 void vnc_parse(const char *str);
 int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
+void vnc_cleanup(void);
 bool vnc_display_reload_certs(const char *id,  Error **errp);
 bool vnc_display_update(DisplayUpdateOptionsVNC *arg, Error **errp);
 
diff --git a/system/runstate.c b/system/runstate.c
index 272801d307..0cb3ba5ec1 100644
--- a/system/runstate.c
+++ b/system/runstate.c
@@ -924,6 +924,8 @@ void qemu_cleanup(int status)
     job_cancel_sync_all();
     bdrv_close_all();
 
+    qemu_cleanup_displays();
+
     /* vhost-user must be cleaned up before chardevs.  */
     tpm_cleanup();
     net_cleanup();
diff --git a/system/vl.c b/system/vl.c
index c17945c493..a781ebd77b 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2679,6 +2679,13 @@ static void qemu_maybe_daemonize(const char *pid_file)
     }
 }
 
+void qemu_cleanup_displays(void)
+{
+#ifdef CONFIG_VNC
+   vnc_cleanup();
+#endif
+}
+
 static void qemu_init_displays(void)
 {
     DisplayState *ds;
diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
index 98055c095f..6618f156b4 100644
--- a/ui/vnc-enc-h264.c
+++ b/ui/vnc-enc-h264.c
@@ -95,6 +95,24 @@ static GstElement *create_encoder(const char *encoder_name)
 
 static void destroy_encoder_context(VncState *vs)
 {
+    GstStateChangeReturn state_change_ret;
+
+    VNC_DEBUG("Destroy h264 context.\n");
+
+    /*
+     * Some encoders can hang indefinitely (i.e. nvh264enc) if
+     * the pipeline is not stopped before it is destroyed
+     * (Observed on Debian bookworm).
+     */
+    if (vs->h264->pipeline != NULL) {
+        state_change_ret = gst_element_set_state(
+            vs->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(&vs->h264->source);
     gst_clear_object(&vs->h264->convert);
     gst_clear_object(&vs->h264->gst_encoder);
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
+        }
+    }
+}
+
 static void vnc_register_config(void)
 {
     qemu_add_opts(&qemu_vnc_opts);
-- 
2.39.5
Re: [PATCH v5 7/7] h264: stop gstreamer pipeline before destroying, cleanup on exit
Posted by Daniel P. Berrangé 7 months, 1 week ago
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 :|