[Spice-devel] [PATCH spice 1/3] dcc_compress_image: Handle NULL drawable

[Frediano Ziglio]

> ---
>  server/dcc.c | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/server/dcc.c b/server/dcc.c
> index eb5e4d1..b617b99 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -1056,6 +1056,7 @@ int dcc_compress_image(DisplayChannelClient *dcc,
>      DisplayChannel *display_channel = DCC_TO_DC(dcc);
>      SpiceImageCompression image_compression = dcc->image_compression;
>      int quic_compress = FALSE;
> +    uint32_t group_id;
>  
>      if ((image_compression == SPICE_IMAGE_COMPRESSION_OFF) ||
>          ((src->y * src->stride) < MIN_SIZE_TO_COMPRESS)) { // TODO: change
>          the size cond
> @@ -1086,7 +1087,8 @@ int dcc_compress_image(DisplayChannelClient *dcc,
>                  if ((src->x < MIN_DIMENSION_TO_QUIC) || (src->y <
>                  MIN_DIMENSION_TO_QUIC)) {
>                      quic_compress = FALSE;
>                  } else {
> -                    if (drawable->copy_bitmap_graduality ==
> BITMAP_GRADUAL_INVALID) {
> +                    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 {
> @@ -1099,6 +1101,12 @@ int dcc_compress_image(DisplayChannelClient *dcc,
>          }
>      }
>  
> +    if (drawable != NULL) {
> +        group_id = drawable->group_id;
> +    } else {
> +        group_id =
> red_worker_get_memslot(display_channel->common.worker)->internal_groupslot_id;
> +    }
> +
>      if (quic_compress) {
>  #ifdef COMPRESS_DEBUG
>          spice_info("QUIC compress");
> @@ -1109,24 +1117,22 @@ int dcc_compress_image(DisplayChannelClient *dcc,
>              (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,
> drawable->group_id);
> +                return dcc_compress_image_jpeg(dcc, dest, src, o_comp_data,
> group_id);
>              }
>          }
> -        return dcc_compress_image_quic(dcc, dest,
> -                                       src, o_comp_data,
> drawable->group_id);
> +        return dcc_compress_image_quic(dcc, dest, src, o_comp_data,
> group_id);
>      } else {
>          int glz;
>          int ret;
> -        if ((image_compression == SPICE_IMAGE_COMPRESSION_AUTO_GLZ) ||
> -            (image_compression == SPICE_IMAGE_COMPRESSION_GLZ)) {
> -            glz = 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)) {
> +        if ((image_compression == SPICE_IMAGE_COMPRESSION_AUTO_LZ) ||
> +            (image_compression == SPICE_IMAGE_COMPRESSION_LZ) ||
> +            (image_compression == SPICE_IMAGE_COMPRESSION_LZ4) ||
> +            drawable == NULL) {
>              glz = FALSE;
> +        } else if ((image_compression == SPICE_IMAGE_COMPRESSION_AUTO_GLZ)
> ||
> +                   (image_compression == SPICE_IMAGE_COMPRESSION_GLZ)) {
> +            glz = bitmap_fmt_has_graduality(src->format) &&
> +                  ((src->x * src->y) <
> glz_enc_dictionary_get_size(dcc->glz_dict->dict));
>          } else {
>              spice_error("invalid image compression type %u",
>              image_compression);
>              return FALSE;

Here the change looks bigger but you just moved the ifs to add the drawable == NULL check.

> @@ -1151,12 +1157,10 @@ int dcc_compress_image(DisplayChannelClient *dcc,
>                  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,
> -                                             drawable->group_id);
> +                ret = dcc_compress_image_lz4(dcc, dest, src, o_comp_data,
> group_id);
>              } else
>  #endif
> -                ret = dcc_compress_image_lz(dcc, dest, src, o_comp_data,
> -                                            drawable->group_id);
> +                ret = dcc_compress_image_lz(dcc, dest, src, o_comp_data,
> group_id);
>  #ifdef COMPRESS_DEBUG
>              spice_info("LZ LOCAL compress");
>  #endif
> --
> 2.5.0
> 

The patch looks good. From next patch this is done to use this function without
a drawable.
What looks ugly is the original code. If not really clear what the end results
will be. Beside all nested ifs and checks. Why for instance MIN_DIMENSION_TO_QUIC
is not checked when image_compression is SPICE_IMAGE_COMPRESSION_QUIC?
Is possible to compress palette images with Quic?
Not clear (at least to me) why glz require a Drawable.

Frediano