[Spice-devel] [PATCH spice-gtk v3] Fix overlay for vaapisink
Snir Sheriber
ssheribe at redhat.com
Mon Jan 14 12:54:06 UTC 2019
Hi,
On 1/14/19 11:48 AM, Frediano Ziglio 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);
>>> - }
>>> }
>>>
>>> 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: ?)
>>
> Added.
>
>>> +
>>> +#if defined(GDK_WINDOWING_X11) && defined(HAVE_LIBVA)
>>> + if (g_strcmp0(context_type, "gst.vaapi.app.Display") == 0) {
>>> + 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.
>>
> I had similar issues in the past but patch was quite different.
> Also I had Fedora 28, now I have Fedora 29, which OS are you running?
f29
(I guess it should not matter much but i'm using cinnamon not gnome)
> How often does it happen?
Very often, every 3-4 consecutive runs
> Does it happen doing something specific like screen resize?
No, when it happens it does not event get to the point of a clear image,
immediately freezes.
fwiw i disabled the resize related callbacks and it still happened.
>
>> 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.
>>
> None of these.
> Are you sure your machine is using VAAPI, did you dump the pipeline?
> window != NULL seems to suggest there's no overlay involved.
This specific warning i got only once, usually i get it without it, i do
have vaapi, and seems
it sets the vaapicontext correctly in replay to the context msg
Snir.
>
> Frediano
More information about the Spice-devel
mailing list