Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing

[Uri Lublin]

On 02/19/2018 06:47 PM, Lukáš Hrázký wrote:

On Mon, 2018-02-19 at 18:29 +0200, Uri Lublin wrote:

On 02/14/2018 06:37 PM, Christophe Fergeau wrote:

On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote:



On 14 Feb 2018, at 13:34, Lukáš Hrázký  wrote:

Introduce a unit test framework (Catch) to the codebase and a simple
unit test for parsing the options of the mjpeg plugin.

Signed-off-by: Lukáš Hrázký 
---
configure.ac  |  3 ++
src/mjpeg-fallback.cpp|  5 +++
src/mjpeg-fallback.hpp|  1 +
src/unittests/.gitignore  |  5 +--
src/unittests/Makefile.am | 15 +
src/unittests/test-mjpeg-fallback.cpp | 58
+++
6 files changed, 85 insertions(+), 2 deletions(-)
create mode 100644 src/unittests/test-mjpeg-fallback.cpp

diff --git a/configure.ac b/configure.ac
index 8795dae..5aab662 100644
--- a/configure.ac
+++ b/configure.ac
@@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then
  AC_MSG_ERROR([C99 compiler is required.])
fi
AC_PROG_CXX
+AC_LANG(C++)
AX_CXX_COMPILE_STDCXX_11
AC_PROG_INSTALL
AC_CANONICAL_HOST
@@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
  AC_MSG_ERROR([libjpeg not found]))
AC_SUBST(JPEG_LIBS)

+AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not find Catch
dependency header (catch/catch.hpp)])])


Instead of an error, shouldn’t we instead fallback to not compiling the unit
tests? Possibly a warning?



Good point but I would suggest a follow up and an explicit 
--I-dont-really-want-unittests
option, I agree people should run tests.
Another follow up is a patch for the spec file.


Alternatively, this could go with a --with-catch flag (or
--enable-unittest or any names which fits you), and
1) if there is nothing specified, then we silently enable/disable tests
depending on the availability of the catch
2) if --with-catch is specified, then we error out if it's not found
3) if --without-catch is used, then we never use it.

Then we add --with-catch to autogen.sh, and all developers will have to
go through extra hoops not to use it.

Christophe



I agree that's a good alternative.


I prefer Frediano's variant. Nobody is forced to use autogen.sh and
also I don't think anybody would expect autogen.sh to modify the
default configure behaviour. I don't think it's a good idea.


If users prefer to not run autogen.sh that's ok.
It provides defaults options for developers.
For example, I do not expect users to run configure with
 --enable-maintainer-mode too.
Most users will use configure directly from the tarball.

Users can choose what options they want to enable/disable



For example, on Gentoo, which doesn't care for autogen.sh and runs
autotools build automatically, the default behaviour (unless the person
writing the ebuild would notice) would be dependent on Catch being
present in the system. This can create subtle inconsistencies, which
aren't critical in this case, but still unnecessary.

So if on Gentoo (or another distribution) there exists no catch
package, they are forced to either add this package or search
for how to disable it.

This package is only required if the user wants
to run the tests.
Those tests do not run by default either.
One have to "make check" for the tests to run.

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

Re: [Spice-devel] [PATCH 09/17] Reorder headers according to style guide

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/spice-streaming-agent.cpp | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 720173a..f845bd0 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -4,6 +4,9 @@
>   * Copyright 2016-2017 Red Hat Inc. All rights reserved.
>   */
>  
> +#include "concrete-agent.hpp"
> +#include "hexdump.h"
> +
>  #include 
>  #include 
>  #include 
> @@ -34,8 +37,7 @@
>  #include 
>  #include 
>  
> -#include "hexdump.h"
> -#include "concrete-agent.hpp"
> +
>  
>  using namespace spice::streaming_agent;

While at it, I would also move the spice installed headers up. Can help
to catch issues with them. I've got a patch where I also separate the
system and C++ headers and order them alphabetically, but you don't
necessarily need to be that fancy :D

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

Re: [Spice-devel] [PATCH 08/17] Use C++ style for cursor message initialization instead of C style

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/spice-streaming-agent.cpp | 47 
> +--
>  1 file changed, 27 insertions(+), 20 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 1e19e43..720173a 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -285,38 +285,45 @@ static void usage(const char *progname)
>  }
>  
>  static void
> -send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x, 
> int hotspot_y,
> +send_cursor(int streamfd,
> +uint16_t width, uint16_t height,
> +uint16_t hotspot_x, uint16_t hotspot_y,
>  std::function fill_cursor)
>  {
>  if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>  height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
>  return;
>  
> -size_t cursor_size =
> -sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
> -width * height * sizeof(uint32_t);
> -std::unique_ptr msg(new uint8_t[cursor_size]);
> +const uint32_t cursor_msgsize =
> +sizeof(SpiceStreamCursorMessage) + width * height * sizeof(uint32_t);
> +const uint32_t hdrsize  = sizeof(StreamDevHeader);

Seems like inconsistent naming "cursor_msgsize" vs "hdrsize", two
spaces before =, maybe the sizeof(StreamDevHeader) could be used
directly below.

> -StreamDevHeader _hdr(*reinterpret_cast(msg.get()));
> -memset(_hdr, 0, sizeof(dev_hdr));
> -dev_hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> -dev_hdr.type = STREAM_TYPE_CURSOR_SET;
> -dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
> +std::unique_ptr storage(new uint8_t[cursor_msgsize]);
>  
> -StreamMsgCursorSet _msg(*reinterpret_cast *>(msg.get() + sizeof(StreamDevHeader)));
> -memset(_msg, 0, sizeof(cursor_msg));
> -
> -cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
> -cursor_msg.width = width;
> -cursor_msg.height = height;
> -cursor_msg.hot_spot_x = hotspot_x;
> -cursor_msg.hot_spot_y = hotspot_y;
> +SpiceStreamCursorMessage *cursor_msg =
> +new(storage.get()) SpiceStreamCursorMessage {
> +.hdr = {
> +.protocol_version = STREAM_DEVICE_PROTOCOL,
> +.padding = 0,   // Workaround GCC internal / not implemented 
> compiler error
> +.type = STREAM_TYPE_CURSOR_SET,
> +.size = cursor_msgsize - hdrsize
> +},
> +.msg = {
> +.width = width,
> +.height = height,
> +.hot_spot_x = hotspot_x,
> +.hot_spot_y = hotspot_y,
> +.type = SPICE_CURSOR_TYPE_ALPHA,
> +.padding1 = { },
> +.data = { }
> +}
> +};
>  
> -uint32_t *pixels = reinterpret_cast(cursor_msg.data);
> +uint32_t *pixels = reinterpret_cast(cursor_msg->msg.data);
>  fill_cursor(pixels);
>  
>  std::lock_guard stream_guard(stream_mtx);
> -write_all(streamfd, msg.get(), cursor_size);
> +write_all(streamfd, storage.get(), cursor_msgsize);
>  }
>  
>  static void cursor_changes(int streamfd, Display *display, int event_base)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-streaming-agent 4/6] Separate handling start/stop message from server

[Uri Lublin]

On 02/19/2018 06:44 PM, Frediano Ziglio wrote:


On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:

Prepare to add support for other messages.

Signed-off-by: Frediano Ziglio 
---
  src/spice-streaming-agent.cpp | 26 ++
  1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 08f4b48..41b4d3d 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -78,10 +78,11 @@ static int have_something_to_read(int timeout)
  return 0;
  }
  
+static int read_stream_start_stop(uint32_t len);

+
  static int read_command_from_device(void)
  {
  StreamDevHeader hdr;
-uint8_t msg[64];
  int n;
  
  std::lock_guard stream_guard(stream_mtx);

@@ -93,15 +94,24 @@ static int read_command_from_device(void)
  ERROR("BAD VERSION " << hdr.protocol_version <<
" (expected is " << STREAM_DEVICE_PROTOCOL << ")");
  }
-if (hdr.type != STREAM_TYPE_START_STOP) {
-ERROR("UNKNOWN msg of type " << hdr.type);
+
+switch (hdr.type) {
+case STREAM_TYPE_START_STOP:
+return read_stream_start_stop(hdr.size);
  }
-if (hdr.size >= sizeof(msg)) {
-ERROR("msg size (" << hdr.size << ") is too long (longer than " <<
sizeof(msg));
+ERROR("UNKNOWN msg of type " << hdr.type);
+}
+
+static int read_stream_start_stop(uint32_t len)
+{
+uint8_t msg[256];
+
+if (len >= sizeof(msg)) {
+ERROR("msg size (" << len << ") is too long (longer than " <<
sizeof(msg));
  }
-n = read(streamfd, , hdr.size);
-if (n != hdr.size) {
-ERROR("read command from device FAILED -- read " << n << "
expected " << hdr.size);
+int n = read(streamfd, , len);
+if (n != len) {
+ERROR("read command from device FAILED -- read " << n << "
expected " << len);
  }
  streaming_requested = (msg[0] != 0); /* num_codecs */
  syslog(LOG_INFO, "GOT START_STOP message -- request to %s
  streaming\n",


So, you started working on the same code as well :P I actually have
this part rewritten in a C++ way. I was surprised to not find it in
Christophe's patches. My head starts going crazy from all the different
patches to the same code...

I would suggest leaving this off until we switch to an encapsulated C++
implementation?

Lukas



Sorry, I thought you were working on the notify error message only,
I'm trying to add mainly capability support for start fixing the
ID mismatch issue, I left the error handling (notify error) as stub.

Yes, unfortunately the code is small and there's quite a lot of clash.


I too wrote some very similar patches :)
(but did not yet send them and now I will not send them)

That's what happens when there are problems in the code and people
are trying to fix them.

Regards,
Uri.

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

Re: [Spice-devel] [PATCH 07/17] Get rid of C-style memset initializations, use C++ style aggregates

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/spice-streaming-agent.cpp | 47 
> ++-
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 69c27a3..1e19e43 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -181,19 +181,24 @@ write_all(int fd, const void *buf, const size_t len)
>  return written;
>  }
>  
> -static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, 
> unsigned c)
> +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, 
> uint8_t c)
>  {
> -
> -SpiceStreamFormatMessage msg;
> -const size_t msgsize = sizeof(msg);
> -const size_t hdrsize  = sizeof(msg.hdr);
> -memset(, 0, msgsize);
> -msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> -msg.hdr.type = STREAM_TYPE_FORMAT;
> -msg.hdr.size = msgsize - hdrsize; /* includes only the body? */
> -msg.msg.width = w;
> -msg.msg.height = h;
> -msg.msg.codec = c;
> +const size_t msgsize = sizeof(SpiceStreamFormatMessage);
> +const size_t hdrsize  = sizeof(StreamDevHeader);
> +SpiceStreamFormatMessage msg = {
> +.hdr = {
> +.protocol_version = STREAM_DEVICE_PROTOCOL,
> +.padding = 0,   // Workaround GCC "not implemented" bug
> +.type = STREAM_TYPE_FORMAT,
> +.size = msgsize - hdrsize
> +},
> +.msg = {
> +.width = w,
> +.height = h,
> +.codec = c,
> +.padding1 = { }
> +}
> +};
>  syslog(LOG_DEBUG, "writing format\n");
>  std::lock_guard stream_guard(stream_mtx);
>  if (write_all(streamfd, , msgsize) != msgsize) {
> @@ -204,14 +209,18 @@ static int spice_stream_send_format(int streamfd, 
> unsigned w, unsigned h, unsign
>  
>  static int spice_stream_send_frame(int streamfd, const void *buf, const 
> unsigned size)
>  {
> -SpiceStreamDataMessage msg;
> -const size_t msgsize = sizeof(msg);
>  ssize_t n;
> +const size_t msgsize = sizeof(SpiceStreamFormatMessage);
> +SpiceStreamDataMessage msg = {
> +.hdr = {
> +.protocol_version = STREAM_DEVICE_PROTOCOL,
> +.padding = 0,   // Workaround GCC "not implemented" bug
> +.type = STREAM_TYPE_DATA,
> +.size = size  /* includes only the body? */
> +},
> +.msg = {}
> +};

So, someone should find out if we can use the designated initializers,
I suppose it depends on the compilers on all platforms we care about
supporting them?

I wasn't able to find much useful information so far. Anyone knows in
which version of gcc it was introduced?

Lukas

> -memset(, 0, msgsize);
> -msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> -msg.hdr.type = STREAM_TYPE_DATA;
> -msg.hdr.size = size; /* includes only the body? */
>  std::lock_guard stream_guard(stream_mtx);
>  n = write_all(streamfd, , msgsize);
>  syslog(LOG_DEBUG,
> @@ -379,7 +388,7 @@ do_capture(int streamfd, const char *streamport, FILE 
> *f_log)
>  
>  if (frame.stream_start) {
>  unsigned width, height;
> -unsigned char codec;
> +uint8_t codec;
>  
>  width = frame.size.width;
>  height = frame.size.height;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:59 +0100, Christophe de Dinechin wrote:
> > On 16 Feb 2018, at 17:40, Frediano Ziglio  wrote:
> > 
> > > 
> > > From: Christophe de Dinechin 
> > > 
> > > Get rid of C-style 'goto done' in do_capture.
> > > Get rid of global streamfd, pass it around (cleaned up in later patch)
> > > Fixes a race condition, make sure we only use stream after opening
> > > 
> > > Signed-off-by: Christophe de Dinechin 
> > > ---
> > > src/spice-streaming-agent.cpp | 79
> > > +--
> > > 1 file changed, 47 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index 646d15b..9b8fd45 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
> > > StreamMsgData msg;
> > > };
> > > 
> > > +struct SpiceStreamCursorMessage
> > > +{
> > > +StreamDevHeader hdr;
> > > +StreamMsgCursorSet msg;
> > > +};
> > > +
> > 
> > This is weird in this patch
> 
> Yes. I may have merged two patches. Will check.
> 
> > 
> > > +class Stream
> > > +{
> > > +public:
> > > +Stream(const char *name)
> > > +{
> > > +fd = open(name, O_RDWR);
> > > +if (fd < 0)
> > > +throw std::runtime_error("failed to open streaming device");

Braces around if body (according to the coding style). Repeated a few
times below.

> > > +}
> > > +~Stream()
> > > +{
> > > +close(fd);
> > 
> > You probably what to check for -1 to avoid calling close(-1)
> 
> I think this cannot happen, since in that case you’d have thrown from the 
> ctor.
> 
> > 
> > > +}
> > > +operator int() { return fd; }
> > 
> > Sure you want this? I think is safer to avoid implicit cast
> > also considering stuff like
> > 
> >  Stream s;
> >  if (s) …
> 
> It is removed in a later patch once it’s no longer needed. The objective here 
> is to minimize noisy changes at each step.

I'd also rather not see it, but if it's removed later, no big deal.

> > 
> > > +
> > > +private:
> > > +int fd = -1;
> > 
> > So with a default constructor you want a class with
> > an invalid state?
> > Or just disable default constructor.
> 
> It’s disabled, there’s a ctor with args.
> 
> > I would disable also copy.
> 
> Also implicitly disabled.

It is not, actually, disabled here. It gets disabled later when you add
the mutex member, as that is a non-copyable type.

It might be a good idea to explicitly delete the copy ctr/ao anyway...
In case someone ever removes the mutex and doesn't realize this.

> > 
> > > +};
> > > +
> > > static bool streaming_requested = false;
> > > static bool quit_requested = false;
> > > static bool log_binary = false;
> > > static std::set client_codecs;
> > > -static int streamfd = -1;
> > > static std::mutex stream_mtx;
> > > 
> > > -static int have_something_to_read(int timeout)
> > > +static int have_something_to_read(int streamfd, int timeout)
> > 
> > maybe would be better to use the "fd" name here, is no more
> > bound to stream.
> 
> Kept to minimize changes
> 
> > > {
> > > struct pollfd pollfd = {streamfd, POLLIN, 0};
> > > 
> > > @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
> > > return 0;
> > > }
> > > 
> > > -static int read_command_from_device(void)
> > > +static int read_command_from_device(int streamfd)
> > 
> > Have you considered Stream::read_command?
> 
> For that specific case, I liked the “from_device”. But…
> 
> > Ok, mostly in a following patch
> 
> 
> > 
> > > {
> > > StreamDevHeader hdr;
> > > uint8_t msg[64];
> > > @@ -121,18 +145,18 @@ static int read_command_from_device(void)
> > > return 1;
> > > }
> > > 
> > > -static int read_command(bool blocking)
> > > +static int read_command(int streamfd, bool blocking)
> > > {
> > > int timeout = blocking?-1:0;
> > > while (!quit_requested) {
> > > -if (!have_something_to_read(timeout)) {
> > > +if (!have_something_to_read(streamfd, timeout)) {
> > > if (!blocking) {
> > > return 0;
> > > }
> > > sleep(1);
> > > continue;
> > > }
> > > -return read_command_from_device();
> > > +return read_command_from_device(streamfd);
> > > }
> > > 
> > > return 1;
> > > @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
> > > return written;
> > > }
> > > 
> > > -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
> > > +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
> > > unsigned c)
> > > {
> > > 
> > > SpiceStreamFormatMessage msg;
> > > @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, 
> > > unsigned
> > > h, unsigned c)
> > > return EXIT_SUCCESS;
> > > }
> > > 
> > > -static int spice_stream_send_frame(const void *buf, 

Re: [Spice-devel] [PATCH spice-streaming-agent 1/6] Be more restrictive about error handling

[Uri Lublin]

On 02/19/2018 05:52 PM, Frediano Ziglio wrote:

There's no much point in ignoring the error if these errors cause the
communication to be out of sync.
Ignoring them just change the state to indetermidate.


Hi Frediano,

There is a typo in the last word.

I would be more specific, mentioning those are header errors.

(Currently, upon returning -1 in those cases, spice-streaming-agent
 exits).



Signed-off-by: Frediano Ziglio 


Acked-by: Uri Lublin 


---
  src/spice-streaming-agent.cpp | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 45a92b9..1f41a6f 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -94,17 +94,17 @@ static int read_command_from_device(void)
  if (hdr.protocol_version != STREAM_DEVICE_PROTOCOL) {
  syslog(LOG_WARNING, "BAD VERSION %d (expected is %d)\n", 
hdr.protocol_version,
 STREAM_DEVICE_PROTOCOL);
-return 0; // return -1; -- fail over this ?
+return -1;
  }
  if (hdr.type != STREAM_TYPE_START_STOP) {
  syslog(LOG_WARNING, "UNKNOWN msg of type %d\n", hdr.type);
-return 0; // return -1;
+return -1;
  }
  if (hdr.size >= sizeof(msg)) {
  syslog(LOG_WARNING,
 "msg size (%d) is too long (longer than %lu)\n",
 hdr.size, sizeof(msg));
-return 0; // return -1;
+return -1;
  }
  n = read(streamfd, , hdr.size);
  if (n != hdr.size) {



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

Re: [Spice-devel] [PATCH 03/17] Eliminate signed/unsigned warning

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/mjpeg-fallback.cpp| 2 +-
>  src/spice-streaming-agent.cpp | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 634864f..53804d9 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -57,7 +57,7 @@ private:
>  std::vector frame;
>  
>  // last frame sizes
> -uint32_t last_width = ~0u, last_height = ~0u;
> +int last_width = ~0u, last_height = ~0u;

This is really weird. The ~ negates an unsigned literal and the result
is a signed type? How come? Maybe remove the 'u' from the literals so
that we can pretend everything is normal? :D

>  // last time before capture
>  uint64_t last_time = 0;
>  };
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 6056129..b5c7e24 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -106,7 +106,7 @@ static int read_command_from_device(void)
>  return 0; // return -1;
>  }
>  n = read(streamfd, , hdr.size);
> -if (n != hdr.size) {
> +if (n != (int) hdr.size) {
>  syslog(LOG_WARNING,
> "read command from device FAILED -- read %d expected %d\n",
> n, hdr.size);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 01/17] Add missing header

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/concrete-agent.cpp | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 891c09b..ac37788 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -9,6 +9,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "concrete-agent.hpp"
>  #include "static-plugin.hpp"

Acked-by: Lukáš Hrázký 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 00/17] WIP: Refactor the streaming agent towards a more standard C++ style

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> The streaming agent started as C code. This series converts
> the style to something that is more usual for C++, notably:
> 
> - Adds encapsulation and RAII for resources such as the stream
> - Splits functionality into several classes with clear roles
> - Puts message formatting and writing in a reused based class
> - Isolate what's specific to each message in three derived classes
> - Isolate X11-specific code in separate classes, one for cursor messages,
>   one for the thread polling the data.
> 
> Reasons for marking this WIP:
> 
> 1. This series is, unfortunately, not correctly tested, because on my
>machine, I currently have a black screen with the 'master' streaming
>agent, server, protocol, common, plugin and spicy. Fallback to MJPEG
>leads to a large repetition of messages like:
> 
>(spicy:4217): GSpice-CRITICAL **: need more input data
> 
>What I have tested using the -d option is that the syslog output
>from the agent looks similar (size of data captured, etc) relative
>to master both for the MJPEG plugin and with fallback. But without
>a picture, I am still concerned about the lack of testing.

Christophe already knows, but in case anyone will benefit from this,
it's the incomplete frame bug, fix should be on the ML here:

[Spice-devel] [PATCH spice-server 8/8] stream-channel: Send the full
frame in a single message

Christophe, I've tested your patches, it's streaming, but the cursor
disappears after a while, so something's not entirely right.

> 2. The classes were isolated, but not moved in separate headers. This
>is intentional. I prefer to make sure that the changes on the code
>are agreed on before we move the individual classes to their own
>headers. It thinks it will also faclitate history browsing

I'd like to point out the classes are not entirely isolated, there is
still the static bool streaming_requested (besides the necessary
quit_requested, although I have an idea for that), which ties together
the ConcreteAgent::CaptureLoop and Stream::read_command_from_device.

I have an unfinished patch for this, which will need heavy rebasing. In
case you are/will be looking into it, Christophe, let me know.

Lukas

> 3. This series compiles without warnings both with GCC in C++11 mode
>and by clang in gnu++11 mode. However, Frediano pointed out that it
>uses a designated intiializer syntax that is presently a GNU
>extension (the C99 designated initializer syntax). We may consider
>it a problem or not. If it's a problem, it's sufficient to remove
>the designators, but I think they add to readability.
> 
> 4. Our current configure.ac requires a warning if all initializers are
>not present. Since padding was made explicit in the protocol, this
>requires the code to initialize padding fields, which I don't like.
> 
> 5. The series integrates off-topic patches sent in a separate series, but
>which are necessary to successfully build with both clang and gcc.
> 
> This series can also be browsed at
> https://gitlab.com/c3d/spice-streaming-agent/merge_requests/1/commits
> 
> Christophe de Dinechin (17):
>   Add missing  header
>   log_binary is really a boolean
>   Eliminate signed/unsigned warning
>   Do not create std::string for constants
>   Use RAII to cleanup stream in case of exception or return
>   Replace inefficient C-style initialization with C++-style
>   Get rid of C-style memset initializations, use C++ style aggregates
>   Use C++ style for cursor message initialization instead of C style
>   Reorder headers according to style guide
>   Since we use a namespace, simplify name of local classes
>   Move read, write and locking into the 'Stream' class
>   Convert message writing from C style to C++ style
>   Add more meaningful syslog reporting
>   Create a class encapsulating the X11 display cursor capture
>   Create FrameLog class to abstract logging of frames
>   Remove client_codecs global variable, moved inside the 'Stream' class
>   Move the capture loop in the ConcreteAgent, get rid of global agent
> variable
> 
>  src/concrete-agent.cpp|   1 +
>  src/concrete-agent.hpp|   4 +
>  src/mjpeg-fallback.cpp|   2 +-
>  src/spice-streaming-agent.cpp | 521 
> +-
>  4 files changed, 311 insertions(+), 217 deletions(-)
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

[Frediano Ziglio]

> 
> On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
> > Allows to reuse it.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  src/Makefile.am|  1 +
> >  src/mjpeg-fallback.cpp |  7 +--
> >  src/utils.hpp  | 18 ++
> >  3 files changed, 20 insertions(+), 6 deletions(-)
> >  create mode 100644 src/utils.hpp
> > 
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 3717b5c..ba3b1bf 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
> > mjpeg-fallback.hpp \
> > jpeg.cpp \
> > jpeg.hpp \
> > +   utils.hpp \
> > $(NULL)
> > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > index cf704c6..0f31834 100644
> > --- a/src/mjpeg-fallback.cpp
> > +++ b/src/mjpeg-fallback.cpp
> > @@ -6,6 +6,7 @@
> >  
> >  #include 
> >  #include "mjpeg-fallback.hpp"
> > +#include "utils.hpp"
> >  
> >  #include 
> >  #include 
> > @@ -19,12 +20,6 @@
> >  
> >  using namespace spice::streaming_agent;
> >  
> > -#define ERROR(args) do { \
> > -std::ostringstream _s; \
> > -_s << args; \
> > -throw std::runtime_error(_s.str()); \
> > -} while(0)
> > -
> >  static inline uint64_t get_time()
> >  {
> >  timespec now;
> > diff --git a/src/utils.hpp b/src/utils.hpp
> > new file mode 100644
> > index 000..1e43eff
> > --- /dev/null
> > +++ b/src/utils.hpp
> > @@ -0,0 +1,18 @@
> > +/* Miscellaneous utilities
> > + *
> > + * \copyright
> > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > + */
> > +#ifndef SPICE_STREAMING_AGENT_UTILS_HPP
> > +#define SPICE_STREAMING_AGENT_UTILS_HPP
> > +
> > +#include 
> > +#include 
> > +
> > +#define ERROR(args) do { \
> > +std::ostringstream _s; \
> > +_s << args; \
> > +throw std::runtime_error(_s.str()); \
> > +} while(0)
> > +
> > +#endif // SPICE_STREAMING_AGENT_UTILS_HPP
> 
> Can we please not do this :) It isn't such a chore to throw the
> exceptions directly. This adds a level of indirection (code-wise) and
> macros are (personal opinion alert) best avoided in C++ unless you
> absolutely have to...
> 
> Lukas
> 

Yes, I'm taking to much shortcuts. I was considering a format library like
https://github.com/fmtlib/fmt. Did you even used a format library?

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

Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing

[Lukáš Hrázký]

On Mon, 2018-02-19 at 18:29 +0200, Uri Lublin wrote:
> On 02/14/2018 06:37 PM, Christophe Fergeau wrote:
> > On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > > On 14 Feb 2018, at 13:34, Lukáš Hrázký  wrote:
> > > > > 
> > > > > Introduce a unit test framework (Catch) to the codebase and a simple
> > > > > unit test for parsing the options of the mjpeg plugin.
> > > > > 
> > > > > Signed-off-by: Lukáš Hrázký 
> > > > > ---
> > > > > configure.ac  |  3 ++
> > > > > src/mjpeg-fallback.cpp|  5 +++
> > > > > src/mjpeg-fallback.hpp|  1 +
> > > > > src/unittests/.gitignore  |  5 +--
> > > > > src/unittests/Makefile.am | 15 +
> > > > > src/unittests/test-mjpeg-fallback.cpp | 58
> > > > > +++
> > > > > 6 files changed, 85 insertions(+), 2 deletions(-)
> > > > > create mode 100644 src/unittests/test-mjpeg-fallback.cpp
> > > > > 
> > > > > diff --git a/configure.ac b/configure.ac
> > > > > index 8795dae..5aab662 100644
> > > > > --- a/configure.ac
> > > > > +++ b/configure.ac
> > > > > @@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then
> > > > >  AC_MSG_ERROR([C99 compiler is required.])
> > > > > fi
> > > > > AC_PROG_CXX
> > > > > +AC_LANG(C++)
> > > > > AX_CXX_COMPILE_STDCXX_11
> > > > > AC_PROG_INSTALL
> > > > > AC_CANONICAL_HOST
> > > > > @@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> > > > >  AC_MSG_ERROR([libjpeg not found]))
> > > > > AC_SUBST(JPEG_LIBS)
> > > > > 
> > > > > +AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not find 
> > > > > Catch
> > > > > dependency header (catch/catch.hpp)])])
> > > > 
> > > > Instead of an error, shouldn’t we instead fallback to not compiling the 
> > > > unit
> > > > tests? Possibly a warning?
> > > > 
> > > 
> > > Good point but I would suggest a follow up and an explicit 
> > > --I-dont-really-want-unittests
> > > option, I agree people should run tests.
> > > Another follow up is a patch for the spec file.
> > 
> > Alternatively, this could go with a --with-catch flag (or
> > --enable-unittest or any names which fits you), and
> > 1) if there is nothing specified, then we silently enable/disable tests
> > depending on the availability of the catch
> > 2) if --with-catch is specified, then we error out if it's not found
> > 3) if --without-catch is used, then we never use it.
> > 
> > Then we add --with-catch to autogen.sh, and all developers will have to
> > go through extra hoops not to use it.
> > 
> > Christophe
> > 
> 
> I agree that's a good alternative.

I prefer Frediano's variant. Nobody is forced to use autogen.sh and
also I don't think anybody would expect autogen.sh to modify the
default configure behaviour. I don't think it's a good idea.

For example, on Gentoo, which doesn't care for autogen.sh and runs
autotools build automatically, the default behaviour (unless the person
writing the ebuild would notice) would be dependent on Catch being
present in the system. This can create subtle inconsistencies, which
aren't critical in this case, but still unnecessary.

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

Re: [Spice-devel] [PATCH spice-streaming-agent 1/6] Be more restrictive about error handling

[Frediano Ziglio]

> 
> On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
> > There's no much point in ignoring the error if these errors cause the
> > communication to be out of sync.
> > Ignoring them just change the state to indetermidate.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  src/spice-streaming-agent.cpp | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index 45a92b9..1f41a6f 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -94,17 +94,17 @@ static int read_command_from_device(void)
> >  if (hdr.protocol_version != STREAM_DEVICE_PROTOCOL) {
> >  syslog(LOG_WARNING, "BAD VERSION %d (expected is %d)\n",
> >  hdr.protocol_version,
> > STREAM_DEVICE_PROTOCOL);
> > -return 0; // return -1; -- fail over this ?
> > +return -1;
> >  }
> >  if (hdr.type != STREAM_TYPE_START_STOP) {
> >  syslog(LOG_WARNING, "UNKNOWN msg of type %d\n", hdr.type);
> > -return 0; // return -1;
> > +return -1;
> >  }
> >  if (hdr.size >= sizeof(msg)) {
> >  syslog(LOG_WARNING,
> > "msg size (%d) is too long (longer than %lu)\n",
> > hdr.size, sizeof(msg));
> > -return 0; // return -1;
> > +return -1;
> >  }
> >  n = read(streamfd, , hdr.size);
> >  if (n != hdr.size) {
> 
> Hi, why not, but why not skip this and use the exceptions straight
> away?
> 
> Lukas
> 

Seems more incremental that way.

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

Re: [Spice-devel] [PATCH spice-streaming-agent 4/6] Separate handling start/stop message from server

[Frediano Ziglio]

> 
> On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
> > Prepare to add support for other messages.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  src/spice-streaming-agent.cpp | 26 ++
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index 08f4b48..41b4d3d 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -78,10 +78,11 @@ static int have_something_to_read(int timeout)
> >  return 0;
> >  }
> >  
> > +static int read_stream_start_stop(uint32_t len);
> > +
> >  static int read_command_from_device(void)
> >  {
> >  StreamDevHeader hdr;
> > -uint8_t msg[64];
> >  int n;
> >  
> >  std::lock_guard stream_guard(stream_mtx);
> > @@ -93,15 +94,24 @@ static int read_command_from_device(void)
> >  ERROR("BAD VERSION " << hdr.protocol_version <<
> >" (expected is " << STREAM_DEVICE_PROTOCOL << ")");
> >  }
> > -if (hdr.type != STREAM_TYPE_START_STOP) {
> > -ERROR("UNKNOWN msg of type " << hdr.type);
> > +
> > +switch (hdr.type) {
> > +case STREAM_TYPE_START_STOP:
> > +return read_stream_start_stop(hdr.size);
> >  }
> > -if (hdr.size >= sizeof(msg)) {
> > -ERROR("msg size (" << hdr.size << ") is too long (longer than " <<
> > sizeof(msg));
> > +ERROR("UNKNOWN msg of type " << hdr.type);
> > +}
> > +
> > +static int read_stream_start_stop(uint32_t len)
> > +{
> > +uint8_t msg[256];
> > +
> > +if (len >= sizeof(msg)) {
> > +ERROR("msg size (" << len << ") is too long (longer than " <<
> > sizeof(msg));
> >  }
> > -n = read(streamfd, , hdr.size);
> > -if (n != hdr.size) {
> > -ERROR("read command from device FAILED -- read " << n << "
> > expected " << hdr.size);
> > +int n = read(streamfd, , len);
> > +if (n != len) {
> > +ERROR("read command from device FAILED -- read " << n << "
> > expected " << len);
> >  }
> >  streaming_requested = (msg[0] != 0); /* num_codecs */
> >  syslog(LOG_INFO, "GOT START_STOP message -- request to %s
> >  streaming\n",
> 
> So, you started working on the same code as well :P I actually have
> this part rewritten in a C++ way. I was surprised to not find it in
> Christophe's patches. My head starts going crazy from all the different
> patches to the same code...
> 
> I would suggest leaving this off until we switch to an encapsulated C++
> implementation?
> 
> Lukas
> 

Sorry, I thought you were working on the notify error message only,
I'm trying to add mainly capability support for start fixing the
ID mismatch issue, I left the error handling (notify error) as stub.

Yes, unfortunately the code is small and there's quite a lot of clash.

Maybe would be better to coordinate a bit more, indeed.

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

Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing

[Uri Lublin]

On 02/14/2018 06:37 PM, Christophe Fergeau wrote:

On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote:



On 14 Feb 2018, at 13:34, Lukáš Hrázký  wrote:

Introduce a unit test framework (Catch) to the codebase and a simple
unit test for parsing the options of the mjpeg plugin.

Signed-off-by: Lukáš Hrázký 
---
configure.ac  |  3 ++
src/mjpeg-fallback.cpp|  5 +++
src/mjpeg-fallback.hpp|  1 +
src/unittests/.gitignore  |  5 +--
src/unittests/Makefile.am | 15 +
src/unittests/test-mjpeg-fallback.cpp | 58
+++
6 files changed, 85 insertions(+), 2 deletions(-)
create mode 100644 src/unittests/test-mjpeg-fallback.cpp

diff --git a/configure.ac b/configure.ac
index 8795dae..5aab662 100644
--- a/configure.ac
+++ b/configure.ac
@@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then
 AC_MSG_ERROR([C99 compiler is required.])
fi
AC_PROG_CXX
+AC_LANG(C++)
AX_CXX_COMPILE_STDCXX_11
AC_PROG_INSTALL
AC_CANONICAL_HOST
@@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
 AC_MSG_ERROR([libjpeg not found]))
AC_SUBST(JPEG_LIBS)

+AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not find Catch
dependency header (catch/catch.hpp)])])


Instead of an error, shouldn’t we instead fallback to not compiling the unit
tests? Possibly a warning?



Good point but I would suggest a follow up and an explicit 
--I-dont-really-want-unittests
option, I agree people should run tests.
Another follow up is a patch for the spec file.


Alternatively, this could go with a --with-catch flag (or
--enable-unittest or any names which fits you), and
1) if there is nothing specified, then we silently enable/disable tests
depending on the availability of the catch
2) if --with-catch is specified, then we error out if it's not found
3) if --without-catch is used, then we never use it.

Then we add --with-catch to autogen.sh, and all developers will have to
go through extra hoops not to use it.

Christophe



I agree that's a good alternative.

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

Re: [Spice-devel] [PATCH spice-streaming-agent 4/6] Separate handling start/stop message from server

[Lukáš Hrázký]

On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
> Prepare to add support for other messages.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  src/spice-streaming-agent.cpp | 26 ++
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 08f4b48..41b4d3d 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -78,10 +78,11 @@ static int have_something_to_read(int timeout)
>  return 0;
>  }
>  
> +static int read_stream_start_stop(uint32_t len);
> +
>  static int read_command_from_device(void)
>  {
>  StreamDevHeader hdr;
> -uint8_t msg[64];
>  int n;
>  
>  std::lock_guard stream_guard(stream_mtx);
> @@ -93,15 +94,24 @@ static int read_command_from_device(void)
>  ERROR("BAD VERSION " << hdr.protocol_version <<
>" (expected is " << STREAM_DEVICE_PROTOCOL << ")");
>  }
> -if (hdr.type != STREAM_TYPE_START_STOP) {
> -ERROR("UNKNOWN msg of type " << hdr.type);
> +
> +switch (hdr.type) {
> +case STREAM_TYPE_START_STOP:
> +return read_stream_start_stop(hdr.size);
>  }
> -if (hdr.size >= sizeof(msg)) {
> -ERROR("msg size (" << hdr.size << ") is too long (longer than " << 
> sizeof(msg));
> +ERROR("UNKNOWN msg of type " << hdr.type);
> +}
> +
> +static int read_stream_start_stop(uint32_t len)
> +{
> +uint8_t msg[256];
> +
> +if (len >= sizeof(msg)) {
> +ERROR("msg size (" << len << ") is too long (longer than " << 
> sizeof(msg));
>  }
> -n = read(streamfd, , hdr.size);
> -if (n != hdr.size) {
> -ERROR("read command from device FAILED -- read " << n << " expected 
> " << hdr.size);
> +int n = read(streamfd, , len);
> +if (n != len) {
> +ERROR("read command from device FAILED -- read " << n << " expected 
> " << len);
>  }
>  streaming_requested = (msg[0] != 0); /* num_codecs */
>  syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",

So, you started working on the same code as well :P I actually have
this part rewritten in a C++ way. I was surprised to not find it in
Christophe's patches. My head starts going crazy from all the different
patches to the same code...

I would suggest leaving this off until we switch to an encapsulated C++
implementation?

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

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

[Lukáš Hrázký]

On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
> Allows to reuse it.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  src/Makefile.am|  1 +
>  src/mjpeg-fallback.cpp |  7 +--
>  src/utils.hpp  | 18 ++
>  3 files changed, 20 insertions(+), 6 deletions(-)
>  create mode 100644 src/utils.hpp
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 3717b5c..ba3b1bf 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
>   mjpeg-fallback.hpp \
>   jpeg.cpp \
>   jpeg.hpp \
> + utils.hpp \
>   $(NULL)
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index cf704c6..0f31834 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -6,6 +6,7 @@
>  
>  #include 
>  #include "mjpeg-fallback.hpp"
> +#include "utils.hpp"
>  
>  #include 
>  #include 
> @@ -19,12 +20,6 @@
>  
>  using namespace spice::streaming_agent;
>  
> -#define ERROR(args) do { \
> -std::ostringstream _s; \
> -_s << args; \
> -throw std::runtime_error(_s.str()); \
> -} while(0)
> -
>  static inline uint64_t get_time()
>  {
>  timespec now;
> diff --git a/src/utils.hpp b/src/utils.hpp
> new file mode 100644
> index 000..1e43eff
> --- /dev/null
> +++ b/src/utils.hpp
> @@ -0,0 +1,18 @@
> +/* Miscellaneous utilities
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +#ifndef SPICE_STREAMING_AGENT_UTILS_HPP
> +#define SPICE_STREAMING_AGENT_UTILS_HPP
> +
> +#include 
> +#include 
> +
> +#define ERROR(args) do { \
> +std::ostringstream _s; \
> +_s << args; \
> +throw std::runtime_error(_s.str()); \
> +} while(0)
> +
> +#endif // SPICE_STREAMING_AGENT_UTILS_HPP

Can we please not do this :) It isn't such a chore to throw the
exceptions directly. This adds a level of indirection (code-wise) and
macros are (personal opinion alert) best avoided in C++ unless you
absolutely have to...

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

Re: [Spice-devel] [spice-gtk] Add --spice-disable-clipboard option

[Christophe Fergeau]

On Thu, Jan 18, 2018 at 12:13:45PM +0100, Christophe Fergeau wrote:
> On Thu, Jan 18, 2018 at 12:06:34PM +0100, Marc-André Lureau wrote:
> > Hi
> > 
> > On Thu, Jan 18, 2018 at 10:31 AM, Christophe Fergeau
> >  wrote:
> > > At least on X.org, malicious code could run the equivalent of "watch
> > > xsel -o --clipboard" in a VM, and would then be able to track all the
> > > clipboard content, even when the spice-gtk widget is not focused.
> > >
> > > At the moment, applications call spice_set_session_option(), and then
> > > set SpiceGtkSession::auto-clipboard to TRUE (or to its saved state).
> > > This commit adds a --spice-disable-clipboard option, and if it's set,
> > > SpiceGtkSession::auto-clipboard will not be changeable and will always
> > > be FALSE.
> > > The only side effect I noticed is that enabling "clipboard sharing" in
> > > GNOME Boxes VM preferences will appear to work, but will not enable
> > > clipboard, and will be reset to off next time the preferences dialog is
> > > open.
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1320263
> > 
> > Looks reasonable to me. However, I thought we wanted a way to disable
> > clipboard by default.
> > 
> > Wouldn't it make sense to introduce some GSetting key(s) for that instead?
> > 
> > This way, the behaviour can be enforced globally without changing the
> > way applications are started.
> 
> I think you want both, you don't necessarily want c for all or none of
> your VMs. I don't know if we can check if the admin locked down a
> particular GSettings through the API? If the global value is locked down
> to FALSE, then we should enforce it, otherwise we should accept
> --spice-disable-clipboard.
> So a GSettings patch would probably be a followup to that one.

Can we move forward with that command line addition? Or is adding a
GSettings key a prerequisite to getting this in?

Thanks,

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-streaming-agent 1/6] Be more restrictive about error handling

[Lukáš Hrázký]

On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
> There's no much point in ignoring the error if these errors cause the
> communication to be out of sync.
> Ignoring them just change the state to indetermidate.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  src/spice-streaming-agent.cpp | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 45a92b9..1f41a6f 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -94,17 +94,17 @@ static int read_command_from_device(void)
>  if (hdr.protocol_version != STREAM_DEVICE_PROTOCOL) {
>  syslog(LOG_WARNING, "BAD VERSION %d (expected is %d)\n", 
> hdr.protocol_version,
> STREAM_DEVICE_PROTOCOL);
> -return 0; // return -1; -- fail over this ?
> +return -1;
>  }
>  if (hdr.type != STREAM_TYPE_START_STOP) {
>  syslog(LOG_WARNING, "UNKNOWN msg of type %d\n", hdr.type);
> -return 0; // return -1;
> +return -1;
>  }
>  if (hdr.size >= sizeof(msg)) {
>  syslog(LOG_WARNING,
> "msg size (%d) is too long (longer than %lu)\n",
> hdr.size, sizeof(msg));
> -return 0; // return -1;
> +return -1;
>  }
>  n = read(streamfd, , hdr.size);
>  if (n != hdr.size) {

Hi, why not, but why not skip this and use the exceptions straight
away?

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

Re: [Spice-devel] [PATCH spice-streaming-agent] spec.in: Add BuildRequires for catch-devel

[Frediano Ziglio]

> 
> ---
> 
> Currently you have to have catch so no conditional is needed.
> 
> ---
>  spice-streaming-agent.spec.in | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/spice-streaming-agent.spec.in b/spice-streaming-agent.spec.in
> index d137840..ac8203a 100644
> --- a/spice-streaming-agent.spec.in
> +++ b/spice-streaming-agent.spec.in
> @@ -9,6 +9,7 @@ Source0:%{name}-%{version}.tar.xz
>  BuildRequires:  spice-protocol >= @SPICE_PROTOCOL_MIN_VER@
>  BuildRequires:  libX11-devel libXfixes-devel
>  BuildRequires:  libjpeg-turbo-devel
> +BuildRequires:  catch-devel
>  # we need /usr/sbin/semanage program which is available on different
>  # packages depending on distribution
>  Requires(post): /usr/sbin/semanage

Yes, reasonable.

Acked

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

[Spice-devel] [PATCH spice-streaming-agent 5/6] Handle capabilities

[Frediano Ziglio]

Do not bail if the server is attempting to communicate some extensions
but just ignore as at the moment we support none.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 41b4d3d..5813fba 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -41,6 +41,8 @@
 
 using namespace spice::streaming_agent;
 
+static size_t write_all(int fd, const void *buf, const size_t len);
+
 static ConcreteAgent agent;
 
 struct SpiceStreamFormatMessage
@@ -78,6 +80,7 @@ static int have_something_to_read(int timeout)
 return 0;
 }
 
+static int read_stream_capabilities(uint32_t len);
 static int read_stream_start_stop(uint32_t len);
 
 static int read_command_from_device(void)
@@ -96,6 +99,8 @@ static int read_command_from_device(void)
 }
 
 switch (hdr.type) {
+case STREAM_TYPE_CAPABILITIES:
+return read_stream_capabilities(hdr.size);
 case STREAM_TYPE_START_STOP:
 return read_stream_start_stop(hdr.size);
 }
@@ -122,6 +127,31 @@ static int read_stream_start_stop(uint32_t len)
 return 1;
 }
 
+static int read_stream_capabilities(uint32_t len)
+{
+uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
+
+if (len > sizeof(caps)) {
+ERROR("capability message too long");
+}
+int n = read(streamfd, caps, len);
+if (n != len) {
+ERROR("read command from device FAILED -- read " << n << " expected " 
<< len);
+}
+
+// we currently do not suppor extensions so just reply so
+StreamDevHeader hdr = {
+STREAM_DEVICE_PROTOCOL,
+0,
+STREAM_TYPE_CAPABILITIES,
+0
+};
+if (write_all(streamfd, , sizeof(hdr)) != sizeof(hdr)) {
+ERROR("error writing capabilities");
+}
+return 1;
+}
+
 static int read_command(bool blocking)
 {
 int timeout = blocking?-1:0;
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent 6/6] Stub to handle errors from server

[Frediano Ziglio]

Base error message handling.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 5813fba..2612ccb 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -82,6 +82,7 @@ static int have_something_to_read(int timeout)
 
 static int read_stream_capabilities(uint32_t len);
 static int read_stream_start_stop(uint32_t len);
+static int read_stream_error(uint32_t len);
 
 static int read_command_from_device(void)
 {
@@ -101,6 +102,8 @@ static int read_command_from_device(void)
 switch (hdr.type) {
 case STREAM_TYPE_CAPABILITIES:
 return read_stream_capabilities(hdr.size);
+case STREAM_TYPE_NOTIFY_ERROR:
+return read_stream_error(hdr.size);
 case STREAM_TYPE_START_STOP:
 return read_stream_start_stop(hdr.size);
 }
@@ -152,6 +155,13 @@ static int read_stream_capabilities(uint32_t len)
 return 1;
 }
 
+static int read_stream_error(uint32_t len)
+{
+// TODO read message and use it
+ERROR("got an error message from server");
+return -1;
+}
+
 static int read_command(bool blocking)
 {
 int timeout = blocking?-1:0;
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

[Frediano Ziglio]

Allows to reuse it.

Signed-off-by: Frediano Ziglio 
---
 src/Makefile.am|  1 +
 src/mjpeg-fallback.cpp |  7 +--
 src/utils.hpp  | 18 ++
 3 files changed, 20 insertions(+), 6 deletions(-)
 create mode 100644 src/utils.hpp

diff --git a/src/Makefile.am b/src/Makefile.am
index 3717b5c..ba3b1bf 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
mjpeg-fallback.hpp \
jpeg.cpp \
jpeg.hpp \
+   utils.hpp \
$(NULL)
diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index cf704c6..0f31834 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -6,6 +6,7 @@
 
 #include 
 #include "mjpeg-fallback.hpp"
+#include "utils.hpp"
 
 #include 
 #include 
@@ -19,12 +20,6 @@
 
 using namespace spice::streaming_agent;
 
-#define ERROR(args) do { \
-std::ostringstream _s; \
-_s << args; \
-throw std::runtime_error(_s.str()); \
-} while(0)
-
 static inline uint64_t get_time()
 {
 timespec now;
diff --git a/src/utils.hpp b/src/utils.hpp
new file mode 100644
index 000..1e43eff
--- /dev/null
+++ b/src/utils.hpp
@@ -0,0 +1,18 @@
+/* Miscellaneous utilities
+ *
+ * \copyright
+ * Copyright 2018 Red Hat Inc. All rights reserved.
+ */
+#ifndef SPICE_STREAMING_AGENT_UTILS_HPP
+#define SPICE_STREAMING_AGENT_UTILS_HPP
+
+#include 
+#include 
+
+#define ERROR(args) do { \
+std::ostringstream _s; \
+_s << args; \
+throw std::runtime_error(_s.str()); \
+} while(0)
+
+#endif // SPICE_STREAMING_AGENT_UTILS_HPP
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent 1/6] Be more restrictive about error handling

[Frediano Ziglio]

There's no much point in ignoring the error if these errors cause the
communication to be out of sync.
Ignoring them just change the state to indetermidate.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 45a92b9..1f41a6f 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -94,17 +94,17 @@ static int read_command_from_device(void)
 if (hdr.protocol_version != STREAM_DEVICE_PROTOCOL) {
 syslog(LOG_WARNING, "BAD VERSION %d (expected is %d)\n", 
hdr.protocol_version,
STREAM_DEVICE_PROTOCOL);
-return 0; // return -1; -- fail over this ?
+return -1;
 }
 if (hdr.type != STREAM_TYPE_START_STOP) {
 syslog(LOG_WARNING, "UNKNOWN msg of type %d\n", hdr.type);
-return 0; // return -1;
+return -1;
 }
 if (hdr.size >= sizeof(msg)) {
 syslog(LOG_WARNING,
"msg size (%d) is too long (longer than %lu)\n",
hdr.size, sizeof(msg));
-return 0; // return -1;
+return -1;
 }
 n = read(streamfd, , hdr.size);
 if (n != hdr.size) {
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent 4/6] Separate handling start/stop message from server

[Frediano Ziglio]

Prepare to add support for other messages.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 08f4b48..41b4d3d 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -78,10 +78,11 @@ static int have_something_to_read(int timeout)
 return 0;
 }
 
+static int read_stream_start_stop(uint32_t len);
+
 static int read_command_from_device(void)
 {
 StreamDevHeader hdr;
-uint8_t msg[64];
 int n;
 
 std::lock_guard stream_guard(stream_mtx);
@@ -93,15 +94,24 @@ static int read_command_from_device(void)
 ERROR("BAD VERSION " << hdr.protocol_version <<
   " (expected is " << STREAM_DEVICE_PROTOCOL << ")");
 }
-if (hdr.type != STREAM_TYPE_START_STOP) {
-ERROR("UNKNOWN msg of type " << hdr.type);
+
+switch (hdr.type) {
+case STREAM_TYPE_START_STOP:
+return read_stream_start_stop(hdr.size);
 }
-if (hdr.size >= sizeof(msg)) {
-ERROR("msg size (" << hdr.size << ") is too long (longer than " << 
sizeof(msg));
+ERROR("UNKNOWN msg of type " << hdr.type);
+}
+
+static int read_stream_start_stop(uint32_t len)
+{
+uint8_t msg[256];
+
+if (len >= sizeof(msg)) {
+ERROR("msg size (" << len << ") is too long (longer than " << 
sizeof(msg));
 }
-n = read(streamfd, , hdr.size);
-if (n != hdr.size) {
-ERROR("read command from device FAILED -- read " << n << " expected " 
<< hdr.size);
+int n = read(streamfd, , len);
+if (n != len) {
+ERROR("read command from device FAILED -- read " << n << " expected " 
<< len);
 }
 streaming_requested = (msg[0] != 0); /* num_codecs */
 syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent 3/6] Use exception handling data from streaming device

[Frediano Ziglio]

In all paths errors from this function are treated like fatal
error, there's no need to handle all manually potentially
forgetting to handle errors.
Also avoid to deal directly with logging moving the responsibility
to other layers.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 24 +++-
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 1f41a6f..08f4b48 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -37,6 +37,7 @@
 #include "hexdump.h"
 #include "concrete-agent.hpp"
 #include "mjpeg-fallback.hpp"
+#include "utils.hpp"
 
 using namespace spice::streaming_agent;
 
@@ -86,32 +87,21 @@ static int read_command_from_device(void)
 std::lock_guard stream_guard(stream_mtx);
 n = read(streamfd, , sizeof(hdr));
 if (n != sizeof(hdr)) {
-syslog(LOG_WARNING,
-   "read command from device FAILED -- read %d expected %lu\n",
-   n, sizeof(hdr));
-return -1;
+ERROR("read command from device FAILED -- read " << n << " expected " 
<< sizeof(hdr));
 }
 if (hdr.protocol_version != STREAM_DEVICE_PROTOCOL) {
-syslog(LOG_WARNING, "BAD VERSION %d (expected is %d)\n", 
hdr.protocol_version,
-   STREAM_DEVICE_PROTOCOL);
-return -1;
+ERROR("BAD VERSION " << hdr.protocol_version <<
+  " (expected is " << STREAM_DEVICE_PROTOCOL << ")");
 }
 if (hdr.type != STREAM_TYPE_START_STOP) {
-syslog(LOG_WARNING, "UNKNOWN msg of type %d\n", hdr.type);
-return -1;
+ERROR("UNKNOWN msg of type " << hdr.type);
 }
 if (hdr.size >= sizeof(msg)) {
-syslog(LOG_WARNING,
-   "msg size (%d) is too long (longer than %lu)\n",
-   hdr.size, sizeof(msg));
-return -1;
+ERROR("msg size (" << hdr.size << ") is too long (longer than " << 
sizeof(msg));
 }
 n = read(streamfd, , hdr.size);
 if (n != hdr.size) {
-syslog(LOG_WARNING,
-   "read command from device FAILED -- read %d expected %d\n",
-   n, hdr.size);
-return -1;
+ERROR("read command from device FAILED -- read " << n << " expected " 
<< hdr.size);
 }
 streaming_requested = (msg[0] != 0); /* num_codecs */
 syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
-- 
2.14.3

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

Re: [Spice-devel] [PATCH spice-streaming-agent v2 2/2] explicit instead of static registration for built-in plugins

[Frediano Ziglio]

> 
> On Fri, 2018-02-16 at 07:23 -0500, Frediano Ziglio wrote:
> > > 
> > > > On 6 Feb 2018, at 10:51, Frediano Ziglio  wrote:
> > > > 
> > > > > > 
> > > > > > On 5 Feb 2018, at 15:29, Lukáš Hrázký  wrote:
> > > > > > 
> > > > > > The static registration (that is, having a static list of pointers
> > > > > > to
> > > > > > static plugin variables and calling a generic function in the main
> > > > > > initialization code path to register them) allows to add plugins
> > > > > > without
> > > > > > registering each of them explicitly.
> > > > > > 
> > > > > > However, in a single codebase, and having very few plugins, there
> > > > > > is
> > > > > > very little advantage to it and the tradeoff for the complexity of
> > > > > > this
> > > > > > initialization is not worth it. A single call for every built-in
> > > > > > plugin
> > > > > > is much more simple and clear.
> > > > > > 
> > > > > > Signed-off-by: Lukáš Hrázký 
> > > > > > ---
> > > > > > src/Makefile.am   |  2 --
> > > > > > src/concrete-agent.cpp|  3 ---
> > > > > > src/mjpeg-fallback.cpp|  6 +-
> > > > > > src/mjpeg-fallback.hpp|  1 +
> > > > > > src/spice-streaming-agent.cpp |  4 
> > > > > > src/static-plugin.cpp | 23 ---
> > > > > > src/static-plugin.hpp | 35
> > > > > > ---
> > > > > > 7 files changed, 6 insertions(+), 68 deletions(-)
> > > > > > delete mode 100644 src/static-plugin.cpp
> > > > > > delete mode 100644 src/static-plugin.hpp
> > > > > > 
> > > > > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > > > > index 8d5c5bd..590db1f 100644
> > > > > > --- a/src/Makefile.am
> > > > > > +++ b/src/Makefile.am
> > > > > > @@ -49,8 +49,6 @@ spice_streaming_agent_LDADD = \
> > > > > > 
> > > > > > spice_streaming_agent_SOURCES = \
> > > > > > spice-streaming-agent.cpp \
> > > > > > -   static-plugin.cpp \
> > > > > > -   static-plugin.hpp \
> > > > > > concrete-agent.cpp \
> > > > > > concrete-agent.hpp \
> > > > > > mjpeg-fallback.cpp \
> > > > > > diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> > > > > > index 873a69e..c69da43 100644
> > > > > > --- a/src/concrete-agent.cpp
> > > > > > +++ b/src/concrete-agent.cpp
> > > > > > @@ -11,7 +11,6 @@
> > > > > > #include 
> > > > > > 
> > > > > > #include "concrete-agent.hpp"
> > > > > > -#include "static-plugin.hpp"
> > > > > > 
> > > > > > using namespace std;
> > > > > > using namespace SpiceStreamingAgent;
> > > > > > @@ -58,8 +57,6 @@ void ConcreteAgent::AddOption(const char *name,
> > > > > > const
> > > > > > char *value)
> > > > > > 
> > > > > > void ConcreteAgent::LoadPlugins(const string )
> > > > > > {
> > > > > > -StaticPlugin::InitAll(*this);
> > > > > > -
> > > > > >string pattern = directory + "/*.so";
> > > > > >glob_t globbuf;
> > > > > > 
> > > > > > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > > > > > index 7c918a7..d7c676d 100644
> > > > > > --- a/src/mjpeg-fallback.cpp
> > > > > > +++ b/src/mjpeg-fallback.cpp
> > > > > > @@ -15,7 +15,6 @@
> > > > > > #include 
> > > > > > #include 
> > > > > > 
> > > > > > -#include "static-plugin.hpp"
> > > > > > #include "jpeg.hpp"
> > > > > > 
> > > > > > using namespace std;
> > > > > > @@ -191,8 +190,7 @@ SpiceVideoCodecType
> > > > > > MjpegPlugin::VideoCodecType()
> > > > > > const
> > > > > > {
> > > > > >return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> > > > > > }
> > > > > > 
> > > > > > -static bool
> > > > > > -mjpeg_plugin_init(Agent* agent)
> > > > > > +bool MjpegPlugin::Register(Agent* agent)
> > > > > > {
> > > > > >if (agent->Version() != PluginVersion)
> > > > > >return false;
> > > > > > @@ -205,5 +203,3 @@ mjpeg_plugin_init(Agent* agent)
> > > > > > 
> > > > > >return true;
> > > > > > }
> > > > > > -
> > > > > > -static StaticPlugin mjpeg_plugin(mjpeg_plugin_init);
> > > > > > diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
> > > > > > index 8044244..0e9ed6a 100644
> > > > > > --- a/src/mjpeg-fallback.hpp
> > > > > > +++ b/src/mjpeg-fallback.hpp
> > > > > > @@ -25,6 +25,7 @@ public:
> > > > > >unsigned Rank() override;
> > > > > >void ParseOptions(const ConfigureOption *options);
> > > > > >SpiceVideoCodecType VideoCodecType() const;
> > > > > > +static bool Register(Agent* agent);
> > > > > > private:
> > > > > >MjpegSettings settings = { 10, 80 };
> > > > > > };
> > > > > > diff --git a/src/spice-streaming-agent.cpp
> > > > > > b/src/spice-streaming-agent.cpp
> > > > > > index 94d9d25..8458975 100644
> > > > > > --- a/src/spice-streaming-agent.cpp
> > > > > > +++ b/src/spice-streaming-agent.cpp
> > > > > > @@ -34,6 +34,7 @@
> > > > > > 
> > > > > > #include "hexdump.h"
> > > > > > #include "concrete-agent.hpp"
> > > > > > +#include "mjpeg-fallback.hpp"
> > > > > > 
> > > > > > using namespace std;
> > > > > > using namespace 

[Spice-devel] [PATCH spice-streaming-agent] spec.in: Add BuildRequires for catch-devel

[Snir Sheriber]

---

Currently you have to have catch so no conditional is needed.

---
 spice-streaming-agent.spec.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spice-streaming-agent.spec.in b/spice-streaming-agent.spec.in
index d137840..ac8203a 100644
--- a/spice-streaming-agent.spec.in
+++ b/spice-streaming-agent.spec.in
@@ -9,6 +9,7 @@ Source0:%{name}-%{version}.tar.xz
 BuildRequires:  spice-protocol >= @SPICE_PROTOCOL_MIN_VER@
 BuildRequires:  libX11-devel libXfixes-devel
 BuildRequires:  libjpeg-turbo-devel
+BuildRequires:  catch-devel
 # we need /usr/sbin/semanage program which is available on different
 # packages depending on distribution
 Requires(post): /usr/sbin/semanage
-- 
2.14.3

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

[Spice-devel] Gitlab - 2018!

[Victor Toso]

Hi,

There was a thread in 2016 about moving to gitlab [0] which it
was not complete done. New projects have been started in gitlab
instead of freedesktop and without notice, some users are even
filling bugs there already [1].

[0] 
https://lists.freedesktop.org/archives/spice-devel/2016-September/032456.html
[1] https://gitlab.com/groups/spice/-/issues

Some issues around this is 'how to do patch review' or 'are we
going to accept PR'. IMHO, I would like to first move the code
infrastructure there while keeping our current work flow with ML.

I would like to suggest moving from bugzilla to gitlab issues
too, in order to centralize code + issues but we don't need to do
it right away.

But I don't think the current state is okay, with code and bugs
in freedesktop + gitlab.

Please, I would love some feedback with this either way
(okay/against it). If people don't want to move, we should make
gitlab's instance a mirror (like github should be, but isn't) and
use its gitlab-ci while disabling the issues/wiki, etc. and
moving to freedesktop what was created in gitlab (spice-nsis,
streaming-agent, maybe more).

Note that I did some naming changes with the gitlab repo that was
pointed out in [0]. I'm 100% okay in moving back to how it is set
in freedesktop.

Cheers,
toso


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-streaming-agent v2 2/2] explicit instead of static registration for built-in plugins

[Lukáš Hrázký]

On Fri, 2018-02-16 at 07:23 -0500, Frediano Ziglio wrote:
> > 
> > > On 6 Feb 2018, at 10:51, Frediano Ziglio  wrote:
> > > 
> > > > > 
> > > > > On 5 Feb 2018, at 15:29, Lukáš Hrázký  wrote:
> > > > > 
> > > > > The static registration (that is, having a static list of pointers to
> > > > > static plugin variables and calling a generic function in the main
> > > > > initialization code path to register them) allows to add plugins 
> > > > > without
> > > > > registering each of them explicitly.
> > > > > 
> > > > > However, in a single codebase, and having very few plugins, there is
> > > > > very little advantage to it and the tradeoff for the complexity of 
> > > > > this
> > > > > initialization is not worth it. A single call for every built-in 
> > > > > plugin
> > > > > is much more simple and clear.
> > > > > 
> > > > > Signed-off-by: Lukáš Hrázký 
> > > > > ---
> > > > > src/Makefile.am   |  2 --
> > > > > src/concrete-agent.cpp|  3 ---
> > > > > src/mjpeg-fallback.cpp|  6 +-
> > > > > src/mjpeg-fallback.hpp|  1 +
> > > > > src/spice-streaming-agent.cpp |  4 
> > > > > src/static-plugin.cpp | 23 ---
> > > > > src/static-plugin.hpp | 35 ---
> > > > > 7 files changed, 6 insertions(+), 68 deletions(-)
> > > > > delete mode 100644 src/static-plugin.cpp
> > > > > delete mode 100644 src/static-plugin.hpp
> > > > > 
> > > > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > > > index 8d5c5bd..590db1f 100644
> > > > > --- a/src/Makefile.am
> > > > > +++ b/src/Makefile.am
> > > > > @@ -49,8 +49,6 @@ spice_streaming_agent_LDADD = \
> > > > > 
> > > > > spice_streaming_agent_SOURCES = \
> > > > >   spice-streaming-agent.cpp \
> > > > > - static-plugin.cpp \
> > > > > - static-plugin.hpp \
> > > > >   concrete-agent.cpp \
> > > > >   concrete-agent.hpp \
> > > > >   mjpeg-fallback.cpp \
> > > > > diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> > > > > index 873a69e..c69da43 100644
> > > > > --- a/src/concrete-agent.cpp
> > > > > +++ b/src/concrete-agent.cpp
> > > > > @@ -11,7 +11,6 @@
> > > > > #include 
> > > > > 
> > > > > #include "concrete-agent.hpp"
> > > > > -#include "static-plugin.hpp"
> > > > > 
> > > > > using namespace std;
> > > > > using namespace SpiceStreamingAgent;
> > > > > @@ -58,8 +57,6 @@ void ConcreteAgent::AddOption(const char *name, 
> > > > > const
> > > > > char *value)
> > > > > 
> > > > > void ConcreteAgent::LoadPlugins(const string )
> > > > > {
> > > > > -StaticPlugin::InitAll(*this);
> > > > > -
> > > > >string pattern = directory + "/*.so";
> > > > >glob_t globbuf;
> > > > > 
> > > > > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > > > > index 7c918a7..d7c676d 100644
> > > > > --- a/src/mjpeg-fallback.cpp
> > > > > +++ b/src/mjpeg-fallback.cpp
> > > > > @@ -15,7 +15,6 @@
> > > > > #include 
> > > > > #include 
> > > > > 
> > > > > -#include "static-plugin.hpp"
> > > > > #include "jpeg.hpp"
> > > > > 
> > > > > using namespace std;
> > > > > @@ -191,8 +190,7 @@ SpiceVideoCodecType MjpegPlugin::VideoCodecType()
> > > > > const
> > > > > {
> > > > >return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> > > > > }
> > > > > 
> > > > > -static bool
> > > > > -mjpeg_plugin_init(Agent* agent)
> > > > > +bool MjpegPlugin::Register(Agent* agent)
> > > > > {
> > > > >if (agent->Version() != PluginVersion)
> > > > >return false;
> > > > > @@ -205,5 +203,3 @@ mjpeg_plugin_init(Agent* agent)
> > > > > 
> > > > >return true;
> > > > > }
> > > > > -
> > > > > -static StaticPlugin mjpeg_plugin(mjpeg_plugin_init);
> > > > > diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
> > > > > index 8044244..0e9ed6a 100644
> > > > > --- a/src/mjpeg-fallback.hpp
> > > > > +++ b/src/mjpeg-fallback.hpp
> > > > > @@ -25,6 +25,7 @@ public:
> > > > >unsigned Rank() override;
> > > > >void ParseOptions(const ConfigureOption *options);
> > > > >SpiceVideoCodecType VideoCodecType() const;
> > > > > +static bool Register(Agent* agent);
> > > > > private:
> > > > >MjpegSettings settings = { 10, 80 };
> > > > > };
> > > > > diff --git a/src/spice-streaming-agent.cpp
> > > > > b/src/spice-streaming-agent.cpp
> > > > > index 94d9d25..8458975 100644
> > > > > --- a/src/spice-streaming-agent.cpp
> > > > > +++ b/src/spice-streaming-agent.cpp
> > > > > @@ -34,6 +34,7 @@
> > > > > 
> > > > > #include "hexdump.h"
> > > > > #include "concrete-agent.hpp"
> > > > > +#include "mjpeg-fallback.hpp"
> > > > > 
> > > > > using namespace std;
> > > > > using namespace SpiceStreamingAgent;
> > > > > @@ -489,6 +490,9 @@ int main(int argc, char* argv[])
> > > > >}
> > > > >}
> > > > > 
> > > > > +// register built-in plugins
> > > > > +MjpegPlugin::Register();
> > > > > +
> > > > >agent.LoadPlugins(PLUGINSDIR);
> > > > > 
> > > > >

Re: [Spice-devel] [spice-gtk v1 2/2] channel-display: use libva to set preferred video codecs

[Victor Toso]

Hi,

On Mon, Feb 19, 2018 at 02:58:22PM +0100, Christophe Fergeau wrote:
> On Thu, Feb 15, 2018 at 10:05:05AM +0100, Victor Toso wrote:
> > From: Victor Toso 
> > 
> > By using the detection of which video codecs (and profiles!) the
> > hardware supports with VA-API capable driver in previous commit.
> > 
> > Signed-off-by: Victor Toso 
> > ---
> >  src/channel-display.c | 33 -
> >  1 file changed, 28 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 45fe38c..7f47ae7 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -175,6 +175,8 @@ static void spice_display_channel_finalize(GObject 
> > *object)
> >  
> > G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize(object);
> >  }
> >  
> > +static void spice_display_send_client_preferred_video_codecs(SpiceChannel 
> > *channel, const GArray *codecs);
> > +
> >  static void spice_display_channel_constructed(GObject *object)
> >  {
> >  SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(object)->priv;
> > @@ -572,6 +574,9 @@ static void 
> > spice_display_send_client_preferred_video_codecs(SpiceChannel *chann
> >  SpiceMsgOut *out;
> >  SpiceMsgcDisplayPreferredVideoCodecType *msg;
> >  
> > +if (codecs == NULL || codecs->len == 0)
> > +return;
> > +
> >  msg = g_malloc0(sizeof(SpiceMsgcDisplayPreferredVideoCodecType) +
> >  (sizeof(SpiceVideoCodecType) * codecs->len));
> >  msg->num_of_codecs = codecs->len;
> > @@ -615,7 +620,9 @@ void 
> > spice_display_change_preferred_video_codec_type(SpiceChannel *channel, gint
> >   */
> >  void spice_display_channel_change_preferred_video_codec_type(SpiceChannel 
> > *channel, gint codec_type)
> >  {
> > +SpiceSession *session;
> >  GArray *codecs;
> > +const GArray *hwa_codecs;
> >  
> >  g_return_if_fail(SPICE_IS_DISPLAY_CHANNEL(channel));
> >  g_return_if_fail(codec_type >= SPICE_VIDEO_CODEC_TYPE_MJPEG &&
> > @@ -626,13 +633,23 @@ void 
> > spice_display_channel_change_preferred_video_codec_type(SpiceChannel *chann
> >  return;
> >  }
> >  
> > -/* FIXME: We should detect video codecs that client machine can do hw
> > - * decoding, store this information (as GArray) and send it to the 
> > server.
> > - * This array can be rearranged to have @codec_type in the front 
> > (which is
> > - * the preferred for the client side) */
> > -CHANNEL_DEBUG(channel, "changing preferred video codec type to %s", 
> > gst_opts[codec_type].name);
> > +session = spice_channel_get_session(channel);
> > +hwa_codecs = spice_session_get_hw_accel_video_codecs(session);
> > +
> >  codecs = g_array_new(FALSE, FALSE, sizeof(gint));
> >  g_array_append_val(codecs, codec_type);
> > +if (hwa_codecs != NULL) {
> > +gint i;
> > +for (i = 0; i < hwa_codecs->len; i++) {
> > +gint hwa_codec_type = g_array_index(hwa_codecs, gint, i);
> > +if (hwa_codec_type == codec_type)
> > +continue;
> > +
> > +g_array_append_val(codecs, hwa_codec_type);
> > +}
> > +}
> > +
> > +CHANNEL_DEBUG(channel, "changing preferred video codec type to %s", 
> > gst_opts[codec_type].name);
> 
> So this is unconditionally appending all hw accelerated codecs
> to the preferred video codec type? If user asks for codec A,
> and it's not available server side, I assume before we were
> getting an error/a blank screen, and now we'd be falling back
> to codec B or C?

Okay, this is actually pretty complex with some support lacking
yet in the host.

First, the client's request is just a matter of preference,
really, it should not mean that its preference is absolute.
Instead, this should be used as a hint to the server on what
should be best to encode a video stream with, so, if client
requests A but server does not have it, no stream will ever be
encoded with A.

Second, if the current stream is set with A and for some reason
there is a problem, based on stream-report (like all frames
dropped), server should fall back to another video-codec. I do
expect glitches, black stream, etc. depending on how that
video-codec + parameters handle errors.

Third, what I mean at the beginning, we still lack support in the
server+qemu on how to manage the *host preference* for encoding,
which means that today, with upstream, client gets what it wants
if server can encode with it but it'll mostly likely change
in near future.

Finally, you are right that we don't check at the client about
server's capabilities on encoding. Server does not set its
SPICE_DISPLAY_CAP_CODEC_*, only client does that. I think it
makes sense the server to set it too, so we can avoid requesting
something that server cannot handle.

But yeah, this patches are kinda ignoring the video-codec caps
and I'll fix it in the next version!

Thanks again 

Re: [Spice-devel] [PATCH 2/2] Eliminate signed/unsigned warning

[Christophe Fergeau]

On Mon, Feb 19, 2018 at 03:06:40PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 19 Feb 2018, at 13:24, Christophe Fergeau  wrote:
> > 
> > On Fri, Feb 16, 2018 at 04:23:06PM +0100, Christophe de Dinechin wrote:
> >> From: Christophe de Dinechin 
> >> 
> >> Without this, GCC complains about signed / unsigned comparisons:
> >> 
> >> mjpeg-fallback.cpp:121:24: warning: comparison between signed and unsigned 
> >> integer expressions [-Wsign-compare]
> >> if (win_info.width != last_width || win_info.height != last_height) {
> >> ~~~^
> > 
> > Are you getting this using the default CXXFLAGS?
> 
> No, this was a side effect of tracking some other warning.

Would be nice to mention this in the commit log then, while we want the
build with the default flags to be warning-free, fixing additional
warnings will definitely be considered on a case by case basis.

> 
> 
> > Here I seem to be getting -Wno-sign-compare by default.
> 
> Why is this silenced?

This probably was not intentionally silenced, but enabling it would be a
good complement to this patch if this warning is deemed useful...

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] [spice-gtk v1 1/2] Use libva to check video hardware accel capabilities

[Victor Toso]

Hi,

On Mon, Feb 19, 2018 at 02:44:04PM +0100, Christophe Fergeau wrote:
> Hey,
> 
> On Thu, Feb 15, 2018 at 10:05:04AM +0100, Victor Toso wrote:
> > From: Victor Toso 
> > 
> > Libva is an implementation for VA-API.
> > 
> > This can be used to automatically send to the server the
> > preferred video codecs as the client would prefer streams
> > with video codecs that can be decoded with gpu support.
> > 
> > We can also use the profiles to detect and set upper limit for
> > video streams quality.
> > e.g: Don't start UHD video stream if client's hardware don't
> > support it.
> > 
> > This patch makes usage of libva in spice-session and exposes this
> > information to all available channel-displays with the internal
> > function spice_session_get_hw_accel_video_codecs()
> 
> This assumes that HW accelerated video decoding is going to go
> through libva when using GStreamer.

Not really. I think it is fairly possible to query hw
capabilities with libva while doing hardware decoding without
libva (nvdec element for NVidia, for instance) but I haven't
tested that.

> I assume it's not possible to directly query GStreamer to know
> what it can hardware decode?

Not really, although I have asked [0] - We still need some
support from GStreamer to detect if we are doing hardware or
software decoding (somehow!) without having to check elements as
this does not scale well, I think.

[0] https://bugzilla.gnome.org/show_bug.cgi?id=782741

Note also that libva here in spice-gtk could tell that we do have
support to vp8/vp9/h264 hw decoding but in fact there is no
element installed in GStreamer to do that.

I plan to send a v2 including the GStreamer check to, at least,
be sure that we are sending the preferred video codecs based on
hw capability + gstreamer support + the fixes from this review
soon :)

> 
> > 
> > Signed-off-by: Victor Toso 
> > ---
> >  configure.ac |  20 +++
> >  src/Makefile.am  |  12 
> >  src/spice-session-priv.h |   1 +
> >  src/spice-session.c  | 139 
> > +++
> >  4 files changed, 172 insertions(+)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 2a14055..0b0db0f 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -321,6 +321,25 @@ AC_SUBST(Z_LIBS)
> >  SPICE_CHECK_SMARTCARD
> >  AM_CONDITIONAL([WITH_SMARTCARD], [test "x$have_smartcard" = "xyes"])
> >  
> > +AC_ARG_ENABLE([libva],
> > +  AS_HELP_STRING([--enable-libva=@<:@auto/yes/no@:>@], [Enable auto 
> > detection of hardware accelerate video decoding support 
> > @<:@default=auto@:>@]),
> > +  [],
> > +  [enable_libva="auto"])
> > +AS_IF([test "x$enable_libva" != "xno"],
> > +  [PKG_CHECK_MODULES(LIBVA, [libva >= 1.0.0],
> > + [AC_DEFINE([HAVE_LIBVA], 1, [Have libva support?])
> > +  enable_libva="yes"],
> > + [AS_IF([test "x$enable_libva" = "xyes"],
> > +AC_MSG_ERROR([Auto detection of hardware accelerated video 
> > decoding explicitly requested, but some required packages are not 
> > available]))
> > +  enable_libva="no"
> > +  ])
> > +PKG_CHECK_MODULES([LIBVA_X11], [libva-x11 >= 1.0.0])
> > +PKG_CHECK_MODULES([LIBVA_WAYLAND], [libva-wayland >= 1.0.0])
> > +PKG_CHECK_MODULES([GDK_X11], [gdk-x11-3.0])
> > +PKG_CHECK_MODULES([GDK_WAYLAND], [gdk-wayland-3.0])
> 
> I don't think we'll necessarily have all of these installed?
> PKG_CHECK_MODULES will error out if any of these is missing.

I'll test and remove them, thanks!

> 
> > +])
> > +AM_CONDITIONAL([HAVE_LIBVA], [test "x$enable_libva" = "xyes"])
> > +
> >  AC_ARG_ENABLE([usbredir],
> >AS_HELP_STRING([--enable-usbredir=@<:@auto/yes/no@:>@],
> >   [Enable usbredir support @<:@default=auto@:>@]),
> > @@ -635,6 +654,7 @@ AC_MSG_NOTICE([
> >  DBus: ${have_dbus}
> >  WebDAV support:   ${have_phodav}
> >  LZ4 support:  ${have_lz4}
> > +Libva support:${enable_libva}
> >  
> >  Now type 'make' to build $PACKAGE
> >  
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 4b6e46d..7b74220 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -75,6 +75,9 @@ SPICE_COMMON_CPPFLAGS =   
> > \
> > $(PIXMAN_CFLAGS)\
> > $(PULSE_CFLAGS) \
> > $(GTK_CFLAGS)   \
> > +   $(GDK_CFLAGS)   \
> > +   $(GDK_X11_CFLAGS)   \
> > +   $(GDK_WAYLAND_CFLAGS)   \
> > $(CAIRO_CFLAGS) \
> > $(GLIB2_CFLAGS) \
> > $(GIO_CFLAGS)   \
> > @@ -88,6 +91,9 @@ SPICE_COMMON_CPPFLAGS =

Re: [Spice-devel] [PATCH spice-server v3 0/2] Implement cursor for streaming device

[Christophe Fergeau]

On Tue, Feb 13, 2018 at 01:58:07PM +, Frediano Ziglio wrote:
> Changes since v2:
> - shrink buffer after the message is handled.
> 
> Frediano Ziglio (2):
>   stream-device: handle cursor from device
>   stream-device: Implement mouse movement
> 
>  server/stream-device.c | 201 
> +++--
>  1 file changed, 194 insertions(+), 7 deletions(-)

Sorry, but no cover letter, and single line logs for commits changes
that many lines is not desirable, see
https://www.berrange.com/posts/2012/06/27/thoughts-on-improving-openstack-git-commit-practicehistory/
for example:
"The commit message must contain all the information required to fully
understand & review the patch for correctness. Less is not more. More is
more."

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 v2] Add support for resolution 800x600

[Yuri Benditovich]

https://bugzilla.redhat.com/show_bug.cgi?id=1477492
https://docs.microsoft.com/en-us/windows-hardware/design/minimum/minimum-hardware-requirements-overview
requires 800x600 to be supported.

Signed-off-by: Yuri Benditovich 
---
 qxldod/QxlDod.cpp | 16 
 qxldod/QxlDod.h   |  6 --
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
index 4f508bd..f74f54e 100755
--- a/qxldod/QxlDod.cpp
+++ b/qxldod/QxlDod.cpp
@@ -2579,8 +2579,8 @@ NTSTATUS VgaDevice::GetModeList(DXGK_DISPLAY_INFORMATION* 
pDispInfo)
 {
 m_ModeNumbers[SuitableModeCount] = ModeTemp;
 SetVideoModeInfo(SuitableModeCount, );
-if (tmpModeInfo.XResolution == MIN_WIDTH_SIZE &&
-tmpModeInfo.YResolution == MIN_HEIGHT_SIZE)
+if (tmpModeInfo.XResolution == INITIAL_WIDTH &&
+tmpModeInfo.YResolution == INITIAL_HEIGHT)
 {
 m_CurrentMode = (USHORT)SuitableModeCount;
 }
@@ -3186,8 +3186,8 @@ NTSTATUS QxlDevice::GetModeList(DXGK_DISPLAY_INFORMATION* 
pDispInfo)
 UINT BitsPerPixel = BPPFromPixelFormat(pDispInfo->ColorFormat);
 if (Width == 0 || Height == 0 || BitsPerPixel != QXL_BPP)
 {
-Width = MIN_WIDTH_SIZE;
-Height = MIN_HEIGHT_SIZE;
+Width = INITIAL_WIDTH;
+Height = INITIAL_HEIGHT;
 BitsPerPixel = QXL_BPP;
 }
 
@@ -3206,8 +3206,8 @@ NTSTATUS QxlDevice::GetModeList(DXGK_DISPLAY_INFORMATION* 
pDispInfo)
 {
 m_ModeNumbers[SuitableModeCount] = SuitableModeCount;
 SetVideoModeInfo(SuitableModeCount, tmpModeInfo);
-if (tmpModeInfo->x_res == MIN_WIDTH_SIZE &&
-tmpModeInfo->y_res == MIN_HEIGHT_SIZE)
+if (tmpModeInfo->x_res == INITIAL_WIDTH &&
+tmpModeInfo->y_res == INITIAL_HEIGHT)
 {
 m_CurrentMode = SuitableModeCount;
 }
@@ -5147,8 +5147,8 @@ NTSTATUS 
HwDeviceInterface::AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& DispInf
 if (DispInfo.Width == 0)
 {
 DispInfo.ColorFormat = D3DDDIFMT_A8R8G8B8;
-DispInfo.Width = MIN_WIDTH_SIZE;
-DispInfo.Height = MIN_HEIGHT_SIZE;
+DispInfo.Width = INITIAL_WIDTH;
+DispInfo.Height = INITIAL_HEIGHT;
 DispInfo.Pitch = DispInfo.Width * 
BPPFromPixelFormat(DispInfo.ColorFormat) / BITS_PER_BYTE;
 DispInfo.TargetId = 0;
 }
diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
index 695b83a..eb6b78d 100755
--- a/qxldod/QxlDod.h
+++ b/qxldod/QxlDod.h
@@ -19,8 +19,10 @@
 #define BITS_PER_BYTE  8
 
 #define POINTER_SIZE   64
-#define MIN_WIDTH_SIZE 1024
-#define MIN_HEIGHT_SIZE768
+#define MIN_WIDTH_SIZE 800
+#define MIN_HEIGHT_SIZE600
+#define INITIAL_WIDTH  1024
+#define INITIAL_HEIGHT 768
 #define QXL_BPP32
 #define VGA_BPP24
 
-- 
2.7.0.windows.1

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

Re: [Spice-devel] [PATCH 2/2] Eliminate signed/unsigned warning

[Christophe de Dinechin]

> On 19 Feb 2018, at 13:24, Christophe Fergeau  wrote:
> 
> On Fri, Feb 16, 2018 at 04:23:06PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> Without this, GCC complains about signed / unsigned comparisons:
>> 
>> mjpeg-fallback.cpp:121:24: warning: comparison between signed and unsigned 
>> integer expressions [-Wsign-compare]
>> if (win_info.width != last_width || win_info.height != last_height) {
>> ~~~^
> 
> Are you getting this using the default CXXFLAGS?

No, this was a side effect of tracking some other warning.


> Here I seem to be getting
> -Wno-sign-compare by default.

Why is this silenced? There aren’t that many of them, and implicit int 
promotion makes some scenarios risky, e.g signed byte vs unsigned byte.

Hmm, I checked, and the “signed-compare” of GCC does not warn in the dangerous 
cases, only in the harmless ones… Weird.

No warning for this, where the result is “surprisingly” false because of int 
promotion (comparison is false), even with -Wall, -Wextra or -Wpedantic
signed char i = -3;
unsigned char j = -3;
printf("i=%x j=%x i==j=%d\n", i, j, i==j);

But a warning for this where the result is true as expected:
signed char i = -3;
unsigned char j = -3;
printf("i=%x j=%x i==j=%d\n", i, j, i==j);

Really strange. Well, if that’s how the option behaves, then -Wno-sign-compare 
seems harmless.

But does anybody understand the rationale for this? I’m a bit puzzled.

> 
> Christophe
> 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> src/mjpeg-fallback.cpp| 2 +-
>> src/spice-streaming-agent.cpp | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
>> index 634864f..53804d9 100644
>> --- a/src/mjpeg-fallback.cpp
>> +++ b/src/mjpeg-fallback.cpp
>> @@ -57,7 +57,7 @@ private:
>> std::vector frame;
>> 
>> // last frame sizes
>> -uint32_t last_width = ~0u, last_height = ~0u;
>> +int last_width = ~0u, last_height = ~0u;
>> // last time before capture
>> uint64_t last_time = 0;
>> };
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 4ec5e42..27b26a4 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -106,7 +106,7 @@ static int read_command_from_device(void)
>> return 0; // return -1;
>> }
>> n = read(streamfd, , hdr.size);
>> -if (n != hdr.size) {
>> +if (n != (int) hdr.size) {
>> syslog(LOG_WARNING,
>>"read command from device FAILED -- read %d expected %d\n",
>>n, hdr.size);
>> -- 
>> 2.13.5 (Apple Git-94)
>> 
>> ___
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

Re: [Spice-devel] [spice-gtk v1 2/2] channel-display: use libva to set preferred video codecs

[Christophe Fergeau]

On Thu, Feb 15, 2018 at 10:05:05AM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> By using the detection of which video codecs (and profiles!) the
> hardware supports with VA-API capable driver in previous commit.
> 
> Signed-off-by: Victor Toso 
> ---
>  src/channel-display.c | 33 -
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 45fe38c..7f47ae7 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -175,6 +175,8 @@ static void spice_display_channel_finalize(GObject 
> *object)
>  G_OBJECT_CLASS(spice_display_channel_parent_class)->finalize(object);
>  }
>  
> +static void spice_display_send_client_preferred_video_codecs(SpiceChannel 
> *channel, const GArray *codecs);
> +
>  static void spice_display_channel_constructed(GObject *object)
>  {
>  SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(object)->priv;
> @@ -572,6 +574,9 @@ static void 
> spice_display_send_client_preferred_video_codecs(SpiceChannel *chann
>  SpiceMsgOut *out;
>  SpiceMsgcDisplayPreferredVideoCodecType *msg;
>  
> +if (codecs == NULL || codecs->len == 0)
> +return;
> +
>  msg = g_malloc0(sizeof(SpiceMsgcDisplayPreferredVideoCodecType) +
>  (sizeof(SpiceVideoCodecType) * codecs->len));
>  msg->num_of_codecs = codecs->len;
> @@ -615,7 +620,9 @@ void 
> spice_display_change_preferred_video_codec_type(SpiceChannel *channel, gint
>   */
>  void spice_display_channel_change_preferred_video_codec_type(SpiceChannel 
> *channel, gint codec_type)
>  {
> +SpiceSession *session;
>  GArray *codecs;
> +const GArray *hwa_codecs;
>  
>  g_return_if_fail(SPICE_IS_DISPLAY_CHANNEL(channel));
>  g_return_if_fail(codec_type >= SPICE_VIDEO_CODEC_TYPE_MJPEG &&
> @@ -626,13 +633,23 @@ void 
> spice_display_channel_change_preferred_video_codec_type(SpiceChannel *chann
>  return;
>  }
>  
> -/* FIXME: We should detect video codecs that client machine can do hw
> - * decoding, store this information (as GArray) and send it to the 
> server.
> - * This array can be rearranged to have @codec_type in the front (which 
> is
> - * the preferred for the client side) */
> -CHANNEL_DEBUG(channel, "changing preferred video codec type to %s", 
> gst_opts[codec_type].name);
> +session = spice_channel_get_session(channel);
> +hwa_codecs = spice_session_get_hw_accel_video_codecs(session);
> +
>  codecs = g_array_new(FALSE, FALSE, sizeof(gint));
>  g_array_append_val(codecs, codec_type);
> +if (hwa_codecs != NULL) {
> +gint i;
> +for (i = 0; i < hwa_codecs->len; i++) {
> +gint hwa_codec_type = g_array_index(hwa_codecs, gint, i);
> +if (hwa_codec_type == codec_type)
> +continue;
> +
> +g_array_append_val(codecs, hwa_codec_type);
> +}
> +}
> +
> +CHANNEL_DEBUG(channel, "changing preferred video codec type to %s", 
> gst_opts[codec_type].name);

So this is unconditionally appending all hw accelerated codecs to the
preferred video codec type? If user asks for codec A, and it's not
available server side, I assume before we were getting an error/a blank
screen, and now we'd be falling back to codec B or C?

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] [RFC PATCH 1/1] separate and encapsulate the agent business code

[Christophe de Dinechin]

> On 19 Feb 2018, at 14:05, Christophe Fergeau  wrote:
> 
> On Wed, Feb 14, 2018 at 10:13:41PM +0100, Christophe de Dinechin wrote:
 I’d write “config.h”. No reason to ever look config.h in system headers.
>>> 
>>> The reason for the <> is described in [1], 4th paragraph. I've
>>> mentioned it during the previous discussion and didn't get any comment
>>> on it IIRC. Either is fine by me, I don't plan to introduce another
>>> file named 'config.h' anywhere in the source tree.
>>> 
>>> [1] 
>>> https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Configuration-Headers.html
>> 
>> That rationale is remarkably inconsistent with the generated makefiles, 
>> which build for example with:
>> 
>> -I. -I..  -I.. -I.. -I/usr/include/pixman-1  -I/usr/include/cacard 
>> -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 
>> -I/usr/include/nspr4   -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include 
>> -pthread  -I/usr/include/opus   -DG_LOG_DOMAIN=\"Spice\" 
>> -I/usr/local/include/spice-1  
>> 
>> or for the streaming agent:
>> 
>> -I. -I..  -DSPICE_STREAMING_AGENT_PROGRAM -I../include 
>> -DPLUGINSDIR=\"/usr/local/lib/spice-streaming-agent/plugins\" 
>> -I/usr/local/include/spice-1  
>> 
>> So you -I. first anyway, and you would prefer the local config.h in any 
>> case. “config.h” just lets compiler find it without looking up at the 
>> command-line options.
> 
> As far as I can tell, this -I. -I.. corresponds to -I$(builddir) -I$(srcdir)
> 
> #include "config.h" would look into $(srcdir) first regardless of these
> -I flags, #include  will look into -I$(builddir) first, which
> is what is documented in the link given by Lukas.

Uri also pointed out more or less the same thing.

Currently, we have a fair number of both.

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

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

Re: [Spice-devel] [PATCH] Add support for resolution 800x600

[Christophe de Dinechin]

> On 19 Feb 2018, at 14:35, Frediano Ziglio  wrote:
> 
>> 
>> https://bugzilla.redhat.com/show_bug.cgi?id=1477492
>> https://docs.microsoft.com/en-us/windows-hardware/design/minimum/minimum-hardware-requirements-overview
>> requires 800x600 to be supported.
>> 
>> Signed-off-by: Yuri Benditovich 
>> ---
>> qxldod/QxlDod.cpp | 28 ++--
>> qxldod/QxlDod.h   |  6 --
>> 2 files changed, 18 insertions(+), 16 deletions(-)
>> 
>> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
>> index 4f508bd..ee97b09 100755
>> --- a/qxldod/QxlDod.cpp
>> +++ b/qxldod/QxlDod.cpp
>> @@ -2572,15 +2572,15 @@ NTSTATUS
>> VgaDevice::GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo)
>> 
>> DbgPrint(TRACE_LEVEL_INFORMATION, ("ModeTemp = 0x%X %dx%d@%d\n",
>> ModeTemp, tmpModeInfo.XResolution, tmpModeInfo.YResolution,
>> tmpModeInfo.BitsPerPixel));
>> 
>> -if (tmpModeInfo.XResolution >= MIN_WIDTH_SIZE &&
>> -tmpModeInfo.YResolution >= MIN_HEIGHT_SIZE &&
>> +if (tmpModeInfo.XResolution >= MINIMAL_WIDTH &&
>> +tmpModeInfo.YResolution >= MINIMAL_HEIGHT &&
>> tmpModeInfo.BitsPerPixel == BitsPerPixel &&
>> tmpModeInfo.PhysBasePtr != 0)
>> {
>> m_ModeNumbers[SuitableModeCount] = ModeTemp;
>> SetVideoModeInfo(SuitableModeCount, );
>> -if (tmpModeInfo.XResolution == MIN_WIDTH_SIZE &&
>> -tmpModeInfo.YResolution == MIN_HEIGHT_SIZE)
>> +if (tmpModeInfo.XResolution == INITIAL_WIDTH &&
>> +tmpModeInfo.YResolution == INITIAL_HEIGHT)
>> {
>> m_CurrentMode = (USHORT)SuitableModeCount;
>> }
>> @@ -3186,8 +3186,8 @@ NTSTATUS
>> QxlDevice::GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo)
>> UINT BitsPerPixel = BPPFromPixelFormat(pDispInfo->ColorFormat);
>> if (Width == 0 || Height == 0 || BitsPerPixel != QXL_BPP)
>> {
>> -Width = MIN_WIDTH_SIZE;
>> -Height = MIN_HEIGHT_SIZE;
>> +Width = MINIMAL_WIDTH;
>> +Height = MINIMAL_HEIGHT;
>> BitsPerPixel = QXL_BPP;
>> }
>> 
> 
> Why not setting to the initial values here?
> 
>> @@ -3200,14 +3200,14 @@ NTSTATUS
>> QxlDevice::GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo)
>> 
>> DbgPrint(TRACE_LEVEL_INFORMATION, ("%s: modes[%d] x_res = %d, y_res
>> = %d, bits = %d BitsPerPixel = %d\n", __FUNCTION__, CurrentMode,
>> tmpModeInfo->x_res, tmpModeInfo->y_res, tmpModeInfo->bits,
>> BitsPerPixel));
>> 
>> -if (tmpModeInfo->x_res >= MIN_WIDTH_SIZE &&
>> -tmpModeInfo->y_res >= MIN_HEIGHT_SIZE &&
>> +if (tmpModeInfo->x_res >= MINIMAL_WIDTH &&
>> +tmpModeInfo->y_res >= MINIMAL_HEIGHT &&
>> tmpModeInfo->bits == QXL_BPP)
>> {
>> m_ModeNumbers[SuitableModeCount] = SuitableModeCount;
>> SetVideoModeInfo(SuitableModeCount, tmpModeInfo);
>> -if (tmpModeInfo->x_res == MIN_WIDTH_SIZE &&
>> -tmpModeInfo->y_res == MIN_HEIGHT_SIZE)
>> +if (tmpModeInfo->x_res == INITIAL_WIDTH &&
>> +tmpModeInfo->y_res == INITIAL_HEIGHT)
>> {
>> m_CurrentMode = SuitableModeCount;
>> }
>> @@ -4883,9 +4883,9 @@ NTSTATUS
>> QxlDevice::SetCustomDisplay(QXLEscapeSetCustomDisplay* custom_display)
>> UINT yres = custom_display->yres;
>> UINT bpp = QXL_BPP;
>> DbgPrint(TRACE_LEVEL_WARNING, ("%s - %d (%dx%d#%d)\n", __FUNCTION__,
>> m_Id, xres, yres, bpp));
>> -if (xres < MIN_WIDTH_SIZE || yres < MIN_HEIGHT_SIZE) {
>> +if (xres < MINIMAL_WIDTH || yres < MINIMAL_HEIGHT) {
>> DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: (%dx%d#%d) less than (%dx%d)\n",
>> __FUNCTION__,
>> -xres, yres, bpp, MIN_WIDTH_SIZE, MIN_HEIGHT_SIZE));
>> +xres, yres, bpp, MINIMAL_WIDTH, MINIMAL_HEIGHT));
>> }
>> m_CustomMode =(USHORT) ((m_CustomMode == m_ModeCount-1)?  m_ModeCount -
>> 2 : m_ModeCount - 1);
>> 
>> @@ -5147,8 +5147,8 @@ NTSTATUS
>> HwDeviceInterface::AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& DispInf
>> if (DispInfo.Width == 0)
>> {
>> DispInfo.ColorFormat = D3DDDIFMT_A8R8G8B8;
>> -DispInfo.Width = MIN_WIDTH_SIZE;
>> -DispInfo.Height = MIN_HEIGHT_SIZE;
>> +DispInfo.Width = INITIAL_WIDTH;
>> +DispInfo.Height = INITIAL_HEIGHT;
>> DispInfo.Pitch = DispInfo.Width *
>> BPPFromPixelFormat(DispInfo.ColorFormat) / BITS_PER_BYTE;
>> DispInfo.TargetId = 0;
>> }
>> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
>> index 695b83a..ba7e6a7 100755
>> --- a/qxldod/QxlDod.h
>> +++ b/qxldod/QxlDod.h
>> @@ -19,8 +19,10 @@
>> #define BITS_PER_BYTE  8
>> 
>> #define POINTER_SIZE   64
>> -#define MIN_WIDTH_SIZE 1024
>> -#define MIN_HEIGHT_SIZE768
>> 

Re: [Spice-devel] [spice-gtk v1 1/2] Use libva to check video hardware accel capabilities

[Christophe Fergeau]

Hey,

On Thu, Feb 15, 2018 at 10:05:04AM +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> Libva is an implementation for VA-API.
> 
> This can be used to automatically send to the server the
> preferred video codecs as the client would prefer streams
> with video codecs that can be decoded with gpu support.
> 
> We can also use the profiles to detect and set upper limit for
> video streams quality.
> e.g: Don't start UHD video stream if client's hardware don't
> support it.
> 
> This patch makes usage of libva in spice-session and exposes this
> information to all available channel-displays with the internal
> function spice_session_get_hw_accel_video_codecs()

This assumes that HW accelerated video decoding is going to go through
libva when using GStreamer. I assume it's not possible to directly query
GStreamer to know what it can hardware decode?

> 
> Signed-off-by: Victor Toso 
> ---
>  configure.ac |  20 +++
>  src/Makefile.am  |  12 
>  src/spice-session-priv.h |   1 +
>  src/spice-session.c  | 139 
> +++
>  4 files changed, 172 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index 2a14055..0b0db0f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -321,6 +321,25 @@ AC_SUBST(Z_LIBS)
>  SPICE_CHECK_SMARTCARD
>  AM_CONDITIONAL([WITH_SMARTCARD], [test "x$have_smartcard" = "xyes"])
>  
> +AC_ARG_ENABLE([libva],
> +  AS_HELP_STRING([--enable-libva=@<:@auto/yes/no@:>@], [Enable auto 
> detection of hardware accelerate video decoding support 
> @<:@default=auto@:>@]),
> +  [],
> +  [enable_libva="auto"])
> +AS_IF([test "x$enable_libva" != "xno"],
> +  [PKG_CHECK_MODULES(LIBVA, [libva >= 1.0.0],
> + [AC_DEFINE([HAVE_LIBVA], 1, [Have libva support?])
> +  enable_libva="yes"],
> + [AS_IF([test "x$enable_libva" = "xyes"],
> +AC_MSG_ERROR([Auto detection of hardware accelerated video 
> decoding explicitly requested, but some required packages are not available]))
> +  enable_libva="no"
> +  ])
> +PKG_CHECK_MODULES([LIBVA_X11], [libva-x11 >= 1.0.0])
> +PKG_CHECK_MODULES([LIBVA_WAYLAND], [libva-wayland >= 1.0.0])
> +PKG_CHECK_MODULES([GDK_X11], [gdk-x11-3.0])
> +PKG_CHECK_MODULES([GDK_WAYLAND], [gdk-wayland-3.0])

I don't think we'll necessarily have all of these installed?
PKG_CHECK_MODULES will error out if any of these is missing.

> +])
> +AM_CONDITIONAL([HAVE_LIBVA], [test "x$enable_libva" = "xyes"])
> +
>  AC_ARG_ENABLE([usbredir],
>AS_HELP_STRING([--enable-usbredir=@<:@auto/yes/no@:>@],
>   [Enable usbredir support @<:@default=auto@:>@]),
> @@ -635,6 +654,7 @@ AC_MSG_NOTICE([
>  DBus: ${have_dbus}
>  WebDAV support:   ${have_phodav}
>  LZ4 support:  ${have_lz4}
> +Libva support:${enable_libva}
>  
>  Now type 'make' to build $PACKAGE
>  
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 4b6e46d..7b74220 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -75,6 +75,9 @@ SPICE_COMMON_CPPFLAGS = 
> \
>   $(PIXMAN_CFLAGS)\
>   $(PULSE_CFLAGS) \
>   $(GTK_CFLAGS)   \
> + $(GDK_CFLAGS)   \
> + $(GDK_X11_CFLAGS)   \
> + $(GDK_WAYLAND_CFLAGS)   \
>   $(CAIRO_CFLAGS) \
>   $(GLIB2_CFLAGS) \
>   $(GIO_CFLAGS)   \
> @@ -88,6 +91,9 @@ SPICE_COMMON_CPPFLAGS = 
> \
>   $(GUDEV_CFLAGS) \
>   $(SOUP_CFLAGS)  \
>   $(PHODAV_CFLAGS)\
> + $(LIBVA_CFLAGS) \
> + $(LIBVA_X11_CFLAGS) \
> + $(LIBVA_WAYLAND_CFLAGS) \
>   $(X11_CFLAGS)   \
>   $(LZ4_CFLAGS)   \
>   $(NULL)
> @@ -195,6 +201,12 @@ libspice_client_glib_2_0_la_LIBADD = 
> \
>   $(USBREDIR_LIBS)\
>   $(GUDEV_LIBS)   \
>   $(PHODAV_LIBS)  \
> + $(GDK_LIBS) \
> + $(GDK_X11_LIBS) \
> + $(GDK_WAYLAND_LIBS) \

Re: [Spice-devel] [PATCH spice-streaming-agent] Make evaluation order more readable

[Victor Toso]

On Mon, Feb 19, 2018 at 01:26:01PM +, Frediano Ziglio wrote:
> As a first sight the XXX = YYY != 0 syntax can be confusing,
> add parenthesis to make clear the order.
> 
> Signed-off-by: Frediano Ziglio 
Acked-by: Victor Toso 

> ---
>  src/spice-streaming-agent.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 4ec5e42..e01de6c 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -112,7 +112,7 @@ static int read_command_from_device(void)
> n, hdr.size);
>  return -1;
>  }
> -streaming_requested = msg[0] != 0; /* num_codecs */
> +streaming_requested = (msg[0] != 0); /* num_codecs */
>  syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
> streaming_requested ? "START" : "STOP");
>  client_codecs.clear();
> -- 
> 2.14.3
> 
> ___
> 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] Add support for resolution 800x600

[Frediano Ziglio]

> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1477492
> https://docs.microsoft.com/en-us/windows-hardware/design/minimum/minimum-hardware-requirements-overview
> requires 800x600 to be supported.
> 
> Signed-off-by: Yuri Benditovich 
> ---
>  qxldod/QxlDod.cpp | 28 ++--
>  qxldod/QxlDod.h   |  6 --
>  2 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index 4f508bd..ee97b09 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -2572,15 +2572,15 @@ NTSTATUS
> VgaDevice::GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo)
>  
>  DbgPrint(TRACE_LEVEL_INFORMATION, ("ModeTemp = 0x%X %dx%d@%d\n",
>  ModeTemp, tmpModeInfo.XResolution, tmpModeInfo.YResolution,
>  tmpModeInfo.BitsPerPixel));
>  
> -if (tmpModeInfo.XResolution >= MIN_WIDTH_SIZE &&
> -tmpModeInfo.YResolution >= MIN_HEIGHT_SIZE &&
> +if (tmpModeInfo.XResolution >= MINIMAL_WIDTH &&
> +tmpModeInfo.YResolution >= MINIMAL_HEIGHT &&
>  tmpModeInfo.BitsPerPixel == BitsPerPixel &&
>  tmpModeInfo.PhysBasePtr != 0)
>  {
>  m_ModeNumbers[SuitableModeCount] = ModeTemp;
>  SetVideoModeInfo(SuitableModeCount, );
> -if (tmpModeInfo.XResolution == MIN_WIDTH_SIZE &&
> -tmpModeInfo.YResolution == MIN_HEIGHT_SIZE)
> +if (tmpModeInfo.XResolution == INITIAL_WIDTH &&
> +tmpModeInfo.YResolution == INITIAL_HEIGHT)
>  {
>  m_CurrentMode = (USHORT)SuitableModeCount;
>  }
> @@ -3186,8 +3186,8 @@ NTSTATUS
> QxlDevice::GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo)
>  UINT BitsPerPixel = BPPFromPixelFormat(pDispInfo->ColorFormat);
>  if (Width == 0 || Height == 0 || BitsPerPixel != QXL_BPP)
>  {
> -Width = MIN_WIDTH_SIZE;
> -Height = MIN_HEIGHT_SIZE;
> +Width = MINIMAL_WIDTH;
> +Height = MINIMAL_HEIGHT;
>  BitsPerPixel = QXL_BPP;
>  }
>  

Why not setting to the initial values here?

> @@ -3200,14 +3200,14 @@ NTSTATUS
> QxlDevice::GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo)
>  
>  DbgPrint(TRACE_LEVEL_INFORMATION, ("%s: modes[%d] x_res = %d, y_res
>  = %d, bits = %d BitsPerPixel = %d\n", __FUNCTION__, CurrentMode,
>  tmpModeInfo->x_res, tmpModeInfo->y_res, tmpModeInfo->bits,
>  BitsPerPixel));
>  
> -if (tmpModeInfo->x_res >= MIN_WIDTH_SIZE &&
> -tmpModeInfo->y_res >= MIN_HEIGHT_SIZE &&
> +if (tmpModeInfo->x_res >= MINIMAL_WIDTH &&
> +tmpModeInfo->y_res >= MINIMAL_HEIGHT &&
>  tmpModeInfo->bits == QXL_BPP)
>  {
>  m_ModeNumbers[SuitableModeCount] = SuitableModeCount;
>  SetVideoModeInfo(SuitableModeCount, tmpModeInfo);
> -if (tmpModeInfo->x_res == MIN_WIDTH_SIZE &&
> -tmpModeInfo->y_res == MIN_HEIGHT_SIZE)
> +if (tmpModeInfo->x_res == INITIAL_WIDTH &&
> +tmpModeInfo->y_res == INITIAL_HEIGHT)
>  {
>  m_CurrentMode = SuitableModeCount;
>  }
> @@ -4883,9 +4883,9 @@ NTSTATUS
> QxlDevice::SetCustomDisplay(QXLEscapeSetCustomDisplay* custom_display)
>  UINT yres = custom_display->yres;
>  UINT bpp = QXL_BPP;
>  DbgPrint(TRACE_LEVEL_WARNING, ("%s - %d (%dx%d#%d)\n", __FUNCTION__,
>  m_Id, xres, yres, bpp));
> -if (xres < MIN_WIDTH_SIZE || yres < MIN_HEIGHT_SIZE) {
> +if (xres < MINIMAL_WIDTH || yres < MINIMAL_HEIGHT) {
>  DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: (%dx%d#%d) less than (%dx%d)\n",
>  __FUNCTION__,
> -xres, yres, bpp, MIN_WIDTH_SIZE, MIN_HEIGHT_SIZE));
> +xres, yres, bpp, MINIMAL_WIDTH, MINIMAL_HEIGHT));
>  }
>  m_CustomMode =(USHORT) ((m_CustomMode == m_ModeCount-1)?  m_ModeCount -
>  2 : m_ModeCount - 1);
>  
> @@ -5147,8 +5147,8 @@ NTSTATUS
> HwDeviceInterface::AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& DispInf
>  if (DispInfo.Width == 0)
>  {
>  DispInfo.ColorFormat = D3DDDIFMT_A8R8G8B8;
> -DispInfo.Width = MIN_WIDTH_SIZE;
> -DispInfo.Height = MIN_HEIGHT_SIZE;
> +DispInfo.Width = INITIAL_WIDTH;
> +DispInfo.Height = INITIAL_HEIGHT;
>  DispInfo.Pitch = DispInfo.Width *
>  BPPFromPixelFormat(DispInfo.ColorFormat) / BITS_PER_BYTE;
>  DispInfo.TargetId = 0;
>  }
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> index 695b83a..ba7e6a7 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -19,8 +19,10 @@
>  #define BITS_PER_BYTE  8
>  
>  #define POINTER_SIZE   64
> -#define MIN_WIDTH_SIZE 1024
> -#define MIN_HEIGHT_SIZE768
> +#define MINIMAL_WIDTH  800
> +#define MINIMAL_HEIGHT 600
> +#define INITIAL_WIDTH  1024
> +#define 

[Spice-devel] [PATCH spice-streaming-agent] Make evaluation order more readable

[Frediano Ziglio]

As a first sight the XXX = YYY != 0 syntax can be confusing,
add parenthesis to make clear the order.

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

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 4ec5e42..e01de6c 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -112,7 +112,7 @@ static int read_command_from_device(void)
n, hdr.size);
 return -1;
 }
-streaming_requested = msg[0] != 0; /* num_codecs */
+streaming_requested = (msg[0] != 0); /* num_codecs */
 syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
streaming_requested ? "START" : "STOP");
 client_codecs.clear();
-- 
2.14.3

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

[Spice-devel] [PATCH] Add support for resolution 800x600

[Yuri Benditovich]

https://bugzilla.redhat.com/show_bug.cgi?id=1477492
https://docs.microsoft.com/en-us/windows-hardware/design/minimum/minimum-hardware-requirements-overview
requires 800x600 to be supported.

Signed-off-by: Yuri Benditovich 
---
 qxldod/QxlDod.cpp | 28 ++--
 qxldod/QxlDod.h   |  6 --
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
index 4f508bd..ee97b09 100755
--- a/qxldod/QxlDod.cpp
+++ b/qxldod/QxlDod.cpp
@@ -2572,15 +2572,15 @@ NTSTATUS 
VgaDevice::GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo)
 
 DbgPrint(TRACE_LEVEL_INFORMATION, ("ModeTemp = 0x%X %dx%d@%d\n", 
ModeTemp, tmpModeInfo.XResolution, tmpModeInfo.YResolution, 
tmpModeInfo.BitsPerPixel));
 
-if (tmpModeInfo.XResolution >= MIN_WIDTH_SIZE &&
-tmpModeInfo.YResolution >= MIN_HEIGHT_SIZE &&
+if (tmpModeInfo.XResolution >= MINIMAL_WIDTH &&
+tmpModeInfo.YResolution >= MINIMAL_HEIGHT &&
 tmpModeInfo.BitsPerPixel == BitsPerPixel &&
 tmpModeInfo.PhysBasePtr != 0)
 {
 m_ModeNumbers[SuitableModeCount] = ModeTemp;
 SetVideoModeInfo(SuitableModeCount, );
-if (tmpModeInfo.XResolution == MIN_WIDTH_SIZE &&
-tmpModeInfo.YResolution == MIN_HEIGHT_SIZE)
+if (tmpModeInfo.XResolution == INITIAL_WIDTH &&
+tmpModeInfo.YResolution == INITIAL_HEIGHT)
 {
 m_CurrentMode = (USHORT)SuitableModeCount;
 }
@@ -3186,8 +3186,8 @@ NTSTATUS QxlDevice::GetModeList(DXGK_DISPLAY_INFORMATION* 
pDispInfo)
 UINT BitsPerPixel = BPPFromPixelFormat(pDispInfo->ColorFormat);
 if (Width == 0 || Height == 0 || BitsPerPixel != QXL_BPP)
 {
-Width = MIN_WIDTH_SIZE;
-Height = MIN_HEIGHT_SIZE;
+Width = MINIMAL_WIDTH;
+Height = MINIMAL_HEIGHT;
 BitsPerPixel = QXL_BPP;
 }
 
@@ -3200,14 +3200,14 @@ NTSTATUS 
QxlDevice::GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo)
 
 DbgPrint(TRACE_LEVEL_INFORMATION, ("%s: modes[%d] x_res = %d, y_res = 
%d, bits = %d BitsPerPixel = %d\n", __FUNCTION__, CurrentMode, 
tmpModeInfo->x_res, tmpModeInfo->y_res, tmpModeInfo->bits, BitsPerPixel));
 
-if (tmpModeInfo->x_res >= MIN_WIDTH_SIZE &&
-tmpModeInfo->y_res >= MIN_HEIGHT_SIZE &&
+if (tmpModeInfo->x_res >= MINIMAL_WIDTH &&
+tmpModeInfo->y_res >= MINIMAL_HEIGHT &&
 tmpModeInfo->bits == QXL_BPP)
 {
 m_ModeNumbers[SuitableModeCount] = SuitableModeCount;
 SetVideoModeInfo(SuitableModeCount, tmpModeInfo);
-if (tmpModeInfo->x_res == MIN_WIDTH_SIZE &&
-tmpModeInfo->y_res == MIN_HEIGHT_SIZE)
+if (tmpModeInfo->x_res == INITIAL_WIDTH &&
+tmpModeInfo->y_res == INITIAL_HEIGHT)
 {
 m_CurrentMode = SuitableModeCount;
 }
@@ -4883,9 +4883,9 @@ NTSTATUS 
QxlDevice::SetCustomDisplay(QXLEscapeSetCustomDisplay* custom_display)
 UINT yres = custom_display->yres;
 UINT bpp = QXL_BPP;
 DbgPrint(TRACE_LEVEL_WARNING, ("%s - %d (%dx%d#%d)\n", __FUNCTION__, m_Id, 
xres, yres, bpp));
-if (xres < MIN_WIDTH_SIZE || yres < MIN_HEIGHT_SIZE) {
+if (xres < MINIMAL_WIDTH || yres < MINIMAL_HEIGHT) {
 DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: (%dx%d#%d) less than (%dx%d)\n", 
__FUNCTION__,
-xres, yres, bpp, MIN_WIDTH_SIZE, MIN_HEIGHT_SIZE));
+xres, yres, bpp, MINIMAL_WIDTH, MINIMAL_HEIGHT));
 }
 m_CustomMode =(USHORT) ((m_CustomMode == m_ModeCount-1)?  m_ModeCount - 2 
: m_ModeCount - 1);
 
@@ -5147,8 +5147,8 @@ NTSTATUS 
HwDeviceInterface::AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& DispInf
 if (DispInfo.Width == 0)
 {
 DispInfo.ColorFormat = D3DDDIFMT_A8R8G8B8;
-DispInfo.Width = MIN_WIDTH_SIZE;
-DispInfo.Height = MIN_HEIGHT_SIZE;
+DispInfo.Width = INITIAL_WIDTH;
+DispInfo.Height = INITIAL_HEIGHT;
 DispInfo.Pitch = DispInfo.Width * 
BPPFromPixelFormat(DispInfo.ColorFormat) / BITS_PER_BYTE;
 DispInfo.TargetId = 0;
 }
diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
index 695b83a..ba7e6a7 100755
--- a/qxldod/QxlDod.h
+++ b/qxldod/QxlDod.h
@@ -19,8 +19,10 @@
 #define BITS_PER_BYTE  8
 
 #define POINTER_SIZE   64
-#define MIN_WIDTH_SIZE 1024
-#define MIN_HEIGHT_SIZE768
+#define MINIMAL_WIDTH  800
+#define MINIMAL_HEIGHT 600
+#define INITIAL_WIDTH  1024
+#define INITIAL_HEIGHT 768
 #define QXL_BPP32
 #define VGA_BPP24
 
-- 
2.7.0.windows.1

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

Re: [Spice-devel] #include "canvas_base.c" ?

[Christophe Fergeau]

On Thu, Feb 15, 2018 at 03:28:42PM +0100, Christophe de Dinechin wrote:
> While looking at some warnings, I came across this in sw_canvas.c:
> 
> #include “canvas_base.c"
> 
> So we include a .c in another one. Apparently, this was inherited from some 
> Cairo canvas. Is that something we care to keep as is?

There used to be gdi_canvas.c and gl_canvas.c in addition to
sw_canvas.c, they all included base_canvas.c after setting a few #define
to tweak its behaviour. Now gdi_canvas.c and gl_canvas.c are gone, so
it's probably possible to get rid of that #include "canvas_base.c".

Note that these files are special though,
spice-common/common/Makefile.am has:

# These 2 files are not build as part of spice-common
# build system, but modules using spice-common will build
# them with the appropriate options. We need to let automake
# know that these are source files so that it can properly
# track these files dependencies


and spice-gtk/src/client_sw_canvas.c includes sw_canvas.c after
#define'ing SW_CANVAS_CACHE.
spice-server uses spice-server/server/sw-canvas.c, but does not define
SW_CANVAS_CACHE.

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] [RFC PATCH 1/1] separate and encapsulate the agent business code

[Christophe Fergeau]

On Wed, Feb 14, 2018 at 10:13:41PM +0100, Christophe de Dinechin wrote:
> >> I’d write “config.h”. No reason to ever look config.h in system headers.
> > 
> > The reason for the <> is described in [1], 4th paragraph. I've
> > mentioned it during the previous discussion and didn't get any comment
> > on it IIRC. Either is fine by me, I don't plan to introduce another
> > file named 'config.h' anywhere in the source tree.
> > 
> > [1] 
> > https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Configuration-Headers.html
> 
> That rationale is remarkably inconsistent with the generated makefiles, which 
> build for example with:
> 
> -I. -I..  -I.. -I.. -I/usr/include/pixman-1  -I/usr/include/cacard 
> -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 
> -I/usr/include/nspr4   -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include 
> -pthread  -I/usr/include/opus   -DG_LOG_DOMAIN=\"Spice\" 
> -I/usr/local/include/spice-1  
> 
> or for the streaming agent:
> 
> -I. -I..  -DSPICE_STREAMING_AGENT_PROGRAM -I../include 
> -DPLUGINSDIR=\"/usr/local/lib/spice-streaming-agent/plugins\" 
> -I/usr/local/include/spice-1  
> 
> So you -I. first anyway, and you would prefer the local config.h in any case. 
> “config.h” just lets compiler find it without looking up at the command-line 
> options.

As far as I can tell, this -I. -I.. corresponds to -I$(builddir) -I$(srcdir)

#include "config.h" would look into $(srcdir) first regardless of these
-I flags, #include  will look into -I$(builddir) first, which
is what is documented in the link given by Lukas.

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 03/14] streaming_requested is really a bool

[Christophe de Dinechin]

> On 19 Feb 2018, at 13:03, Christophe Fergeau  wrote:
> 
> On Wed, Feb 14, 2018 at 06:52:11PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> src/spice-streaming-agent.cpp | 8 
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index c4c52ef..760c211 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -52,7 +52,7 @@ struct SpiceStreamDataMessage
>> StreamMsgData msg;
>> };
>> 
>> -static int streaming_requested;
>> +static bool streaming_requested = false;
>> static std::set client_codecs;
>> static bool quit;
>> static int streamfd = -1;
>> @@ -97,9 +97,9 @@ static int read_command_from_stdin(void)
>> if (strcmp(cmd, "quit") == 0) {
>> quit = true;
>> } else if (strcmp(cmd, "start") == 0) {
>> -streaming_requested = 1;
>> +streaming_requested = true;
>> } else if (strcmp(cmd, "stop") == 0) {
>> -streaming_requested = 0;
>> +streaming_requested = false;
>> } else {
>> syslog(LOG_WARNING, "unknown command %s\n", cmd);
>> }
>> @@ -142,7 +142,7 @@ static int read_command_from_device(void)
>>n, hdr.size);
>> return -1;
>> }
>> -streaming_requested = msg[0]; /* num_codecs */
>> +streaming_requested = msg[0] != 0; /* num_codecs */
> 
> For what it's worth, I find
>streaming_requested = (msg[0] != 0); /* num_codecs */
> much easier to read.

Extra parentheses? Well, I don’t mind. Will do.

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

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

Re: [Spice-devel] [PATCH spice-streaming-agent v2 2/4] Remove clang warning on missing 'override'

[Christophe Fergeau]

On Thu, Feb 15, 2018 at 03:58:15PM +0100, Christophe de Dinechin wrote:
> Again, I really don’t want to have to post-process my patches manually
> to split-out whitespace corrections. I see that as “punishment for
> doing the right thing”.

This is also a matter of differing workflows, I mostly exclusively use
git add -p when working on patches, so filtering out unwanted hunks is
part of my workflow, not some kind of annoying post-processing.

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-gtk v2 2/4] uri: learn to parse spice+tls:// form

[Christophe Fergeau]

On Fri, Feb 16, 2018 at 10:30:18AM +, Daniel P. Berrangé wrote:
> On Fri, Feb 16, 2018 at 11:13:06AM +0100, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau 
> > 
> > spice:// has a weird scheme encoding, where it can accept both plain
> > and tls ports with URI query parameters. However, it's not very
> > convenient nor very common to use (who really want to mix plain & tls
> > channels?).
> 
> Is it worth formally deprecating the mixing of plain & tls on a per
> channel basis in QEMU ?  The idea that you can be secure, and yet
> still have some channels plain text is really dubious and promotes
> dangerous practice to users.

Yup, probably best to deprecate this, even though RHV was still setting
per-channel security last time I checked (but everything was set to
either secure or unsecure).

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 2/2] Eliminate signed/unsigned warning

[Christophe Fergeau]

On Fri, Feb 16, 2018 at 04:23:06PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Without this, GCC complains about signed / unsigned comparisons:
> 
> mjpeg-fallback.cpp:121:24: warning: comparison between signed and unsigned 
> integer expressions [-Wsign-compare]
>  if (win_info.width != last_width || win_info.height != last_height) {
>  ~~~^

Are you getting this using the default CXXFLAGS? Here I seem to be getting
-Wno-sign-compare by default.

Christophe

> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/mjpeg-fallback.cpp| 2 +-
>  src/spice-streaming-agent.cpp | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 634864f..53804d9 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -57,7 +57,7 @@ private:
>  std::vector frame;
>  
>  // last frame sizes
> -uint32_t last_width = ~0u, last_height = ~0u;
> +int last_width = ~0u, last_height = ~0u;
>  // last time before capture
>  uint64_t last_time = 0;
>  };
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 4ec5e42..27b26a4 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -106,7 +106,7 @@ static int read_command_from_device(void)
>  return 0; // return -1;
>  }
>  n = read(streamfd, , hdr.size);
> -if (n != hdr.size) {
> +if (n != (int) hdr.size) {
>  syslog(LOG_WARNING,
> "read command from device FAILED -- read %d expected %d\n",
> n, hdr.size);
> -- 
> 2.13.5 (Apple Git-94)
> 
> ___
> 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 03/14] streaming_requested is really a bool

[Christophe Fergeau]

On Wed, Feb 14, 2018 at 06:52:11PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/spice-streaming-agent.cpp | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index c4c52ef..760c211 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -52,7 +52,7 @@ struct SpiceStreamDataMessage
>  StreamMsgData msg;
>  };
>  
> -static int streaming_requested;
> +static bool streaming_requested = false;
>  static std::set client_codecs;
>  static bool quit;
>  static int streamfd = -1;
> @@ -97,9 +97,9 @@ static int read_command_from_stdin(void)
>  if (strcmp(cmd, "quit") == 0) {
>  quit = true;
>  } else if (strcmp(cmd, "start") == 0) {
> -streaming_requested = 1;
> +streaming_requested = true;
>  } else if (strcmp(cmd, "stop") == 0) {
> -streaming_requested = 0;
> +streaming_requested = false;
>  } else {
>  syslog(LOG_WARNING, "unknown command %s\n", cmd);
>  }
> @@ -142,7 +142,7 @@ static int read_command_from_device(void)
> n, hdr.size);
>  return -1;
>  }
> -streaming_requested = msg[0]; /* num_codecs */
> +streaming_requested = msg[0] != 0; /* num_codecs */

For what it's worth, I find
streaming_requested = (msg[0] != 0); /* num_codecs */
much easier to read.

Christophe


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