[Spice-devel] [RFC PATCH spice-gtk] Fix overlay for vaapisink

[Snir Sheriber]

Hi,


On 11/22/18 4:50 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.
> Note that overlay message should be handled synchronously and
> not asynchronously so gst_bus_set_sync_handler is used.
> To avoid possible thread errors from X11 call XInitThreads before
> any X11 calls.
>
> 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.
>
> Linking directly to X11 or Gtk should be avoided and made in spice-gtk
> instead. This require a bit of refactory of the interaction between
> spice-gtk and spice-glib which after the direct rendering supports
> seems a bit broken.

Hmm, yes we still have this issue :p

>
> Currently we don't support Wayland. Tried to pass the wl_surface instead
> of the XID but got some weird message about X11 errors (weird because
> X11 should not be used at all! Maybe some gstreamer plugin is connected
> to X11 emulation in wayland? Need investigating, unsetting, also from
> gstreamer documentation is not clear what gstreamer is expecting
> for gst_video_overlay_set_window_handle on wayland).
>
> Note that this patch also add a dependency to libva. This could be
> removed using dlopen/dlsym.
>
> Performance are so better that you don't need to measure much!
> ---
>   src/Makefile.am           |  2 ++
>   src/channel-display-gst.c | 71 ++++++++++++++++++++++++++++++---------
>   2 files changed, 58 insertions(+), 15 deletions(-)
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 1bb6f9bf..c4cd19a7 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -196,6 +196,8 @@ libspice_client_glib_2_0_la_LIBADD =					\
>   	$(USBREDIR_LIBS)						\
>   	$(GUDEV_LIBS)							\
>   	$(PHODAV_LIBS)							\
> +	$(GTK_LIBS)							\
> +	-lva-x11 -lX11							\

:(


>   	$(NULL)
>   
>   if WITH_POLKIT
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 2c07f350..70539719 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -28,6 +28,9 @@
>   #include <gst/app/gstappsink.h>
>   #include <gst/video/gstvideometa.h>
>   
> +#include <gdk/gdkx.h>
> +#include <va/va_x11.h>
> +
>   
>   typedef struct SpiceGstFrame SpiceGstFrame;
>   
> @@ -299,6 +302,14 @@ static void free_pipeline(SpiceGstDecoder *decoder)
>       decoder->pipeline = NULL;
>   }
>   
> +// See https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/GstGLDisplay.html
> +// (look for XInitThreads). We call it into a constructor to make sure
> +// we call before any X11 function.

Aren't we use /*..*/ comment?


> +SPICE_CONSTRUCTOR_FUNC(i18n_init)
> +{
> +    XInitThreads();
> +}
> +
>   static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer video_decoder)
>   {
>       SpiceGstDecoder *decoder = video_decoder;
> @@ -334,6 +345,19 @@ static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer v
>           g_free(filename);
>           break;
>       }
> +    default:
> +        /* not being handled */
> +        break;
> +    }
> +    return TRUE;
> +}
> +
> +static GstBusSyncReply
> +handle_pipeline_message_sync(GstBus *bus, GstMessage *msg, gpointer video_decoder)
> +{
> +    SpiceGstDecoder *decoder = video_decoder;
> +
> +    switch (GST_MESSAGE_TYPE(msg)) {
>       case GST_MESSAGE_ELEMENT: {
>           if (gst_is_video_overlay_prepare_window_handle_message(msg)) {
>               GstVideoOverlay *overlay;
> @@ -348,11 +372,41 @@ static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer v
>           }
>           break;
>       }
> +    case GST_MESSAGE_NEED_CONTEXT:
> +    {
> +        const gchar *context_type;
> +
> +        gst_message_parse_context_type(msg, &context_type);
> +        spice_debug("got need context %s from %s\n", context_type, GST_MESSAGE_SRC_NAME(msg));
> +
> +        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;
> +
> +            if (!x11_display) {
> +                x11_display = gdk_x11_get_default_xdisplay();
> +                g_assert_nonnull(x11_display);
> +            }
> +            if (!va_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);
> +            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);
> +        }
> +        break;
> +    }
>       default:
>           /* not being handled */
>           break;
>       }
> -    return TRUE;
> +    return GST_BUS_PASS;
>   }
>   
>   #if GST_CHECK_VERSION(1,9,0)
> @@ -425,21 +479,7 @@ 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;
> -
> -        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);
> @@ -494,6 +534,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>       }
>       bus = gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline));
>       gst_bus_add_watch(bus, handle_pipeline_message, decoder);
> +    gst_bus_set_sync_handler(bus, handle_pipeline_message_sync, decoder, NULL);
So meanwhile i think we should have a patch that moves the overlay 
handling to the sync handler and
another one for XInitThread which i guess both of these are needed 
regardless the context handling.


>       gst_object_unref(bus);
>   
>       decoder->clock = gst_pipeline_get_clock(GST_PIPELINE(decoder->pipeline));


Works well! and seems to indeed fix this vaapi issue. I also want to 
test with some experimental code that should
allow to resize, I'll update if something will come up.

(BTW for some reason doesn't apply on upstream master)


Snir.