[Spice-devel,01/10] server: move display_channel_client_create() to dcc_new()

Submitted by Frediano Ziglio on Nov. 2, 2015, 9:55 a.m.

Details

Message ID 1446458166-31403-2-git-send-email-fziglio@redhat.com
State New
Headers show
Series "Backported some patches from refactory branches (2nd Nov)" ( rev: 4 3 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Nov. 2, 2015, 9:55 a.m.
From: Marc-André Lureau <marcandre.lureau@gmail.com>

Move function from server/red_worker.c to new server/display-channel.c.
---
 server/Makefile.am       |  1 +
 server/display-channel.c | 38 +++++++++++++++++++++++++++
 server/display-channel.h | 50 ++++++++++++++++++++++++++++++++---
 server/red_worker.c      | 68 +++---------------------------------------------
 4 files changed, 89 insertions(+), 68 deletions(-)
 create mode 100644 server/display-channel.c

Patch hide | download patch | download mbox

diff --git a/server/Makefile.am b/server/Makefile.am
index 87288cc..428417b 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -105,6 +105,7 @@  libspice_server_la_SOURCES =			\
 	red_parse_qxl.h				\
 	red_worker.c				\
 	red_worker.h				\
+	display-channel.c			\
 	display-channel.h			\
 	cursor-channel.c			\
 	cursor-channel.h			\
diff --git a/server/display-channel.c b/server/display-channel.c
new file mode 100644
index 0000000..00be665
--- /dev/null
+++ b/server/display-channel.c
@@ -0,0 +1,38 @@ 
+/*
+   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 <http://www.gnu.org/licenses/>.
+*/
+#include "display-channel.h"
+
+DisplayChannelClient *dcc_new(DisplayChannel *display,
+                              RedClient *client, RedsStream *stream,
+                              int mig_target,
+                              uint32_t *common_caps, int num_common_caps,
+                              uint32_t *caps, int num_caps)
+{
+    DisplayChannelClient *dcc;
+
+    dcc = (DisplayChannelClient*)common_channel_new_client(
+        (CommonChannel *)display, sizeof(DisplayChannelClient),
+        client, stream, mig_target, TRUE,
+        common_caps, num_common_caps,
+        caps, num_caps);
+    spice_return_val_if_fail(dcc, NULL);
+
+    ring_init(&dcc->palette_cache_lru);
+    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
+
+    return dcc;
+}
diff --git a/server/display-channel.h b/server/display-channel.h
index e1ddc11..5d2eee5 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -15,14 +15,47 @@ 
    You should have received a copy of the GNU Lesser General Public
    License along with this library; if not, see <http://www.gnu.org/licenses/>.
 */
-#ifndef RED_WORKER_CLIENT_H_
-# define RED_WORKER_CLIENT_H_
+#ifndef DISPLAY_CHANNEL_H_
+# define DISPLAY_CHANNEL_H_
+
+#include <setjmp.h>
 
 #include "red_worker.h"
+#include "reds_stream.h"
 #include "cache-item.h"
 #include "pixmap-cache.h"
+#ifdef USE_OPENGL
+#include "common/ogl_ctx.h"
+#include "reds_gl_canvas.h"
+#endif /* USE_OPENGL */
+#include "reds_sw_canvas.h"
+#include "glz_encoder_dictionary.h"
+#include "glz_encoder.h"
+#include "stat.h"
+#include "reds.h"
+#include "mjpeg_encoder.h"
+#include "red_memslots.h"
+#include "red_parse_qxl.h"
+#include "red_record_qxl.h"
+#include "jpeg_encoder.h"
+#ifdef USE_LZ4
+#include "lz4_encoder.h"
+#endif
+#include "demarshallers.h"
+#include "zlib_encoder.h"
+#include "red_channel.h"
+#include "red_dispatcher.h"
+#include "dispatcher.h"
+#include "main_channel.h"
+#include "migration_protocol.h"
+#include "main_dispatcher.h"
+#include "spice_server_utils.h"
+#include "spice_bitmap_utils.h"
+#include "spice_image_cache.h"
 #include "utils.h"
 
+typedef struct DisplayChannel DisplayChannel;
+
 typedef struct Drawable Drawable;
 
 #define PALETTE_CACHE_HASH_SHIFT 8
@@ -30,6 +63,8 @@  typedef struct Drawable Drawable;
 #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
 #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
 
+#define CLIENT_PALETTE_CACHE_SIZE 128
+
 /* Each drawable can refer to at most 3 images: src, brush and mask */
 #define MAX_DRAWABLE_PIXMAP_CACHE_ITEMS 3
 
@@ -193,4 +228,13 @@  struct DisplayChannelClient {
     uint64_t streams_max_bit_rate;
 };
 
-#endif /* RED_WORKER_CLIENT_H_ */
+DisplayChannelClient*      dcc_new                                   (DisplayChannel *display,
+                                                                      RedClient *client,
+                                                                      RedsStream *stream,
+                                                                      int mig_target,
+                                                                      uint32_t *common_caps,
+                                                                      int num_common_caps,
+                                                                      uint32_t *caps,
+                                                                      int num_caps);
+
+#endif /* DISPLAY_CHANNEL_H_ */
diff --git a/server/red_worker.c b/server/red_worker.c
index 68cb153..89cb25c 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -43,7 +43,6 @@ 
 #include <poll.h>
 #include <pthread.h>
 #include <netinet/tcp.h>
-#include <setjmp.h>
 #include <openssl/ssl.h>
 #include <inttypes.h>
 #include <glib.h>
@@ -58,41 +57,11 @@ 
 #include "common/ring.h"
 #include "common/generated_server_marshallers.h"
 
-#ifdef USE_OPENGL
-#include "common/ogl_ctx.h"
-#include "reds_gl_canvas.h"
-#endif /* USE_OPENGL */
+#include "display-channel.h"
 
 #include "spice.h"
 #include "red_worker.h"
-#include "reds_stream.h"
-#include "reds_sw_canvas.h"
-#include "glz_encoder_dictionary.h"
-#include "glz_encoder.h"
-#include "stat.h"
-#include "reds.h"
-#include "mjpeg_encoder.h"
-#include "red_memslots.h"
-#include "red_parse_qxl.h"
-#include "red_record_qxl.h"
-#include "jpeg_encoder.h"
-#ifdef USE_LZ4
-#include "lz4_encoder.h"
-#endif
-#include "demarshallers.h"
-#include "zlib_encoder.h"
-#include "red_channel.h"
-#include "red_dispatcher.h"
-#include "dispatcher.h"
-#include "main_channel.h"
-#include "migration_protocol.h"
 #include "spice_timer_queue.h"
-#include "main_dispatcher.h"
-#include "spice_server_utils.h"
-#include "spice_bitmap_utils.h"
-#include "spice_image_cache.h"
-#include "pixmap-cache.h"
-#include "display-channel.h"
 #include "cursor-channel.h"
 
 //#define COMPRESS_STAT
@@ -294,8 +263,6 @@  typedef struct StreamActivateReportItem {
 #define WIDE_CLIENT_ACK_WINDOW 40
 #define NARROW_CLIENT_ACK_WINDOW 20
 
-#define CLIENT_PALETTE_CACHE_SIZE 128
-
 typedef struct ImageItem {
     PipeItem link;
     int refs;
@@ -311,8 +278,6 @@  typedef struct ImageItem {
     uint8_t data[0];
 } ImageItem;
 
-typedef struct DisplayChannel DisplayChannel;
-
 enum {
     STREAM_FRAME_NONE,
     STREAM_FRAME_NATIVE,
@@ -9472,28 +9437,6 @@  CommonChannelClient *common_channel_new_client(CommonChannel *common,
 }
 
 
-DisplayChannelClient *display_channel_client_create(CommonChannel *common,
-                                                    RedClient *client, RedsStream *stream,
-                                                    int mig_target,
-                                                    uint32_t *common_caps, int num_common_caps,
-                                                    uint32_t *caps, int num_caps)
-{
-    DisplayChannelClient *dcc =
-        (DisplayChannelClient*)common_channel_new_client(
-            common, sizeof(DisplayChannelClient), client, stream,
-            mig_target,
-            TRUE,
-            common_caps, num_common_caps,
-            caps, num_caps);
-
-    if (!dcc) {
-        return NULL;
-    }
-    ring_init(&dcc->palette_cache_lru);
-    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
-    return dcc;
-}
-
 RedChannel *red_worker_new_channel(RedWorker *worker, int size,
                                    uint32_t channel_type, int migration_flags,
                                    ChannelCbs *channel_cbs,
@@ -9776,13 +9719,8 @@  static void handle_new_display_channel(RedWorker *worker, RedClient *client, Red
     }
     display_channel = worker->display_channel;
     spice_info("add display channel client");
-    dcc = display_channel_client_create(&display_channel->common, client, stream,
-                                        migrate,
-                                        common_caps, num_common_caps,
-                                        caps, num_caps);
-    if (!dcc) {
-        return;
-    }
+    dcc = dcc_new(display_channel, client, stream, migrate,
+                  common_caps, num_common_caps, caps, num_caps);
     spice_info("New display (client %p) dcc %p stream %p", client, dcc, stream);
     stream_buf_size = 32*1024;
     dcc->send_data.stream_outbuf = spice_malloc(stream_buf_size);

Comments

> 
> From: Marc-André Lureau <marcandre.lureau@gmail.com>
> 
> Move function from server/red_worker.c to new server/display-channel.c.
> ---
>  server/Makefile.am       |  1 +
>  server/display-channel.c | 38 +++++++++++++++++++++++++++
>  server/display-channel.h | 50 ++++++++++++++++++++++++++++++++---
>  server/red_worker.c      | 68
>  +++---------------------------------------------
>  4 files changed, 89 insertions(+), 68 deletions(-)
>  create mode 100644 server/display-channel.c
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index 87288cc..428417b 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -105,6 +105,7 @@ libspice_server_la_SOURCES =			\
>  	red_parse_qxl.h				\
>  	red_worker.c				\
>  	red_worker.h				\
> +	display-channel.c			\
>  	display-channel.h			\
>  	cursor-channel.c			\
>  	cursor-channel.h			\
> diff --git a/server/display-channel.c b/server/display-channel.c
> new file mode 100644
> index 0000000..00be665
> --- /dev/null
> +++ b/server/display-channel.c
> @@ -0,0 +1,38 @@
> +/*
> +   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
> <http://www.gnu.org/licenses/>.
> +*/
> +#include "display-channel.h"
> +
> +DisplayChannelClient *dcc_new(DisplayChannel *display,
> +                              RedClient *client, RedsStream *stream,
> +                              int mig_target,
> +                              uint32_t *common_caps, int num_common_caps,
> +                              uint32_t *caps, int num_caps)
> +{
> +    DisplayChannelClient *dcc;
> +
> +    dcc = (DisplayChannelClient*)common_channel_new_client(
> +        (CommonChannel *)display, sizeof(DisplayChannelClient),
> +        client, stream, mig_target, TRUE,
> +        common_caps, num_common_caps,
> +        caps, num_caps);
> +    spice_return_val_if_fail(dcc, NULL);
> +
> +    ring_init(&dcc->palette_cache_lru);
> +    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
> +
> +    return dcc;
> +}
> diff --git a/server/display-channel.h b/server/display-channel.h
> index e1ddc11..5d2eee5 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -15,14 +15,47 @@
>     You should have received a copy of the GNU Lesser General Public
>     License along with this library; if not, see
>     <http://www.gnu.org/licenses/>.
>  */
> -#ifndef RED_WORKER_CLIENT_H_
> -# define RED_WORKER_CLIENT_H_
> +#ifndef DISPLAY_CHANNEL_H_
> +# define DISPLAY_CHANNEL_H_
> +
> +#include <setjmp.h>
>  
>  #include "red_worker.h"
> +#include "reds_stream.h"
>  #include "cache-item.h"
>  #include "pixmap-cache.h"
> +#ifdef USE_OPENGL
> +#include "common/ogl_ctx.h"
> +#include "reds_gl_canvas.h"
> +#endif /* USE_OPENGL */
> +#include "reds_sw_canvas.h"
> +#include "glz_encoder_dictionary.h"
> +#include "glz_encoder.h"
> +#include "stat.h"
> +#include "reds.h"
> +#include "mjpeg_encoder.h"
> +#include "red_memslots.h"
> +#include "red_parse_qxl.h"
> +#include "red_record_qxl.h"
> +#include "jpeg_encoder.h"
> +#ifdef USE_LZ4
> +#include "lz4_encoder.h"
> +#endif
> +#include "demarshallers.h"
> +#include "zlib_encoder.h"
> +#include "red_channel.h"
> +#include "red_dispatcher.h"
> +#include "dispatcher.h"
> +#include "main_channel.h"
> +#include "migration_protocol.h"
> +#include "main_dispatcher.h"
> +#include "spice_server_utils.h"
> +#include "spice_bitmap_utils.h"
> +#include "spice_image_cache.h"
>  #include "utils.h"
>  
> +typedef struct DisplayChannel DisplayChannel;
> +
>  typedef struct Drawable Drawable;
>  
>  #define PALETTE_CACHE_HASH_SHIFT 8
> @@ -30,6 +63,8 @@ typedef struct Drawable Drawable;
>  #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
>  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
>  
> +#define CLIENT_PALETTE_CACHE_SIZE 128
> +
>  /* Each drawable can refer to at most 3 images: src, brush and mask */
>  #define MAX_DRAWABLE_PIXMAP_CACHE_ITEMS 3
>  
> @@ -193,4 +228,13 @@ struct DisplayChannelClient {
>      uint64_t streams_max_bit_rate;
>  };
>  
> -#endif /* RED_WORKER_CLIENT_H_ */
> +DisplayChannelClient*      dcc_new
> (DisplayChannel *display,
> +
> RedClient
> *client,
> +
> RedsStream
> *stream,
> +                                                                      int
> mig_target,
> +
> uint32_t
> *common_caps,
> +                                                                      int
> num_common_caps,
> +
> uint32_t
> *caps,
> +                                                                      int
> num_caps);
> +
> +#endif /* DISPLAY_CHANNEL_H_ */
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 68cb153..89cb25c 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -43,7 +43,6 @@
>  #include <poll.h>
>  #include <pthread.h>
>  #include <netinet/tcp.h>
> -#include <setjmp.h>
>  #include <openssl/ssl.h>
>  #include <inttypes.h>
>  #include <glib.h>
> @@ -58,41 +57,11 @@
>  #include "common/ring.h"
>  #include "common/generated_server_marshallers.h"
>  
> -#ifdef USE_OPENGL
> -#include "common/ogl_ctx.h"
> -#include "reds_gl_canvas.h"
> -#endif /* USE_OPENGL */
> +#include "display-channel.h"
>  
>  #include "spice.h"
>  #include "red_worker.h"
> -#include "reds_stream.h"
> -#include "reds_sw_canvas.h"
> -#include "glz_encoder_dictionary.h"
> -#include "glz_encoder.h"
> -#include "stat.h"
> -#include "reds.h"
> -#include "mjpeg_encoder.h"
> -#include "red_memslots.h"
> -#include "red_parse_qxl.h"
> -#include "red_record_qxl.h"
> -#include "jpeg_encoder.h"
> -#ifdef USE_LZ4
> -#include "lz4_encoder.h"
> -#endif
> -#include "demarshallers.h"
> -#include "zlib_encoder.h"
> -#include "red_channel.h"
> -#include "red_dispatcher.h"
> -#include "dispatcher.h"
> -#include "main_channel.h"
> -#include "migration_protocol.h"
>  #include "spice_timer_queue.h"
> -#include "main_dispatcher.h"
> -#include "spice_server_utils.h"
> -#include "spice_bitmap_utils.h"
> -#include "spice_image_cache.h"
> -#include "pixmap-cache.h"
> -#include "display-channel.h"
>  #include "cursor-channel.h"
>  
>  //#define COMPRESS_STAT
> @@ -294,8 +263,6 @@ typedef struct StreamActivateReportItem {
>  #define WIDE_CLIENT_ACK_WINDOW 40
>  #define NARROW_CLIENT_ACK_WINDOW 20
>  
> -#define CLIENT_PALETTE_CACHE_SIZE 128
> -
>  typedef struct ImageItem {
>      PipeItem link;
>      int refs;
> @@ -311,8 +278,6 @@ typedef struct ImageItem {
>      uint8_t data[0];
>  } ImageItem;
>  
> -typedef struct DisplayChannel DisplayChannel;
> -
>  enum {
>      STREAM_FRAME_NONE,
>      STREAM_FRAME_NATIVE,
> @@ -9472,28 +9437,6 @@ CommonChannelClient
> *common_channel_new_client(CommonChannel *common,
>  }
>  
>  
> -DisplayChannelClient *display_channel_client_create(CommonChannel *common,
> -                                                    RedClient *client,
> RedsStream *stream,
> -                                                    int mig_target,
> -                                                    uint32_t *common_caps,
> int num_common_caps,
> -                                                    uint32_t *caps, int
> num_caps)
> -{
> -    DisplayChannelClient *dcc =
> -        (DisplayChannelClient*)common_channel_new_client(
> -            common, sizeof(DisplayChannelClient), client, stream,
> -            mig_target,
> -            TRUE,
> -            common_caps, num_common_caps,
> -            caps, num_caps);
> -
> -    if (!dcc) {
> -        return NULL;
> -    }
> -    ring_init(&dcc->palette_cache_lru);
> -    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
> -    return dcc;
> -}
> -
>  RedChannel *red_worker_new_channel(RedWorker *worker, int size,
>                                     uint32_t channel_type, int
>                                     migration_flags,
>                                     ChannelCbs *channel_cbs,
> @@ -9776,13 +9719,8 @@ static void handle_new_display_channel(RedWorker
> *worker, RedClient *client, Red
>      }
>      display_channel = worker->display_channel;
>      spice_info("add display channel client");
> -    dcc = display_channel_client_create(&display_channel->common, client,
> stream,
> -                                        migrate,
> -                                        common_caps, num_common_caps,
> -                                        caps, num_caps);
> -    if (!dcc) {
> -        return;
> -    }

The removal of this test does not make sense looking at same test in
display-channel.c. If can be NULL inside dcc_new why not checking here?
Or are we just going to accept the core in next lines?

I would add again these lines and ack it.

> +    dcc = dcc_new(display_channel, client, stream, migrate,
> +                  common_caps, num_common_caps, caps, num_caps);
>      spice_info("New display (client %p) dcc %p stream %p", client, dcc,
>      stream);
>      stream_buf_size = 32*1024;
>      dcc->send_data.stream_outbuf = spice_malloc(stream_buf_size);

Frediano
On Mon, Nov 2, 2015 at 11:02 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
>
>>
>> From: Marc-André Lureau <marcandre.lureau@gmail.com>
>>
>> Move function from server/red_worker.c to new server/display-channel.c.
>> ---
>>  server/Makefile.am       |  1 +
>>  server/display-channel.c | 38 +++++++++++++++++++++++++++
>>  server/display-channel.h | 50 ++++++++++++++++++++++++++++++++---
>>  server/red_worker.c      | 68
>>  +++---------------------------------------------
>>  4 files changed, 89 insertions(+), 68 deletions(-)
>>  create mode 100644 server/display-channel.c
>>
>> diff --git a/server/Makefile.am b/server/Makefile.am
>> index 87288cc..428417b 100644
>> --- a/server/Makefile.am
>> +++ b/server/Makefile.am
>> @@ -105,6 +105,7 @@ libspice_server_la_SOURCES =                      \
>>       red_parse_qxl.h                         \
>>       red_worker.c                            \
>>       red_worker.h                            \
>> +     display-channel.c                       \
>>       display-channel.h                       \
>>       cursor-channel.c                        \
>>       cursor-channel.h                        \
>> diff --git a/server/display-channel.c b/server/display-channel.c
>> new file mode 100644
>> index 0000000..00be665
>> --- /dev/null
>> +++ b/server/display-channel.c
>> @@ -0,0 +1,38 @@
>> +/*
>> +   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
>> <http://www.gnu.org/licenses/>.
>> +*/
>> +#include "display-channel.h"
>> +
>> +DisplayChannelClient *dcc_new(DisplayChannel *display,
>> +                              RedClient *client, RedsStream *stream,
>> +                              int mig_target,
>> +                              uint32_t *common_caps, int num_common_caps,
>> +                              uint32_t *caps, int num_caps)
>> +{
>> +    DisplayChannelClient *dcc;
>> +
>> +    dcc = (DisplayChannelClient*)common_channel_new_client(
>> +        (CommonChannel *)display, sizeof(DisplayChannelClient),
>> +        client, stream, mig_target, TRUE,
>> +        common_caps, num_common_caps,
>> +        caps, num_caps);
>> +    spice_return_val_if_fail(dcc, NULL);
>> +
>> +    ring_init(&dcc->palette_cache_lru);
>> +    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
>> +
>> +    return dcc;
>> +}
>> diff --git a/server/display-channel.h b/server/display-channel.h
>> index e1ddc11..5d2eee5 100644
>> --- a/server/display-channel.h
>> +++ b/server/display-channel.h
>> @@ -15,14 +15,47 @@
>>     You should have received a copy of the GNU Lesser General Public
>>     License along with this library; if not, see
>>     <http://www.gnu.org/licenses/>.
>>  */
>> -#ifndef RED_WORKER_CLIENT_H_
>> -# define RED_WORKER_CLIENT_H_
>> +#ifndef DISPLAY_CHANNEL_H_
>> +# define DISPLAY_CHANNEL_H_
>> +
>> +#include <setjmp.h>
>>
>>  #include "red_worker.h"
>> +#include "reds_stream.h"
>>  #include "cache-item.h"
>>  #include "pixmap-cache.h"
>> +#ifdef USE_OPENGL
>> +#include "common/ogl_ctx.h"
>> +#include "reds_gl_canvas.h"
>> +#endif /* USE_OPENGL */
>> +#include "reds_sw_canvas.h"
>> +#include "glz_encoder_dictionary.h"
>> +#include "glz_encoder.h"
>> +#include "stat.h"
>> +#include "reds.h"
>> +#include "mjpeg_encoder.h"
>> +#include "red_memslots.h"
>> +#include "red_parse_qxl.h"
>> +#include "red_record_qxl.h"
>> +#include "jpeg_encoder.h"
>> +#ifdef USE_LZ4
>> +#include "lz4_encoder.h"
>> +#endif
>> +#include "demarshallers.h"
>> +#include "zlib_encoder.h"
>> +#include "red_channel.h"
>> +#include "red_dispatcher.h"
>> +#include "dispatcher.h"
>> +#include "main_channel.h"
>> +#include "migration_protocol.h"
>> +#include "main_dispatcher.h"
>> +#include "spice_server_utils.h"
>> +#include "spice_bitmap_utils.h"
>> +#include "spice_image_cache.h"
>>  #include "utils.h"
>>
>> +typedef struct DisplayChannel DisplayChannel;
>> +
>>  typedef struct Drawable Drawable;
>>
>>  #define PALETTE_CACHE_HASH_SHIFT 8
>> @@ -30,6 +63,8 @@ typedef struct Drawable Drawable;
>>  #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
>>  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
>>
>> +#define CLIENT_PALETTE_CACHE_SIZE 128
>> +
>>  /* Each drawable can refer to at most 3 images: src, brush and mask */
>>  #define MAX_DRAWABLE_PIXMAP_CACHE_ITEMS 3
>>
>> @@ -193,4 +228,13 @@ struct DisplayChannelClient {
>>      uint64_t streams_max_bit_rate;
>>  };
>>
>> -#endif /* RED_WORKER_CLIENT_H_ */
>> +DisplayChannelClient*      dcc_new
>> (DisplayChannel *display,
>> +
>> RedClient
>> *client,
>> +
>> RedsStream
>> *stream,
>> +                                                                      int
>> mig_target,
>> +
>> uint32_t
>> *common_caps,
>> +                                                                      int
>> num_common_caps,
>> +
>> uint32_t
>> *caps,
>> +                                                                      int
>> num_caps);
>> +
>> +#endif /* DISPLAY_CHANNEL_H_ */
>> diff --git a/server/red_worker.c b/server/red_worker.c
>> index 68cb153..89cb25c 100644
>> --- a/server/red_worker.c
>> +++ b/server/red_worker.c
>> @@ -43,7 +43,6 @@
>>  #include <poll.h>
>>  #include <pthread.h>
>>  #include <netinet/tcp.h>
>> -#include <setjmp.h>
>>  #include <openssl/ssl.h>
>>  #include <inttypes.h>
>>  #include <glib.h>
>> @@ -58,41 +57,11 @@
>>  #include "common/ring.h"
>>  #include "common/generated_server_marshallers.h"
>>
>> -#ifdef USE_OPENGL
>> -#include "common/ogl_ctx.h"
>> -#include "reds_gl_canvas.h"
>> -#endif /* USE_OPENGL */
>> +#include "display-channel.h"
>>
>>  #include "spice.h"
>>  #include "red_worker.h"
>> -#include "reds_stream.h"
>> -#include "reds_sw_canvas.h"
>> -#include "glz_encoder_dictionary.h"
>> -#include "glz_encoder.h"
>> -#include "stat.h"
>> -#include "reds.h"
>> -#include "mjpeg_encoder.h"
>> -#include "red_memslots.h"
>> -#include "red_parse_qxl.h"
>> -#include "red_record_qxl.h"
>> -#include "jpeg_encoder.h"
>> -#ifdef USE_LZ4
>> -#include "lz4_encoder.h"
>> -#endif
>> -#include "demarshallers.h"
>> -#include "zlib_encoder.h"
>> -#include "red_channel.h"
>> -#include "red_dispatcher.h"
>> -#include "dispatcher.h"
>> -#include "main_channel.h"
>> -#include "migration_protocol.h"
>>  #include "spice_timer_queue.h"
>> -#include "main_dispatcher.h"
>> -#include "spice_server_utils.h"
>> -#include "spice_bitmap_utils.h"
>> -#include "spice_image_cache.h"
>> -#include "pixmap-cache.h"
>> -#include "display-channel.h"
>>  #include "cursor-channel.h"
>>
>>  //#define COMPRESS_STAT
>> @@ -294,8 +263,6 @@ typedef struct StreamActivateReportItem {
>>  #define WIDE_CLIENT_ACK_WINDOW 40
>>  #define NARROW_CLIENT_ACK_WINDOW 20
>>
>> -#define CLIENT_PALETTE_CACHE_SIZE 128
>> -
>>  typedef struct ImageItem {
>>      PipeItem link;
>>      int refs;
>> @@ -311,8 +278,6 @@ typedef struct ImageItem {
>>      uint8_t data[0];
>>  } ImageItem;
>>
>> -typedef struct DisplayChannel DisplayChannel;
>> -
>>  enum {
>>      STREAM_FRAME_NONE,
>>      STREAM_FRAME_NATIVE,
>> @@ -9472,28 +9437,6 @@ CommonChannelClient
>> *common_channel_new_client(CommonChannel *common,
>>  }
>>
>>
>> -DisplayChannelClient *display_channel_client_create(CommonChannel *common,
>> -                                                    RedClient *client,
>> RedsStream *stream,
>> -                                                    int mig_target,
>> -                                                    uint32_t *common_caps,
>> int num_common_caps,
>> -                                                    uint32_t *caps, int
>> num_caps)
>> -{
>> -    DisplayChannelClient *dcc =
>> -        (DisplayChannelClient*)common_channel_new_client(
>> -            common, sizeof(DisplayChannelClient), client, stream,
>> -            mig_target,
>> -            TRUE,
>> -            common_caps, num_common_caps,
>> -            caps, num_caps);
>> -
>> -    if (!dcc) {
>> -        return NULL;
>> -    }
>> -    ring_init(&dcc->palette_cache_lru);
>> -    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
>> -    return dcc;
>> -}
>> -
>>  RedChannel *red_worker_new_channel(RedWorker *worker, int size,
>>                                     uint32_t channel_type, int
>>                                     migration_flags,
>>                                     ChannelCbs *channel_cbs,
>> @@ -9776,13 +9719,8 @@ static void handle_new_display_channel(RedWorker
>> *worker, RedClient *client, Red
>>      }
>>      display_channel = worker->display_channel;
>>      spice_info("add display channel client");
>> -    dcc = display_channel_client_create(&display_channel->common, client,
>> stream,
>> -                                        migrate,
>> -                                        common_caps, num_common_caps,
>> -                                        caps, num_caps);
>> -    if (!dcc) {
>> -        return;
>> -    }
>
> The removal of this test does not make sense looking at same test in
> display-channel.c. If can be NULL inside dcc_new why not checking here?
> Or are we just going to accept the core in next lines?
>
> I would add again these lines and ack it.

Same feeling here. common_channel_new_client() can return NULL inside
the dcc_new(), so the check is still valid.

ACK keeping the check.

>
>> +    dcc = dcc_new(display_channel, client, stream, migrate,
>> +                  common_caps, num_common_caps, caps, num_caps);
>>      spice_info("New display (client %p) dcc %p stream %p", client, dcc,
>>      stream);
>>      stream_buf_size = 32*1024;
>>      dcc->send_data.stream_outbuf = spice_malloc(stream_buf_size);
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> On Mon, Nov 2, 2015 at 11:02 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
> >
> >>
> >> From: Marc-André Lureau <marcandre.lureau@gmail.com>
> >>
> >> Move function from server/red_worker.c to new server/display-channel.c.
> >> ---
> >>  server/Makefile.am       |  1 +
> >>  server/display-channel.c | 38 +++++++++++++++++++++++++++
> >>  server/display-channel.h | 50 ++++++++++++++++++++++++++++++++---
> >>  server/red_worker.c      | 68
> >>  +++---------------------------------------------
> >>  4 files changed, 89 insertions(+), 68 deletions(-)
> >>  create mode 100644 server/display-channel.c
> >>
> >> diff --git a/server/Makefile.am b/server/Makefile.am
> >> index 87288cc..428417b 100644
> >> --- a/server/Makefile.am
> >> +++ b/server/Makefile.am
> >> @@ -105,6 +105,7 @@ libspice_server_la_SOURCES =                      \
> >>       red_parse_qxl.h                         \
> >>       red_worker.c                            \
> >>       red_worker.h                            \
> >> +     display-channel.c                       \
> >>       display-channel.h                       \
> >>       cursor-channel.c                        \
> >>       cursor-channel.h                        \
> >> diff --git a/server/display-channel.c b/server/display-channel.c
> >> new file mode 100644
> >> index 0000000..00be665
> >> --- /dev/null
> >> +++ b/server/display-channel.c
> >> @@ -0,0 +1,38 @@
> >> +/*
> >> +   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
> >> <http://www.gnu.org/licenses/>.
> >> +*/
> >> +#include "display-channel.h"
> >> +
> >> +DisplayChannelClient *dcc_new(DisplayChannel *display,
> >> +                              RedClient *client, RedsStream *stream,
> >> +                              int mig_target,
> >> +                              uint32_t *common_caps, int num_common_caps,
> >> +                              uint32_t *caps, int num_caps)
> >> +{
> >> +    DisplayChannelClient *dcc;
> >> +
> >> +    dcc = (DisplayChannelClient*)common_channel_new_client(
> >> +        (CommonChannel *)display, sizeof(DisplayChannelClient),
> >> +        client, stream, mig_target, TRUE,
> >> +        common_caps, num_common_caps,
> >> +        caps, num_caps);
> >> +    spice_return_val_if_fail(dcc, NULL);
> >> +
> >> +    ring_init(&dcc->palette_cache_lru);
> >> +    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
> >> +
> >> +    return dcc;
> >> +}
> >> diff --git a/server/display-channel.h b/server/display-channel.h
> >> index e1ddc11..5d2eee5 100644
> >> --- a/server/display-channel.h
> >> +++ b/server/display-channel.h
> >> @@ -15,14 +15,47 @@
> >>     You should have received a copy of the GNU Lesser General Public
> >>     License along with this library; if not, see
> >>     <http://www.gnu.org/licenses/>.
> >>  */
> >> -#ifndef RED_WORKER_CLIENT_H_
> >> -# define RED_WORKER_CLIENT_H_
> >> +#ifndef DISPLAY_CHANNEL_H_
> >> +# define DISPLAY_CHANNEL_H_
> >> +
> >> +#include <setjmp.h>
> >>
> >>  #include "red_worker.h"
> >> +#include "reds_stream.h"
> >>  #include "cache-item.h"
> >>  #include "pixmap-cache.h"
> >> +#ifdef USE_OPENGL
> >> +#include "common/ogl_ctx.h"
> >> +#include "reds_gl_canvas.h"
> >> +#endif /* USE_OPENGL */
> >> +#include "reds_sw_canvas.h"
> >> +#include "glz_encoder_dictionary.h"
> >> +#include "glz_encoder.h"
> >> +#include "stat.h"
> >> +#include "reds.h"
> >> +#include "mjpeg_encoder.h"
> >> +#include "red_memslots.h"
> >> +#include "red_parse_qxl.h"
> >> +#include "red_record_qxl.h"
> >> +#include "jpeg_encoder.h"
> >> +#ifdef USE_LZ4
> >> +#include "lz4_encoder.h"
> >> +#endif
> >> +#include "demarshallers.h"
> >> +#include "zlib_encoder.h"
> >> +#include "red_channel.h"
> >> +#include "red_dispatcher.h"
> >> +#include "dispatcher.h"
> >> +#include "main_channel.h"
> >> +#include "migration_protocol.h"
> >> +#include "main_dispatcher.h"
> >> +#include "spice_server_utils.h"
> >> +#include "spice_bitmap_utils.h"
> >> +#include "spice_image_cache.h"
> >>  #include "utils.h"
> >>
> >> +typedef struct DisplayChannel DisplayChannel;
> >> +
> >>  typedef struct Drawable Drawable;
> >>
> >>  #define PALETTE_CACHE_HASH_SHIFT 8
> >> @@ -30,6 +63,8 @@ typedef struct Drawable Drawable;
> >>  #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
> >>  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
> >>
> >> +#define CLIENT_PALETTE_CACHE_SIZE 128
> >> +
> >>  /* Each drawable can refer to at most 3 images: src, brush and mask */
> >>  #define MAX_DRAWABLE_PIXMAP_CACHE_ITEMS 3
> >>
> >> @@ -193,4 +228,13 @@ struct DisplayChannelClient {
> >>      uint64_t streams_max_bit_rate;
> >>  };
> >>
> >> -#endif /* RED_WORKER_CLIENT_H_ */
> >> +DisplayChannelClient*      dcc_new
> >> (DisplayChannel *display,
> >> +
> >> RedClient
> >> *client,
> >> +
> >> RedsStream
> >> *stream,
> >> +                                                                      int
> >> mig_target,
> >> +
> >> uint32_t
> >> *common_caps,
> >> +                                                                      int
> >> num_common_caps,
> >> +
> >> uint32_t
> >> *caps,
> >> +                                                                      int
> >> num_caps);
> >> +
> >> +#endif /* DISPLAY_CHANNEL_H_ */
> >> diff --git a/server/red_worker.c b/server/red_worker.c
> >> index 68cb153..89cb25c 100644
> >> --- a/server/red_worker.c
> >> +++ b/server/red_worker.c
> >> @@ -43,7 +43,6 @@
> >>  #include <poll.h>
> >>  #include <pthread.h>
> >>  #include <netinet/tcp.h>
> >> -#include <setjmp.h>
> >>  #include <openssl/ssl.h>
> >>  #include <inttypes.h>
> >>  #include <glib.h>
> >> @@ -58,41 +57,11 @@
> >>  #include "common/ring.h"
> >>  #include "common/generated_server_marshallers.h"
> >>
> >> -#ifdef USE_OPENGL
> >> -#include "common/ogl_ctx.h"
> >> -#include "reds_gl_canvas.h"
> >> -#endif /* USE_OPENGL */
> >> +#include "display-channel.h"
> >>
> >>  #include "spice.h"
> >>  #include "red_worker.h"
> >> -#include "reds_stream.h"
> >> -#include "reds_sw_canvas.h"
> >> -#include "glz_encoder_dictionary.h"
> >> -#include "glz_encoder.h"
> >> -#include "stat.h"
> >> -#include "reds.h"
> >> -#include "mjpeg_encoder.h"
> >> -#include "red_memslots.h"
> >> -#include "red_parse_qxl.h"
> >> -#include "red_record_qxl.h"
> >> -#include "jpeg_encoder.h"
> >> -#ifdef USE_LZ4
> >> -#include "lz4_encoder.h"
> >> -#endif
> >> -#include "demarshallers.h"
> >> -#include "zlib_encoder.h"
> >> -#include "red_channel.h"
> >> -#include "red_dispatcher.h"
> >> -#include "dispatcher.h"
> >> -#include "main_channel.h"
> >> -#include "migration_protocol.h"
> >>  #include "spice_timer_queue.h"
> >> -#include "main_dispatcher.h"
> >> -#include "spice_server_utils.h"
> >> -#include "spice_bitmap_utils.h"
> >> -#include "spice_image_cache.h"
> >> -#include "pixmap-cache.h"
> >> -#include "display-channel.h"
> >>  #include "cursor-channel.h"
> >>
> >>  //#define COMPRESS_STAT
> >> @@ -294,8 +263,6 @@ typedef struct StreamActivateReportItem {
> >>  #define WIDE_CLIENT_ACK_WINDOW 40
> >>  #define NARROW_CLIENT_ACK_WINDOW 20
> >>
> >> -#define CLIENT_PALETTE_CACHE_SIZE 128
> >> -
> >>  typedef struct ImageItem {
> >>      PipeItem link;
> >>      int refs;
> >> @@ -311,8 +278,6 @@ typedef struct ImageItem {
> >>      uint8_t data[0];
> >>  } ImageItem;
> >>
> >> -typedef struct DisplayChannel DisplayChannel;
> >> -
> >>  enum {
> >>      STREAM_FRAME_NONE,
> >>      STREAM_FRAME_NATIVE,
> >> @@ -9472,28 +9437,6 @@ CommonChannelClient
> >> *common_channel_new_client(CommonChannel *common,
> >>  }
> >>
> >>
> >> -DisplayChannelClient *display_channel_client_create(CommonChannel
> >> *common,
> >> -                                                    RedClient *client,
> >> RedsStream *stream,
> >> -                                                    int mig_target,
> >> -                                                    uint32_t
> >> *common_caps,
> >> int num_common_caps,
> >> -                                                    uint32_t *caps, int
> >> num_caps)
> >> -{
> >> -    DisplayChannelClient *dcc =
> >> -        (DisplayChannelClient*)common_channel_new_client(
> >> -            common, sizeof(DisplayChannelClient), client, stream,
> >> -            mig_target,
> >> -            TRUE,
> >> -            common_caps, num_common_caps,
> >> -            caps, num_caps);
> >> -
> >> -    if (!dcc) {
> >> -        return NULL;
> >> -    }
> >> -    ring_init(&dcc->palette_cache_lru);
> >> -    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
> >> -    return dcc;
> >> -}
> >> -
> >>  RedChannel *red_worker_new_channel(RedWorker *worker, int size,
> >>                                     uint32_t channel_type, int
> >>                                     migration_flags,
> >>                                     ChannelCbs *channel_cbs,
> >> @@ -9776,13 +9719,8 @@ static void handle_new_display_channel(RedWorker
> >> *worker, RedClient *client, Red
> >>      }
> >>      display_channel = worker->display_channel;
> >>      spice_info("add display channel client");
> >> -    dcc = display_channel_client_create(&display_channel->common, client,
> >> stream,
> >> -                                        migrate,
> >> -                                        common_caps, num_common_caps,
> >> -                                        caps, num_caps);
> >> -    if (!dcc) {
> >> -        return;
> >> -    }
> >
> > The removal of this test does not make sense looking at same test in
> > display-channel.c. If can be NULL inside dcc_new why not checking here?
> > Or are we just going to accept the core in next lines?
> >
> > I would add again these lines and ack it.
> 
> Same feeling here. common_channel_new_client() can return NULL inside
> the dcc_new(), so the check is still valid.
> 
> ACK keeping the check.
> 
> >
> >> +    dcc = dcc_new(display_channel, client, stream, migrate,
> >> +                  common_caps, num_common_caps, caps, num_caps);
> >>      spice_info("New display (client %p) dcc %p stream %p", client, dcc,
> >>      stream);
> >>      stream_buf_size = 32*1024;
> >>      dcc->send_data.stream_outbuf = spice_malloc(stream_buf_size);
> >

Pushed.

Frediano
On Mon, Nov 2, 2015 at 2:50 PM, Fabiano Fidêncio <fidencio@redhat.com> wrote:
> On Mon, Nov 2, 2015 at 11:02 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
>>
>>>
>>> From: Marc-André Lureau <marcandre.lureau@gmail.com>
>>>
>>> Move function from server/red_worker.c to new server/display-channel.c.
>>> ---
>>>  server/Makefile.am       |  1 +
>>>  server/display-channel.c | 38 +++++++++++++++++++++++++++
>>>  server/display-channel.h | 50 ++++++++++++++++++++++++++++++++---
>>>  server/red_worker.c      | 68
>>>  +++---------------------------------------------
>>>  4 files changed, 89 insertions(+), 68 deletions(-)
>>>  create mode 100644 server/display-channel.c
>>>
>>> diff --git a/server/Makefile.am b/server/Makefile.am
>>> index 87288cc..428417b 100644
>>> --- a/server/Makefile.am
>>> +++ b/server/Makefile.am
>>> @@ -105,6 +105,7 @@ libspice_server_la_SOURCES =                      \
>>>       red_parse_qxl.h                         \
>>>       red_worker.c                            \
>>>       red_worker.h                            \
>>> +     display-channel.c                       \
>>>       display-channel.h                       \
>>>       cursor-channel.c                        \
>>>       cursor-channel.h                        \
>>> diff --git a/server/display-channel.c b/server/display-channel.c
>>> new file mode 100644
>>> index 0000000..00be665
>>> --- /dev/null
>>> +++ b/server/display-channel.c
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> +   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
>>> <http://www.gnu.org/licenses/>.
>>> +*/
>>> +#include "display-channel.h"
>>> +
>>> +DisplayChannelClient *dcc_new(DisplayChannel *display,
>>> +                              RedClient *client, RedsStream *stream,
>>> +                              int mig_target,
>>> +                              uint32_t *common_caps, int num_common_caps,
>>> +                              uint32_t *caps, int num_caps)
>>> +{
>>> +    DisplayChannelClient *dcc;
>>> +
>>> +    dcc = (DisplayChannelClient*)common_channel_new_client(
>>> +        (CommonChannel *)display, sizeof(DisplayChannelClient),
>>> +        client, stream, mig_target, TRUE,
>>> +        common_caps, num_common_caps,
>>> +        caps, num_caps);
>>> +    spice_return_val_if_fail(dcc, NULL);
>>> +
>>> +    ring_init(&dcc->palette_cache_lru);
>>> +    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
>>> +
>>> +    return dcc;
>>> +}
>>> diff --git a/server/display-channel.h b/server/display-channel.h
>>> index e1ddc11..5d2eee5 100644
>>> --- a/server/display-channel.h
>>> +++ b/server/display-channel.h
>>> @@ -15,14 +15,47 @@
>>>     You should have received a copy of the GNU Lesser General Public
>>>     License along with this library; if not, see
>>>     <http://www.gnu.org/licenses/>.
>>>  */
>>> -#ifndef RED_WORKER_CLIENT_H_
>>> -# define RED_WORKER_CLIENT_H_
>>> +#ifndef DISPLAY_CHANNEL_H_
>>> +# define DISPLAY_CHANNEL_H_
>>> +
>>> +#include <setjmp.h>
>>>
>>>  #include "red_worker.h"
>>> +#include "reds_stream.h"
>>>  #include "cache-item.h"
>>>  #include "pixmap-cache.h"
>>> +#ifdef USE_OPENGL
>>> +#include "common/ogl_ctx.h"
>>> +#include "reds_gl_canvas.h"
>>> +#endif /* USE_OPENGL */
>>> +#include "reds_sw_canvas.h"
>>> +#include "glz_encoder_dictionary.h"
>>> +#include "glz_encoder.h"
>>> +#include "stat.h"
>>> +#include "reds.h"
>>> +#include "mjpeg_encoder.h"
>>> +#include "red_memslots.h"
>>> +#include "red_parse_qxl.h"
>>> +#include "red_record_qxl.h"
>>> +#include "jpeg_encoder.h"
>>> +#ifdef USE_LZ4
>>> +#include "lz4_encoder.h"
>>> +#endif
>>> +#include "demarshallers.h"
>>> +#include "zlib_encoder.h"
>>> +#include "red_channel.h"
>>> +#include "red_dispatcher.h"
>>> +#include "dispatcher.h"
>>> +#include "main_channel.h"
>>> +#include "migration_protocol.h"
>>> +#include "main_dispatcher.h"
>>> +#include "spice_server_utils.h"
>>> +#include "spice_bitmap_utils.h"
>>> +#include "spice_image_cache.h"
>>>  #include "utils.h"
>>>
>>> +typedef struct DisplayChannel DisplayChannel;
>>> +
>>>  typedef struct Drawable Drawable;
>>>
>>>  #define PALETTE_CACHE_HASH_SHIFT 8
>>> @@ -30,6 +63,8 @@ typedef struct Drawable Drawable;
>>>  #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
>>>  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
>>>
>>> +#define CLIENT_PALETTE_CACHE_SIZE 128
>>> +
>>>  /* Each drawable can refer to at most 3 images: src, brush and mask */
>>>  #define MAX_DRAWABLE_PIXMAP_CACHE_ITEMS 3
>>>
>>> @@ -193,4 +228,13 @@ struct DisplayChannelClient {
>>>      uint64_t streams_max_bit_rate;
>>>  };
>>>
>>> -#endif /* RED_WORKER_CLIENT_H_ */
>>> +DisplayChannelClient*      dcc_new
>>> (DisplayChannel *display,
>>> +
>>> RedClient
>>> *client,
>>> +
>>> RedsStream
>>> *stream,
>>> +                                                                      int
>>> mig_target,
>>> +
>>> uint32_t
>>> *common_caps,
>>> +                                                                      int
>>> num_common_caps,
>>> +
>>> uint32_t
>>> *caps,
>>> +                                                                      int
>>> num_caps);
>>> +
>>> +#endif /* DISPLAY_CHANNEL_H_ */
>>> diff --git a/server/red_worker.c b/server/red_worker.c
>>> index 68cb153..89cb25c 100644
>>> --- a/server/red_worker.c
>>> +++ b/server/red_worker.c
>>> @@ -43,7 +43,6 @@
>>>  #include <poll.h>
>>>  #include <pthread.h>
>>>  #include <netinet/tcp.h>
>>> -#include <setjmp.h>
>>>  #include <openssl/ssl.h>
>>>  #include <inttypes.h>
>>>  #include <glib.h>
>>> @@ -58,41 +57,11 @@
>>>  #include "common/ring.h"
>>>  #include "common/generated_server_marshallers.h"
>>>
>>> -#ifdef USE_OPENGL
>>> -#include "common/ogl_ctx.h"
>>> -#include "reds_gl_canvas.h"
>>> -#endif /* USE_OPENGL */
>>> +#include "display-channel.h"
>>>
>>>  #include "spice.h"
>>>  #include "red_worker.h"
>>> -#include "reds_stream.h"
>>> -#include "reds_sw_canvas.h"
>>> -#include "glz_encoder_dictionary.h"
>>> -#include "glz_encoder.h"
>>> -#include "stat.h"
>>> -#include "reds.h"
>>> -#include "mjpeg_encoder.h"
>>> -#include "red_memslots.h"
>>> -#include "red_parse_qxl.h"
>>> -#include "red_record_qxl.h"
>>> -#include "jpeg_encoder.h"
>>> -#ifdef USE_LZ4
>>> -#include "lz4_encoder.h"
>>> -#endif
>>> -#include "demarshallers.h"
>>> -#include "zlib_encoder.h"
>>> -#include "red_channel.h"
>>> -#include "red_dispatcher.h"
>>> -#include "dispatcher.h"
>>> -#include "main_channel.h"
>>> -#include "migration_protocol.h"
>>>  #include "spice_timer_queue.h"
>>> -#include "main_dispatcher.h"
>>> -#include "spice_server_utils.h"
>>> -#include "spice_bitmap_utils.h"
>>> -#include "spice_image_cache.h"
>>> -#include "pixmap-cache.h"
>>> -#include "display-channel.h"
>>>  #include "cursor-channel.h"
>>>
>>>  //#define COMPRESS_STAT
>>> @@ -294,8 +263,6 @@ typedef struct StreamActivateReportItem {
>>>  #define WIDE_CLIENT_ACK_WINDOW 40
>>>  #define NARROW_CLIENT_ACK_WINDOW 20
>>>
>>> -#define CLIENT_PALETTE_CACHE_SIZE 128
>>> -
>>>  typedef struct ImageItem {
>>>      PipeItem link;
>>>      int refs;
>>> @@ -311,8 +278,6 @@ typedef struct ImageItem {
>>>      uint8_t data[0];
>>>  } ImageItem;
>>>
>>> -typedef struct DisplayChannel DisplayChannel;
>>> -
>>>  enum {
>>>      STREAM_FRAME_NONE,
>>>      STREAM_FRAME_NATIVE,
>>> @@ -9472,28 +9437,6 @@ CommonChannelClient
>>> *common_channel_new_client(CommonChannel *common,
>>>  }
>>>
>>>
>>> -DisplayChannelClient *display_channel_client_create(CommonChannel *common,
>>> -                                                    RedClient *client,
>>> RedsStream *stream,
>>> -                                                    int mig_target,
>>> -                                                    uint32_t *common_caps,
>>> int num_common_caps,
>>> -                                                    uint32_t *caps, int
>>> num_caps)
>>> -{
>>> -    DisplayChannelClient *dcc =
>>> -        (DisplayChannelClient*)common_channel_new_client(
>>> -            common, sizeof(DisplayChannelClient), client, stream,
>>> -            mig_target,
>>> -            TRUE,
>>> -            common_caps, num_common_caps,
>>> -            caps, num_caps);
>>> -
>>> -    if (!dcc) {
>>> -        return NULL;
>>> -    }
>>> -    ring_init(&dcc->palette_cache_lru);
>>> -    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
>>> -    return dcc;
>>> -}
>>> -
>>>  RedChannel *red_worker_new_channel(RedWorker *worker, int size,
>>>                                     uint32_t channel_type, int
>>>                                     migration_flags,
>>>                                     ChannelCbs *channel_cbs,
>>> @@ -9776,13 +9719,8 @@ static void handle_new_display_channel(RedWorker
>>> *worker, RedClient *client, Red
>>>      }
>>>      display_channel = worker->display_channel;
>>>      spice_info("add display channel client");
>>> -    dcc = display_channel_client_create(&display_channel->common, client,
>>> stream,
>>> -                                        migrate,
>>> -                                        common_caps, num_common_caps,
>>> -                                        caps, num_caps);
>>> -    if (!dcc) {
>>> -        return;
>>> -    }
>>
>> The removal of this test does not make sense looking at same test in
>> display-channel.c. If can be NULL inside dcc_new why not checking here?
>> Or are we just going to accept the core in next lines?
>>
>> I would add again these lines and ack it.
>
> Same feeling here. common_channel_new_client() can return NULL inside
> the dcc_new(), so the check is still valid.
>
> ACK keeping the check.

Getting the ACK back. Would be better to have these whole series
rebased to the git master.

>
>>
>>> +    dcc = dcc_new(display_channel, client, stream, migrate,
>>> +                  common_caps, num_common_caps, caps, num_caps);
>>>      spice_info("New display (client %p) dcc %p stream %p", client, dcc,
>>>      stream);
>>>      stream_buf_size = 32*1024;
>>>      dcc->send_data.stream_outbuf = spice_malloc(stream_buf_size);
>>
>> Frediano
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Mon, Nov 2, 2015 at 3:14 PM, Fabiano Fidêncio <fabiano@fidencio.org> wrote:
> On Mon, Nov 2, 2015 at 2:50 PM, Fabiano Fidêncio <fidencio@redhat.com> wrote:
>> On Mon, Nov 2, 2015 at 11:02 AM, Frediano Ziglio <fziglio@redhat.com> wrote:
>>>
>>>>
>>>> From: Marc-André Lureau <marcandre.lureau@gmail.com>
>>>>
>>>> Move function from server/red_worker.c to new server/display-channel.c.
>>>> ---
>>>>  server/Makefile.am       |  1 +
>>>>  server/display-channel.c | 38 +++++++++++++++++++++++++++
>>>>  server/display-channel.h | 50 ++++++++++++++++++++++++++++++++---
>>>>  server/red_worker.c      | 68
>>>>  +++---------------------------------------------
>>>>  4 files changed, 89 insertions(+), 68 deletions(-)
>>>>  create mode 100644 server/display-channel.c
>>>>
>>>> diff --git a/server/Makefile.am b/server/Makefile.am
>>>> index 87288cc..428417b 100644
>>>> --- a/server/Makefile.am
>>>> +++ b/server/Makefile.am
>>>> @@ -105,6 +105,7 @@ libspice_server_la_SOURCES =                      \
>>>>       red_parse_qxl.h                         \
>>>>       red_worker.c                            \
>>>>       red_worker.h                            \
>>>> +     display-channel.c                       \
>>>>       display-channel.h                       \
>>>>       cursor-channel.c                        \
>>>>       cursor-channel.h                        \
>>>> diff --git a/server/display-channel.c b/server/display-channel.c
>>>> new file mode 100644
>>>> index 0000000..00be665
>>>> --- /dev/null
>>>> +++ b/server/display-channel.c
>>>> @@ -0,0 +1,38 @@
>>>> +/*
>>>> +   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
>>>> <http://www.gnu.org/licenses/>.
>>>> +*/
>>>> +#include "display-channel.h"
>>>> +
>>>> +DisplayChannelClient *dcc_new(DisplayChannel *display,
>>>> +                              RedClient *client, RedsStream *stream,
>>>> +                              int mig_target,
>>>> +                              uint32_t *common_caps, int num_common_caps,
>>>> +                              uint32_t *caps, int num_caps)
>>>> +{
>>>> +    DisplayChannelClient *dcc;
>>>> +
>>>> +    dcc = (DisplayChannelClient*)common_channel_new_client(
>>>> +        (CommonChannel *)display, sizeof(DisplayChannelClient),
>>>> +        client, stream, mig_target, TRUE,
>>>> +        common_caps, num_common_caps,
>>>> +        caps, num_caps);
>>>> +    spice_return_val_if_fail(dcc, NULL);
>>>> +
>>>> +    ring_init(&dcc->palette_cache_lru);
>>>> +    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
>>>> +
>>>> +    return dcc;
>>>> +}
>>>> diff --git a/server/display-channel.h b/server/display-channel.h
>>>> index e1ddc11..5d2eee5 100644
>>>> --- a/server/display-channel.h
>>>> +++ b/server/display-channel.h
>>>> @@ -15,14 +15,47 @@
>>>>     You should have received a copy of the GNU Lesser General Public
>>>>     License along with this library; if not, see
>>>>     <http://www.gnu.org/licenses/>.
>>>>  */
>>>> -#ifndef RED_WORKER_CLIENT_H_
>>>> -# define RED_WORKER_CLIENT_H_
>>>> +#ifndef DISPLAY_CHANNEL_H_
>>>> +# define DISPLAY_CHANNEL_H_
>>>> +
>>>> +#include <setjmp.h>
>>>>
>>>>  #include "red_worker.h"
>>>> +#include "reds_stream.h"
>>>>  #include "cache-item.h"
>>>>  #include "pixmap-cache.h"
>>>> +#ifdef USE_OPENGL
>>>> +#include "common/ogl_ctx.h"
>>>> +#include "reds_gl_canvas.h"
>>>> +#endif /* USE_OPENGL */
>>>> +#include "reds_sw_canvas.h"
>>>> +#include "glz_encoder_dictionary.h"
>>>> +#include "glz_encoder.h"
>>>> +#include "stat.h"
>>>> +#include "reds.h"
>>>> +#include "mjpeg_encoder.h"
>>>> +#include "red_memslots.h"
>>>> +#include "red_parse_qxl.h"
>>>> +#include "red_record_qxl.h"
>>>> +#include "jpeg_encoder.h"
>>>> +#ifdef USE_LZ4
>>>> +#include "lz4_encoder.h"
>>>> +#endif
>>>> +#include "demarshallers.h"
>>>> +#include "zlib_encoder.h"
>>>> +#include "red_channel.h"
>>>> +#include "red_dispatcher.h"
>>>> +#include "dispatcher.h"
>>>> +#include "main_channel.h"
>>>> +#include "migration_protocol.h"
>>>> +#include "main_dispatcher.h"
>>>> +#include "spice_server_utils.h"
>>>> +#include "spice_bitmap_utils.h"
>>>> +#include "spice_image_cache.h"
>>>>  #include "utils.h"
>>>>
>>>> +typedef struct DisplayChannel DisplayChannel;
>>>> +
>>>>  typedef struct Drawable Drawable;
>>>>
>>>>  #define PALETTE_CACHE_HASH_SHIFT 8
>>>> @@ -30,6 +63,8 @@ typedef struct Drawable Drawable;
>>>>  #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
>>>>  #define PALETTE_CACHE_HASH_KEY(id) ((id) & PALETTE_CACHE_HASH_MASK)
>>>>
>>>> +#define CLIENT_PALETTE_CACHE_SIZE 128
>>>> +
>>>>  /* Each drawable can refer to at most 3 images: src, brush and mask */
>>>>  #define MAX_DRAWABLE_PIXMAP_CACHE_ITEMS 3
>>>>
>>>> @@ -193,4 +228,13 @@ struct DisplayChannelClient {
>>>>      uint64_t streams_max_bit_rate;
>>>>  };
>>>>
>>>> -#endif /* RED_WORKER_CLIENT_H_ */
>>>> +DisplayChannelClient*      dcc_new
>>>> (DisplayChannel *display,
>>>> +
>>>> RedClient
>>>> *client,
>>>> +
>>>> RedsStream
>>>> *stream,
>>>> +                                                                      int
>>>> mig_target,
>>>> +
>>>> uint32_t
>>>> *common_caps,
>>>> +                                                                      int
>>>> num_common_caps,
>>>> +
>>>> uint32_t
>>>> *caps,
>>>> +                                                                      int
>>>> num_caps);
>>>> +
>>>> +#endif /* DISPLAY_CHANNEL_H_ */
>>>> diff --git a/server/red_worker.c b/server/red_worker.c
>>>> index 68cb153..89cb25c 100644
>>>> --- a/server/red_worker.c
>>>> +++ b/server/red_worker.c
>>>> @@ -43,7 +43,6 @@
>>>>  #include <poll.h>
>>>>  #include <pthread.h>
>>>>  #include <netinet/tcp.h>
>>>> -#include <setjmp.h>
>>>>  #include <openssl/ssl.h>
>>>>  #include <inttypes.h>
>>>>  #include <glib.h>
>>>> @@ -58,41 +57,11 @@
>>>>  #include "common/ring.h"
>>>>  #include "common/generated_server_marshallers.h"
>>>>
>>>> -#ifdef USE_OPENGL
>>>> -#include "common/ogl_ctx.h"
>>>> -#include "reds_gl_canvas.h"
>>>> -#endif /* USE_OPENGL */
>>>> +#include "display-channel.h"
>>>>
>>>>  #include "spice.h"
>>>>  #include "red_worker.h"
>>>> -#include "reds_stream.h"
>>>> -#include "reds_sw_canvas.h"
>>>> -#include "glz_encoder_dictionary.h"
>>>> -#include "glz_encoder.h"
>>>> -#include "stat.h"
>>>> -#include "reds.h"
>>>> -#include "mjpeg_encoder.h"
>>>> -#include "red_memslots.h"
>>>> -#include "red_parse_qxl.h"
>>>> -#include "red_record_qxl.h"
>>>> -#include "jpeg_encoder.h"
>>>> -#ifdef USE_LZ4
>>>> -#include "lz4_encoder.h"
>>>> -#endif
>>>> -#include "demarshallers.h"
>>>> -#include "zlib_encoder.h"
>>>> -#include "red_channel.h"
>>>> -#include "red_dispatcher.h"
>>>> -#include "dispatcher.h"
>>>> -#include "main_channel.h"
>>>> -#include "migration_protocol.h"
>>>>  #include "spice_timer_queue.h"
>>>> -#include "main_dispatcher.h"
>>>> -#include "spice_server_utils.h"
>>>> -#include "spice_bitmap_utils.h"
>>>> -#include "spice_image_cache.h"
>>>> -#include "pixmap-cache.h"
>>>> -#include "display-channel.h"
>>>>  #include "cursor-channel.h"
>>>>
>>>>  //#define COMPRESS_STAT
>>>> @@ -294,8 +263,6 @@ typedef struct StreamActivateReportItem {
>>>>  #define WIDE_CLIENT_ACK_WINDOW 40
>>>>  #define NARROW_CLIENT_ACK_WINDOW 20
>>>>
>>>> -#define CLIENT_PALETTE_CACHE_SIZE 128
>>>> -
>>>>  typedef struct ImageItem {
>>>>      PipeItem link;
>>>>      int refs;
>>>> @@ -311,8 +278,6 @@ typedef struct ImageItem {
>>>>      uint8_t data[0];
>>>>  } ImageItem;
>>>>
>>>> -typedef struct DisplayChannel DisplayChannel;
>>>> -
>>>>  enum {
>>>>      STREAM_FRAME_NONE,
>>>>      STREAM_FRAME_NATIVE,
>>>> @@ -9472,28 +9437,6 @@ CommonChannelClient
>>>> *common_channel_new_client(CommonChannel *common,
>>>>  }
>>>>
>>>>
>>>> -DisplayChannelClient *display_channel_client_create(CommonChannel *common,
>>>> -                                                    RedClient *client,
>>>> RedsStream *stream,
>>>> -                                                    int mig_target,
>>>> -                                                    uint32_t *common_caps,
>>>> int num_common_caps,
>>>> -                                                    uint32_t *caps, int
>>>> num_caps)
>>>> -{
>>>> -    DisplayChannelClient *dcc =
>>>> -        (DisplayChannelClient*)common_channel_new_client(
>>>> -            common, sizeof(DisplayChannelClient), client, stream,
>>>> -            mig_target,
>>>> -            TRUE,
>>>> -            common_caps, num_common_caps,
>>>> -            caps, num_caps);
>>>> -
>>>> -    if (!dcc) {
>>>> -        return NULL;
>>>> -    }
>>>> -    ring_init(&dcc->palette_cache_lru);
>>>> -    dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
>>>> -    return dcc;
>>>> -}
>>>> -
>>>>  RedChannel *red_worker_new_channel(RedWorker *worker, int size,
>>>>                                     uint32_t channel_type, int
>>>>                                     migration_flags,
>>>>                                     ChannelCbs *channel_cbs,
>>>> @@ -9776,13 +9719,8 @@ static void handle_new_display_channel(RedWorker
>>>> *worker, RedClient *client, Red
>>>>      }
>>>>      display_channel = worker->display_channel;
>>>>      spice_info("add display channel client");
>>>> -    dcc = display_channel_client_create(&display_channel->common, client,
>>>> stream,
>>>> -                                        migrate,
>>>> -                                        common_caps, num_common_caps,
>>>> -                                        caps, num_caps);
>>>> -    if (!dcc) {
>>>> -        return;
>>>> -    }
>>>
>>> The removal of this test does not make sense looking at same test in
>>> display-channel.c. If can be NULL inside dcc_new why not checking here?
>>> Or are we just going to accept the core in next lines?
>>>
>>> I would add again these lines and ack it.
>>
>> Same feeling here. common_channel_new_client() can return NULL inside
>> the dcc_new(), so the check is still valid.
>>
>> ACK keeping the check.
>
> Getting the ACK back. Would be better to have these whole series
> rebased to the git master.

Okay, too late :-\

>
>>
>>>
>>>> +    dcc = dcc_new(display_channel, client, stream, migrate,
>>>> +                  common_caps, num_common_caps, caps, num_caps);
>>>>      spice_info("New display (client %p) dcc %p stream %p", client, dcc,
>>>>      stream);
>>>>      stream_buf_size = 32*1024;
>>>>      dcc->send_data.stream_outbuf = spice_malloc(stream_buf_size);
>>>
>>> Frediano
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Fabiano Fidêncio