[Spice-devel] [PATCH 03/11] worker: move compression parameters to dcc
Pavel Grunt
pgrunt at redhat.com
Wed Nov 18 07:09:35 PST 2015
Hi,
On Wed, 2015-11-18 at 06:45 -0500, Frediano Ziglio wrote:
> >
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> > [updated for the preferred compression]
> > Signed-off-by: Pavel Grunt <pgrunt at redhat.com>
> > ---
> > 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
Yeah, I guess there is more similar cases.
>
> 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
> > uint3
> > 2_t
> > *comm
> > on_caps,
> > int
> > num_c
> > ommon_caps,
> > uint3
> > 2_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,
> > uint3
> > 2_t
> > surfa
> > ce_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_ALWA
> > YS);
> > }
> >
> > --
> > 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.
>
Ack
>
> 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.
>
I would prefer to do it in a separate patch, because it is change of behavior.
Pavel
> Frediano
More information about the Spice-devel
mailing list