On Tue, 2015-11-03 at 10:20 +0000, Frediano Ziglio wrote: > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx> > > --- > server/Makefile.am | 2 + > server/red_common.h | 13 ---- > server/red_parse_qxl.c | 1 + > server/red_worker.c | 158 ++++------------------------------ > ---------- > server/spice-bitmap-utils.c | 119 +++++++++++++++++++++++++++++++++ > server/spice-bitmap-utils.h | 91 +++++++++++++++++++++++++ > server/tree.h | 9 +-- > 7 files changed, 226 insertions(+), 167 deletions(-) > create mode 100644 server/spice-bitmap-utils.c > create mode 100644 server/spice-bitmap-utils.h > > diff --git a/server/Makefile.am b/server/Makefile.am > index d2a7343..7216ab0 100644 > --- a/server/Makefile.am > +++ b/server/Makefile.am > @@ -133,6 +133,8 @@ libspice_server_la_SOURCES = > \ > pixmap-cache.c \ > tree.h \ > tree.c \ > + spice-bitmap-utils.h \ > + spice-bitmap-utils.c \ > utils.h \ > $(NULL) > > diff --git a/server/red_common.h b/server/red_common.h > index 47e591d..04d4c02 100644 > --- a/server/red_common.h > +++ b/server/red_common.h > @@ -44,17 +44,4 @@ static const LzImageType > MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[] = { > LZ_IMAGE_TYPE_A8 > }; > > -static inline int bitmap_fmt_is_rgb(uint8_t fmt) > -{ > - static const int BITMAP_FMT_IS_RGB[SPICE_BITMAP_FMT_ENUM_END] = > - {0, 0, 0, 0, 0, 0, 1, 1, 1, > 1, 1}; > - > - if (fmt >= SPICE_BITMAP_FMT_ENUM_END) { > - spice_warning("fmt >= SPICE_BITMAP_FMT_ENUM_END; %d >= %d", > - fmt, SPICE_BITMAP_FMT_ENUM_END); > - return 0; > - } > - return BITMAP_FMT_IS_RGB[fmt]; > -} > - > #endif > diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c > index dd52602..5fc1a13 100644 > --- a/server/red_parse_qxl.c > +++ b/server/red_parse_qxl.c > @@ -23,6 +23,7 @@ > #include <inttypes.h> > #include <glib.h> > #include "common/lz_common.h" > +#include "spice-bitmap-utils.h" > #include "red_common.h" > #include "red_memslots.h" > #include "red_parse_qxl.h" > diff --git a/server/red_worker.c b/server/red_worker.c > index 0d0c603..bad8e47 100644 > --- a/server/red_worker.c > +++ b/server/red_worker.c > @@ -310,13 +310,6 @@ typedef struct StreamClipItem { > SpiceClipRects *rects; > } StreamClipItem; > > -static const int BITMAP_FMT_IS_PLT[] = {0, 1, 1, 1, 1, 1, 0, 0, 0, > 0, 0}; > -static const int BITMAP_FMP_BYTES_PER_PIXEL[] = {0, 0, 0, 0, 0, 1, > 2, 3, 4, 4, 1}; > - > -#define BITMAP_FMT_HAS_GRADUALITY(f) > \ > - (bitmap_fmt_is_rgb(f) && > \ > - ((f) != SPICE_BITMAP_FMT_8BIT_A)) > - > typedef struct { > QuicUsrContext usr; > EncoderData data; > @@ -613,10 +606,6 @@ static int > red_display_free_some_independent_glz_drawables(DisplayChannelClient > static void red_display_free_glz_drawable(DisplayChannelClient *dcc, > RedGlzDrawable *drawable); > static ImageItem *red_add_surface_area_image(DisplayChannelClient > *dcc, int surface_id, > SpiceRect *area, > PipeItem *pos, int can_lossy); > -static BitmapGradualType _get_bitmap_graduality_level(RedWorker > *worker, SpiceBitmap *bitmap, > - uint32_t > group_id); > -static inline int _stride_is_extra(SpiceBitmap *bitmap); > - > static void > display_channel_client_release_item_before_push(DisplayChannelClient > *dcc, > PipeItem > *item); > static void > display_channel_client_release_item_after_push(DisplayChannelClient > *dcc, > @@ -2650,12 +2639,11 @@ static inline void > red_update_copy_graduality(RedWorker* worker, Drawable *drawa > > bitmap = &drawable->red_drawable->u.copy.src_bitmap->u.bitmap; > > - if (!BITMAP_FMT_HAS_GRADUALITY(bitmap->format) || > _stride_is_extra(bitmap) || > + if (!bitmap_fmt_has_graduality(bitmap->format) || > bitmap_has_extra_stride(bitmap) || > (bitmap->data->flags & SPICE_CHUNKS_FLAGS_UNSTABLE)) { > drawable->copy_bitmap_graduality = BITMAP_GRADUAL_NOT_AVAIL; > } else { > - drawable->copy_bitmap_graduality = > - _get_bitmap_graduality_level(worker, bitmap,drawable > ->group_id); > + drawable->copy_bitmap_graduality = > bitmap_get_graduality_level(bitmap); > } > } > > @@ -4902,124 +4890,6 @@ static inline void red_init_zlib(RedWorker > *worker) > } > } > > -typedef struct { > - uint8_t b; > - uint8_t g; > - uint8_t r; > - uint8_t pad; > -} rgb32_pixel_t; > - > -G_STATIC_ASSERT(sizeof(rgb32_pixel_t) == 4); > - > -typedef struct { > - uint8_t b; > - uint8_t g; > - uint8_t r; > -} rgb24_pixel_t; > - > -G_STATIC_ASSERT(sizeof(rgb24_pixel_t) == 3); > - > -typedef uint16_t rgb16_pixel_t; > - > -#define RED_BITMAP_UTILS_RGB16 > -#include "red_bitmap_utils_tmpl.c" > -#define RED_BITMAP_UTILS_RGB24 > -#include "red_bitmap_utils_tmpl.c" > -#define RED_BITMAP_UTILS_RGB32 > -#include "red_bitmap_utils_tmpl.c" > - > -#define GRADUAL_HIGH_RGB24_TH -0.03 > -#define GRADUAL_HIGH_RGB16_TH 0 > - > -// setting a more permissive threshold for stream identification in > order > -// not to miss streams that were artificially scaled on the guest > (e.g., full screen view > -// in window media player 12). see red_stream_add_frame > -#define GRADUAL_MEDIUM_SCORE_TH 0.002 > - > -// assumes that stride doesn't overflow > -static BitmapGradualType _get_bitmap_graduality_level(RedWorker > *worker, SpiceBitmap *bitmap, > - uint32_t > group_id) > -{ > - double score = 0.0; > - int num_samples = 0; > - int num_lines; > - double chunk_score = 0.0; > - int chunk_num_samples = 0; > - uint32_t x, i; > - SpiceChunk *chunk; > - > - chunk = bitmap->data->chunk; > - for (i = 0; i < bitmap->data->num_chunks; i++) { > - num_lines = chunk[i].len / bitmap->stride; > - x = bitmap->x; > - switch (bitmap->format) { > - case SPICE_BITMAP_FMT_16BIT: > - compute_lines_gradual_score_rgb16((rgb16_pixel_t > *)chunk[i].data, x, num_lines, > - &chunk_score, > &chunk_num_samples); > - break; > - case SPICE_BITMAP_FMT_24BIT: > - compute_lines_gradual_score_rgb24((rgb24_pixel_t > *)chunk[i].data, x, num_lines, > - &chunk_score, > &chunk_num_samples); > - break; > - case SPICE_BITMAP_FMT_32BIT: > - case SPICE_BITMAP_FMT_RGBA: > - compute_lines_gradual_score_rgb32((rgb32_pixel_t > *)chunk[i].data, x, num_lines, > - &chunk_score, > &chunk_num_samples); > - break; > - default: > - spice_error("invalid bitmap format (not RGB) %u", bitmap > ->format); > - } > - score += chunk_score; > - num_samples += chunk_num_samples; > - } > - > - spice_assert(num_samples); > - score /= num_samples; > - > - if (bitmap->format == SPICE_BITMAP_FMT_16BIT) { > - if (score < GRADUAL_HIGH_RGB16_TH) { > - return BITMAP_GRADUAL_HIGH; > - } > - } else { > - if (score < GRADUAL_HIGH_RGB24_TH) { > - return BITMAP_GRADUAL_HIGH; > - } > - } > - > - if (score < GRADUAL_MEDIUM_SCORE_TH) { > - return BITMAP_GRADUAL_MEDIUM; > - } else { > - return BITMAP_GRADUAL_LOW; > - } > -} > - > -static inline int _stride_is_extra(SpiceBitmap *bitmap) > -{ > - spice_assert(bitmap); > - if (bitmap_fmt_is_rgb(bitmap->format)) { > - return ((bitmap->x * BITMAP_FMP_BYTES_PER_PIXEL[bitmap > ->format]) < bitmap->stride); > - } else { > - switch (bitmap->format) { > - case SPICE_BITMAP_FMT_8BIT: > - return (bitmap->x < bitmap->stride); > - case SPICE_BITMAP_FMT_4BIT_BE: > - case SPICE_BITMAP_FMT_4BIT_LE: { > - int bytes_width = SPICE_ALIGN(bitmap->x, 2) >> 1; > - return bytes_width < bitmap->stride; > - } > - case SPICE_BITMAP_FMT_1BIT_BE: > - case SPICE_BITMAP_FMT_1BIT_LE: { > - int bytes_width = SPICE_ALIGN(bitmap->x, 8) >> 3; > - return bytes_width < bitmap->stride; > - } > - default: > - spice_error("invalid image type %u", bitmap->format); > - return 0; > - } > - } > - return 0; > -} > - > typedef struct compress_send_data_t { > void* comp_buf; > uint32_t comp_buf_size; > @@ -5517,7 +5387,7 @@ static inline int > red_compress_image(DisplayChannelClient *dcc, > ((src->y * src->stride) < MIN_SIZE_TO_COMPRESS)) { // TODO: > change the size cond > return FALSE; > } else if (image_compression == SPICE_IMAGE_COMPRESSION_QUIC) { > - if (BITMAP_FMT_IS_PLT[src->format]) { > + if (bitmap_fmt_is_plt(src->format)) { > return FALSE; > } else { > quic_compress = TRUE; > @@ -5527,11 +5397,11 @@ static inline int > red_compress_image(DisplayChannelClient *dcc, > lz doesn't handle (1) bitmaps with strides that are > larger than the width > of the image in bytes (2) unstable bitmaps > */ > - if (_stride_is_extra(src) || (src->data->flags & > SPICE_CHUNKS_FLAGS_UNSTABLE)) { > + 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]) { > + bitmap_fmt_is_plt(src->format)) { > return FALSE; > } else { > quic_compress = TRUE; > @@ -5543,10 +5413,8 @@ static inline int > red_compress_image(DisplayChannelClient *dcc, > quic_compress = FALSE; > } else { > if (drawable->copy_bitmap_graduality == > BITMAP_GRADUAL_INVALID) { > - quic_compress = > BITMAP_FMT_HAS_GRADUALITY(src->format) && > - > (_get_bitmap_graduality_level(display_channel->common.worker, src, > - drawable > ->group_id) == > - BITMAP_GRADUAL_HIGH); > + 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); > } > @@ -5566,7 +5434,7 @@ static inline int > red_compress_image(DisplayChannelClient *dcc, > ((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 || > !_stride_is_extra(src)) { > + if (src->format != SPICE_BITMAP_FMT_RGBA || > !bitmap_has_extra_stride(src)) { > return red_jpeg_compress_image(dcc, dest, > src, o_comp_data, > drawable->group_id); > } > @@ -5578,7 +5446,7 @@ static inline int > red_compress_image(DisplayChannelClient *dcc, > int ret; > if ((image_compression == SPICE_IMAGE_COMPRESSION_AUTO_GLZ) > || > (image_compression == SPICE_IMAGE_COMPRESSION_GLZ)) { > - glz = BITMAP_FMT_HAS_GRADUALITY(src->format) && ( > + 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) || > @@ -7847,14 +7715,12 @@ static void > red_marshall_image(RedChannelClient *rcc, SpiceMarshaller *m, ImageI > comp_mode = display_channel->common.worker->image_compression; > > if (((comp_mode == SPICE_IMAGE_COMPRESSION_AUTO_LZ) || > - (comp_mode == SPICE_IMAGE_COMPRESSION_AUTO_GLZ)) && > !_stride_is_extra(&bitmap)) { > + (comp_mode == SPICE_IMAGE_COMPRESSION_AUTO_GLZ)) && > !bitmap_has_extra_stride(&bitmap)) { > > - if (BITMAP_FMT_HAS_GRADUALITY(item->image_format)) { > + if (bitmap_fmt_has_graduality(item->image_format)) { > BitmapGradualType grad_level; > > - grad_level = > _get_bitmap_graduality_level(display_channel->common.worker, > - &bitmap, > - worker > ->mem_slots.internal_groupslot_id); > + grad_level = bitmap_get_graduality_level(&bitmap); > if (grad_level == BITMAP_GRADUAL_HIGH) { > // if we use lz for alpha, the stride can't be extra > lossy_comp = display_channel->enable_jpeg && item > ->can_lossy; > diff --git a/server/spice-bitmap-utils.c b/server/spice-bitmap > -utils.c > new file mode 100644 > index 0000000..c4ec625 > --- /dev/null > +++ b/server/spice-bitmap-utils.c > @@ -0,0 +1,119 @@ > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */ > +/* > + Copyright (C) 2009-2015 Red Hat, Inc. > + > + This library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later > version. > + > + This library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with this library; if not, see < > http://www.gnu.org/licenses/>. > +*/ > +#ifdef HAVE_CONFIG_H > +#include <config.h> > +#endif > +#include "spice-bitmap-utils.h" > + > +#define RED_BITMAP_UTILS_RGB16 > +#include "red_bitmap_utils_tmpl.c" > +#define RED_BITMAP_UTILS_RGB24 > +#include "red_bitmap_utils_tmpl.c" > +#define RED_BITMAP_UTILS_RGB32 > +#include "red_bitmap_utils_tmpl.c" > + > +#define GRADUAL_HIGH_RGB24_TH -0.03 > +#define GRADUAL_HIGH_RGB16_TH 0 > + > +// setting a more permissive threshold for stream identification in > order > +// not to miss streams that were artificially scaled on the guest > (e.g., full screen view > +// in window media player 12). see red_stream_add_frame > +#define GRADUAL_MEDIUM_SCORE_TH 0.002 > + > +// assumes that stride doesn't overflow > +BitmapGradualType bitmap_get_graduality_level(SpiceBitmap *bitmap) > +{ > + double score = 0.0; > + int num_samples = 0; > + int num_lines; > + double chunk_score = 0.0; > + int chunk_num_samples = 0; > + uint32_t x, i; > + SpiceChunk *chunk; > + > + chunk = bitmap->data->chunk; > + for (i = 0; i < bitmap->data->num_chunks; i++) { > + num_lines = chunk[i].len / bitmap->stride; > + x = bitmap->x; > + switch (bitmap->format) { > + case SPICE_BITMAP_FMT_16BIT: > + compute_lines_gradual_score_rgb16((rgb16_pixel_t > *)chunk[i].data, x, num_lines, > + &chunk_score, > &chunk_num_samples); > + break; > + case SPICE_BITMAP_FMT_24BIT: > + compute_lines_gradual_score_rgb24((rgb24_pixel_t > *)chunk[i].data, x, num_lines, > + &chunk_score, > &chunk_num_samples); > + break; > + case SPICE_BITMAP_FMT_32BIT: > + case SPICE_BITMAP_FMT_RGBA: > + compute_lines_gradual_score_rgb32((rgb32_pixel_t > *)chunk[i].data, x, num_lines, > + &chunk_score, > &chunk_num_samples); > + break; > + default: > + spice_error("invalid bitmap format (not RGB) %u", bitmap > ->format); > + } > + score += chunk_score; > + num_samples += chunk_num_samples; > + } > + > + spice_assert(num_samples); > + score /= num_samples; > + > + if (bitmap->format == SPICE_BITMAP_FMT_16BIT) { > + if (score < GRADUAL_HIGH_RGB16_TH) { > + return BITMAP_GRADUAL_HIGH; > + } > + } else { > + if (score < GRADUAL_HIGH_RGB24_TH) { > + return BITMAP_GRADUAL_HIGH; > + } > + } > + > + if (score < GRADUAL_MEDIUM_SCORE_TH) { > + return BITMAP_GRADUAL_MEDIUM; > + } else { > + return BITMAP_GRADUAL_LOW; > + } > +} > + > +int bitmap_has_extra_stride(SpiceBitmap *bitmap) > +{ > + spice_assert(bitmap); > + if (bitmap_fmt_is_rgb(bitmap->format)) { > + return ((bitmap->x * bitmap_fmt_get_bytes_per_pixel(bitmap > ->format)) < bitmap->stride); > + } else { > + switch (bitmap->format) { > + case SPICE_BITMAP_FMT_8BIT: > + return (bitmap->x < bitmap->stride); > + case SPICE_BITMAP_FMT_4BIT_BE: > + case SPICE_BITMAP_FMT_4BIT_LE: { > + int bytes_width = SPICE_ALIGN(bitmap->x, 2) >> 1; > + return bytes_width < bitmap->stride; > + } > + case SPICE_BITMAP_FMT_1BIT_BE: > + case SPICE_BITMAP_FMT_1BIT_LE: { > + int bytes_width = SPICE_ALIGN(bitmap->x, 8) >> 3; > + return bytes_width < bitmap->stride; > + } > + default: > + spice_error("invalid image type %u", bitmap->format); > + return 0; > + } > + } > + return 0; > +} > diff --git a/server/spice-bitmap-utils.h b/server/spice-bitmap > -utils.h > new file mode 100644 > index 0000000..38cb88a > --- /dev/null > +++ b/server/spice-bitmap-utils.h > @@ -0,0 +1,91 @@ > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */ > +/* > + Copyright (C) 2009-2015 Red Hat, Inc. > + > + This library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later > version. > + > + This library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with this library; if not, see < > http://www.gnu.org/licenses/>. > +*/ > +#ifndef RED_BITMAP_UTILS_H_ > +# define RED_BITMAP_UTILS_H_ > + > +#include <glib.h> > +#include <stdint.h> > +#include "common/draw.h" > +#include "common/log.h" > + > +typedef enum { > + BITMAP_GRADUAL_INVALID, > + BITMAP_GRADUAL_NOT_AVAIL, > + BITMAP_GRADUAL_LOW, > + BITMAP_GRADUAL_MEDIUM, > + BITMAP_GRADUAL_HIGH, > +} BitmapGradualType; > + > +typedef struct { > + uint8_t b; > + uint8_t g; > + uint8_t r; > + uint8_t pad; > +} rgb32_pixel_t; > + > +G_STATIC_ASSERT(sizeof(rgb32_pixel_t) == 4); > + > +typedef struct { > + uint8_t b; > + uint8_t g; > + uint8_t r; > +} rgb24_pixel_t; > + > +G_STATIC_ASSERT(sizeof(rgb24_pixel_t) == 3); > + > +typedef uint16_t rgb16_pixel_t; > + > + > +static inline int bitmap_fmt_get_bytes_per_pixel(uint8_t fmt) > +{ > + static const int bytes_per_pixel[] = {0, 0, 0, 0, 0, 1, 2, 3, 4, > 4, 1}; > + > + spice_return_val_if_fail(fmt < > SPICE_N_ELEMENTS(bytes_per_pixel), 0); > + > + return bytes_per_pixel[fmt]; > +} > + > + > +static inline int bitmap_fmt_is_plt(uint8_t fmt) > +{ > + static const int fmt_is_plt[] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, > 0}; > + > + spice_return_val_if_fail(fmt < SPICE_N_ELEMENTS(fmt_is_plt), 0); > + > + return fmt_is_plt[fmt]; > +} > + > +static inline int bitmap_fmt_is_rgb(uint8_t fmt) > +{ > + static const int fmt_is_rgb[] = {0, 0, 0, 0, 0, 0, 1, 1, 1, 1, > 1}; > + > + spice_return_val_if_fail(fmt < SPICE_N_ELEMENTS(fmt_is_rgb), 0); > + > + return fmt_is_rgb[fmt]; > +} > + > +static inline int bitmap_fmt_has_graduality(uint8_t fmt) > +{ > + return bitmap_fmt_is_rgb(fmt) && fmt != SPICE_BITMAP_FMT_8BIT_A; > +} > + > + > +BitmapGradualType bitmap_get_graduality_level (SpiceBitmap > *bitmap); > +int bitmap_has_extra_stride (SpiceBitmap > *bitmap); > + > +#endif /* RED_BITMAP_UTILS_H_ */ > diff --git a/server/tree.h b/server/tree.h > index 8cd7b05..77f1fbf 100644 > --- a/server/tree.h > +++ b/server/tree.h > @@ -21,6 +21,7 @@ > #include <stdint.h> > #include "common/region.h" > #include "common/ring.h" > +#include "spice-bitmap-utils.h" > > enum { > TREE_ITEM_TYPE_NONE, > @@ -64,14 +65,6 @@ struct DrawItem { > > #define IS_DRAW_ITEM(item) ((item)->type == TREE_ITEM_TYPE_DRAWABLE) > > -typedef enum { > - BITMAP_GRADUAL_INVALID, > - BITMAP_GRADUAL_NOT_AVAIL, > - BITMAP_GRADUAL_LOW, > - BITMAP_GRADUAL_MEDIUM, > - BITMAP_GRADUAL_HIGH, > -} BitmapGradualType; > - > typedef struct DependItem { > Drawable *drawable; > RingItem ring_item; ACK from me. I almost want to suggest changing some of these functions from an int return type to gboolean while we're moving them, but I don't know if it's really important enough to (potentially) introduce rebasing conflicts. Also, it's probably worth mentioning in the commit log that we're removing some unused parameters from bitmap_get_graduality_level() Jonathon _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel