[Spice-devel] [PATCH 05/11] worker: move compression parameters to dcc
Jonathon Jongsma
jjongsma at redhat.com
Wed Nov 11 08:40:42 PST 2015
On Wed, 2015-11-11 at 14:45 +0100, Fabiano Fidêncio wrote:
> On Wed, Nov 11, 2015 at 1:20 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 5f422c9..163f6b7 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 334dbf7..3db6eef 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
> > (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 3908f97..5cdb348 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -4623,8 +4623,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) ||
> > @@ -5028,15 +5027,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);
> > }
> > @@ -6923,7 +6921,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)) {
> > @@ -8022,10 +8020,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;
> > }
> > }
> > @@ -8558,7 +8556,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;
> > }
> > @@ -8573,19 +8572,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
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> In the first time this patch was sent, Pavel pointed out:
> "Hi,
> it is keeping the compression parameters in the RedWorker structure.
> I think they should be removed from the structure and only used from dcc."
>
> I'm not sure if it's possible. RedWorker keeps these info and use them
> when creating a new DCC.
>
> One thing that I am not exactly sure is whether we want to keep the
> RedWorker and DCC values in sync as we have to keep both.If that's the
> case, the patch is just going to make things worse :-\
The values in the the RedWorker should be the values passed on the commandline
(or default values). These are the default values used for new channel clients.
At the moment, we only support one client at a time, but theoretically we could
suport multiple clients in the future. And each of these clients could negotiate
a different preferred compression type. So I don't think we necessarily want to
keep the client compression values in sync with the worker. That said, this
isn't an area I have much experience in...
>
> Also, and about this small patch ... shall it be squashed? If not, why?
That looks correct to me, but where does this patch come from?
>
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 5cdb348..5e2bcd6 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -8091,6 +8091,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");
> _______________________________________________
> 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