Re: [spice-gtk v1 1/2] Use libva to check video hardware accel capabilities

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Mon, Feb 19, 2018 at 02:44:04PM +0100, Christophe Fergeau wrote:
> Hey,
> 
> On Thu, Feb 15, 2018 at 10:05:04AM +0100, Victor Toso wrote:
> > From: Victor Toso <me@xxxxxxxxxxxxxx>
> > 
> > Libva is an implementation for VA-API.
> > 
> > This can be used to automatically send to the server the
> > preferred video codecs as the client would prefer streams
> > with video codecs that can be decoded with gpu support.
> > 
> > We can also use the profiles to detect and set upper limit for
> > video streams quality.
> > e.g: Don't start UHD video stream if client's hardware don't
> > support it.
> > 
> > This patch makes usage of libva in spice-session and exposes this
> > information to all available channel-displays with the internal
> > function spice_session_get_hw_accel_video_codecs()
> 
> This assumes that HW accelerated video decoding is going to go
> through libva when using GStreamer.

Not really. I think it is fairly possible to query hw
capabilities with libva while doing hardware decoding without
libva (nvdec element for NVidia, for instance) but I haven't
tested that.

> I assume it's not possible to directly query GStreamer to know
> what it can hardware decode?

Not really, although I have asked [0] - We still need some
support from GStreamer to detect if we are doing hardware or
software decoding (somehow!) without having to check elements as
this does not scale well, I think.

[0] https://bugzilla.gnome.org/show_bug.cgi?id=782741

Note also that libva here in spice-gtk could tell that we do have
support to vp8/vp9/h264 hw decoding but in fact there is no
element installed in GStreamer to do that.

I plan to send a v2 including the GStreamer check to, at least,
be sure that we are sending the preferred video codecs based on
hw capability + gstreamer support + the fixes from this review
soon :)

> 
> > 
> > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> > ---
> >  configure.ac             |  20 +++++++
> >  src/Makefile.am          |  12 ++++
> >  src/spice-session-priv.h |   1 +
> >  src/spice-session.c      | 139 +++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 172 insertions(+)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 2a14055..0b0db0f 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -321,6 +321,25 @@ AC_SUBST(Z_LIBS)
> >  SPICE_CHECK_SMARTCARD
> >  AM_CONDITIONAL([WITH_SMARTCARD], [test "x$have_smartcard" = "xyes"])
> >  
> > +AC_ARG_ENABLE([libva],
> > +  AS_HELP_STRING([--enable-libva=@<:@auto/yes/no@:>@], [Enable auto detection of hardware accelerate video decoding support @<:@default=auto@:>@]),
> > +  [],
> > +  [enable_libva="auto"])
> > +AS_IF([test "x$enable_libva" != "xno"],
> > +      [PKG_CHECK_MODULES(LIBVA, [libva >= 1.0.0],
> > +         [AC_DEFINE([HAVE_LIBVA], 1, [Have libva support?])
> > +          enable_libva="yes"],
> > +         [AS_IF([test "x$enable_libva" = "xyes"],
> > +                AC_MSG_ERROR([Auto detection of hardware accelerated video decoding explicitly requested, but some required packages are not available]))
> > +          enable_libva="no"
> > +      ])
> > +    PKG_CHECK_MODULES([LIBVA_X11], [libva-x11 >= 1.0.0])
> > +    PKG_CHECK_MODULES([LIBVA_WAYLAND], [libva-wayland >= 1.0.0])
> > +    PKG_CHECK_MODULES([GDK_X11], [gdk-x11-3.0])
> > +    PKG_CHECK_MODULES([GDK_WAYLAND], [gdk-wayland-3.0])
> 
> I don't think we'll necessarily have all of these installed?
> PKG_CHECK_MODULES will error out if any of these is missing.

I'll test and remove them, thanks!

> 
> > +])
> > +AM_CONDITIONAL([HAVE_LIBVA], [test "x$enable_libva" = "xyes"])
> > +
> >  AC_ARG_ENABLE([usbredir],
> >    AS_HELP_STRING([--enable-usbredir=@<:@auto/yes/no@:>@],
> >                   [Enable usbredir support @<:@default=auto@:>@]),
> > @@ -635,6 +654,7 @@ AC_MSG_NOTICE([
> >          DBus:                     ${have_dbus}
> >          WebDAV support:           ${have_phodav}
> >          LZ4 support:              ${have_lz4}
> > +        Libva support:            ${enable_libva}
> >  
> >          Now type 'make' to build $PACKAGE
> >  
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 4b6e46d..7b74220 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -75,6 +75,9 @@ SPICE_COMMON_CPPFLAGS =						\
> >  	$(PIXMAN_CFLAGS)					\
> >  	$(PULSE_CFLAGS)						\
> >  	$(GTK_CFLAGS)						\
> > +	$(GDK_CFLAGS)						\
> > +	$(GDK_X11_CFLAGS)					\
> > +	$(GDK_WAYLAND_CFLAGS)					\
> >  	$(CAIRO_CFLAGS)						\
> >  	$(GLIB2_CFLAGS)						\
> >  	$(GIO_CFLAGS)						\
> > @@ -88,6 +91,9 @@ SPICE_COMMON_CPPFLAGS =						\
> >  	$(GUDEV_CFLAGS)						\
> >  	$(SOUP_CFLAGS)						\
> >  	$(PHODAV_CFLAGS)					\
> > +	$(LIBVA_CFLAGS)						\
> > +	$(LIBVA_X11_CFLAGS)					\
> > +	$(LIBVA_WAYLAND_CFLAGS)					\
> >  	$(X11_CFLAGS)					\
> >  	$(LZ4_CFLAGS)					\
> >  	$(NULL)
> > @@ -195,6 +201,12 @@ libspice_client_glib_2_0_la_LIBADD =					\
> >  	$(USBREDIR_LIBS)						\
> >  	$(GUDEV_LIBS)							\
> >  	$(PHODAV_LIBS)							\
> > +	$(GDK_LIBS)							\
> > +	$(GDK_X11_LIBS)							\
> > +	$(GDK_WAYLAND_LIBS)						\
> 
> Adding GDK_* to libspice_client_glib_2_0_la_LIBADD does not look good.
> Maybe you want to move this to SpiceGtkSession rather than SpiceSession.

True, I'll do.

> 
> > +	$(LIBVA_LIBS)							\
> > +	$(LIBVA_X11_LIBS)						\
> > +	$(LIBVA_WAYLAND_LIBS)						\
> >  	$(NULL)
> >  
> >  if WITH_POLKIT
> > diff --git a/src/spice-session-priv.h b/src/spice-session-priv.h
> > index 03005aa..7137cf6 100644
> > --- a/src/spice-session-priv.h
> > +++ b/src/spice-session-priv.h
> > @@ -100,6 +100,7 @@ void spice_session_set_main_channel(SpiceSession *session, SpiceChannel *channel
> >  gboolean spice_session_set_migration_session(SpiceSession *session, SpiceSession *mig_session);
> >  SpiceAudio *spice_audio_get(SpiceSession *session, GMainContext *context);
> >  const gchar* spice_audio_data_mode_to_string(gint mode);
> > +const GArray *spice_session_get_hw_accel_video_codecs(SpiceSession *session);
> >  G_END_DECLS
> >  
> >  #endif /* __SPICE_CLIENT_SESSION_PRIV_H__ */
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index 2aabf58..e26d375 100644
> > --- a/src/spice-session.c
> > +++ b/src/spice-session.c
> > @@ -23,6 +23,19 @@
> >  #include <gio/gunixsocketaddress.h>
> >  #endif
> >  #include "common/ring.h"
> > +#ifdef HAVE_LIBVA
> > +#include <gdk/gdk.h>
> > +#include <va/va.h>
> > +#include <va/va_str.h>
> > +#ifdef GDK_WINDOWING_WAYLAND
> > +#include <gdk/gdkwayland.h>
> > +#include <va/va_wayland.h>
> > +#endif
> > +#ifdef GDK_WINDOWING_X11
> > +#include <gdk/gdkx.h>
> > +#include <va/va_x11.h>
> > +#endif
> > +#endif
> >  
> >  #include "spice-client.h"
> >  #include "spice-common.h"
> > @@ -33,6 +46,7 @@
> >  #include "spice-uri-priv.h"
> >  #include "channel-playback-priv.h"
> >  #include "spice-audio-priv.h"
> > +#include "channel-display-priv.h"
> >  
> >  struct channel {
> >      SpiceChannel      *channel;
> > @@ -116,6 +130,9 @@ struct _SpiceSessionPrivate {
> >      guint8            uuid[16];
> >      gchar             *name;
> >      SpiceImageCompression preferred_compression;
> > + 
> > +    /* Array of SpiceVideoCodecType with hw accelerated video decoding capability */
> > +    GArray            *video_codecs;
> >  
> >      /* associated objects */
> >      SpiceAudio        *audio_manager;
> > @@ -248,6 +265,7 @@ spice_image_compress_get_type (void)
> >  static guint signals[SPICE_SESSION_LAST_SIGNAL];
> >  
> >  static void spice_session_channel_destroy(SpiceSession *session, SpiceChannel *channel);
> > +static void spice_session_check_video_hw_caps(SpiceSession *session);
> >  
> >  static void update_proxy(SpiceSession *self, const gchar *str)
> >  {
> > @@ -299,6 +317,9 @@ static void spice_session_init(SpiceSession *session)
> >          SPICE_DEBUG("Could not initialize SpiceUsbDeviceManager - %s", err->message);
> >          g_clear_error(&err);
> >      }
> > +
> > +    session->priv->video_codecs = NULL;
> 
> Here, session->priv->video_codecs should already be NULL.

Indeed

> 
> > +    spice_session_check_video_hw_caps(session);
> >  }
> >  
> >  static void
> > @@ -2801,3 +2822,121 @@ gboolean spice_session_set_migration_session(SpiceSession *session, SpiceSession
> >  
> >      return TRUE;
> >  }
> > +
> > +G_GNUC_INTERNAL
> > +const GArray *spice_session_get_hw_accel_video_codecs(SpiceSession *session)
> > +{
> > +    g_return_val_if_fail(SPICE_IS_SESSION(session), NULL);
> > +    return session->priv->video_codecs;
> > +}
> > +
> > +static void
> > +spice_session_check_video_hw_caps(SpiceSession *session)
> > +{
> > +#ifdef HAVE_LIBVA
> > +    VADisplay va_dpy = NULL;
> > +    VAStatus va_status;
> > +    GdkDisplay *display;
> > +    int major_version, minor_version;
> > +    GArray *codecs;
> > +    const gchar *last_profile = NULL;
> > +    VAProfile *profile_list = NULL;
> > +    int num_profiles, max_num_profiles, i;
> > +    int num_entrypoint;
> > +
> > +    display = gdk_display_get_default();
> > +    spice_debug("Display: %s", gdk_display_get_name(display));
> > +
> > +#ifdef GDK_WINDOWING_X11
> > +    if (GDK_IS_X11_DISPLAY(display))
> > +        va_dpy = vaGetDisplay(gdk_x11_display_get_xdisplay(display));
> > +#endif
> > +#ifdef GDK_WINDOWING_WAYLAND
> > +    if (GDK_IS_WAYLAND_DISPLAY(display))
> > +        va_dpy = vaGetDisplayWl(gdk_wayland_display_get_wl_display(display));
> > +#endif
> > +
> > +    if (va_dpy == NULL) {
> > +        spice_warning("Failed to get VADisplay, unable to detect hardware capabilities");
> 
> g_warning? but maybe g_debug() is enough?

Okay

> 
> > +        return;
> > +    }
> > +
> > +    va_status = vaInitialize(va_dpy, &major_version, &minor_version);
> > +    if (va_status != VA_STATUS_SUCCESS) {
> > +        spice_warning("Failed to initialize libva");
> > +        return;
> > +    }
> > +
> > +    max_num_profiles = vaMaxNumProfiles(va_dpy);
> > +    profile_list = g_new(VAProfile, max_num_profiles);
> > +
> > +    if (!profile_list) {
> > +        spice_warning("libva: failed to allocate memory for profile list");
> 
> g_new will never return NULL.

But if max_num_profiles is 0, profile_list shall be NULL but
yeah, in this case the warning is misleading. I'll move the
check. Thanks!

> > +        vaTerminate(va_dpy);
> > +        return;
> > +    }
> > +
> > +    va_status = vaQueryConfigProfiles(va_dpy, profile_list, &num_profiles);
> > +    if (va_status != VA_STATUS_SUCCESS) {
> > +        spice_warning("libva: failed to query profiles");
> > +        g_free(profile_list);
> > +        vaTerminate(va_dpy);
> > +        return;
> > +    }
> > +
> > +    codecs = g_array_new(FALSE, FALSE, sizeof(gint));
> > +    for (i = 0; i < num_profiles; i++) {
> > +        int j;
> > +        VAEntrypoint entrypoints[50];
> > +        VAProfile profile = profile_list[i];
> > +        const char *profile_str = vaProfileStr(profile);
> > +
> > +        /* Spice protocol does not support different profiles for a given codec
> > +         * at the moment, which means that we can jump to the next codec. */
> > +        if (last_profile != NULL &&
> > +                g_ascii_strncasecmp(profile_str + strlen("VAProfile"),
> > +                                    last_profile,
> > +                                    strlen(last_profile)) == 0)
> 
> This is repeated twice, might deserve a small helper for improved
> readability if you can come up with a good name.

I'll try

> 
> > +            continue;
> > +
> > +        va_status = vaQueryConfigEntrypoints(va_dpy, profile, entrypoints, &num_entrypoint);
> > +        if (va_status == VA_STATUS_ERROR_UNSUPPORTED_PROFILE)
> > +            continue;
> > +        else if (va_status != VA_STATUS_SUCCESS) {
> > +            spice_warning("Error on vaQueryConfigEntrypoints()");
> > +            break;
> > +        }
> > +
> > +        /* Find if current profile has decoding support */
> > +        for (j = 0; j < num_entrypoint; j++) {
> > +            int k;
> > +
> > +            if (entrypoints[j] != VAEntrypointVLD)
> > +                continue;
> > +
> > +            /* Found decoding entrypoing, check if it is supported by Spice protocol */
> > +            for (k = 1; k < SPICE_VIDEO_CODEC_TYPE_ENUM_END; k++) {
> > +                if (g_ascii_strncasecmp(profile_str + strlen("VAProfile"),
> > +                                        gst_opts[k].name,
> > +                                        strlen(gst_opts[k].name)) == 0) {
> > +                    last_profile = gst_opts[k].name;
> > +                    g_array_append_val(codecs, k);
> > +                    spice_debug("Support to decode %s found with profile %s",
> > +                                gst_opts[k].name, profile_str);
> > +                    break;
> > +                }
> > +            }
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (codecs->len > 0) {
> > +        g_clear_pointer(&session->priv->video_codecs, g_array_unref);
> > +        session->priv->video_codecs = g_array_ref(codecs);
> 
> This also needs to be _unref'ed in _dispose or _finalize.

True, I forgot about it. Thanks again!

Cheers,
        toso
> 
> Christophe
> 
> > +    }
> > +
> > +    g_array_unref(codecs);
> > +    g_free(profile_list);
> > +    vaTerminate(va_dpy);
> > +#endif
> > +}
> > -- 
> > 2.16.1
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel


Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]