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; - https://gitlab.freedesktop.org/fziglio/spice-gtk/-/commit/22c032c67528e0899b6e2faf56ffa9584c208755 the fallback patch I described in previous email, turned out it was easier than expected. 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 > >