[Spice-devel] [PATCH spice 1/3] dcc_compress_image: Handle NULL drawable
Frediano Ziglio
fziglio at redhat.com
Thu Jan 14 07:27:02 PST 2016
>
> > ---
> > 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.
>
Had a small discussion with Pavel.
We agree that original code is quite complicated and is hard to understand
the final compression format used.
So we would like to have some public discussion about the topic.
I personally agree we should have a single code deciding the compression
to use.
This is the list of actual compressions:
- AUTO_GLZ;
- AUTO_LZ;
- QUIC;
- GLZ;
- LZ;
- LZ4.
A client can also decide to disable compression.
The AUTO_XXX looks like they should use QUIC as a fallback if XXX is not
possible or if an image with high graduality is detected.
Is not clear when JPEG is used. Looks like is used instead of QUIC if lossy
compression is enabled. But not always (for instance when we are sending an
image with red_marshall_image and we selected QUIC as default compression).
The LZ/GLZ algorithms do not support images with extra stride. But the
fallback if this happens is not clear (can be none or QUIC).
For LZ4 there is a client capability (SPICE_DISPLAY_CAP_LZ4_COMPRESSION) but
there is no such capability for LZ/GLZ. Looks like LZ/GLZ are used only if
AUTO_GLZ/AUTO_LZ/GLZ/LZ are selected (so the image compression setting in
this case define the capability).
Pavel noted that if an image has a palette (not true color/grey) there is
not compression.
I think there should be a single way to understand the format to use, some
kind of definitions.
Frediano
More information about the Spice-devel
mailing list