Re: [Spice-devel] [RFC spice-gtk 1/1] Gstreamer: Use GstVideoOverlay if possible

2018-07-26 Thread Frediano Ziglio
> Hi,

> On 07/24/2018 06:47 PM, Marc-André Lureau wrote:

> > Hi
> 

> > On Sun, Mar 11, 2018 at 10:44 AM, Snir Sheriber 
> > wrote:
> 

> > > Currently when gstreamer is used to decode a full-screen
> > 
> 

> > > ...
> > 
> 

> > > 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");
> > 
> 

> > Was there a good reason to use a seperate drawing area rather than the
> 
> > existing one?
> 

> There were some reasons for this, iirc there was an issue with the signals
> that was needed to be disabled to avoid interference with the drawings (i
> had a version where it uses the same drawing area but eventually it was
> dropped) and also at first i was trying both gl-area and drawing-area so it
> was
> easier to switch.
> (Also please notice that this patch is not the recent version which was
> pushed)

I think the "there were some reasons" is an indication that a comment in 
the code with the reason would be good. 

> > Is there a simple way to test the "streaming mode"? A test case like
> 
> > server/tests/test-display-streaming would be great.
> 

> Could be tested by using spice-streaming-agent that streams mjpeg to a client
> that was
> built with disabled builtin_mjpeg (so it will use the spice-gst-decoder)
> or
> By using spice-streaming-agent that streams h264/vp8 video stream using the
> gst-plugin
> which was recently sent to ML and it is still not pushed upstream.

> Currently I'm not aware to any similar\other test option that is available
> upstream.

> Snir.

Tests are always great. I don't think we really need all these system settings. 
But that would be the target of this/these tests? Run into a CI? Manually 
testing? 
I think gstreamer and video output are used here, the patch is supposed to 
optimize 
the rendering part and the full pipeline. Also depends on codec and plugins 
installed 
(although in a CI we can force to have some plugins installed, but still hard 
to get 
a display server... or a vncserver is enough?) 

Frediano 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [RFC spice-gtk 1/1] Gstreamer: Use GstVideoOverlay if possible

2018-07-26 Thread Snir Sheriber

Hi,


On 07/24/2018 06:47 PM, Marc-André Lureau wrote:

Hi

On Sun, Mar 11, 2018 at 10:44 AM, Snir Sheriber  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.
---
  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", , 
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", , 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 = 

Re: [Spice-devel] [RFC spice-gtk 1/1] Gstreamer: Use GstVideoOverlay if possible

2018-07-24 Thread Marc-André Lureau
Hi

On Sun, Mar 11, 2018 at 10:44 AM, Snir Sheriber  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.
> ---
>  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", , 
> 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", , 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 

Re: [Spice-devel] [RFC spice-gtk 1/1] Gstreamer: Use GstVideoOverlay if possible

2018-04-09 Thread Frediano Ziglio
> 
> 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;
> >   boolprimary;
> > +boolstreaming_mode;
> >   enum SpiceSurfaceFmtformat;
> >   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",
> >> ,
> >> NULL);
> >> +SPICE_DEBUG("prepare-window-handle msg received (handle:
> >> %lu)",
> >> handle);
> >> +   

Re: [Spice-devel] [RFC spice-gtk 1/1] Gstreamer: Use GstVideoOverlay if possible

2018-04-08 Thread Snir Sheriber

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;
  boolprimary;
+boolstreaming_mode;
  enum SpiceSurfaceFmtformat;
  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.


Frediano


---
  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", ,
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 

Re: [Spice-devel] [RFC spice-gtk 1/1] Gstreamer: Use GstVideoOverlay if possible

2018-04-06 Thread Frediano Ziglio
> 
> 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;
 boolprimary;
+boolstreaming_mode;
 enum SpiceSurfaceFmtformat;
 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) {


Frediano

> ---
>  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", ,
> 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");
> -