Re: [PATCH 03/11] worker: move compression parameters to dcc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> [updated for the preferred compression]
> Signed-off-by: Pavel Grunt <pgrunt@xxxxxxxxxx>
> ---
>  server/display-channel.c |  9 ++++++++-
>  server/display-channel.h |  8 +++++++-
>  server/red_worker.c      | 32 ++++++++++++++++----------------
>  3 files changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 3b5a246..1d5d8d3 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -124,7 +124,11 @@ 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)
> +                              uint32_t *caps, int num_caps,
> +                              SpiceImageCompression image_compression,
> +                              spice_wan_compression_t jpeg_state,
> +                              spice_wan_compression_t zlib_glz_state)
> +
>  {
>      DisplayChannelClient *dcc;
>  

Style note (more for future consideration):
  these 3 parameters are more or less together. Would be worth having them
inside a structure to you could use


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)
                              uint32_t *caps, int num_caps,
                              ImageCompressSettings compress_settings)

and ...

> @@ -137,6 +141,9 @@ DisplayChannelClient *dcc_new(DisplayChannel *display,
>  
>      ring_init(&dcc->palette_cache_lru);
>      dcc->palette_cache_available = CLIENT_PALETTE_CACHE_SIZE;
> +    dcc->image_compression = image_compression;
> +    dcc->jpeg_state = jpeg_state;
> +    dcc->zlib_glz_state = zlib_glz_state;
>  
>      return dcc;
>  }

dcc->compress_settings = compress_settings;

??

> diff --git a/server/display-channel.h b/server/display-channel.h
> index cae0343..edbd4b9 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -168,6 +168,9 @@ struct Drawable {
>  
>  struct DisplayChannelClient {
>      CommonChannelClient common;
> +    SpiceImageCompression image_compression;
> +    spice_wan_compression_t jpeg_state;
> +    spice_wan_compression_t zlib_glz_state;
>  
>      int expect_init;
>  
> @@ -240,7 +243,10 @@ DisplayChannelClient*      dcc_new
> (DisplayCha
>                                                                        uint32_t
>                                                                        *common_caps,
>                                                                        int
>                                                                        num_common_caps,
>                                                                        uint32_t
>                                                                        *caps,
> -                                                                      int
> num_caps);
> +                                                                      int
> num_caps,
> +
> SpiceImageCompression
> image_compression,
> +
> spice_wan_compression_t
> jpeg_state,
> +
> spice_wan_compression_t
> zlib_glz_state);
>  void                       dcc_push_monitors_config
>  (DisplayChannelClient *dcc);
>  void                       dcc_push_destroy_surface
>  (DisplayChannelClient *dcc,
>                                                                        uint32_t
>                                                                        surface_id);
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 608229c..5a27879 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -4406,8 +4406,7 @@ static inline int
> red_compress_image(DisplayChannelClient *dcc,
>                                       compress_send_data_t* o_comp_data)
>  {
>      DisplayChannel *display_channel = DCC_TO_DC(dcc);
> -    SpiceImageCompression image_compression =
> -        display_channel->common.worker->image_compression;
> +    SpiceImageCompression image_compression = dcc->image_compression;
>      int quic_compress = FALSE;
>  
>      if ((image_compression == SPICE_IMAGE_COMPRESSION_OFF) ||
> @@ -4811,15 +4810,14 @@ static void fill_mask(RedChannelClient *rcc,
> SpiceMarshaller *m,
>                        SpiceImage *mask_bitmap, Drawable *drawable)
>  {
>      DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
> -    DisplayChannel *display = DCC_TO_DC(dcc);
>  
>      if (mask_bitmap && m) {
> -        if (display->common.worker->image_compression !=
> SPICE_IMAGE_COMPRESSION_OFF) {
> -            SpiceImageCompression save_img_comp =
> -                display->common.worker->image_compression;
> -            display->common.worker->image_compression =
> SPICE_IMAGE_COMPRESSION_OFF;
> +        if (dcc->image_compression != SPICE_IMAGE_COMPRESSION_OFF) {
> +            /* todo: pass compression argument */
> +            SpiceImageCompression save_img_comp = dcc->image_compression;
> +            dcc->image_compression = SPICE_IMAGE_COMPRESSION_OFF;
>              fill_bits(dcc, m, mask_bitmap, drawable, FALSE);
> -            display->common.worker->image_compression = save_img_comp;
> +            dcc->image_compression = save_img_comp;
>          } else {
>              fill_bits(dcc, m, mask_bitmap, drawable, FALSE);
>          }
> @@ -6706,7 +6704,7 @@ static void red_marshall_image(RedChannelClient *rcc,
> SpiceMarshaller *m, ImageI
>  
>      compress_send_data_t comp_send_data = {0};
>  
> -    comp_mode = display_channel->common.worker->image_compression;
> +    comp_mode = dcc->image_compression;
>  
>      if (((comp_mode == SPICE_IMAGE_COMPRESSION_AUTO_LZ) ||
>          (comp_mode == SPICE_IMAGE_COMPRESSION_AUTO_GLZ)) &&
>          !bitmap_has_extra_stride(&bitmap)) {
> @@ -7808,10 +7806,10 @@ static int
> display_channel_handle_migrate_data(RedChannelClient *rcc, uint32_t s
>  
>      if (migrate_data->low_bandwidth_setting) {
>          red_channel_client_ack_set_client_window(rcc,
>          WIDE_CLIENT_ACK_WINDOW);
> -        if (DCC_TO_WORKER(dcc)->jpeg_state == SPICE_WAN_COMPRESSION_AUTO) {
> +        if (dcc->jpeg_state == SPICE_WAN_COMPRESSION_AUTO) {
>              display_channel->enable_jpeg = TRUE;
>          }
> -        if (DCC_TO_WORKER(dcc)->zlib_glz_state ==
> SPICE_WAN_COMPRESSION_AUTO) {
> +        if (dcc->zlib_glz_state == SPICE_WAN_COMPRESSION_AUTO) {
>              display_channel->enable_zlib_glz_wrap = TRUE;
>          }
>      }
> @@ -7879,6 +7877,7 @@ static int
> display_channel_handle_preferred_compression(DisplayChannelClient *dc
>      case SPICE_IMAGE_COMPRESSION_GLZ:
>      case SPICE_IMAGE_COMPRESSION_OFF:
>          display_channel->common.worker->image_compression =
>          pc->image_compression;
> +        dcc->image_compression = pc->image_compression;
>          return TRUE;
>      default:
>          spice_warning("preferred-compression: unsupported image compression
>          setting");
> @@ -8349,7 +8348,8 @@ static void handle_new_display_channel(RedWorker
> *worker, RedClient *client, Red
>      display_channel = worker->display_channel;
>      spice_info("add display channel client");
>      dcc = dcc_new(display_channel, client, stream, migrate,
> -                  common_caps, num_common_caps, caps, num_caps);
> +                  common_caps, num_common_caps, caps, num_caps,
> +                  worker->image_compression, worker->jpeg_state,
> worker->zlib_glz_state);
>      if (!dcc) {
>          return;
>      }
> @@ -8364,19 +8364,19 @@ static void handle_new_display_channel(RedWorker
> *worker, RedClient *client, Red
>                       DISPLAY_FREE_LIST_DEFAULT_SIZE *
>                       sizeof(SpiceResourceID));
>      dcc->send_data.free_list.res_size = DISPLAY_FREE_LIST_DEFAULT_SIZE;
>  
> -    if (worker->jpeg_state == SPICE_WAN_COMPRESSION_AUTO) {
> +    if (dcc->jpeg_state == SPICE_WAN_COMPRESSION_AUTO) {
>          display_channel->enable_jpeg = dcc->common.is_low_bandwidth;
>      } else {
> -        display_channel->enable_jpeg = (worker->jpeg_state ==
> SPICE_WAN_COMPRESSION_ALWAYS);
> +        display_channel->enable_jpeg = (dcc->jpeg_state ==
> SPICE_WAN_COMPRESSION_ALWAYS);
>      }
>  
>      // todo: tune quality according to bandwidth
>      display_channel->jpeg_quality = 85;
>  
> -    if (worker->zlib_glz_state == SPICE_WAN_COMPRESSION_AUTO) {
> +    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 = (worker->zlib_glz_state ==
> +        display_channel->enable_zlib_glz_wrap = (dcc->zlib_glz_state ==
>                                                   SPICE_WAN_COMPRESSION_ALWAYS);
>      }
>  
> --
> 2.4.3

Code is good (now)... comment not really. We are not moving compression parameters
but adding to DCC. I'll try a new proposal:


worker: add compression parameters to dcc

This allow different dcc to have different settings from default one.
The parameters are copied initially from default settings but then they can
change independently for each client.
Even having a single client a future client is not affected by a previous
setting on the old dcc.


Note: on a future patch the 

  display_channel->common.worker->image_compression = pc->image_compression;

line is removed from display_channel_handle_preferred_compression.
Perhaps this change can be incorporated now.

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]