[Spice-devel] [PATCH 2/2] gstreamer-encoder: Use a h/w based encoder with Intel GPUs if possible (v2)

[Kasireddy, Vivek]

Hi Frediano,

> >
> > Once it is determined that an Intel GPU is available/active (after
> > looking into udev's database), we try to see if there is a h/w
> > based encoder (element) available (in Gstreamer's registry cache)
> > for the user selected video codec. In other words, if we find that
> > the Intel Media SDK Gstreamer plugin (libgstmsdk.so) and associated
> > libraries (such as va or vaapi) are all installed properly, we add
> > the appropriate h/w based encoder and post-processor/converter
> > elements to the pipeline (along with any relevant options) instead
> > of the s/w based elements.
> >
> > For example, if the user selects h264 as the preferred codec format,
> > msdkh264enc and vapostproc will be preferred instead of x264enc
> > and videoconvert.
> >
> > v2: (addressed some review comments from Frediano)
> > - Moved the udev helper into spice-common
> > - Refactored the code to choose plugins in order msdk > va > vaapi
> >
> > 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>
> > Co-developed-by: Jin Chung Teng <jin.chung.teng at intel.com>
> > Co-developed-by: Hazwan Arif Mazlan <hazwan.arif.mazlan at intel.com>
> > ---
> >  server/gstreamer-encoder.c | 96
> ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 93 insertions(+), 3 deletions(-)
> >
> > diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> > index 057509b5..44666f42 100644
> > --- a/server/gstreamer-encoder.c
> > +++ b/server/gstreamer-encoder.c
> > @@ -31,6 +31,7 @@
> >  #include "red-common.h"
> >  #include "video-encoder.h"
> >  #include "utils.h"
> > +#include "common/udev.h"
> >
> >
> >  #define SPICE_GST_DEFAULT_FPS 30
> > @@ -913,14 +914,94 @@ static const gchar* get_gst_codec_name(const
> SpiceGstEncoder *encoder)
> >      }
> >  }
> >
> > +static const char video_codecs[][8] = {
> > +    { "" },
> > +    { "mjpeg" },
> > +    { "vp8" },
> > +    { "h264" },
> > +    { "vp9" },
> > +    { "h265" },
> > +};
> > +
> > +static bool gst_features_lookup(const gchar *feature_name)
> > +{
> > +    GstRegistry *registry;
> > +    GstPluginFeature *feature;
> > +
> > +    registry = gst_registry_get();
> > +    if (!registry) {
> > +        return false;
> > +    }
> > +
> > +    feature = gst_registry_lookup_feature(registry, feature_name);
> > +    if (!feature) {
> > +        return false;
> > +    }
> > +
> > +    gst_object_unref(feature);
> > +    return true;
> > +}
> > +
> > +static gchar *find_best_plugin(const gchar *codec_name)
> > +{
> > +    const char *plugins[3] = {"msdk", "va", "vaapi"};
> > +    gchar *feature_name;
> > +    int i;
> > +
> > +    for (i = 0; i < 3; i++) {
> > +        feature_name = !codec_name ? g_strconcat(plugins[i], "postproc",
> NULL) :
> 
> The name for the msdk postproc is msdkvpp, not msdkpostproc.
Right, the VPP element associated with msdk is indeed msdkvpp but we do not
intend to use msdkvpp currently as it has bugs that prevent the pipeline from
working correctly. And, it is not clear when these bugs are going to be fixed.
That's why we prefer to use either vapostproc or vaapipostproc for now.

> Looking at this function and the next one it looks correct to use,
> let's say an encoder from msdk (like msdkvp9enc) and the vaapi post
> processor (like vaapipostproc), but it seems wrong to me.
> 
> > +                       g_strconcat(plugins[i], codec_name, "enc", NULL);
> > +        if (!gst_features_lookup(feature_name)) {
> > +            g_free(feature_name);
> > +            feature_name = NULL;
> > +            continue;
> > +        }
> > +        break;
> > +    }
> > +    return feature_name;
> > +}
> > +
> > +static void try_intel_hw_plugins(const gchar *codec_name, gchar
> **converter,
> > +                                 gchar **gstenc_name, gchar **gstenc_opts)
> > +{
> > +    gchar *encoder = find_best_plugin(codec_name);
> > +    if (!encoder) {
> > +        return;
> > +    }
> > +    gchar *vpp = find_best_plugin(NULL);
> > +    if (!vpp) {
> > +        return;
> > +    }
> > +
> > +    g_free(*converter);
> > +    g_free(*gstenc_name);
> > +    g_free(*gstenc_opts);
> > +
> > +    *gstenc_name = encoder;
> > +    if (strstr(encoder, "msdk")) {
> > +        *gstenc_opts = g_strdup("async-depth=1 rate-control=3 gop-size=1
> tune=16 b-frames=0 target-usage=7 min-qp=15 max-qp=35");
> 
> These options are nice for h264 (and probably h265) but are wrong for
> vp9 and probably mjpeg.
You are right; these options may not be correct if we use VP8 or VP9 as the codec.
@Jin Chung, what are the correct options to use in this case?

> 
> > +    } else if (strstr(encoder, "vaapi")) {
> > +        *gstenc_opts = g_strdup("rate-control=cqp max-bframes=0 min-
> qp=15 max-qp=35");
> > +    } else {
> > +        *gstenc_opts = g_strdup("rate-control=16 b-frames=0 target-usage=7
> min-qp=15 max-qp=35");
> > +    }
> 
> Similar comment for these.
> 
> > +
> > +    if (strstr(vpp, "vaapi")) {
> > +        *converter = g_strconcat(vpp, " ! video/x-
> raw(memory:VASurface),format=NV12", NULL);
> > +    } else {
> > +        *converter = g_strconcat(vpp, " ! video/x-
> raw(memory:VAMemory),format=NV12", NULL);
> > +    }
> 
> From gst-inspect msdkvpp does not support any of these. Is this expected?
As explained above, we do not want to use msdkvpp at this time.

Thanks,
Vivek

> 
> > +    g_free(vpp);
> > +}
> > +
> >  static gboolean create_pipeline(SpiceGstEncoder *encoder)
> >  {
> >  #ifdef HAVE_GSTREAMER_0_10
> > -    const gchar *converter = "ffmpegcolorspace";
> > +    gchar *converter = g_strdup("ffmpegcolorspace");
> >  #else
> > -    const gchar *converter = "videoconvert ! video/x-raw,format=NV12";
> > +    gchar *converter = g_strdup("videoconvert ! video/x-
> raw,format=NV12");
> >  #endif
> > -    const gchar* gstenc_name = get_gst_codec_name(encoder);
> > +    gchar* gstenc_name = g_strdup(get_gst_codec_name(encoder));
> >      if (!gstenc_name) {
> >          return FALSE;
> >      }
> > @@ -973,12 +1054,21 @@ static gboolean
> create_pipeline(SpiceGstEncoder *encoder)
> >          return FALSE;
> >      }
> >
> > +    const char *codec_name = video_codecs[encoder->base.codec_type];
> > +    GpuVendor vendor = spice_udev_detect_gpu();
> > +    if (vendor == GPU_VENDOR_INTEL) {
> > +        try_intel_hw_plugins(codec_name, &converter, &gstenc_name,
> > +                             &gstenc_opts);
> > +    }
> > +
> >      GError *err = NULL;
> >      gchar *desc = g_strdup_printf("appsrc is-live=true format=time do-
> timestamp=true name=src !"
> >                                    " %s ! %s name=encoder %s ! appsink name=sink",
> >                                    converter, gstenc_name, gstenc_opts);
> >      spice_debug("GStreamer pipeline: %s", desc);
> >      encoder->pipeline = gst_parse_launch_full(desc, NULL,
> GST_PARSE_FLAG_FATAL_ERRORS, &err);
> > +    g_free(converter);
> > +    g_free(gstenc_name);
> >      g_free(gstenc_opts);
> >      g_free(desc);
> >      if (!encoder->pipeline || err) {
> 
> Frediano