[Spice-devel] [PATCH spice-server v2] image-encoders: Initialize Zlib lazily

[Frediano Ziglio]

> On Mon, Jan 28, 2019 at 07:33:54PM +0000, Frediano Ziglio wrote:
> > Zlib structure take up more than 1MB and it is rarely used nowadays
> > as it is not much effective.
> > Initialise it only when necessary saving some memory in the normal
> > case.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/image-encoders.c | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > Changes since v1:
> > - log a warning if zlib encoding cannot be initialized
> > 
> > diff --git a/server/image-encoders.c b/server/image-encoders.c
> > index 88073a3e..f35621ff 100644
> > --- a/server/image-encoders.c
> > +++ b/server/image-encoders.c
> > @@ -451,12 +451,6 @@ static void image_encoders_init_zlib(ImageEncoders
> > *enc)
> >  {
> >      enc->zlib_data.usr.more_space = zlib_usr_more_space;
> >      enc->zlib_data.usr.more_input = zlib_usr_more_input;
> > -
> > -    enc->zlib = zlib_encoder_create(&enc->zlib_data.usr,
> > ZLIB_DEFAULT_COMPRESSION_LEVEL);
> > -
> > -    if (!enc->zlib) {
> > -        spice_critical("create zlib encoder failed");
> > -    }
> >  }
> >  
> >  void image_encoders_init(ImageEncoders *enc, ImageEncoderSharedData
> >  *shared_data)
> > @@ -494,8 +488,10 @@ void image_encoders_free(ImageEncoders *enc)
> >      lz4_encoder_destroy(enc->lz4);
> >      enc->lz4 = NULL;
> >  #endif
> > -    zlib_encoder_destroy(enc->zlib);
> > -    enc->zlib = NULL;
> > +    if (enc->zlib) {
> 
> if (enc->zlib != NULL)
> 
> > +        zlib_encoder_destroy(enc->zlib);
> > +        enc->zlib = NULL;
> > +    }
> >      pthread_mutex_destroy(&enc->glz_drawables_inst_to_free_lock);
> >  }
> >  
> > @@ -1261,6 +1257,13 @@ bool image_encoders_compress_glz(ImageEncoders *enc,
> >      if (!enable_zlib_glz_wrap || (glz_size < MIN_GLZ_SIZE_FOR_ZLIB)) {
> >          goto glz;
> >      }
> > +    if (!enc->zlib) {
> 
> This is a pointer, not a boolean, so I'd prefer if (enc->zlib == NULL)
> 

In C99 is perfectly fine to check pointer like this, is just
a question of style, we just use C89 here instead of C99.

> > +        enc->zlib = zlib_encoder_create(&enc->zlib_data.usr,
> > ZLIB_DEFAULT_COMPRESSION_LEVEL);
> > +        if (!enc->zlib) {
> 
> Ditto
> 
> > +            g_warning("create zlib encoder failed");
> 
> I believe any of "zlib_encoder_create failed" or "zlib encoder creation
> failed" or
> "creating zlib encoder failed" would be more grammatically correct.
> 

I think was just copied, updated.

> > +            goto glz;
> > +        }
> > +    }
> 
> 
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> 

Frediano