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

[Teng, Jin Chung]

Hi Vivek,

> 
> > >
> > > 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?
[Jin Chung] Please remove VP8, as msdk, vaapi and va no longer support VP8 encode. 
As for other codec option, please use the following:
1. msdkmjpegenc
2. vajpegenc
3. vaapijpegenc
4. msdkvp9enc async-depth=1 b-frames=0 rate-control=3 target-usage=7
5. vavp9lpenc min-qp=15 max-qp=35 rate-control=16 ref-frames=0 target-usage=7
6. vaapivp9enc tune=3 rate-control=1 
> 
> >
> > > +    } 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
[Jin Chung]