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

Submitted by Victor Toso on Feb. 15, 2018, 9:05 a.m.

Details

Message ID 20180215090505.30993-2-victortoso@redhat.com
State New
Headers show
Series "Using libva to detect hardware capabilities" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Feb. 15, 2018, 9:05 a.m.
From: Victor Toso <me@victortoso.com>

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()

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 configure.ac             |  20 +++++++
 src/Makefile.am          |  12 ++++
 src/spice-session-priv.h |   1 +
 src/spice-session.c      | 139 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 172 insertions(+)

Patch hide | download patch | download mbox

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])
+])
+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)						\
+	$(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;
+    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");
+        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");
+        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)
+            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);
+    }
+
+    g_array_unref(codecs);
+    g_free(profile_list);
+    vaTerminate(va_dpy);
+#endif
+}

Comments

Hey,

On Thu, Feb 15, 2018 at 10:05:04AM +0100, Victor Toso wrote:
> From: Victor Toso <me@victortoso.com>
> 
> 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. I assume it's not possible to directly query
GStreamer to know what it can hardware decode?

> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  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.

> +])
> +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.

> +	$(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.

> +    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?

> +        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.

> +        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.

> +            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.

Christophe

> +    }
> +
> +    g_array_unref(codecs);
> +    g_free(profile_list);
> +    vaTerminate(va_dpy);
> +#endif
> +}
> -- 
> 2.16.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
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@victortoso.com>
> > 
> > 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@redhat.com>
> > ---
> >  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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel