Re: [Spice-devel] [PATCH spice-protocol 2/8 v2] Add the StreamMsgGraphicsDeviceInfo message

[Frediano Ziglio]

> 
> The message contains information about the graphics device and monitor
> belonging to a particular video stream (which maps to a channel) from
> the streaming agent.
> 
> Signed-off-by: Lukáš Hrázký 

Acked-by: Frediano Ziglio 

> ---
>  spice/stream-device.h | 31 +++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/spice/stream-device.h b/spice/stream-device.h
> index 6add42b..c70690a 100644
> --- a/spice/stream-device.h
> +++ b/spice/stream-device.h
> @@ -90,6 +90,8 @@ typedef enum StreamMsgType {
>  STREAM_TYPE_CURSOR_SET,
>  /* guest cursor position */
>  STREAM_TYPE_CURSOR_MOVE,
> +/* the graphics device display information message (device address and
> display id) */
> +STREAM_TYPE_DEVICE_DISPLAY_INFO,
>  } StreamMsgType;
>  
>  typedef enum StreamCapabilities {
> @@ -140,6 +142,35 @@ typedef struct StreamMsgData {
>  uint8_t data[0];
>  } StreamMsgData;
>  
> +/* This message contains information about the graphics device and monitor
> + * belonging to a particular video stream (which maps to a channel) from
> + * the streaming agent.
> + *
> + * The device_address is the hardware address of the device (e.g. PCI),
> + * device_display_id is the id of the monitor on the device.
> + *
> + * The supported device address format is:
> + * "pci//./.../."
> + *
> + * The "pci" identifies the rest of the string as a PCI address. It is the
> only
> + * supported address at the moment, other identifiers can be introduced
> later.
> + *  is the PCI domain, followed by . of any PCI
> bridges
> + * in the chain leading to the device. The last . is the
> + * graphics device. All of , ,  are hexadecimal
> numbers
> + * with the following number of digits:
> + *   : 4
> + *   : 2
> + *   : 1
> + *
> + * Sent from the streaming agent to the server.
> + */
> +typedef struct StreamMsgDeviceDisplayInfo {
> +uint32_t stream_id;
> +uint32_t device_display_id;
> +uint32_t device_address_len;
> +uint8_t device_address[0];  // a zero-terminated string
> +} StreamMsgDeviceDisplayInfo;
> +

Curiosity: is there a reason why this message was not added at the end
of the header?

>  /* Tell to stop current stream and possibly start a new one.
>   * This message is sent by the host to the guest.
>   * Allows to communicate the codecs supported by the clients.

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

Re: [Spice-devel] [PATCH spice-gtk v7] Fix overlay for vaapisink

[Snir Sheriber]

Hi,

On 1/15/19 3:03 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 
---
To test it probably you'll have to remove the gstreamer registry,
usually in ~/.cache/gstreamer-1.0/registry.x86_64.bin.

Changes since v6:
- rebase on master.

Changes since v5:
- test GStreamer version to use or not vaapisink.

Changes since v4:
- factor out a create_vaapi_context.

Changes since v3:
- none, add a patch to fix another issue.

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 |  8 --
  src/spice-widget.c| 57 +++
  5 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 5d6a1a01..2f634223 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 94d660a6..d7062af2 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 abc2f694..a9617d47 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..5b7b776a 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -406,14 +406,17 @@ 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
+ */
+SPICE_DEBUG("Video is presented using gstreamer's GstVideoOverlay 
interface");
+
+#if !GST_CHECK_VERSION(1,14,0)



It indeed possible to check plugin version as was done in "[PATCH 
spice-gtk v3] gst: check pulsesrc version >= 1.14.5​"


but i think GST_CHECK_VERSION is enough here


ACK


+/* 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");
@@ -422,6 +425,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
  gst_plugin_feature_set_rank(vaapisink, GST_RANK_NONE);
  gst_object_unref(vaapisink);
  }
+#endif
  }
  
  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 0c6f16fd..8adcc384 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -28,6 +28,9 @@
  #ifdef 

[Spice-devel] [PATCH v2] gstreamer: set timestamp in buffer's GstReferenceTimestampMeta

[Snir Sheriber]

Currently we set timestamps as buffer's PTS, this value may be changed by
the pipeline in some cases and cause an unexpected buffer warnings (when
GstVideoOverlay is not used). Use GstReferenceTimestampMeta when
synchronization is made by spice.

Before applying this patch you can reproduce the warnings by runing with
DISABLE_GSTVIDEOOVERLAY=1 and starting some audio playback in the guest.

Signed-off-by: Snir Sheriber 
---
Changes from v1:
-Commit message
---
 src/channel-display-gst.c | 27 ---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 2f556fe..a90fa81 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -33,6 +33,10 @@ typedef struct SpiceGstFrame SpiceGstFrame;
 
 /* GStreamer decoder implementation */
 
+#if GST_CHECK_VERSION(1,14,0)
+static GstStaticCaps stream_reference = 
GST_STATIC_CAPS("timestamp/spice-stream");
+#endif
+
 typedef struct SpiceGstDecoder {
 VideoDecoder base;
 
@@ -86,7 +90,14 @@ struct SpiceGstFrame {
 static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceFrame *frame)
 {
 SpiceGstFrame *gstframe = g_new(SpiceGstFrame, 1);
+#if GST_CHECK_VERSION(1,14,0)
+GstReferenceTimestampMeta *time_meta;
+
+time_meta = gst_buffer_get_reference_timestamp_meta(buffer, 
gst_static_caps_get(_reference));
+gstframe->timestamp = time_meta->timestamp;
+#else
 gstframe->timestamp = GST_BUFFER_PTS(buffer);
+#endif
 gstframe->frame = frame;
 gstframe->sample = NULL;
 return gstframe;
@@ -211,6 +222,12 @@ static void fetch_pending_sample(SpiceGstDecoder *decoder)
 decoder->pending_samples--;
 
 GstBuffer *buffer = gst_sample_get_buffer(sample);
+GstClockTime buffer_ts;
+#if GST_CHECK_VERSION(1,14,0)
+buffer_ts = gst_buffer_get_reference_timestamp_meta(buffer, 
gst_static_caps_get(_reference))->timestamp;
+#else
+buffer_ts = GST_BUFFER_PTS(buffer);
+#endif
 
 /* gst_app_sink_pull_sample() sometimes returns the same buffer twice
  * or buffers that have a modified, and thus unrecognizable, PTS.
@@ -223,7 +240,7 @@ static void fetch_pending_sample(SpiceGstDecoder *decoder)
 GList *l = g_queue_peek_head_link(decoder->decoding_queue);
 while (l) {
 gstframe = l->data;
-if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
+if (gstframe->timestamp == buffer_ts) {
 /* The frame is now ready for display */
 gstframe->sample = sample;
 decoder->display_frame = gstframe;
@@ -232,7 +249,7 @@ static void fetch_pending_sample(SpiceGstDecoder *decoder)
  * frames from the decoding queue.
  */
 while ((gstframe = g_queue_pop_head(decoder->decoding_queue))) 
{
-if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
+if (gstframe->timestamp == buffer_ts) {
 break;
 }
 /* The GStreamer pipeline dropped the corresponding
@@ -626,9 +643,13 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder 
*video_decoder,
 frame->data, frame->size, 
0, frame->size,
 frame->data_opaque, 
frame->unref_data);
 
+GstClockTime pts = gst_clock_get_time(decoder->clock) - 
gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0, latency)) * 
1000 * 1000;
 GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
 GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
-GST_BUFFER_PTS(buffer) = gst_clock_get_time(decoder->clock) - 
gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0, latency)) * 
1000 * 1000;
+GST_BUFFER_PTS(buffer) = pts;
+#if GST_CHECK_VERSION(1,14,0)
+gst_buffer_add_reference_timestamp_meta(buffer, gst_static_caps_get 
(_reference), pts, GST_CLOCK_TIME_NONE);
+#endif
 
 if (decoder->appsink != NULL) {
 SpiceGstFrame *gst_frame = create_gst_frame(buffer, frame);
-- 
2.19.1

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

Re: [Spice-devel] [PATCH spice-gtk v4 1/2] Fix overlay for vaapisink

[Snir Sheriber]

On 1/15/19 12:22 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 
---
To test it probably you'll have to remove the gstreamer registry,
usually in ~/.cache/gstreamer-1.0/registry.x86_64.bin.

Changes since v3:
- none, add a patch to fix another issue;

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| 45 +++
  5 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/configure.ac b/configure.ac
index bba13fba..0f69b3c2 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 acd5dcaf..69bbee57 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 d1de9473..66884a74 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);
-}
  }



I think the support gstcontext for vaapi elements was introduced pretty 
lately
(if i'm not mistaken 1.13.90) so i guess it worth removing this code 
only in higher

versions


  
  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..69c00558 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -28,6 +28,9 @@
  #ifdef GDK_WINDOWING_X11
  #include 



  #include 
+#ifdef HAVE_LIBVA
+#include 
+#endif
  #endif
  #ifdef G_OS_WIN32
  #include 
@@ -2592,6 +2595,48 @@ 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, _type);
+SPICE_DEBUG("GStreamer: 

Re: [Spice-devel] [PATCH spice-common 3/3] docs: add spice URI scheme

[Marc-André Lureau]

On Mon, Jan 14, 2019 at 8:36 PM Frediano Ziglio  wrote:
>
> >
> > From: Marc-André Lureau 
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  docs/Makefile.am  |   1 +
> >  docs/meson.build  |   2 +-
> >  docs/spice_uri_scheme.txt | 131 ++
> >  3 files changed, 133 insertions(+), 1 deletion(-)
> >  create mode 100644 docs/spice_uri_scheme.txt
> >
> > diff --git a/docs/Makefile.am b/docs/Makefile.am
> > index e7e126b..6f7a0ac 100644
> > --- a/docs/Makefile.am
> > +++ b/docs/Makefile.am
> > @@ -3,6 +3,7 @@ CLEANFILES =
> >
> 
>
> > +documentation.
> > +
> > +Data Types
> > +--
> > +
> > +Spice URIs can be percent-encoded as specified in [RFC3986] and MUST
> > +be decoded.  After decoding, the following type constraints and
> > +semantics apply:
> > +
> > +string
> > +~~
> > +
> > +Values of "string" type are UTF-encoded strings as specified in
>
> Minor: UTF8, not any UTF.

ack series with that change?

>
> > +[RFC3629].
> > +
> > +ushort
> > +~~
> > +
> > +The "ushort" values represent unsigned 16-bit integers expressed
> > +in decimal digits with value between 0-65535 inclusive.
>
> Frediano
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common 3/3] docs: add spice URI scheme

[Christophe Fergeau]

Hey,

On Fri, Jan 11, 2019 at 11:07:14PM +0400, marcandre.lur...@redhat.com wrote:
> @@ -0,0 +1,131 @@
> +The "spice" URI scheme
> +==
> +
> +This document is inspired by 'The "vnc" URI Scheme' (rfc7869) and
> +attempts to document a standard Spice URI scheme.
> +
> +The normative syntax of the Spice URI is defined in the 
> +rule in the following syntax specification.  This specification
> +uses the Augmented Backus-Naur Form (ABNF) as described in
> +[RFC5234].  The Spice URI conforms to the generic URI syntax
> +specified in [RFC3986].  The , , ,
> +, and  rules are defined in [RFC3986].
> +
> + spice-uri = spice-scheme "://" [ userinfo "@" ] [ host [ ":" port ] ]
> +  [ "?" [ spice-params ] ]
> +
> + spice-scheme = "spice" / "spice+unix" / "spice+tls"
> +
> + spice-params = param "=" value *("&" param "=" value) ["&"]
> +
> + param = 1*( param-char )
> +
> + value = *( param-char )
> +
> + param-char = unreserved / pct-encoded / unreserved-symbols
> +
> + unreserved-symbols = ":" / "/" / "@" / "!" / "$" / "'"
> +   / "(" / ")" / "*" / "," / ";"
> +
> +The "?", "=", and "&" characters are used to delimit Spice parameters
> +and must be percent-encoded when representing a data octet as
> +specified in [RFC3986].  Within the  portion of a Spice
> +URI, the  do not have special meaning and need not
> +be percent-encoded when representing a data octet.
> +
> +A Spice URI has the general form:
> +
> + spice-scheme://host:port?param1=value1=value2...

I'd mention 'userinfo' in that URI.

> +The  value in the "spice+unix://" URI specify the UNIX domain
> +socket path of the Spice server on the local host:
> +
> +[options="header"]
> +|===
> +| Name   | Type   | Description | Default
> +| host   | string | UNIX domain socket path | none
> +|===

RFC3986 has a definition for a 'path-absolute', but this is something which is
concatenated to the 'host'. A 'host' for RFC3986 cannot contain a '/':

   host  = IP-literal / IPv4address / reg-name
with
   reg-name  = *( unreserved / pct-encoded / sub-delims )
   unreserved= ALPHA / DIGIT / "-" / "." / "_" / "~"
   sub-delims= "!" / "$" / "&" / "'" / "(" / ")"
   / "*" / "+" / "," / ";" / "="

I guess you could rework the current definition with something like:

URI = URI-unix / URI-net
URI-net = spice-scheme-net://host:port?param1=value1=value2...
URI-unix = spice+unix://path-absolute

Or decide that for now, the current definition is good enough ;)


Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

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 
---
To test it probably you'll have to remove the gstreamer registry,
usually in ~/.cache/gstreamer-1.0/registry.x86_64.bin.

Changes since v6:
- rebase on master.

Changes since v5:
- test GStreamer version to use or not vaapisink.

Changes since v4:
- factor out a create_vaapi_context.

Changes since v3:
- none, add a patch to fix another issue.

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 |  8 --
 src/spice-widget.c| 57 +++
 5 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 5d6a1a01..2f634223 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 94d660a6..d7062af2 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 abc2f694..a9617d47 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..5b7b776a 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -406,14 +406,17 @@ 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
+ */
+SPICE_DEBUG("Video is presented using gstreamer's GstVideoOverlay 
interface");
+
+#if !GST_CHECK_VERSION(1,14,0)
+/* 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");
@@ -422,6 +425,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
 gst_plugin_feature_set_rank(vaapisink, GST_RANK_NONE);
 gst_object_unref(vaapisink);
 }
+#endif
 }
 
 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 0c6f16fd..8adcc384 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -28,6 +28,9 @@
 #ifdef GDK_WINDOWING_X11
 #include 
 #include 
+#ifdef HAVE_LIBVA
+#include 
+#endif
 #endif
 #ifdef G_OS_WIN32
 #include 
@@ -2566,6 +2569,40 @@ static void queue_draw_area(SpiceDisplay *display, gint 
x, gint y,
 }
 
 #if defined(GDK_WINDOWING_X11)
+
+#if 

Re: [Spice-devel] What algorithm does SPICE server use in video stream detection?

[Christophe Fergeau]

Hey,

On Thu, Jan 10, 2019 at 03:03:05PM +0800, 陈炤 wrote:
> Hi,
> 
> 
> 
> I read the spice server code, and find that the video stream detection
> code is in server/spice-bitmap-utils.c. func
> bitmap_get_graduality_level will calculate a score, and the GRADUALITY
> is set based on the score.
> 
> So what's the meaning of this score, and what algorithm does it use to
> calculate this score?

Long time since I looked at that code, but iirc it tries to detect if
the region looks like text or an image, and if it's changing a lot. If
it's an image which is changing a lot, then it's recognized as a video
stream.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-protocol 1/8 v2] Add the VDAgentGraphicsDeviceInfo message

[Frediano Ziglio]

> The message serves for passing the device address and device display ID
> information for all display channels from SPICE server to the vd_agent.
> 
> Signed-off-by: Lukáš Hrázký 

Acked-by: Frediano Ziglio 

> ---
>  spice/vd_agent.h | 23 +++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> index dda7044..42ec77a 100644
> --- a/spice/vd_agent.h
> +++ b/spice/vd_agent.h
> @@ -91,6 +91,7 @@ enum {
>  VD_AGENT_CLIENT_DISCONNECTED,
>  VD_AGENT_MAX_CLIPBOARD,
>  VD_AGENT_AUDIO_VOLUME_SYNC,
> +VD_AGENT_GRAPHICS_DEVICE_INFO,
>  VD_AGENT_END_MESSAGE,
>  };
>  
> @@ -248,6 +249,27 @@ typedef struct SPICE_ATTR_PACKED VDAgentAudioVolumeSync
> {
>  uint16_t volume[0];
>  } VDAgentAudioVolumeSync;
>  
> +typedef struct SPICE_ATTR_PACKED VDAgentDeviceDisplayInfo {
> +uint32_t channel_id;
> +uint32_t monitor_id;
> +uint32_t device_display_id;
> +uint32_t device_address_len;
> +uint8_t device_address[0];  // a zero-terminated string
> +} VDAgentDeviceDisplayInfo;
> +
> +
> +/* This message contains the mapping of (channel_id, monitor_id) pair to a
> + * "physical" (virtualized) device and its monitor identified by
> device_address
> + * and device_display_id.
> + *
> + * It's used on the vd_agent to identify the guest monitors for the
> + * mouse_position and monitors_config messages.
> + */
> +typedef struct SPICE_ATTR_PACKED VDAgentGraphicsDeviceInfo {
> +uint32_t count;
> +VDAgentDeviceDisplayInfo display_info[0];
> +} VDAgentGraphicsDeviceInfo;
> +
>  enum {
>  VD_AGENT_CAP_MOUSE_STATE = 0,
>  VD_AGENT_CAP_MONITORS_CONFIG,
> @@ -264,6 +286,7 @@ enum {
>  VD_AGENT_CAP_MONITORS_CONFIG_POSITION,
>  VD_AGENT_CAP_FILE_XFER_DISABLED,
>  VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
> +VD_AGENT_CAP_GRAPHICS_DEVICE_INFO,
>  VD_AGENT_END_CAP,
>  };
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer

[Christophe Fergeau]

Hey,

On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote:
> >
> > >  if (!priv->host)
> > > -g_error("Out of memory allocating usbredirhost");
> > > +g_error("Out of memory initializing redirection support");
> > >
> > > -#if USBREDIR_VERSION >= 0x000701
> > > -usbredirhost_set_buffered_output_size_cb(priv->host,
> > usbredir_buffered_output_size_callback);
> > > -#endif
> > >  #ifdef USE_LZ4
> > >  spice_channel_set_capability(channel,
> > SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4);
> > >  #endif
> > > @@ -299,8 +279,6 @@ static gboolean spice_usbredir_channel_open_device(
> > >  {
> > >  SpiceUsbredirChannelPrivate *priv = channel->priv;
> > >  SpiceSession *session;
> > > -libusb_device_handle *handle = NULL;
> > > -int rc, status;
> > >  SpiceUsbDeviceManager *manager;
> > >
> > >  g_return_val_if_fail(priv->state == STATE_DISCONNECTED
> > > @@ -309,21 +287,16 @@ static gboolean spice_usbredir_channel_open_device(
> > >  #endif
> > >   , FALSE);
> > >
> > > -rc = libusb_open(priv->device, );
> > > -if (rc != 0) {
> > > -g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > > -"Could not open usb device: %s [%i]",
> > > -spice_usbutil_libusb_strerror(rc), rc);
> > > -return FALSE;
> > > -}
> > > -
> > >  priv->catch_error = err;
> > > -status = usbredirhost_set_device(priv->host, handle);
> > > -priv->catch_error = NULL;
> > > -if (status != usb_redir_success) {
> > > -g_return_val_if_fail(err == NULL || *err != NULL, FALSE);
> > > +if (!spice_usb_backend_channel_attach(priv->host, priv->device,
> > err)) {
> > > +priv->catch_error = NULL;
> > > +if (*err == NULL) {
> > > +g_set_error(err, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED,
> > > +"Error attaching device: (no error information)");
> >
> > You could line up the "Error .." with the opening parenthesis.
> >
> >
> This is not described exactly in Spice coding style document and existing
> code of spice-gtk
> uses different solution for indentation, so this is point of personal
> preference.

From a quick look, aligning multiple line arguments lists seem much more
common that then opposite. See the g_set_error call that this hunk is
removing for example.

> > > @@ -362,8 +335,7 @@ static void spice_usbredir_channel_open_acl_cb(
> > >  spice_usbredir_channel_open_device(channel, );
> > >  }
> > >  if (err) {
> > > -libusb_unref_device(priv->device);
> > > -priv->device = NULL;
> > > +g_clear_pointer(>device,
> > spice_usb_backend_device_release);
> >
> > _unref rather than _release?
> >
> 
> This can be changed in V3, although I would prefer not to change it.
> Semantics of ref/unref and acquire/release are not different.
> Please let me know whether this change in mandatory.

I assume you mean that the semantics of ref/unref and acquire/release
*are* different? If this is what you mean, how are they different?
If the semantics are the same, then let's use the same name.

> > > @@ -446,7 +420,8 @@ void spice_usbredir_channel_connect_device_async(
> > >  goto done;
> > >  }
> > >
> > > -priv->device = libusb_ref_device(device);
> > > +spice_usb_backend_device_acquire(device);
> > > +priv->device = device;
> >
> > You could mimic libusb API actually,
> > priv->device = spice_usb_backend_device_ref(device);
> >
> 
> I do not think I need to mimic libusb or something other.
> Please let me know whether this change is mandatory.

libusb_ref_device, g_object_ref all return a pointer to the new ref, and
this makes that code slightly simpler, so just a suggestion in case you
think that's a good change.

> >
> > > +void *msc;
> > > +} d;
> > > +gboolean is_libusb;
> > > +gint ref_count;
> > > +SpiceUsbBackendChannel *attached_to;
> >
> > You don't need 'msc' just yet, nor 'is_libusb', and I'm not sure about
> > 'attached_to'
> >
> 
> msc can be changed to reserved if you do not like it so much.
> removal of is_libusb makes further merges harder (and current commit is
> only prerequisite for further merge).
> attached_to is useful for debugging/support
> Please let me know whether this is a requirement.

This patch is refactoring the existing code so that we can add more
stuff on top of it. It should not be adding new things, especially if
they are not used in the current commit. attached_to can be added in a
separate commit with the debugging checks that it lets us do, msc can be
added when it's needed.

> 
> >
> > > +void *hotplug_user_data;
> > > +libusb_hotplug_callback_handle hotplug_handle;
> > > +};
> > > +
> > > +struct _SpiceUsbBackendChannel
> > > +{
> > > +struct usbredirhost *usbredirhost;
> > > +uint8_t *read_buf;
> > > +int read_buf_size;
> > > +struct usbredirfilter_rule *rules;
> > > +int 

Re: [Spice-devel] [PATCH spice-gtk v4 2/2] spice-widget: Avoid deadlock for VAAPI

[Snir Sheriber]

This patch fixes the issues I've experienced before

Ack

On 1/15/19 12:22 PM, Frediano Ziglio wrote:

Calling gst_video_overlay_handle_events after
gst_video_overlay_set_window_handle causes often a deadlock in
gstreamer-vaapi.
Reverting the calls fix this issue.

Signed-off-by: Frediano Ziglio 
---
  src/spice-widget.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/spice-widget.c b/src/spice-widget.c
index 69c00558..1d7c8c17 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -2588,8 +2588,8 @@ static void gst_sync_bus_call(GstBus *bus, GstMessage 
*msg, SpiceDisplay *displa
  
  GstVideoOverlay *overlay = GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg));

  g_weak_ref_set(>overlay_weak_ref, overlay);
-gst_video_overlay_set_window_handle(overlay, 
(uintptr_t)GDK_WINDOW_XID(window));
  gst_video_overlay_handle_events(overlay, false);
+gst_video_overlay_set_window_handle(overlay, 
(uintptr_t)GDK_WINDOW_XID(window));
  return;
  }
  }

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

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

[Frediano Ziglio]

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 
---
To test it probably you'll have to remove the gstreamer registry,
usually in ~/.cache/gstreamer-1.0/registry.x86_64.bin.

Changes since v5:
- test GStreamer version to use or not vaapisink.

Changes since v4:
- factor out a create_vaapi_context.

Changes since v3:
- none, add a patch to fix another issue.

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 |  8 --
 src/spice-widget.c| 57 +++
 5 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index bba13fba..0f69b3c2 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 acd5dcaf..69bbee57 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 d1de9473..66884a74 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..5b7b776a 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -406,14 +406,17 @@ 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
+ */
+SPICE_DEBUG("Video is presented using gstreamer's GstVideoOverlay 
interface");
+
+#if !GST_CHECK_VERSION(1,14,0)
+/* 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");
@@ -422,6 +425,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
 gst_plugin_feature_set_rank(vaapisink, GST_RANK_NONE);
 gst_object_unref(vaapisink);
 }
+#endif
 }
 
 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 7e9dc07e..7141fd17 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -28,6 +28,9 @@
 #ifdef GDK_WINDOWING_X11
 #include 
 #include 
+#ifdef HAVE_LIBVA
+#include 
+#endif
 #endif
 #ifdef G_OS_WIN32
 #include 
@@ -2571,6 +2574,40 @@ static void queue_draw_area(SpiceDisplay *display, gint 
x, gint y,
 }
 
 #if defined(HAVE_GSTVIDEO) && defined(GDK_WINDOWING_X11)
+
+#if defined(HAVE_LIBVA)
+static 

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

[Frediano Ziglio]

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 
---
To test it probably you'll have to remove the gstreamer registry,
usually in ~/.cache/gstreamer-1.0/registry.x86_64.bin.

Changes since v4:
- factor out a create_vaapi_context.

Changes since v3:
- none, add a patch to fix another issue.

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| 57 +++
 5 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/configure.ac b/configure.ac
index bba13fba..0f69b3c2 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 acd5dcaf..69bbee57 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 d1de9473..66884a74 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 7e9dc07e..7141fd17 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -28,6 +28,9 @@
 #ifdef GDK_WINDOWING_X11
 #include 
 #include 
+#ifdef HAVE_LIBVA
+#include 
+#endif
 #endif
 #ifdef G_OS_WIN32
 #include 
@@ -2571,6 +2574,40 @@ static void queue_draw_area(SpiceDisplay *display, gint 
x, gint y,
 }
 
 #if defined(HAVE_GSTVIDEO) && defined(GDK_WINDOWING_X11)
+
+#if defined(HAVE_LIBVA)
+static GstContext *create_vaapi_context(void)
+{
+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);
+
+  

Re: [Spice-devel] [PATCH spice-gtk v2 04/15] build-sys: drop gstvideo option, make it required

[Snir Sheriber]

Acked-by: Snir Sheriber 

On 1/9/19 12:09 PM, Frediano Ziglio wrote:

From: Marc-André Lureau 

GStreamer is being increasingly used by spice-gtk. Let's make it a
core requirement.

Signed-off-by: Marc-André Lureau 
---
  .gitlab-ci.yml |  2 --
  configure.ac   | 41 ++
  meson.build| 17 +---
  meson_options.txt  |  5 -
  src/Makefile.am|  7 +--
  src/channel-display-priv.h | 10 +-
  src/channel-display.c  |  7 ---
  src/meson.build|  5 +
  src/spice-widget-priv.h|  4 
  src/spice-widget.c | 13 +++-
  10 files changed, 22 insertions(+), 89 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 78b339e7..c956adda 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -32,7 +32,6 @@ makecheck_simple:
script:
- ./autogen.sh --enable-static
  --enable-lz4=no
---enable-gstvideo=no
  --enable-webdav=no
  --with-sasl=no
  --with-coroutine=auto
@@ -46,7 +45,6 @@ makecheck_simple:
  makecheck_simple-meson:
script:
- meson build -Dauto_features=disabled
--Dgstvideo=false
  -Dusbredir=false
  -Ddbus=false || (cat build/meson-logs/meson-log.txt && exit 1)
- ninja -C build
diff --git a/configure.ac b/configure.ac
index 85827b1d..ea4e8ff2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -221,28 +221,21 @@ SPICE_CHECK_GSTREAMER(GSTAUDIO, 1.0,
  ],
  [AC_MSG_ERROR([Required GStreamer packages missing])])
  
-AC_ARG_ENABLE([gstvideo],

-  AS_HELP_STRING([--enable-gstvideo=@<:@auto/yes/no@:>@],
- [Enable GStreamer video support @<:@default=auto@:>@]),
-  [],
-  [enable_gstvideo="auto"])
-AS_IF([test "x$enable_gstvideo" != "xno"],
-  [SPICE_CHECK_GSTREAMER(GSTVIDEO, 1.0,
- [gstreamer-1.0 gstreamer-base-1.0 gstreamer-app-1.0 
gstreamer-video-1.0],
- [missing_gstreamer_elements=""
-  SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gst-plugins-base 
1.0], [appsrc videoconvert appsink])
-  SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gst-plugins-good 
1.0], [jpegdec vp8dec vp9dec])
-  SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gst-plugins-bad 
1.0], [h264parse h265parse])
-  SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0, [gstreamer-libav 
1.0], [avdec_h264 avdec_h265])
-  AS_IF([test x"$missing_gstreamer_elements" = "xyes"],
-SPICE_WARNING([The GStreamer video decoder can be built but 
may not work.]))
- ],
- [AS_IF([test "x$enable_gstvideo" = "xyes"],
-AC_MSG_ERROR([GStreamer 1.0 video requested but not found]))
- ])
-  ], [have_gstvideo="no"]
-)
-AM_CONDITIONAL([HAVE_GSTVIDEO], [test "x$have_gstvideo" = "xyes"])
+SPICE_CHECK_GSTREAMER(GSTVIDEO, 1.0,
+[gstreamer-1.0 gstreamer-base-1.0 gstreamer-app-1.0 gstreamer-video-1.0],
+[missing_gstreamer_elements=""
+ SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0,
+ [gst-plugins-base 1.0], [appsrc videoconvert appsink])
+ SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0,
+ [gst-plugins-good 1.0], [jpegdec vp8dec vp9dec])
+ SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0,
+ [gst-plugins-bad 1.0], [h264parse h265parse])
+ SPICE_CHECK_GSTREAMER_ELEMENTS($GST_INSPECT_1_0,
+ [gstreamer-libav 1.0], [avdec_h264 avdec_h265])
+ AS_IF([test x"$missing_gstreamer_elements" = "xyes"],
+ SPICE_WARNING([The GStreamer video decoder can be built but may not 
work.]))
+],
+[AC_MSG_ERROR([Required GStreamer packages missing])])
  
  AC_ARG_ENABLE([builtin-mjpeg],

AS_HELP_STRING([--enable-builtin-mjpeg], [Enable the builtin mjpeg video decoder 
@<:@default=yes@:>@]),
@@ -252,9 +245,6 @@ AS_IF([test "x$enable_builtin_mjpeg" = "xyes"],
[AC_DEFINE([HAVE_BUILTIN_MJPEG], 1, [Use the builtin mjpeg decoder?])])
  AM_CONDITIONAL(HAVE_BUILTIN_MJPEG, [test "x$enable_builtin_mjpeg" != "xno"])
  
-AS_IF([test "x$enable_builtin_mjpeg$enable_gstvideo" = "xnono"],

-  [SPICE_WARNING([No builtin MJPEG or GStreamer decoder, video will not be 
streamed])])
-
  AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
  AC_MSG_CHECKING([for jpeglib.h])
  AC_TRY_CPP(
@@ -547,7 +537,6 @@ AC_MSG_NOTICE([
  Gtk:  ${with_gtk}
  Coroutine:${with_coroutine}
  PulseAudio:   ${enable_pulse}
-GStreamer Video:  ${have_gstvideo}
  SASL support: ${have_sasl}
  Smartcard support:${have_smartcard}
  USB redirection support:  ${have_usbredir} ${with_usbredir_hotplug}
diff --git a/meson.build b/meson.build
index 4ade991e..e1e5e919 100644
--- a/meson.build
+++ b/meson.build
@@ -161,22 +161,11 @@ if d.found()
spice_gtk_has_pulse = true
  endif
  
-deps = ['gstreamer-1.0', 

Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h

[Neil Armstrong]

On 15/01/2019 11:41, Daniel Vetter wrote:
> Having the probe helper stuff (which pretty much everyone needs) in
> the drm_crtc_helper.h file (which atomic drivers should never need) is
> confusing. Split them out.
> 
> To make sure I actually achieved the goal here I went through all
> drivers. And indeed, all atomic drivers are now free of
> drm_crtc_helper.h includes.
> 
> v2: Make it compile. There was so much compile fail on arm drivers
> that I figured I'll better not include any of the acks on v1.
> 
> v3: Massive rebase because i915 has lost a lot of drmP.h includes, but
> not all: Through drm_crtc_helper.h > drm_modeset_helper.h -> drmP.h
> there was still one, which this patch largely removes. Which means
> rolling out lots more includes all over.
> 
> This will also conflict with ongoing drmP.h cleanup by others I
> expect.
> 
> v3: Rebase on top of atomic bochs.
> 
> Cc: Sam Ravnborg 
> Cc: Jani Nikula 
> Cc: Laurent Pinchart 
> Acked-by: Rodrigo Vivi  (v2)
> Acked-by: Benjamin Gaignard  (v2)
> Signed-off-by: Daniel Vetter 
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: etna...@lists.freedesktop.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-amlo...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: spice-devel@lists.freedesktop.org
> Cc: amd-...@lists.freedesktop.org
> Cc: linux-renesas-...@vger.kernel.org
> Cc: linux-rockc...@lists.infradead.org
> Cc: linux-st...@st-md-mailman.stormreply.com
> Cc: linux-te...@vger.kernel.org
> Cc: xen-de...@lists.xen.org
> ---
> Merging this is going to be a bit a mess due to all the ongoing drmP.h
> cleanups. I think the following should work:
> - Apply Sam's prep patches for removing drmP.h from
>   drm_modeset_helper.h
> - Get the i915 drmP.h cleanup backmerged into drm-misc-next
> - Apply this patch.
> - Apply Sam's patch to remove drmP.h from drm_modeset_helper.h
> - All through drm-misc-next, which has some potential for trivial
>   conflicts around #includes with other drivers unfortunately.
> 
> I hope there's no other driver who'll blow up accidentally because
> someone else is doing a drmP.h cleanup. Laurent maybe?
> 
> Jani, ack on this?
> -Daniel
> ---
>  .../gpu/drm/amd/amdgpu/amdgpu_connectors.c|  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c|  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h  |  1 +
>  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
>  .../amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c  |  2 +-
>  .../display/amdgpu_dm/amdgpu_dm_services.c|  2 +-
>  drivers/gpu/drm/arc/arcpgu_crtc.c |  2 +-
>  drivers/gpu/drm/arc/arcpgu_drv.c  |  2 +-
>  drivers/gpu/drm/arc/arcpgu_sim.c  |  2 +-
>  drivers/gpu/drm/arm/hdlcd_crtc.c  |  2 +-
>  drivers/gpu/drm/arm/hdlcd_drv.c   |  2 +-
>  drivers/gpu/drm/arm/malidp_crtc.c |  2 +-
>  drivers/gpu/drm/arm/malidp_drv.c  |  2 +-
>  drivers/gpu/drm/arm/malidp_mw.c   |  2 +-
>  drivers/gpu/drm/armada/armada_510.c   |  2 +-
>  drivers/gpu/drm/armada/armada_crtc.c  |  2 +-
>  drivers/gpu/drm/armada/armada_crtc.h  |  2 +
>  drivers/gpu/drm/armada/armada_drv.c   |  2 +-
>  drivers/gpu/drm/armada/armada_fb.c|  2 +-
>  drivers/gpu/drm/ast/ast_drv.c |  1 +
>  drivers/gpu/drm/ast/ast_mode.c|  1 +
>  .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c|  2 +-
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h  |  2 +-
>  drivers/gpu/drm/bochs/bochs_drv.c |  1 +
>  drivers/gpu/drm/bochs/bochs_kms.c |  1 +
>  drivers/gpu/drm/bridge/adv7511/adv7511.h  |  5 +-
>  drivers/gpu/drm/bridge/analogix-anx78xx.c |  2 +-
>  .../drm/bridge/analogix/analogix_dp_core.c|  2 +-
>  drivers/gpu/drm/bridge/cdns-dsi.c |  2 +-
>  drivers/gpu/drm/bridge/dumb-vga-dac.c |  2 +-
>  .../bridge/megachips-stdp-ge-b850v3-fw.c  |  2 +-
>  drivers/gpu/drm/bridge/nxp-ptn3460.c  |  2 +-
>  drivers/gpu/drm/bridge/panel.c|  2 +-
>  drivers/gpu/drm/bridge/parade-ps8622.c|  2 +-
>  drivers/gpu/drm/bridge/sii902x.c  |  2 +-
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  2 +-
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c |  2 +-
>  drivers/gpu/drm/bridge/tc358764.c |  2 +-
>  drivers/gpu/drm/bridge/tc358767.c |  2 +-
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c |  2 +-
>  drivers/gpu/drm/bridge/ti-tfp410.c|  2 +-
>  drivers/gpu/drm/cirrus/cirrus_drv.c   |  1 +
>  drivers/gpu/drm/cirrus/cirrus_mode.c  |  1 +
>  drivers/gpu/drm/drm_atomic_helper.c   |  1 -
>  drivers/gpu/drm/drm_dp_mst_topology.c |  2 +-
>  

Re: [Spice-devel] [PATCH spice-common 3/3] docs: add spice URI scheme

[Marc-André Lureau]

Hi

On Tue, Jan 15, 2019 at 4:53 PM Christophe Fergeau  wrote:
>
> Hey,
>
> On Fri, Jan 11, 2019 at 11:07:14PM +0400, marcandre.lur...@redhat.com wrote:
> > @@ -0,0 +1,131 @@
> > +The "spice" URI scheme
> > +==
> > +
> > +This document is inspired by 'The "vnc" URI Scheme' (rfc7869) and
> > +attempts to document a standard Spice URI scheme.
> > +
> > +The normative syntax of the Spice URI is defined in the 
> > +rule in the following syntax specification.  This specification
> > +uses the Augmented Backus-Naur Form (ABNF) as described in
> > +[RFC5234].  The Spice URI conforms to the generic URI syntax
> > +specified in [RFC3986].  The , , ,
> > +, and  rules are defined in [RFC3986].
> > +
> > + spice-uri = spice-scheme "://" [ userinfo "@" ] [ host [ ":" port ] ]
> > +  [ "?" [ spice-params ] ]
> > +
> > + spice-scheme = "spice" / "spice+unix" / "spice+tls"
> > +
> > + spice-params = param "=" value *("&" param "=" value) ["&"]
> > +
> > + param = 1*( param-char )
> > +
> > + value = *( param-char )
> > +
> > + param-char = unreserved / pct-encoded / unreserved-symbols
> > +
> > + unreserved-symbols = ":" / "/" / "@" / "!" / "$" / "'"
> > +   / "(" / ")" / "*" / "," / ";"
> > +
> > +The "?", "=", and "&" characters are used to delimit Spice parameters
> > +and must be percent-encoded when representing a data octet as
> > +specified in [RFC3986].  Within the  portion of a Spice
> > +URI, the  do not have special meaning and need not
> > +be percent-encoded when representing a data octet.
> > +
> > +A Spice URI has the general form:
> > +
> > + spice-scheme://host:port?param1=value1=value2...
>
> I'd mention 'userinfo' in that URI.

The point of this is to show a general form, not a form that has all
the details.

Given that it is discouraged to use userinfo (unless it is done
carefully), I don't think we need to mention it here.

However, there could be more details about it, a line such as:

 userinfo = 

You can also read usage is discouraged there.


>
> > +The  value in the "spice+unix://" URI specify the UNIX domain
> > +socket path of the Spice server on the local host:
> > +
> > +[options="header"]
> > +|===
> > +| Name   | Type   | Description | Default
> > +| host   | string | UNIX domain socket path | none
> > +|===
>
> RFC3986 has a definition for a 'path-absolute', but this is something which is
> concatenated to the 'host'. A 'host' for RFC3986 cannot contain a '/':
>
>host  = IP-literal / IPv4address / reg-name
> with
>reg-name  = *( unreserved / pct-encoded / sub-delims )
>unreserved= ALPHA / DIGIT / "-" / "." / "_" / "~"
>sub-delims= "!" / "$" / "&" / "'" / "(" / ")"
>/ "*" / "+" / "," / ";" / "="
>
> I guess you could rework the current definition with something like:
>
> URI = URI-unix / URI-net
> URI-net = spice-scheme-net://host:port?param1=value1=value2...
> URI-unix = spice+unix://path-absolute
>
> Or decide that for now, the current definition is good enough ;)

I agree it could be improved, but for a v1, I would rather have
external contributions for this kind of refinements :)

Thanks

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



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common 2/3] build-sys: improve asciidoc rules to allow multiple targets

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Signed-off-by: Marc-André Lureau 

Acked-by: Frediano Ziglio 

> ---
>  docs/Makefile.am | 22 +++---
>  docs/meson.build | 12 +++-
>  2 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/docs/Makefile.am b/docs/Makefile.am
> index 421e5f9..e7e126b 100644
> --- a/docs/Makefile.am
> +++ b/docs/Makefile.am
> @@ -1,18 +1,26 @@
>  NULL =
> -ASCIIDOC_FLAGS = -a icons -a toc
> +CLEANFILES =
> +
> +ASCIIDOC_FILES = \
> + spice_protocol.txt  \
> + $(NULL)
> +
> +ASCIIDOC_FLAGS = -n -a icons -a toc
>  
>  EXTRA_DIST = \
> + $(ASCIIDOC_FILES)   \
>   meson.build \
> - spice_protocol.txt  \
>   $(NULL)
>  
>  if BUILD_HTML_MANUAL
> -all-local: spice_protocol.html
> +ASCIIDOC_HTML = $(ASCIIDOC_FILES:.txt=.html)
>  
> -spice_protocol.html: spice_protocol.txt
> - $(AM_V_GEN) $(ASCIIDOC) -n $(ASCIIDOC_FLAGS) -o $@ $<
> -endif
> +CLEANFILES += $(ASCIIDOC_HTML)
>  
> -CLEANFILES = spice_protocol.html
> +all-local: $(ASCIIDOC_HTML)
> +
> +.txt.html:
> + $(AM_V_GEN)$(ASCIIDOC) $(ASCIIDOC_FLAGS) -o $@ $<
> +endif
>  
>  -include $(top_srcdir)/git.mk
> diff --git a/docs/meson.build b/docs/meson.build
> index 5e10d76..8901762 100644
> --- a/docs/meson.build
> +++ b/docs/meson.build
> @@ -1,10 +1,12 @@
>  if get_option('manual')
>asciidoc = find_program('asciidoc', required : false)
>if asciidoc.found()
> -custom_target('spice_protocol.html',
> -  input : files('spice_protocol.txt'),
> -  output : 'spice_protocol.html',
> -  build_by_default : true,
> -  command : [asciidoc, '-n', '-a', 'icons', '-a', 'toc',
> '-o', '@OUTPUT@', '@INPUT@'])
> +foreach f: ['spice_protocol.txt']
> +  custom_target('HTML for @0@'.format(f),
> +input : f,
> +output : '@BASENAME@.html',
> +build_by_default : true,
> +command : [asciidoc, '-n', '-a', 'icons', '-a', 'toc',
> '-o', '@OUTPUT@', '@INPUT@'])
> +endforeach
>endif
>  endif

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

Re: [Spice-devel] [PATCH spice-common 3/3] docs: add spice URI scheme

[Frediano Ziglio]

> 
> On Mon, Jan 14, 2019 at 8:36 PM Frediano Ziglio  wrote:
> >
> > >
> > > From: Marc-André Lureau 
> > >
> > > Signed-off-by: Marc-André Lureau 
> > > ---
> > >  docs/Makefile.am  |   1 +
> > >  docs/meson.build  |   2 +-
> > >  docs/spice_uri_scheme.txt | 131 ++
> > >  3 files changed, 133 insertions(+), 1 deletion(-)
> > >  create mode 100644 docs/spice_uri_scheme.txt
> > >
> > > diff --git a/docs/Makefile.am b/docs/Makefile.am
> > > index e7e126b..6f7a0ac 100644
> > > --- a/docs/Makefile.am
> > > +++ b/docs/Makefile.am
> > > @@ -3,6 +3,7 @@ CLEANFILES =
> > >
> > 
> >
> > > +documentation.
> > > +
> > > +Data Types
> > > +--
> > > +
> > > +Spice URIs can be percent-encoded as specified in [RFC3986] and MUST
> > > +be decoded.  After decoding, the following type constraints and
> > > +semantics apply:
> > > +
> > > +string
> > > +~~
> > > +
> > > +Values of "string" type are UTF-encoded strings as specified in
> >
> > Minor: UTF8, not any UTF.
> 
> ack series with that change?
> 

I didn't read all this document, I saw Christophe did it, I just caught this
minor typo by "accident".

> >
> > > +[RFC3629].
> > > +
> > > +ushort
> > > +~~
> > > +
> > > +The "ushort" values represent unsigned 16-bit integers expressed
> > > +in decimal digits with value between 0-65535 inclusive.
> >

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

Re: [Spice-devel] [PATCH spice-gtk v4 1/2] Fix overlay for vaapisink

[Frediano Ziglio]

> 
> On 1/15/19 12:22 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 
> > ---
> > To test it probably you'll have to remove the gstreamer registry,
> > usually in ~/.cache/gstreamer-1.0/registry.x86_64.bin.
> >
> > Changes since v3:
> > - none, add a patch to fix another issue;
> >
> > 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| 45 +++
> >   5 files changed, 57 insertions(+), 14 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index bba13fba..0f69b3c2 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 acd5dcaf..69bbee57 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 d1de9473..66884a74 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);
> > -}
> >   }
> 
> 
> I think the support gstcontext for vaapi elements was introduced pretty
> lately
> (if i'm not mistaken 1.13.90) so i guess it worth removing this code
> only in higher
> versions
> 

1.13.1. Is there a way to detect the version (better the plugin, not all
GStreamer)? Or a simple check for GStreamer 0.14 would be good?

> 
> >   
> >   g_signal_connect(playbin, "source-setup",
> >   

Re: [Spice-devel] [PATCH spice 4/8 v2] Receive the GraphicsDeviceInfo message from the streaming agent

[Frediano Ziglio]

> 
> Receives the GraphicsDeviceInfo message from the streaming agent and
> stores the data in a list on the streaming device.
> 
> Signed-off-by: Lukáš Hrázký 
> ---
>  server/display-limits.h|  3 ++
>  server/red-qxl.c   |  2 +-
>  server/red-stream-device.c | 67 --
>  server/red-stream-device.h |  8 +
>  server/reds.c  |  1 +
>  5 files changed, 78 insertions(+), 3 deletions(-)
> 
> diff --git a/server/display-limits.h b/server/display-limits.h
> index e875149b..d79d3211 100644
> --- a/server/display-limits.h
> +++ b/server/display-limits.h
> @@ -25,4 +25,7 @@
>  /** Maximum number of streams created by spice-server */
>  #define NUM_STREAMS 50
>  
> +/** Maximum length of the device address string */
> +#define MAX_DEVICE_ADDRESS_LEN 256
> +
>  #endif /* DISPLAY_LIMITS_H_ */
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index a56d9a52..943ccb08 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -37,11 +37,11 @@
>  #include "dispatcher.h"
>  #include "red-parse-qxl.h"
>  #include "red-channel-client.h"
> +#include "display-limits.h"
>  
>  #include "red-qxl.h"
>  
>  
> -#define MAX_DEVICE_ADDRESS_LEN 256
>  #define MAX_MONITORS_COUNT 16
>  
>  struct QXLState {
> diff --git a/server/red-stream-device.c b/server/red-stream-device.c
> index 215ddbe7..20bf115d 100644
> --- a/server/red-stream-device.c
> +++ b/server/red-stream-device.c
> @@ -37,6 +37,7 @@ struct StreamDevice {
>  StreamMsgCapabilities capabilities;
>  StreamMsgCursorSet cursor_set;
>  StreamMsgCursorMove cursor_move;
> +StreamMsgDeviceDisplayInfo device_display_info;
>  uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
>  } *msg;
>  uint32_t msg_pos;
> @@ -49,6 +50,7 @@ struct StreamDevice {
>  CursorChannel *cursor_channel;
>  SpiceTimer *close_timer;
>  uint32_t frame_mmtime;
> +StreamDeviceDisplayInfo device_display_info;
>  };
>  
>  struct StreamDeviceClass {
> @@ -64,8 +66,8 @@ static void char_device_set_state(RedCharDevice *char_dev,
> int state);
>  typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance
>  *sin)
>  SPICE_GNUC_WARN_UNUSED_RESULT;
>  
> -static StreamMsgHandler handle_msg_format, handle_msg_data,
> handle_msg_cursor_set,
> -handle_msg_cursor_move, handle_msg_capabilities;
> +static StreamMsgHandler handle_msg_format, handle_msg_device_display_info,
> handle_msg_data,
> +handle_msg_cursor_set, handle_msg_cursor_move, handle_msg_capabilities;
>  
>  static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance
>  *sin,
> const char *error_msg)
> SPICE_GNUC_WARN_UNUSED_RESULT;
> @@ -146,6 +148,13 @@ stream_device_partial_read(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  handled = handle_msg_format(dev, sin);
>  }
>  break;
> +case STREAM_TYPE_DEVICE_DISPLAY_INFO:
> +if (dev->hdr.size > sizeof(StreamMsgDeviceDisplayInfo) +
> MAX_DEVICE_ADDRESS_LEN) {
> +handled = handle_msg_invalid(dev, sin,
> "StreamMsgDeviceDisplayInfo too large");
> +} else {
> +handled = handle_msg_device_display_info(dev, sin);
> +}
> +break;
>  case STREAM_TYPE_DATA:
>  if (dev->hdr.size > 32*1024*1024) {
>  handled = handle_msg_invalid(dev, sin, "STREAM_DATA too large");
> @@ -271,6 +280,60 @@ handle_msg_format(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  return true;
>  }
>  
> +static bool
> +handle_msg_device_display_info(StreamDevice *dev, SpiceCharDeviceInstance
> *sin)
> +{
> +SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> +
> +if (spice_extra_checks) {
> +spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> +spice_assert(dev->hdr.type == STREAM_TYPE_DEVICE_DISPLAY_INFO);
> +}
> +
> +if (dev->msg_len < dev->hdr.size) {
> +dev->msg = g_realloc(dev->msg, dev->hdr.size);
> +dev->msg_len = dev->hdr.size;
> +}
> +
> +/* read from device */
> +ssize_t n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size -
> dev->msg_pos);
> +if (n <= 0) {
> +return dev->msg_pos == dev->hdr.size;
> +}
> +
> +dev->msg_pos += n;
> +if (dev->msg_pos != dev->hdr.size) { /* some bytes are still missing */
> +return false;
> +}
> +
> +StreamMsgDeviceDisplayInfo *display_info_msg =
> >msg->device_display_info;
> +
> +size_t device_address_len =
> GUINT32_FROM_LE(display_info_msg->device_address_len);
> +if (device_address_len > MAX_DEVICE_ADDRESS_LEN) {
> +g_error("Received a device address longer than %u (%zu), "
> +"will be truncated!", MAX_DEVICE_ADDRESS_LEN,
> device_address_len);

DoS, g_error will abort() Qemu.

> +device_address_len =
> sizeof(dev->device_display_info.device_address);
> +}
> +
> +

Re: [Spice-devel] [PATCH v2] gstreamer: set timestamp in buffer's GstReferenceTimestampMeta

[Frediano Ziglio]

> 
> Currently we set timestamps as buffer's PTS, this value may be changed by
> the pipeline in some cases and cause an unexpected buffer warnings (when
> GstVideoOverlay is not used). Use GstReferenceTimestampMeta when
> synchronization is made by spice.
> 
> Before applying this patch you can reproduce the warnings by runing with
> DISABLE_GSTVIDEOOVERLAY=1 and starting some audio playback in the guest.
> 
> Signed-off-by: Snir Sheriber 
> ---
> Changes from v1:
> -Commit message
> ---
>  src/channel-display-gst.c | 27 ---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 2f556fe..a90fa81 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -33,6 +33,10 @@ typedef struct SpiceGstFrame SpiceGstFrame;
>  
>  /* GStreamer decoder implementation */
>  
> +#if GST_CHECK_VERSION(1,14,0)
> +static GstStaticCaps stream_reference =
> GST_STATIC_CAPS("timestamp/spice-stream");
> +#endif
> +
>  typedef struct SpiceGstDecoder {
>  VideoDecoder base;
>  
> @@ -86,7 +90,14 @@ struct SpiceGstFrame {
>  static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceFrame *frame)
>  {
>  SpiceGstFrame *gstframe = g_new(SpiceGstFrame, 1);
> +#if GST_CHECK_VERSION(1,14,0)
> +GstReferenceTimestampMeta *time_meta;
> +
> +time_meta = gst_buffer_get_reference_timestamp_meta(buffer,
> gst_static_caps_get(_reference));
> +gstframe->timestamp = time_meta->timestamp;
> +#else
>  gstframe->timestamp = GST_BUFFER_PTS(buffer);
> +#endif
>  gstframe->frame = frame;
>  gstframe->sample = NULL;
>  return gstframe;
> @@ -211,6 +222,12 @@ static void fetch_pending_sample(SpiceGstDecoder
> *decoder)
>  decoder->pending_samples--;
>  
>  GstBuffer *buffer = gst_sample_get_buffer(sample);
> +GstClockTime buffer_ts;
> +#if GST_CHECK_VERSION(1,14,0)
> +buffer_ts = gst_buffer_get_reference_timestamp_meta(buffer,
> gst_static_caps_get(_reference))->timestamp;

This line crashes for me, gst_buffer_get_reference_timestamp_meta returns
NULL. It does not surprise me, you attach metadata to an input buffer
and you expect these metadata to be attached to output buffers.
Apparently this is not guaranteed.

> +#else
> +buffer_ts = GST_BUFFER_PTS(buffer);
> +#endif
>  
>  /* gst_app_sink_pull_sample() sometimes returns the same buffer
>  twice
>   * or buffers that have a modified, and thus unrecognizable, PTS.
> @@ -223,7 +240,7 @@ static void fetch_pending_sample(SpiceGstDecoder
> *decoder)
>  GList *l = g_queue_peek_head_link(decoder->decoding_queue);
>  while (l) {
>  gstframe = l->data;
> -if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
> +if (gstframe->timestamp == buffer_ts) {
>  /* The frame is now ready for display */
>  gstframe->sample = sample;
>  decoder->display_frame = gstframe;
> @@ -232,7 +249,7 @@ static void fetch_pending_sample(SpiceGstDecoder
> *decoder)
>   * frames from the decoding queue.
>   */
>  while ((gstframe =
>  g_queue_pop_head(decoder->decoding_queue))) {
> -if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
> +if (gstframe->timestamp == buffer_ts) {
>  break;
>  }
>  /* The GStreamer pipeline dropped the corresponding
> @@ -626,9 +643,13 @@ static gboolean
> spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>  frame->data,
>  frame->size, 0,
>  frame->size,
>  frame->data_opaque,
>  frame->unref_data);
>  
> +GstClockTime pts = gst_clock_get_time(decoder->clock) -
> gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0, latency)) *
> 1000 * 1000;
>  GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
>  GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
> -GST_BUFFER_PTS(buffer) = gst_clock_get_time(decoder->clock) -
> gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0, latency)) *
> 1000 * 1000;
> +GST_BUFFER_PTS(buffer) = pts;
> +#if GST_CHECK_VERSION(1,14,0)
> +gst_buffer_add_reference_timestamp_meta(buffer, gst_static_caps_get
> (_reference), pts, GST_CLOCK_TIME_NONE);
> +#endif
>  
>  if (decoder->appsink != NULL) {
>  SpiceGstFrame *gst_frame = create_gst_frame(buffer, frame);

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

Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h

[Jani Nikula]

On Tue, 15 Jan 2019, Daniel Vetter  wrote:
> Having the probe helper stuff (which pretty much everyone needs) in
> the drm_crtc_helper.h file (which atomic drivers should never need) is
> confusing. Split them out.
>
> To make sure I actually achieved the goal here I went through all
> drivers. And indeed, all atomic drivers are now free of
> drm_crtc_helper.h includes.
>
> v2: Make it compile. There was so much compile fail on arm drivers
> that I figured I'll better not include any of the acks on v1.
>
> v3: Massive rebase because i915 has lost a lot of drmP.h includes, but
> not all: Through drm_crtc_helper.h > drm_modeset_helper.h -> drmP.h
> there was still one, which this patch largely removes. Which means
> rolling out lots more includes all over.
>
> This will also conflict with ongoing drmP.h cleanup by others I
> expect.
>
> v3: Rebase on top of atomic bochs.
>
> Cc: Sam Ravnborg 
> Cc: Jani Nikula 
> Cc: Laurent Pinchart 
> Acked-by: Rodrigo Vivi  (v2)
> Acked-by: Benjamin Gaignard  (v2)
> Signed-off-by: Daniel Vetter 
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: etna...@lists.freedesktop.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-amlo...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: spice-devel@lists.freedesktop.org
> Cc: amd-...@lists.freedesktop.org
> Cc: linux-renesas-...@vger.kernel.org
> Cc: linux-rockc...@lists.infradead.org
> Cc: linux-st...@st-md-mailman.stormreply.com
> Cc: linux-te...@vger.kernel.org
> Cc: xen-de...@lists.xen.org
> ---
> Merging this is going to be a bit a mess due to all the ongoing drmP.h
> cleanups. I think the following should work:
> - Apply Sam's prep patches for removing drmP.h from
>   drm_modeset_helper.h
> - Get the i915 drmP.h cleanup backmerged into drm-misc-next
> - Apply this patch.
> - Apply Sam's patch to remove drmP.h from drm_modeset_helper.h
> - All through drm-misc-next, which has some potential for trivial
>   conflicts around #includes with other drivers unfortunately.
>
> I hope there's no other driver who'll blow up accidentally because
> someone else is doing a drmP.h cleanup. Laurent maybe?
>
> Jani, ack on this?

Acked-by: Jani Nikula 



-- 
Jani Nikula, Intel Open Source Graphics Center
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2] gstreamer: set timestamp in buffer's GstReferenceTimestampMeta

[Frediano Ziglio]

> 
> Hi,
> 
> On 1/15/19 3:42 PM, Frediano Ziglio wrote:
> >> Currently we set timestamps as buffer's PTS, this value may be changed by
> >> the pipeline in some cases and cause an unexpected buffer warnings (when
> >> GstVideoOverlay is not used). Use GstReferenceTimestampMeta when
> >> synchronization is made by spice.
> >>
> >> Before applying this patch you can reproduce the warnings by runing with
> >> DISABLE_GSTVIDEOOVERLAY=1 and starting some audio playback in the guest.
> >>
> >> Signed-off-by: Snir Sheriber 
> >> ---
> >> Changes from v1:
> >> -Commit message
> >> ---
> >>   src/channel-display-gst.c | 27 ---
> >>   1 file changed, 24 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> >> index 2f556fe..a90fa81 100644
> >> --- a/src/channel-display-gst.c
> >> +++ b/src/channel-display-gst.c
> >> @@ -33,6 +33,10 @@ typedef struct SpiceGstFrame SpiceGstFrame;
> >>   
> >>   /* GStreamer decoder implementation */
> >>   
> >> +#if GST_CHECK_VERSION(1,14,0)
> >> +static GstStaticCaps stream_reference =
> >> GST_STATIC_CAPS("timestamp/spice-stream");
> >> +#endif
> >> +
> >>   typedef struct SpiceGstDecoder {
> >>   VideoDecoder base;
> >>   
> >> @@ -86,7 +90,14 @@ struct SpiceGstFrame {
> >>   static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceFrame
> >>   *frame)
> >>   {
> >>   SpiceGstFrame *gstframe = g_new(SpiceGstFrame, 1);
> >> +#if GST_CHECK_VERSION(1,14,0)
> >> +GstReferenceTimestampMeta *time_meta;
> >> +
> >> +time_meta = gst_buffer_get_reference_timestamp_meta(buffer,
> >> gst_static_caps_get(_reference));
> >> +gstframe->timestamp = time_meta->timestamp;
> >> +#else
> >>   gstframe->timestamp = GST_BUFFER_PTS(buffer);
> >> +#endif
> >>   gstframe->frame = frame;
> >>   gstframe->sample = NULL;
> >>   return gstframe;
> >> @@ -211,6 +222,12 @@ static void fetch_pending_sample(SpiceGstDecoder
> >> *decoder)
> >>   decoder->pending_samples--;
> >>   
> >>   GstBuffer *buffer = gst_sample_get_buffer(sample);
> >> +GstClockTime buffer_ts;
> >> +#if GST_CHECK_VERSION(1,14,0)
> >> +buffer_ts = gst_buffer_get_reference_timestamp_meta(buffer,
> >> gst_static_caps_get(_reference))->timestamp;
> > This line crashes for me, gst_buffer_get_reference_timestamp_meta returns
> > NULL. It does not surprise me, you attach metadata to an input buffer
> > and you expect these metadata to be attached to output buffers.
> > Apparently this is not guaranteed.
> 
> 
> IIUC It should:
> 
> "A GstBuffer is the object that is passed from an upstream element to a
> downstream element and contains memory and metadata information."
> 

However does not specify that no additional GstBuffers are created.
Also does not mean that the next downstream element can use and create
another to pass to next in the chain.

> Actually this is pretty similar to what we are currently doing- counting
> on the PTS (which is may be
> changed by the elements in oppose to GstReferenceTimestampMeta which
> should be used as an
> alternative timestamp to PTS which is not being touched)
> 

Is similar but reading the PTS from the buffer cannot cause a NULL
pointer dereference, the field is always there, maybe changed.

> 
> Anyway this could be fixed with a fallback to PTS which i set in any
> case but i want to understand if
> there's some bug here, does it always happen to you? can you reproduce
> it consistently?
> 

I would say 95%, but even 1% crash would be something we should fix.
I use a script to replay videos and with that is really consistent.

> Snir.
> 
> 
> >
> >> +#else
> >> +buffer_ts = GST_BUFFER_PTS(buffer);
> >> +#endif
> >>   
> >>   /* gst_app_sink_pull_sample() sometimes returns the same buffer
> >>   twice
> >>* or buffers that have a modified, and thus unrecognizable,
> >>PTS.
> >> @@ -223,7 +240,7 @@ static void fetch_pending_sample(SpiceGstDecoder
> >> *decoder)
> >>   GList *l = g_queue_peek_head_link(decoder->decoding_queue);
> >>   while (l) {
> >>   gstframe = l->data;
> >> -if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
> >> +if (gstframe->timestamp == buffer_ts) {
> >>   /* The frame is now ready for display */
> >>   gstframe->sample = sample;
> >>   decoder->display_frame = gstframe;
> >> @@ -232,7 +249,7 @@ static void fetch_pending_sample(SpiceGstDecoder
> >> *decoder)
> >>* frames from the decoding queue.
> >>*/
> >>   while ((gstframe =
> >>   g_queue_pop_head(decoder->decoding_queue))) {
> >> -if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
> >> +if (gstframe->timestamp == buffer_ts) {
> >>   break;
> >>   }
> >>  

[Spice-devel] [spice-gtk [rfc] 0/2] Clipboard managers and Spice

[Victor Toso]

From: Victor Toso 

Hi,

Several iteractions trying to avoid some bug in X11 but in the end I
found the iteraction with Clibpoard managers (or any other application
that request/set clipboard data) a bit more urgent.

Simple try here, to not allow another application to request clipboard
data from guest while the user is theoretically interacting with that
guest machine as spice client holds the keyboard-grab.

As pointed out by elmarco [0], that might be something desireable. So,
I'm introducing the possibility to enable it but have it disabled by
default.

Tested on X11 and Wayland clients.

There are more than on away to achieve this idea so feedback is welcome.

I expect that the spice client would implement some sort to commandline
option like --allow-clipobard-managers to enable/disable the
SpiceGtkSession property on the fly. For now, there is an option in
spicy testing tool.

James, would be great if you could verify if this series keep your
environment bug free.

Cheers,

Victor Toso (2):
  gtk-session: introduce clipboard-managers property
  gtk-session: add request targets delayed

 src/spice-gtk-session.c | 128 +---
 tools/spicy.c   |   6 ++
 2 files changed, 125 insertions(+), 9 deletions(-)

-- 
2.20.1

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

[Spice-devel] [spice-gtk [rfc] 1/2] gtk-session: introduce clipboard-managers property

[Victor Toso]

From: Victor Toso 

SpiceGtkSession::allow-clipboard-managers property is introduced to
enable other applications in the Client OS to set or fetch clipboard
data from a spice-gtk-session that is under keyboard-grab, which is
expected to be in use by the user.

This option is disabled by default as to avoid:
1) Another application in the client to fetch clipboard data while the
   user is interacting with the remote machine;
2) Another application to set clipboard data *and* send that to the
   remote machine while the user is *not* interacting with the remote
   machine (without keyboard grab)

This patch disables (1) and following patch disables (2) if
allow-clipboard-managers is disabled which is set to be default.

This patch also fixes some indentation found in spice-gtk-session.c

Signed-off-by: Victor Toso 
---
 src/spice-gtk-session.c | 51 +
 tools/spicy.c   |  6 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index adc72a2..7391e6a 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -59,6 +59,7 @@ struct _SpiceGtkSessionPrivate {
 gbooleanclip_hasdata[CLIPBOARD_LAST];
 gbooleanclip_grabbed[CLIPBOARD_LAST];
 gbooleanclipboard_by_guest[CLIPBOARD_LAST];
+gbooleanback_from_focus_out;
 /* auto-usbredir related */
 gbooleanauto_usbredir_enable;
 int auto_usbredir_reqs;
@@ -66,6 +67,7 @@ struct _SpiceGtkSessionPrivate {
 gbooleankeyboard_has_focus;
 gbooleanmouse_has_pointer;
 gbooleansync_modifiers;
+gbooleanallow_clipboard_managers;
 };
 
 /**
@@ -114,6 +116,7 @@ enum {
 PROP_AUTO_USBREDIR,
 PROP_POINTER_GRABBED,
 PROP_SYNC_MODIFIERS,
+PROP_ALLOW_CLIPBOARD_MANAGER,
 };
 
 static guint32 get_keyboard_lock_modifiers(void)
@@ -287,6 +290,9 @@ static void spice_gtk_session_get_property(GObject
*gobject,
 case PROP_SYNC_MODIFIERS:
 g_value_set_boolean(value, s->sync_modifiers);
 break;
+case PROP_ALLOW_CLIPBOARD_MANAGER:
+g_value_set_boolean(value, s->allow_clipboard_managers);
+break;
 default:
G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
break;
@@ -337,6 +343,9 @@ static void spice_gtk_session_set_property(GObject  
*gobject,
 case PROP_SYNC_MODIFIERS:
 s->sync_modifiers = g_value_get_boolean(value);
 break;
+case PROP_ALLOW_CLIPBOARD_MANAGER:
+s->allow_clipboard_managers = g_value_get_boolean(value);
+break;
 default:
 G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
 break;
@@ -441,6 +450,23 @@ static void 
spice_gtk_session_class_init(SpiceGtkSessionClass *klass)
   G_PARAM_READWRITE |
   G_PARAM_CONSTRUCT |
   G_PARAM_STATIC_STRINGS));
+/**
+ * SpiceGtkSession:allow-clipboard-managers
+ *
+ * Allow clipboard managers to set and get clipboard data. With this option
+ * disabled, only user direct actions should change the clipboard data.
+ *
+ * Since: 0.36
+ **/
+g_object_class_install_property
+(gobject_class, PROP_ALLOW_CLIPBOARD_MANAGER,
+ g_param_spec_boolean("allow-clipboard-managers",
+  "Allow clipboard managers",
+  "Allow clipboard managers to interact with 
Clipboard",
+  FALSE,
+  G_PARAM_READWRITE |
+  G_PARAM_CONSTRUCT |
+  G_PARAM_STATIC_STRINGS));
 }
 
 /*  */
@@ -745,7 +771,15 @@ static void clipboard_get(GtkClipboard *clipboard,
 gulong agent_handler;
 int selection;
 
-SPICE_DEBUG("clipboard get");
+if (!s->allow_clipboard_managers &&
+spice_gtk_session_get_keyboard_has_focus(self)) {
+SPICE_DEBUG("clipboard get: clipboard-manager=no, ignore request");
+return;
+}
+
+SPICE_DEBUG("clipboard get: clipboard-manager=%s, keyboard-grab=%s",
+spice_yes_no(s->allow_clipboard_managers),
+spice_yes_no(spice_gtk_session_get_keyboard_has_focus(self)));
 
 selection = get_selection_from_clipboard(s, clipboard);
 g_return_if_fail(selection != -1);
@@ -1277,16 +1311,25 @@ gboolean 
spice_gtk_session_get_pointer_grabbed(SpiceGtkSession *self)
 
 G_GNUC_INTERNAL
 void spice_gtk_session_set_keyboard_has_focus(SpiceGtkSession *self,
-gboolean keyboard_has_focus)
+  gboolean keyboard_has_focus)
 {
+SpiceGtkSessionPrivate *s;
+
 

[Spice-devel] [spice-gtk [rfc] 2/2] gtk-session: add request targets delayed

[Victor Toso]

From: Victor Toso 

If SpiceGtkSession::allow-clipboard-managers is disabled, while the
spice client does not hold the keyboard-grab, every owner-change event
to clipboard change will be delayed and requested later when
keyboard-grab is received.

Signed-off-by: Victor Toso 
---
 src/spice-gtk-session.c | 77 ++---
 1 file changed, 72 insertions(+), 5 deletions(-)

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 7391e6a..384d090 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -56,6 +56,7 @@ struct _SpiceGtkSessionPrivate {
 GtkClipboard*clipboard_primary;
 GtkTargetEntry  *clip_targets[CLIPBOARD_LAST];
 guint   nclip_targets[CLIPBOARD_LAST];
+gbooleanrequest_delayed_ptr[CLIPBOARD_LAST];
 gbooleanclip_hasdata[CLIPBOARD_LAST];
 gbooleanclip_grabbed[CLIPBOARD_LAST];
 gbooleanclipboard_by_guest[CLIPBOARD_LAST];
@@ -259,6 +260,7 @@ static void spice_gtk_session_finalize(GObject *gobject)
 /* release stuff */
 for (i = 0; i < CLIPBOARD_LAST; ++i) {
 g_clear_pointer(>clip_targets[i], g_free);
+s->request_delayed_ptr[i] = FALSE;
 }
 
 /* Chain up to the parent class */
@@ -588,6 +590,9 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
 int a;
 int selection;
 
+/* Disable owner-event requests now if allow_clipboard_managers is 
disabled */
+s->back_from_focus_out = FALSE;
+
 if (s->main == NULL)
 return;
 
@@ -644,6 +649,56 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
 s->nclip_targets[selection] = 0;
 }
 
+static bool clipboard_client_request_targets(SpiceGtkSession *self,
+ GtkClipboard *clipboard)
+{
+SpiceGtkSessionPrivate *s = self->priv;
+gint selection = get_selection_from_clipboard(s, clipboard);
+
+SPICE_DEBUG("clipboard request: %p %d", clipboard, selection);
+s->clipboard_by_guest[selection] = FALSE;
+s->clip_hasdata[selection] = TRUE;
+if (s->auto_clipboard_enable && !read_only(self)) {
+gtk_clipboard_request_targets(clipboard, clipboard_get_targets,
+  get_weak_ref(self));
+return true;
+}
+return false;
+}
+
+static bool clipboard_owner_change_delayed(SpiceGtkSession *self, gint 
selection)
+{
+SpiceGtkSessionPrivate *s = self->priv;
+GtkClipboard *clipboard = get_clipboard_from_selection(s, selection);
+
+g_return_val_if_fail(clipboard != NULL, false);
+
+if (s->clip_grabbed[selection]) {
+SPICE_DEBUG("clipboard delayed request: already made for %p %d",
+clipboard, selection);
+}
+
+s->request_delayed_ptr[selection] = FALSE;
+return clipboard_client_request_targets(self, clipboard);
+}
+
+static bool request_delayed_run_all(SpiceGtkSession *self)
+{
+gint i;
+bool ret = false;
+SpiceGtkSessionPrivate *s = self->priv;
+
+for (i = 0; i < CLIPBOARD_LAST; ++i) {
+if (s->request_delayed_ptr[i]) {
+if (clipboard_owner_change_delayed(self, i)) {
+ret = true;
+}
+}
+}
+return ret;
+}
+
+
 /* Callback for every owner-change event for given @clipboard.
  * This event is triggered in different ways depending on the environment of
  * the Client, some examples:
@@ -700,11 +755,17 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,
 return;
 }
 
-s->clipboard_by_guest[selection] = FALSE;
-s->clip_hasdata[selection] = TRUE;
-if (s->auto_clipboard_enable && !read_only(self))
-gtk_clipboard_request_targets(clipboard, clipboard_get_targets,
-  get_weak_ref(self));
+if (!s->allow_clipboard_managers && !s->back_from_focus_out) {
+if (!s->request_delayed_ptr[selection]) {
+SPICE_DEBUG("clipboard: owner-change: wait focus-in for 
clipboard-request %p %d",
+clipboard, selection);
+}
+s->request_delayed_ptr[selection] = TRUE;
+return;
+}
+
+s->request_delayed_ptr[selection] = FALSE;
+clipboard_client_request_targets(self, clipboard);
 }
 
 typedef struct
@@ -1324,6 +1385,12 @@ void 
spice_gtk_session_set_keyboard_has_focus(SpiceGtkSession *self,
 s->back_from_focus_out = FALSE;
 }
 
+/* Clipboard requests made while on focus-out may be done now, if any is
+ * successfull, we can reset back_from_focus_out helper */
+if (s->back_from_focus_out && request_delayed_run_all(self)) {
+s->back_from_focus_out = FALSE;
+}
+
 s->keyboard_has_focus = keyboard_has_focus;
 }
 
-- 
2.20.1

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

Re: [Spice-devel] [spice-gtk [rfc] 0/2] Clipboard managers and Spice

[Victor Toso]

Hi,

On Tue, Jan 15, 2019 at 05:11:36PM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> Hi,
> 
> Several iteractions trying to avoid some bug in X11 but in the end I
> found the iteraction with Clibpoard managers (or any other application
> that request/set clipboard data) a bit more urgent.
> 
> Simple try here, to not allow another application to request clipboard
> data from guest while the user is theoretically interacting with that
> guest machine as spice client holds the keyboard-grab.

And to not send clipboard-data to the guest while the user is
not interacting with the guest.

> As pointed out by elmarco [0], that might be something desireable. So,
> I'm introducing the possibility to enable it but have it disabled by
> default.

[0] https://lists.freedesktop.org/archives/spice-devel/2019-January/047221.html

> Tested on X11 and Wayland clients.
> 
> There are more than on away to achieve this idea so feedback is welcome.
> 
> I expect that the spice client would implement some sort to commandline
> option like --allow-clipobard-managers to enable/disable the
> SpiceGtkSession property on the fly. For now, there is an option in
> spicy testing tool.
> 
> James, would be great if you could verify if this series keep your
> environment bug free.
> 
> Cheers,
> 
> Victor Toso (2):
>   gtk-session: introduce clipboard-managers property
>   gtk-session: add request targets delayed
> 
>  src/spice-gtk-session.c | 128 +---
>  tools/spicy.c   |   6 ++
>  2 files changed, 125 insertions(+), 9 deletions(-)
> 
> -- 
> 2.20.1
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common 3/3] docs: add spice URI scheme

[Christophe Fergeau]

On Tue, Jan 15, 2019 at 06:00:09PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jan 15, 2019 at 4:53 PM Christophe Fergeau  
> wrote:
> >
> > Hey,
> >
> > On Fri, Jan 11, 2019 at 11:07:14PM +0400, marcandre.lur...@redhat.com wrote:
> > > @@ -0,0 +1,131 @@
> > > +The "spice" URI scheme
> > > +==
> > > +
> > > +This document is inspired by 'The "vnc" URI Scheme' (rfc7869) and
> > > +attempts to document a standard Spice URI scheme.
> > > +
> > > +The normative syntax of the Spice URI is defined in the 
> > > +rule in the following syntax specification.  This specification
> > > +uses the Augmented Backus-Naur Form (ABNF) as described in
> > > +[RFC5234].  The Spice URI conforms to the generic URI syntax
> > > +specified in [RFC3986].  The , , ,
> > > +, and  rules are defined in [RFC3986].
> > > +
> > > + spice-uri = spice-scheme "://" [ userinfo "@" ] [ host [ ":" port ] ]
> > > +  [ "?" [ spice-params ] ]
> > > +
> > > + spice-scheme = "spice" / "spice+unix" / "spice+tls"
> > > +
> > > + spice-params = param "=" value *("&" param "=" value) ["&"]
> > > +
> > > + param = 1*( param-char )
> > > +
> > > + value = *( param-char )
> > > +
> > > + param-char = unreserved / pct-encoded / unreserved-symbols
> > > +
> > > + unreserved-symbols = ":" / "/" / "@" / "!" / "$" / "'"
> > > +   / "(" / ")" / "*" / "," / ";"
> > > +
> > > +The "?", "=", and "&" characters are used to delimit Spice parameters
> > > +and must be percent-encoded when representing a data octet as
> > > +specified in [RFC3986].  Within the  portion of a Spice
> > > +URI, the  do not have special meaning and need not
> > > +be percent-encoded when representing a data octet.
> > > +
> > > +A Spice URI has the general form:
> > > +
> > > + spice-scheme://host:port?param1=value1=value2...
> >
> > I'd mention 'userinfo' in that URI.
> 
> The point of this is to show a general form, not a form that has all
> the details.
> 
> Given that it is discouraged to use userinfo (unless it is done
> carefully), I don't think we need to mention it here.
> 
> However, there could be more details about it, a line such as:
> 
>  userinfo = 

Ok.
> 
> 
> >
> > > +The  value in the "spice+unix://" URI specify the UNIX domain
> > > +socket path of the Spice server on the local host:
> > > +
> > > +[options="header"]
> > > +|===
> > > +| Name   | Type   | Description | Default
> > > +| host   | string | UNIX domain socket path | none
> > > +|===
> >
> > RFC3986 has a definition for a 'path-absolute', but this is something which 
> > is
> > concatenated to the 'host'. A 'host' for RFC3986 cannot contain a '/':
> >
> >host  = IP-literal / IPv4address / reg-name
> > with
> >reg-name  = *( unreserved / pct-encoded / sub-delims )
> >unreserved= ALPHA / DIGIT / "-" / "." / "_" / "~"
> >sub-delims= "!" / "$" / "&" / "'" / "(" / ")"
> >/ "*" / "+" / "," / ";" / "="
> >
> > I guess you could rework the current definition with something like:
> >
> > URI = URI-unix / URI-net
> > URI-net = spice-scheme-net://host:port?param1=value1=value2...
> > URI-unix = spice+unix://path-absolute
> >
> > Or decide that for now, the current definition is good enough ;)
> 
> I agree it could be improved, but for a v1, I would rather have
> external contributions for this kind of refinements :)

Ok.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH vd_agent_linux 3/7] file-xfers: Allocate file_path after disk space check

[Frediano Ziglio]

Variable is not used. Keep code to create file together.

Signed-off-by: Frediano Ziglio 
---
 src/vdagent/file-xfers.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
index 67bad9e..77029e7 100644
--- a/src/vdagent/file-xfers.c
+++ b/src/vdagent/file-xfers.c
@@ -202,8 +202,6 @@ void vdagent_file_xfers_start(struct vdagent_file_xfers 
*xfers,
 
 task->debug = xfers->debug;
 
-file_path = g_build_filename(xfers->save_dir, task->file_name, NULL);
-
 free_space = get_free_space_available(xfers->save_dir);
 if (task->file_size > free_space) {
 gchar *free_space_str, *file_size_str;
@@ -223,6 +221,7 @@ void vdagent_file_xfers_start(struct vdagent_file_xfers 
*xfers,
 goto cleanup;
 }
 
+file_path = g_build_filename(xfers->save_dir, task->file_name, NULL);
 dir = g_path_get_dirname(file_path);
 if (g_mkdir_with_parents(dir, S_IRWXU) == -1) {
 syslog(LOG_ERR, "file-xfer: Failed to create dir %s", dir);
-- 
2.20.1

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

[Spice-devel] [PATCH vd_agent_linux 2/7] file-xfers: Reuse cleanup code

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 src/vdagent/file-xfers.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
index 78d5db3..67bad9e 100644
--- a/src/vdagent/file-xfers.c
+++ b/src/vdagent/file-xfers.c
@@ -220,10 +220,7 @@ void vdagent_file_xfers_start(struct vdagent_file_xfers 
*xfers,
 VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE,
 (uint8_t *)_space,
 sizeof(free_space));
-vdagent_file_xfer_task_free(task);
-g_free(file_path);
-g_free(dir);
-return;
+goto cleanup;
 }
 
 dir = g_path_get_dirname(file_path);
@@ -276,6 +273,7 @@ void vdagent_file_xfers_start(struct vdagent_file_xfers 
*xfers,
 error:
 udscs_write(xfers->vdagentd, VDAGENTD_FILE_XFER_STATUS,
 msg->id, VD_AGENT_FILE_XFER_STATUS_ERROR, NULL, 0);
+cleanup:
 if (task)
 vdagent_file_xfer_task_free(task);
 g_free(file_path);
-- 
2.20.1

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

[Spice-devel] [PATCH vd_agent_linux 4/7] Factor out function to create file

[Frediano Ziglio]

Make easier to test file creation

Signed-off-by: Frediano Ziglio 
---
 src/vdagent/file-xfers.c | 88 
 src/vdagent/file-xfers.h |  1 +
 2 files changed, 53 insertions(+), 36 deletions(-)

diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
index 77029e7..941524a 100644
--- a/src/vdagent/file-xfers.c
+++ b/src/vdagent/file-xfers.c
@@ -178,13 +178,58 @@ static uint64_t get_free_space_available(const char *path)
 return stat.f_bsize * stat.f_bavail;
 }
 
+int
+vdagent_file_xfers_create_file(const char *save_dir, char **file_name_p)
+{
+char *file_path = NULL;
+char *dir = NULL;
+char *path = NULL;
+int file_fd = -1;
+int i;
+struct stat st;
+
+file_path = g_build_filename(save_dir, *file_name_p, NULL);
+dir = g_path_get_dirname(file_path);
+if (g_mkdir_with_parents(dir, S_IRWXU) == -1) {
+syslog(LOG_ERR, "file-xfer: Failed to create dir %s", dir);
+goto error;
+}
+
+path = g_strdup(file_path);
+for (i = 0; i < 64 && (stat(path, ) == 0 || errno != ENOENT); i++) {
+g_free(path);
+char *extension = strrchr(file_path, '.');
+int basename_len = extension != NULL ? extension - file_path : 
strlen(file_path);
+path = g_strdup_printf("%.*s (%i)%s", basename_len, file_path,
+   i + 1, extension ? extension : "");
+}
+g_free(*file_name_p);
+*file_name_p = path;
+path = NULL;
+if (i == 64) {
+syslog(LOG_ERR, "file-xfer: more than 63 copies of %s exist?",
+   file_path);
+goto error;
+}
+
+file_fd = open(*file_name_p, O_CREAT | O_WRONLY, 0644);
+if (file_fd == -1) {
+syslog(LOG_ERR, "file-xfer: failed to create file %s: %s",
+   path, strerror(errno));
+goto error;
+}
+
+error:
+g_free(path);
+g_free(file_path);
+g_free(dir);
+return file_fd;
+}
+
 void vdagent_file_xfers_start(struct vdagent_file_xfers *xfers,
 VDAgentFileXferStartMessage *msg)
 {
 AgentFileXferTask *task;
-char *dir = NULL, *path = NULL, *file_path = NULL;
-struct stat st;
-int i;
 uint64_t free_space;
 
 g_return_if_fail(xfers != NULL);
@@ -221,39 +266,14 @@ void vdagent_file_xfers_start(struct vdagent_file_xfers 
*xfers,
 goto cleanup;
 }
 
-file_path = g_build_filename(xfers->save_dir, task->file_name, NULL);
-dir = g_path_get_dirname(file_path);
-if (g_mkdir_with_parents(dir, S_IRWXU) == -1) {
-syslog(LOG_ERR, "file-xfer: Failed to create dir %s", dir);
-goto error;
-}
-
-path = g_strdup(file_path);
-for (i = 0; i < 64 && (stat(path, ) == 0 || errno != ENOENT); i++) {
-g_free(path);
-char *extension = strrchr(file_path, '.');
-int basename_len = extension != NULL ? extension - file_path : 
strlen(file_path);
-path = g_strdup_printf("%.*s (%i)%s", basename_len, file_path,
-   i + 1, extension ? extension : "");
-}
-g_free(task->file_name);
-task->file_name = path;
-if (i == 64) {
-syslog(LOG_ERR, "file-xfer: more than 63 copies of %s exist?",
-   file_path);
-goto error;
-}
-
-task->file_fd = open(path, O_CREAT | O_WRONLY, 0644);
-if (task->file_fd == -1) {
-syslog(LOG_ERR, "file-xfer: failed to create file %s: %s",
-   path, strerror(errno));
+task->file_fd = vdagent_file_xfers_create_file(xfers->save_dir, 
>file_name);
+if (task->file_fd < 0) {
 goto error;
 }
 
 if (ftruncate(task->file_fd, task->file_size) < 0) {
 syslog(LOG_ERR, "file-xfer: err reserving %"PRIu64" bytes for %s: %s",
-   task->file_size, path, strerror(errno));
+   task->file_size, task->file_name, strerror(errno));
 goto error;
 }
 
@@ -261,12 +281,10 @@ void vdagent_file_xfers_start(struct vdagent_file_xfers 
*xfers,
 
 if (xfers->debug)
 syslog(LOG_DEBUG, "file-xfer: Adding task %u %s %"PRIu64" bytes",
-   task->id, path, task->file_size);
+   task->id, task->file_name, task->file_size);
 
 udscs_write(xfers->vdagentd, VDAGENTD_FILE_XFER_STATUS,
 msg->id, VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA, NULL, 0);
-g_free(file_path);
-g_free(dir);
 return ;
 
 error:
@@ -275,8 +293,6 @@ error:
 cleanup:
 if (task)
 vdagent_file_xfer_task_free(task);
-g_free(file_path);
-g_free(dir);
 }
 
 void vdagent_file_xfers_status(struct vdagent_file_xfers *xfers,
diff --git a/src/vdagent/file-xfers.h b/src/vdagent/file-xfers.h
index 3e2ed1d..29a7717 100644
--- a/src/vdagent/file-xfers.h
+++ b/src/vdagent/file-xfers.h
@@ -39,5 +39,6 @@ void vdagent_file_xfers_data(struct vdagent_file_xfers *xfers,
 VDAgentFileXferDataMessage *msg);
 void vdagent_file_xfers_error_disabled(struct udscs_connection *vdagentd,
 uint32_t msg_id);
+int 

[Spice-devel] [PATCH vd_agent_linux 7/7] file-xfer: Do not strip filename if file part has no extension

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 src/tests/test-file-xfers.c | 6 ++
 src/vdagent/file-xfers.c| 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/tests/test-file-xfers.c b/src/tests/test-file-xfers.c
index 494adc9..e87efac 100644
--- a/src/tests/test-file-xfers.c
+++ b/src/tests/test-file-xfers.c
@@ -67,6 +67,12 @@ int main(int argc, char *argv[])
 // try to create a file with a path where there's a file (should fail)
 test_file("test.txt/out", NULL);
 
+// create a file without extension in a directory with extension
+test_file("sub.dir/test", "./test-dir/sub.dir/test");
+
+// create a file with same name above, should not strip the filename
+test_file("sub.dir/test", "./test-dir/sub.dir/test (1)");
+
 assert(system("rm -rf test-dir") == 0);
 
 return 0;
diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
index a26ee46..3b431ee 100644
--- a/src/vdagent/file-xfers.c
+++ b/src/vdagent/file-xfers.c
@@ -206,7 +206,8 @@ vdagent_file_xfers_create_file(const char *save_dir, char 
**file_name_p)
 goto error;
 }
 g_free(path);
-char *extension = strrchr(file_path, '.');
+char *extension = strrchr(file_path, '/');
+extension = strrchr(extension != NULL ? extension + 1 : file_path, 
'.');
 int basename_len = extension != NULL ? extension - file_path : 
strlen(file_path);
 path = g_strdup_printf("%.*s (%i)%s", basename_len, file_path,
i + 1, extension ? extension : "");
-- 
2.20.1

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

[Spice-devel] [PATCH vd_agent_linux 5/7] Add a test to test file creation

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 Makefile.am | 22 +++
 src/tests/test-file-xfers.c | 73 +
 2 files changed, 95 insertions(+)
 create mode 100644 src/tests/test-file-xfers.c

diff --git a/Makefile.am b/Makefile.am
index 3e405bc..2aee440 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -3,6 +3,8 @@ NULL =
 
 bin_PROGRAMS = src/spice-vdagent
 sbin_PROGRAMS = src/spice-vdagentd
+check_PROGRAMS = src/test-file-xfers
+TESTS = $(check_PROGRAMS)
 
 common_sources =   \
src/udscs.c \
@@ -44,6 +46,26 @@ src_spice_vdagent_SOURCES =  \
src/vdagent/vdagent.c   \
$(NULL)
 
+src_test_file_xfers_CFLAGS =   \
+   $(SPICE_CFLAGS) \
+   $(GLIB2_CFLAGS) \
+   -I$(srcdir)/src \
+   -I$(srcdir)/src/vdagent \
+   -DUDSCS_NO_SERVER   \
+   $(NULL)
+
+src_test_file_xfers_LDADD =\
+   $(SPICE_LIBS)   \
+   $(GLIB2_LIBS)   \
+   $(NULL)
+
+src_test_file_xfers_SOURCES =  \
+   $(common_sources)   \
+   src/vdagent/file-xfers.c\
+   src/vdagent/file-xfers.h\
+   src/tests/test-file-xfers.c \
+   $(NULL)
+
 src_spice_vdagentd_CFLAGS =\
$(DBUS_CFLAGS)  \
$(LIBSYSTEMD_DAEMON_CFLAGS) \
diff --git a/src/tests/test-file-xfers.c b/src/tests/test-file-xfers.c
new file mode 100644
index 000..494adc9
--- /dev/null
+++ b/src/tests/test-file-xfers.c
@@ -0,0 +1,73 @@
+/*  test-file-xfers.c  - test file transfer
+
+Copyright 2019 Red Hat, Inc.
+
+This program is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 3 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see .
+*/
+#include 
+
+#undef NDEBUG
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include "file-xfers.h"
+
+static void test_file(const char *file_name, const char *out)
+{
+char *fn = g_strdup(file_name);
+int fd = vdagent_file_xfers_create_file("./test-dir", );
+if (out) {
+g_assert_cmpint(fd, !=, -1);
+g_assert_cmpstr(fn, ==, out);
+close(fd);
+g_assert_cmpint(access(out, W_OK), ==, 0);
+} else {
+g_assert_cmpint(fd, ==, -1);
+}
+g_free(fn);
+}
+
+int main(int argc, char *argv[])
+{
+assert(system("rm -rf test-dir && mkdir test-dir") == 0);
+
+// create a file
+test_file("test.txt", "./test-dir/test.txt");
+
+// create a file with an existing name
+for (int i = 1; i < 64; ++i) {
+char out_name[64];
+sprintf(out_name, "./test-dir/test (%d).txt", i);
+test_file("test.txt", out_name);
+}
+
+// check too much files with the same name
+test_file("test.txt", NULL);
+
+// create a file in a subdirectory not existing
+test_file("subdir/test.txt", "./test-dir/subdir/test.txt");
+
+// try to create a file with a path where there's a file (should fail)
+test_file("test.txt/out", NULL);
+
+assert(system("rm -rf test-dir") == 0);
+
+return 0;
+}
-- 
2.20.1

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

[Spice-devel] [PATCH vd_agent_linux 6/7] Fix race condition creating file

[Frediano Ziglio]

Avoid race condition creating file between checking file existence
with stat() and opening with open().
Directly create the file passing the O_EXCL flag.

Signed-off-by: Frediano Ziglio 
---
 src/vdagent/file-xfers.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
index 941524a..a26ee46 100644
--- a/src/vdagent/file-xfers.c
+++ b/src/vdagent/file-xfers.c
@@ -186,7 +186,6 @@ vdagent_file_xfers_create_file(const char *save_dir, char 
**file_name_p)
 char *path = NULL;
 int file_fd = -1;
 int i;
-struct stat st;
 
 file_path = g_build_filename(save_dir, *file_name_p, NULL);
 dir = g_path_get_dirname(file_path);
@@ -196,28 +195,29 @@ vdagent_file_xfers_create_file(const char *save_dir, char 
**file_name_p)
 }
 
 path = g_strdup(file_path);
-for (i = 0; i < 64 && (stat(path, ) == 0 || errno != ENOENT); i++) {
+for (i = 0; i < 64; i++) {
+file_fd = open(path, O_CREAT | O_WRONLY | O_EXCL, 0644);
+if (file_fd >= 0) {
+break;
+}
+if (errno != EEXIST) {
+syslog(LOG_ERR, "file-xfer: failed to create file %s: %s",
+   path, strerror(errno));
+goto error;
+}
 g_free(path);
 char *extension = strrchr(file_path, '.');
 int basename_len = extension != NULL ? extension - file_path : 
strlen(file_path);
 path = g_strdup_printf("%.*s (%i)%s", basename_len, file_path,
i + 1, extension ? extension : "");
 }
+if (file_fd < 0) {
+syslog(LOG_ERR, "file-xfer: more than 63 copies of %s exist?", 
file_path);
+goto error;
+}
 g_free(*file_name_p);
 *file_name_p = path;
 path = NULL;
-if (i == 64) {
-syslog(LOG_ERR, "file-xfer: more than 63 copies of %s exist?",
-   file_path);
-goto error;
-}
-
-file_fd = open(*file_name_p, O_CREAT | O_WRONLY, 0644);
-if (file_fd == -1) {
-syslog(LOG_ERR, "file-xfer: failed to create file %s: %s",
-   path, strerror(errno));
-goto error;
-}
 
 error:
 g_free(path);
-- 
2.20.1

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

[Spice-devel] [PATCH vd_agent_linux 1/7] file-xfers: Initialise correctly AgentFileXferTask::file_fd field

[Frediano Ziglio]

Correct invalid value for a file descriptor is -1, not 0.

Signed-off-by: Frediano Ziglio 
---
 src/vdagent/file-xfers.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/vdagent/file-xfers.c b/src/vdagent/file-xfers.c
index b5aedd0..78d5db3 100644
--- a/src/vdagent/file-xfers.c
+++ b/src/vdagent/file-xfers.c
@@ -133,6 +133,7 @@ static AgentFileXferTask *vdagent_parse_start_msg(
 goto error;
 }
 task = g_new0(AgentFileXferTask, 1);
+task->file_fd = -1;
 task->id = msg->id;
 task->file_name = g_key_file_get_string(
 keyfile, "vdagent-file-xfer", "name", );
-- 
2.20.1

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

Re: [Spice-devel] [spice-gtk [rfc] 0/2] Clipboard managers and Spice

[Marc-André Lureau]

On Tue, Jan 15, 2019 at 8:11 PM Victor Toso  wrote:
>
> From: Victor Toso 
>
> Hi,
>
> Several iteractions trying to avoid some bug in X11 but in the end I
> found the iteraction with Clibpoard managers (or any other application
> that request/set clipboard data) a bit more urgent.
>
> Simple try here, to not allow another application to request clipboard
> data from guest while the user is theoretically interacting with that
> guest machine as spice client holds the keyboard-grab.
>
> As pointed out by elmarco [0], that might be something desireable. So,
> I'm introducing the possibility to enable it but have it disabled by
> default.

Iho, this kind of desktop policy doesn't belong in spice-gtk.

If you don't trust the desktop, how can you trust the client itself?

Isn't it already the clipboard behaviour on Wayland?

If really more secure, shouldn't it be enforced at a lower-level, at gtk level?

In any case, I don't think this needs to delay v0.36, since it's not a
regression. Hopefully, you agree and we can solve this for the next
release.

> Tested on X11 and Wayland clients.
>
> There are more than on away to achieve this idea so feedback is welcome.
>
> I expect that the spice client would implement some sort to commandline
> option like --allow-clipobard-managers to enable/disable the
> SpiceGtkSession property on the fly. For now, there is an option in
> spicy testing tool.
>
> James, would be great if you could verify if this series keep your
> environment bug free.
>
> Cheers,
>
> Victor Toso (2):
>   gtk-session: introduce clipboard-managers property


>   gtk-session: add request targets delayed
>
>  src/spice-gtk-session.c | 128 +---
>  tools/spicy.c   |   6 ++
>  2 files changed, 125 insertions(+), 9 deletions(-)
>
> --
> 2.20.1
>


--
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v5] gtk-session: improve doc on owner-changed

[Jakub Janku]

Hi,

On Tue, Jan 15, 2019 at 11:22 AM Victor Toso  wrote:
>
> From: Victor Toso 
>
> * Set -> Sets (Jakub)
> * Clarify when onwer-changed event is called with
>   owner == self (Jakub)
>
> Signed-off-by: Victor Toso 
> ---
>  src/spice-gtk-session.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index adc72a2..32f857d 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -631,7 +631,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
>   * Situation 2: When spice-gtk holds the focus and is changing the clipboard 
> by
>   * either setting new content information with 
> gtk_clipboard_set_with_owner() or
>   * clearing up old content with gtk_clipboard_clear(). The main difference 
> between
> - * Wayland and X11 is that on X11, gtk_clipboard_clear() set the owner to 
> none, which
> + * Wayland and X11 is that on X11, gtk_clipboard_clear() sets the owner to 
> none, which
>   * emits owner-change event; On Wayland that does not happen as spice-gtk 
> still is
>   * the owner of the clipboard.
>   */
> @@ -668,8 +668,7 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
>  return;
>  }
>
> -/* This situation happens when clipboard is being cleared by us, when 
> agent
> - * sends a release-grab for instance */
> +/* This situation happens when clipboard is being set by us (grab 
> message) */
>  if (gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)) {
>  return;
>  }
> --
> 2.20.1
>

Now it looks good :)

Acked-by: Jakub Janků 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] What algorithm does SPICE server use in video stream detection?

[陈炤]

Hi Christophe,


Thank you for your answer！


BR,
Don








At 2019-01-15 21:54:36, "Christophe Fergeau"  wrote:
>Hey,
>
>On Thu, Jan 10, 2019 at 03:03:05PM +0800, 陈炤 wrote:
>> Hi,
>> 
>> 
>> 
>> I read the spice server code, and find that the video stream detection
>> code is in server/spice-bitmap-utils.c. func
>> bitmap_get_graduality_level will calculate a score, and the GRADUALITY
>> is set based on the score.
>> 
>> So what's the meaning of this score, and what algorithm does it use to
>> calculate this score?
>
>Long time since I looked at that code, but iirc it tries to detect if
>the region looks like text or an image, and if it's changing a lot. If
>it's an image which is changing a lot, then it's recognized as a video
>stream.
>
>Christophe
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk [rfc] 0/2] Clipboard managers and Spice

[Victor Toso]

Hi,

On Wed, Jan 16, 2019 at 12:08:31AM +0400, Marc-André Lureau wrote:
> On Tue, Jan 15, 2019 at 8:11 PM Victor Toso  wrote:
> >
> > From: Victor Toso 
> >
> > Hi,
> >
> > Several iteractions trying to avoid some bug in X11 but in the end I
> > found the iteraction with Clibpoard managers (or any other application
> > that request/set clipboard data) a bit more urgent.
> >
> > Simple try here, to not allow another application to request clipboard
> > data from guest while the user is theoretically interacting with that
> > guest machine as spice client holds the keyboard-grab.
> >
> > As pointed out by elmarco [0], that might be something desireable. So,
> > I'm introducing the possibility to enable it but have it disabled by
> > default.
> 
> Iho, this kind of desktop policy doesn't belong in spice-gtk.

Spice protocol implements this grab/request/release and who
send/receive those messages should be wary of them.

> If you don't trust the desktop, how can you trust the client
> itself?

What? How many process do you have running in you machine? Do you
know every little thing about them? Do you expect all users to
trust every piece of software running on all target desktops?

For all I know, browsers might run malicious javascript code that
interact with clipboard; X11 is bad by design in ta regard; In
GNOME3, GPaste running with x11 backend was able to get clipboard
data

> Isn't it already the clipboard behaviour on Wayland?

Why you bring 'wayland' on a generic proposal while you nack x11
proposal by bringing 'not being generic'?

> If really more secure, shouldn't it be enforced at a
> lower-level, at gtk level?

Gtk handles application policies. Spice, in some regards, should
extend that, if the rationale here is good enough for that.

> In any case, I don't think this needs to delay v0.36, since
> it's not a regression. Hopefully, you agree and we can solve
> this for the next release.

You are eager to do a release :)

I should probably have put the 'since 0.37' for the proposal but
this is jut a RFC, I wish we could discuss why this is important
instead of merging it quickly for a release or not.

(so, yes, go ahead with the release and thanks for pushing for
that!)

> > Tested on X11 and Wayland clients.
> >
> > There are more than on away to achieve this idea so feedback is welcome.
> >
> > I expect that the spice client would implement some sort to commandline
> > option like --allow-clipobard-managers to enable/disable the
> > SpiceGtkSession property on the fly. For now, there is an option in
> > spicy testing tool.
> >
> > James, would be great if you could verify if this series keep your
> > environment bug free.
> >
> > Cheers,
> >
> > Victor Toso (2):
> >   gtk-session: introduce clipboard-managers property
> 
> 
> >   gtk-session: add request targets delayed
> >
> >  src/spice-gtk-session.c | 128 +---
> >  tools/spicy.c   |   6 ++
> >  2 files changed, 125 insertions(+), 9 deletions(-)
> >
> > --
> > 2.20.1
> >
> 
> 
> --
> Marc-André Lureau

Victor


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-gtk v1 2/2] gtk-session: clipboard request: notify agent on failure

[Victor Toso]

From: Victor Toso 

Similar to 172c521271a3d - if we don't, the agent might be waiting for
data till some timeout happens in the system, blocking copy-paste
feature and possibly freezing applications.

A way to reproduce is copy-paste big clipboard data between two spice
clients.

Also add some documentation to the checks, in order to quickly
understand what they are about.

Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876

Signed-off-by: Victor Toso 
---
 src/spice-gtk-session.c | 32 +++-
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 1a19bca..aa812d1 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -1015,12 +1015,30 @@ static gboolean clipboard_request(SpiceMainChannel 
*main, guint selection,
 int m;
 
 cb = get_clipboard_from_selection(s, selection);
-g_return_val_if_fail(cb != NULL, FALSE);
-g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, FALSE);
-g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
+if (cb == NULL) {
+goto notify_agent;
+}
 
-if (read_only(self))
-return FALSE;
+/* As we are holding clipboard data sent by selection-grab from agent, the
+ * selection-request of clipboard data from client OS must fail. We either
+ * sent a bad selection-grab to the agent or the agent is buggy. */
+if (s->clipboard_by_guest[selection]) {
+SPICE_DEBUG("clipboard: agent request: grab on hold by agent, possible 
race");
+goto notify_agent;
+}
+
+/* The selection-request by agent should happen only if the clipboard data 
is set
+ * by client */
+if (!s->clip_grabbed[selection]) {
+SPICE_DEBUG("clipboard: agent request: data set by agent, possible 
race");
+goto notify_agent;
+}
+
+/* Ready only, still should reply to agent to avoid it waiting for data */
+if (read_only(self)) {
+g_warning("clipboard: agent request: read only, deny request");
+goto notify_agent;
+}
 
 if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
 gtk_clipboard_request_text(cb, clipboard_received_text_cb,
@@ -1039,6 +1057,10 @@ static gboolean clipboard_request(SpiceMainChannel 
*main, guint selection,
 }
 
 return TRUE;
+
+notify_agent:
+spice_main_channel_clipboard_selection_notify(s->main, selection, type, 
NULL, 0);
+return FALSE;
 }
 
 static void clipboard_release(SpiceMainChannel *main, guint selection,
-- 
2.20.1

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

[Spice-devel] [spice-gtk v1 1/2] gtk-session: clipboard: avoid possible index out of bounds

[Victor Toso]

From: Victor Toso 

Should first check selection value before accessing those arrays.
The get_clipboard_from_selection() function does that for us.

Signed-off-by: Victor Toso 
---
 src/spice-gtk-session.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 32f857d..1a19bca 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -1014,15 +1014,14 @@ static gboolean clipboard_request(SpiceMainChannel 
*main, guint selection,
 GtkClipboard* cb;
 int m;
 
+cb = get_clipboard_from_selection(s, selection);
+g_return_val_if_fail(cb != NULL, FALSE);
 g_return_val_if_fail(s->clipboard_by_guest[selection] == FALSE, FALSE);
 g_return_val_if_fail(s->clip_grabbed[selection], FALSE);
 
 if (read_only(self))
 return FALSE;
 
-cb = get_clipboard_from_selection(s, selection);
-g_return_val_if_fail(cb != NULL, FALSE);
-
 if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
 gtk_clipboard_request_text(cb, clipboard_received_text_cb,
get_weak_ref(self));
-- 
2.20.1

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

Re: [Spice-devel] [spice-gtk v4 2/2] gtk-session: clipboard: x11: owner-change improvement

[Victor Toso]

Hi,

On Tue, Jan 15, 2019 at 01:40:48AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jan 15, 2019 at 12:40 AM Jakub Janku  wrote:
> >
> > Hi,
> >
> > On Mon, Jan 14, 2019 at 1:41 PM Marc-André Lureau
> >  wrote:
> > >
> > > Hi
> > >
> > > On Mon, Jan 14, 2019 at 4:34 PM Victor Toso  wrote:
> > > >
> > > > From: Victor Toso 
> > > >
> > > > On X11, the release-grab message might end up clearing the
> > > > GtkClipboard which triggers the owner-changed callback having the
> > > > event owner as NULL.
> > > >
> > > > We should not be calling gtk_clipboard_request_targets() in this
> > > > situation as is prone to errors as the intention here is request
> > > > clipboard information from changes made by client OS.
> > > >
> > > > The fix is to avoid any such request while spice client is holding the
> > > > keyboard grab.
> > >
> > > Two things that bug me about this:
> > > - it's x11 specific, weird
> >
> > What's so weird about that?
> > It's also X11-specific that we can receive these "owner-change" events
> > while spice-gtk is holding the focus since on Wayland, apps need to
> > gain focus first to be able to set the clipboard, afaik.
> > The clipboard protocols on X11 and Wayland have simply been designed
> > in different ways. So although GTK tries to unify the behavior and
> > provide the same experience in both environments, some limitations
> > just cannot be overcome.
> 
> The patch is not about integration with X11, the way for
> example we have overlay, key locks, or mouse acceleration code
> that gtk doesn't provide. It's about an API that gtk+ provides
> that should be uniform.  Sometime it fails to provide the
> require abstraction. In this case, I don't undestand *yet* why
> the code should be specific to X11, it could be equally "valid"
> on wayland or windows etc...

My first attempt to make it generic was a failure because, on
Wayland, when we receive the owner-change event, we already had
the focus and would miss the event due my buggy code. I have it
fixed now in a more generic way, I have yet to test on windows.

> > > - the condition seems wrong: if an application has the keyboard grab,
> > > that doesn't mean that another cannot update the clipboard. For
> > > example, I suppose this can easily happen with multiple
> > > pointers/inputs or some automation.
> >
> > Sure, another app can update the clipboard, but imagine the following 
> > situation:
> > 1) user copies something in the guest
> > 2) an app in the client's system grabs the clipboard without having
> > keyboard focus -- this means that the grab likely wasn't directly
> > initiated by the user
> 
> I don't know, you could have some automation framework changing the
> clipboard. For testing, for example, or to remove swear words, or
> other kind of protected content..

Would be okay if we propose a gtk-session property to
enable/disable clipboard managers? If that's a use case you see
important, having it as a command line option should be enough

eg: remote-viewer --allow-clipboard-manager

That would keep the code behavior as is.

> And I think you could have multiple keyboard/pointers on the same
> seat, although this may be esoteric, I agree.
> 
> > 3) spice-gtk receives "owner-change" event and subsequently sends grab
> > to the agent
> > 4) based on the grab message from spice-gtk in step 3), vdagent grabs
> > the clipboard in the guest system -- and by that, it overwrites the
> > clipboard contents copied in step 1)
> 
> That would be what I expect indeed.
> 
> >
> > So the clipboard is suddenly changed, although the user didn't take any 
> > action.
> 
> And this may also be what the user wanted.

I think the implication of 'not being what user wanted' be quite
severe. e.g: The user having his password stored in clipboard
temporally would have that sent to all guests that it is
connected at that moment.

> > I think this is unwanted and this patch solves it, so I'm all for
> > ack-ing this just for this reason. If this also solves the bug, it's
> > even better.
> >
> > Nevertheless, I would be really happy if we could track down what
> > exactly causes the issues that have been reported and describe it in
> > more detail.
> 
> Yes, that sounds like a better course to me.

Sorry, really not sure how to write it better myself. I tried.

I'm not taking in consideration that this is misbehavior of gtk
but expected behavior that we are not handling well, that's all.

> > > Other than that, if it fixes or workaround real clipboard issues, I am
> > > not against it, but I think we should add more comments about the
> > > "hack".
> > >
> > >
> > > >
> > > > Related: https://gitlab.freedesktop.org/spice/win32/vd_agent/issues/6
> > > > Related: https://gitlab.freedesktop.org/spice/linux/vd_agent/issues/9
> > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
> > > >
> > > > Changed in v4:
> > > > - Updated commit log
> > > >
> > > > Signed-off-by: Victor Toso 
> > > > Tested-by: James Harvey 

Re: [Spice-devel] [PATCH v2] gstreamer: set timestamp in buffer's GstReferenceTimestampMeta

[Snir Sheriber]

Hi,

On 1/15/19 3:42 PM, Frediano Ziglio wrote:

Currently we set timestamps as buffer's PTS, this value may be changed by
the pipeline in some cases and cause an unexpected buffer warnings (when
GstVideoOverlay is not used). Use GstReferenceTimestampMeta when
synchronization is made by spice.

Before applying this patch you can reproduce the warnings by runing with
DISABLE_GSTVIDEOOVERLAY=1 and starting some audio playback in the guest.

Signed-off-by: Snir Sheriber 
---
Changes from v1:
-Commit message
---
  src/channel-display-gst.c | 27 ---
  1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 2f556fe..a90fa81 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -33,6 +33,10 @@ typedef struct SpiceGstFrame SpiceGstFrame;
  
  /* GStreamer decoder implementation */
  
+#if GST_CHECK_VERSION(1,14,0)

+static GstStaticCaps stream_reference =
GST_STATIC_CAPS("timestamp/spice-stream");
+#endif
+
  typedef struct SpiceGstDecoder {
  VideoDecoder base;
  
@@ -86,7 +90,14 @@ struct SpiceGstFrame {

  static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceFrame *frame)
  {
  SpiceGstFrame *gstframe = g_new(SpiceGstFrame, 1);
+#if GST_CHECK_VERSION(1,14,0)
+GstReferenceTimestampMeta *time_meta;
+
+time_meta = gst_buffer_get_reference_timestamp_meta(buffer,
gst_static_caps_get(_reference));
+gstframe->timestamp = time_meta->timestamp;
+#else
  gstframe->timestamp = GST_BUFFER_PTS(buffer);
+#endif
  gstframe->frame = frame;
  gstframe->sample = NULL;
  return gstframe;
@@ -211,6 +222,12 @@ static void fetch_pending_sample(SpiceGstDecoder
*decoder)
  decoder->pending_samples--;
  
  GstBuffer *buffer = gst_sample_get_buffer(sample);

+GstClockTime buffer_ts;
+#if GST_CHECK_VERSION(1,14,0)
+buffer_ts = gst_buffer_get_reference_timestamp_meta(buffer,
gst_static_caps_get(_reference))->timestamp;

This line crashes for me, gst_buffer_get_reference_timestamp_meta returns
NULL. It does not surprise me, you attach metadata to an input buffer
and you expect these metadata to be attached to output buffers.
Apparently this is not guaranteed.



IIUC It should:

"A GstBuffer is the object that is passed from an upstream element to a 
downstream element and contains memory and metadata information."


Actually this is pretty similar to what we are currently doing- counting 
on the PTS (which is may be
changed by the elements in oppose to GstReferenceTimestampMeta which 
should be used as an

alternative timestamp to PTS which is not being touched)


Anyway this could be fixed with a fallback to PTS which i set in any 
case but i want to understand if
there's some bug here, does it always happen to you? can you reproduce 
it consistently?


Snir.





+#else
+buffer_ts = GST_BUFFER_PTS(buffer);
+#endif
  
  /* gst_app_sink_pull_sample() sometimes returns the same buffer

  twice
   * or buffers that have a modified, and thus unrecognizable, PTS.
@@ -223,7 +240,7 @@ static void fetch_pending_sample(SpiceGstDecoder
*decoder)
  GList *l = g_queue_peek_head_link(decoder->decoding_queue);
  while (l) {
  gstframe = l->data;
-if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
+if (gstframe->timestamp == buffer_ts) {
  /* The frame is now ready for display */
  gstframe->sample = sample;
  decoder->display_frame = gstframe;
@@ -232,7 +249,7 @@ static void fetch_pending_sample(SpiceGstDecoder
*decoder)
   * frames from the decoding queue.
   */
  while ((gstframe =
  g_queue_pop_head(decoder->decoding_queue))) {
-if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
+if (gstframe->timestamp == buffer_ts) {
  break;
  }
  /* The GStreamer pipeline dropped the corresponding
@@ -626,9 +643,13 @@ static gboolean
spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
  frame->data,
  frame->size, 0,
  frame->size,
  frame->data_opaque,
  frame->unref_data);
  
+GstClockTime pts = gst_clock_get_time(decoder->clock) -

gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0, latency)) *
1000 * 1000;
  GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
  GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
-GST_BUFFER_PTS(buffer) = gst_clock_get_time(decoder->clock) -
gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0, latency)) *
1000 * 1000;
+

Re: [Spice-devel] [vdagent-linux] systemd: Update path in unit file

[Frediano Ziglio]

> 
> From: Christian Hesse 
> 
> When loading the unit file, systemd 240 logs:
> 
> systemd[1]: /usr/lib/systemd/system/spice-vdagentd.service:9: PIDFile=
> references path below legacy directory /var/run/, updating
> /var/run/spice-vdagentd/spice-vdagentd.pid →
> /run/spice-vdagentd/spice-vdagentd.pid; please update the unit file
> accordingly.
> 
> So update the path.
> 
> Signed-off-by: Christian Hesse 

Acked-by: Frediano Ziglio 

> ---
>  data/spice-vdagentd.service | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/data/spice-vdagentd.service b/data/spice-vdagentd.service
> index 365b2c1..9c70b30 100644
> --- a/data/spice-vdagentd.service
> +++ b/data/spice-vdagentd.service
> @@ -7,7 +7,7 @@ Requires=spice-vdagentd.socket
>  Type=forking
>  EnvironmentFile=-/etc/sysconfig/spice-vdagentd
>  ExecStart=/usr/sbin/spice-vdagentd $SPICE_VDAGENTD_EXTRA_ARGS
> -PIDFile=/var/run/spice-vdagentd/spice-vdagentd.pid
> +PIDFile=/run/spice-vdagentd/spice-vdagentd.pid
>  PrivateTmp=true
>  Restart=on-failure
>  

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

Re: [Spice-devel] [spice-gtk v4 1/2] gtk-session: improve doc on owner-changed

[Victor Toso]

Hi,

On Mon, Jan 14, 2019 at 08:49:52PM +0100, Jakub Janku wrote:
> Hi,
> 
> On Mon, Jan 14, 2019 at 1:34 PM Victor Toso  wrote:
> >
> > From: Victor Toso 
> >
> > * Sets -> Set (Jakub)
> > * Clarify when onwer-changed event is called with
> >   owner == self (Jakub)
> >
> > Signed-off-by: Victor Toso 
> > ---
> >  src/spice-gtk-session.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index adc72a2..abce43f 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -631,7 +631,7 @@ static void clipboard_get_targets(GtkClipboard 
> > *clipboard,
> >   * Situation 2: When spice-gtk holds the focus and is changing the 
> > clipboard by
> >   * either setting new content information with 
> > gtk_clipboard_set_with_owner() or
> >   * clearing up old content with gtk_clipboard_clear(). The main difference 
> > between
> > - * Wayland and X11 is that on X11, gtk_clipboard_clear() set the owner to 
> > none, which
> > + * Wayland and X11 is that on X11, gtk_clipboard_clear() sets the owner to 
> > none, which
> >   * emits owner-change event; On Wayland that does not happen as spice-gtk 
> > still is
> >   * the owner of the clipboard.
> >   */
> > @@ -668,8 +668,8 @@ static void clipboard_owner_change(GtkClipboard
> > *clipboard,
> >  return;
> >  }
> >
> > -/* This situation happens when clipboard is being cleared by us, when 
> > agent
> > - * sends a release-grab for instance */
> > +/* This situation happens when clipboard is being set by us (grab 
> > message)
> 
> this is fine
> 
> > + * and on X11 also when cleared by us (release-grab) */
> 
> But I don't understand why you added this.
> If spice-gtk receives release-grab from vdagent, it calls
> gtk_clipboard_clear(), then "owner-change" is emitted. In the
> callback, gtk_clipboard_get_owner() returns NULL, so the condition
> below evaluates as FALSE -- which is the case you're trying to handle
> in 2/2 of this series, if I'm not mistaken.
> 
> So I think this line should be removed.

I'm stupid. I've documented what I saw but indeed, again, not
pertinent to the check below. Thanks, I'll remove it.

> >  if (gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)) {
> >  return;
> >  }
> > --
> > 2.20.1
> >
> Cheers,
> Jakub


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk v3 1/3] gtk-session: clipboard: document owner-changed event

[Victor Toso]

Hi,

On Mon, Jan 14, 2019 at 10:15:04PM +0100, Jakub Janku wrote:
> > The comment refers to the gtk_clipboard_clear() not
> > owner-changed event so you are right, of course. This comment
> > was probably a left over from different interactions that I
> > did in this code :(
> >
> > I'll send a fix for the documentation.
> >
> > For the other changes, I'd love to hear your opinion too. Did
> > not get 2/3 and 3/3 patches acked but more keen to discuss
> > the idea.
> >
> > I was wrong in regards to clipboard managers on Wayland, that
> > is, clipboard managers on Wayland *can* get clipboard data of
> > spice-gtk while spice-gtk's widgets are holding the focus :(
> 
> That's weird. I didn't think this was possible.
> I've just had a very brief look at the code of GPaste:
> 
> https://github.com/Keruspe/GPaste/blob/master/src/daemon/gpaste-daemon.c
> 
> the main function includes the following lines:
> |/* FIXME: remove this once gtk supports clipboard correctly
> on wayland */
> |gdk_set_allowed_backends ("x11");
> This probably indicates that the clipboard manager actually runs on
> XWayland, which would explain why it's able to eavesdrop on the
> clipboard without having focus.
> It is the same "workaround" that we use in vdagent.

Ah, good. Interesting that remote-viewer running on Wayland could
have its clipboard data fetched by another application running
with x11 backend.

> > I didn't check how that happen but I'd rather avoid/deny that to
> > happen..
> >
> > Thanks again Jakub,
> >
> > > > +if (gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)) {
> > > > +return;
> > > > +}
> > > > +
> > > > +s->clipboard_by_guest[selection] = FALSE;
> > > > +s->clip_hasdata[selection] = TRUE;
> > > > +if (s->auto_clipboard_enable && !read_only(self))
> > > > +gtk_clipboard_request_targets(clipboard, clipboard_get_targets,
> > > > +  get_weak_ref(self));
> > > >  }
> > > >
> > > >  typedef struct
> > > > --
> > > > 2.20.1
> > > >
> > >
> > > Cheers,
> > > Jakub
> >
> >


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-gtk v4 1/2] Fix overlay for vaapisink

[Frediano Ziglio]

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 
---
To test it probably you'll have to remove the gstreamer registry,
usually in ~/.cache/gstreamer-1.0/registry.x86_64.bin.

Changes since v3:
- none, add a patch to fix another issue;

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| 45 +++
 5 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/configure.ac b/configure.ac
index bba13fba..0f69b3c2 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 acd5dcaf..69bbee57 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 d1de9473..66884a74 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..69c00558 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -28,6 +28,9 @@
 #ifdef GDK_WINDOWING_X11
 #include 
 #include 
+#ifdef HAVE_LIBVA
+#include 
+#endif
 #endif
 #ifdef G_OS_WIN32
 #include 
@@ -2592,6 +2595,48 @@ 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, _type);
+SPICE_DEBUG("GStreamer: got need context %s from %s", context_type,
+GST_MESSAGE_SRC_NAME(msg));
+
+#if defined(GDK_WINDOWING_X11) && defined(HAVE_LIBVA)
+if (g_strcmp0(context_type, "gst.vaapi.app.Display") == 0) {
+static Display 

[Spice-devel] [spice-gtk v5] gtk-session: improve doc on owner-changed

[Victor Toso]

From: Victor Toso 

* Set -> Sets (Jakub)
* Clarify when onwer-changed event is called with
  owner == self (Jakub)

Signed-off-by: Victor Toso 
---
 src/spice-gtk-session.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index adc72a2..32f857d 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -631,7 +631,7 @@ static void clipboard_get_targets(GtkClipboard *clipboard,
  * Situation 2: When spice-gtk holds the focus and is changing the clipboard by
  * either setting new content information with gtk_clipboard_set_with_owner() 
or
  * clearing up old content with gtk_clipboard_clear(). The main difference 
between
- * Wayland and X11 is that on X11, gtk_clipboard_clear() set the owner to 
none, which
+ * Wayland and X11 is that on X11, gtk_clipboard_clear() sets the owner to 
none, which
  * emits owner-change event; On Wayland that does not happen as spice-gtk 
still is
  * the owner of the clipboard.
  */
@@ -668,8 +668,7 @@ static void clipboard_owner_change(GtkClipboard
*clipboard,
 return;
 }
 
-/* This situation happens when clipboard is being cleared by us, when agent
- * sends a release-grab for instance */
+/* This situation happens when clipboard is being set by us (grab message) 
*/
 if (gtk_clipboard_get_owner(clipboard) == G_OBJECT(self)) {
 return;
 }
-- 
2.20.1

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

[Spice-devel] [PATCH spice-gtk v4 2/2] spice-widget: Avoid deadlock for VAAPI

[Frediano Ziglio]

Calling gst_video_overlay_handle_events after
gst_video_overlay_set_window_handle causes often a deadlock in
gstreamer-vaapi.
Reverting the calls fix this issue.

Signed-off-by: Frediano Ziglio 
---
 src/spice-widget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/spice-widget.c b/src/spice-widget.c
index 69c00558..1d7c8c17 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -2588,8 +2588,8 @@ static void gst_sync_bus_call(GstBus *bus, GstMessage 
*msg, SpiceDisplay *displa
 
 GstVideoOverlay *overlay = 
GST_VIDEO_OVERLAY(GST_MESSAGE_SRC(msg));
 g_weak_ref_set(>overlay_weak_ref, overlay);
-gst_video_overlay_set_window_handle(overlay, 
(uintptr_t)GDK_WINDOW_XID(window));
 gst_video_overlay_handle_events(overlay, false);
+gst_video_overlay_set_window_handle(overlay, 
(uintptr_t)GDK_WINDOW_XID(window));
 return;
 }
 }
-- 
2.20.1

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