Re: [Spice-devel] [PATCH spice-server 6/8] record: Allows to use recording function for multiple purposes

[Jonathon Jongsma]

On Fri, 2016-11-04 at 13:16 +, Frediano Ziglio wrote:
> Allocate recording file globally from reds.c.

I think this line makes more sense as the commit summary

Acked-by: Jonathon Jongsma 


> This will allow to register multiple screen VM or recording
> additional stuff like sound.
> The mutex code added to red-record-qxl.c is required to avoid
> mixing writing from multiple threads.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-record-qxl.c | 26 ++
>  server/red-record-qxl.h |  3 ++-
>  server/red-worker.c |  6 +-
>  server/reds-private.h   |  2 ++
>  server/reds.c   | 17 +
>  server/reds.h   |  5 +
>  6 files changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index bd8869e..ad4b709 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -31,7 +31,9 @@
>  
>  struct RedRecord {
>  FILE *fd;
> +pthread_mutex_t lock;
>  unsigned int counter;
> +gint refs;
>  };
>  
>  #if 0
> @@ -795,12 +797,14 @@ void
> red_record_primary_surface_create(RedRecord *record,
>  {
>  FILE *fd = record->fd;
>  
> +pthread_mutex_lock(>lock);
>  fprintf(fd, "%d %d %d %d\n", surface->width, surface->height,
>  surface->stride, surface->format);
>  fprintf(fd, "%d %d %d %d\n", surface->position, surface-
> >mouse_mode,
>  surface->flags, surface->type);
>  write_binary(fd, "data", line_0 ? abs(surface->stride)*surface-
> >height : 0,
>  line_0);
> +pthread_mutex_unlock(>lock);
>  }
>  
>  static void red_record_event_start(RedRecord *record, int what,
> uint32_t type)
> @@ -816,13 +820,16 @@ static void red_record_event_start(RedRecord
> *record, int what, uint32_t type)
>  
>  void red_record_event(RedRecord *record, int what, uint32_t type)
>  {
> +pthread_mutex_lock(>lock);
>  red_record_event_start(record, what, type);
> +pthread_mutex_unlock(>lock);
>  }
>  void red_record_qxl_command(RedRecord *record, RedMemSlotInfo
> *slots,
>  QXLCommandExt ext_cmd)
>  {
>  FILE *fd = record->fd;
>  
> +pthread_mutex_lock(>lock);
>  red_record_event_start(record, 0, ext_cmd.cmd.type);
>  
>  switch (ext_cmd.cmd.type) {
> @@ -842,6 +849,7 @@ void red_record_qxl_command(RedRecord *record,
> RedMemSlotInfo *slots,
>  red_record_cursor_cmd(fd, slots, ext_cmd.group_id,
> ext_cmd.cmd.data);
>  break;
>  }
> +pthread_mutex_unlock(>lock);
>  }
>  
>  /**
> @@ -904,15 +912,25 @@ RedRecord *red_record_new(const char *filename)
>  }
>  
>  record = g_new(RedRecord, 1);
> +record->refs = 1;
>  record->fd = f;
>  record->counter = 0;
> +pthread_mutex_init(>lock, NULL);
>  return record;
>  }
>  
> -void red_record_free(RedRecord *record)
> +RedRecord *red_record_ref(RedRecord *record)
>  {
> -if (record) {
> -fclose(record->fd);
> -g_free(record);
> +g_atomic_int_inc(>refs);
> +return record;
> +}
> +
> +void red_record_unref(RedRecord *record)
> +{
> +if (!record || !g_atomic_int_dec_and_test(>refs)) {
> +return;
>  }
> +fclose(record->fd);
> +pthread_mutex_destroy(>lock);
> +g_free(record);
>  }
> diff --git a/server/red-record-qxl.h b/server/red-record-qxl.h
> index 0685393..293e24a 100644
> --- a/server/red-record-qxl.h
> +++ b/server/red-record-qxl.h
> @@ -33,7 +33,8 @@ typedef struct RedRecord RedRecord;
>   */
>  RedRecord* red_record_new(const char *filename);
>  
> -void red_record_free(RedRecord *record);
> +RedRecord *red_record_ref(RedRecord *record);
> +void red_record_unref(RedRecord *record);
>  
>  void red_record_primary_surface_create(RedRecord *record,
> QXLDevSurfaceCreate *surface,
> diff --git a/server/red-worker.c b/server/red-worker.c
> index e1765c1..322c2b3 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -1306,7 +1306,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
>  QXLDevInitInfo init_info;
>  RedWorker *worker;
>  Dispatcher *dispatcher;
> -const char *record_filename;
>  RedsState *reds = red_qxl_get_server(qxl->st);
>  RedChannel *channel;
>  
> @@ -1316,10 +1315,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
>  worker->core = event_loop_core;
>  worker->core.main_context = g_main_context_new();
>  
> -record_filename = getenv("SPICE_WORKER_RECORD_FILENAME");
> -if (record_filename) {
> -worker->record = red_record_new(record_filename);
> -}
> +worker->record = reds_get_record(reds);
>  dispatcher = red_qxl_get_dispatcher(qxl);
>  dispatcher_set_opaque(dispatcher, worker);
>  
> diff --git a/server/reds-private.h b/server/reds-private.h
> index ce78945..85ad279 100644
> --- a/server/reds-private.h
> +++ b/server/reds-private.h
> @@ -24,6 +24,7 @@
>  #include 

Re: [Spice-devel] [PATCH spice-server 5/8] replay: Remove time argument from recording functions

[Jonathon Jongsma]

On Fri, 2016-11-04 at 13:16 +, Frediano Ziglio wrote:
> Time is always the the current real time so avoid to compute
> it for every call but move to red-record-qxl.c.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-record-qxl.c | 11 ---
>  server/red-record-qxl.h |  4 ++--
>  server/red-worker.c | 14 +++---
>  3 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index 8af2c9c..bd8869e 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -803,8 +803,9 @@ void red_record_primary_surface_create(RedRecord
> *record,
>  line_0);
>  }
>  
> -void red_record_event(RedRecord *record, int what, uint32_t type,
> red_time_t ts)
> +static void red_record_event_start(RedRecord *record, int what,
> uint32_t type)
>  {
> +red_time_t ts = spice_get_monotonic_time_ns();
>  // TODO: record the size of the packet in the header. This would
> make
>  // navigating it much faster (well, I can add an index while I'm
> at it..)
>  // and make it trivial to get a histogram from a file.
> @@ -813,12 +814,16 @@ void red_record_event(RedRecord *record, int
> what, uint32_t type, red_time_t ts)
>  fprintf(record->fd, "event %u %d %u %"PRIu64"\n", record-
> >counter++, what, type, ts);
>  }
>  
> +void red_record_event(RedRecord *record, int what, uint32_t type)
> +{
> +red_record_event_start(record, what, type);
> +}

missing a newline here after the function definition, but...

What's the point of making this a wrapper function that simply takes
the same arguments and calls the internal function? It seems completely
unnecessary to me, and just makes things slightly confusing.



>  void red_record_qxl_command(RedRecord *record, RedMemSlotInfo
> *slots,
> -QXLCommandExt ext_cmd, red_time_t ts)
> +QXLCommandExt ext_cmd)
>  {
>  FILE *fd = record->fd;
>  
> -red_record_event(record, 0, ext_cmd.cmd.type, ts);
> +red_record_event_start(record, 0, ext_cmd.cmd.type);
>  
>  switch (ext_cmd.cmd.type) {
>  case QXL_CMD_DRAW:
> diff --git a/server/red-record-qxl.h b/server/red-record-qxl.h
> index 7332afe..0685393 100644
> --- a/server/red-record-qxl.h
> +++ b/server/red-record-qxl.h
> @@ -39,9 +39,9 @@ void red_record_primary_surface_create(RedRecord
> *record,
> QXLDevSurfaceCreate *surface,
> uint8_t *line_0);
>  
> -void red_record_event(RedRecord *record, int what, uint32_t type,
> red_time_t ts);
> +void red_record_event(RedRecord *record, int what, uint32_t type);
>  
>  void red_record_qxl_command(RedRecord *record, RedMemSlotInfo
> *slots,
> -QXLCommandExt ext_cmd, red_time_t ts);
> +QXLCommandExt ext_cmd);
>  
>  #endif
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 4383646..e1765c1 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -127,9 +127,9 @@ static int red_process_cursor(RedWorker *worker,
> int *ring_is_empty)
>  return n;
>  }
>  
> -if (worker->record)
> -red_record_qxl_command(worker->record, 
> >mem_slots, ext_cmd,
> -   spice_get_monotonic_time_ns());
> +if (worker->record) {
> +red_record_qxl_command(worker->record, 
> >mem_slots, ext_cmd);
> +}
>  
>  worker->cursor_poll_tries = 0;
>  switch (ext_cmd.cmd.type) {
> @@ -190,9 +190,9 @@ static int red_process_display(RedWorker *worker,
> int *ring_is_empty)
>  return n;
>  }
>  
> -if (worker->record)
> -red_record_qxl_command(worker->record, 
> >mem_slots, ext_cmd,
> -   spice_get_monotonic_time_ns());
> +if (worker->record) {
> +red_record_qxl_command(worker->record, 
> >mem_slots, ext_cmd);
> +}
>  
>  stat_inc_counter(reds, worker->command_counter, 1);
>  worker->display_poll_tries = 0;
> @@ -1033,7 +1033,7 @@ static void worker_dispatcher_record(void
> *opaque, uint32_t message_type, void *
>  {
>  RedWorker *worker = opaque;
>  
> -red_record_event(worker->record, 1, message_type,
> spice_get_monotonic_time_ns());
> +red_record_event(worker->record, 1, message_type);
>  }
>  
>  static void register_callbacks(Dispatcher *dispatcher)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 4/8] replay: Check properly version number

[Jonathon Jongsma]

Would be nice if we could somehow maintain backward compatibility, but
i don't think it's worth the effort to maintain it.

Acked-by: Jonathon Jongsma 


On Fri, 2016-11-04 at 13:16 +, Frediano Ziglio wrote:
> 0 as version was never used so don't allow it
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-replay-qxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 680e212..79e00e5 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -1415,7 +1415,7 @@ SpiceReplay *spice_replay_new(FILE *file, int
> nsurfaces)
>  spice_return_val_if_fail(file != NULL, NULL);
>  
>  if (fscanf(file, "SPICE_REPLAY %u\n", ) == 1) {
> -if (version > 1) {
> +if (version != 1) {
>  spice_warning("Replay file version unsupported");
>  return NULL;
>  }
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 3/8] replay: Replicate properly wakeups

[Jonathon Jongsma]

A few minor wording suggestions. The subject should say "Replicate
wakeups properly".

On Fri, 2016-11-04 at 13:16 +, Frediano Ziglio wrote:
> Instead of waking up the command loop for every command queued

add a comma after queued

> handle saved wakeups and replicate these.
> This better reproduce what happened in the server.

reproduce > reproduces


Otherwise seems fine to me

Acked-by: Jonathon Jongsma 



> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-replay-qxl.c | 2 +-
>  server/tests/replay.c   | 4 
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 0ab87d4..680e212 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -1279,7 +1279,7 @@ static void replay_handle_dev_input(QXLWorker
> *worker, SpiceReplay *replay,
>  // we want to ignore this one - it is sent on client
> connection, we
>  // shall have our own clients
>  case RED_WORKER_MESSAGE_WAKEUP:
> -// safe to ignore
> +worker->wakeup(worker);
>  break;
>  default:
>  spice_debug("unhandled %d\n", message);
> diff --git a/server/tests/replay.c b/server/tests/replay.c
> index 6c6e01e..3a0d515 100644
> --- a/server/tests/replay.c
> +++ b/server/tests/replay.c
> @@ -111,7 +111,6 @@ static void get_init_info(QXLInstance *qin,
> QXLDevInitInfo *info)
>  static gboolean fill_queue_idle(gpointer user_data)
>  {
>  gboolean keep = FALSE;
> -gboolean wakeup = FALSE;
>  
>  while ((g_async_queue_length(display_queue) +
>  g_async_queue_length(cursor_queue)) < 50) {
> @@ -128,7 +127,6 @@ static gboolean fill_queue_idle(gpointer
> user_data)
>  g_usleep(slow);
>  }
>  
> -wakeup = TRUE;
>  if (cmd->cmd.type == QXL_CMD_CURSOR) {
>  g_async_queue_push(cursor_queue, cmd);
>  } else {
> @@ -146,8 +144,6 @@ end:
>  }
>  pthread_mutex_unlock();
>  }
> -if (wakeup)
> -spice_qxl_wakeup(_sin);
>  
>  return keep;
>  }
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server] build: Fix wrong -Wno-missing-field-initializers detection

[Frediano Ziglio]

> 
> Hey,
> 
> On Wed, Nov 09, 2016 at 09:13:32AM +, Frediano Ziglio wrote:
> > The small code in m4/manywarnings.m4 wrongly detects if
> > -Wno-missing-field-initializers is needed. This happens if
> > -Wunused-variable is set. In this case the code fails to compile
> > due to -Werror even if -Wno-missing-field-initializers would be
> > perfectly fine.
> 
> This m4 macro comes from gnulib:
> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=m4/manywarnings.m4;h=89fd0ae38e387b9451fb62cb37936ccaefff3686;hb=HEAD#l55
> so this would need to be fixed there first.
> 
> Christophe
> 

Accepted and merged

http://git.savannah.gnu.org/cgit/gnulib.git/tree/m4/manywarnings.m4
http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=94d81adc2f56c048b7872cd8ae2dd8568aef6dcf

Not sure we should update all the file too.
I think the list of warnings will change.

Frediano

> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  m4/manywarnings.m4 | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
> > index 3e6dd21..dab6a0b 100644
> > --- a/m4/manywarnings.m4
> > +++ b/m4/manywarnings.m4
> > @@ -62,10 +62,11 @@ AC_DEFUN([gl_MANYWARN_ALL_GCC],
> >  CFLAGS="$CFLAGS -W -Werror"
> >  AC_COMPILE_IFELSE(
> >[AC_LANG_PROGRAM(
> > - [[void f (void)
> > + [[int f (void)
> > {
> >   typedef struct { int a; int b; } s_t;
> >   s_t s1 = { 0, };
> > + return s1.b;
> > }
> >   ]],
> >   [[]])],

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

Re: [Spice-devel] [PATCH spice-server 2/8] Make red-replay-qxl.h a public header

[Jonathon Jongsma]

Acked-by: Jonathon Jongsma 


On Fri, 2016-11-04 at 13:16 +, Frediano Ziglio wrote:
> The functions declared in that header are all exported by the
> library.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/Makefile.am  |  2 +-
>  server/red-replay-qxl.c |  1 -
>  server/red-replay-qxl.h | 35 ---
>  server/spice-replay.h   | 37 +
>  server/spice.h  |  1 +
>  server/tests/replay.c   |  1 -
>  6 files changed, 39 insertions(+), 38 deletions(-)
>  delete mode 100644 server/red-replay-qxl.h
>  create mode 100644 server/spice-replay.h
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index ca67be1..91f9666 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -64,6 +64,7 @@ libspice_serverinclude_HEADERS =\
>   spice-qxl.h \
>   spice-server.h  \
>   spice-version.h \
> + spice-replay.h  \
>   spice.h \
>   $(NULL)
>  
> @@ -121,7 +122,6 @@ libserver_la_SOURCES =
> \
>   red-record-qxl.c\
>   red-record-qxl.h\
>   red-replay-qxl.c\
> - red-replay-qxl.h\
>   red-parse-qxl.h \
>   red-worker.c\
>   red-worker.h\
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 78de48b..0ab87d4 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -28,7 +28,6 @@
>  #include "red-common.h"
>  #include "memslot.h"
>  #include "red-parse-qxl.h"
> -#include "red-replay-qxl.h"
>  #include 
>  
>  #define QXLPHYSICAL_FROM_PTR(ptr) ((QXLPHYSICAL)(intptr_t)(ptr))
> diff --git a/server/red-replay-qxl.h b/server/red-replay-qxl.h
> deleted file mode 100644
> index c9e074d..000
> --- a/server/red-replay-qxl.h
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> -/*
> -   Copyright (C) 2009-2015 Red Hat, Inc.
> -
> -   This library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later
> version.
> -
> -   This library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with this library; if not, see  licenses/>.
> -*/
> -#ifndef RED_REPLAY_QXL_H
> -#define RED_REPLAY_QXL_H
> -
> -#include 
> -#include 
> -
> -#include "spice.h"
> -
> -typedef struct SpiceReplay SpiceReplay;
> -
> -/* reads until encountering a cmd, processing any recorded messages
> (io) on the
> - * way */
> -QXLCommandExt*  spice_replay_next_cmd(SpiceReplay *replay, QXLWorker
> *worker);
> -voidspice_replay_free_cmd(SpiceReplay *replay,
> QXLCommandExt *cmd);
> -voidspice_replay_free(SpiceReplay *replay);
> -SpiceReplay *   spice_replay_new(FILE *file, int nsurfaces);
> -
> -#endif // RED_REPLAY_QXL_H
> diff --git a/server/spice-replay.h b/server/spice-replay.h
> new file mode 100644
> index 000..9a02869
> --- /dev/null
> +++ b/server/spice-replay.h
> @@ -0,0 +1,37 @@
> +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> +/*
> +   Copyright (C) 2009-2015 Red Hat, Inc.
> +
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later
> version.
> +
> +   This library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with this library; if not, see  licenses/>.
> +*/
> +#ifndef SPICE_REPLAY_H_
> +#define SPICE_REPLAY_H_
> +
> +#if !defined(SPICE_H_INSIDE) && !defined(SPICE_SERVER_INTERNAL)
> +#error "Only spice.h can be included directly."
> +#endif
> +
> +#include 
> +#include "spice-core.h"
> +
> +typedef struct SpiceReplay SpiceReplay;
> +
> +/* reads until encountering a cmd, processing any recorded messages
> (io) on the
> + * way */
> +QXLCommandExt*  spice_replay_next_cmd(SpiceReplay 

Re: [Spice-devel] [PATCH spice-server 1/8] red-parse-qxl: Use same fuction to parse blend and copy commands

[Jonathon Jongsma]

Acked-by: Jonathon Jongsma 


On Fri, 2016-11-04 at 13:16 +, Frediano Ziglio wrote:
> SpiceBlend and SpiceCopy are just different names for the same
> structure.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-parse-qxl.c | 15 ---
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index d75e27e..aa911f3 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -706,15 +706,8 @@ static void red_put_copy(SpiceCopy *red)
>  red_put_qmask(>mask);
>  }
>  
> -static void red_get_blend_ptr(RedMemSlotInfo *slots, int group_id,
> - SpiceBlend *red, QXLBlend *qxl,
> uint32_t flags)
> -{
> -red->src_bitmap  = red_get_image(slots, group_id, qxl-
> >src_bitmap, flags, FALSE);
> -   red_get_rect_ptr(>src_area, >src_area);
> -   red->rop_descriptor  = qxl->rop_descriptor;
> -   red->scale_mode  = qxl->scale_mode;
> -   red_get_qmask_ptr(slots, group_id, >mask, >mask,
> flags);
> -}
> +// these types are really the same thing
> +#define red_get_blend_ptr red_get_copy_ptr
>  
>  static void red_put_blend(SpiceBlend *red)
>  {
> @@ -1074,7 +1067,7 @@ static int
> red_get_native_drawable(RedMemSlotInfo *slots, int group_id,
>    >u.blackness, >u.blackness,
> flags);
>  break;
>  case QXL_DRAW_BLEND:
> -red_get_blend_ptr(slots, group_id, >u.blend, 
> >u.blend, flags);
> +error = red_get_blend_ptr(slots, group_id, >u.blend,
> >u.blend, flags);
>  break;
>  case QXL_DRAW_COPY:
>  error = red_get_copy_ptr(slots, group_id, >u.copy,
> >u.copy, flags);
> @@ -1157,7 +1150,7 @@ static int
> red_get_compat_drawable(RedMemSlotInfo *slots, int group_id,
>    >u.blackness, >u.blackness,
> flags);
>  break;
>  case QXL_DRAW_BLEND:
> -red_get_blend_ptr(slots, group_id, >u.blend, 
> >u.blend, flags);
> +error = red_get_blend_ptr(slots, group_id, >u.blend,
> >u.blend, flags);
>  break;
>  case QXL_DRAW_COPY:
>  error = red_get_copy_ptr(slots, group_id, >u.copy,
> >u.copy, flags);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] spice-vdagent and X Display Managers on Ubuntu 16.04

[Christophe Fergeau]

Hey,
I'm very late to the party, just a few comments,

On Tue, Oct 11, 2016 at 02:06:27PM +0200, Sergio L. Pascual wrote:
> I've just spent some time tuning an Ubuntu 16.04 (I know Fedora is
> preferred around here, but a lot of clients seem to prefer Ubuntu and
> its derivatives) desktop for VDI usage, and I had a hard time finding a
> X Display Manager which plays nice with spice-vdagent (in the sense of
> adapting properly to resolution changes, something critical for mobile
> clients). I'd like to share my findings:
> 
>  - lightdm, slim, kdm: there's no way to specify a program to be
> executed while running the greeter, must be worked around with a
> wrapper. Changing resolution results in the greeter crashing and/or
> hanging.

What do you call "a wrapper"? Any idea if the crash is caused by the QXL
driver, or if it's an issue on the *dm side?

> 
>  - gdm: applications to be run alongside with greeter can be specified
> in "/usr/share/gdm/greeter/autostart" (which spice-vdagent already
> uses). Sadly, gdm > 3.14 tries to run Xorg as gdm (on greeter) or
> logged in user (after log in), which is not supported by QXL driver, as
> it needs direct PCI access.

I believe this works with Fedora, so this should be manageable somehow?

>  - sddm: needs a wrapper too, but deals nicely with resolution changes
> *IF* not using the "breeze" theme (this drove me crazy for a while,
> diving into its source code).

Same question about the need for a wrapper ?

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-server 0/5] syntax-check fixes/workarounds

[Christophe Fergeau]

ACK series except for 4/5 "syntax-check: Avoid unwanted autoconf
replacements in Makefile.am"

Christophe

On Wed, Nov 09, 2016 at 04:00:07PM +, Frediano Ziglio wrote:
> Frediano Ziglio (5):
>   syntax-check: Remove space at end of line
>   syntax-check: Remove empty line at end of file
>   syntax-check: Include config.h file #include <>
>   syntax-check: Avoid unwanted autoconf replacements in Makefile.am
>   syntax-check: Silent a wrong positive
> 
>  .gitlab-ci.yml | 2 +-
>  server/Makefile.am | 5 +++--
>  server/mjpeg-encoder.c | 2 +-
>  server/red-channel.c   | 1 -
>  server/red-client.c| 2 +-
>  5 files changed, 6 insertions(+), 6 deletions(-)
> 
> -- 
> 2.7.4
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-server 4/5] syntax-check: Avoid unwanted autoconf replacements in Makefile.am

[Christophe Fergeau]

nack, better to switch to --template rather than obfuscating this
generation even more.

On Wed, Nov 09, 2016 at 04:00:11PM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/Makefile.am | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index ca67be1..3a0c71e 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -1,4 +1,5 @@
>  NULL =
> +AT = @
>  SUBDIRS = . tests
>  
>  AM_CPPFLAGS =\
> @@ -207,7 +208,7 @@ spice-server-enums.c: spice-server.h
>   --fhead "#include \"spice-server-enums.h\"\n\n" \
>   --fprod "\n#include \"spice-server.h\"\n" \
>   --vhead "static const G@Type@Value 
> _@enum_name@_values[] = {" \
> - --vprod "  { @VALUENAME@, \"@VALUENAME@\", 
> \"@valuenick@\" }," \
> + --vprod "  { $(AT)VALUENAME$(AT), 
> \"$(AT)VALUENAME$(AT)\", \"@valuenick@\" }," \
>   --vtail "  { 0, NULL, NULL }\n};\n\n" \
>   --vtail "GType\n@enum_name@_get_type (void)\n{\n" \
>   --vtail "  static GType type = 0;\n" \
> @@ -225,7 +226,7 @@ spice-server-enums.h: spice-server.h
>   --fhead "G_BEGIN_DECLS\n\n" \
>   --ftail "G_END_DECLS\n\n" \
>   --ftail "#endif /* SPICE_SERVER_ENUMS_H */\n" \
> - --eprod "#define SPICE_TYPE_@ENUMSHORT@ 
> @enum_name@_get_type()\n" \
> + --eprod "#define SPICE_TYPE_$(AT)ENUMSHORT$(AT) 
> @enum_name@_get_type()\n" \
>   --eprod "GType @enum_name@_get_type (void);\n" \
>   $^ >  $@
>  
> -- 
> 2.7.4
> 
> ___
> 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

[Spice-devel] [PATCH] vd_agent: Add some comments to clipboard messages

[Frediano Ziglio]

Document what the messages are meant to do.

Signed-off-by: Frediano Ziglio 
---
 spice/vd_agent.h | 13 +
 1 file changed, 13 insertions(+)

diff --git a/spice/vd_agent.h b/spice/vd_agent.h
index c49f596..b95f924 100644
--- a/spice/vd_agent.h
+++ b/spice/vd_agent.h
@@ -66,10 +66,23 @@ enum {
 VD_AGENT_MOUSE_STATE = 1,
 VD_AGENT_MONITORS_CONFIG,
 VD_AGENT_REPLY,
+/* Set clipboard data (both directions).
+ * Message came with type and data.
+ * See VDAgentClipboard structure.
+ */
 VD_AGENT_CLIPBOARD,
 VD_AGENT_DISPLAY_CONFIG,
 VD_AGENT_ANNOUNCE_CAPABILITIES,
+/* Asks to listen for clipboard changes (both directions).
+ * Remote will empty clipboard and wait for one
+ * of the types passed.
+ * See VDAgentClipboardGrab structure.
+ */
 VD_AGENT_CLIPBOARD_GRAB,
+/* Asks for clipboard data (both directions).
+ * Request came with a specific type.
+ * See VDAgentClipboardRequest structure.
+ */
 VD_AGENT_CLIPBOARD_REQUEST,
 VD_AGENT_CLIPBOARD_RELEASE,
 VD_AGENT_FILE_XFER_START,
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server 2/5] syntax-check: Remove empty line at end of file

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/red-channel.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/server/red-channel.c b/server/red-channel.c
index 9279882..0936548 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -841,4 +841,3 @@ void red_channel_disconnect_client(RedChannel *channel, 
RedChannelClient *rcc)
 {
 channel->priv->client_cbs.disconnect(rcc);
 }
-
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server 5/5] syntax-check: Silent a wrong positive

[Frediano Ziglio]

Due to syntax-check limitation this free calls results in
a syntax error.

Signed-off-by: Frediano Ziglio 
---
 server/mjpeg-encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
index d95c645..21778b8 100644
--- a/server/mjpeg-encoder.c
+++ b/server/mjpeg-encoder.c
@@ -969,7 +969,7 @@ static int mjpeg_encoder_encode_frame(VideoEncoder 
*video_encoder,
 }
 
 if (ret != VIDEO_ENCODER_FRAME_ENCODE_DONE) {
-buffer->base.free((VideoBuffer*)buffer);
+buffer->base.free(>base);
 }
 return ret;
 }
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server 3/5] syntax-check: Include config.h file #include <>

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/red-client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/red-client.c b/server/red-client.c
index f7f4562..de40acf 100644
--- a/server/red-client.c
+++ b/server/red-client.c
@@ -16,7 +16,7 @@
 
 */
 #ifdef HAVE_CONFIG_H
-#include "config.h"
+#include 
 #endif /* HAVE_CONFIG_H */
 
 #include "red-channel.h"
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server 4/5] syntax-check: Avoid unwanted autoconf replacements in Makefile.am

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/Makefile.am | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/server/Makefile.am b/server/Makefile.am
index ca67be1..3a0c71e 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -1,4 +1,5 @@
 NULL =
+AT = @
 SUBDIRS = . tests
 
 AM_CPPFLAGS =  \
@@ -207,7 +208,7 @@ spice-server-enums.c: spice-server.h
--fhead "#include \"spice-server-enums.h\"\n\n" \
--fprod "\n#include \"spice-server.h\"\n" \
--vhead "static const G@Type@Value 
_@enum_name@_values[] = {" \
-   --vprod "  { @VALUENAME@, \"@VALUENAME@\", 
\"@valuenick@\" }," \
+   --vprod "  { $(AT)VALUENAME$(AT), 
\"$(AT)VALUENAME$(AT)\", \"@valuenick@\" }," \
--vtail "  { 0, NULL, NULL }\n};\n\n" \
--vtail "GType\n@enum_name@_get_type (void)\n{\n" \
--vtail "  static GType type = 0;\n" \
@@ -225,7 +226,7 @@ spice-server-enums.h: spice-server.h
--fhead "G_BEGIN_DECLS\n\n" \
--ftail "G_END_DECLS\n\n" \
--ftail "#endif /* SPICE_SERVER_ENUMS_H */\n" \
-   --eprod "#define SPICE_TYPE_@ENUMSHORT@ 
@enum_name@_get_type()\n" \
+   --eprod "#define SPICE_TYPE_$(AT)ENUMSHORT$(AT) 
@enum_name@_get_type()\n" \
--eprod "GType @enum_name@_get_type (void);\n" \
$^ >  $@
 
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server 1/5] syntax-check: Remove space at end of line

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 .gitlab-ci.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 9b2ae2d..ca57ff5 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,7 +1,7 @@
 image: fedora:latest
 
 before_script:
-  - dnf install 'dnf-command(copr)' git libtool make -y 
+  - dnf install 'dnf-command(copr)' git libtool make -y
   - dnf copr enable @spice/nightly -y
   - dnf builddep spice -y
 
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server 0/5] syntax-check fixes/workarounds

[Frediano Ziglio]

Frediano Ziglio (5):
  syntax-check: Remove space at end of line
  syntax-check: Remove empty line at end of file
  syntax-check: Include config.h file #include <>
  syntax-check: Avoid unwanted autoconf replacements in Makefile.am
  syntax-check: Silent a wrong positive

 .gitlab-ci.yml | 2 +-
 server/Makefile.am | 5 +++--
 server/mjpeg-encoder.c | 2 +-
 server/red-channel.c   | 1 -
 server/red-client.c| 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.7.4

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

Re: [Spice-devel] [PATCH spice-server] Fix typo in comment

[Jonathon Jongsma]

Trivial. 

Acked-by: Jonathon Jongsma 



On Wed, 2016-11-09 at 14:19 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/main-channel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/main-channel.c b/server/main-channel.c
> index b900b62..6449c16 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -318,7 +318,7 @@ int main_channel_getpeername(MainChannel
> *main_chan, struct sockaddr *sa, sockle
>  getpeername(red_channel_get_first_socket(RED_CHANNEL(main_ch
> an)), sa, salen) : -1;
>  }
>  
> -// TODO: ? shouldn't it disonnect all clients? or shutdown all
> main_channels?
> +// TODO: ? shouldn't it disconnect all clients? or shutdown all
> main_channels?
>  void main_channel_close(MainChannel *main_chan)
>  {
>  int socketfd;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [vdagent-linux] udscs: Fix memory ownership issues with udscs_read_callback

[Jonathon Jongsma]

Looks fine to me.

Acked-by: Jonathon Jongsma 


On Wed, 2016-11-09 at 11:47 +0100, Christophe Fergeau wrote:
> Before this commit, udscs_read_callback is supposed to free the
> 'data'
> buffer unless it calls udscs_destroy_connection(). It's currently not
> doing this, causing a potential double free in error cases. The udscs
> code would also leak the 'data' buffer if no udscs_read_callback is
> set.
> 
> This commit moves ownership of the 'data' buffer to the generic code
> calling the udscs_read_callback and fixes the memory management of
> this
> 'data' buffer. As the only udscs_read_callback currently implemented
> is
> not keeping pointers to 'data' after it returns, this should not
> cause
> any issues.
> ---
>  src/udscs.c | 2 ++
>  src/udscs.h | 7 ---
>  src/vdagentd/vdagentd.c | 4 
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/src/udscs.c b/src/udscs.c
> index 427a844..b468e71 100644
> --- a/src/udscs.c
> +++ b/src/udscs.c
> @@ -132,6 +132,7 @@ void udscs_destroy_connection(struct
> udscs_connection **connp)
>  }
>  
>  free(conn->data.buf);
> +conn->data.buf = NULL;
>  
>  if (conn->next)
>  conn->next->prev = conn->prev;
> @@ -235,6 +236,7 @@ static void udscs_read_complete(struct
> udscs_connection **connp)
>  if (!*connp) /* Was the connection disconnected by the
> callback ? */
>  return;
>  }
> +free(conn->data.buf);
>  
>  conn->header_read = 0;
>  memset(>data, 0, sizeof(conn->data));
> diff --git a/src/udscs.h b/src/udscs.h
> index d65630d..6e960f8 100644
> --- a/src/udscs.h
> +++ b/src/udscs.h
> @@ -39,9 +39,10 @@ struct udscs_message_header {
>  };
>  
>  /* Callbacks with this type will be called when a complete message
> has been
> - * received. The callback is responsible for free()-ing the data
> buffer. It
> - * may call udscs_destroy_connection, in which case *connp must be
> made NULL
> - * (which udscs_destroy_connection takes care of).
> + * received. The callback does not own the data buffer and should
> not free it.
> + * The data buffer will be freed shortly after the read callback
> returns.
> + * The callback may call udscs_destroy_connection, in which case
> *connp must be
> + * made NULL (which udscs_destroy_connection takes care of).
>   */
>  typedef void (*udscs_read_callback)(struct udscs_connection **connp,
>  struct udscs_message_header *header, uint8_t *data);
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 18e82a7..a1faf23 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -727,7 +727,6 @@ static void agent_read_complete(struct
> udscs_connection **connp,
>  if (header->arg1 == 0 && header->arg2 == 0) {
>  syslog(LOG_INFO, "got old session agent xorg resolution
> message, "
>   "ignoring");
> -free(data);
>  return;
>  }
>  
> @@ -735,7 +734,6 @@ static void agent_read_complete(struct
> udscs_connection **connp,
>  syslog(LOG_ERR, "guest xorg resolution message has wrong
> size, "
>  "disconnecting agent");
>  udscs_destroy_connection(connp);
> -free(data);
>  return;
>  }
>  
> @@ -760,7 +758,6 @@ static void agent_read_complete(struct
> udscs_connection **connp,
>  case VDAGENTD_CLIPBOARD_RELEASE:
>  if (do_agent_clipboard(*connp, header, data)) {
>  udscs_destroy_connection(connp);
> -free(data);
>  return;
>  }
>  break;
> @@ -783,7 +780,6 @@ static void agent_read_complete(struct
> udscs_connection **connp,
>  syslog(LOG_ERR, "unknown message from vdagent: %u,
> ignoring",
> header->type);
>  }
> -free(data);
>  }
>  
>  /* main */
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server 5/9] syntax-check: Add exception for server/stat.h

[Frediano Ziglio]

> 
> Due to the use of static inline functions, the headers uses
> G_GNUC_UNUSED, which the sc_avoid_attribute_unused_in_header test
> complains about.
> ---
>  cfg.mk | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/cfg.mk b/cfg.mk
> index 8809df0..3e5d345 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -145,3 +145,5 @@ exclude_file_name_regexp--sc_unmarked_diagnostics =
> ^.*\.(c|py|h)
>  exclude_file_name_regexp--sc_prohibit_path_max_allocation =
>  server/tests/test_display_base.c
>  
>  exclude_file_name_regexp--sc_cast_of_argument_to_free =
>  server/red-replay-qxl.c
> +
> +exclude_file_name_regexp--sc_avoid_attribute_unused_in_header =
> server/stat.h

Acked-by: Frediano Ziglio 

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

Re: [Spice-devel] [spice-server 5/9] syntax-check: Add exception for server/stat.h

[Christophe Fergeau]

Hey, only this patch from the series was not reviewed, anyone up for it?

Christophe

On Thu, Oct 27, 2016 at 04:12:13PM +0200, Christophe Fergeau wrote:
> Due to the use of static inline functions, the headers uses
> G_GNUC_UNUSED, which the sc_avoid_attribute_unused_in_header test
> complains about.
> ---
>  cfg.mk | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/cfg.mk b/cfg.mk
> index 8809df0..3e5d345 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -145,3 +145,5 @@ exclude_file_name_regexp--sc_unmarked_diagnostics = 
> ^.*\.(c|py|h)
>  exclude_file_name_regexp--sc_prohibit_path_max_allocation = 
> server/tests/test_display_base.c
>  
>  exclude_file_name_regexp--sc_cast_of_argument_to_free = 
> server/red-replay-qxl.c
> +
> +exclude_file_name_regexp--sc_avoid_attribute_unused_in_header = server/stat.h
> -- 
> 2.9.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] [protocol] macros: Use GLib's G_DEPRECATED macro if available

[Christophe Fergeau]

On Fri, Oct 28, 2016 at 11:27:02AM +0200, Francois Gouget wrote:
> This gains us automatic support for whichever compilers GLib supports in
> this macro when used in projects that use GLib.
> 
> Signed-off-by: Francois Gouget 

Looks good to me, though the #endif /* __GNUC__ */ needs to be changed
too, I've done that locally so no need to resend.

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] [protocol] macros: Update SPICE_GNUC_DEPRECATED for MSVC

[Christophe Fergeau]

On Fri, Oct 28, 2016 at 11:57:33AM +0200, Francois Gouget wrote:
> Signed-off-by: Francois Gouget 
> ---
> 
> This is an alternative take on the previous patch: instead of reusing 
> the GLib macro it just duplicates its logic.

Why alternative? I think I would take both patches ;) We'd have a MSVC
fallback when glib is not in use, so an improvement from before.

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] [protocol] macros: Use GLib's G_DEPRECATED macro if available

[Christophe Fergeau]

Hey,

On Fri, Oct 28, 2016 at 11:27:02AM +0200, Francois Gouget wrote:
> This gains us automatic support for whichever compilers GLib supports in
> this macro when used in projects that use GLib.
> 
> Signed-off-by: Francois Gouget 
> ---
> 
> This can make sense if we consider GLib to be a platform similar to gcc, 
> MSVC: it just provides a G_XXX API instead of an __attribute((xxx)) or 
> __declspec(xxx) one.
> 
> Also note that the same thing could be done with a number of other 
> macros like SPICE_GNUC_WARN_UNUSED_RESULT, SPICE_GNUC_MAY_ALIAS, 
> SPICE_GNUC_CONST, etc. Let me know if I should submit one patch for 
> each, one for all or something else...

I believe all of these were more or less directly imported from glib.
Grepping around, I could only find SPICE_GNUC_VISIBLE, SPICE_GNUC_NORETURN,
SPICE_GNUC_UNUSED, SPICE_GNUC_PRINTF and SPICE_GNUC_DEPRECATED being
used. At this point, I'd just make sure the other ones emit a (compile
time) warning when they are used unless some magic define is set, and
drop them after a while.

Christophe


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

[Spice-devel] [PATCH spice-server] Fix typo in comment

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/main-channel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/main-channel.c b/server/main-channel.c
index b900b62..6449c16 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -318,7 +318,7 @@ int main_channel_getpeername(MainChannel *main_chan, struct 
sockaddr *sa, sockle
 getpeername(red_channel_get_first_socket(RED_CHANNEL(main_chan)), sa, 
salen) : -1;
 }
 
-// TODO: ? shouldn't it disonnect all clients? or shutdown all main_channels?
+// TODO: ? shouldn't it disconnect all clients? or shutdown all main_channels?
 void main_channel_close(MainChannel *main_chan)
 {
 int socketfd;
-- 
2.7.4

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

[Spice-devel] [vdagent-linux] udscs: Fix memory ownership issues with udscs_read_callback

[Christophe Fergeau]

Before this commit, udscs_read_callback is supposed to free the 'data'
buffer unless it calls udscs_destroy_connection(). It's currently not
doing this, causing a potential double free in error cases. The udscs
code would also leak the 'data' buffer if no udscs_read_callback is set.

This commit moves ownership of the 'data' buffer to the generic code
calling the udscs_read_callback and fixes the memory management of this
'data' buffer. As the only udscs_read_callback currently implemented is
not keeping pointers to 'data' after it returns, this should not cause
any issues.
---
 src/udscs.c | 2 ++
 src/udscs.h | 7 ---
 src/vdagentd/vdagentd.c | 4 
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/udscs.c b/src/udscs.c
index 427a844..b468e71 100644
--- a/src/udscs.c
+++ b/src/udscs.c
@@ -132,6 +132,7 @@ void udscs_destroy_connection(struct udscs_connection 
**connp)
 }
 
 free(conn->data.buf);
+conn->data.buf = NULL;
 
 if (conn->next)
 conn->next->prev = conn->prev;
@@ -235,6 +236,7 @@ static void udscs_read_complete(struct udscs_connection 
**connp)
 if (!*connp) /* Was the connection disconnected by the callback ? */
 return;
 }
+free(conn->data.buf);
 
 conn->header_read = 0;
 memset(>data, 0, sizeof(conn->data));
diff --git a/src/udscs.h b/src/udscs.h
index d65630d..6e960f8 100644
--- a/src/udscs.h
+++ b/src/udscs.h
@@ -39,9 +39,10 @@ struct udscs_message_header {
 };
 
 /* Callbacks with this type will be called when a complete message has been
- * received. The callback is responsible for free()-ing the data buffer. It
- * may call udscs_destroy_connection, in which case *connp must be made NULL
- * (which udscs_destroy_connection takes care of).
+ * received. The callback does not own the data buffer and should not free it.
+ * The data buffer will be freed shortly after the read callback returns.
+ * The callback may call udscs_destroy_connection, in which case *connp must be
+ * made NULL (which udscs_destroy_connection takes care of).
  */
 typedef void (*udscs_read_callback)(struct udscs_connection **connp,
 struct udscs_message_header *header, uint8_t *data);
diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 18e82a7..a1faf23 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -727,7 +727,6 @@ static void agent_read_complete(struct udscs_connection 
**connp,
 if (header->arg1 == 0 && header->arg2 == 0) {
 syslog(LOG_INFO, "got old session agent xorg resolution message, "
  "ignoring");
-free(data);
 return;
 }
 
@@ -735,7 +734,6 @@ static void agent_read_complete(struct udscs_connection 
**connp,
 syslog(LOG_ERR, "guest xorg resolution message has wrong size, "
 "disconnecting agent");
 udscs_destroy_connection(connp);
-free(data);
 return;
 }
 
@@ -760,7 +758,6 @@ static void agent_read_complete(struct udscs_connection 
**connp,
 case VDAGENTD_CLIPBOARD_RELEASE:
 if (do_agent_clipboard(*connp, header, data)) {
 udscs_destroy_connection(connp);
-free(data);
 return;
 }
 break;
@@ -783,7 +780,6 @@ static void agent_read_complete(struct udscs_connection 
**connp,
 syslog(LOG_ERR, "unknown message from vdagent: %u, ignoring",
header->type);
 }
-free(data);
 }
 
 /* main */
-- 
2.9.3

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

Re: [Spice-devel] [vdagent-win RFC PATCH 0/5] Replace CxImage code

[Frediano Ziglio]

> 
> On Wed, Nov 09, 2016 at 10:32:40AM +, Frediano Ziglio wrote:
> > CxImage is used for image conversion for clipboard support.
> > CxImage have currently some issue:
> > - library is old and unsupported;
> > - required an old libpng library.
> > Currently the MingW binary we distribute due to some
> > issue have PNG disabled (so no clipboard image support).
> > Note that currently we support (before and after this patch)
> > only BMP and PNG. PNG is required as the default agent format.
> > However Windows programs wants to have BMP (specifically DIB)
> > in the clipboard.
> > This patch remove CxImage and use directly libpng (BMP is
> > supported directly by Windows APIs).
> > Tested with various formats and the compiled agent with a
> > Linux client.
> > The main concern is actually the possible problem to
> > support some weird/old BMP file formats.
> 
> Is there any sense in using gdk-pixbuf for image processing to
> enable support for formats beyond bmp/png ?
> 
> 
> Regards,
> Daniel

Actually I didn't find any Windows application that seems to
support other formats than DIB in the clipboard.
Would make sense for instance to avoid the conversion from
a Linux client of JPEG (perhaps reducing bandwidth) but
the conversion would happen on the guest anyway.
I also cannot find any agent capability for image formats
so it's not clear who and how the format is decided.
So I don't see much point in adding other formats.

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

Re: [Spice-devel] [PATCH spice-server] build: Fix wrong -Wno-missing-field-initializers detection

[Christophe Fergeau]

Hey,

On Wed, Nov 09, 2016 at 09:13:32AM +, Frediano Ziglio wrote:
> The small code in m4/manywarnings.m4 wrongly detects if
> -Wno-missing-field-initializers is needed. This happens if
> -Wunused-variable is set. In this case the code fails to compile
> due to -Werror even if -Wno-missing-field-initializers would be
> perfectly fine.

This m4 macro comes from gnulib:
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=m4/manywarnings.m4;h=89fd0ae38e387b9451fb62cb37936ccaefff3686;hb=HEAD#l55
so this would need to be fixed there first.

Christophe

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  m4/manywarnings.m4 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
> index 3e6dd21..dab6a0b 100644
> --- a/m4/manywarnings.m4
> +++ b/m4/manywarnings.m4
> @@ -62,10 +62,11 @@ AC_DEFUN([gl_MANYWARN_ALL_GCC],
>  CFLAGS="$CFLAGS -W -Werror"
>  AC_COMPILE_IFELSE(
>[AC_LANG_PROGRAM(
> - [[void f (void)
> + [[int f (void)
> {
>   typedef struct { int a; int b; } s_t;
>   s_t s1 = { 0, };
> + return s1.b;
> }
>   ]],
>   [[]])],
> -- 
> 2.7.4
> 
> ___
> 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] [vdagent-win RFC PATCH 0/5] Replace CxImage code

[Daniel P. Berrange]

On Wed, Nov 09, 2016 at 10:32:40AM +, Frediano Ziglio wrote:
> CxImage is used for image conversion for clipboard support.
> CxImage have currently some issue:
> - library is old and unsupported;
> - required an old libpng library.
> Currently the MingW binary we distribute due to some
> issue have PNG disabled (so no clipboard image support).
> Note that currently we support (before and after this patch)
> only BMP and PNG. PNG is required as the default agent format.
> However Windows programs wants to have BMP (specifically DIB)
> in the clipboard.
> This patch remove CxImage and use directly libpng (BMP is
> supported directly by Windows APIs).
> Tested with various formats and the compiled agent with a
> Linux client.
> The main concern is actually the possible problem to
> support some weird/old BMP file formats.

Is there any sense in using gdk-pixbuf for image processing to
enable support for formats beyond bmp/png ?  


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [vdagent-win PATCH 2/5] Initial rewrite of image conversion code

[Frediano Ziglio]

Remove CxImage linking.
Support Windows BMP format.

Signed-off-by: Frediano Ziglio 
---
 Makefile.am   |   4 +-
 configure.ac  |   3 -
 vdagent/image.cpp | 182 ++
 vdagent/image.h   |  13 
 4 files changed, 158 insertions(+), 44 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 577a839..5626d0c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -23,8 +23,8 @@ LIBS = -lversion
 
 bin_PROGRAMS = vdagent vdservice
 
-vdagent_LDADD = -lwtsapi32 $(CXIMAGE_LIBS) vdagent_rc.$(OBJEXT)
-vdagent_CXXFLAGS = $(AM_CXXFLAGS) $(CXIMAGE_CFLAGS)
+vdagent_LDADD = -lwtsapi32 -lgdi32 vdagent_rc.$(OBJEXT)
+vdagent_CXXFLAGS = $(AM_CXXFLAGS)
 vdagent_LDFLAGS = $(AM_LDFLAGS) -Wl,--subsystem,windows
 vdagent_SOURCES =  \
common/vdcommon.cpp \
diff --git a/configure.ac b/configure.ac
index 7f6511d..49384f3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -94,9 +94,6 @@ dnl 
---
 dnl - Check library dependencies
 dnl ---
 
-PKG_CHECK_MODULES(CXIMAGE, [cximage])
-CXIMAGE_LIBS=`$PKG_CONFIG --static --libs cximage`
-
 dnl ---
 dnl - Makefiles, etc.
 dnl ---
diff --git a/vdagent/image.cpp b/vdagent/image.cpp
index fb19dbc..4f1dbb1 100644
--- a/vdagent/image.cpp
+++ b/vdagent/image.cpp
@@ -16,71 +16,175 @@
 */
 
 #include 
+#include 
+#include 
 
 #include "vdcommon.h"
 #include "image.h"
 
-#include "ximage.h"
+ImageCoder *create_bitmap_coder();
+ImageCoder *create_png_coder();
 
-typedef struct ImageType {
-uint32_t type;
-DWORD cximage_format;
-} ImageType;
-
-static const ImageType image_types[] = {
-{VD_AGENT_CLIPBOARD_IMAGE_PNG, CXIMAGE_FORMAT_PNG},
-{VD_AGENT_CLIPBOARD_IMAGE_BMP, CXIMAGE_FORMAT_BMP},
-};
-
-static DWORD get_cximage_format(uint32_t type)
+static ImageCoder *get_coder(uint32_t vdagent_type)
 {
-for (unsigned int i = 0; i < SPICE_N_ELEMENTS(image_types); i++) {
-if (image_types[i].type == type) {
-return image_types[i].cximage_format;
-}
+switch (vdagent_type) {
+case VD_AGENT_CLIPBOARD_IMAGE_BMP:
+return create_bitmap_coder();
+case VD_AGENT_CLIPBOARD_IMAGE_PNG:
+return create_png_coder();
 }
-return 0;
+return NULL;
 }
 
 HANDLE get_image_handle(const VDAgentClipboard& clipboard, uint32_t size, UINT 
)
 {
-HANDLE clip_data;
-DWORD cximage_format = get_cximage_format(clipboard.type);
-ASSERT(cximage_format);
-CxImage image((BYTE*)clipboard.data, size, cximage_format);
-clip_data = image.CopyToHandle();
+std::unique_ptr coder(get_coder(clipboard.type));
+if (!coder) {
+return NULL;
+}
+
+format = CF_DIB;
+size_t dib_size = coder->get_dib_size(clipboard.data, size);
+if (!dib_size) {
+return NULL;
+}
+HANDLE clip_data = GlobalAlloc(GMEM_MOVEABLE, dib_size);
+if (clip_data) {
+uint8_t* dst = (uint8_t*)GlobalLock(clip_data);
+if (!dst) {
+GlobalFree(clip_data);
+return NULL;
+}
+coder->get_dib_data(dst, clipboard.data, size);
+GlobalUnlock(clip_data);
+}
 return clip_data;
 }
 
 uint8_t* get_raw_clipboard_image(const VDAgentClipboardRequest& 
clipboard_request,
  HANDLE clip_data, long& new_size)
 {
-CxImage image;
-uint8_t *new_data = NULL;
-DWORD cximage_format = get_cximage_format(clipboard_request.type);
-HPALETTE pal = 0;
+if (GetObjectType(clip_data) != OBJ_BITMAP) {
+return NULL;
+}
+
+std::unique_ptr coder(get_coder(clipboard_request.type));
+if (!coder) {
+return NULL;
+}
 
-ASSERT(cximage_format);
+HPALETTE pal = 0;
 if (IsClipboardFormatAvailable(CF_PALETTE)) {
 pal = (HPALETTE)GetClipboardData(CF_PALETTE);
 }
-if (!image.CreateFromHBITMAP((HBITMAP)clip_data, pal)) {
-vd_printf("Image create from handle failed");
-return NULL;
+
+// extract DIB
+BITMAP bitmap;
+GetObject(clip_data, sizeof(bitmap), );
+
+struct {
+BITMAPINFOHEADER head;
+RGBQUAD colors[256];
+} info;
+
+BITMAPINFOHEADER& head(info.head);
+memset(, 0, sizeof(head));
+head.biSize = sizeof(head);
+head.biWidth = bitmap.bmWidth;
+head.biHeight = bitmap.bmHeight;
+head.biPlanes = bitmap.bmPlanes;
+head.biBitCount = bitmap.bmBitsPixel >= 16 ? 24 : bitmap.bmBitsPixel;
+head.biCompression = BI_RGB;
+
+HDC dc = GetDC(NULL);
+HPALETTE old_pal = NULL;
+if (pal) {
+old_pal = (HPALETTE)SelectObject(dc, pal);
+RealizePalette(dc);
+}
+size_t stride = ((head.biWidth * head.biBitCount + 31u) & 

[Spice-devel] [vdagent-win PATCH 3/5] Write code to decode PNG format

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 Makefile.am  |   6 +-
 configure.ac |   3 +
 vdagent/image.cpp|   8 +-
 vdagent/imagepng.cpp | 244 +++
 vdagent/imagepng.h   |  25 ++
 5 files changed, 277 insertions(+), 9 deletions(-)
 create mode 100644 vdagent/imagepng.cpp
 create mode 100644 vdagent/imagepng.h

diff --git a/Makefile.am b/Makefile.am
index 5626d0c..0f67cff 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -23,8 +23,8 @@ LIBS = -lversion
 
 bin_PROGRAMS = vdagent vdservice
 
-vdagent_LDADD = -lwtsapi32 -lgdi32 vdagent_rc.$(OBJEXT)
-vdagent_CXXFLAGS = $(AM_CXXFLAGS)
+vdagent_LDADD = $(LIBPNG_LIBS) $(ZLIB_LIBS) -lwtsapi32 -lgdi32 
vdagent_rc.$(OBJEXT)
+vdagent_CXXFLAGS = $(AM_CXXFLAGS) $(LIBPNG_CFLAGS)
 vdagent_LDFLAGS = $(AM_LDFLAGS) -Wl,--subsystem,windows
 vdagent_SOURCES =  \
common/vdcommon.cpp \
@@ -44,6 +44,8 @@ vdagent_SOURCES = \
vdagent/as_user.h   \
vdagent/image.cpp   \
vdagent/image.h \
+   vdagent/imagepng.cpp\
+   vdagent/imagepng.h  \
$(NULL)
 
 vdagent_rc.$(OBJEXT): vdagent/vdagent.rc
diff --git a/configure.ac b/configure.ac
index 49384f3..1645393 100644
--- a/configure.ac
+++ b/configure.ac
@@ -94,6 +94,9 @@ dnl 
---
 dnl - Check library dependencies
 dnl ---
 
+PKG_CHECK_MODULES(LIBPNG, [libpng])
+PKG_CHECK_MODULES(ZLIB, [zlib])
+
 dnl ---
 dnl - Makefiles, etc.
 dnl ---
diff --git a/vdagent/image.cpp b/vdagent/image.cpp
index 4f1dbb1..7050e40 100644
--- a/vdagent/image.cpp
+++ b/vdagent/image.cpp
@@ -21,9 +21,9 @@
 
 #include "vdcommon.h"
 #include "image.h"
+#include "imagepng.h"
 
 ImageCoder *create_bitmap_coder();
-ImageCoder *create_png_coder();
 
 static ImageCoder *get_coder(uint32_t vdagent_type)
 {
@@ -182,9 +182,3 @@ ImageCoder *create_bitmap_coder()
 {
 return new BitmapCoder();
 }
-
-// TODO
-ImageCoder *create_png_coder()
-{
-return NULL;
-}
diff --git a/vdagent/imagepng.cpp b/vdagent/imagepng.cpp
new file mode 100644
index 000..c5d3791
--- /dev/null
+++ b/vdagent/imagepng.cpp
@@ -0,0 +1,244 @@
+/*
+   Copyright (C) 2016 Red Hat, Inc.
+
+   This program is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License as
+   published by the Free Software Foundation; either version 2 of
+   the License, or (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see .
+*/
+
+#include "vdcommon.h"
+
+#include 
+#include 
+
+#include "imagepng.h"
+
+class PngCoder: public ImageCoder
+{
+public:
+PngCoder() {};
+size_t get_dib_size(const uint8_t *data, size_t size);
+void get_dib_data(uint8_t *dib, const uint8_t *data, size_t size);
+uint8_t *from_bitmap(const BITMAPINFO& info, const void *bits, long );
+private:
+size_t convert_to_dib(uint8_t *out_buf, const uint8_t *data, size_t size);
+};
+
+struct BufferIo {
+uint8_t *buf;
+uint32_t pos, size;
+};
+
+static void read_from_bufio(png_structp png, png_bytep out, png_size_t size)
+{
+BufferIo& io(*(BufferIo*)png_get_io_ptr(png));
+if (io.pos + size >= io.size)
+png_error(png, "read past end");
+memcpy(out, io.buf+io.pos, size);
+io.pos += size;
+}
+
+size_t PngCoder::get_dib_size(const uint8_t *data, size_t size)
+{
+return convert_to_dib(NULL, data, size);
+}
+
+typedef void line_fixup_t(uint8_t *line, unsigned width);
+
+static void line_fixup_none(uint8_t *line, unsigned width)
+{
+}
+
+static void line_fixup_2to4(uint8_t *line, unsigned width)
+{
+width /= 4u;
+while (width--) {
+uint8_t from = line[width];
+line[width*2+1] = ((from & 0x03) << 0) | ((from & 0x0c) << 2);
+line[width*2+0] = ((from & 0x30) >> 4) | ((from & 0xc0) >> 2);
+}
+}
+
+static void line_fixup_rgb2bgr(uint8_t *line, unsigned width)
+{
+for (; width; --width) {
+std::swap(line[0], line[2]);
+line += 3;
+}
+}
+
+size_t PngCoder::convert_to_dib(uint8_t *out_buf, const uint8_t *data, size_t 
size)
+{
+BufferIo io;
+io.buf = (uint8_t *)data;
+io.pos = 0;
+io.size = size;
+
+png_structp png = png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL, 
NULL, NULL);
+if 

[Spice-devel] [vdagent-win RFC PATCH 0/5] Replace CxImage code

[Frediano Ziglio]

CxImage is used for image conversion for clipboard support.
CxImage have currently some issue:
- library is old and unsupported;
- required an old libpng library.
Currently the MingW binary we distribute due to some
issue have PNG disabled (so no clipboard image support).
Note that currently we support (before and after this patch)
only BMP and PNG. PNG is required as the default agent format.
However Windows programs wants to have BMP (specifically DIB)
in the clipboard.
This patch remove CxImage and use directly libpng (BMP is
supported directly by Windows APIs).
Tested with various formats and the compiled agent with a
Linux client.
The main concern is actually the possible problem to
support some weird/old BMP file formats.

Frediano Ziglio (5):
  Move image handling to a separate file
  Initial rewrite of image conversion code
  Write code to decode PNG format
  Support encoding PNG images
  RFC: Add test for PNG files

 Makefile.am   |  25 +++-
 configure.ac  |   4 +-
 vdagent/image.cpp | 184 +
 vdagent/image.h   |  59 
 vdagent/imagepng.cpp  | 365 ++
 vdagent/imagepng.h|  25 
 vdagent/imagetest.cpp |  77 +++
 vdagent/vdagent.cpp   |  57 ++--
 8 files changed, 742 insertions(+), 54 deletions(-)
 create mode 100644 vdagent/image.cpp
 create mode 100644 vdagent/image.h
 create mode 100644 vdagent/imagepng.cpp
 create mode 100644 vdagent/imagepng.h
 create mode 100644 vdagent/imagetest.cpp

-- 
2.7.4

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

[Spice-devel] [vdagent-win PATCH 4/5] Support encoding PNG images

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 vdagent/imagepng.cpp | 133 ---
 1 file changed, 127 insertions(+), 6 deletions(-)

diff --git a/vdagent/imagepng.cpp b/vdagent/imagepng.cpp
index c5d3791..a4a27db 100644
--- a/vdagent/imagepng.cpp
+++ b/vdagent/imagepng.cpp
@@ -19,6 +19,7 @@
 
 #include 
 #include 
+#include 
 
 #include "imagepng.h"
 
@@ -36,6 +37,13 @@ private:
 struct BufferIo {
 uint8_t *buf;
 uint32_t pos, size;
+bool allocated;
+BufferIo(uint8_t *_buf, uint32_t _size):
+buf(_buf), pos(0), size(_size),
+allocated(false)
+{}
+~BufferIo() { if (allocated) free(buf); }
+uint8_t *release() { allocated = false; return buf; }
 };
 
 static void read_from_bufio(png_structp png, png_bytep out, png_size_t size)
@@ -47,6 +55,26 @@ static void read_from_bufio(png_structp png, png_bytep out, 
png_size_t size)
 io.pos += size;
 }
 
+static void write_to_bufio(png_structp png, png_bytep in, png_size_t size)
+{
+BufferIo& io(*(BufferIo*)png_get_io_ptr(png));
+if (io.pos + size >= io.size) {
+io.allocated = true;
+uint32_t new_size = io.size ? io.size * 2 : 4096;
+uint8_t *p = (uint8_t*) realloc(io.buf, new_size);
+if (!p)
+png_error(png, "out of memory");
+io.buf = p;
+io.size = new_size;
+}
+memcpy(io.buf+io.pos, in, size);
+io.pos += size;
+}
+
+static void flush_bufio(png_structp png)
+{
+}
+
 size_t PngCoder::get_dib_size(const uint8_t *data, size_t size)
 {
 return convert_to_dib(NULL, data, size);
@@ -76,12 +104,21 @@ static void line_fixup_rgb2bgr(uint8_t *line, unsigned 
width)
 }
 }
 
+static void line_fixup_bgr2rgb(uint8_t *dst, const uint8_t *src, unsigned 
width,
+   unsigned src_pixel_len)
+{
+for (; width; --width) {
+dst[0] = src[2];
+dst[1] = src[1];
+dst[2] = src[0];
+dst += 3;
+src += src_pixel_len;
+}
+}
+
 size_t PngCoder::convert_to_dib(uint8_t *out_buf, const uint8_t *data, size_t 
size)
 {
-BufferIo io;
-io.buf = (uint8_t *)data;
-io.pos = 0;
-io.size = size;
+BufferIo io((uint8_t *)data, size);
 
 png_structp png = png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL, 
NULL, NULL);
 if (!png)
@@ -233,9 +270,93 @@ void PngCoder::get_dib_data(uint8_t *dib, const uint8_t 
*data, size_t size)
 convert_to_dib(dib, data, size);
 }
 
-uint8_t *PngCoder::from_bitmap(const BITMAPINFO& info, const void *bits, long 
)
+uint8_t *PngCoder::from_bitmap(const BITMAPINFO& bmp_info, const void *bits, 
long )
 {
-return NULL;
+// this vector is here to avoid problems as libpng use setjmp/longjmp
+std::vector palette;
+BufferIo io(NULL, 0);
+
+png_structp png = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, 
NULL, NULL);
+if (!png)
+return 0;
+
+png_infop info = png_create_info_struct(png);
+if (!info) {
+png_destroy_write_struct(, );
+return 0;
+}
+
+if (setjmp(png_jmpbuf(png))) {
+png_destroy_write_struct(, );
+return 0;
+}
+
+png_set_write_fn(png, , write_to_bufio, flush_bufio);
+
+const BITMAPINFOHEADER& head(bmp_info.bmiHeader);
+int color_type;
+int out_bits = head.biBitCount;
+switch (out_bits) {
+case 1:
+color_type = PNG_COLOR_TYPE_PALETTE;
+break;
+case 4:
+color_type = PNG_COLOR_TYPE_PALETTE;
+break;
+case 8:
+color_type = PNG_COLOR_TYPE_PALETTE;
+break;
+case 24:
+case 32:
+color_type = PNG_COLOR_TYPE_RGB;
+break;
+default:
+longjmp(png_jmpbuf(png), 1);
+break;
+}
+// TODO detect gray
+png_set_IHDR(png, info, head.biWidth, head.biHeight,
+ out_bits > 8 ? 8 : out_bits, color_type,
+ PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_DEFAULT,
+ PNG_FILTER_TYPE_DEFAULT);
+
+// palette
+if (color_type == PNG_COLOR_TYPE_PALETTE) {
+palette.resize(head.biClrUsed);
+const RGBQUAD *rgb = bmp_info.bmiColors;
+for (unsigned color = 0; color < head.biClrUsed; ++color) {
+palette[color].red = rgb->rgbRed;
+palette[color].green = rgb->rgbGreen;
+palette[color].blue = rgb->rgbBlue;
+++rgb;
+}
+png_set_PLTE(png, info, [0], palette.size());
+}
+
+png_write_info(png, info);
+
+const unsigned width = head.biWidth;
+const unsigned height = head.biHeight;
+const size_t stride = ((width * out_bits + 31u) & ~31u) / 8u;
+const size_t image_size = stride * height;
+
+// now do the actual conversion!
+const uint8_t *src = (const uint8_t*)bits + image_size;
+for (unsigned row = 0; row < height; ++row) {
+src -= stride;
+if (out_bits >= 24) {
+uint8_t line[stride];
+line_fixup_bgr2rgb(line, 

[Spice-devel] [vdagent-win PATCH 1/5] Move image handling to a separate file

[Frediano Ziglio]

This will make easier to change code that handle images.

Signed-off-by: Frediano Ziglio 
---
 Makefile.am |  2 ++
 vdagent/image.cpp   | 86 +
 vdagent/image.h | 46 
 vdagent/vdagent.cpp | 57 +--
 4 files changed, 142 insertions(+), 49 deletions(-)
 create mode 100644 vdagent/image.cpp
 create mode 100644 vdagent/image.h

diff --git a/Makefile.am b/Makefile.am
index 84507e8..577a839 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -42,6 +42,8 @@ vdagent_SOURCES = \
vdagent/vdagent.cpp \
vdagent/as_user.cpp \
vdagent/as_user.h   \
+   vdagent/image.cpp   \
+   vdagent/image.h \
$(NULL)
 
 vdagent_rc.$(OBJEXT): vdagent/vdagent.rc
diff --git a/vdagent/image.cpp b/vdagent/image.cpp
new file mode 100644
index 000..fb19dbc
--- /dev/null
+++ b/vdagent/image.cpp
@@ -0,0 +1,86 @@
+/*
+   Copyright (C) 2013-2016 Red Hat, Inc.
+
+   This program is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License as
+   published by the Free Software Foundation; either version 2 of
+   the License, or (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see .
+*/
+
+#include 
+
+#include "vdcommon.h"
+#include "image.h"
+
+#include "ximage.h"
+
+typedef struct ImageType {
+uint32_t type;
+DWORD cximage_format;
+} ImageType;
+
+static const ImageType image_types[] = {
+{VD_AGENT_CLIPBOARD_IMAGE_PNG, CXIMAGE_FORMAT_PNG},
+{VD_AGENT_CLIPBOARD_IMAGE_BMP, CXIMAGE_FORMAT_BMP},
+};
+
+static DWORD get_cximage_format(uint32_t type)
+{
+for (unsigned int i = 0; i < SPICE_N_ELEMENTS(image_types); i++) {
+if (image_types[i].type == type) {
+return image_types[i].cximage_format;
+}
+}
+return 0;
+}
+
+HANDLE get_image_handle(const VDAgentClipboard& clipboard, uint32_t size, UINT 
)
+{
+HANDLE clip_data;
+DWORD cximage_format = get_cximage_format(clipboard.type);
+ASSERT(cximage_format);
+CxImage image((BYTE*)clipboard.data, size, cximage_format);
+clip_data = image.CopyToHandle();
+return clip_data;
+}
+
+uint8_t* get_raw_clipboard_image(const VDAgentClipboardRequest& 
clipboard_request,
+ HANDLE clip_data, long& new_size)
+{
+CxImage image;
+uint8_t *new_data = NULL;
+DWORD cximage_format = get_cximage_format(clipboard_request.type);
+HPALETTE pal = 0;
+
+ASSERT(cximage_format);
+if (IsClipboardFormatAvailable(CF_PALETTE)) {
+pal = (HPALETTE)GetClipboardData(CF_PALETTE);
+}
+if (!image.CreateFromHBITMAP((HBITMAP)clip_data, pal)) {
+vd_printf("Image create from handle failed");
+return NULL;
+}
+if (!image.Encode(new_data, new_size, cximage_format)) {
+vd_printf("Image encode to type %u failed", clipboard_request.type);
+return NULL;
+}
+vd_printf("Image encoded to %lu bytes", new_size);
+return new_data;
+}
+
+void free_raw_clipboard_image(uint8_t *data)
+{
+// this is really just a free however is better to make
+// the free from CxImage code as on Windows the free
+// can be different between libraries
+CxImage image;
+image.FreeMemory(data);
+}
diff --git a/vdagent/image.h b/vdagent/image.h
new file mode 100644
index 000..9ba6003
--- /dev/null
+++ b/vdagent/image.h
@@ -0,0 +1,46 @@
+/*
+   Copyright (C) 2013-2016 Red Hat, Inc.
+
+   This program is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License as
+   published by the Free Software Foundation; either version 2 of
+   the License, or (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see .
+*/
+
+#ifndef VDAGENT_IMAGE_H_
+#define VDAGENT_IMAGE_H_
+
+/**
+ * Returns image to put in the clipboard.
+ * @param clipboard  data to write in the clipboard
+ * @param size   size of data
+ * @param format format decided. This can be changed by the function to
+ *   reflect a better format
+ */
+HANDLE get_image_handle(const 

[Spice-devel] [vdagent-win PATCH 5/5] RFC: Add test for PNG files

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 Makefile.am   | 17 +++-
 vdagent/imagetest.cpp | 77 +++
 2 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 vdagent/imagetest.cpp

diff --git a/Makefile.am b/Makefile.am
index 0f67cff..24df562 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -21,7 +21,7 @@ endif
 # -lversion is needed for the GetFileVersion* API which is used by vdlog.cpp
 LIBS = -lversion
 
-bin_PROGRAMS = vdagent vdservice
+bin_PROGRAMS = vdagent vdservice imagetest
 
 vdagent_LDADD = $(LIBPNG_LIBS) $(ZLIB_LIBS) -lwtsapi32 -lgdi32 
vdagent_rc.$(OBJEXT)
 vdagent_CXXFLAGS = $(AM_CXXFLAGS) $(LIBPNG_CFLAGS)
@@ -68,6 +68,21 @@ vdservice_rc.$(OBJEXT): vdservice/vdservice.rc
 
 MAINTAINERCLEANFILES += vdservice_rc.$(OBJEXT)
 
+imagetest_LDADD = $(LIBPNG_LIBS) $(ZLIB_LIBS) -lwtsapi32 -lgdi32
+imagetest_CXXFLAGS = $(AM_CXXFLAGS) $(LIBPNG_CFLAGS)
+imagetest_LDFLAGS = $(AM_LDFLAGS) -Wl,--subsystem,console
+imagetest_SOURCES =\
+   common/vdcommon.cpp \
+   common/vdcommon.h   \
+   common/vdlog.cpp\
+   common/vdlog.h  \
+   vdagent/imagetest.cpp   \
+   vdagent/image.cpp   \
+   vdagent/image.h \
+   vdagent/imagepng.cpp\
+   vdagent/imagepng.h  \
+   $(NULL)
+
 deps.txt:
$(AM_V_GEN)rpm -qa | grep $(host_os) | sort | unix2dos > $@
 
diff --git a/vdagent/imagetest.cpp b/vdagent/imagetest.cpp
new file mode 100644
index 000..b0dd4e1
--- /dev/null
+++ b/vdagent/imagetest.cpp
@@ -0,0 +1,77 @@
+/*
+   Copyright (C) 2016 Red Hat, Inc.
+
+   This program is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License as
+   published by the Free Software Foundation; either version 2 of
+   the License, or (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see .
+*/
+
+#undef NDEBUG
+#include "vdcommon.h"
+#include "image.h"
+#include "imagepng.h"
+#include 
+#include 
+
+int main(int argc, char **argv)
+{
+ImageCoder *coder = create_png_coder();
+
+assert(coder);
+assert(argc >= 1);
+
+// read all file into memory
+FILE *f = fopen(argv[1], "rb");
+assert(f);
+assert(fseek(f, 0, SEEK_END) == 0);
+long len = ftell(f);
+assert(fseek(f, 0, SEEK_SET) == 0);
+
+std::vector data(len);
+assert(fread([0], 1, len, f) == len);
+fclose(f);
+
+size_t dib_size = coder->get_dib_size([0], len);
+assert(dib_size);
+std::vector out(dib_size);
+memset([0], 0xcc, dib_size);
+coder->get_dib_data([0], [0], len);
+
+// looks like many tools wants this header so craft it
+BITMAPFILEHEADER head;
+memset(, 0, sizeof(head));
+head.bfType = 'B'+'M'*256u;
+head.bfSize = sizeof(head) + dib_size;
+BITMAPINFOHEADER& info(*(BITMAPINFOHEADER*)[0]);
+head.bfOffBits = sizeof(head) + sizeof(BITMAPINFOHEADER) + 4 * 
info.biClrUsed;
+
+f = fopen("out.bmp", "wb");
+assert(f);
+assert(fwrite(, 1, sizeof(head), f) == sizeof(head));
+assert(fwrite([0], 1, dib_size, f) == dib_size);
+fclose(f);
+
+// convert back to PNG
+long png_size = 0;
+uint8_t *png = coder->from_bitmap(*((BITMAPINFO*)[0]), 
[sizeof(BITMAPINFOHEADER) + 4 * info.biClrUsed], png_size);
+assert(png && png_size);
+
+f = fopen("out.png", "wb");
+assert(f);
+assert(fwrite(png, 1, png_size, f) == png_size);
+fclose(f);
+free(png);
+png = NULL;
+
+return 0;
+}
+
-- 
2.7.4

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

[Spice-devel] [vdagent-win 2/2] build: Update copyright/version in vdagent.rc/vdservice.rc

[Christophe Fergeau]

They were very outdated
---
 vdagent/vdagent.rc | 10 +-
 vdservice/vdservice.rc | 10 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/vdagent/vdagent.rc b/vdagent/vdagent.rc
index 0973068..4a5c330 100644
--- a/vdagent/vdagent.rc
+++ b/vdagent/vdagent.rc
@@ -53,8 +53,8 @@ END
 //
 
 VS_VERSION_INFO VERSIONINFO
- FILEVERSION 0,5,1,0
- PRODUCTVERSION 0,5,1,0
+ FILEVERSION 0,8,0,0
+ PRODUCTVERSION 0,8,0,0
  FILEFLAGSMASK 0x17L
 #ifdef _DEBUG
  FILEFLAGS 0x1L
@@ -71,12 +71,12 @@ BEGIN
 BEGIN
 VALUE "CompanyName", "Red Hat Inc."
 VALUE "FileDescription", "Spice agent"
-VALUE "FileVersion", "0, 5, 1, 0"
+VALUE "FileVersion", "0,8,0,0"
 VALUE "InternalName", "vdagent"
-VALUE "LegalCopyright", "Copyright (c) 2009  Red Hat Inc. and/or 
its affiliates"
+VALUE "LegalCopyright", "Copyright (c) 2009-2016  Red Hat Inc. 
and/or its affiliates"
 VALUE "OriginalFilename", "vdagent.exe"
 VALUE "ProductName", "Red Hat Spice"
-VALUE "ProductVersion", "0, 5, 1, 0"
+VALUE "ProductVersion", "0,8,0,0"
 END
 END
 BLOCK "VarFileInfo"
diff --git a/vdservice/vdservice.rc b/vdservice/vdservice.rc
index f0bf88f..244c932 100644
--- a/vdservice/vdservice.rc
+++ b/vdservice/vdservice.rc
@@ -53,8 +53,8 @@ END
 //
 
 VS_VERSION_INFO VERSIONINFO
- FILEVERSION 0,5,1,0
- PRODUCTVERSION 0,5,1,0
+ FILEVERSION 0,8,0,0
+ PRODUCTVERSION 0,8,0,0
  FILEFLAGSMASK 0x17L
 #ifdef _DEBUG
  FILEFLAGS 0x1L
@@ -71,12 +71,12 @@ BEGIN
 BEGIN
 VALUE "CompanyName", "Red Hat Inc."
 VALUE "FileDescription", "Spice service"
-VALUE "FileVersion", "0, 5, 1, 0"
+VALUE "FileVersion", "0,8,0,0"
 VALUE "InternalName", "vdservice"
-VALUE "LegalCopyright", "Copyright (c) 2009  Red Hat Inc. and/or 
its affiliates"
+VALUE "LegalCopyright", "Copyright (c) 2009-2016  Red Hat Inc. 
and/or its affiliates"
 VALUE "OriginalFilename", "vdservice.exe"
 VALUE "ProductName", "Red Hat Spice"
-VALUE "ProductVersion", "0, 5, 1, 0"
+VALUE "ProductVersion", "0,8,0,0"
 END
 END
 BLOCK "VarFileInfo"
-- 
2.9.3

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

[Spice-devel] [vdagent-win 1/2] build: Automatically update version/copyright in .rc files

[Christophe Fergeau]

The versions/copyright in vdservice.rc and vdagent.rc are very outdated.
Updating them automatically at configure time should ensure they are
updated more often.
The generated .rc files are kept in git as they are needed for VC++
builds.
---
 configure.ac  |   8 +++-
 vdagent/vdagent.rc.in | 102 ++
 vdservice/vdservice.rc.in | 102 ++
 3 files changed, 211 insertions(+), 1 deletion(-)
 create mode 100644 vdagent/vdagent.rc.in
 create mode 100644 vdservice/vdservice.rc.in

diff --git a/configure.ac b/configure.ac
index 7f6511d..6689602 100644
--- a/configure.ac
+++ b/configure.ac
@@ -32,7 +32,11 @@ fi
 build=`expr $micro \* 256 + $buildid`
 WINDOWS_PRODUCTVERSION="$major.$minor.$build"
 AC_SUBST([WINDOWS_PRODUCTVERSION])
-
+microdigit=`echo $micro | cut -d- -f1`
+RC_PRODUCTVERSION="$major,$minor,$microdigit,0"
+AC_SUBST([RC_PRODUCTVERSION])
+BUILD_YEAR=`date +%Y`
+AC_SUBST([BUILD_YEAR])
 # Check for programs
 AC_PROG_CC
 AC_PROG_CXX
@@ -105,6 +109,8 @@ AC_CONFIG_FILES([
 Makefile
 mingw-spice-vdagent.spec
 spice-vdagent.wxs
+vdagent/vdagent.rc
+vdservice/vdservice.rc
 ])
 AC_OUTPUT
 
diff --git a/vdagent/vdagent.rc.in b/vdagent/vdagent.rc.in
new file mode 100644
index 000..7c4481f
--- /dev/null
+++ b/vdagent/vdagent.rc.in
@@ -0,0 +1,102 @@
+// Microsoft Visual C++ generated resource script.
+//
+#include "resource.h"
+
+#define APSTUDIO_READONLY_SYMBOLS
+/
+//
+// Generated from the TEXTINCLUDE 2 resource.
+//
+#include "afxres.h"
+
+/
+#undef APSTUDIO_READONLY_SYMBOLS
+
+/
+// Hebrew resources
+
+#if !defined(AFX_RESOURCE_DLL) || defined(AFX_TARG_HEB)
+#ifdef _WIN32
+LANGUAGE LANG_HEBREW, SUBLANG_DEFAULT
+#pragma code_page(1255)
+#endif //_WIN32
+
+#ifdef APSTUDIO_INVOKED
+/
+//
+// TEXTINCLUDE
+//
+
+1 TEXTINCLUDE 
+BEGIN
+"resource.h\0"
+END
+
+2 TEXTINCLUDE 
+BEGIN
+"#include ""afxres.h""\r\n"
+"\0"
+END
+
+3 TEXTINCLUDE 
+BEGIN
+"\r\n"
+"\0"
+END
+
+#endif// APSTUDIO_INVOKED
+
+
+/
+//
+// Version
+//
+
+VS_VERSION_INFO VERSIONINFO
+ FILEVERSION @RC_PRODUCTVERSION@
+ PRODUCTVERSION @RC_PRODUCTVERSION@
+ FILEFLAGSMASK 0x17L
+#ifdef _DEBUG
+ FILEFLAGS 0x1L
+#else
+ FILEFLAGS 0x0L
+#endif
+ FILEOS 0x4L
+ FILETYPE 0x1L
+ FILESUBTYPE 0x0L
+BEGIN
+BLOCK "StringFileInfo"
+BEGIN
+BLOCK "000904b0"
+BEGIN
+VALUE "CompanyName", "Red Hat Inc."
+VALUE "FileDescription", "Spice agent"
+VALUE "FileVersion", "@RC_PRODUCTVERSION@"
+VALUE "InternalName", "vdagent"
+VALUE "LegalCopyright", "Copyright (c) 2009-@BUILD_YEAR@  Red Hat 
Inc. and/or its affiliates"
+VALUE "OriginalFilename", "vdagent.exe"
+VALUE "ProductName", "Red Hat Spice"
+VALUE "ProductVersion", "@RC_PRODUCTVERSION@"
+END
+END
+BLOCK "VarFileInfo"
+BEGIN
+VALUE "Translation", 0x9, 1200
+END
+END
+
+#endif// Hebrew resources
+/
+
+
+
+#ifndef APSTUDIO_INVOKED
+/
+//
+// Generated from the TEXTINCLUDE 3 resource.
+//
+
+
+/
+#endif// not APSTUDIO_INVOKED
+
diff --git a/vdservice/vdservice.rc.in b/vdservice/vdservice.rc.in
new file mode 100644
index 000..6a2284f
--- /dev/null
+++ b/vdservice/vdservice.rc.in
@@ -0,0 +1,102 @@
+// Microsoft Visual C++ generated resource script.
+//
+#include "resource.h"
+
+#define APSTUDIO_READONLY_SYMBOLS
+/
+//
+// Generated from the TEXTINCLUDE 2 resource.
+//
+#include "afxres.h"
+
+/
+#undef APSTUDIO_READONLY_SYMBOLS
+
+/
+// Hebrew resources
+
+#if !defined(AFX_RESOURCE_DLL) || defined(AFX_TARG_HEB)
+#ifdef _WIN32
+LANGUAGE LANG_HEBREW, SUBLANG_DEFAULT
+#pragma code_page(1255)
+#endif //_WIN32
+
+#ifdef APSTUDIO_INVOKED
+/
+//
+// TEXTINCLUDE
+//
+
+1 TEXTINCLUDE 
+BEGIN
+"resource.h\0"
+END
+
+2 TEXTINCLUDE 
+BEGIN
+"#include ""afxres.h""\r\n"
+"\0"
+END
+
+3 TEXTINCLUDE 
+BEGIN
+"\r\n"
+"\0"
+END
+
+#endif// APSTUDIO_INVOKED
+
+
+/
+//

[Spice-devel] [PATCH spice-server v3 5/5] Compatibility for GStreamer 0.10 for test utility

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/tests/gst-test.c | 61 +++--
 1 file changed, 59 insertions(+), 2 deletions(-)

Changes since v2:
- use a gst_buffer_new_wrapped_full replacement;
- use 1.0 style for gst_bus_set_sync_handler.
(all comments by Christophe).

diff --git a/server/tests/gst-test.c b/server/tests/gst-test.c
index 62f1f1c..bee1f22 100644
--- a/server/tests/gst-test.c
+++ b/server/tests/gst-test.c
@@ -69,6 +69,57 @@ typedef struct {
 SpiceBitmap *bitmap;
 } TestFrame;
 
+#ifdef HAVE_GSTREAMER_0_10
+
+#define VIDEOCONVERT "ffmpegcolorspace"
+#define BGRx_CAPS 
"caps=video/x-raw-rgb,bpp=32,depth=24,blue_mask=-16777216,green_mask=16711680,red_mask=65280"
+
+typedef GstBuffer GstSample;
+#define gst_sample_get_buffer(s) (s)
+#define gst_sample_get_caps(s) GST_BUFFER_CAPS(s)
+#define gst_sample_unref(s) gst_buffer_unref(s)
+#define gst_app_sink_pull_sample(s) gst_app_sink_pull_buffer(s)
+typedef struct {
+uint8_t *data;
+} GstMapInfo;
+#define GST_MAP_READ 1
+static inline void
+gst_buffer_unmap(GstBuffer *buffer, GstMapInfo *mapinfo)
+{ }
+
+static inline gboolean
+gst_buffer_map(GstBuffer *buffer, GstMapInfo *mapinfo, int flags)
+{
+mapinfo->data = GST_BUFFER_DATA(buffer);
+return mapinfo->data != NULL;
+}
+
+static GstBuffer*
+gst_buffer_new_wrapped_full(int flags SPICE_GNUC_UNUSED, gpointer data, gsize 
maxsize,
+gsize offset, gsize size,
+gpointer user_data, GDestroyNotify notify)
+{
+GstBuffer *buffer = gst_buffer_new();
+
+buffer->malloc_data = user_data;
+GST_BUFFER_FREE_FUNC(buffer) = notify;
+GST_BUFFER_DATA(buffer) = (uint8_t *) data + offset;
+GST_BUFFER_SIZE(buffer) = size;
+
+return buffer;
+}
+
+#define GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS 0
+
+#define gst_bus_set_sync_handler(bus, proc, param, destroy) G_STMT_START {\
+verify(destroy == NULL); \
+gst_bus_set_sync_handler(bus, proc, param); \
+} G_STMT_END
+#else
+#define VIDEOCONVERT "videoconvert"
+#define BGRx_CAPS "caps=video/x-raw,format=BGRx"
+#endif
+
 typedef void (*SampleProc)(GstSample *sample, void *param);
 
 typedef struct {
@@ -230,7 +281,11 @@ static const EncoderInfo encoder_infos[] = {
 { "gstreamer:vp8",   gstreamer_encoder_new, SPICE_VIDEO_CODEC_TYPE_VP8,
   "caps=video/x-vp8", "vp8dec" },
 { "gstreamer:h264",  gstreamer_encoder_new, SPICE_VIDEO_CODEC_TYPE_H264,
+#ifdef HAVE_GSTREAMER_0_10
+  "", "h264parse ! ffdec_h264" },
+#else
   "", "h264parse ! avdec_h264" },
+#endif
 { NULL, NULL }
 };
 
@@ -526,7 +581,7 @@ create_output_pipeline(const EncoderInfo *encoder, 
SampleProc sample_proc, void
 {
 gchar *desc =
 g_strdup_printf("appsrc name=src is-live=true format=time max-bytes=0 
block=true "
-"%s ! %s ! videoconvert ! appsink name=sink 
caps=video/x-raw,format=BGRx"
+"%s ! %s ! " VIDEOCONVERT " ! appsink name=sink " 
BGRx_CAPS
 " sync=false drop=false", encoder->caps, 
encoder->decoder);
 
 TestPipeline *pipeline = create_pipeline(desc, sample_proc, param);
@@ -543,7 +598,7 @@ static void
 create_input_pipeline(const char *input_pipeline_desc, SampleProc sample_proc, 
void *param)
 {
 gchar *desc =
-g_strdup_printf("%s ! appsink name=sink caps=video/x-raw,format=BGRx"
+g_strdup_printf("%s ! appsink name=sink " BGRx_CAPS
 " sync=false drop=false", input_pipeline_desc);
 
 TestPipeline *pipeline = create_pipeline(desc, sample_proc, param);
@@ -573,7 +628,9 @@ pipeline_send_raw_data(TestPipeline *pipeline, VideoBuffer 
*video_buffer)
 video_buffer, (void (*)(void*)) 
video_buffer_release);
 
 GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
+#ifndef HAVE_GSTREAMER_0_10
 GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
+#endif
 
 if (gst_app_src_push_buffer(pipeline->appsrc, buffer) != GST_FLOW_OK) {
 g_printerr("GStreamer error: unable to push frame of size %u\n", 
video_buffer->size);
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server v3 3/5] Add an helper to test VideoEncoder

[Frediano Ziglio]

Add an utility to make possible to check various features of
VideoEncoder.
2 GStreamer plugins are used in a chain like this:
  (1) input pipeline -> (2) video encoder -> (3) output pipeline
While converting output from (1) is compared with output of (3)
making sure the streaming is working correctly.
You can set various options:
- part of the input pipeline description to allow specifying different
  video from GStreamer test ones to a video file;
- the encoder to use;
- different image properties to use for (2) input:
  - different bit depth;
  - top/down or down/up;
- initial bitrate.

The idea is to use this helper in combination with a shell script
and some video sources to make able to test various settings.
Also can be used to extend the current encoder list.

As an example you can use a command like

$ ./gst-test -e gstreamer:vp8 -i \
  'filesrc location=bbb_sunflower_1080p_30fps_normal.mp4 \
  ! decodebin ! videoconvert'

to check vp8 encoding.

Currently it does not emulate bandwidth changes as stream reports
from the client are not coded.

Signed-off-by: Frediano Ziglio 
---
 server/tests/Makefile.am |   9 +
 server/tests/gst-test.c  | 957 +++
 2 files changed, 966 insertions(+)
 create mode 100644 server/tests/gst-test.c

Changes since v2:
- add description to the program;
- separate caps and decoder in EncoderInfo;
- specify clipping units;
- expand a macro.
(all comments by Christophe).

diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index 8580a9a..c799779 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -57,6 +57,7 @@ noinst_PROGRAMS = \
test_vdagent\
test_display_width_stride   \
spice-server-replay \
+   gst-test\
$(check_PROGRAMS)   \
$(NULL)
 
@@ -105,3 +106,11 @@ libstat_test4_a_SOURCES = stat-test.c
 libstat_test4_a_CPPFLAGS = $(AM_CPPFLAGS) -DTEST_COMPRESS_STAT=1 
-DTEST_RED_WORKER_STAT=1 -DTEST_NAME=stat_test4
 
 test_qxl_parsing_LDADD = ../libserver.la $(LDADD)
+
+gst_test_SOURCES = gst-test.c \
+   $(NULL)
+gst_test_CPPFLAGS = \
+   $(AM_CPPFLAGS) \
+   $(GSTREAMER_0_10_CFLAGS)\
+   $(GSTREAMER_1_0_CFLAGS) \
+   $(NULL)
diff --git a/server/tests/gst-test.c b/server/tests/gst-test.c
new file mode 100644
index 000..62f1f1c
--- /dev/null
+++ b/server/tests/gst-test.c
@@ -0,0 +1,957 @@
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2016 Red Hat, Inc.
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see .
+*/
+/* Utility to check video encoder code
+ * (see program_description below)
+ */
+#undef NDEBUG
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "video-encoder.h"
+#include "spice-bitmap-utils.h"
+#include "reds.h" // reds_get_mm_time
+
+static const char program_description[] =
+"2 GStreamer plugins are used in a chain like this:\n"
+"(1) input pipeline -> (2) video encoder -> (3) output pipeline\n"
+"While converting output from (1) is compared with output of (3)\n"
+"making sure the streaming is working correctly.\n"
+"\n"
+"As an example you can use a command like\n"
+"\n"
+"  $ ./gst-test -e gstreamer:vp8 -i \\\n"
+"'filesrc location=bbb_sunflower_1080p_30fps_normal.mp4 \\\n"
+"! decodebin ! videoconvert'\n"
+"\n"
+"to check vp8 encoding.";
+
+// clipping informations passed in command line
+typedef enum {
+COORDS_INVALID,
+COORDS_NUMBER,
+COORDS_PERCENT,
+} CoordsUnit;
+static struct {
+unsigned int value;
+CoordsUnit unit;
+} clipping_coords[4];
+enum {
+COORDS_BOX,
+COORDS_SIZE
+} clipping_type;
+
+typedef struct {
+gint refs;
+SpiceBitmap *bitmap;
+} TestFrame;
+
+typedef void (*SampleProc)(GstSample *sample, void *param);
+
+typedef struct {
+GstAppSrc *appsrc;
+GstAppSink *appsink;
+GstElement *gst_pipeline;
+SampleProc sample_proc;
+void *sample_param;
+gboolean got_eos;
+} TestPipeline;
+
+typedef struct {
+const char *name;
+new_video_encoder_t new_encoder;
+

[Spice-devel] [PATCH spice-server v3 1/5] Handle top down bitmaps dumping

[Frediano Ziglio]

The top down flag can be specified using negative heights.

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

diff --git a/server/spice-bitmap-utils.c b/server/spice-bitmap-utils.c
index 72a9285..439f05d 100644
--- a/server/spice-bitmap-utils.c
+++ b/server/spice-bitmap-utils.c
@@ -269,7 +269,7 @@ void dump_bitmap(SpiceBitmap *bitmap)
 put_32le(, bitmap_data_offset);
 put_32le(, header_size - 14);
 put_32le(, bitmap->x);
-put_32le(, bitmap->y);
+put_32le(, bitmap->flags & SPICE_BITMAP_FLAGS_TOP_DOWN ? -bitmap->y : 
bitmap->y);
 
 put_16le(, 1); // plane
 put_16le(, n_pixel_bits);
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server v3 4/5] gstreamer: Do not warn for tested formats

[Frediano Ziglio]

These formats were tested manually using gst-test utility
in server/tests.

Signed-off-by: Frediano Ziglio 
---
 server/gstreamer-encoder.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Changes since v2:
- extended commit message.

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 54e1b2e..27872cf 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -758,7 +758,7 @@ static const SpiceFormatForGStreamer format_map[] =  {
 {SPICE_BITMAP_FMT_INVALID, "", 0, 0, 0, 0, 0, 0},
 {SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff, 
0xff00},
 {SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0, 0x7C00},
-/* TODO: Test the other formats */
+/* TODO: Test the other formats under GStreamer 0.10*/
 {SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00, 0xff, 
0xff00},
 {SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00, 0xff},
 };
@@ -770,9 +770,11 @@ static const SpiceFormatForGStreamer 
*map_format(SpiceBitmapFmt format)
 int i;
 for (i = 0; i < G_N_ELEMENTS(format_map); i++) {
 if (format_map[i].spice_format == format) {
+#ifdef HAVE_GSTREAMER_0_10
 if (i > 2) {
 spice_warning("The %d format has not been tested yet", format);
 }
+#endif
 return _map[i];
 }
 }
-- 
2.7.4

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

[Spice-devel] [PATCH spice-server v3 0/5] Gstreamer testing

[Frediano Ziglio]

This patch series add some tests fot Gstreamer.

Frediano Ziglio (5):
  Handle top down bitmaps dumping
  Simplify gstreamer 0.10 compatibility
  Add an helper to test VideoEncoder
  gstreamer: Do not warn for tested formats
  Compatibility for GStreamer 0.10 for test utility

 server/gstreamer-encoder.c  |   10 +-
 server/spice-bitmap-utils.c |2 +-
 server/tests/Makefile.am|9 +
 server/tests/gst-test.c | 1014 +++
 4 files changed, 1028 insertions(+), 7 deletions(-)
 create mode 100644 server/tests/gst-test.c

-- 
2.7.4

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

[Spice-devel] [PATCH spice-server v3 2/5] Simplify gstreamer 0.10 compatibility

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/gstreamer-encoder.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 3d5f8a0..54e1b2e 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -969,11 +969,7 @@ static gboolean create_pipeline(SpiceGstEncoder *encoder)
 encoder->gstenc = gst_bin_get_by_name(GST_BIN(encoder->pipeline), 
"encoder");
 encoder->appsink = 
GST_APP_SINK(gst_bin_get_by_name(GST_BIN(encoder->pipeline), "sink"));
 
-#ifdef HAVE_GSTREAMER_0_10
-GstAppSinkCallbacks appsink_cbs = {NULL, NULL, _sample, NULL, {NULL}};
-#else
-GstAppSinkCallbacks appsink_cbs = {NULL, NULL, _sample, {NULL}};
-#endif
+GstAppSinkCallbacks appsink_cbs = {NULL, NULL, _sample, ._gst_reserved 
= {NULL}};
 gst_app_sink_set_callbacks(encoder->appsink, _cbs, encoder, NULL);
 
 /* Hook into the bus so we can handle errors */
-- 
2.7.4

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

Re: [Spice-devel] [PATCH spice-server v2 4/6] Add an helper to test VideoEncoder

[Frediano Ziglio]

 
> On Fri, Oct 21, 2016 at 01:40:38PM +0100, Frediano Ziglio wrote:
> > Add an utility to make possible to check various features of
> > VideoEncoder.
> > 2 GStreamer plugins are used in a chain like this:
> >   (1) input pipeline -> (2) video encoder -> (3) output pipeline
> > While converting output from (1) is compared with output of (3)
> > making sure the streaming is working correctly.
> 
> Maybe a short high level description could be added to the source of the
> test program too.
> 

Yes, I'll put some additional comments too.
Or maybe in the usage description.

> For what it's worth, that's a lot of code, sometimes quite close to code
> already present in spice-server or spice-gtk. Hopefully the testcase by
> itself will not have too many bugs ;)
> 

Well, in case of tests some bug are acceptable :-)
The first compiling version was not far from this one.

> > You can set various options:
> > - part of the input pipeline description to allow specifying different
> >   video from GStreamer test ones to a video file;
> > - the encoder to use;
> > - different image properties to use for (2) input:
> >   - different bit depth;
> >   - top/down or down/up;
> > - initial bitrate.
> > 
> > The idea is to use this helper in combination with a shell script
> > and some video sources to make able to test various settings.
> > Also can be used to extend the current encoder list.
> > 
> > As an example you can use a command like
> > 
> > $ ./gst-test -e gstreamer:vp8 -i \
> >   'filesrc location=bbb_sunflower_1080p_30fps_normal.mp4 \
> >   ! decodebin ! videoconvert'
> > 
> > to check vp8 encoding.
> > 
> > Currently it does not emulate bandwidth changes as stream reports
> > from the client are not coded.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/tests/Makefile.am |   9 +
> >  server/tests/gst-test.c  | 936
> >  +++
> >  2 files changed, 945 insertions(+)
> >  create mode 100644 server/tests/gst-test.c
> > 
> > Changes since v1:
> > - added option to split image into multiple chunks;
> > - added option to specify minimum PSNR;
> > - fix check for video finished;
> > - do not queue too much data to output;
> > - fix clipping;
> > - remove RFC.
> > 
> > diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
> > index 8580a9a..c799779 100644
> > --- a/server/tests/Makefile.am
> > +++ b/server/tests/Makefile.am
> > @@ -57,6 +57,7 @@ noinst_PROGRAMS = \
> > test_vdagent\
> > test_display_width_stride   \
> > spice-server-replay \
> > +   gst-test\
> > $(check_PROGRAMS)   \
> > $(NULL)
> >  
> > @@ -105,3 +106,11 @@ libstat_test4_a_SOURCES = stat-test.c
> >  libstat_test4_a_CPPFLAGS = $(AM_CPPFLAGS) -DTEST_COMPRESS_STAT=1
> >  -DTEST_RED_WORKER_STAT=1 -DTEST_NAME=stat_test4
> >  
> >  test_qxl_parsing_LDADD = ../libserver.la $(LDADD)
> > +
> > +gst_test_SOURCES = gst-test.c \
> > +   $(NULL)
> > +gst_test_CPPFLAGS = \
> > +   $(AM_CPPFLAGS) \
> > +   $(GSTREAMER_0_10_CFLAGS)\
> > +   $(GSTREAMER_1_0_CFLAGS) \
> > +   $(NULL)
> > diff --git a/server/tests/gst-test.c b/server/tests/gst-test.c
> > new file mode 100644
> > index 000..0a68d7d
> > --- /dev/null
> > +++ b/server/tests/gst-test.c
> > @@ -0,0 +1,936 @@
> > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > +/*
> > +   Copyright (C) 2016 Red Hat, Inc.
> > +
> > +   This library is free software; you can redistribute it and/or
> > +   modify it under the terms of the GNU Lesser General Public
> > +   License as published by the Free Software Foundation; either
> > +   version 2.1 of the License, or (at your option) any later version.
> > +
> > +   This library is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +   Lesser General Public License for more details.
> > +
> > +   You should have received a copy of the GNU Lesser General Public
> > +   License along with this library; if not, see
> > .
> > +*/
> > +/* Utility to check video encoder code
> > + */
> [snip]
> > +
> > +// handle output frames from input pipeline
> > +static void
> > +input_frames(GstSample *sample, void *param)
> > +{
> > +spice_assert(video_encoder && sample);
> > +
> > +if (SPICE_UNLIKELY(!clipping_type_computed)) {
> > +compute_clipping_rect(sample);
> > +}
> > +
> > +VideoBuffer *p_outbuf = NULL;
> > +// TODO correct ?? emulate another timer ??
> > +uint32_t frame_mm_time = reds_get_mm_time();
> > +
> > +// convert frame to SpiceBitmap/DRM prime
> > +TestFrame *frame = gst_to_spice_frame(sample);
> > +
> > +// send frame to our video encoder (must be from a single 

Re: [Spice-devel] [PATCH spice-server v2 1/6] gstreamer: Do not warn for tested formats

[Christophe Fergeau]

On Wed, Nov 09, 2016 at 04:17:19AM -0500, Frediano Ziglio wrote:
> > 
> > 
> > From the commit log, I have no idea why/how they were tested now. If you
> > tested them locally, just mention this in the log.
> > 
> 
> This was tested with the utility at 4/6 and the script at 6/6.
> 
> Maybe:
> 
> "These formats were tested manually using a small test utility."


You could move it after the addition of the utility, and say that they
were tested with the newly introduced test program.

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-server v2 1/6] gstreamer: Do not warn for tested formats

[Frediano Ziglio]

> 
> 
> From the commit log, I have no idea why/how they were tested now. If you
> tested them locally, just mention this in the log.
> 

This was tested with the utility at 4/6 and the script at 6/6.

Maybe:

"These formats were tested manually using a small test utility."

> Acked-by: Christophe Fergeau 
> 
> Christophe
> 

Frediano

> On Fri, Oct 21, 2016 at 01:40:35PM +0100, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/gstreamer-encoder.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> > index d575c67..53dfb98 100644
> > --- a/server/gstreamer-encoder.c
> > +++ b/server/gstreamer-encoder.c
> > @@ -757,7 +757,7 @@ static const SpiceFormatForGStreamer format_map[] =  {
> >  {SPICE_BITMAP_FMT_INVALID, "", 0, 0, 0, 0, 0, 0},
> >  {SPICE_BITMAP_FMT_RGBA, "BGRA", 32, 24, 4321, 0xff00, 0xff,
> >  0xff00},
> >  {SPICE_BITMAP_FMT_16BIT, "RGB15", 16, 15, 4321, 0x001f, 0x03E0,
> >  0x7C00},
> > -/* TODO: Test the other formats */
> > +/* TODO: Test the other formats under GStreamer 0.10*/
> >  {SPICE_BITMAP_FMT_32BIT, "BGRx", 32, 24, 4321, 0xff00, 0xff,
> >  0xff00},
> >  {SPICE_BITMAP_FMT_24BIT, "BGR", 24, 24, 4321, 0xff, 0xff00, 0xff},
> >  };
> > @@ -769,9 +769,11 @@ static const SpiceFormatForGStreamer
> > *map_format(SpiceBitmapFmt format)
> >  int i;
> >  for (i = 0; i < G_N_ELEMENTS(format_map); i++) {
> >  if (format_map[i].spice_format == format) {
> > +#ifdef HAVE_GSTREAMER_0_10
> >  if (i > 2) {
> >  spice_warning("The %d format has not been tested yet",
> >  format);
> >  }
> > +#endif
> >  return _map[i];
> >  }
> >  }
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v2 3/6] Simplify gstreamer 0.10 compatibility

[Frediano Ziglio]

> On Fri, Oct 21, 2016 at 01:40:37PM +0100, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/gstreamer-encoder.c | 6 +-
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> > index 53dfb98..d8437e4 100644
> > --- a/server/gstreamer-encoder.c
> > +++ b/server/gstreamer-encoder.c
> > @@ -967,11 +967,7 @@ static gboolean create_pipeline(SpiceGstEncoder
> > *encoder)
> >  encoder->gstenc = gst_bin_get_by_name(GST_BIN(encoder->pipeline),
> >  "encoder");
> >  encoder->appsink =
> >  GST_APP_SINK(gst_bin_get_by_name(GST_BIN(encoder->pipeline),
> >  "sink"));
> >  
> > -#ifdef HAVE_GSTREAMER_0_10
> > -GstAppSinkCallbacks appsink_cbs = {NULL, NULL, _sample, NULL,
> > {NULL}};
> > -#else
> > -GstAppSinkCallbacks appsink_cbs = {NULL, NULL, _sample, {NULL}};
> > -#endif
> > +GstAppSinkCallbacks appsink_cbs = { NULL, NULL, new_sample };
> 
> This actually breaks the build with -Werror=missing-field-initializers
> which we have by default in our flags.
> 
> Christophe
> 

Apparently on my build the same script that is supposed to turn this
flag on also turn it off later!
I'll write an update for both.

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

[Spice-devel] [PATCH spice-server] build: Fix wrong -Wno-missing-field-initializers detection

[Frediano Ziglio]

The small code in m4/manywarnings.m4 wrongly detects if
-Wno-missing-field-initializers is needed. This happens if
-Wunused-variable is set. In this case the code fails to compile
due to -Werror even if -Wno-missing-field-initializers would be
perfectly fine.

Signed-off-by: Frediano Ziglio 
---
 m4/manywarnings.m4 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
index 3e6dd21..dab6a0b 100644
--- a/m4/manywarnings.m4
+++ b/m4/manywarnings.m4
@@ -62,10 +62,11 @@ AC_DEFUN([gl_MANYWARN_ALL_GCC],
 CFLAGS="$CFLAGS -W -Werror"
 AC_COMPILE_IFELSE(
   [AC_LANG_PROGRAM(
- [[void f (void)
+ [[int f (void)
{
  typedef struct { int a; int b; } s_t;
  s_t s1 = { 0, };
+ return s1.b;
}
  ]],
  [[]])],
-- 
2.7.4

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