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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

Hi

On Fri, Apr 18, 2025 at 3:30 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
>
> Some encoders can hang indefinetly (i.e. nvh264enc) if

indefinitely

> the pipeline is not stopped before it is destroyed
> (Observed on Debian bookworm).

but why do you need the extra shutdown notifier?

>
> 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);
>
>      VNC_DEBUG("Allow H264 using encoder '%s`\n", encoder_name);
>
> @@ -362,7 +392,9 @@ void vnc_h264_clear(VncState *vs)
>          return;
>      }
>
> -    destroy_encoder_context(vs);
> +    notifier_remove(&vs->h264->shutdown_notifier);
> +
> +    destroy_encoder_context(vs->h264);
>      g_free(vs->h264->encoder_name);
>
>      g_clear_pointer(&vs->h264, g_free);
> diff --git a/ui/vnc.h b/ui/vnc.h
> index 789b18806b..ea52085b19 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -248,6 +248,7 @@ typedef struct VncH264 {
>      size_t width;
>      size_t height;
>      guint keep_dirty;
> +    Notifier shutdown_notifier;
>  } VncH264;
>  #endif
>
> --
> 2.39.5
>