[Spice-devel] [PATCH] channel-display-gst: Use h/w based decoders with Intel GPUs if possible
Frediano Ziglio
freddy77 at gmail.com
Thu Apr 27 19:51:55 UTC 2023
Il giorno gio 27 apr 2023 alle ore 07:37 Vivek Kasireddy
<vivek.kasireddy at intel.com> 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.
>
nice!
> Cc: Frediano Ziglio <freddy77 at gmail.com>
> Cc: Gerd Hoffmann <kraxel at redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau at redhat.com>
> Cc: Dongwon Kim <dongwon.kim at intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
> Signed-off-by: Mazlan, Hazwan Arif <hazwan.arif.mazlan at intel.com>
> Signed-off-by: Teng, Jin Chung <jin.chung.teng at intel.com>
> ---
> meson.build | 3 +
> src/channel-display-gst.c | 195 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 198 insertions(+)
>
> diff --git a/meson.build b/meson.build
> index c163a44..e373544 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -216,6 +216,9 @@ foreach dep : deps
> spice_glib_deps += dependency(dep, version: gstreamer_version_info)
> endforeach
>
> +#udev
> +spice_glib_deps += dependency('libudev', required : true)
> +
> # builtin-mjpeg
> spice_gtk_has_builtin_mjpeg = false
> if get_option('builtin-mjpeg')
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 36db3a3..e7df83a 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -27,6 +27,7 @@
> #include <gst/app/gstappsrc.h>
> #include <gst/app/gstappsink.h>
> #include <gst/video/gstvideometa.h>
> +#include <libudev.h>
>
>
> typedef struct SpiceGstFrame SpiceGstFrame;
> @@ -64,6 +65,8 @@ typedef struct SpiceGstDecoder {
> /* Decoded frames are big so limit how many are queued by GStreamer */
> #define MAX_DECODED_FRAMES 2
>
> +#define INTEL_GFX_DRV_NAME "i915"
> +
> /* GstPlayFlags enum is in plugin's header which should not be exported.
> * https://bugzilla.gnome.org/show_bug.cgi?id=784279
> */
> @@ -489,6 +492,190 @@ deep_element_added_cb(GstBin *pipeline, GstBin *bin, GstElement *element,
> }
> }
>
> +static gboolean detect_intel_gpu()
I would prefer bool, true and false, C99, after 23 years I think we
can assume we have them.
> +{
> + struct udev *udev;
> + struct udev_device *udev_dev;
> + struct udev_enumerate *udev_enum;
> + struct udev_list_entry *entry, *devices;
> + const char *path, *driver;
> + gboolean found = FALSE;
> +
> + udev = udev_new();
> + if (!udev) {
> + return FALSE;
> + }
> +
> + udev_enum = udev_enumerate_new(udev);
> + if (udev_enum) {
> + udev_enumerate_add_match_subsystem(udev_enum, "pci");
> + udev_enumerate_scan_devices(udev_enum);
> + devices = udev_enumerate_get_list_entry(udev_enum);
> +
> + udev_list_entry_foreach(entry, devices) {
> + path = udev_list_entry_get_name(entry);
> + udev_dev = udev_device_new_from_syspath(udev, path);
> +
> + driver = udev_device_get_driver(udev_dev);
> + if (!g_strcmp0(driver, INTEL_GFX_DRV_NAME)) {
> + found = TRUE;
> + udev_device_unref(udev_dev);
> + break;
> + }
> + udev_device_unref(udev_dev);
> + }
> + udev_enumerate_unref(udev_enum);
> + }
> + udev_unref(udev);
> +
> + return found;
> +}
> +
> +static gboolean msdk_vaapi_features_lookup(const gchar *dec_name)
I would use a simple char instead of all these gchar.. not strong about it.
> +{
> + GstRegistry *registry = NULL;
> + GstPluginFeature *msdkdec = NULL;
> + GstPluginFeature *vaapivpp = NULL;
> + gchar *msdk_dec_name = g_strconcat("msdk", dec_name,"dec", NULL);
not that long, a fixed buffer would fit here.
> +
> + registry = gst_registry_get();
> + if (!registry) {
> + return FALSE;
> + }
> + msdkdec = gst_registry_lookup_feature(registry, msdk_dec_name);
> + g_free(msdk_dec_name);
> + if (!msdkdec) {
> + return FALSE;
> + }
> + vaapivpp = gst_registry_lookup_feature(registry, "vaapipostproc");
> + if (!vaapivpp) {
> + gst_object_unref(msdkdec);
> + return FALSE;
> + }
> +
> + gst_object_unref(msdkdec);
> + gst_object_unref(vaapivpp);
> + return TRUE;
> +}
> +
> +static gboolean create_msdk_pipeline(SpiceGstDecoder *decoder)
> +{
> + GstElement *pipeline, *src, *sink, *parser, *msdk_decoder, *vpp;
> + GstCaps *src_caps, *sink_caps;
> + int codec_type = decoder->base.codec_type;
> + const gchar *dec_name = gst_opts[codec_type].name;
> + gchar *msdk_dec_name, *parser_name, *dec_caps;
> + 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 == NULL) {
In the same code you are using "!pointer" and "pointer == NULL", for
me both are fine but at least be consistent
> + spice_warning("error upon creation of 'appsrc' element");
> + return FALSE;
> + }
> + sink = gst_element_factory_make("appsink", NULL);
> + if (sink == NULL) {
> + spice_warning("error upon creation of 'appsink' element");
> + goto err_sink;
> + }
> +
> + 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 == NULL) {
> + spice_warning("error upon creation of 'parser' element");
> + goto err_parser;
> + }
> + }
> +
> + msdk_dec_name = g_strconcat("msdk", dec_name,"dec", NULL);
> + msdk_decoder = gst_element_factory_make(msdk_dec_name, NULL);
> + g_free(msdk_dec_name);
> + if (msdk_decoder == NULL) {
> + spice_warning("error upon creation of 'decoder' element");
> + goto err_decoder;
> + }
> + vpp = gst_element_factory_make("vaapipostproc", NULL);
> + if (vpp == NULL) {
> + spice_warning("error upon creation of 'vpp' element");
> + goto err_vpp;
> + }
> + g_object_set(vpp,
> + "format", GST_VIDEO_FORMAT_BGRx,
> + NULL);
> +
> + pipeline = gst_pipeline_new(NULL);
> + if (pipeline == NULL) {
> + spice_warning("error upon creation of 'pipeline' element");
> + goto err_pipeline;
> + }
> +
> + dec_caps = g_strconcat(gst_opts[codec_type].dec_caps,
> + ",stream-format=byte-stream", NULL);
> + src_caps = gst_caps_from_string(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);
> + g_free(dec_caps);
> + 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_pipeline;
> + }
> +
> + if (use_parser) {
> + gst_bin_add_many(GST_BIN(pipeline), src, parser, msdk_decoder,
> + vpp, sink, NULL);
> + if (!gst_element_link_many(src, parser, msdk_decoder, vpp,
> + sink, NULL)) {
> + spice_warning("error linking elements");
> + goto err_pipeline;
> + }
> + } else {
> + gst_bin_add_many(GST_BIN(pipeline), src, msdk_decoder,
> + vpp, sink, NULL);
> + if (!gst_element_link_many(src, msdk_decoder, vpp, sink, NULL)) {
> + spice_warning("error linking elements");
> + goto err_pipeline;
> + }
> + }
> + decoder->pipeline = pipeline;
> + return TRUE;
> +
> +err_pipeline:
I don't like all these labels and goto, can we have at maximum one
cleanup label?
> + gst_object_unref(vpp);
> +err_vpp:
> + gst_object_unref(msdk_decoder);
> +err_decoder:
> + if (use_parser) {
> + gst_object_unref(parser);
> + }
> +err_parser:
> + gst_object_unref(sink);
> +err_sink:
> + gst_object_unref(src);
> + return FALSE;
> +}
> +
> static gboolean create_pipeline(SpiceGstDecoder *decoder)
> {
> GstBus *bus;
> @@ -496,6 +683,13 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
> SpiceGstPlayFlags flags;
> GstCaps *caps;
>
> + if (detect_intel_gpu() &&
> + msdk_vaapi_features_lookup(gst_opts[decoder->base.codec_type].name)) {
> + if (create_msdk_pipeline(decoder)) {
> + goto end;
Why not a simple return here?
> + }
> + }
> +
> playbin = gst_element_factory_make("playbin", "playbin");
> if (playbin == NULL) {
> spice_warning("error upon creation of 'playbin' element");
> @@ -565,6 +759,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
> g_warn_if_fail(decoder->appsrc == NULL);
> decoder->pipeline = playbin;
>
> +end:
> if (decoder->appsink) {
> GstAppSinkCallbacks appsink_cbs = { NULL };
> appsink_cbs.new_sample = new_sample;
Frediano
More information about the Spice-devel
mailing list