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