On Wed, Jan 30, 2019 at 08:09:57AM -0500, Frediano Ziglio wrote:
> > 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@xxxxxxxxxx>
> > > ---
> > > 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.
>
I never said it's incorrect, all I'm saying is that we should not make
something look like a boolean when it's a pointer. More explicit type
when reading the code is better in my opinion.
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel