Re: [PATCH 07/30] Encapsulate lz information in EncodersData structure

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

 



On Wed, 2016-06-08 at 15:44 -0500, Jonathon Jongsma wrote:
> On Tue, 2016-06-07 at 11:17 +0100, Frediano Ziglio wrote:
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> >  server/dcc-encoders.c | 30 ++++++++++++++++--------------
> >  server/dcc-encoders.h |  3 +++
> >  server/dcc.c          | 23 +++++++++++++----------
> >  server/dcc.h          |  2 --
> >  4 files changed, 32 insertions(+), 26 deletions(-)
> > 
> > diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> > index 99e5507..54b970a 100644
> > --- a/server/dcc-encoders.c
> > +++ b/server/dcc-encoders.c
> > @@ -308,19 +308,19 @@ static void dcc_init_quic(EncodersData *enc)
> >      }
> >  }
> >  
> > -static void dcc_init_lz(DisplayChannelClient *dcc)
> > +static void dcc_init_lz(EncodersData *enc)
> Same comment as previous patch about dcc_ vs encoders_data_ prefix
> 
> > 
> >  {
> > -    dcc->lz_data.usr.error = lz_usr_error;
> > -    dcc->lz_data.usr.warn = lz_usr_warn;
> > -    dcc->lz_data.usr.info = lz_usr_warn;
> > -    dcc->lz_data.usr.malloc = lz_usr_malloc;
> > -    dcc->lz_data.usr.free = lz_usr_free;
> > -    dcc->lz_data.usr.more_space = lz_usr_more_space;
> > -    dcc->lz_data.usr.more_lines = lz_usr_more_lines;
> > +    enc->lz_data.usr.error = lz_usr_error;
> > +    enc->lz_data.usr.warn = lz_usr_warn;
> > +    enc->lz_data.usr.info = lz_usr_warn;
> > +    enc->lz_data.usr.malloc = lz_usr_malloc;
> > +    enc->lz_data.usr.free = lz_usr_free;
> > +    enc->lz_data.usr.more_space = lz_usr_more_space;
> > +    enc->lz_data.usr.more_lines = lz_usr_more_lines;
> >  
> > -    dcc->lz = lz_create(&dcc->lz_data.usr);
> > +    enc->lz = lz_create(&enc->lz_data.usr);
> >  
> > -    if (!dcc->lz) {
> > +    if (!enc->lz) {
> >          spice_critical("create lz failed");
> >      }
> >  }
> > @@ -400,9 +400,11 @@ static void dcc_init_zlib(DisplayChannelClient *dcc)
> >  
> >  void dcc_encoders_init(DisplayChannelClient *dcc)
> >  {
> > +    EncodersData *enc = &dcc->encoders;
> > +
> >      dcc_init_glz_data(dcc);
> > -    dcc_init_quic(&dcc->encoders);
> > -    dcc_init_lz(dcc);
> > +    dcc_init_quic(enc);
> > +    dcc_init_lz(enc);
> >      dcc_init_jpeg(dcc);
> >  #ifdef USE_LZ4
> >      dcc_init_lz4(dcc);
> > @@ -418,8 +420,8 @@ void dcc_encoders_free(DisplayChannelClient *dcc)
> >      EncodersData *enc = &dcc->encoders;
> >      quic_destroy(enc->quic);
> >      enc->quic = NULL;
> > -    lz_destroy(dcc->lz);
> > -    dcc->lz = NULL;
> > +    lz_destroy(enc->lz);
> > +    enc->lz = NULL;
> >      jpeg_encoder_destroy(dcc->jpeg);
> >      dcc->jpeg = NULL;
> >  #ifdef USE_LZ4
> > diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> > index a75678e..21049cd 100644
> > --- a/server/dcc-encoders.h
> > +++ b/server/dcc-encoders.h
> > @@ -165,6 +165,9 @@ struct RedGlzDrawable {
> >  struct EncodersData {
> >      QuicData quic_data;
> >      QuicContext *quic;
> > +
> > +    LzData lz_data;
> > +    LzContext  *lz;
> >  };
> >  
> >  typedef struct compress_send_data_t {
> > diff --git a/server/dcc.c b/server/dcc.c
> > index 0bc806a..05a511d 100644
> > --- a/server/dcc.c
> > +++ b/server/dcc.c
> > @@ -784,17 +784,18 @@ glz:
> >      return TRUE;
> >  }
> >  
> > -static int dcc_compress_image_lz(DisplayChannelClient *dcc,
> > +static int dcc_compress_image_lz(EncodersData *enc,
> >                                   SpiceImage *dest, SpiceBitmap *src,
> > -                                 compress_send_data_t* o_comp_data)
> > +                                 compress_send_data_t* o_comp_data,
> > +                                 stat_info_t *stats)
> as in quic patch, I think this stats change should be done in a separate
> patch.
> Presumably it could be done for all encoders at once (if there are more of
> them
> coming).

So I just realized (yeah, apparently I'm a bit slow...) that the reason that the
stats change is done as part of this change is because this is now an
EncodersData method rather than a DisplayChannelClient method. And there's no
good way to get from EncodersData to DisplayChannel. So it makes more sense why
they're done in the same commit. Nevertheless, I think I'd still prefer a
separate commit to do the stats change for all encoders.

> 
> > 
> >  {
> > -    LzData *lz_data = &dcc->lz_data;
> > -    LzContext *lz = dcc->lz;
> > +    LzData *lz_data = &enc->lz_data;
> > +    LzContext *lz = enc->lz;
> >      LzImageType type = bitmap_fmt_to_lz_image_type[src->format];
> >      int size;            // size of the compressed data
> >  
> >      stat_start_time_t start_time;
> > -    stat_start_time_init(&start_time, &DCC_TO_DC(dcc)->lz_stat);
> > +    stat_start_time_init(&start_time, stats);
> >  
> >  #ifdef COMPRESS_DEBUG
> >      spice_info("LZ LOCAL compress");
> > @@ -841,11 +842,10 @@ static int dcc_compress_image_lz(DisplayChannelClient
> > *dcc,
> >          o_comp_data->comp_buf = lz_data->data.bufs_head;
> >          o_comp_data->comp_buf_size = size;
> >  
> > -        dcc_palette_cache_palette(dcc, dest->u.lz_plt.palette, &(dest-
> > > 
> > > u.lz_plt.flags));
> I see this is moving to the caller. That's fine, but it doesn't really belong
> in
> this patch.
> 
> > 
> >          o_comp_data->lzplt_palette = dest->u.lz_plt.palette;
> >      }
> >  
> > -    stat_compress_add(&DCC_TO_DC(dcc)->lz_stat, start_time, src->stride *
> > src->y,
> > +    stat_compress_add(stats, start_time, src->stride * src->y,
> >                        o_comp_data->comp_buf_size);
> >      return TRUE;
> >  }
> > @@ -854,9 +854,9 @@ static int dcc_compress_image_jpeg(DisplayChannelClient
> > *dcc, SpiceImage *dest,
> >                                     SpiceBitmap *src, compress_send_data_t*
> > o_comp_data)
> >  {
> >      JpegData *jpeg_data = &dcc->jpeg_data;
> > -    LzData *lz_data = &dcc->lz_data;
> > +    LzData *lz_data = &dcc->encoders.lz_data;
> >      JpegEncoderContext *jpeg = dcc->jpeg;
> > -    LzContext *lz = dcc->lz;
> > +    LzContext *lz = dcc->encoders.lz;
> >      volatile JpegEncoderImageType jpeg_in_type;
> >      int jpeg_size = 0;
> >      volatile int has_alpha = FALSE;
> > @@ -1159,7 +1159,10 @@ int dcc_compress_image(DisplayChannelClient *dcc,
> >  #endif
> >  lz_compress:
> >      case SPICE_IMAGE_COMPRESSION_LZ:
> > -        success = dcc_compress_image_lz(dcc, dest, src, o_comp_data);
> > +        success = dcc_compress_image_lz(&dcc->encoders, dest, src,
> > o_comp_data, &display_channel->lz_stat);
> > +        if (success && !bitmap_fmt_is_rgb(src->format)) {
> > +            dcc_palette_cache_palette(dcc, dest->u.lz_plt.palette, &(dest-
> > > 
> > > u.lz_plt.flags));
> so the palette call has moved here, but why the additional check for
> bitmap_fmt_is_rgb()? This needs some additional explanation.
> 
> > 
> > +        }
> >          break;
> >      default:
> >          spice_error("invalid image compression type %u",
> > image_compression);
> > diff --git a/server/dcc.h b/server/dcc.h
> > index 4b93c73..532ce7b 100644
> > --- a/server/dcc.h
> > +++ b/server/dcc.h
> > @@ -66,8 +66,6 @@ struct DisplayChannelClient {
> >  
> >      int zlib_level;
> >  
> > -    LzData lz_data;
> > -    LzContext  *lz;
> >      JpegData jpeg_data;
> >      JpegEncoderContext *jpeg;
> >  #ifdef USE_LZ4
> Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://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]