[Spice-devel] [PATCH 5/7] worker: move compression parameters to dcc
Jonathon Jongsma
jjongsma at redhat.com
Thu Nov 12 09:41:26 PST 2015
On Thu, 2015-11-12 at 16:24 +0100, Pavel Grunt wrote:
> On Thu, 2015-11-12 at 15:40 +0100, Fabiano Fidêncio wrote:
> > On Thu, Nov 12, 2015 at 3:00 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > >
> > > ---
> > > server/display-channel.c | 9 ++++++++-
> > > server/display-channel.h | 8 +++++++-
> > > server/red_worker.c | 31 +++++++++++++++----------------
> > > 3 files changed, 30 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > index f6f96a4..824f601 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;
> > >
> > > @@ -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;
> > > }
> > > diff --git a/server/display-channel.h b/server/display-channel.h
> > > index 5518494..12ef60a 100644
> > > --- a/server/display-channel.h
> > > +++ b/server/display-channel.h
> > > @@ -161,6 +161,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;
> > >
> > > @@ -233,7 +236,10 @@
> > > DisplayChannelClient* dcc_new
> > > (Displa
> > > yCha
> > >
> > > uint3
> > > 2_t *common_caps,
> > > int
> > > num_common_caps,
> > >
> > > uint3
> > > 2_t *caps,
> > > - int
> > > num_caps);
> > > + int
> > > num_caps,
> > > +
> > > Spice
> > > ImageCompression image_compression,
> > > +
> > > spice
> > > _wan_compression_t jpeg_state,
> > > +
> > > spice
> > > _wan_compression_t zlib_glz_state);
> > > void dcc_push_monitors_config
> > > (Displ
> > > ayChannelClient *dcc);
> > > void dcc_push_destroy_surface
> > > (Displ
> > > ayChannelClient *dcc,
> > >
> > > uint3
> > > 2_t surface_id);
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 68ac527..af85591 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -4488,8 +4488,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) ||
> > > @@ -4893,15 +4892,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);
> > > }
> > > @@ -6788,7 +6786,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)) {
> > > @@ -7887,10 +7885,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;
> > > }
> > > }
> > > @@ -8428,7 +8426,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;
> > > }
> > > @@ -8443,19 +8442,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
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> > Already reviewed. Important comments on these 3 emails:
> > http://lists.freedesktop.org/archives/spice-devel/2015-November/023481.html
> > http://lists.freedesktop.org/archives/spice-devel/2015-November/023498.html
> > and
> > http://lists.freedesktop.org/archives/spice-devel/2015-November/023503.htm
> > l
> >
> Original commit could not count with the preferred image compression message,
> because it was introduced recently. It needs this fixup [1], the preferred
> compression will not work:
>
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 8aa7e70..569080a 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -6967,6 +6967,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");
>
> [1] http://paste.fedoraproject.org/289662/41749144/
>
> Ack from me with this change.
>
> Pavel
>
I don't think I agree with changing the worker's image_compression value in
_handle_preferred_compression(). I think that the line you added in this patch
should replace the existing one, not be in addition to it.
The preferred compression value is very much a client-specific thing. Currently
we only support a single client, so it doesn't matter very much. But if we did
add support for multiple clients, we wouldn't necessarily want the preferred
compression of one client to affect the compression value for other clients.
right?
>
>
>
>
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list