On Wed, 2015-11-11 at 15:10 +0100, Pavel Grunt wrote:
> Hi Fabiano,
>
> 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@xxxxxxxxxx> wrote:
> > > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> > >
> > > ---
> > > 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 (Disp
> > > la
> > > yCha
> > > uin
> > > t3
> > > 2_t *common_caps,
> > > int
> > > num_common_caps,
> > > uin
> > > t3
> > > 2_t *caps,
> > > - int
> > > num_caps);
> > > + int
> > > num_caps,
> > > + Spi
> > > ce
> > > ImageCompression image_compression,
> > > + spi
> > > ce
> > > _wan_compression_t jpeg_state,
> > > + spi
> > > ce
> > > _wan_compression_t zlib_glz_state);
> > > void dcc_push_monitors_config (Dis
> > > pl
> > > ayChannelClient *dcc);
> > > void dcc_push_destroy_surface (Dis
> > > pl
> > > ayChannelClient *dcc,
> > > uin
> > > t3
> > > 2_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_AL
> > > WA
> > > YS);
> > > }
> > >
> > > --
> > > 2.4.3
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > > 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.
> >
> Since this patch. Before all the compression stuffs were handled by RedWorker,
> right?
>
> > 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 :-\
> >
> > Also, and about this small patch ... shall it be squashed? If not, why?
>
> According to the previous discussion [1], it is change of behavior. Instead of
> changing it for the future dcc, you are changing it only for the actual one.
> Personally I think your patch is correct.
>
> I don't think we should change it for the new dcc. I think the new should be
> created with the default compression settings (which comes from qemu and they
> are stored in global variables).
>
> Consider:
> 1. A starts server with image compression = xxx
> 2. B connects to the server and changes compression (using the preferred
> compression message) to yyy
> 3. A connects expecting compression xxx, but it will receive yyy
>
>
> Pavel
>
> [1] http://lists.freedesktop.org/archives/spice-devel/2015-November/023407.htm
> l
>
> >
> > 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;
actually to not change the behavior you should do both:
/* change for future dcc */
display_channel->common.worker->image_compression = pc->image_compression;
/* change for current dcc */
dcc->image_compression = pc->image_compression;
Pavel
> > default:
> > spice_warning("preferred-compression: unsupported image
> > compression setting");
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel