Re: [Spice-devel] [PATCH 14/15] server: move dispatcher GSource handling code
On Thu, 2015-12-03 at 16:27 +, Frediano Ziglio wrote:
> From: Marc-André Lureau
>
> ---
> server/dispatcher.c | 25 +
> server/dispatcher.h | 2 ++
> server/red-worker.c | 40
> 3 files changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/server/dispatcher.c b/server/dispatcher.c
> index d6c03ca..974fa75 100644
> --- a/server/dispatcher.c
> +++ b/server/dispatcher.c
> @@ -289,3 +289,28 @@ int dispatcher_get_recv_fd(Dispatcher *dispatcher)
> {
> return dispatcher->recv_fd;
> }
> +
> +static gboolean dispatch_cb(GIOChannel *source, GIOCondition condition,
> +gpointer data)
> +{
> +Dispatcher *dispatcher = data;
> +
> +spice_debug(NULL);
> +dispatcher_handle_recv_read(dispatcher);
> +
> +/* FIXME: remove source cb if error */
> +return TRUE;
> +}
> +
> +void dispatcher_attach(Dispatcher *dispatcher, GMainContext *main_context)
> +{
> +spice_return_if_fail(dispatcher != NULL);
> +spice_return_if_fail(main_context != NULL);
> +
> +GIOChannel *channel = g_io_channel_unix_new(dispatcher->recv_fd);
> +GSource *source = g_io_create_watch(channel, G_IO_IN);
> +
> +g_source_set_callback(source, (GSourceFunc)dispatch_cb, dispatcher,
> NULL);
> +g_source_attach(source, main_context);
> +g_source_unref(source);
> +}
> diff --git a/server/dispatcher.h b/server/dispatcher.h
> index 78ef663..f171332 100644
> --- a/server/dispatcher.h
> +++ b/server/dispatcher.h
> @@ -72,6 +72,8 @@ void dispatcher_send_message(Dispatcher *dispatcher,
> uint32_t message_type,
> void dispatcher_init(Dispatcher *dispatcher, size_t max_message_type,
> void *opaque);
>
> +void dispatcher_attach(Dispatcher *dispatcher, GMainContext *main_context);
> +
> enum {
> DISPATCHER_NONE = 0,
> DISPATCHER_ACK,
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 6fc4382..71d18b7 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -1449,11 +1449,12 @@ static void worker_dispatcher_record(void *opaque,
> uint32_t message_type, void *
> red_record_event(worker->record_fd, 1, message_type, stat_now(worker
> ->clockid));
> }
>
> -static void register_callbacks(Dispatcher *dispatcher)
> +static void worker_dispatcher_register(RedWorker *worker, Dispatcher
> *dispatcher)
> {
> -dispatcher_register_async_done_callback(
> -dispatcher,
> -worker_handle_dispatcher_async_done);
> +dispatcher_set_opaque(dispatcher, worker);
> +
> +dispatcher_register_async_done_callback(dispatcher,
> +
> worker_handle_dispatcher_async_done);
>
> /* TODO: register cursor & display specific msg in respective channel
> files */
> dispatcher_register_handler(dispatcher,
> @@ -1621,21 +1622,11 @@ static void register_callbacks(Dispatcher *dispatcher)
> handle_dev_driver_unload,
> sizeof(RedWorkerMessageDriverUnload),
> DISPATCHER_NONE);
> -}
>
> -
> -
> -static gboolean worker_dispatcher_cb(GIOChannel *source, GIOCondition
> condition,
> - gpointer data)
> -{
> -RedWorker *worker = data;
> -
> -spice_debug(NULL);
> -dispatcher_handle_recv_read(red_dispatcher_get_dispatcher(worker
> ->red_dispatcher));
> -
> -return TRUE;
> +dispatcher_attach(dispatcher, worker->main_context);
> }
>
> +
> typedef struct RedWorkerSource {
> GSource source;
> RedWorker *worker;
> @@ -1722,14 +1713,13 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> RedDispatcher *red_dispatcher)
> }
> }
> dispatcher = red_dispatcher_get_dispatcher(red_dispatcher);
> -dispatcher_set_opaque(dispatcher, worker);
> -
> -worker->red_dispatcher = red_dispatcher;
> -worker->qxl = qxl;
> -register_callbacks(dispatcher);
> +worker_dispatcher_register(worker, dispatcher);
> if (worker->record_fd) {
> dispatcher_register_universal_handler(dispatcher,
> worker_dispatcher_record);
> }
> +
> +worker->red_dispatcher = red_dispatcher;
> +worker->qxl = qxl;
> worker->image_compression = image_compression;
> worker->jpeg_state = jpeg_state;
> worker->zlib_glz_state = zlib_glz_state;
> @@ -1742,13 +1732,7 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> RedDispatcher *red_dispatcher)
> worker->command_counter = stat_add_counter(worker->stat, "commands",
> TRUE);
> #endif
>
> -GIOChannel *channel =
> g_io_channel_unix_new(dispatcher_get_recv_fd(dispatcher));
> -GSource *source = g_io_create_watch(channel, G_IO_IN);
> -g_source_set_callback(source, (GSourceFunc)worker_dispatcher_cb, worker,
> NULL);
> -g_source_attach(source, worker->main_context);
> -g_source_unref(source);
> -
> -source = g_source
Re: [Spice-devel] [PATCH 13/15] worker: make sure we dispatch after releasing items
On Thu, 2015-12-03 at 16:27 +, Frediano Ziglio wrote:
> From: Marc-André Lureau
>
> ---
> server/display-channel.c | 2 ++
> server/red-worker.c | 10 ++
> server/red-worker.h | 1 +
> 3 files changed, 13 insertions(+)
>
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 02ccc15..838b8ff 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1987,9 +1987,11 @@ static void hold_item(RedChannelClient *rcc, PipeItem
> *item)
> static void release_item(RedChannelClient *rcc, PipeItem *item, int
> item_pushed)
> {
> DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
> +RedWorker *worker = DCC_TO_WORKER(dcc);
>
> spice_return_if_fail(item != NULL);
> dcc_release_item(dcc, item, item_pushed);
> +red_worker_update_timeout(worker, 0);
> }
>
> static int handle_migrate_flush_mark(RedChannelClient *rcc)
> diff --git a/server/red-worker.c b/server/red-worker.c
> index ba3454e..6fc4382 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -110,6 +110,14 @@ RedMemSlotInfo* red_worker_get_memslot(RedWorker *worker)
> return &worker->mem_slots;
> }
>
> +void red_worker_update_timeout(RedWorker *worker, gint timeout)
> +{
> +spice_return_if_fail(worker != NULL);
> +spice_return_if_fail(timeout >= 0);
> +
> +worker->timeout = MIN(worker->timeout, timeout);
> +}
> +
> static int display_is_connected(RedWorker *worker)
> {
> return (worker->display_channel && red_channel_is_connected(
> @@ -1641,6 +1649,8 @@ static gboolean worker_source_prepare(GSource *source,
> gint *timeout)
> *timeout = worker->timeout;
> *timeout = MIN(worker->timeout,
> display_channel_get_streams_timeout(worker
> ->display_channel));
> +if (*timeout == 0)
> +return TRUE;
I think that this particular hunk could probably be squashed into the original
glib loop commit.
>
> return FALSE; /* do no timeout poll */
> }
> diff --git a/server/red-worker.h b/server/red-worker.h
> index b55a45c..3a9dc19 100644
> --- a/server/red-worker.h
> +++ b/server/red-worker.h
> @@ -94,6 +94,7 @@ static inline void red_pipes_add_verb(RedChannel *channel,
> uint16_t verb)
>
> RedWorker* red_worker_new(QXLInstance *qxl, RedDispatcher *red_dispatcher);
> bool red_worker_run(RedWorker *worker);
> +void red_worker_update_timeout(RedWorker *worker, gint timeout);
> QXLInstance* red_worker_get_qxl(RedWorker *worker);
> RedChannel* red_worker_get_cursor_channel(RedWorker *worker);
> RedChannel* red_worker_get_display_channel(RedWorker *worker);
I don't really understand the reason for this patch. Why do we need to wakeup
the worker immediately after a display pipe item has been released, but not a
cursor item? Why can't we wait for the next worker poll timeout to expire?
Updating the worker timeout seems like an implementation detail, so adding an
API for other classes to call seems a little weird to me.
Reviewed-by: Jonathon Jongsma
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 12/15] channel: do not free rcc->stream in red_channel_client_disconnect
On Thu, 2015-12-03 at 16:27 +, Frediano Ziglio wrote:
> From: Marc-André Lureau
>
> This fixes the following scary racy corruption after glib loop switch:
>
Note that this valgrind trace is not valid for the current tree. Probably
commits got re-ordered and code got shifted around since this patch was made.
It's still probably useful to have the trace in the log, but we should at least
note that it's out-of-date. Perhaps we should also test whether this is still
reproducible with the current code.
> ==28173==
> ==28173== Debugger has detached. Valgrind regains control. We
> continue.
> ==28173== Invalid read of size 8
> ==28173==at 0x4C7871E: reds_stream_read (reds.c:4521)
> ==28173==by 0x4C2F9D7: red_peer_receive (red_channel.c:209)
> ==28173==by 0x4C2FB59: red_peer_handle_incoming (red_channel.c:255)
> ==28173==by 0x4C2FF36:
> red_channel_client_receive (red_channel.c:329)
> ==28173==by 0x4C33D6D: red_channel_client_event (red_channel.c:1577)
> ==28173==by 0x4C65098: watch_func (red_worker.c:10292)
> ==28173==by 0x504DDAB: g_io_unix_dispatch (giounix.c:167)
> ==28173==by 0x4FFACB6: g_main_dispatch (gmain.c:3065)
> ==28173==by 0x4FFBA0D: g_main_context_dispatch (gmain.c:3641)
> ==28173==by 0x4FFBBFF: g_main_context_iterate (gmain.c:3712)
> ==28173==by 0x4FFC028: g_main_loop_run (gmain.c:3906)
> ==28173==by 0x4C6ABF2: red_worker_main (red_worker.c:12180)
> ==28173== Address 0x7d688e0 is 32 bytes inside a block of size 160
> free'd
> ==28173==at 0x4A074C4: free (in
> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==28173==by 0x4C78A84: reds_stream_free (reds.c:4594)
> ==28173==by 0x4C34E2E:
> red_channel_client_disconnect (red_channel.c:1865)
> ==28173==by 0x4C30363:
> red_channel_client_default_peer_on_error (red_channel.c:417)
> ==28173==by 0x4C3011B: red_peer_handle_outgoing (red_channel.c:372)
> ==28173==by 0x4C3305F: red_channel_client_send (red_channel.c:1298)
> ==28173==by 0x4C33FB6:
> red_channel_client_begin_send_message (red_channel.c:1616)
> ==28173==by 0x4C5F4B4:
> display_begin_send_message (red_worker.c:8360)
> ==28173==by 0x4C61DE8: display_channel_send_item (red_worker.c:9164)
> ==28173==by 0x4C30C69:
> red_channel_client_send_item (red_channel.c:599)
> ==28173==by 0x4C332BB: red_channel_client_push (red_channel.c:1351)
> ==28173==by 0x4C33C3A:
> red_channel_client_handle_message (red_channel.c:1545)
> ---
> server/red-channel.c | 39 +--
> server/red-worker.c | 2 ++
> 2 files changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 2a64bc8..ae16b9c 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -1238,22 +1238,25 @@ static void red_channel_client_ref(RedChannelClient
> *rcc)
>
> static void red_channel_client_unref(RedChannelClient *rcc)
> {
> -if (!--rcc->refs) {
> -spice_debug("destroy rcc=%p", rcc);
> -if (rcc->send_data.main.marshaller) {
> -spice_marshaller_destroy(rcc->send_data.main.marshaller);
> -}
> +spice_debug("rcc=%p", rcc);
>
> -if (rcc->send_data.urgent.marshaller) {
> -spice_marshaller_destroy(rcc->send_data.urgent.marshaller);
> -}
> +if (--rcc->refs != 0)
> +return;
>
> -red_channel_client_destroy_remote_caps(rcc);
> -if (rcc->channel) {
> -red_channel_unref(rcc->channel);
> -}
> -free(rcc);
> -}
> +reds_stream_free(rcc->stream);
> +rcc->stream = NULL;
> +
> +if (rcc->send_data.main.marshaller)
> +spice_marshaller_destroy(rcc->send_data.main.marshaller);
> +
> +if (rcc->send_data.urgent.marshaller)
> +spice_marshaller_destroy(rcc->send_data.urgent.marshaller);
> +
> +red_channel_client_destroy_remote_caps(rcc);
> +if (rcc->channel)
> +red_channel_unref(rcc->channel);
> +
> +free(rcc);
> }
>
> void red_channel_client_destroy(RedChannelClient *rcc)
> @@ -1758,7 +1761,7 @@ void red_channel_pipes_add_empty_msg(RedChannel
> *channel, int msg_type)
> int red_channel_client_is_connected(RedChannelClient *rcc)
> {
> if (!rcc->dummy) {
> -return rcc->stream != NULL;
> +return (rcc->stream != NULL) && ring_item_is_linked(&rcc
> ->channel_link);
This change seems harmless, but it doesn't seem strictly related to the bug
being fixed.
> } else {
> return rcc->dummy_connected;
> }
> @@ -1813,8 +1816,10 @@ static void red_channel_remove_client(RedChannelClient
> *rcc)
>rcc->channel->type, rcc->channel->id,
>rcc->channel->thread_id, pthread_self());
> }
> +spice_return_if_fail(ring_item_is_linked(&rcc->channel_link));
> +
> ring_remove(&rcc->channel_link);
> -spice_assert(rcc->channel->clients_num > 0);
> +spice_return_if_fail(rcc->channel->clients_num
Re: [Spice-devel] [PATCH 07/15] server: merge spice-bitmap-utils and spice_bitmap_utils
On Fri, 2015-12-04 at 06:32 -0500, Frediano Ziglio wrote:
> >
> > This commit introduces a compile failure:
> >
> > included from ../../server/spice-bitmap-utils.c:21:0:
> > ../../server/spice-bitmap-utils.h:38:17: error: expected declaration
> > specifiers
> > or '...' before 'sizeof'
> > G_STATIC_ASSERT(sizeof(rgb32_pixel_t) == 4);
> >
>
> That's really weird. It works for me.
> Can you try with and without this patch?
see comment below
>
> Frediano
>
>
> >
> > On Thu, 2015-12-03 at 16:26 +, Frediano Ziglio wrote:
> > > Signed-off-by: Marc-André Lureau
> > > Signed-off-by: Frediano Ziglio
> > > ---
> > > server/Makefile.am | 2 -
> > > server/display-channel.h| 2 +-
> > > server/spice-bitmap-utils.c | 162 ++
> > > server/spice-bitmap-utils.h | 15 ++--
> > > server/spice_bitmap_utils.c | 188
> > > ---
> > > -
> > > server/spice_bitmap_utils.h | 8 --
> > > 6 files changed, 171 insertions(+), 206 deletions(-)
> > > delete mode 100644 server/spice_bitmap_utils.c
> > > delete mode 100644 server/spice_bitmap_utils.h
> > >
> > > diff --git a/server/Makefile.am b/server/Makefile.am
> > > index 1e7e8fa..2bb4fec 100644
> > > --- a/server/Makefile.am
> > > +++ b/server/Makefile.am
> > > @@ -124,8 +124,6 @@ libspice_server_la_SOURCES = \
> > > spice_timer_queue.h \
> > > zlib-encoder.c \
> > > zlib-encoder.h \
> > > - spice_bitmap_utils.h\
> > > - spice_bitmap_utils.c\
> > > image-cache.h \
> > > image-cache.c \
> > > pixmap-cache.h \
> > > diff --git a/server/display-channel.h b/server/display-channel.h
> > > index 7cbc58d..8adff0a 100644
> > > --- a/server/display-channel.h
> > > +++ b/server/display-channel.h
> > > @@ -39,7 +39,7 @@
> > > #include "main-channel.h"
> > > #include "migration-protocol.h"
> > > #include "main-dispatcher.h"
> > > -#include "spice_bitmap_utils.h"
> > > +#include "spice-bitmap-utils.h"
> > > #include "image-cache.h"
> > > #include "utils.h"
> > > #include "tree.h"
> > > diff --git a/server/spice-bitmap-utils.c b/server/spice-bitmap-utils.c
> > > index 03d7694..8d6e7c6 100644
> > > --- a/server/spice-bitmap-utils.c
> > > +++ b/server/spice-bitmap-utils.c
> > > @@ -117,3 +117,165 @@ int bitmap_has_extra_stride(SpiceBitmap *bitmap)
> > > }
> > > return 0;
> > > }
> > > +
> > > +int spice_bitmap_from_surface_type(uint32_t surface_format)
> > > +{
> > > +switch (surface_format) {
> > > +case SPICE_SURFACE_FMT_16_555:
> > > +return SPICE_BITMAP_FMT_16BIT;
> > > +case SPICE_SURFACE_FMT_32_xRGB:
> > > +return SPICE_BITMAP_FMT_32BIT;
> > > +case SPICE_SURFACE_FMT_32_ARGB:
> > > +return SPICE_BITMAP_FMT_RGBA;
> > > +case SPICE_SURFACE_FMT_8_A:
> > > +return SPICE_BITMAP_FMT_8BIT_A;
> > > +default:
> > > +spice_critical("Unsupported surface format");
> > > +}
> > > +return 0;
> > > +}
> > > +
> > > +#define RAM_PATH "/tmp/tmpfs"
> > > +
> > > +static void dump_palette(FILE *f, SpicePalette* plt)
> > > +{
> > > +int i;
> > > +for (i = 0; i < plt->num_ents; i++) {
> > > +fwrite(plt->ents + i, sizeof(uint32_t), 1, f);
> > > +}
> > > +}
> > > +
> > > +static void dump_line(FILE *f, uint8_t* line, uint16_t n_pixel_bits, int
> > > width, int row_size)
> > > +{
> > > +int i;
> > > +int copy_bytes_size = SPICE_ALIGN(n_pixel_bits * width, 8) / 8;
> > > +
> > > +fwrite(line, 1, copy_bytes_size, f);
> > > +if (row_size > copy_bytes_size) {
> > > +// each line should be 4 bytes aligned
> > > +for (i = copy_bytes_size; i < row_size; i++) {
> > > +fprintf(f, "%c", 0);
> > > +}
> > > +}
> > > +}
> > > +void dump_bitmap(SpiceBitmap *bitmap)
> > > +{
> > > +static uint32_t file_id = 0;
> > > +
> > > +char file_str[200];
> > > +int rgb = TRUE;
> > > +uint16_t n_pixel_bits;
> > > +SpicePalette *plt = NULL;
> > > +uint32_t id;
> > > +int row_size;
> > > +uint32_t file_size;
> > > +int alpha = 0;
> > > +uint32_t header_size = 14 + 40;
> > > +uint32_t bitmap_data_offset;
> > > +uint32_t tmp_u32;
> > > +int32_t tmp_32;
> > > +uint16_t tmp_u16;
> > > +FILE *f;
> > > +int i, j;
> > > +
> > > +switch (bitmap->format) {
> > > +case SPICE_BITMAP_FMT_1BIT_BE:
> > > +case SPICE_BITMAP_FMT_1BIT_LE:
> > > +rgb = FALSE;
> > > +n_pixel_bits = 1;
> > > +break;
> > > +case SPICE_BITMAP_FMT_4BIT_BE:
> > > +case SPICE_BITMAP_FMT_4BIT_LE:
> > > +rgb = FALSE;
> > > +n_pixel_bits = 4;
> > > +break;
> > > +case SPICE_BITMAP_FMT_8BIT:
> > > +rgb = FALSE;
> > > +n_pixel_bits = 8;
> > > +break;
> > > +case SPICE_BITMA
Re: [Spice-devel] [PATCH 10/15] worker: use glib main loop
>
>
> - Original Message -
> > On Thu, 2015-12-03 at 16:27 +, Frediano Ziglio wrote:
> > > From: Marc-André Lureau
> > >
> > > Clean up, more extensible.
> >
> > I think this should be a bit less terse... perhaps:
> > Use the glib mainloop instead of writing our own. The glib loop is both
> > cleaner
> > to use and is more extensible. It is also very mature and reduces the
> > maintenance burden on the spice server.
> >
> > >
> > > Avoid server hanging when no client are connected.
> >
> >
> > This sounds like something that might belong in a separate patch?
> >
Think so.
> >
> > > ---
> > > server/Makefile.am | 2 -
> > > server/red-worker.c| 395
> > >
> > > -
> > > server/red-worker.h| 1 +
> > > server/spice_timer_queue.c | 273 ---
> > > server/spice_timer_queue.h | 43 -
> > > 5 files changed, 246 insertions(+), 468 deletions(-)
> > > delete mode 100644 server/spice_timer_queue.c
> > > delete mode 100644 server/spice_timer_queue.h
> > >
> > > diff --git a/server/Makefile.am b/server/Makefile.am
> > > index d4fc972..88825d8 100644
> > > --- a/server/Makefile.am
> > > +++ b/server/Makefile.am
> > > @@ -121,8 +121,6 @@ libspice_server_la_SOURCES = \
> > > spice.h \
> > > stat.h \
> > > spicevmc.c \
> > > - spice_timer_queue.c \
> > > - spice_timer_queue.h \
> > > zlib-encoder.c \
> > > zlib-encoder.h \
> > > image-cache.h \
> > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > index aecfcf9..9e8fcbb 100644
> > > --- a/server/red-worker.c
> > > +++ b/server/red-worker.c
> > > @@ -49,31 +49,21 @@
> > >
> > > #include "spice.h"
> > > #include "red-worker.h"
> > > -#include "spice_timer_queue.h"
> > > #include "cursor-channel.h"
> > > #include "tree.h"
> > >
> > > #define CMD_RING_POLL_TIMEOUT 10 //milli
> > > #define CMD_RING_POLL_RETRIES 200
> > >
> > > -#define MAX_EVENT_SOURCES 20
> > > -#define INF_EVENT_WAIT ~0
> > > -
> > > -struct SpiceWatch {
> > > -struct RedWorker *worker;
> > > -SpiceWatchFunc watch_func;
> > > -void *watch_func_opaque;
> > > -};
> > > -
> > > struct RedWorker {
> > > pthread_t thread;
> > > clockid_t clockid;
> > > +GMainContext *main_context;
> > > QXLInstance *qxl;
> > > RedDispatcher *red_dispatcher;
> > > int running;
> > > -struct pollfd poll_fds[MAX_EVENT_SOURCES];
> > > -struct SpiceWatch watches[MAX_EVENT_SOURCES];
> > > -unsigned int event_timeout;
> > > +
> > > +gint timeout;
I would revert to event_timeout. What's wrong with the old name?
> > >
> > > DisplayChannel *display_channel;
> > > uint32_t display_poll_tries;
> > > @@ -99,6 +89,13 @@ struct RedWorker {
> > > FILE *record_fd;
> > > };
> > >
> > > +GMainContext* red_worker_get_context(RedWorker *worker)
> >
> >
> > I'd prefer _get_main_context()
>
> fwiw, glib api uses _get_context() too (task_get_context(),
> source_get_context() etc)
>
It is the graphics context of the drawing?
Really, the name is quite misleading.
What about worker_get_event_context ?
> >
> >
> > > +{
> > > +spice_return_val_if_fail(worker, NULL);
> > > +
> > > +return worker->main_context;
> > > +}
> > > +
> > > QXLInstance* red_worker_get_qxl(RedWorker *worker)
> > > {
> > > spice_return_val_if_fail(worker != NULL, NULL);
> > > @@ -182,7 +179,9 @@ static int red_process_cursor(RedWorker *worker,
> > > uint32_t
> > > max_pipe_size, int *ri
> > > *ring_is_empty = TRUE;
> > > if (worker->cursor_poll_tries < CMD_RING_POLL_RETRIES) {
> > > worker->cursor_poll_tries++;
> > > -worker->event_timeout = MIN(worker->event_timeout,
> > > CMD_RING_POLL_TIMEOUT);
> > > +worker->timeout = worker->timeout == -1 ?
> > > +CMD_RING_POLL_TIMEOUT :
> > > +MIN(worker->timeout, CMD_RING_POLL_TIMEOUT);
> > > return n;
> > > }
> > > if (worker->cursor_poll_tries > CMD_RING_POLL_RETRIES ||
> > > @@ -228,7 +227,6 @@ static int red_process_display(RedWorker *worker,
> > > uint32_t
> > > max_pipe_size, int *r
> > > {
> > > QXLCommandExt ext_cmd;
> > > int n = 0;
> > > -uint64_t start = red_get_monotonic_time();
> > >
> > > if (!worker->running) {
> > > *ring_is_empty = TRUE;
> > > @@ -237,14 +235,30 @@ static int red_process_display(RedWorker *worker,
> > > uint32_t max_pipe_size, int *r
> > >
> > > worker->process_display_generation++;
> > > *ring_is_empty = FALSE;
> > > -while (!display_is_connected(worker) ||
> > > - // TODO: change to average pipe size?
> > > -
> > >
Re: [Spice-devel] [PATCH] server: multiple clients works ok if we limit the pipe to the slowest client
On Fri, Dec 4, 2015 at 4:27 PM, Frediano Ziglio wrote:
> From: Alon Levy
>
> Signed-off-by: Alon Levy
> Signed-off-by: Frediano Ziglio
> ---
> server/red-worker.c | 7 +++
> server/reds.c | 2 +-
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 1b2bb77..baa8458 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -177,7 +177,7 @@ static int red_process_cursor(RedWorker *worker, uint32_t
> max_pipe_size, int *ri
>
> *ring_is_empty = FALSE;
> while (!cursor_is_connected(worker) ||
> - red_channel_min_pipe_size(RED_CHANNEL(worker->cursor_channel)) <=
> max_pipe_size) {
> + red_channel_max_pipe_size(RED_CHANNEL(worker->cursor_channel)) <=
> max_pipe_size) {
> if (!worker->qxl->st->qif->get_cursor_command(worker->qxl,
> &ext_cmd)) {
> *ring_is_empty = TRUE;
> if (worker->cursor_poll_tries < CMD_RING_POLL_RETRIES) {
> @@ -238,8 +238,7 @@ static int red_process_display(RedWorker *worker,
> uint32_t max_pipe_size, int *r
> worker->process_display_generation++;
> *ring_is_empty = FALSE;
> while (!display_is_connected(worker) ||
> - // TODO: change to average pipe size?
> - red_channel_min_pipe_size(RED_CHANNEL(worker->display_channel))
> <= max_pipe_size) {
> + red_channel_max_pipe_size(RED_CHANNEL(worker->display_channel))
> <= max_pipe_size) {
> if (!worker->qxl->st->qif->get_command(worker->qxl, &ext_cmd)) {
> *ring_is_empty = TRUE;;
> if (worker->display_poll_tries < CMD_RING_POLL_RETRIES) {
> @@ -447,7 +446,7 @@ static void flush_cursor_commands(RedWorker *worker)
> for (;;) {
> red_channel_push(RED_CHANNEL(worker->cursor_channel));
> if (!cursor_is_connected(worker)
> -|| red_channel_min_pipe_size(cursor_red_channel) <=
> MAX_PIPE_SIZE) {
> +|| red_channel_max_pipe_size(cursor_red_channel) <=
> MAX_PIPE_SIZE) {
> break;
> }
> RedChannel *channel = (RedChannel *)worker->cursor_channel;
> diff --git a/server/reds.c b/server/reds.c
> index f3d1b24..d890616 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3347,7 +3347,7 @@ static int do_spice_init(SpiceCoreInterface
> *core_interface)
>
> reds->allow_multiple_clients = getenv(SPICE_DEBUG_ALLOW_MC_ENV) != NULL;
> if (reds->allow_multiple_clients) {
> -spice_warning("spice: allowing multiple client connections
> (crashy)");
> +spice_warning("spice: allowing multiple client connections");
> }
> atexit(reds_exit);
> return 0;
> --
> 2.4.3
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Yeah. Jezz, I've acked the wrong patch. This is the one I've tested.
Go for it, please.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH] server: multiple clients works ok if we limit the pipe to the slowest client
From: Alon Levy
Signed-off-by: Alon Levy
Signed-off-by: Frediano Ziglio
---
server/red-worker.c | 7 +++
server/reds.c | 2 +-
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/server/red-worker.c b/server/red-worker.c
index 1b2bb77..baa8458 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -177,7 +177,7 @@ static int red_process_cursor(RedWorker *worker, uint32_t
max_pipe_size, int *ri
*ring_is_empty = FALSE;
while (!cursor_is_connected(worker) ||
- red_channel_min_pipe_size(RED_CHANNEL(worker->cursor_channel)) <=
max_pipe_size) {
+ red_channel_max_pipe_size(RED_CHANNEL(worker->cursor_channel)) <=
max_pipe_size) {
if (!worker->qxl->st->qif->get_cursor_command(worker->qxl, &ext_cmd)) {
*ring_is_empty = TRUE;
if (worker->cursor_poll_tries < CMD_RING_POLL_RETRIES) {
@@ -238,8 +238,7 @@ static int red_process_display(RedWorker *worker, uint32_t
max_pipe_size, int *r
worker->process_display_generation++;
*ring_is_empty = FALSE;
while (!display_is_connected(worker) ||
- // TODO: change to average pipe size?
- red_channel_min_pipe_size(RED_CHANNEL(worker->display_channel)) <=
max_pipe_size) {
+ red_channel_max_pipe_size(RED_CHANNEL(worker->display_channel)) <=
max_pipe_size) {
if (!worker->qxl->st->qif->get_command(worker->qxl, &ext_cmd)) {
*ring_is_empty = TRUE;;
if (worker->display_poll_tries < CMD_RING_POLL_RETRIES) {
@@ -447,7 +446,7 @@ static void flush_cursor_commands(RedWorker *worker)
for (;;) {
red_channel_push(RED_CHANNEL(worker->cursor_channel));
if (!cursor_is_connected(worker)
-|| red_channel_min_pipe_size(cursor_red_channel) <=
MAX_PIPE_SIZE) {
+|| red_channel_max_pipe_size(cursor_red_channel) <=
MAX_PIPE_SIZE) {
break;
}
RedChannel *channel = (RedChannel *)worker->cursor_channel;
diff --git a/server/reds.c b/server/reds.c
index f3d1b24..d890616 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3347,7 +3347,7 @@ static int do_spice_init(SpiceCoreInterface
*core_interface)
reds->allow_multiple_clients = getenv(SPICE_DEBUG_ALLOW_MC_ENV) != NULL;
if (reds->allow_multiple_clients) {
-spice_warning("spice: allowing multiple client connections (crashy)");
+spice_warning("spice: allowing multiple client connections");
}
atexit(reds_exit);
return 0;
--
2.4.3
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 11/15] server: multiple clients works ok if we limit the pipe to the slowest client
On Thu, Dec 3, 2015 at 5:27 PM, Frediano Ziglio wrote:
> From: Alon Levy
>
> ---
> server/red-worker.c | 8
> server/reds.c | 2 +-
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 9e8fcbb..9ec90ce 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -174,7 +174,7 @@ static int red_process_cursor(RedWorker *worker, uint32_t
> max_pipe_size, int *ri
>
> *ring_is_empty = FALSE;
> while (!cursor_is_connected(worker) ||
> - red_channel_min_pipe_size(RED_CHANNEL(worker->cursor_channel)) <=
> max_pipe_size) {
> + red_channel_max_pipe_size(RED_CHANNEL(worker->cursor_channel)) <=
> max_pipe_size) {
> if (!worker->qxl->st->qif->get_cursor_command(worker->qxl,
> &ext_cmd)) {
> *ring_is_empty = TRUE;
> if (worker->cursor_poll_tries < CMD_RING_POLL_RETRIES) {
> @@ -244,10 +244,10 @@ static int red_process_display(RedWorker *worker,
> uint32_t max_pipe_size, int *r
> return n;
> }
>
> -
> -// TODO: change to average pipe size?
> +/* this is safe but slow. in the future client groups will rule
> the world, and
> + * dial up will live with T1 pipes in harmony */
> if
> (red_channel_min_pipe_size(RED_CHANNEL(worker->display_channel)) >
> max_pipe_size) {
> -spice_info("too much item in the display clients pipe
> already");
> +spice_info("too many items in the display clients pipe
> already");
> return n;
> }
> }
> diff --git a/server/reds.c b/server/reds.c
> index f3d1b24..d890616 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3347,7 +3347,7 @@ static int do_spice_init(SpiceCoreInterface
> *core_interface)
>
> reds->allow_multiple_clients = getenv(SPICE_DEBUG_ALLOW_MC_ENV) != NULL;
> if (reds->allow_multiple_clients) {
> -spice_warning("spice: allowing multiple client connections
> (crashy)");
> +spice_warning("spice: allowing multiple client connections");
> }
> atexit(reds_exit);
> return 0;
> --
> 2.4.3
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
I've tested Frediano change and the multiple client still works as expected.
Acked-by: Fabiano Fidêncio
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] pass proper type to SPICE_CONTAINEROF
On Fri, Dec 4, 2015 at 12:38 PM, Frediano Ziglio wrote:
> In some case the member specified to SPICE_CONTAINEROF was not
> exactly the same type of the pointer passed.
> This can cause issues if structure changes so use proper member.
>
> Signed-off-by: Frediano Ziglio
> ---
> server/display-channel.c | 2 +-
> server/sound.c | 2 +-
> server/tree.c| 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 2ef1c88..f0ff2bd 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -429,7 +429,7 @@ static void current_remove(DisplayChannel *display,
> TreeItem *item)
> RingItem *ring_item;
>
> if (now->type == TREE_ITEM_TYPE_DRAWABLE) {
> -Drawable *drawable = SPICE_CONTAINEROF(now, Drawable, tree_item);
> +Drawable *drawable = SPICE_CONTAINEROF(now, Drawable,
> tree_item.base);
> ring_item = now->siblings_link.prev;
> drawable_remove_from_pipes(drawable);
> current_remove_drawable(display, drawable);
> diff --git a/server/sound.c b/server/sound.c
> index 2d3f393..0c94964 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -1102,7 +1102,7 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_put_samples(SpicePlaybackInstance
> PlaybackChannel *playback_channel;
> AudioFrame *frame;
>
> -frame = SPICE_CONTAINEROF(samples, AudioFrame, samples);
> +frame = SPICE_CONTAINEROF(samples, AudioFrame, samples[0]);
> playback_channel = frame->channel;
> spice_assert(playback_channel);
> if (!snd_channel_put(&playback_channel->base) ||
> diff --git a/server/tree.c b/server/tree.c
> index e5e3c76..9e5a281 100644
> --- a/server/tree.c
> +++ b/server/tree.c
> @@ -129,7 +129,7 @@ static void dump_item(TreeItem *item, void *data)
>
> switch (item->type) {
> case TREE_ITEM_TYPE_DRAWABLE: {
> -Drawable *drawable = SPICE_CONTAINEROF(item, Drawable, tree_item);
> +Drawable *drawable = SPICE_CONTAINEROF(item, Drawable,
> tree_item.base);
> const int max_indent = 200;
> char indent_str[max_indent + 1];
> int indent_str_len;
> --
> 2.4.3
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Acked-by: Fabiano Fidêncio
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] worker: improve validation for update command
On Fri, Dec 4, 2015 at 12:42 PM, Frediano Ziglio wrote:
> If surface_id is not valid we should still release resource allocated
> by red_get_update_cmd and from the guest.
> This to reduce leaks in case of a race or another error in the guest
> driver.
> Also not issue a warning on invalid surface number to avoid filling
> log space unconditionally.
>
> Signed-off-by: Frediano Ziglio
> ---
> server/red-worker.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 626b481..6ec9106 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -284,10 +284,10 @@ static int red_process_display(RedWorker *worker,
> uint32_t max_pipe_size, int *r
> }
> if (!validate_surface(worker->display_channel,
> update.surface_id)) {
> spice_warning("Invalid surface in QXL_CMD_UPDATE");
> -break;
> +} else {
> +display_channel_draw(worker->display_channel, &update.area,
> update.surface_id);
> +worker->qxl->st->qif->notify_update(worker->qxl,
> update.update_id);
> }
> -display_channel_draw(worker->display_channel, &update.area,
> update.surface_id);
> -worker->qxl->st->qif->notify_update(worker->qxl,
> update.update_id);
> release_info_ext.group_id = ext_cmd.group_id;
> release_info_ext.info = update.release_info;
> worker->qxl->st->qif->release_resource(worker->qxl,
> release_info_ext);
> --
> 2.4.3
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Acked-by: Fabiano Fidêncio
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 09/15] tests: test_display_base: use a faster wakeup time to easily test multiple client blocking
>
> On Thu, 2015-12-03 at 16:27 +, Frediano Ziglio wrote:
> > From: Alon Levy
> >
> > ---
> > server/tests/test_display_base.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/server/tests/test_display_base.c
> > b/server/tests/test_display_base.c
> > index 8c8cef1..b574756 100644
> > --- a/server/tests/test_display_base.c
> > +++ b/server/tests/test_display_base.c
> > @@ -864,7 +864,7 @@ Test *test_new(SpiceCoreInterface *core)
> >
> > test->core = core;
> > test->server = server;
> > -test->wakeup_ms = 50;
> > +test->wakeup_ms = 1;
> > test->cursor_notify = NOTIFY_CURSOR_BATCH;
> > // some common initialization for all display tests
> > printf("TESTER: listening on port %d (unsecure)\n", port);
>
>
> I don't have much opinion on this patch, since it's a test program that I've
> never really used before. It doesn't look like it will cause any big issues.
> ACK, I guess.
>
> Acked-by: Jonathon Jongsma
>
Merged
I think this change was used to increase cursors command sent by tests.
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH] worker: improve validation for update command
If surface_id is not valid we should still release resource allocated
by red_get_update_cmd and from the guest.
This to reduce leaks in case of a race or another error in the guest
driver.
Also not issue a warning on invalid surface number to avoid filling
log space unconditionally.
Signed-off-by: Frediano Ziglio
---
server/red-worker.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/server/red-worker.c b/server/red-worker.c
index 626b481..6ec9106 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -284,10 +284,10 @@ static int red_process_display(RedWorker *worker,
uint32_t max_pipe_size, int *r
}
if (!validate_surface(worker->display_channel, update.surface_id))
{
spice_warning("Invalid surface in QXL_CMD_UPDATE");
-break;
+} else {
+display_channel_draw(worker->display_channel, &update.area,
update.surface_id);
+worker->qxl->st->qif->notify_update(worker->qxl,
update.update_id);
}
-display_channel_draw(worker->display_channel, &update.area,
update.surface_id);
-worker->qxl->st->qif->notify_update(worker->qxl, update.update_id);
release_info_ext.group_id = ext_cmd.group_id;
release_info_ext.info = update.release_info;
worker->qxl->st->qif->release_resource(worker->qxl,
release_info_ext);
--
2.4.3
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH] pass proper type to SPICE_CONTAINEROF
In some case the member specified to SPICE_CONTAINEROF was not
exactly the same type of the pointer passed.
This can cause issues if structure changes so use proper member.
Signed-off-by: Frediano Ziglio
---
server/display-channel.c | 2 +-
server/sound.c | 2 +-
server/tree.c| 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/server/display-channel.c b/server/display-channel.c
index 2ef1c88..f0ff2bd 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -429,7 +429,7 @@ static void current_remove(DisplayChannel *display,
TreeItem *item)
RingItem *ring_item;
if (now->type == TREE_ITEM_TYPE_DRAWABLE) {
-Drawable *drawable = SPICE_CONTAINEROF(now, Drawable, tree_item);
+Drawable *drawable = SPICE_CONTAINEROF(now, Drawable,
tree_item.base);
ring_item = now->siblings_link.prev;
drawable_remove_from_pipes(drawable);
current_remove_drawable(display, drawable);
diff --git a/server/sound.c b/server/sound.c
index 2d3f393..0c94964 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1102,7 +1102,7 @@ SPICE_GNUC_VISIBLE void
spice_server_playback_put_samples(SpicePlaybackInstance
PlaybackChannel *playback_channel;
AudioFrame *frame;
-frame = SPICE_CONTAINEROF(samples, AudioFrame, samples);
+frame = SPICE_CONTAINEROF(samples, AudioFrame, samples[0]);
playback_channel = frame->channel;
spice_assert(playback_channel);
if (!snd_channel_put(&playback_channel->base) ||
diff --git a/server/tree.c b/server/tree.c
index e5e3c76..9e5a281 100644
--- a/server/tree.c
+++ b/server/tree.c
@@ -129,7 +129,7 @@ static void dump_item(TreeItem *item, void *data)
switch (item->type) {
case TREE_ITEM_TYPE_DRAWABLE: {
-Drawable *drawable = SPICE_CONTAINEROF(item, Drawable, tree_item);
+Drawable *drawable = SPICE_CONTAINEROF(item, Drawable, tree_item.base);
const int max_indent = 200;
char indent_str[max_indent + 1];
int indent_str_len;
--
2.4.3
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 07/15] server: merge spice-bitmap-utils and spice_bitmap_utils
>
> This commit introduces a compile failure:
>
> included from ../../server/spice-bitmap-utils.c:21:0:
> ../../server/spice-bitmap-utils.h:38:17: error: expected declaration
> specifiers
> or '...' before 'sizeof'
> G_STATIC_ASSERT(sizeof(rgb32_pixel_t) == 4);
>
That's really weird. It works for me.
Can you try with and without this patch?
Frediano
>
> On Thu, 2015-12-03 at 16:26 +, Frediano Ziglio wrote:
> > Signed-off-by: Marc-André Lureau
> > Signed-off-by: Frediano Ziglio
> > ---
> > server/Makefile.am | 2 -
> > server/display-channel.h| 2 +-
> > server/spice-bitmap-utils.c | 162 ++
> > server/spice-bitmap-utils.h | 15 ++--
> > server/spice_bitmap_utils.c | 188
> > ---
> > -
> > server/spice_bitmap_utils.h | 8 --
> > 6 files changed, 171 insertions(+), 206 deletions(-)
> > delete mode 100644 server/spice_bitmap_utils.c
> > delete mode 100644 server/spice_bitmap_utils.h
> >
> > diff --git a/server/Makefile.am b/server/Makefile.am
> > index 1e7e8fa..2bb4fec 100644
> > --- a/server/Makefile.am
> > +++ b/server/Makefile.am
> > @@ -124,8 +124,6 @@ libspice_server_la_SOURCES =\
> > spice_timer_queue.h \
> > zlib-encoder.c \
> > zlib-encoder.h \
> > - spice_bitmap_utils.h\
> > - spice_bitmap_utils.c\
> > image-cache.h \
> > image-cache.c \
> > pixmap-cache.h \
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 7cbc58d..8adff0a 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -39,7 +39,7 @@
> > #include "main-channel.h"
> > #include "migration-protocol.h"
> > #include "main-dispatcher.h"
> > -#include "spice_bitmap_utils.h"
> > +#include "spice-bitmap-utils.h"
> > #include "image-cache.h"
> > #include "utils.h"
> > #include "tree.h"
> > diff --git a/server/spice-bitmap-utils.c b/server/spice-bitmap-utils.c
> > index 03d7694..8d6e7c6 100644
> > --- a/server/spice-bitmap-utils.c
> > +++ b/server/spice-bitmap-utils.c
> > @@ -117,3 +117,165 @@ int bitmap_has_extra_stride(SpiceBitmap *bitmap)
> > }
> > return 0;
> > }
> > +
> > +int spice_bitmap_from_surface_type(uint32_t surface_format)
> > +{
> > +switch (surface_format) {
> > +case SPICE_SURFACE_FMT_16_555:
> > +return SPICE_BITMAP_FMT_16BIT;
> > +case SPICE_SURFACE_FMT_32_xRGB:
> > +return SPICE_BITMAP_FMT_32BIT;
> > +case SPICE_SURFACE_FMT_32_ARGB:
> > +return SPICE_BITMAP_FMT_RGBA;
> > +case SPICE_SURFACE_FMT_8_A:
> > +return SPICE_BITMAP_FMT_8BIT_A;
> > +default:
> > +spice_critical("Unsupported surface format");
> > +}
> > +return 0;
> > +}
> > +
> > +#define RAM_PATH "/tmp/tmpfs"
> > +
> > +static void dump_palette(FILE *f, SpicePalette* plt)
> > +{
> > +int i;
> > +for (i = 0; i < plt->num_ents; i++) {
> > +fwrite(plt->ents + i, sizeof(uint32_t), 1, f);
> > +}
> > +}
> > +
> > +static void dump_line(FILE *f, uint8_t* line, uint16_t n_pixel_bits, int
> > width, int row_size)
> > +{
> > +int i;
> > +int copy_bytes_size = SPICE_ALIGN(n_pixel_bits * width, 8) / 8;
> > +
> > +fwrite(line, 1, copy_bytes_size, f);
> > +if (row_size > copy_bytes_size) {
> > +// each line should be 4 bytes aligned
> > +for (i = copy_bytes_size; i < row_size; i++) {
> > +fprintf(f, "%c", 0);
> > +}
> > +}
> > +}
> > +void dump_bitmap(SpiceBitmap *bitmap)
> > +{
> > +static uint32_t file_id = 0;
> > +
> > +char file_str[200];
> > +int rgb = TRUE;
> > +uint16_t n_pixel_bits;
> > +SpicePalette *plt = NULL;
> > +uint32_t id;
> > +int row_size;
> > +uint32_t file_size;
> > +int alpha = 0;
> > +uint32_t header_size = 14 + 40;
> > +uint32_t bitmap_data_offset;
> > +uint32_t tmp_u32;
> > +int32_t tmp_32;
> > +uint16_t tmp_u16;
> > +FILE *f;
> > +int i, j;
> > +
> > +switch (bitmap->format) {
> > +case SPICE_BITMAP_FMT_1BIT_BE:
> > +case SPICE_BITMAP_FMT_1BIT_LE:
> > +rgb = FALSE;
> > +n_pixel_bits = 1;
> > +break;
> > +case SPICE_BITMAP_FMT_4BIT_BE:
> > +case SPICE_BITMAP_FMT_4BIT_LE:
> > +rgb = FALSE;
> > +n_pixel_bits = 4;
> > +break;
> > +case SPICE_BITMAP_FMT_8BIT:
> > +rgb = FALSE;
> > +n_pixel_bits = 8;
> > +break;
> > +case SPICE_BITMAP_FMT_16BIT:
> > +n_pixel_bits = 16;
> > +break;
> > +case SPICE_BITMAP_FMT_24BIT:
> > +n_pixel_bits = 24;
> > +break;
> > +case SPICE_BITMAP_FMT_32BIT:
> > +n_pixel_bits = 32;
> > +break;
> > +case SPICE_BITMAP_FMT_RGBA:
> > +n_pixel_bits = 32;
> > +alpha = 1;
> > +
Re: [Spice-devel] [PATCH] memslot: change some spice_assert to spice_return_if_fail
>
>
> On Thu, 2015-12-03 at 16:45 +, Frediano Ziglio wrote:
> > Signed-off-by: Marc-André Lureau
> > Signed-off-by: Frediano Ziglio
> > ---
> > server/red_memslots.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/server/red_memslots.c b/server/red_memslots.c
> > index 2fd939e..e883c54 100644
> > --- a/server/red_memslots.c
> > +++ b/server/red_memslots.c
> > @@ -168,8 +168,8 @@ void memslot_info_add_slot(RedMemSlotInfo *info,
> > uint32_t
> > slot_group_id, uint32_
> >
> > void memslot_info_del_slot(RedMemSlotInfo *info, uint32_t slot_group_id,
> > uint32_t slot_id)
> > {
> > -spice_assert(info->num_memslots_groups > slot_group_id);
> > -spice_assert(info->num_memslots > slot_id);
> > +spice_return_if_fail(info->num_memslots_groups > slot_group_id);
> > +spice_return_if_fail(info->num_memslots > slot_id);
> >
> > info->mem_slots[slot_group_id][slot_id].virt_start_addr = 0;
> > info->mem_slots[slot_group_id][slot_id].virt_end_addr = 0;
>
> Acked-by: Jonathon Jongsma
>
Merged
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 06/15] server: rename files
>
> On Thu, 2015-12-03 at 16:26 +, Frediano Ziglio wrote:
> > Signed-off-by: Marc-André Lureau
> > Signed-off-by: Frediano Ziglio
> > ---
> > NEWS | 2 +-
> > server/Makefile.am | 90
> > +++--
> > -
> > server/agent-msg-filter.c | 4 +-
> > server/cache-item.h| 2 +-
> > server/{char_device.c => char-device.c}| 4 +-
> > server/{char_device.h => char-device.h}| 10 +--
> > server/cursor-channel.h| 4 +-
> > server/dcc-encoders.h | 16 ++--
> > server/dcc.h | 2 +-
> > server/display-channel.h | 26 +++
> > ...glz_encoder_dictionary.c => glz-encoder-dict.c} | 4 +-
> > ...glz_encoder_dictionary.h => glz-encoder-dict.h} | 8 +-
> > ...r_dictionary_protected.h => glz-encoder-priv.h} | 6 +-
> > server/{glz_encoder.c => glz-encoder.c}| 4 +-
> > server/{glz_encoder.h => glz-encoder.h}| 2 +-
> > server/{spice_image_cache.c => image-cache.c} | 4 +-
> > server/{spice_image_cache.h => image-cache.h} | 6 +-
> > server/{inputs_channel.c => inputs-channel.c} | 12 +--
> > server/{inputs_channel.h => inputs-channel.h} | 2 +-
> > server/{jpeg_encoder.c => jpeg-encoder.c} | 4 +-
> > server/{jpeg_encoder.h => jpeg-encoder.h} | 0
> > server/{lz4_encoder.c => lz4-encoder.c}| 4 +-
> > server/{lz4_encoder.h => lz4-encoder.h}| 0
> > server/{main_channel.c => main-channel.c} | 10 +--
> > server/{main_channel.h => main-channel.h} | 2 +-
> > server/{main_dispatcher.c => main-dispatcher.c}| 6 +-
> > server/{main_dispatcher.h => main-dispatcher.h}| 2 +-
> > server/{red_memslots.c => memslot.c} | 4 +-
> > server/{red_memslots.h => memslot.h} | 8 +-
> > .../{migration_protocol.h => migration-protocol.h} | 2 +-
> > server/{mjpeg_encoder.c => mjpeg-encoder.c}| 4 +-
> > server/{mjpeg_encoder.h => mjpeg-encoder.h}| 2 +-
> > server/pixmap-cache.h | 2 +-
> > server/{red_channel.c => red-channel.c}| 8 +-
> > server/{red_channel.h => red-channel.h}| 6 +-
> > server/{red_common.h => red-common.h} | 0
> > server/{red_dispatcher.c => red-dispatcher.c} | 8 +-
> > server/{red_dispatcher.h => red-dispatcher.h} | 2 +-
> > server/{red_parse_qxl.c => red-parse-qxl.c}| 6 +-
> > server/{red_parse_qxl.h => red-parse-qxl.h}| 4 +-
> > server/{red_record_qxl.c => red-record-qxl.c} | 10 +--
> > server/{red_record_qxl.h => red-record-qxl.h} | 4 +-
> > server/{red_replay_qxl.c => red-replay-qxl.c} | 10 +--
> > server/{red_replay_qxl.h => red-replay-qxl.h} | 0
> > server/{red_worker.c => red-worker.c} | 2 +-
> > server/{red_worker.h => red-worker.h} | 6 +-
> > server/{reds_stream.c => reds-stream.c}| 6 +-
> > server/{reds_stream.h => reds-stream.h}| 0
> > server/reds.c | 18 ++---
> > server/reds.h | 6 +-
> > server/smartcard.c | 6 +-
> > server/{snd_worker.c => sound.c} | 8 +-
> > server/{snd_worker.h => sound.h} | 4 +-
> > server/spice_timer_queue.c | 2 +-
> > server/spicevmc.c | 6 +-
> > server/stream.h| 6 +-
> > server/{reds_sw_canvas.c => sw-canvas.c} | 2 +-
> > server/{reds_sw_canvas.h => sw-canvas.h} | 6 +-
> > server/tests/replay.c | 2 +-
> > server/tests/test_display_base.c | 2 +-
> > server/tree.c | 2 +-
> > server/{zlib_encoder.c => zlib-encoder.c} | 4 +-
> > server/{zlib_encoder.h => zlib-encoder.h} | 0
> > 63 files changed, 202 insertions(+), 202 deletions(-)
> > rename server/{char_device.c => char-device.c} (99%)
> > rename server/{char_device.h => char-device.h} (98%)
> > rename server/{glz_encoder_dictionary.c => glz-encoder-dict.c} (99%)
> > rename server/{glz_encoder_dictionary.h => glz-encoder-dict.h} (95%)
> > rename server/{glz_encoder_dictionary_protected.h => glz-encoder-priv.h}
> > (98%)
> > rename server/{glz_encoder.c => glz-encoder.c} (99%)
> > rename server/{glz_encoder.h => glz-encoder.h} (98%)
> > rename server/{spice_image_cache.c => image-cache.c} (99%)
> > rename server/{spice_image_cache.h => image-cache.h} (94%)
> > rename server/{inputs_channel.c => inputs-channel.c} (99%)
> > rename server/{inputs_channel.h => inputs-channel.h} (95%)
> > rename
Re: [Spice-devel] [PATCH 03/15] worker: move red_process_draw to display-channel.c
>
> On Thu, 2015-12-03 at 16:26 +, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio
> > Acked-by: Fabiano Fidêncio
>
> I'll ACK it as well
>
> Acked-by: Jonathon Jongsma
>
>
> > ---
> > server/display-channel.c | 25 +
> > server/display-channel.h | 7 ++-
> > server/red_worker.c | 20 ++--
> > 3 files changed, 25 insertions(+), 27 deletions(-)
> >
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 007512e..94c32e3 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1098,9 +1098,9 @@ static int validate_drawable_bbox(DisplayChannel
> > *display, RedDrawable *drawable
> > *
> > * @return initialized Drawable or NULL on failure
> > */
> > -Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t
> > effect,
> > - RedDrawable *red_drawable, uint32_t
> > group_id,
> > - uint32_t
> > process_commands_generation)
> > +static Drawable *display_channel_get_drawable(DisplayChannel *display,
> > uint8_t effect,
> > + RedDrawable *red_drawable,
> > uint32_t group_id,
> > + uint32_t
> > process_commands_generation)
> > {
> > Drawable *drawable;
> > int x;
> > @@ -1145,7 +1145,7 @@ Drawable *display_channel_get_drawable(DisplayChannel
> > *display, uint8_t effect,
> > * Add a Drawable to the items to draw.
> > * On failure the Drawable is not added.
> > */
> > -void display_channel_add_drawable(DisplayChannel *display, Drawable
> > *drawable)
> > +static void display_channel_add_drawable(DisplayChannel *display, Drawable
> > *drawable)
> > {
> > int success = FALSE, surface_id = drawable->surface_id;
> > RedDrawable *red_drawable = drawable->red_drawable;
> > @@ -1195,6 +1195,23 @@ void display_channel_add_drawable(DisplayChannel
> > *display, Drawable *drawable)
> > #endif
> > }
> >
> > +void display_channel_process_draw(DisplayChannel *display, RedDrawable
> > *red_drawable,
> > + uint32_t group_id, int
> > process_commands_generation)
> > +{
> > +Drawable *drawable =
> > +display_channel_get_drawable(display, red_drawable->effect,
> > red_drawable, group_id,
> > + process_commands_generation);
> > +
> > +if (!drawable) {
> > +return;
> > +}
> > +
> > +display_channel_add_drawable(display, drawable);
> > +
> > +display_channel_drawable_unref(display, drawable);
> > +}
> > +
> > +
> > int display_channel_wait_for_migrate_data(DisplayChannel *display)
> > {
> > uint64_t end_time = red_get_monotonic_time() +
> > DISPLAY_CLIENT_MIGRATE_DATA_TIMEOUT;
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 83b50ca..a990e09 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -286,8 +286,6 @@ void
> > display_channel_surface_unref
> >(DisplayCha
> >
> > uint32_t surface_id);
> > bool display_channel_surface_has_canvas
> > (DisplayChannel *display,
> >
> > uint32_t surface_id);
> > -void display_channel_add_drawable
> > (DisplayChannel *display,
> > -
> > Drawable *drawable);
> > void display_channel_current_flush
> > (DisplayChannel *display,
> >int
> > surface_id);
> > intdisplay_channel_wait_for_migrate_data
> > (DisplayChannel *display);
> > @@ -300,11 +298,10 @@ void
> > display_channel_destroy_surfaces (DisplayCha
> > void display_channel_destroy_surface
> > (DisplayChannel *display,
> >
> > uint32_t surface_id);
> > uint32_t display_channel_generate_uid
> > (DisplayChannel *display);
> > -Drawable * display_channel_get_drawable
> > (DisplayChannel *display,
> > -
> > uint8_t
> > effect,
> > +void display_channel_process_draw
> > (DisplayChannel *display,
> >
> > RedDrawable *red_drawable,
> >
> > uint32_t group_id,
> > -
> > uint32_t process_commands_generation);
> > + int
> > process_commands_generation);
> > void display_channel_process_surface_cmd
> > (DisplayChannel *display,
> >
> > RedSurfaceCmd *surface,
> >
Re: [Spice-devel] [PATCH 02/15] display: make get_drawable symmetric to display_channel_drawable_unref
>
> On Thu, 2015-12-03 at 16:26 +, Frediano Ziglio wrote:
> > Make possible to safely call display_channel_drawable_unref straight
> > forward to get_drawable call.
>
> I would suggest changing this to "straight after calling get_drawable"
>
> >
> > Problem was function definitions and dependency.
> >
> > display_channel_drawable_try_new is supposed to return an uninitialized
> > pointer (or NULL on failure) to a Drawable structure.
> >
> > (display_channel_)get_drawable is supposed to return an initialized
> > pointer (or NULL) to a Drawable structure.
> >
> > (display_channel_)add_drawable is supposed to add the Drawable to the
> > list/tree of drawing to draw.
> >
> > Now, with these definitions after get_drawable the Drawable state (if
> > pointer is not NULL) should be consistent and we should be able to call
> > display_channel_drawable_unref.
> >
> > In the current code this was not true as for instance surface_id was
> > copied to Drawable but the reference counter of the surface was not
> > incremented leading possible unref call to decrement the counter and
> > free the surface. This can happen if any call between get_drawable and
> > unref does not increment the reference in a consistent way. This
> > basically means that every present or future code in the path between
> > get_drawable and unref have to know this unconsistency and handle it.
> > This is a maintaining problem as people need to know these details when
> > editing existing code (actually this is display_channel_add_drawable
> > code).
> > This basically create a dependency between get_drawable and
> > add_drawable.
> >
> > Signed-off-by: Frediano Ziglio
> > ---
> > server/display-channel.c | 39 ---
> > 1 file changed, 28 insertions(+), 11 deletions(-)
> >
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 722ee86..007512e 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1091,6 +1091,13 @@ static int validate_drawable_bbox(DisplayChannel
> > *display, RedDrawable *drawable
> > return TRUE;
> > }
> >
> > +/**
> > + * @brief Get a new Drawable
> > + *
> > + * The Drawable returned is fully initialized.
> > + *
> > + * @return initialized Drawable or NULL on failure
> > + */
> > Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t
> > effect,
> > RedDrawable *red_drawable, uint32_t
> > group_id,
> > uint32_t
> > process_commands_generation)
> > @@ -1098,6 +1105,9 @@ Drawable *display_channel_get_drawable(DisplayChannel
> > *display, uint8_t effect,
> > Drawable *drawable;
> > int x;
> >
> > +/* Validate all surface ids before updating counters
> > + * to avoid invalid updates if we find an invalid id.
> > + */
> > if (!validate_drawable_bbox(display, red_drawable)) {
> > return NULL;
> > }
> > @@ -1117,20 +1127,30 @@ Drawable
> > *display_channel_get_drawable(DisplayChannel
> > *display, uint8_t effect,
> > drawable->red_drawable = red_drawable_ref(red_drawable);
> >
> > drawable->surface_id = red_drawable->surface_id;
> > +display->surfaces[drawable->surface_id].refs++;
> > +
> > memcpy(drawable->surface_deps, red_drawable->surface_deps,
> > sizeof(drawable->surface_deps));
> > +/*
> > +surface->refs is affected by a drawable (that is
> > +dependent on the surface) as long as the drawable is alive.
> > +However, surface->depend_on_me is affected by a drawable only
> > +as long as it is in the current tree (hasn't been rendered yet).
> > +*/
> > +red_inc_surfaces_drawable_dependencies(display, drawable);
> >
> > return drawable;
> > }
> >
> > +/**
> > + * Add a Drawable to the items to draw.
> > + * On failure the Drawable is not added.
> > + */
> > void display_channel_add_drawable(DisplayChannel *display, Drawable
> > *drawable)
> > {
> > int success = FALSE, surface_id = drawable->surface_id;
> > RedDrawable *red_drawable = drawable->red_drawable;
> >
> > red_drawable->mm_time = reds_get_mm_time();
> > -surface_id = drawable->surface_id;
> > -
> > -display->surfaces[surface_id].refs++;
> >
> > region_add(&drawable->tree_item.base.rgn, &red_drawable->bbox);
> >
> > @@ -1143,14 +1163,6 @@ void display_channel_add_drawable(DisplayChannel
> > *display, Drawable *drawable)
> > region_destroy(&rgn);
> > }
> >
> > -/*
> > -surface->refs is affected by a drawable (that is
> > -dependent on the surface) as long as the drawable is alive.
> > -However, surface->depend_on_me is affected by a drawable only
> > -as long as it is in the current tree (hasn't been rendered yet).
> > -*/
> > -red_inc_surfaces_drawable_dependencies(display, drawable);
> > -
> > if (regio
Re: [Spice-devel] [PATCH 18/19] display: rename detach_streams_behind
>
> On Wed, Nov 25, 2015 at 4:27 PM, Frediano Ziglio wrote:
> > From: Marc-André Lureau
> >
> > ---
> > server/display-channel.c | 12 ++--
> > server/display-channel.h | 1 -
> > server/stream.c | 2 +-
> > server/stream.h | 3 +++
> > 4 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index f2c3fc6..811799a 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -711,7 +711,7 @@ static int current_add_with_shadow(DisplayChannel
> > *display, Ring *ring, Drawable
> >
> > // only primary surface streams are supported
> > if (is_primary_surface(display, item->surface_id)) {
> > -detach_streams_behind(display, &shadow->base.rgn, NULL);
> > +stream_detach_behind(display, &shadow->base.rgn, NULL);
> > }
> >
> > ring_add(ring, &shadow->base.siblings_link);
> > @@ -724,7 +724,7 @@ static int current_add_with_shadow(DisplayChannel
> > *display, Ring *ring, Drawable
> > streams_update_visible_region(display, item);
> > } else {
> > if (is_primary_surface(display, item->surface_id)) {
> > -detach_streams_behind(display, &item->tree_item.base.rgn,
> > item);
> > +stream_detach_behind(display, &item->tree_item.base.rgn,
> > item);
> > }
> > }
> > stat_add(&display->add_stat, start_time);
> > @@ -834,14 +834,14 @@ static int current_add(DisplayChannel *display, Ring
> > *ring, Drawable *drawable)
> > current_add_drawable(display, drawable, ring);
> > } else {
> > /*
> > - * red_detach_streams_behind can affect the current tree since
> > + * stream_detach_behind can affect the current tree since
> > * it may trigger calls to display_channel_draw. Thus, the
> > * drawable should be added to the tree before calling
> > - * red_detach_streams_behind
> > + * stream_detach_behind
> > */
> > current_add_drawable(display, drawable, ring);
> > if (is_primary_surface(display, drawable->surface_id)) {
> > -detach_streams_behind(display, &drawable->tree_item.base.rgn,
> > drawable);
> > +stream_detach_behind(display, &drawable->tree_item.base.rgn,
> > drawable);
> > }
> > }
> > region_destroy(&exclude_rgn);
> > @@ -1024,7 +1024,7 @@ static int handle_surface_deps(DisplayChannel
> > *display, Drawable *drawable)
> > QRegion depend_region;
> > region_init(&depend_region);
> > region_add(&depend_region,
> > &drawable->red_drawable->surfaces_rects[x]);
> > -detach_streams_behind(display, &depend_region, NULL);
> > +stream_detach_behind(display, &depend_region, NULL);
> > }
> > }
> > }
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index b89f84e..0e0c1f1 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -432,7 +432,6 @@ static inline void region_add_clip_rects(QRegion *rgn,
> > SpiceClipRects *data)
> > }
> > }
> >
> > -void detach_streams_behind(DisplayChannel *display, QRegion *region,
> > Drawable *drawable);
>
> I suggested this change in some of the previous patches ...
>
> > void drawable_draw(DisplayChannel *display, Drawable *item);
> > void drawables_init(DisplayChannel *display);
> >
> > diff --git a/server/stream.c b/server/stream.c
> > index 50881e5..33a9893 100644
> > --- a/server/stream.c
> > +++ b/server/stream.c
> > @@ -842,7 +842,7 @@ static void detach_stream_gracefully(DisplayChannel
> > *display, Stream *stream,
> > * involves sending an upgrade image to the client, this
> > drawable won't be rendered
> > * (see dcc_detach_stream_gracefully).
> > */
> > -void detach_streams_behind(DisplayChannel *display, QRegion *region,
> > Drawable *drawable)
> > +void stream_detach_behind(DisplayChannel *display, QRegion *region,
> > Drawable *drawable)
> > {
> > Ring *ring = &display->streams;
> > RingItem *item = ring_get_head(ring);
> > diff --git a/server/stream.h b/server/stream.h
> > index b11b10f..d7d9a37 100644
> > --- a/server/stream.h
> > +++ b/server/stream.h
> > @@ -159,6 +159,9 @@ void stream_timeout
> > (DisplayChan
> > void stream_detach_and_stop
> > (DisplayChannel *display);
> > void stream_trace_add_drawable
> > (DisplayChannel *display,
> >
> > Drawable
> >
> > *item);
> > +void stream_detach_behind
> > (DisplayChannel *display,
> > +
> > QRegion
> > *region,
> > +
> > Drawable
> > *drawable);
> >
> > void stream_agent_unref
> > (DisplayChannel *display,
> >
Re: [Spice-devel] [PATCH 10/16] display: add update_compression() method
>
> On Thu, 2015-11-26 at 16:06 +, Frediano Ziglio wrote:
> > From: Marc-André Lureau
> >
> > ---
> > server/display-channel.c | 17 +
> > server/display-channel.h | 2 ++
> > server/red_worker.c | 21 -
> > 3 files changed, 23 insertions(+), 17 deletions(-)
> >
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 28fd565..13a14a2 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -2088,3 +2088,20 @@ exit:
> > red_put_surface_cmd(surface);
> > free(surface);
> > }
> > +
> > +void display_channel_update_compression(DisplayChannel *display,
> > DisplayChannelClient *dcc)
> > +{
> > +if (dcc->jpeg_state == SPICE_WAN_COMPRESSION_AUTO) {
> > +display->enable_jpeg = dcc->common.is_low_bandwidth;
> > +} else {
> > +display->enable_jpeg = (dcc->jpeg_state ==
> > SPICE_WAN_COMPRESSION_ALWAYS);
> > +}
> > +
> > +if (dcc->zlib_glz_state == SPICE_WAN_COMPRESSION_AUTO) {
> > +display->enable_zlib_glz_wrap = dcc->common.is_low_bandwidth;
> > +} else {
> > +display->enable_zlib_glz_wrap = (dcc->zlib_glz_state ==
> > SPICE_WAN_COMPRESSION_ALWAYS);
> > +}
> > +spice_info("jpeg %s", display->enable_jpeg ? "enabled" : "disabled");
> > +spice_info("zlib-over-glz %s", display->enable_zlib_glz_wrap ?
> > "enabled"
> > : "disabled");
> > +}
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 5fa17e6..7f3d408 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -305,6 +305,8 @@
> > void display_channel_process_surface_cmd
> > (DisplayC
> > ha
> >
> > RedSurf
> > aceCmd *surface,
> >
> > uint32_
> > t group_id,
> > int
> > loadvm);
> > +void display_channel_update_compression
> > (Display
> > Channel *display,
> > +
> >
> > Display
> > ChannelClient *dcc);
> >
> > static inline int validate_surface(DisplayChannel *display, uint32_t
> > surface_id)
> > {
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index cf9e41f..2a58052 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -727,34 +727,21 @@ static void handle_new_display_channel(RedWorker
> > *worker, RedClient *client, Red
> > uint32_t *common_caps, int
> > num_common_caps,
> > uint32_t *caps, int num_caps)
> > {
> > -DisplayChannel *display_channel;
> > +DisplayChannel *display;
>
> I don't see a reason for these renames, but ok
>
> > DisplayChannelClient *dcc;
> >
> > spice_return_if_fail(worker->display_channel);
> >
> > -display_channel = worker->display_channel;
> > +display = worker->display_channel;
> > spice_info("add display channel client");
> > -dcc = dcc_new(display_channel, client, stream, migrate,
> > +dcc = dcc_new(display, client, stream, migrate,
> > common_caps, num_common_caps, caps, num_caps,
> > worker->image_compression, worker->jpeg_state, worker-
> > >zlib_glz_state);
> > if (!dcc) {
> > return;
> > }
> >
> > -if (dcc->jpeg_state == SPICE_WAN_COMPRESSION_AUTO) {
> > -display_channel->enable_jpeg = dcc->common.is_low_bandwidth;
> > -} else {
> > -display_channel->enable_jpeg = (dcc->jpeg_state ==
> > SPICE_WAN_COMPRESSION_ALWAYS);
> > -}
> > -
> > -if (dcc->zlib_glz_state == SPICE_WAN_COMPRESSION_AUTO) {
> > -display_channel->enable_zlib_glz_wrap =
> > dcc->common.is_low_bandwidth;
> > -} else {
> > -display_channel->enable_zlib_glz_wrap = (dcc->zlib_glz_state ==
> > -
> >
> > SPICE_WAN_COMPRESSION_ALWAYS
> > );
> > -}
> > -spice_info("jpeg %s", display_channel->enable_jpeg ? "enabled" :
> > "disabled");
> > -spice_info("zlib-over-glz %s", display_channel->enable_zlib_glz_wrap ?
> > "enabled" : "disabled");
> > +display_channel_update_compression(display, dcc);
> >
> > guest_set_client_capabilities(worker);
> > dcc_start(dcc);
>
> Acked-by: Pavel Grunt
>
>
Merged
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 09/16] worker: move red_process_surface
>
> Acked-by: Jonathon Jongsma
>
>
> On Thu, 2015-11-26 at 16:06 +, Frediano Ziglio wrote:
> > From: Marc-André Lureau
> >
> > ---
> > server/display-channel.c | 59 +++
> > server/display-channel.h | 4 +++
> > server/red_worker.c | 66
> > +++
> > -
> > 3 files changed, 67 insertions(+), 62 deletions(-)
> >
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 1eb2fa1..28fd565 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -2029,3 +2029,62 @@ DisplayChannel* display_channel_new(RedWorker
> > *worker,
> > int migrate, int stream_v
> >
> > return display;
> > }
> > +
> > +static inline void set_surface_release_info(QXLReleaseInfoExt
> > *release_info_ext,
> > +QXLReleaseInfo *release_info,
> > uint32_t group_id)
> > +{
> > +release_info_ext->info = release_info;
> > +release_info_ext->group_id = group_id;
> > +}
> > +
> > +void display_channel_process_surface_cmd(DisplayChannel *display,
> > RedSurfaceCmd *surface,
> > + uint32_t group_id, int loadvm)
> > +{
> > +uint32_t surface_id;
> > +RedSurface *red_surface;
> > +uint8_t *data;
> > +
> > +surface_id = surface->surface_id;
> > +if SPICE_UNLIKELY(surface_id >= display->n_surfaces) {
> > +goto exit;
> > +}
> > +
> > +red_surface = &display->surfaces[surface_id];
> > +
> > +switch (surface->type) {
> > +case QXL_SURFACE_CMD_CREATE: {
> > +uint32_t height = surface->u.surface_create.height;
> > +int32_t stride = surface->u.surface_create.stride;
> > +int reloaded_surface = loadvm || (surface->flags &
> > QXL_SURF_FLAG_KEEP_DATA);
> > +
> > +if (red_surface->refs) {
> > +spice_warning("avoiding creating a surface twice");
> > +break;
> > +}
> > +data = surface->u.surface_create.data;
> > +if (stride < 0) {
> > +data -= (int32_t)(stride * (height - 1));
> > +}
> > +display_channel_create_surface(display, surface_id, surface
> > ->u.surface_create.width,
> > + height, stride, surface
> > ->u.surface_create.format, data,
> > + reloaded_surface,
> > + // reloaded surfaces will be sent
> > on
> > demand
> > + !reloaded_surface);
> > +set_surface_release_info(&red_surface->create,
> > surface->release_info,
> > group_id);
> > +break;
> > +}
> > +case QXL_SURFACE_CMD_DESTROY:
> > +if (!red_surface->refs) {
> > +spice_warning("avoiding destroying a surface twice");
> > +break;
> > +}
> > +set_surface_release_info(&red_surface->destroy, surface
> > ->release_info, group_id);
> > +display_channel_destroy_surface(display, surface_id);
> > +break;
> > +default:
> > +spice_warn_if_reached();
> > +};
> > +exit:
> > +red_put_surface_cmd(surface);
> > +free(surface);
> > +}
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index e17381c..5fa17e6 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -301,6 +301,10 @@ void
> > display_channel_destroy_surfaces (DisplayCha
> > void display_channel_destroy_surface
> > (DisplayChannel *display,
> >
> > uint32_t surface_id);
> > uint32_t display_channel_generate_uid
> > (DisplayChannel *display);
> > +void display_channel_process_surface_cmd
> > (DisplayChannel *display,
> > +
> > RedSurfaceCmd *surface,
> > +
> > uint32_t group_id,
> > + int
> > loadvm);
> >
> > static inline int validate_surface(DisplayChannel *display, uint32_t
> > surface_id)
> > {
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 77ad06b..cf9e41f 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -149,13 +149,6 @@ static void common_release_recv_buf(RedChannelClient
> > *rcc, uint16_t type, uint32
> > }
> > }
> >
> > -static inline void set_surface_release_info(QXLReleaseInfoExt
> > *release_info_ext,
> > -QXLReleaseInfo *release_info,
> > uint32_t group_id)
> > -{
> > -release_info_ext->info = release_info;
> > -release_info_ext->group_id = group_id;
> > -}
> > -
> > void red_drawable_unref(RedWorker *worker, RedDrawable *red_drawable,
> > uint32_t group_id)
> > {
> > @@ -192,59 +185,6 @@ static void red_process_draw(RedWorker *worker,
> > RedDrawable *red_drawable,
> > }
> >
> >
> > -s
Re: [Spice-devel] [PATCH 11/16] worker: merge handle_new_display_channel
>
> Fine. It's just code movement, and the old function name was a bit misleading
> anyway (we're handling a new display client, not a new display channel)
>
> Acked-by: Jonathon Jongsma
>
>
> On Thu, 2015-11-26 at 16:06 +, Frediano Ziglio wrote:
> > From: Marc-André Lureau
> >
> > ---
> > server/red_worker.c | 49 -
> > 1 file changed, 16 insertions(+), 33 deletions(-)
> >
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index 2a58052..cf20ccd 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -722,31 +722,6 @@ static void guest_set_client_capabilities(RedWorker
> > *worker)
> > }
> > }
> >
> > -static void handle_new_display_channel(RedWorker *worker, RedClient
> > *client,
> > RedsStream *stream,
> > - int migrate,
> > - uint32_t *common_caps, int
> > num_common_caps,
> > - uint32_t *caps, int num_caps)
> > -{
> > -DisplayChannel *display;
> > -DisplayChannelClient *dcc;
> > -
> > -spice_return_if_fail(worker->display_channel);-
> > -display = worker->display_channel;
> > -spice_info("add display channel client");
> > -dcc = dcc_new(display, client, stream, migrate,
> > - common_caps, num_common_caps, caps, num_caps,
> > - worker->image_compression, worker->jpeg_state, worker
> > ->zlib_glz_state);
> > -if (!dcc) {
> > -return;
> > -}
> > -
> > -display_channel_update_compression(display, dcc);
> > -
> > -guest_set_client_capabilities(worker);
> > -dcc_start(dcc);
> > -}
> > -
> > static void cursor_connect(RedWorker *worker, RedClient *client,
> > RedsStream
> > *stream,
> > int migrate,
> > uint32_t *common_caps, int num_common_caps,
> > @@ -1107,14 +1082,22 @@ static void handle_dev_display_connect(void
> > *opaque,
> > void *payload)
> > {
> > RedWorkerMessageDisplayConnect *msg = payload;
> > RedWorker *worker = opaque;
> > -RedsStream *stream = msg->stream;
> > -RedClient *client = msg->client;
> > -int migration = msg->migration;
> > -
> > -spice_info("connect");
> > -handle_new_display_channel(worker, client, stream, migration,
> > - msg->common_caps, msg->num_common_caps,
> > - msg->caps, msg->num_caps);
> > +DisplayChannel *display = worker->display_channel;
> > +DisplayChannelClient *dcc;
> > +
> > +spice_info("connect new client");
> > +spice_return_if_fail(display);
> > +
> > +dcc = dcc_new(display, msg->client, msg->stream, msg->migration,
> > + msg->common_caps, msg->num_common_caps, msg->caps, msg
> > ->num_caps,
> > + worker->image_compression, worker->jpeg_state, worker
> > ->zlib_glz_state);
> > +if (!dcc) {
> > +return;
> > +}
> > +display_channel_update_compression(display, dcc);
> > +guest_set_client_capabilities(worker);
> > +dcc_start(dcc);
> > +
> > free(msg->caps);
> > free(msg->common_caps);
> > }
>
Merged
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 15/15] server/red_worker: add env SPICE_NOWAIT_CLIENTS
>
> >
> > From: Marc-André Lureau
> >
> > ---
> > server/red-worker.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/server/red-worker.c b/server/red-worker.c
> > index 71d18b7..49d5aa2 100644
> > --- a/server/red-worker.c
> > +++ b/server/red-worker.c
> > @@ -87,6 +87,7 @@ struct RedWorker {
> > int driver_cap_monitors_config;
> >
> > FILE *record_fd;
> > +bool wait_for_clients;
> > };
> >
> > GMainContext* red_worker_get_context(RedWorker *worker)
> > @@ -245,7 +246,7 @@ static int red_process_display(RedWorker *worker,
> > uint32_t max_pipe_size, int *r
> > *ring_is_empty = FALSE;
> > for (;;) {
> >
> > -if (display_is_connected(worker)) {
> > +if (display_is_connected(worker) && worker->wait_for_clients) {
> >
> > if
> > (red_channel_all_blocked(RED_CHANNEL(worker->display_channel)))
> > {
> > spice_info("all display clients are blocking");
> > @@ -1754,6 +1755,8 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> > RedDispatcher *red_dispatcher)
> > worker->display_channel = display_channel_new(worker, FALSE,
> > streaming_video,
> >init_info.n_surfaces);
> >
> > +worker->wait_for_clients = !g_getenv("SPICE_NOWAIT_CLIENTS");
> > +
> > return worker;
> > }
> >
>
> Nack this patch
>
> Frediano
Rejected
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 10/15] worker: use glib main loop
- Original Message -
> On Thu, 2015-12-03 at 16:27 +, Frediano Ziglio wrote:
> > From: Marc-André Lureau
> >
> > Clean up, more extensible.
>
> I think this should be a bit less terse... perhaps:
> Use the glib mainloop instead of writing our own. The glib loop is both
> cleaner
> to use and is more extensible. It is also very mature and reduces the
> maintenance burden on the spice server.
>
> >
> > Avoid server hanging when no client are connected.
>
>
> This sounds like something that might belong in a separate patch?
>
>
> > ---
> > server/Makefile.am | 2 -
> > server/red-worker.c| 395
> >
> > -
> > server/red-worker.h| 1 +
> > server/spice_timer_queue.c | 273 ---
> > server/spice_timer_queue.h | 43 -
> > 5 files changed, 246 insertions(+), 468 deletions(-)
> > delete mode 100644 server/spice_timer_queue.c
> > delete mode 100644 server/spice_timer_queue.h
> >
> > diff --git a/server/Makefile.am b/server/Makefile.am
> > index d4fc972..88825d8 100644
> > --- a/server/Makefile.am
> > +++ b/server/Makefile.am
> > @@ -121,8 +121,6 @@ libspice_server_la_SOURCES =\
> > spice.h \
> > stat.h \
> > spicevmc.c \
> > - spice_timer_queue.c \
> > - spice_timer_queue.h \
> > zlib-encoder.c \
> > zlib-encoder.h \
> > image-cache.h \
> > diff --git a/server/red-worker.c b/server/red-worker.c
> > index aecfcf9..9e8fcbb 100644
> > --- a/server/red-worker.c
> > +++ b/server/red-worker.c
> > @@ -49,31 +49,21 @@
> >
> > #include "spice.h"
> > #include "red-worker.h"
> > -#include "spice_timer_queue.h"
> > #include "cursor-channel.h"
> > #include "tree.h"
> >
> > #define CMD_RING_POLL_TIMEOUT 10 //milli
> > #define CMD_RING_POLL_RETRIES 200
> >
> > -#define MAX_EVENT_SOURCES 20
> > -#define INF_EVENT_WAIT ~0
> > -
> > -struct SpiceWatch {
> > -struct RedWorker *worker;
> > -SpiceWatchFunc watch_func;
> > -void *watch_func_opaque;
> > -};
> > -
> > struct RedWorker {
> > pthread_t thread;
> > clockid_t clockid;
> > +GMainContext *main_context;
> > QXLInstance *qxl;
> > RedDispatcher *red_dispatcher;
> > int running;
> > -struct pollfd poll_fds[MAX_EVENT_SOURCES];
> > -struct SpiceWatch watches[MAX_EVENT_SOURCES];
> > -unsigned int event_timeout;
> > +
> > +gint timeout;
> >
> > DisplayChannel *display_channel;
> > uint32_t display_poll_tries;
> > @@ -99,6 +89,13 @@ struct RedWorker {
> > FILE *record_fd;
> > };
> >
> > +GMainContext* red_worker_get_context(RedWorker *worker)
>
>
> I'd prefer _get_main_context()
fwiw, glib api uses _get_context() too (task_get_context(),
source_get_context() etc)
>
>
> > +{
> > +spice_return_val_if_fail(worker, NULL);
> > +
> > +return worker->main_context;
> > +}
> > +
> > QXLInstance* red_worker_get_qxl(RedWorker *worker)
> > {
> > spice_return_val_if_fail(worker != NULL, NULL);
> > @@ -182,7 +179,9 @@ static int red_process_cursor(RedWorker *worker,
> > uint32_t
> > max_pipe_size, int *ri
> > *ring_is_empty = TRUE;
> > if (worker->cursor_poll_tries < CMD_RING_POLL_RETRIES) {
> > worker->cursor_poll_tries++;
> > -worker->event_timeout = MIN(worker->event_timeout,
> > CMD_RING_POLL_TIMEOUT);
> > +worker->timeout = worker->timeout == -1 ?
> > +CMD_RING_POLL_TIMEOUT :
> > +MIN(worker->timeout, CMD_RING_POLL_TIMEOUT);
> > return n;
> > }
> > if (worker->cursor_poll_tries > CMD_RING_POLL_RETRIES ||
> > @@ -228,7 +227,6 @@ static int red_process_display(RedWorker *worker,
> > uint32_t
> > max_pipe_size, int *r
> > {
> > QXLCommandExt ext_cmd;
> > int n = 0;
> > -uint64_t start = red_get_monotonic_time();
> >
> > if (!worker->running) {
> > *ring_is_empty = TRUE;
> > @@ -237,14 +235,30 @@ static int red_process_display(RedWorker *worker,
> > uint32_t max_pipe_size, int *r
> >
> > worker->process_display_generation++;
> > *ring_is_empty = FALSE;
> > -while (!display_is_connected(worker) ||
> > - // TODO: change to average pipe size?
> > - red_channel_min_pipe_size(RED_CHANNEL(worker->display_channel))
> > <=
> > max_pipe_size) {
> > +for (;;) {
> > +
> > +if (display_is_connected(worker)) {
> > +
> > +if (red_channel_all_blocked(RED_CHANNEL(worker
> > ->display_channel))) {
> > +spice_info("all display clients are blocking");
> > +return n;
> > +}
>
> This is a change of behavior. The previous code checked for all_block
