[Spice-devel] [PATCH spice-gtk v3] Fix overlay for vaapisink
Snir Sheriber
ssheribe at redhat.com
Sun Jan 13 14:37:07 UTC 2019
More comments
On 1/13/19 3:11 PM, Snir Sheriber wrote:
> Hi,
>
>
> On 1/11/19 6:36 PM, Frediano Ziglio wrote:
>> The vaapisink plugin to support overlay requires the application
>> to provide the proper context. If you don't do so the plugin will
>> cause a crash of the application.
>> To avoid possible thread errors from X11 create a new Display
>> connection to pass to vaapisink.
>>
>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>> ---
>> To test it probably you'll have to remove the gstreamer registry,
>> usually in ~/.cache/gstreamer-1.0/registry.x86_64.bin.
>>
>> Changes since v2:
>> - remove hard dependency to libva-x11.
>>
>> Changes since v1:
>> - updated to master;
>> - use a different X11 connection as discussed in a previous thread;
>> - remove some comments, now obsoleted;
>> - fixed direct X11 link, now code is in spice-widget (as it should be);
>> - better libva linking, using now build systems.
>> ---
>> configure.ac | 5 +++++
>> meson.build | 5 +++++
>> src/Makefile.am | 2 ++
>> src/channel-display-gst.c | 14 -------------
>> src/spice-widget.c | 44 +++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 56 insertions(+), 14 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 8c5c4d38..ead6c1bb 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -175,6 +175,11 @@ PKG_CHECK_MODULES(GTHREAD, gthread-2.0)
>> PKG_CHECK_MODULES(JSON, json-glib-1.0)
>> +PKG_CHECK_EXISTS([libva-x11], [
>> + PKG_CHECK_MODULES(LIBVA, libva-x11)
>> + AC_DEFINE([HAVE_LIBVA], [1], [Define if libva is available])
>> +])
>> +
>> AC_ARG_ENABLE([webdav],
>> AS_HELP_STRING([--enable-webdav=@<:@auto/yes/no@:>@],
>> [Enable webdav support @<:@default=auto@:>@]),
>> diff --git a/meson.build b/meson.build
>> index 37377680..6e96cca8 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -134,6 +134,11 @@ if d.found()
>> if host_machine.system() != 'windows'
>> spice_gtk_deps += dependency('epoxy')
>> spice_gtk_deps += dependency('x11')
>> + d = dependency('libva-x11', required: false)
>> + if d.found()
>> + spice_gtk_deps += d
>> + spice_gtk_config_data.set('HAVE_LIBVA', '1')
>> + endif
>> endif
>> spice_gtk_has_gtk = true
>> endif
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index b876530c..784b18d0 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -89,6 +89,7 @@ SPICE_COMMON_CPPFLAGS = \
>> $(GUDEV_CFLAGS) \
>> $(SOUP_CFLAGS) \
>> $(PHODAV_CFLAGS) \
>> + $(LIBVA_CFLAGS) \
>> $(X11_CFLAGS) \
>> $(LZ4_CFLAGS) \
>> $(NULL)
>> @@ -112,6 +113,7 @@ SPICE_GTK_LIBADD_COMMON = \
>> $(CAIRO_LIBS) \
>> $(X11_LIBS) \
>> $(LIBM) \
>> + $(LIBVA_LIBS) \
>> $(NULL)
>> SPICE_GTK_SOURCES_COMMON = \
>> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
>> index 2f556fe4..01ee83cb 100644
>> --- a/src/channel-display-gst.c
>> +++ b/src/channel-display-gst.c
>> @@ -406,22 +406,8 @@ static gboolean create_pipeline(SpiceGstDecoder
>> *decoder)
>> } else {
>> /* handle has received, it means playbin will render
>> directly into
>> * widget using the gstvideooverlay interface instead of
>> app-sink.
>> - * Also avoid using vaapisink if exist since vaapisink could be
>> - * buggy when it is combined with playbin. changing its rank to
>> - * none will make playbin to avoid of using it.
>> */
>> - GstRegistry *registry = NULL;
>> - GstPluginFeature *vaapisink = NULL;
>> -
>> SPICE_DEBUG("Video is presented using gstreamer's
>> GstVideoOverlay interface");
>> - registry = gst_registry_get();
>> - if (registry) {
>> - vaapisink = gst_registry_lookup_feature(registry,
>> "vaapisink");
>> - }
>> - if (vaapisink) {
>> - gst_plugin_feature_set_rank(vaapisink, GST_RANK_NONE);
>> - gst_object_unref(vaapisink);
>> - }
>> }
Shouldn't we keep avoid this context can't be handled correctly (i mean
if version is too old, if i'm not mistaken lower than 1.13.90)
>> g_signal_connect(playbin, "source-setup",
>> G_CALLBACK(app_source_setup), decoder);
>> diff --git a/src/spice-widget.c b/src/spice-widget.c
>> index af0301c1..85707704 100644
>> --- a/src/spice-widget.c
>> +++ b/src/spice-widget.c
>> @@ -28,6 +28,9 @@
>> #ifdef GDK_WINDOWING_X11
>> #include <X11/Xlib.h>
>> #include <gdk/gdkx.h>
>> +#ifdef HAVE_LIBVA
>> +#include <va/va_x11.h>
>> +#endif
>> #endif
>> #ifdef G_OS_WIN32
>> #include <windows.h>
>> @@ -2592,6 +2595,47 @@ static void gst_sync_bus_call(GstBus *bus,
>> GstMessage *msg, SpiceDisplay *displa
>> }
>> break;
>> }
>> + case GST_MESSAGE_NEED_CONTEXT:
>> + {
>> + const gchar *context_type;
>> +
>> + gst_message_parse_context_type(msg, &context_type);
>> + g_debug("got need context %s from %s", context_type,
>> GST_MESSAGE_SRC_NAME(msg));
>
>
> Maybe worth mentioning it's gstreamer related (Gstreamer: ?)
>
>> +
>> +#if defined(GDK_WINDOWING_X11) && defined(HAVE_LIBVA)
>> + if (g_strcmp0(context_type, "gst.vaapi.app.Display") == 0) {
Maybe would be nice to implement in a create_context function so later
we can have something like:
create_vaapi_context()
create_gl_context()
...
>> + static Display *x11_display = NULL;
>> + static VADisplay va_display = NULL;
>> +
>> + // note that if VAAPI do not get the context for the
>> + // overlay it crashes the entire program!
>> + GdkDisplay *display = gdk_display_get_default();
>> + g_assert_nonnull(display);
>> +
>> + // Compute display pointers
>> + if (!x11_display && GDK_IS_X11_DISPLAY(display)) {
>> + x11_display = gdk_x11_display_get_xdisplay(display);
>> + // for thread problems we need a different Display,
>> + // VAAPI access the Display from another thread
>> + x11_display =
>> XOpenDisplay(XDisplayString(x11_display));
>> + g_assert_nonnull(x11_display);
>> + va_display = vaGetDisplay(x11_display);
>> + g_assert_nonnull(va_display);
>> + }
>> +
>> + GstContext *context =
>> gst_context_new("gst.vaapi.app.Display", FALSE);
>> + GstStructure *structure =
>> gst_context_writable_structure(context);
>> + if (x11_display) {
>> + gst_structure_set(structure, "x11-display",
>> G_TYPE_POINTER, x11_display, NULL);
>> + }
>> + gst_structure_set(structure, "va-display",
>> G_TYPE_POINTER, va_display, NULL);
>> +
>> + gst_element_set_context(GST_ELEMENT(msg->src), context);
>> + gst_context_unref(context);
>> + }
>> +#endif
>> + break;
>> + }
>> default:
>> /* not being handled */
>> break;
>
>
> It seems there is some kind of a race, every few runs it gets frozen
> and then crashes, i looked a bit,
>
> could not found a clear cause.
>
>
> One time i got these errors but may not be always related since it
> usually crashes without any hints
>
>
> ** (remote-viewer:19435): CRITICAL **: 11:44:18.845:
> gst_vaapi_window_reconfigure: assertion 'window != NULL' failed
>
> ** (remote-viewer:19435): CRITICAL **: 11:44:18.845:
> gst_vaapi_window_get_size: assertion 'window != NULL' failed
>
>
> Snir.
>
More information about the Spice-devel
mailing list