Re: [PATCH spice 1/3] dcc: Rewrite dcc_image_compress

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

 



> 
> On Tue, 2016-02-16 at 06:59 -0500, Frediano Ziglio wrote:
> > > 
> > > Rules are now:
> > > 
> > > Compression type:
> > >   off      -> uncompressed
> > >   quic     -> jpeg if possible else quic else off
> > >   lz       -> lz if possible else off
> > >   glz      -> glz if possible else lz else off
> > >   auto_lz  -> lz if possible else jpeg else quic else off
> > >   auto_glz -> glz if possible else lz else jpeg else quic else off
> > >   lz4      -> lz4 if possible else lz else off
> > > ---
> > >  server/dcc.c | 210
> > >  +++++++++++++++++++++++++++++++++--------------------------
> > >  1 file changed, 118 insertions(+), 92 deletions(-)
> > > 
> > > diff --git a/server/dcc.c b/server/dcc.c
> > > index 9a4e90c..c479091 100644
> > > --- a/server/dcc.c
> > > +++ b/server/dcc.c
> > > @@ -1080,124 +1080,150 @@ static int
> > > dcc_compress_image_quic(DisplayChannelClient *dcc, SpiceImage
> > > *dest,
> > >      return TRUE;
> > >  }
> > >  
> > > -#define MIN_SIZE_TO_COMPRESS 54
> > >  #define MIN_DIMENSION_TO_QUIC 3
> > > +/**
> > > + * quic doesn't handle:
> > > + *       (1) palette
> > > + */
> > > +static bool can_quic_compress(SpiceBitmap *bitmap)
> > > +{
> > > +    return !bitmap_fmt_is_plt(bitmap->format) &&
> > > +            bitmap->x >= MIN_DIMENSION_TO_QUIC && bitmap->y >=
> > > MIN_DIMENSION_TO_QUIC;
> > > +}
> > > +/**
> > > + * lz doesn't handle:
> > > + *       (1) bitmaps with strides that are larger than the width
> > > of the
> > > image in bytes
> > > + *       (2) unstable bitmaps
> > > + */
> > > +static bool can_lz_compress(SpiceBitmap *bitmap)
> > > +{
> > > +    return !bitmap_has_extra_stride(bitmap) &&
> > > +           !(bitmap->data->flags & SPICE_CHUNKS_FLAGS_UNSTABLE);
> > > +}
> > > +
> > > +#define MIN_SIZE_TO_COMPRESS 54
> > > +static SpiceImageCompression
> > > get_compression_for_bitmap(SpiceBitmap *bitmap,
> > > +
> > > SpiceImageCompression
> > > preferred_compression,
> > > +                                                        Drawable
> > > *drawable)
> > > +{
> > > +    if (bitmap->y * bitmap->stride < MIN_SIZE_TO_COMPRESS) { //
> > > TODO: change
> > > the size cond
> > > +        return SPICE_IMAGE_COMPRESSION_OFF;
> > > +    }
> > > +    if (preferred_compression == SPICE_IMAGE_COMPRESSION_OFF) {
> > > +        return SPICE_IMAGE_COMPRESSION_OFF;
> > > +    }
> > > +    if (preferred_compression == SPICE_IMAGE_COMPRESSION_QUIC) {
> > > +        if (can_quic_compress(bitmap)) {
> > > +            return SPICE_IMAGE_COMPRESSION_QUIC;
> > > +        }
> > > +        return SPICE_IMAGE_COMPRESSION_OFF;
> > > +    }
> > > +
> > > +    if (preferred_compression == SPICE_IMAGE_COMPRESSION_AUTO_GLZ
> > > ||
> > > +        preferred_compression == SPICE_IMAGE_COMPRESSION_AUTO_LZ)
> > > {
> > > +        if (can_quic_compress(bitmap)) {
> > > +            if (drawable == NULL ||
> > > +                drawable->copy_bitmap_graduality ==
> > > BITMAP_GRADUAL_INVALID)
> > > {
> > > +                if (bitmap_fmt_has_graduality(bitmap->format) &&
> > > +                    bitmap_get_graduality_level(bitmap) ==
> > > BITMAP_GRADUAL_HIGH) {
> > > +                    return SPICE_IMAGE_COMPRESSION_QUIC;
> > > +                }
> > > +            } else if (!can_lz_compress(bitmap) ||
> > > +                       drawable->copy_bitmap_graduality ==
> > > BITMAP_GRADUAL_HIGH) {
> > > +                return SPICE_IMAGE_COMPRESSION_QUIC;
> > > +            }
> > > +        }
> > > +        if (preferred_compression ==
> > > SPICE_IMAGE_COMPRESSION_AUTO_LZ) {
> > > +            preferred_compression = SPICE_IMAGE_COMPRESSION_LZ;
> > > +        } else {
> > > +            preferred_compression = SPICE_IMAGE_COMPRESSION_GLZ;
> > > +        }
> > > +    }
> > > +
> > > +    if (preferred_compression == SPICE_IMAGE_COMPRESSION_GLZ) {
> > > +        if (drawable == NULL || bitmap_fmt_has_graduality(bitmap-
> > > >format)) {
> > 
> > I think the old condition was
> > 
> >    glz = drawable != NULL && bitmap_fmt_has_graduality(src->format)
> > &&
> >          ((src->x * src->y) < glz_enc_dictionary_get_size(dcc-
> > >glz_dict->dict));
> > 
> > so the reverse should be
> > 
> >    if (drawable == NULL || !bitmap_fmt_has_graduality(bitmap-
> > >format)) {
> > 
> right, thanks
> 
> 
> > > +            preferred_compression = SPICE_IMAGE_COMPRESSION_LZ;
> > > +        }
> > > +    }
> > > +
> > > +    if (preferred_compression == SPICE_IMAGE_COMPRESSION_LZ4) {
> > > +        if (!bitmap_fmt_is_rgb(bitmap->format)) {
> > > +            preferred_compression = SPICE_IMAGE_COMPRESSION_LZ;
> > > +        }
> > > +    }
> > > +
> > > +    if (preferred_compression == SPICE_IMAGE_COMPRESSION_LZ ||
> > > +        preferred_compression == SPICE_IMAGE_COMPRESSION_LZ4 ||
> > > +        preferred_compression == SPICE_IMAGE_COMPRESSION_GLZ) {
> > > +        if (can_lz_compress(bitmap)) {
> > > +            return preferred_compression;
> > > +        }
> > > +        return SPICE_IMAGE_COMPRESSION_OFF;
> > > +    }
> > > +
> > > +    return SPICE_IMAGE_COMPRESSION_INVALID;
> > > +}
> > > +
> > >  int dcc_compress_image(DisplayChannelClient *dcc,
> > >                         SpiceImage *dest, SpiceBitmap *src,
> > > Drawable
> > >                         *drawable,
> > >                         int can_lossy,
> > >                         compress_send_data_t* o_comp_data)
> > >  {
> > >      DisplayChannel *display_channel = DCC_TO_DC(dcc);
> > > -    SpiceImageCompression image_compression = dcc-
> > > >image_compression;
> > > -    int quic_compress = FALSE;
> > > +    SpiceImageCompression image_compression;
> > >  
> > > -    if ((image_compression == SPICE_IMAGE_COMPRESSION_OFF) ||
> > > -        ((src->y * src->stride) < MIN_SIZE_TO_COMPRESS)) { //
> > > TODO: change
> > > the size cond
> > > +    image_compression = get_compression_for_bitmap(src,
> > > dcc->image_compression, drawable);
> > > +    switch (image_compression) {
> > > +    case SPICE_IMAGE_COMPRESSION_OFF:
> > >          return FALSE;
> > > -    } else if (image_compression == SPICE_IMAGE_COMPRESSION_QUIC)
> > > {
> > > -        if (bitmap_fmt_is_plt(src->format)) {
> > > -            return FALSE;
> > > -        } else {
> > > -            quic_compress = TRUE;
> > > -        }
> > > -    } else {
> > > -        /*
> > > -            lz doesn't handle (1) bitmaps with strides that are
> > > larger than
> > > the width
> > > -            of the image in bytes (2) unstable bitmaps
> > > -        */
> > > -        if (bitmap_has_extra_stride(src) || (src->data->flags &
> > > SPICE_CHUNKS_FLAGS_UNSTABLE)) {
> > > -            if ((image_compression == SPICE_IMAGE_COMPRESSION_LZ)
> > > ||
> > > -                (image_compression == SPICE_IMAGE_COMPRESSION_GLZ)
> > > ||
> > > -                (image_compression == SPICE_IMAGE_COMPRESSION_LZ4)
> > > ||
> > > -                bitmap_fmt_is_plt(src->format)) {
> > > -                return FALSE;
> > > -            } else {
> > > -                quic_compress = TRUE;
> > > -            }
> > > -        } else {
> > > -            if ((image_compression ==
> > > SPICE_IMAGE_COMPRESSION_AUTO_LZ) ||
> > > -                (image_compression ==
> > > SPICE_IMAGE_COMPRESSION_AUTO_GLZ)) {
> > > -                if ((src->x < MIN_DIMENSION_TO_QUIC) || (src->y <
> > > MIN_DIMENSION_TO_QUIC)) {
> > > -                    quic_compress = FALSE;
> > > -                } else {
> > > -                    if (drawable == NULL ||
> > > -                        drawable->copy_bitmap_graduality ==
> > > BITMAP_GRADUAL_INVALID) {
> > > -                        quic_compress =
> > > bitmap_fmt_has_graduality(src->format) &&
> > > -                            bitmap_get_graduality_level(src) ==
> > > BITMAP_GRADUAL_HIGH;
> > > -                    } else {
> > > -                        quic_compress = (drawable-
> > > >copy_bitmap_graduality ==
> > > BITMAP_GRADUAL_HIGH);
> > > -                    }
> > > -                }
> > > -            } else {
> > > -                quic_compress = FALSE;
> > > -            }
> > > +    case SPICE_IMAGE_COMPRESSION_QUIC:
> > > +        if (can_lossy && display_channel->enable_jpeg &&
> > > +            (src->format != SPICE_BITMAP_FMT_RGBA ||
> > > !bitmap_has_extra_stride(src))) {
> > > +#ifdef COMPRESS_DEBUG
> > > +            spice_info("JPEG compress");
> > > +#endif
> > > +            return dcc_compress_image_jpeg(dcc, dest, src,
> > > o_comp_data);
> > >          }
> > > -    }
> > > -
> > > -    if (quic_compress) {
> > >  #ifdef COMPRESS_DEBUG
> > >          spice_info("QUIC compress");
> > >  #endif
> > > -        // if bitmaps is picture-like, compress it using jpeg
> > > -        if (can_lossy && display_channel->enable_jpeg &&
> > > -            ((image_compression ==
> > > SPICE_IMAGE_COMPRESSION_AUTO_LZ) ||
> > > -            (image_compression ==
> > > SPICE_IMAGE_COMPRESSION_AUTO_GLZ))) {
> > > -            // if we use lz for alpha, the stride can't be extra
> > > -            if (src->format != SPICE_BITMAP_FMT_RGBA ||
> > > !bitmap_has_extra_stride(src)) {
> > > -                return dcc_compress_image_jpeg(dcc, dest, src,
> > > o_comp_data);
> > > -            }
> > > -        }
> > >          return dcc_compress_image_quic(dcc, dest, src,
> > > o_comp_data);
> > > -    } else {
> > > -        int glz;
> > > -        int ret;
> > > -        if ((image_compression ==
> > > SPICE_IMAGE_COMPRESSION_AUTO_GLZ) ||
> > > -            (image_compression == SPICE_IMAGE_COMPRESSION_GLZ)) {
> > > -            glz = drawable != NULL &&
> > > bitmap_fmt_has_graduality(src->format)
> > > &&
> > > -                  ((src->x * src->y) <
> > > glz_enc_dictionary_get_size(dcc->glz_dict->dict));
> > > -        } else if ((image_compression ==
> > > SPICE_IMAGE_COMPRESSION_AUTO_LZ) ||
> > > -                   (image_compression ==
> > > SPICE_IMAGE_COMPRESSION_LZ) ||
> > > -                   (image_compression ==
> > > SPICE_IMAGE_COMPRESSION_LZ4)) {
> > > -            glz = FALSE;
> > > -        } else {
> > > -            spice_error("invalid image compression type %u",
> > > image_compression);
> > > -            return FALSE;
> > > -        }
> > > -
> > > -        if (glz) {
> > > +    case SPICE_IMAGE_COMPRESSION_GLZ:
> > > +        if ((src->x * src->y) <
> > > glz_enc_dictionary_get_size(dcc->glz_dict->dict)) {
> > > +            int ret, frozen;
> > >              /* using the global dictionary only if it is not
> > > frozen */
> > >              pthread_rwlock_rdlock(&dcc->glz_dict->encode_lock);
> > > -            if (!dcc->glz_dict->migrate_freeze) {
> > > -                ret = dcc_compress_image_glz(dcc,
> > > -                                             dest, src,
> > > -                                             drawable,
> > > o_comp_data);
> > > -            } else {
> > > -                glz = FALSE;
> > > +            frozen = dcc->glz_dict->migrate_freeze;
> > > +            if (!frozen) {
> > > +#ifdef COMPRESS_DEBUG
> > > +                spice_info("LZ global compress fmt=%d", src-
> > > >format);
> > > +#endif
> > > +                ret = dcc_compress_image_glz(dcc, dest, src,
> > > drawable,
> > > o_comp_data);
> > >              }
> > >              pthread_rwlock_unlock(&dcc->glz_dict->encode_lock);
> > > +            if (!frozen) {
> > > +                return ret;
> > > +            }
> > >          }
> > 
> > Note that here you falling through lz4 instead of lz.
> > 
> > This is against the rules
> > 
> >  glz      -> glz if possible else lz else off
> >  auto_glz -> glz if possible else lz else jpeg else quic else off
> 
> true, after yesterday's discussion with fidencio and Fantu on irc I
> would change it to:
> 
> glz -> glz if possible else off
> auto_glz -> glz if possible else jpeg else quic else off
> 

I'd rather go for a 

case GLZ:
   // glx
   goto lz_compress;
case LZ4:
   // lz4
lz_compress:
case LZ:
   // lz

Frediano

> > 
> > > -
> > > -        if (!glz) {
> > >  #ifdef USE_LZ4
> > > -            if (image_compression == SPICE_IMAGE_COMPRESSION_LZ4
> > > &&
> > > -                bitmap_fmt_is_rgb(src->format) &&
> > > -                red_channel_client_test_remote_cap(&dcc-
> > > >common.base,
> > > -                        SPICE_DISPLAY_CAP_LZ4_COMPRESSION)) {
> > > -                ret = dcc_compress_image_lz4(dcc, dest, src,
> > > o_comp_data);
> > > -            } else
> > > -#endif
> > > -                ret = dcc_compress_image_lz(dcc, dest, src,
> > > o_comp_data);
> > > +    case SPICE_IMAGE_COMPRESSION_LZ4:
> > > +        if (red_channel_client_test_remote_cap(&dcc->common.base,
> > > +
> > > SPICE_DISPLAY_CAP_LZ4_COMPRESSION))
> > > {
> > >  #ifdef COMPRESS_DEBUG
> > > -            spice_info("LZ LOCAL compress");
> > > +            spice_info("LZ4 compress");
> > >  #endif
> > > +            return dcc_compress_image_lz4(dcc, dest, src,
> > > o_comp_data);
> > >          }
> > > +#endif
> > > +    case SPICE_IMAGE_COMPRESSION_LZ:
> > >  #ifdef COMPRESS_DEBUG
> > > -        else {
> > > -            spice_info("LZ global compress fmt=%d", src->format);
> > > -        }
> > > +        spice_info("LZ LOCAL compress");
> > >  #endif
> > > -        return ret;
> > > +        return dcc_compress_image_lz(dcc, dest, src, o_comp_data);
> > > +    default:
> > > +        spice_error("invalid image compression type %u",
> > > image_compression);
> > >      }
> > > +    return FALSE;
> > >  }
> > >  
> > >  #define CLIENT_PALETTE_CACHE
> > 
> > Looking at the code you can see clearly the intention of the code!
> > Much more nice than before!
> > 
> > Note for reviewers: better to see before and after the change.
> > 
> > Frediano
> 
> Thanks for the review, I will send a new version.
> 
> Pavel
> 
> 
> 
> 
_______________________________________________
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]