Re: [PATCH v4] channel-display-gst: Use h/w based decoders with Intel GPUs if possible (v4)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Frediano,

> 
> Hi Vivek,
>    I finally came up with something better.
> 
> See https://gitlab.freedesktop.org/fziglio/spice-gtk/-
> /commits/hw_decoding/,
> it contains
> - https://gitlab.freedesktop.org/fziglio/spice-gtk/-
> /commit/52d8622bf620f2ad47d6f001926ed918d50d30cd,
> fix some leaks, not due to your patch but good for testing, opened a
> reparate PR at https://gitlab.freedesktop.org/spice/spice-gtk/-
> /merge_requests/123;
> - https://gitlab.freedesktop.org/fziglio/spice-gtk/-
> /commit/de6d228349607d8d0d711f5c2a11f791167c8c25
> required update for spice-common submodule;
> - https://gitlab.freedesktop.org/fziglio/spice-gtk/-
> /commit/e481a2b84e3d02e5d0c38ee5ae8d268fba15dc77
> fix dangling pointers in free_pipeline (remove some warnings running
> your patch);
> - https://gitlab.freedesktop.org/fziglio/spice-gtk/-
> /commit/8d59e88e88553497049a9fc380cac7395225bc75
> (fixup) the proposed change in the previous mail to avoid some leaks
> and memory errors;
> - https://gitlab.freedesktop.org/fziglio/spice-gtk/-
> /commit/c01fa619b836b413fecd594a97a03cbf4b655b04
> (fixup) really minor style change;
I looked at the commits and they indeed look better. Thank you for taking
the time to add these fixups; I'll include them in the next version.

> - https://gitlab.freedesktop.org/fziglio/spice-gtk/-
> /commit/22c032c67528e0899b6e2faf56ffa9584c208755
> the fallback patch I described in previous email, turned out it was
> easier than expected.
Nice! This does make the decoding process more robust. I am curious how
you tested this fallback scenario though?

Thanks,
Vivek

> 
> Regards,
>    Frediano
> 
> Il giorno dom 29 ott 2023 alle ore 20:32 Frediano Ziglio
> <freddy77@xxxxxxxxx> ha scritto:
> >
> > Il giorno mar 17 ott 2023 alle ore 08:46 Vivek Kasireddy
> > <vivek.kasireddy@xxxxxxxxx> ha scritto:
> > >
> > > We first try to detect if an Intel GPU is available (by looking into
> > > udev's database) and then probe Gstreamer's registry cache to see
> > > if there is h/w based decoder (element) available for the incoming
> > > video codec format. If both these conditions are satisfied (i.e,
> > > Intel Media SDK Gstreamer plugin (libgstmsdk.so) and associated
> > > libraries are properly installed), we then create a simple decode
> > > pipeline using appropriate h/w based decoder and post-processor
> > > elements instead of relying on playbin -- which may not be able to
> > > auto-select these elements.
> > >
> > > For example, if the incoming codec format is h264, we then create
> > > a pipeline using msdkh264dec and vaapipostproc elements instead of
> > > avdec_h264 and videoconvert.
> > >
> > > v2: (addressed some review comments from Frediano)
> > > - s/gboolean/bool, s/TRUE/true, s/FALSE/false
> > > - Used a single cleanup label instead of multiple while instantiating
> > >   elements
> > > - Moved the code that launches the Gst pipeline into a helper that
> > >   is used while trying h/w based plugins
> > >
> > > v3:
> > > - Updated the patch to reflect the new signature and return value of
> > >   spice_udev_detect_gpu().
> > >
> > > v4:
> > > - Include the string "_hw_" in the function that finds best h/w plugins
> > > - Change type and determine plugins array length using G_N_ELEMENTS
> > > - Free vpp_name immediately after using it to prevent leak
> > > - Rebase on master
> > >
> > > Cc: Frediano Ziglio <freddy77@xxxxxxxxx>
> > > Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > > Cc: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> > > Cc: Dongwon Kim <dongwon.kim@xxxxxxxxx>
> > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> > > Signed-off-by: Hazwan Arif Mazlan <hazwan.arif.mazlan@xxxxxxxxx>
> > > Signed-off-by: Jin Chung Teng <jin.chung.teng@xxxxxxxxx>
> > > ---
> > >  src/channel-display-gst.c | 217 ++++++++++++++++++++++++++++++++++-
> ---
> > >  1 file changed, 195 insertions(+), 22 deletions(-)
> > >
> >
> > Hi,
> >    the patch seems good.
> >
> > Not sure what I did with my system but I'm not able to test it very
> > well. The system keeps complaining about some issue with the format.
> > I wanted to add a patch for compatibility in case the server is
> > sending some profile not supported by H/W decoder (mainly saving first
> > encoded frames till an error or an output frame and try with S/W
> > decoder) but I got stuck with my system errors.
> >
> > In the meantime I manage to find the memory issue you had that caused:
> >
> > (remote-viewer:2272): GStreamer-CRITICAL **: 18:30:37.263:
> > Trying to dispose object "appsink26", but it still has a parent "pipeline28".
> > You need to let the parent manage the object instead of unreffing the
> > object directly.
> >
> > The reason is due to a missing reference to the sink, I fixed it and
> > changed the order of free (reverse of allocations), see
> >
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index de606b56..82c500c8 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -628,7 +628,7 @@ static bool try_intel_hw_pipeline(SpiceGstDecoder
> *decoder)
> >                   "drop", FALSE,
> >                   NULL);
> >      gst_caps_unref(sink_caps);
> > -    decoder->appsink = GST_APP_SINK(sink);
> > +    decoder->appsink = GST_APP_SINK(gst_object_ref(sink));
> >
> >      if (hand_pipeline_to_widget(decoder->base.stream,
> >          GST_PIPELINE(pipeline))) {
> > @@ -642,6 +642,8 @@ static bool try_intel_hw_pipeline(SpiceGstDecoder
> *decoder)
> >          if (!gst_element_link_many(src, parser, hw_decoder, vpp,
> >                                     sink, NULL)) {
> >              spice_warning("error linking elements");
> > +            /* ownership moved to pipeline, don't unreference these */
> > +            src = parser = hw_decoder = vpp = sink = NULL;
> >              goto err;
> >          }
> >      } else {
> > @@ -649,6 +651,8 @@ static bool try_intel_hw_pipeline(SpiceGstDecoder
> *decoder)
> >                           vpp, sink, NULL);
> >          if (!gst_element_link_many(src, hw_decoder, vpp, sink, NULL)) {
> >              spice_warning("error linking elements");
> > +            /* ownership moved to pipeline, don't unreference these */
> > +            src = hw_decoder = vpp = sink = NULL;
> >              goto err;
> >          }
> >      }
> > @@ -657,20 +661,31 @@ static bool
> > try_intel_hw_pipeline(SpiceGstDecoder *decoder)
> >      return launch_pipeline(decoder);
> >
> >  err:
> > -    if (src) {
> > -        gst_object_unref(src);
> > +    if (decoder->appsink) {
> > +        gst_object_unref(decoder->appsink);
> > +        decoder->appsink = NULL;
> >      }
> > -    if (sink) {
> > -        gst_object_unref(sink);
> > +    if (decoder->appsrc) {
> > +        gst_object_unref(decoder->appsrc);
> > +        decoder->appsrc = NULL;
> >      }
> > -    if (use_parser) {
> > -        gst_object_unref(parser);
> > +    if (pipeline) {
> > +        gst_object_unref(pipeline);
> > +    }
> > +    if (vpp) {
> > +        gst_object_unref(vpp);
> >      }
> >      if (hw_decoder) {
> >          gst_object_unref(hw_decoder);
> >      }
> > -    if (vpp) {
> > -        gst_object_unref(vpp);
> > +    if (parser) {
> > +        gst_object_unref(parser);
> > +    }
> > +    if (sink) {
> > +        gst_object_unref(sink);
> > +    }
> > +    if (src) {
> > +        gst_object_unref(src);
> >      }
> >      return false;
> >  }
> > --
> >
> > Frediano
> >
> > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > index 5c9927b..de606b5 100644
> > > --- a/src/channel-display-gst.c
> > > +++ b/src/channel-display-gst.c
> > > @@ -20,6 +20,7 @@
> > >  #include "spice-common.h"
> > >  #include "spice-channel-priv.h"
> > >  #include "common/recorder.h"
> > > +#include "common/udev.h"
> > >
> > >  #include "channel-display-priv.h"
> > >
> > > @@ -489,13 +490,205 @@ deep_element_added_cb(GstBin *pipeline,
> GstBin *bin, GstElement *element,
> > >      }
> > >  }
> > >
> > > -static gboolean create_pipeline(SpiceGstDecoder *decoder)
> > > +static gchar *find_best_hw_plugin(const gchar *dec_name)
> > > +{
> > > +    static const char plugins[][8] = {"msdk", "va", "vaapi"};
> > > +    GstRegistry *registry;
> > > +    GstPluginFeature *feature;
> > > +    gchar *feature_name;
> > > +    int i;
> > > +
> > > +    registry = gst_registry_get();
> > > +    if (!registry) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    for (i = 0; i < G_N_ELEMENTS(plugins); i++) {
> > > +        feature_name = !dec_name ? g_strconcat(plugins[i], "postproc",
> NULL) :
> > > +                       g_strconcat(plugins[i], dec_name, "dec", NULL);
> > > +        feature = gst_registry_lookup_feature(registry, feature_name);
> > > +        if (!feature) {
> > > +            g_free(feature_name);
> > > +            feature_name = NULL;
> > > +            continue;
> > > +        }
> > > +        gst_object_unref(feature);
> > > +        break;
> > > +    }
> > > +    return feature_name;
> > > +}
> > > +
> > > +static bool launch_pipeline(SpiceGstDecoder *decoder)
> > >  {
> > >      GstBus *bus;
> > > +
> > > +    if (decoder->appsink) {
> > > +        GstAppSinkCallbacks appsink_cbs = { NULL };
> > > +        appsink_cbs.new_sample = new_sample;
> > > +        gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs,
> decoder, NULL);
> > > +        gst_app_sink_set_max_buffers(decoder->appsink,
> MAX_DECODED_FRAMES);
> > > +        gst_app_sink_set_drop(decoder->appsink, FALSE);
> > > +    }
> > > +    bus = gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline));
> > > +    gst_bus_add_watch(bus, handle_pipeline_message, decoder);
> > > +    gst_object_unref(bus);
> > > +
> > > +    decoder->clock = gst_pipeline_get_clock(GST_PIPELINE(decoder-
> >pipeline));
> > > +
> > > +    if (gst_element_set_state(decoder->pipeline, GST_STATE_PLAYING) ==
> GST_STATE_CHANGE_FAILURE) {
> > > +        SPICE_DEBUG("GStreamer error: Unable to set the pipeline to the
> playing state.");
> > > +        free_pipeline(decoder);
> > > +        return false;
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > +
> > > +static bool try_intel_hw_pipeline(SpiceGstDecoder *decoder)
> > > +{
> > > +    GstElement *pipeline = NULL, *src = NULL, *sink = NULL;
> > > +    GstElement *parser = NULL, *hw_decoder = NULL, *vpp = NULL;
> > > +    GstCaps *src_caps, *sink_caps;
> > > +    int codec_type = decoder->base.codec_type;
> > > +    const gchar *dec_name = gst_opts[codec_type].name;
> > > +    gchar *hw_dec_name, *vpp_name, *parser_name;
> > > +    gboolean use_parser;
> > > +
> > > +    use_parser = codec_type == SPICE_VIDEO_CODEC_TYPE_H264 ||
> > > +                 codec_type == SPICE_VIDEO_CODEC_TYPE_H265;
> > > +
> > > +    src = gst_element_factory_make("appsrc", NULL);
> > > +    if (!src) {
> > > +        spice_warning("error upon creation of 'appsrc' element");
> > > +        return false;
> > > +    }
> > > +    sink = gst_element_factory_make("appsink", NULL);
> > > +    if (!sink) {
> > > +        spice_warning("error upon creation of 'appsink' element");
> > > +        goto err;
> > > +    }
> > > +
> > > +    if (use_parser) {
> > > +        parser_name = g_strconcat(dec_name, "parse", NULL);
> > > +        parser = gst_element_factory_make(parser_name, NULL);
> > > +        g_free(parser_name);
> > > +        if (!parser) {
> > > +            spice_warning("error upon creation of 'parser' element");
> > > +            goto err;
> > > +        }
> > > +    }
> > > +
> > > +    hw_dec_name = find_best_hw_plugin(dec_name);
> > > +    if (!hw_dec_name) {
> > > +        spice_warning("error finding suitable plugin for decode");
> > > +        goto err;
> > > +    }
> > > +    hw_decoder = gst_element_factory_make(hw_dec_name, NULL);
> > > +    g_free(hw_dec_name);
> > > +    if (!hw_decoder) {
> > > +        spice_warning("error upon creation of 'decoder' element");
> > > +        goto err;
> > > +    }
> > > +    vpp_name = find_best_hw_plugin(NULL);
> > > +    if (!vpp_name) {
> > > +        spice_warning("error finding suitable plugin for postproc");
> > > +        goto err;
> > > +    }
> > > +    vpp = gst_element_factory_make(vpp_name, NULL);
> > > +    g_free(vpp_name);
> > > +    if (!vpp) {
> > > +        spice_warning("error upon creation of 'vpp' element");
> > > +        goto err;
> > > +    }
> > > +    g_object_set(vpp,
> > > +                 "format", GST_VIDEO_FORMAT_BGRx,
> > > +                 NULL);
> > > +
> > > +    pipeline = gst_pipeline_new(NULL);
> > > +    if (!pipeline) {
> > > +        spice_warning("error upon creation of 'pipeline' element");
> > > +        goto err;
> > > +    }
> > > +
> > > +    src_caps = gst_caps_from_string(gst_opts[codec_type].dec_caps);
> > > +    g_object_set(src,
> > > +                 "caps", src_caps,
> > > +                 "is-live", TRUE,
> > > +                 "format", GST_FORMAT_TIME,
> > > +                 "max-bytes", G_GINT64_CONSTANT(0),
> > > +                 "block", TRUE,
> > > +                 NULL);
> > > +    gst_caps_unref(src_caps);
> > > +    decoder->appsrc = GST_APP_SRC(gst_object_ref(src));
> > > +
> > > +    sink_caps = gst_caps_from_string("video/x-raw,format=BGRx");
> > > +    g_object_set(sink,
> > > +                 "caps", sink_caps,
> > > +                 "sync", FALSE,
> > > +                 "drop", FALSE,
> > > +                 NULL);
> > > +    gst_caps_unref(sink_caps);
> > > +    decoder->appsink = GST_APP_SINK(sink);
> > > +
> > > +    if (hand_pipeline_to_widget(decoder->base.stream,
> > > +        GST_PIPELINE(pipeline))) {
> > > +        spice_warning("error handing pipeline to widget");
> > > +        goto err;
> > > +    }
> > > +
> > > +    if (use_parser) {
> > > +        gst_bin_add_many(GST_BIN(pipeline), src, parser, hw_decoder,
> > > +                         vpp, sink, NULL);
> > > +        if (!gst_element_link_many(src, parser, hw_decoder, vpp,
> > > +                                   sink, NULL)) {
> > > +            spice_warning("error linking elements");
> > > +            goto err;
> > > +        }
> > > +    } else {
> > > +        gst_bin_add_many(GST_BIN(pipeline), src, hw_decoder,
> > > +                         vpp, sink, NULL);
> > > +        if (!gst_element_link_many(src, hw_decoder, vpp, sink, NULL)) {
> > > +            spice_warning("error linking elements");
> > > +            goto err;
> > > +        }
> > > +    }
> > > +
> > > +    decoder->pipeline = pipeline;
> > > +    return launch_pipeline(decoder);
> > > +
> > > +err:
> > > +    if (src) {
> > > +        gst_object_unref(src);
> > > +    }
> > > +    if (sink) {
> > > +        gst_object_unref(sink);
> > > +    }
> > > +    if (use_parser) {
> > > +        gst_object_unref(parser);
> > > +    }
> > > +    if (hw_decoder) {
> > > +        gst_object_unref(hw_decoder);
> > > +    }
> > > +    if (vpp) {
> > > +        gst_object_unref(vpp);
> > > +    }
> > > +    return false;
> > > +}
> > > +
> > > +static gboolean create_pipeline(SpiceGstDecoder *decoder)
> > > +{
> > >      GstElement *playbin, *sink;
> > >      SpiceGstPlayFlags flags;
> > >      GstCaps *caps;
> > >      static bool playbin3_supported = true;
> > > +    GpuVendor vendor = spice_udev_detect_gpu(INTEL_VENDOR_ID);
> > > +
> > > +    if (vendor == VENDOR_GPU_DETECTED ||
> > > +        vendor == VENDOR_GPU_UNKNOWN) {
> > > +        if (try_intel_hw_pipeline(decoder)) {
> > > +            return TRUE;
> > > +        }
> > > +    }
> > >
> > >      playbin = playbin3_supported ?
> > >                gst_element_factory_make("playbin3", "playbin") : NULL;
> > > @@ -571,29 +764,9 @@ static gboolean
> create_pipeline(SpiceGstDecoder *decoder)
> > >      g_warn_if_fail(decoder->appsrc == NULL);
> > >      decoder->pipeline = playbin;
> > >
> > > -    if (decoder->appsink) {
> > > -        GstAppSinkCallbacks appsink_cbs = { NULL };
> > > -        appsink_cbs.new_sample = new_sample;
> > > -        gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs,
> decoder, NULL);
> > > -        gst_app_sink_set_max_buffers(decoder->appsink,
> MAX_DECODED_FRAMES);
> > > -        gst_app_sink_set_drop(decoder->appsink, FALSE);
> > > -    }
> > > -    bus = gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline));
> > > -    gst_bus_add_watch(bus, handle_pipeline_message, decoder);
> > > -    gst_object_unref(bus);
> > > -
> > > -    decoder->clock = gst_pipeline_get_clock(GST_PIPELINE(decoder-
> >pipeline));
> > > -
> > > -    if (gst_element_set_state(decoder->pipeline, GST_STATE_PLAYING) ==
> GST_STATE_CHANGE_FAILURE) {
> > > -        SPICE_DEBUG("GStreamer error: Unable to set the pipeline to the
> playing state.");
> > > -        free_pipeline(decoder);
> > > -        return FALSE;
> > > -    }
> > > -
> > > -    return TRUE;
> > > +    return launch_pipeline(decoder);
> > >  }
> > >
> > > -
> > >  /* ---------- VideoDecoder's public API ---------- */
> > >
> > >  static void spice_gst_decoder_reschedule(VideoDecoder
> *video_decoder)
> > > --
> > > 2.39.2
> > >




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]