On Wed, Nov 11, 2015 at 5:40 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:
> 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
>> > (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@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.
>>
>> 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?
This patch came from my review :-) It's mine.
>
>>
>> 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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel