[Spice-devel] [PATCH 16/18] worker: move drawable_draw
Frediano Ziglio
fziglio at redhat.com
Mon Nov 23 05:28:15 PST 2015
> Reduced diff
>
>
> --- before.c 2015-11-23 13:20:37.534895725 +0000
> +++ after.c 2015-11-23 13:12:44.527503563 +0000
> @@ -1034,53 +1034,53 @@
> drawable->tree_item.base.type = TREE_ITEM_TYPE_DRAWABLE;
> region_init(&drawable->tree_item.base.rgn);
> ring_init(&drawable->pipes);
> ring_init(&drawable->glz_ring);
> drawable->process_commands_generation = process_commands_generation;
> drawable->group_id = group_id;
>
> return drawable;
> }
>
> -static void remove_depended_item(DependItem *item)
> +static void depended_item_remove(DependItem *item)
> {
> - spice_assert(item->drawable);
> - spice_assert(ring_item_is_linked(&item->ring_item));
> + spice_return_if_fail(item->drawable);
> + spice_return_if_fail(ring_item_is_linked(&item->ring_item));
>
> item->drawable = NULL;
> ring_remove(&item->ring_item);
> }
Didn't spot this previously.
>
> -static void drawable_unref_surface_deps(DisplayChannel *display, Drawable
> *drawable)
> +static void drawable_remove_dependencies(DisplayChannel *display, Drawable
> *drawable)
> {
> int x;
> int surface_id;
>
> for (x = 0; x < 3; ++x) {
> surface_id = drawable->surface_deps[x];
> - if (surface_id == -1) {
> - continue;
> + if (surface_id != -1 && drawable->depend_items[x].drawable) {
> + depended_item_remove(&drawable->depend_items[x]);
> }
> - display_channel_surface_unref(display, surface_id);
> }
> }
>
> -static void drawable_remove_dependencies(DisplayChannel *display, Drawable
> *drawable)
> +static void drawable_unref_surface_deps(DisplayChannel *display, Drawable
> *drawable)
> {
> int x;
> int surface_id;
>
> for (x = 0; x < 3; ++x) {
> surface_id = drawable->surface_deps[x];
> - if (surface_id != -1 && drawable->depend_items[x].drawable) {
> - remove_depended_item(&drawable->depend_items[x]);
> + if (surface_id == -1) {
> + continue;
> }
> + display_channel_surface_unref(display, surface_id);
> }
> }
>
Where is gone the call to display_channel_surface_unref?
This was replaced by remove_depended_item which does not change surface references.
However from this patch the the unreference is removed but the reference is still there.
Considering that usually there are only very few surface 0 allocations (as other surfaces
are usually not used) this is hard to spot (would be easier to detect with a proper
cleanup and leak analysis... not present at the moment!).
If this is true would be possible to possibly increment the counter till is reach zero
using overflow and causing memory corruptions.
> void display_channel_drawable_unref(DisplayChannel *display, Drawable
> *drawable)
> {
> RingItem *item, *next;
>
> if (--drawable->refs != 0)
> return;
>
> @@ -1093,51 +1093,48 @@
> region_destroy(&drawable->tree_item.base.rgn);
>
> drawable_remove_dependencies(display, drawable);
> drawable_unref_surface_deps(display, drawable);
> display_channel_surface_unref(display, drawable->surface_id);
>
> RING_FOREACH_SAFE(item, next, &drawable->glz_ring) {
> SPICE_CONTAINEROF(item, RedGlzDrawable, drawable_link)->drawable =
> NULL;
> ring_remove(item);
> }
> + if (drawable->red_drawable) {
> red_drawable_unref(COMMON_CHANNEL(display)->worker,
> drawable->red_drawable, drawable->group_id);
> + }
> drawable_free(display, drawable);
> display->drawable_count--;
> }
>
> -static void surface_flush(DisplayChannel *display, int surface_id, SpiceRect
> *rect)
> -{
> - red_update_area(display, rect, surface_id);
> -}
> -
> -static void red_flush_source_surfaces(DisplayChannel *display, Drawable
> *drawable)
> +static void drawable_deps_update(DisplayChannel *display, Drawable
> *drawable)
> {
> int x;
> int surface_id;
>
> for (x = 0; x < 3; ++x) {
> surface_id = drawable->surface_deps[x];
> if (surface_id != -1 && drawable->depend_items[x].drawable) {
> - remove_depended_item(&drawable->depend_items[x]);
> - surface_flush(display, surface_id,
> &drawable->red_drawable->surfaces_rects[x]);
> + depended_item_remove(&drawable->depend_items[x]);
> + red_update_area(display,
> &drawable->red_drawable->surfaces_rects[x], surface_id);
> }
> }
> }
>
> void drawable_draw(DisplayChannel *display, Drawable *drawable)
> {
> RedSurface *surface;
> SpiceCanvas *canvas;
> SpiceClip clip = drawable->red_drawable->clip;
>
> - red_flush_source_surfaces(display, drawable);
> + drawable_deps_update(display, drawable);
>
> surface = &display->surfaces[drawable->surface_id];
> canvas = surface->context.canvas;
> spice_return_if_fail(canvas);
>
> image_cache_aging(&display->image_cache);
>
> region_add(&surface->draw_dirty_region, &drawable->red_drawable->bbox);
>
> switch (drawable->red_drawable->type) {
> @@ -1653,20 +1650,24 @@
> region_add(rgn, data->rects + i);
> }
> }
>
> void current_remove_drawable(DisplayChannel *display, Drawable *item);
> void red_pipes_remove_drawable(Drawable *drawable);
> void current_remove(DisplayChannel *display, TreeItem *item);
> void detach_streams_behind(DisplayChannel *display, QRegion *region,
> Drawable *drawable);
> void drawable_draw(DisplayChannel *display, Drawable *item);
> void current_remove_all(DisplayChannel *display, int surface_id);
> +void drawables_init(DisplayChannel *display);
> +void red_update_area(DisplayChannel *display, const SpiceRect *area, int
> surface_id);
> +void red_update_area_till(DisplayChannel *display, const SpiceRect *area,
> int surface_id,
> + Drawable *last);
>
> #endif /* DISPLAY_CHANNEL_H_ */
> /* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> /*
> Copyright (C) 2009 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.
> @@ -1794,23 +1795,20 @@
> } BitmapDataType;
>
> typedef struct BitmapData {
> BitmapDataType type;
> uint64_t id; // surface id or cache item id
> SpiceRect lossy_rect;
> } BitmapData;
>
> static inline int validate_surface(DisplayChannel *display, uint32_t
> surface_id);
>
> -static void red_update_area(DisplayChannel *display, const SpiceRect *area,
> int surface_id);
> -static void red_update_area_till(DisplayChannel *display, const SpiceRect
> *area, int surface_id,
> - Drawable *last);
> static inline void display_begin_send_message(RedChannelClient *rcc);
> static void red_create_surface(DisplayChannel *display, uint32_t surface_id,
> uint32_t width,
> uint32_t height, int32_t stride, uint32_t
> format,
> void *line_0, int data_is_valid, int
> send_client);
>
> QXLInstance* red_worker_get_qxl(RedWorker *worker)
> {
> spice_return_val_if_fail(worker != NULL, NULL);
>
> return worker->qxl;
> @@ -2400,21 +2398,21 @@
> {
> 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;
> - surface_flush(display, drawable->surface_id,
> &drawable->red_drawable->bbox);
> + red_update_area(display, &drawable->red_drawable->bbox,
> drawable->surface_id);
> }
>
> return TRUE;
> }
>
> static inline void add_to_surface_dependency(DisplayChannel *display, int
> depend_on_surface_id,
> DependItem *depend_item,
> Drawable *drawable)
> {
> RedSurface *surface;
>
>
Maybe I'm wrong with the consideration but at the moment I would nack.
Frediano
More information about the Spice-devel
mailing list