[Spice-devel] [RFC spice-gtk 1/1] Gstreamer: Use GstVideoOverlay if possible
Frediano Ziglio
fziglio at redhat.com
Mon Apr 9 10:52:17 UTC 2018
>
> Hi,
>
>
> On 04/06/2018 04:12 PM, Frediano Ziglio wrote:
> >> Currently when gstreamer is used to decode a full-screen
> >> stream sent from the server, the decoding frames are being
> >> forced to RBGA format and pushed using appsink to be scaled
> >> and rendered to screen.
> >>
> >> Today most of the gstreamer sinks supports the GstVideoOverlay
> >> interface which allows to render directly from the pipeline to
> >> a window by a given windowing system id (i.e. xid). This patch
> >> makes playbin to use this feature if possible.
> > Got some time to test, works correctly. Finally I manage to test wayland
> > and different resolutions/settings.
> >
> > I have some just minor changes (attached).
> >
> > Another change to use the new flag would be (didn't still test it):
> >
> >
> > diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> > index 6a90a78..c44f94e 100644
> > --- a/src/channel-display-priv.h
> > +++ b/src/channel-display-priv.h
> > @@ -98,6 +98,7 @@ gboolean gstvideo_has_codec(int codec_type);
> > typedef struct display_surface {
> > guint32 surface_id;
> > bool primary;
> > + bool streaming_mode;
> > enum SpiceSurfaceFmt format;
> > int width, height, stride, size;
> > uint8_t *data;
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index a58119d..7d639b5 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -1292,9 +1292,7 @@ static display_stream
> > *display_stream_create(SpiceChannel *channel,
> > #endif
> > default:
> > #ifdef HAVE_GSTVIDEO
> > - if (c->primary->width == (dest->right - dest->left) &&
> > - c->primary->height == (dest->bottom - dest->top)) {
> > - SPICE_DEBUG("It seems to be a FULL-SCREEN stream");
> > + if (c->primary->streaming_mode) {
> > /* stream area location is sent but currently not used */
> > g_coroutine_signal_emit(channel,
> > signals[SPICE_DISPLAY_STREAM_AREA], 0,
> > dest->left, dest->top,
> > @@ -1829,9 +1827,10 @@ static void
> > display_handle_surface_create(SpiceChannel *channel, SpiceMsgIn *in)
> > surface->height = create->height;
> > surface->stride = create->width * 4;
> > surface->size = surface->height * surface->stride;
> > + surface->streaming_mode = !!(create->flags &
> > SPICE_SURFACE_FLAGS_STREAMING_MODE);
> >
> > if (create->flags & SPICE_SURFACE_FLAGS_PRIMARY) {
> > - SPICE_DEBUG("primary flags: %x", create->flags);
> > + SPICE_DEBUG("surface flags: %x", create->flags);
> > surface->primary = true;
> > create_canvas(channel, surface);
> > if (c->mark_false_event_id != 0) {
> >
>
> Thanks!
>
> I have also made a similar change that uses the flag, seems to work fine.
> And then i noticed that if we use the overlay interface we can also avoid
> of allocating the canvas, but then if there is no canvas it's a bit more
> complicated to fallback in case the overlay interface cannot be applied.
> I hope I'll be able to make it work without too many changes, if not I'll
> leave it that way.
>
> Snir.
>
Tested too now, works fine.
I think the canvas optimization would be better as a follow up, beside
using the flag there's not much in common.
> >
> >> ---
> >> src/channel-display-gst.c | 71
> >> +++++++++++++++++++++++++++++++++++++----------
> >> src/channel-display.c | 54 +++++++++++++++++++++++++++++++++++
> >> src/spice-widget.c | 46 ++++++++++++++++++++++++++++++
> >> 3 files changed, 156 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> >> index c6280d3..6fa19e0 100644
> >> --- a/src/channel-display-gst.c
> >> +++ b/src/channel-display-gst.c
> >> @@ -265,7 +265,8 @@ static void free_pipeline(SpiceGstDecoder *decoder)
> >>
> >> gst_element_set_state(decoder->pipeline, GST_STATE_NULL);
> >> gst_object_unref(decoder->appsrc);
> >> - gst_object_unref(decoder->appsink);
> >> + if (decoder->appsink)
> >> + gst_object_unref(decoder->appsink);
> >> gst_object_unref(decoder->pipeline);
> >> gst_object_unref(decoder->clock);
> >> decoder->pipeline = NULL;
> >> @@ -308,7 +309,18 @@ static gboolean handle_pipeline_message(GstBus *bus,
> >> GstMessage *msg, gpointer v
> >> break;
> >> }
> >> default:
> >> - /* not being handled */
> >> + if (gst_is_video_overlay_prepare_window_handle_message(msg)) {
> >> + GstVideoOverlay *overlay;
> >> + guintptr handle = 0;
> >> +
> >> + g_object_get(decoder->base.stream->channel, "handle",
> >> &handle,
> >> NULL);
> >> + SPICE_DEBUG("prepare-window-handle msg received (handle:
> >> %lu)",
> >> handle);
> >> + if (handle != 0) {
> >> + overlay = GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg));
> >> + gst_video_overlay_set_window_handle(overlay, handle);
> >> + }
> >> + }
> >> + /* else- not being handled */
> >> break;
> >> }
> >> return TRUE;
> >> @@ -348,6 +360,7 @@ static gboolean create_pipeline(SpiceGstDecoder
> >> *decoder)
> >> #if GST_CHECK_VERSION(1,9,0)
> >> GstElement *playbin, *sink;
> >> SpiceGstPlayFlags flags;
> >> + guintptr handle;
> >> GstCaps *caps;
> >>
> >> playbin = gst_element_factory_make("playbin", "playbin");
> >> @@ -356,26 +369,53 @@ static gboolean create_pipeline(SpiceGstDecoder
> >> *decoder)
> >> return FALSE;
> >> }
> >>
> >> - sink = gst_element_factory_make("appsink", "sink");
> >> - if (sink == NULL) {
> >> - spice_warning("error upon creation of 'appsink' element");
> >> - gst_object_unref(playbin);
> >> - return FALSE;
> >> - }
> >> -
> >> - caps = gst_caps_from_string("video/x-raw,format=BGRx");
> >> - g_object_set(sink,
> >> + g_object_get(decoder->base.stream->channel, "handle", &handle, NULL);
> >> + SPICE_DEBUG("Creating Gstreamer pipline (handle for overlay %s)\n",
> >> + handle ? "received" : "not received");
> >> + if (handle == 0) {
> >> + sink = gst_element_factory_make("appsink", "sink");
> >> + if (sink == NULL) {
> >> + spice_warning("error upon creation of 'appsink' element");
> >> + gst_object_unref(playbin);
> >> + return FALSE;
> >> + }
> >> + caps = gst_caps_from_string("video/x-raw,format=BGRx");
> >> + g_object_set(sink,
> >> "caps", caps,
> >> "sync", FALSE,
> >> "drop", FALSE,
> >> NULL);
> >> - gst_caps_unref(caps);
> >> + gst_caps_unref(caps);
> >> + g_object_set(playbin,
> >> + "video-sink", gst_object_ref(sink),
> >> + NULL);
> >> +
> >> + decoder->appsink = GST_APP_SINK(sink);
> >> + } else {
> >> + /*
> >> + * handle has received, it means playbin will render directly into
> >> + * widget using the gstoverlay 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);
> >>
> >> g_object_set(playbin,
> >> "uri", "appsrc://",
> >> - "video-sink", gst_object_ref(sink),
> >> NULL);
> >>
> >> /* Disable audio in playbin */
> >> @@ -384,7 +424,6 @@ static gboolean create_pipeline(SpiceGstDecoder
> >> *decoder)
> >> g_object_set(playbin, "flags", flags, NULL);
> >>
> >> g_warn_if_fail(decoder->appsrc == NULL);
> >> - decoder->appsink = GST_APP_SINK(sink);
> >> decoder->pipeline = playbin;
> >> #else
> >> gchar *desc;
> >> @@ -417,7 +456,9 @@ static gboolean create_pipeline(SpiceGstDecoder
> >> *decoder)
> >> #endif
> >>
> >> appsink_cbs.new_sample = new_sample;
> >> - gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs, decoder,
> >> NULL);
> >> + if (decoder->appsink) {
> >> + gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs,
> >> decoder,
> >> NULL);
> >> + }
> >> bus = gst_pipeline_get_bus(GST_PIPELINE(decoder->pipeline));
> >> gst_bus_add_watch(bus, handle_pipeline_message, decoder);
> >> gst_object_unref(bus);
> >> diff --git a/src/channel-display.c b/src/channel-display.c
> >> index b41093b..8e63764 100644
> >> --- a/src/channel-display.c
> >> +++ b/src/channel-display.c
> >> @@ -70,6 +70,7 @@ struct _SpiceDisplayChannelPrivate {
> >> GArray *monitors;
> >> guint monitors_max;
> >> gboolean enable_adaptive_streaming;
> >> + guintptr handle;
> >> SpiceGlScanout scanout;
> >> };
> >>
> >> @@ -83,6 +84,7 @@ enum {
> >> PROP_MONITORS,
> >> PROP_MONITORS_MAX,
> >> PROP_GL_SCANOUT,
> >> + PROP_HANDLE,
> >> };
> >>
> >> enum {
> >> @@ -91,6 +93,7 @@ enum {
> >> SPICE_DISPLAY_INVALIDATE,
> >> SPICE_DISPLAY_MARK,
> >> SPICE_DISPLAY_GL_DRAW,
> >> + SPICE_DISPLAY_STREAM_AREA,
> >>
> >> SPICE_DISPLAY_LAST_SIGNAL,
> >> };
> >> @@ -227,6 +230,10 @@ static void spice_display_get_property(GObject
> >> *object,
> >> g_value_set_static_boxed(value,
> >> spice_display_channel_get_gl_scanout(channel));
> >> break;
> >> }
> >> + case PROP_HANDLE: {
> >> + g_value_set_ulong(value, c->handle);
> >> + break;
> >> + }
brackets are not needed, just a bit confusing.
> >> default:
> >> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> >> break;
> >> @@ -238,7 +245,14 @@ static void spice_display_set_property(GObject
> >> *object,
> >> const GValue *value,
> >> GParamSpec *pspec)
> >> {
> >> + SpiceDisplayChannel *channel = SPICE_DISPLAY_CHANNEL(object);
> >> + SpiceDisplayChannelPrivate *c = channel->priv;
> >> +
> >> switch (prop_id) {
> >> + case PROP_HANDLE: {
> >> + c->handle = g_value_get_ulong(value);
> >> + break;
> >> + }
> >> default:
> >> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> >> break;
> >> @@ -338,6 +352,16 @@ static void
> >> spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
> >> G_PARAM_READABLE |
> >> G_PARAM_STATIC_STRINGS));
> >>
> >> + g_object_class_install_property
> >> + (gobject_class, PROP_HANDLE,
> >> + g_param_spec_ulong("handle",
> >> + "Display handle",
> >> + "Display handle",
> >> + 0, G_MAXULONG, 0,
> >> + G_PARAM_WRITABLE |
> >> + G_PARAM_READABLE |
> >> + G_PARAM_STATIC_STRINGS));
> >> +
> >> /**
> >> * SpiceDisplayChannel::display-primary-create:
> >> * @display: the #SpiceDisplayChannel that emitted the signal
> >> @@ -454,6 +478,27 @@ static void
> >> spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
> >> 4,
> >> G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT,
> >> G_TYPE_UINT);
> >>
> >> + /**
> >> + * SpiceDisplayChannel::stream-area:
> >> + * @display: the #SpiceDisplayChannel that emitted the signal
> >> + * @x: x position
> >> + * @y: y position
> >> + * @width: width
> >> + * @height: height
> >> + *
> >> + * The #SpiceDisplayChannel::stream-area signal is emitted when
> >> + * the rectangular region x/y/w/h is known as an area of a
> >> + * video stream to be received.
> >> + **/
> >> + signals[SPICE_DISPLAY_STREAM_AREA] =
> >> + g_signal_new("stream-area",
> >> + G_OBJECT_CLASS_TYPE(gobject_class),
> >> + 0, 0, NULL, NULL,
> >> + g_cclosure_user_marshal_VOID__UINT_UINT_UINT_UINT,
maybe we should use INT instead of UINT ? We pass values from a rectangle which
have signed values.
> >> + G_TYPE_NONE,
> >> + 4,
> >> + G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT);
> >> +
> >> g_type_class_add_private(klass, sizeof(SpiceDisplayChannelPrivate));
> >>
> >> channel_set_handlers(SPICE_CHANNEL_CLASS(klass));
> >> @@ -1261,6 +1306,15 @@ static display_stream
> >> *display_stream_create(SpiceChannel *channel, uint32_t sur
> >> #endif
> >> default:
> >> #ifdef HAVE_GSTVIDEO
> >> + if (c->primary->width == (dest->right - dest->left) &&
> >> + c->primary->height == (dest->bottom - dest->top)) {
> >> + SPICE_DEBUG("It seems to be a FULL-SCREEN stream");
> >> + /* stream area location is sent but currently not used */
> >> + g_coroutine_signal_emit(channel,
> >> signals[SPICE_DISPLAY_STREAM_AREA], 0,
> >> + dest->left, dest->top,
> >> + c->primary->width,
> >> c->primary->height);
> >> + //TODO: may not finished before creating decoder? also there is
> >> no
> >> fallback, assuming success
> >> + }
> >> st->video_decoder = create_gstreamer_decoder(codec_type, st);
> >> #endif
> >> break;
> >> diff --git a/src/spice-widget.c b/src/spice-widget.c
> >> index 1e7add4..73a77b7 100644
> >> --- a/src/spice-widget.c
> >> +++ b/src/spice-widget.c
> >> @@ -612,6 +612,29 @@ G_GNUC_END_IGNORE_DEPRECATIONS
> >> #endif
> >> #endif
> >>
> >> +static void
> >> +gst_area_realize(GtkGLArea *area, gpointer user_data)
> >> +{
> >> +//TODO: needs rework, currently works only under X
> >> +#ifdef GDK_WINDOWING_X11
> >> + SpiceDisplay *display = SPICE_DISPLAY(user_data);
> >> + SpiceDisplayPrivate *d = display->priv;
> >> + GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(display));
> >> +
> >> + if (window) {
> >> +#if GTK_CHECK_VERSION(2,18,0)
> >> + if (!gdk_window_ensure_native (window)) {
> >> + g_warning("Couldn't create native window needed for
> >> GstVideoOverlay!");
> >> + return;
> >> + }
> >> +#endif
> >> + g_object_set(G_OBJECT (d->display),
> >> + "handle", GDK_WINDOW_XID(window),
> >> + NULL);
> >> + }
> >> +#endif
> >> +}
> >> +
> >> static void
> >> drawing_area_realize(GtkWidget *area, gpointer user_data)
> >> {
> >> @@ -660,6 +683,13 @@ G_GNUC_BEGIN_IGNORE_DEPRECATIONS
> >> G_GNUC_END_IGNORE_DEPRECATIONS
> >> #endif
> >> #endif
> >> +
> >> + area = gtk_drawing_area_new();
> >> + g_object_connect(area,
> >> + "signal::realize", gst_area_realize, display,
> >> + NULL);
> >> + gtk_stack_add_named(d->stack, area, "gst-area");
> >> +
> >> gtk_widget_show_all(widget);
> >>
> >> g_signal_connect(display, "grab-broken-event",
> >> G_CALLBACK(grab_broken),
> >> NULL);
> >> @@ -2589,6 +2619,20 @@ static void queue_draw_area(SpiceDisplay *display,
> >> gint x, gint y,
> >> x, y, width, height);
> >> }
> >>
> >> +static void pop_stream_area(SpiceChannel *channel, guint x, guint y,
> >> + guint width, guint height, gpointer data)
> >> +{
> >> +/* Currently it works only in X11 environment - do not set area visible
> >> if
> >> it's not X11 */
> >> +#ifdef GDK_WINDOWING_X11
> >> + SpiceDisplay *display = data;
> >> + SpiceDisplayPrivate *d = display->priv;
> >> +
> >> + if (GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
> >> + gtk_stack_set_visible_child_name(d->stack, "gst-area");
> >> + }
> >> +#endif
> >> +}
> >> +
> >> static void invalidate(SpiceChannel *channel,
> >> gint x, gint y, gint w, gint h, gpointer data)
> >> {
> >> @@ -2953,6 +2997,8 @@ static void channel_new(SpiceSession *s,
> >> SpiceChannel
> >> *channel, gpointer data)
> >> spice_g_signal_connect_object(channel, "notify::monitors",
> >> G_CALLBACK(spice_display_widget_update_monitor_area),
> >> display, G_CONNECT_AFTER |
> >> G_CONNECT_SWAPPED);
> >> + spice_g_signal_connect_object(channel, "stream-area",
> >> + G_CALLBACK(pop_stream_area),
> >> display,
> >> 0);
> >> if (spice_display_channel_get_primary(channel, 0, &primary)) {
> >> primary_create(channel, primary.format, primary.width,
> >> primary.height,
> >> primary.stride, primary.shmid, primary.data,
> >> display);
>
>
Frediano
More information about the Spice-devel
mailing list