Re: [PATCH 07/19] display: move more logic in add_drawable()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> On Wed, Nov 25, 2015 at 4:27 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> >
> > ---
> >  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 4a533db..ded306b 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 6ea7cd5..84048fe 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
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
> Acked-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
> --
> Fabiano Fidêncio
> 

I don't like that much this patch.
It should be split as is a combination of moving code and a bit
refactoring.
About the refactoring is basing putting mostly of the work in
an expanded display_channel_add_drawable which is doing a lot
of stuff. Previously there was a get_drawable that took a
RedDrawable, did some validations on it and initialize a
Drawable. Now all these creation stuff are done by the
display_channel_add_drawable which is passed a not initialized
Drawable structure.
Also see my FIXME note, this patch is adding spice-server leaks
just if guest attempts to send invalid surface ids.
Not hard to fix but still present.

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]