Re: [Spice-devel] [PATCH v4] channel-display-gst: Use h/w based decoders with Intel GPUs if possible (v4)
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 > ha scritto: > > > > Il giorno mar 17 ott 2023 alle ore 08:46 Vivek Kasireddy > > 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 > > > Cc: Gerd Hoffmann > > > Cc: Marc-André Lureau > > > Cc: Dongwon Kim > > > Signed-off-by: Vivek Kasireddy > > > Signed-off-by: Hazwan Arif Mazlan > > > Signed-off-by: Jin Chung Teng > > > --- > > > 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 =
Re: [Spice-devel] [PATCH v4] channel-display-gst: Use h/w based decoders with Intel GPUs if possible (v4)
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
ha scritto:
>
> Il giorno mar 17 ott 2023 alle ore 08:46 Vivek Kasireddy
> 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
> > Cc: Gerd Hoffmann
> > Cc: Marc-André Lureau
> > Cc: Dongwon Kim
> > Signed-off-by: Vivek Kasireddy
> > Signed-off-by: Hazwan Arif Mazlan
> > Signed-off-by: Jin Chung Teng
> > ---
> > 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 */
> +
