Re: [PATCH 5/7] worker: move compression parameters to dcc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> Hi Jonathon,
> 
> On Thu, 2015-11-12 at 11:41 -0600, Jonathon Jongsma wrote:
> > 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@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 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,
> > > > >                                                                        i
> > > > > nt
> > > > > num_common_caps,
> > > > >                                                                       
> > > > >  uint3
> > > > > 2_t *caps,
> > > > > -
> > > > >                                                                       i
> > > > > nt
> > > > > num_caps);
> > > > > +
> > > > >                                                                       i
> > > > > nt
> > > > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > > > 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.htm
> > > > l
> > > > http://lists.freedesktop.org/archives/spice-devel/2015-November/023498.htm
> > > > l
> > > > 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.
> > 
> It would change the current behavior. Please, try it using --spice-preferred-
> compression. Currently (v0.12.6) it changes the compression for the connected
> client and for the future ones as well.
> 
> I don't think that we should do the move & change of behavior in one step.
> That
> is reason why I suggested to add the line, instead of replacing 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?
> 
> Of course, each of display channels can have different compression (it is
> possible now with the current git master). I think we should also remove the
> compression from RedWorker at all.
> 
> Pavel
> > 

I'm lost a bit with this patch.
Can you or Jonathon came out with a patch that replace/update/fix the one I posted?

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]