[Spice-devel,1/3] Add support for LZ compression of A8 images

Submitted by Søren Sandmann Pedersen on Aug. 19, 2012, 8:53 p.m.

Details

Message ID 1345409627-28721-2-git-send-email-ssp@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Søren Sandmann Pedersen Aug. 19, 2012, 8:53 p.m.
Graduality is irrelevant for A8 images, so instead of using RGB-ness
as a short-cut, add a new macro BITMAP_FMT_HAS_GRADUALITY() that
returns true for the existing RGB images, but false for A8.
---
 server/red_common.h    |  2 +-
 server/red_parse_qxl.c |  2 +-
 server/red_worker.c    | 23 +++++++++++++++--------
 spice-common           |  2 +-
 4 files changed, 18 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/red_common.h b/server/red_common.h
index b52a7d1..478b56a 100644
--- a/server/red_common.h
+++ b/server/red_common.h
@@ -38,7 +38,7 @@  enum {
 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};
+	                                {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");
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index 3bf49a0..2953e80 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -329,7 +329,7 @@  static SpiceChunks *red_get_image_data_chunked(RedMemSlotInfo *slots, int group_
 
 // This is based on SPICE_BITMAP_FMT_*, copied from server/red_worker.c
 // to avoid a possible unoptimization from making it non static.
-static const int BITMAP_FMT_IS_RGB[] = {0, 0, 0, 0, 0, 0, 1, 1, 1, 1};
+static const int BITMAP_FMT_IS_RGB[] = {0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1};
 
 static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
                                  QXLPHYSICAL addr, uint32_t flags)
diff --git a/server/red_worker.c b/server/red_worker.c
index 9330fff..59749f9 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -445,8 +445,12 @@  struct RedCompressBuf {
     RedCompressBuf *send_next;
 };
 
-static const int BITMAP_FMT_IS_PLT[] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0};
-static const int BITMAP_FMP_BYTES_PER_PIXEL[] = {0, 0, 0, 0, 0, 1, 2, 3, 4, 4};
+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))
 
 pthread_mutex_t cache_lock = PTHREAD_MUTEX_INITIALIZER;
 Ring pixmap_cache_list = {&pixmap_cache_list, &pixmap_cache_list};
@@ -3111,7 +3115,7 @@  static inline void red_update_copy_graduality(RedWorker* worker, Drawable *drawa
 
     bitmap = &drawable->red_drawable->u.copy.src_bitmap->u.bitmap;
 
-    if (!bitmap_fmt_is_rgb(bitmap->format) || _stride_is_extra(bitmap) ||
+    if (!BITMAP_FMT_HAS_GRADUALITY(bitmap->format) || _stride_is_extra(bitmap) ||
         (bitmap->data->flags & SPICE_CHUNKS_FLAGS_UNSTABLE)) {
         drawable->copy_bitmap_graduality = BITMAP_GRADUAL_NOT_AVAIL;
     } else  {
@@ -3671,6 +3675,8 @@  static int surface_format_to_image_type(uint32_t surface_format)
         return SPICE_BITMAP_FMT_32BIT;
     case SPICE_SURFACE_FMT_32_ARGB:
         return SPICE_BITMAP_FMT_RGBA;
+    case SPICE_SURFACE_FMT_8_A:
+        return SPICE_BITMAP_FMT_8BIT_A;
     default:
         spice_critical("Unsupported surface format");
     }
@@ -5882,7 +5888,8 @@  static const LzImageType MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[] = {
     LZ_IMAGE_TYPE_RGB16,
     LZ_IMAGE_TYPE_RGB24,
     LZ_IMAGE_TYPE_RGB32,
-    LZ_IMAGE_TYPE_RGBA
+    LZ_IMAGE_TYPE_RGBA,
+    LZ_IMAGE_TYPE_A8
 };
 
 typedef struct compress_send_data_t {
@@ -6344,8 +6351,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_is_rgb(src->format) &&
-                            (_get_bitmap_graduality_level(display_channel->common.worker, src,
+                        quic_compress = BITMAP_FMT_HAS_GRADUALITY(src->format) &&
+			    (_get_bitmap_graduality_level(display_channel->common.worker, src,
                                                           drawable->group_id) ==
                              BITMAP_GRADUAL_HIGH);
                     } else {
@@ -6379,7 +6386,7 @@  static inline int red_compress_image(DisplayChannelClient *dcc,
         int ret;
         if ((image_compression == SPICE_IMAGE_COMPRESS_AUTO_GLZ) ||
             (image_compression == SPICE_IMAGE_COMPRESS_GLZ)) {
-            glz = bitmap_fmt_is_rgb(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_COMPRESS_AUTO_LZ) ||
@@ -8452,7 +8459,7 @@  static void red_marshall_image(RedChannelClient *rcc, SpiceMarshaller *m, ImageI
 
     if ((comp_mode == SPICE_IMAGE_COMPRESS_AUTO_LZ) ||
         (comp_mode == SPICE_IMAGE_COMPRESS_AUTO_GLZ)) {
-        if (bitmap_fmt_is_rgb(item->image_format)) {
+        if (BITMAP_FMT_HAS_GRADUALITY(item->image_format)) {
             if (!_stride_is_extra(&bitmap)) {
                 BitmapGradualType grad_level;
                 grad_level = _get_bitmap_graduality_level(display_channel->common.worker,
diff --git a/spice-common b/spice-common
index c2adbb0..f7f5b60 160000
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@ 
-Subproject commit c2adbb00dc0b29de0fe297f241fb0efeb4a81510
+Subproject commit f7f5b603f2ea078b224caf22ebfb3ddd4222fab6

Comments

Hi,

On 08/19/2012 10:53 PM, Søren Sandmann Pedersen wrote:
> Graduality is irrelevant for A8 images, so instead of using RGB-ness
> as a short-cut, add a new macro BITMAP_FMT_HAS_GRADUALITY() that
> returns true for the existing RGB images, but false for A8.
> ---
>   server/red_common.h    |  2 +-
>   server/red_parse_qxl.c |  2 +-
>   server/red_worker.c    | 23 +++++++++++++++--------
>   spice-common           |  2 +-
>   4 files changed, 18 insertions(+), 11 deletions(-)
>

<snip>

> diff --git a/server/red_worker.c b/server/red_worker.c
> index 9330fff..59749f9 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c

<snip>

> @@ -5882,7 +5888,8 @@ static const LzImageType MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[] = {
>       LZ_IMAGE_TYPE_RGB16,
>       LZ_IMAGE_TYPE_RGB24,
>       LZ_IMAGE_TYPE_RGB32,
> -    LZ_IMAGE_TYPE_RGBA
> +    LZ_IMAGE_TYPE_RGBA,
> +    LZ_IMAGE_TYPE_A8
>   };
>
>   typedef struct compress_send_data_t {

Here you are using A8 again, versus 8BIT_A in some other patches, please pick one and
be consistent about it. Other then that this and the other 2 patches look good.

<snip>

Regards,

Hans
Hans de Goede <hdegoede@redhat.com> writes:

>> @@ -5882,7 +5888,8 @@ static const LzImageType MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[] = {
>>       LZ_IMAGE_TYPE_RGB16,
>>       LZ_IMAGE_TYPE_RGB24,
>>       LZ_IMAGE_TYPE_RGB32,
>> -    LZ_IMAGE_TYPE_RGBA
>> +    LZ_IMAGE_TYPE_RGBA,
>> +    LZ_IMAGE_TYPE_A8
>>   };
>>
>>   typedef struct compress_send_data_t {
>
> Here you are using A8 again, versus 8BIT_A in some other patches, please pick one and
> be consistent about it. Other then that this and the other 2 patches
> look good.

Note that there are two enums here, and the I chose the names to be
consistent with the existing entries. One is this:

    SPICE_BITMAP_FMT_INVALID,
    SPICE_BITMAP_FMT_1BIT_LE,
    SPICE_BITMAP_FMT_1BIT_BE,
    SPICE_BITMAP_FMT_4BIT_LE,
    SPICE_BITMAP_FMT_4BIT_BE,
    SPICE_BITMAP_FMT_8BIT,
    SPICE_BITMAP_FMT_16BIT,
    SPICE_BITMAP_FMT_24BIT,
    SPICE_BITMAP_FMT_32BIT,
    SPICE_BITMAP_FMT_RGBA,
    SPICE_BITMAP_FMT_8BIT_A,

where I chose to use 8BIT_A to be consistent with the rest of the
entries (but distinct from 8BIT). The other is this:

    LZ_IMAGE_TYPE_INVALID,
    LZ_IMAGE_TYPE_PLT1_LE,
    LZ_IMAGE_TYPE_PLT1_BE,      // PLT stands for palette
    LZ_IMAGE_TYPE_PLT4_LE,
    LZ_IMAGE_TYPE_PLT4_BE,
    LZ_IMAGE_TYPE_PLT8,
    LZ_IMAGE_TYPE_RGB16,
    LZ_IMAGE_TYPE_RGB24,
    LZ_IMAGE_TYPE_RGB32,
    LZ_IMAGE_TYPE_RGBA,
    LZ_IMAGE_TYPE_XXXA,
    LZ_IMAGE_TYPE_A8

where A8 seemed most consistent. 

You're right that for the BITMAP_FMT enum, the first patch had A8 and
the second changed it to 8BIT_A. I have fixed that in the following
patch series along with your other comments.


Thanks for the review,
Søren
ssia
ssia
ssia
ssia
Series looks good, ack.

On 08/24/2012 02:59 AM, Søren Sandmann Pedersen wrote:
> ssia
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
Series looks good, ack.

On 08/24/2012 03:00 AM, Søren Sandmann Pedersen wrote:
> ssia
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
Series looks good, ack series.

On 08/24/2012 03:02 AM, Søren Sandmann Pedersen wrote:
> ssia
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>