Re: [PATCH v3 9/9] h264: register shutdown notifiers, stop pipeline in destroy_encoder_context

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

On Fri, Apr 18, 2025 at 01:29:53PM +0200, Dietmar Maurer wrote:
> Some encoders can hang indefinetly (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>
> ---
>  ui/vnc-enc-h264.c | 50 ++++++++++++++++++++++++++++++++++++++---------
>  ui/vnc.h          |  1 +
>  2 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/ui/vnc-enc-h264.c b/ui/vnc-enc-h264.c
> index 840674dbdb..9dbfba3a16 100644
> --- a/ui/vnc-enc-h264.c
> +++ b/ui/vnc-enc-h264.c
> @@ -23,6 +23,8 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "system/runstate.h"
> +
>  #include "vnc.h"
>  
>  #include <gst/gst.h>
> @@ -114,13 +116,33 @@ static GstElement *create_encoder(const char *encoder_name)
>      return encoder;
>  }
>  
> -static void destroy_encoder_context(VncState *vs)
> +static void destroy_encoder_context(VncH264 *h264)
>  {
> -    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);
> +    GstStateChangeReturn state_change_ret;
> +
> +    g_assert(h264 != NULL);
> +
> +    VNC_DEBUG("Destroy h264 context.\n");
> +
> +    /*
> +     * Some encoders can hang indefinetly (i.e. nvh264enc) if
> +     * the pipeline is not stopped before it is destroyed
> +     * (Observed on Debian bookworm).
> +     */
> +    if (h264->pipeline != NULL) {
> +        state_change_ret = gst_element_set_state(
> +            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(&h264->source);
> +    gst_clear_object(&h264->convert);
> +    gst_clear_object(&h264->gst_encoder);
> +    gst_clear_object(&h264->sink);
> +    gst_clear_object(&h264->pipeline);
>  }
>  
>  static bool create_encoder_context(VncState *vs, int w, int h)
> @@ -132,7 +154,7 @@ static bool create_encoder_context(VncState *vs, int w, int h)
>  
>      if (vs->h264->sink) {
>          if (w != vs->h264->width || h != vs->h264->height) {
> -            destroy_encoder_context(vs);
> +            destroy_encoder_context(vs->h264);
>          }
>      }
>  
> @@ -239,10 +261,16 @@ static bool create_encoder_context(VncState *vs, int w, int h)
>      return TRUE;
>  
>   error:
> -    destroy_encoder_context(vs);
> +    destroy_encoder_context(vs->h264);
>      return FALSE;
>  }
>  
> +static void shutdown_h264(Notifier *n, void *opaque)
> +{
> +    VncH264 *h264 =  container_of(n, VncH264, shutdown_notifier);
> +    destroy_encoder_context(h264);
> +}
> +
>  bool vnc_h264_encoder_init(VncState *vs)
>  {
>      char *encoder_name;
> @@ -259,6 +287,8 @@ bool vnc_h264_encoder_init(VncState *vs)
>  
>      vs->h264 = g_new0(VncH264, 1);
>      vs->h264->encoder_name = encoder_name;
> +    vs->h264->shutdown_notifier.notify = shutdown_h264;
> +    qemu_register_shutdown_notifier(&vs->h264->shutdown_notifier);

This is broken - in some configurations, when a guest shutdown is
actioned, QEMU will merely pause guest CPU execution. The mgmt
app can then reset the machine and resume CPU execution. IOW you
can't assume QEMU is about to exit from a shutdown notifier.


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