[Spice-devel] [PATCH 17/18] display: move more logic in add_drawable()

Frediano Ziglio fziglio at redhat.com
Tue Nov 24 03:13:21 PST 2015


From: Marc-André Lureau <marcandre.lureau at gmail.com>

---
 server/display-channel.c | 229 +++++++++++++++++++++++++++++++++++++++--
 server/display-channel.h |  23 ++---
 server/red_worker.c      | 258 +++--------------------------------------------
 3 files changed, 243 insertions(+), 267 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index fce9dd5..9857e1e 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -20,6 +20,13 @@
 
 #include "display-channel.h"
 
+uint32_t generate_uid(DisplayChannel *display)
+{
+    spice_return_val_if_fail(display != NULL, 0);
+
+    return ++display->bits_unique;
+}
+
 static stat_time_t display_channel_stat_now(DisplayChannel *display)
 {
 #ifdef RED_WORKER_STAT
@@ -832,26 +839,236 @@ void display_channel_print_stats(DisplayChannel *display)
 #endif
 }
 
-void display_channel_add_drawable(DisplayChannel *display, Drawable *drawable)
+static void drawable_ref_surface_deps(DisplayChannel *display, Drawable *drawable)
+{
+    int x;
+    int surface_id;
+    RedSurface *surface;
+
+    for (x = 0; x < 3; ++x) {
+        surface_id = drawable->surface_deps[x];
+        if (surface_id == -1) {
+            continue;
+        }
+        surface = &display->surfaces[surface_id];
+        surface->refs++;
+    }
+}
+
+static void surface_read_bits(DisplayChannel *display, int surface_id,
+                              const SpiceRect *area, uint8_t *dest, int dest_stride)
+{
+    SpiceCanvas *canvas;
+    RedSurface *surface = &display->surfaces[surface_id];
+
+    canvas = surface->context.canvas;
+    canvas->ops->read_bits(canvas, dest, dest_stride, area);
+}
+
+static void handle_self_bitmap(DisplayChannel *display, Drawable *drawable)
 {
-    int success = FALSE, surface_id = drawable->surface_id;
     RedDrawable *red_drawable = drawable->red_drawable;
-    Ring *ring = &display->surfaces[surface_id].current;
+    SpiceImage *image;
+    int32_t width;
+    int32_t height;
+    uint8_t *dest;
+    int dest_stride;
+    RedSurface *surface;
+    int bpp;
+    int all_set;
+
+    surface = &display->surfaces[drawable->surface_id];
+
+    bpp = SPICE_SURFACE_FMT_DEPTH(surface->context.format) / 8;
+    width = red_drawable->self_bitmap_area.right - red_drawable->self_bitmap_area.left;
+    height = red_drawable->self_bitmap_area.bottom - red_drawable->self_bitmap_area.top;
+    dest_stride = SPICE_ALIGN(width * bpp, 4);
+
+    image = spice_new0(SpiceImage, 1);
+    image->descriptor.type = SPICE_IMAGE_TYPE_BITMAP;
+    image->descriptor.flags = 0;
+    QXL_SET_IMAGE_ID(image, QXL_IMAGE_GROUP_RED, generate_uid(display));
+    image->u.bitmap.flags = surface->context.top_down ? SPICE_BITMAP_FLAGS_TOP_DOWN : 0;
+    image->u.bitmap.format = spice_bitmap_from_surface_type(surface->context.format);
+    image->u.bitmap.stride = dest_stride;
+    image->descriptor.width = image->u.bitmap.x = width;
+    image->descriptor.height = image->u.bitmap.y = height;
+    image->u.bitmap.palette = NULL;
+
+    dest = (uint8_t *)spice_malloc_n(height, dest_stride);
+    image->u.bitmap.data = spice_chunks_new_linear(dest, height * dest_stride);
+    image->u.bitmap.data->flags |= SPICE_CHUNKS_FLAGS_FREE;
+
+    display_channel_draw(display, &red_drawable->self_bitmap_area, drawable->surface_id);
+    surface_read_bits(display, drawable->surface_id,
+        &red_drawable->self_bitmap_area, dest, dest_stride);
+
+    /* For 32bit non-primary surfaces we need to keep any non-zero
+       high bytes as the surface may be used as source to an alpha_blend */
+    if (!is_primary_surface(display, drawable->surface_id) &&
+        image->u.bitmap.format == SPICE_BITMAP_FMT_32BIT &&
+        rgb32_data_has_alpha(width, height, dest_stride, dest, &all_set)) {
+        if (all_set) {
+            image->descriptor.flags |= SPICE_IMAGE_FLAGS_HIGH_BITS_SET;
+        } else {
+            image->u.bitmap.format = SPICE_BITMAP_FMT_RGBA;
+        }
+    }
+
+    red_drawable->self_bitmap_image = image;
+}
+
+static void surface_add_reverse_dependency(DisplayChannel *display, int surface_id,
+                                           DependItem *depend_item, Drawable *drawable)
+{
+    RedSurface *surface;
+
+    if (surface_id == -1) {
+        depend_item->drawable = NULL;
+        return;
+    }
+
+    surface = &display->surfaces[surface_id];
+
+    depend_item->drawable = drawable;
+    ring_add(&surface->depend_on_me, &depend_item->ring_item);
+}
+
+static int handle_surface_deps(DisplayChannel *display, Drawable *drawable)
+{
+    int x;
+
+    for (x = 0; x < 3; ++x) {
+        // surface self dependency is handled by shadows in "current", or by
+        // handle_self_bitmap
+        if (drawable->surface_deps[x] != drawable->surface_id) {
+            surface_add_reverse_dependency(display, drawable->surface_deps[x],
+                                           &drawable->depend_items[x], drawable);
+
+            if (drawable->surface_deps[x] == 0) {
+                QRegion depend_region;
+                region_init(&depend_region);
+                region_add(&depend_region, &drawable->red_drawable->surfaces_rects[x]);
+                detach_streams_behind(display, &depend_region, NULL);
+            }
+        }
+    }
+
+    return TRUE;
+}
+
+static void draw_depend_on_me(DisplayChannel *display, uint32_t surface_id)
+{
+    RedSurface *surface;
+    RingItem *ring_item;
+
+    surface = &display->surfaces[surface_id];
+
+    while ((ring_item = ring_get_tail(&surface->depend_on_me))) {
+        Drawable *drawable;
+        DependItem *depended_item = SPICE_CONTAINEROF(ring_item, DependItem, ring_item);
+        drawable = depended_item->drawable;
+        display_channel_draw(display, &drawable->red_drawable->bbox, drawable->surface_id);
+    }
+}
+
+static int validate_drawable_bbox(DisplayChannel *display, RedDrawable *drawable)
+{
+        DrawContext *context;
+        uint32_t surface_id = drawable->surface_id;
+
+        /* surface_id must be validated before calling into
+         * validate_drawable_bbox
+         */
+        if (!validate_surface(display, drawable->surface_id)) {
+            return FALSE;
+        }
+        context = &display->surfaces[surface_id].context;
+
+        if (drawable->bbox.top < 0)
+                return FALSE;
+        if (drawable->bbox.left < 0)
+                return FALSE;
+        if (drawable->bbox.bottom < 0)
+                return FALSE;
+        if (drawable->bbox.right < 0)
+                return FALSE;
+        if (drawable->bbox.bottom > context->height)
+                return FALSE;
+        if (drawable->bbox.right > context->width)
+                return FALSE;
 
+        return TRUE;
+}
+
+
+int display_channel_add_drawable(DisplayChannel *display, Drawable *drawable, RedDrawable *red_drawable)
+{
+    int surface_id, x;
+
+    drawable->red_drawable = red_drawable_ref(red_drawable);
+    drawable->tree_item.effect = red_drawable->effect;
+    surface_id = drawable->surface_id = red_drawable->surface_id;
+    if (!validate_drawable_bbox(display, red_drawable))
+        return FALSE;
+    // FIXME here can leak if after we return!
+    display->surfaces[surface_id].refs++;
+    red_drawable->mm_time = reds_get_mm_time();
+
+    for (x = 0; x < 3; ++x) {
+        drawable->surface_deps[x] = red_drawable->surface_deps[x];
+        if (drawable->surface_deps[x] != -1
+            && !validate_surface(display, drawable->surface_deps[x]))
+            return FALSE;
+    }
+
+    region_add(&drawable->tree_item.base.rgn, &red_drawable->bbox);
+    if (red_drawable->clip.type == SPICE_CLIP_TYPE_RECTS) {
+        QRegion rgn;
+
+        region_init(&rgn);
+        region_add_clip_rects(&rgn, red_drawable->clip.rects);
+        region_and(&drawable->tree_item.base.rgn, &rgn);
+        region_destroy(&rgn);
+    }
+
+    if (region_is_empty(&drawable->tree_item.base.rgn))
+        return TRUE;
+
+    /*
+        surface->refs is affected by a drawable (that is
+        dependent on the surface) as long as the drawable is alive.
+        However, surface->depend_on_me is affected by a drawable only
+        as long as it is in the current tree (hasn't been rendered yet).
+    */
+    drawable_ref_surface_deps(display, drawable);
+
+    if (red_drawable->self_bitmap)
+        handle_self_bitmap(display, drawable);
+
+    draw_depend_on_me(display, surface_id);
+
+    if (!handle_surface_deps(display, drawable))
+        return FALSE;
+
+    Ring *ring = &display->surfaces[surface_id].current;
+    int add_to_pipe = FALSE;
     if (has_shadow(red_drawable)) {
-        success = current_add_with_shadow(display, ring, drawable);
+        add_to_pipe = current_add_with_shadow(display, ring, drawable);
     } else {
         drawable->streamable = drawable_can_stream(display, drawable);
-        success = current_add(display, ring, drawable);
+        add_to_pipe = current_add(display, ring, drawable);
     }
 
-    if (success)
+    if (add_to_pipe)
         pipes_add_drawable(display, drawable);
 
 #ifdef RED_WORKER_STAT
     if ((++display->add_count % 100) == 0)
         display_channel_print_stats(display);
 #endif
+
+    return TRUE;
 }
 
 int display_channel_wait_for_migrate_data(DisplayChannel *display)
diff --git a/server/display-channel.h b/server/display-channel.h
index 0d79463..4a79700 100644
--- a/server/display-channel.h
+++ b/server/display-channel.h
@@ -165,6 +165,7 @@ struct _Drawable {
 
 struct DisplayChannel {
     CommonChannel common; // Must be the first thing
+    uint32_t bits_unique;
 
     MonitorsConfig *monitors_config;
 
@@ -286,8 +287,9 @@ void                       display_channel_surface_unref             (DisplayCha
                                                                       uint32_t surface_id);
 bool                       display_channel_surface_has_canvas        (DisplayChannel *display,
                                                                       uint32_t surface_id);
-void                       display_channel_add_drawable              (DisplayChannel *display,
-                                                                      Drawable *drawable);
+int                        display_channel_add_drawable              (DisplayChannel *display,
+                                                                      Drawable *drawable,
+                                                                      RedDrawable *red_drawable);
 void                       display_channel_current_flush             (DisplayChannel *display,
                                                                       int surface_id);
 int                        display_channel_wait_for_migrate_data     (DisplayChannel *display);
@@ -299,6 +301,7 @@ void                       display_channel_destroy_surface_wait      (DisplayCha
 void                       display_channel_destroy_surfaces          (DisplayChannel *display);
 void                       display_channel_destroy_surface           (DisplayChannel *display,
                                                                       uint32_t surface_id);
+uint32_t                   display_channel_generate_uid              (DisplayChannel *display);
 
 static inline int validate_surface(DisplayChannel *display, uint32_t surface_id)
 {
@@ -430,21 +433,7 @@ static inline void region_add_clip_rects(QRegion *rgn, SpiceClipRects *data)
     }
 }
 
-static inline void draw_depend_on_me(DisplayChannel *display, uint32_t surface_id)
-{
-    RedSurface *surface;
-    RingItem *ring_item;
-
-    surface = &display->surfaces[surface_id];
-
-    while ((ring_item = ring_get_tail(&surface->depend_on_me))) {
-        Drawable *drawable;
-        DependItem *depended_item = SPICE_CONTAINEROF(ring_item, DependItem, ring_item);
-        drawable = depended_item->drawable;
-        display_channel_draw(display, &drawable->red_drawable->bbox, drawable->surface_id);
-    }
-}
-
+uint32_t generate_uid(DisplayChannel *display);
 void current_remove_drawable(DisplayChannel *display, Drawable *item);
 void red_pipes_remove_drawable(Drawable *drawable);
 void current_remove(DisplayChannel *display, TreeItem *item);
diff --git a/server/red_worker.c b/server/red_worker.c
index 0816f63..4cc9fe9 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -99,8 +99,6 @@ struct RedWorker {
     CursorChannel *cursor_channel;
     uint32_t cursor_poll_tries;
 
-    uint32_t bits_unique;
-
     RedMemSlotInfo mem_slots;
 
     SpiceImageCompression image_compression;
@@ -142,35 +140,6 @@ QXLInstance* red_worker_get_qxl(RedWorker *worker)
     return worker->qxl;
 }
 
-static int validate_drawable_bbox(DisplayChannel *display, RedDrawable *drawable)
-{
-        DrawContext *context;
-        uint32_t surface_id = drawable->surface_id;
-
-        /* surface_id must be validated before calling into
-         * validate_drawable_bbox
-         */
-        if (!validate_surface(display, drawable->surface_id)) {
-            return FALSE;
-        }
-        context = &display->surfaces[surface_id].context;
-
-        if (drawable->bbox.top < 0)
-                return FALSE;
-        if (drawable->bbox.left < 0)
-                return FALSE;
-        if (drawable->bbox.bottom < 0)
-                return FALSE;
-        if (drawable->bbox.right < 0)
-                return FALSE;
-        if (drawable->bbox.bottom > context->height)
-                return FALSE;
-        if (drawable->bbox.right > context->width)
-                return FALSE;
-
-        return TRUE;
-}
-
 static int display_is_connected(RedWorker *worker)
 {
     return (worker->display_channel && red_channel_is_connected(
@@ -489,227 +458,28 @@ static void display_channel_streams_timeout(DisplayChannel *display)
     }
 }
 
-static void red_get_area(DisplayChannel *display, int surface_id, const SpiceRect *area,
-                         uint8_t *dest, int dest_stride, int update)
-{
-    SpiceCanvas *canvas;
-    RedSurface *surface;
-
-    surface = &display->surfaces[surface_id];
-    if (update) {
-        display_channel_draw(display, area, surface_id);
-    }
-
-    canvas = surface->context.canvas;
-    canvas->ops->read_bits(canvas, dest, dest_stride, area);
-}
-
-static inline int red_handle_self_bitmap(RedWorker *worker, Drawable *drawable)
-{
-    DisplayChannel *display = worker->display_channel;
-    SpiceImage *image;
-    int32_t width;
-    int32_t height;
-    uint8_t *dest;
-    int dest_stride;
-    RedSurface *surface;
-    int bpp;
-    int all_set;
-    RedDrawable *red_drawable = drawable->red_drawable;
-
-    if (!red_drawable->self_bitmap) {
-        return TRUE;
-    }
-
-    surface = &display->surfaces[drawable->surface_id];
-
-    bpp = SPICE_SURFACE_FMT_DEPTH(surface->context.format) / 8;
-
-    width = red_drawable->self_bitmap_area.right
-            - red_drawable->self_bitmap_area.left;
-    height = red_drawable->self_bitmap_area.bottom
-            - red_drawable->self_bitmap_area.top;
-    dest_stride = SPICE_ALIGN(width * bpp, 4);
-
-    image = spice_new0(SpiceImage, 1);
-    image->descriptor.type = SPICE_IMAGE_TYPE_BITMAP;
-    image->descriptor.flags = 0;
-
-    QXL_SET_IMAGE_ID(image, QXL_IMAGE_GROUP_RED, ++worker->bits_unique);
-    image->u.bitmap.flags = surface->context.top_down ? SPICE_BITMAP_FLAGS_TOP_DOWN : 0;
-    image->u.bitmap.format = spice_bitmap_from_surface_type(surface->context.format);
-    image->u.bitmap.stride = dest_stride;
-    image->descriptor.width = image->u.bitmap.x = width;
-    image->descriptor.height = image->u.bitmap.y = height;
-    image->u.bitmap.palette = NULL;
-
-    dest = (uint8_t *)spice_malloc_n(height, dest_stride);
-    image->u.bitmap.data = spice_chunks_new_linear(dest, height * dest_stride);
-    image->u.bitmap.data->flags |= SPICE_CHUNKS_FLAGS_FREE;
-
-    red_get_area(display, drawable->surface_id,
-                 &red_drawable->self_bitmap_area, dest, dest_stride, TRUE);
-
-    /* For 32bit non-primary surfaces we need to keep any non-zero
-       high bytes as the surface may be used as source to an alpha_blend */
-    if (!is_primary_surface(display, drawable->surface_id) &&
-        image->u.bitmap.format == SPICE_BITMAP_FMT_32BIT &&
-        rgb32_data_has_alpha(width, height, dest_stride, dest, &all_set)) {
-        if (all_set) {
-            image->descriptor.flags |= SPICE_IMAGE_FLAGS_HIGH_BITS_SET;
-        } else {
-            image->u.bitmap.format = SPICE_BITMAP_FMT_RGBA;
-        }
-    }
-
-    red_drawable->self_bitmap_image = image;
-    return TRUE;
-}
-
-static Drawable *get_drawable(RedWorker *worker, uint8_t effect, RedDrawable *red_drawable,
-                              uint32_t group_id)
+static void red_process_draw(RedWorker *worker, RedDrawable *red_drawable,
+                             uint32_t group_id)
 {
     DisplayChannel *display = worker->display_channel;
     Drawable *drawable;
-    int x;
-
-    if (!validate_drawable_bbox(display, red_drawable)) {
-        return NULL;
-    }
-    for (x = 0; x < 3; ++x) {
-        if (red_drawable->surface_deps[x] != -1
-            && !validate_surface(display, red_drawable->surface_deps[x])) {
-            return NULL;
-        }
-    }
-
-    drawable = display_channel_drawable_try_new(display, group_id, worker->process_commands_generation);
-    if (!drawable) {
-        return NULL;
-    }
-
-    drawable->tree_item.effect = effect;
-    drawable->red_drawable = red_drawable_ref(red_drawable);
-
-    drawable->surface_id = red_drawable->surface_id;
-    memcpy(drawable->surface_deps, red_drawable->surface_deps, sizeof(drawable->surface_deps));
-
-    return drawable;
-}
-
-static inline void add_to_surface_dependency(DisplayChannel *display, int depend_on_surface_id,
-                                             DependItem *depend_item, Drawable *drawable)
-{
-    RedSurface *surface;
-
-    if (depend_on_surface_id == -1) {
-        depend_item->drawable = NULL;
-        return;
-    }
-
-    surface = &display->surfaces[depend_on_surface_id];
-
-    depend_item->drawable = drawable;
-    ring_add(&surface->depend_on_me, &depend_item->ring_item);
-}
-
-static inline int red_handle_surfaces_dependencies(DisplayChannel *display, Drawable *drawable)
-{
-    int x;
-
-    for (x = 0; x < 3; ++x) {
-        // surface self dependency is handled by shadows in "current", or by
-        // handle_self_bitmap
-        if (drawable->surface_deps[x] != drawable->surface_id) {
-            add_to_surface_dependency(display, drawable->surface_deps[x],
-                                      &drawable->depend_items[x], drawable);
-
-            if (drawable->surface_deps[x] == 0) {
-                QRegion depend_region;
-                region_init(&depend_region);
-                region_add(&depend_region, &drawable->red_drawable->surfaces_rects[x]);
-                detach_streams_behind(display, &depend_region, NULL);
-            }
-        }
-    }
-
-    return TRUE;
-}
-
-static inline void red_inc_surfaces_drawable_dependencies(DisplayChannel *display, Drawable *drawable)
-{
-    int x;
-    int surface_id;
-    RedSurface *surface;
-
-    for (x = 0; x < 3; ++x) {
-        surface_id = drawable->surface_deps[x];
-        if (surface_id == -1) {
-            continue;
-        }
-        surface = &display->surfaces[surface_id];
-        surface->refs++;
-    }
-}
-
-static inline void red_process_draw(RedWorker *worker, RedDrawable *red_drawable,
-                                    uint32_t group_id)
-{
-    DisplayChannel *display = worker->display_channel;
-    int surface_id;
-    Drawable *drawable = get_drawable(worker, red_drawable->effect, red_drawable, group_id);
+    bool success = FALSE;
 
+    drawable = display_channel_drawable_try_new(display, group_id,
+                                                worker->process_commands_generation);
     if (!drawable) {
         return;
     }
 
-    red_drawable->mm_time = reds_get_mm_time();
-    surface_id = drawable->surface_id;
-
-    display->surfaces[surface_id].refs++;
-
-    region_add(&drawable->tree_item.base.rgn, &red_drawable->bbox);
-
-    if (red_drawable->clip.type == SPICE_CLIP_TYPE_RECTS) {
-        QRegion rgn;
-
-        region_init(&rgn);
-        region_add_clip_rects(&rgn, red_drawable->clip.rects);
-        region_and(&drawable->tree_item.base.rgn, &rgn);
-        region_destroy(&rgn);
-    }
-
-    /*
-        surface->refs is affected by a drawable (that is
-        dependent on the surface) as long as the drawable is alive.
-        However, surface->depend_on_me is affected by a drawable only
-        as long as it is in the current tree (hasn't been rendered yet).
-    */
-    red_inc_surfaces_drawable_dependencies(worker->display_channel, drawable);
+    success = display_channel_add_drawable(worker->display_channel, drawable, red_drawable);
+    spice_warn_if_fail(success);
 
-    if (region_is_empty(&drawable->tree_item.base.rgn)) {
-        goto cleanup;
-    }
-
-    if (!red_handle_self_bitmap(worker, drawable)) {
-        goto cleanup;
-    }
-
-    draw_depend_on_me(display, surface_id);
-
-    if (!red_handle_surfaces_dependencies(worker->display_channel, drawable)) {
-        goto cleanup;
-    }
-
-    display_channel_add_drawable(worker->display_channel, drawable);
-
-cleanup:
     display_channel_drawable_unref(display, drawable);
 }
 
 
-static inline void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface,
-                                       uint32_t group_id, int loadvm)
+static void red_process_surface(RedWorker *worker, RedSurfaceCmd *surface,
+                                uint32_t group_id, int loadvm)
 {
     DisplayChannel *display = worker->display_channel;
     uint32_t surface_id;
@@ -2965,7 +2735,7 @@ static void display_channel_marshall_reset_cache(RedChannelClient *rcc,
 static void red_marshall_image(RedChannelClient *rcc, SpiceMarshaller *m, ImageItem *item)
 {
     DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
-    DisplayChannel *display_channel = DCC_TO_DC(dcc);
+    DisplayChannel *display = DCC_TO_DC(dcc);
     SpiceImage red_image;
     RedWorker *worker;
     SpiceBitmap bitmap;
@@ -2979,10 +2749,10 @@ static void red_marshall_image(RedChannelClient *rcc, SpiceMarshaller *m, ImageI
     SpiceMarshaller *src_bitmap_out, *mask_bitmap_out;
     SpiceMarshaller *bitmap_palette_out, *lzplt_palette_out;
 
-    spice_assert(rcc && display_channel && item);
-    worker = display_channel->common.worker;
+    spice_assert(rcc && display && item);
+    worker = display->common.worker;
 
-    QXL_SET_IMAGE_ID(&red_image, QXL_IMAGE_GROUP_RED, ++worker->bits_unique);
+    QXL_SET_IMAGE_ID(&red_image, QXL_IMAGE_GROUP_RED, generate_uid(display));
     red_image.descriptor.type = SPICE_IMAGE_TYPE_BITMAP;
     red_image.descriptor.flags = item->image_flags;
     red_image.descriptor.width = item->width;
@@ -3039,7 +2809,7 @@ static void red_marshall_image(RedChannelClient *rcc, SpiceMarshaller *m, ImageI
             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;
+                lossy_comp = display->enable_jpeg && item->can_lossy;
                 quic_comp = TRUE;
             }
         }
-- 
2.4.3



More information about the Spice-devel mailing list