[Spice-devel] [PATCH 17/18] display: factor out draw_until

[Fabiano Fidêncio]

On Tue, Nov 24, 2015 at 11:08 AM, Frediano Ziglio <fziglio at redhat.com> wrote:
>>
>> On Mon, Nov 23, 2015 at 8:05 PM, Fabiano Fidêncio <fidencio at redhat.com>
>> wrote:
>> > On Mon, Nov 23, 2015 at 6:02 PM, Frediano Ziglio <fziglio at redhat.com>
>> > wrote:
>> >> From: Marc-André Lureau <marcandre.lureau at gmail.com>
>> >>
>> >> ---
>> >>  server/display-channel.c | 70
>> >>  ++++++++++++++++++++++--------------------------
>> >>  1 file changed, 32 insertions(+), 38 deletions(-)
>> >>
>> >> diff --git a/server/display-channel.c b/server/display-channel.c
>> >> index c5a69e3..a78f86a 100644
>> >> --- a/server/display-channel.c
>> >> +++ b/server/display-channel.c
>> >> @@ -1281,6 +1281,24 @@ static void surface_update_dest(RedSurface
>> >> *surface, const SpiceRect *area)
>> >>      canvas->ops->read_bits(canvas, dest, -stride, area);
>> >>  }
>> >>
>> >> +static void draw_until(DisplayChannel *display, RedSurface *surface,
>> >> Drawable *last)
>> >> +{
>> >> +    RingItem *ring_item;
>> >> +    Container *container;
>> >> +    Drawable *now;
>> >> +
>> >> +    do {
>> >> +        ring_item = ring_get_tail(&surface->current_list);
>> >> +        now = SPICE_CONTAINEROF(ring_item, Drawable, surface_list_link);
>> >> +        now->refs++;
>> >> +        container = now->tree_item.base.container;
>> >> +        current_remove_drawable(display, now);
>> >> +        container_cleanup(container);
>> >> +        drawable_draw(display, now);
>> >> +        display_channel_drawable_unref(display, now);
>> >> +    } while (now != last);
>> >> +}
>> >> +
>> >>  /*
>> >>   * Renders drawables for updating the requested area, but only drawables
>> >>   that are older
>> >>   * than 'last' (exclusive).
>> >> @@ -1340,27 +1358,18 @@ void display_channel_draw_till(DisplayChannel
>> >> *display, const SpiceRect *area, i
>> >>
>> >>      region_destroy(&rgn);
>> >>
>> >> -    if (!surface_last) {
>> >> -        return;
>> >> -    }
>> >> -
>
> Note: previously if surface_last was NULL surface_update_dest was not called.
>
>> >> -    do {
>> >> -        Container *container;
>> >> +    /* drawable_draw may call display_channel_draw for the surfaces
>> >> +     * 'now' depends on. Notice, that it is valid to call
>> >> +     * display_channel_draw in this case and not
>> >> +     * display_channel_draw_till: It is impossible that there was
>> >> +     * newer item then 'last' in one of the surfaces that
>> >> +     * display_channel_draw is called for, Otherwise, 'now' would have
>> >> +     * already been rendered.  See the call for
>> >> +     * red_handle_depends_on_target_surface in red_process_draw
>> >> +     */
>> >> +    if (surface_last)
>> >> +        draw_until(display, surface, surface_last);
>> >>
>> >> -        ring_item = ring_get_tail(&surface->current_list);
>> >> -        now = SPICE_CONTAINEROF(ring_item, Drawable, surface_list_link);
>> >> -        now->refs++;
>> >> -        container = now->tree_item.base.container;
>> >> -        current_remove_drawable(display, now);
>> >> -        container_cleanup(container);
>> >> -        /* drawable_draw may call display_channel_draw for the surfaces
>> >> 'now' depends on. Notice,
>> >> -           that it is valid to call display_channel_draw in this case and
>> >> not display_channel_draw_till:
>> >> -           It is impossible that there was newer item then 'last' in one
>> >> of the surfaces
>> >> -           that display_channel_draw is called for, Otherwise, 'now'
>> >> would have already been rendered.
>> >> -           See the call for red_handle_depends_on_target_surface in
>> >> red_process_draw */
>> >> -        drawable_draw(display, now);
>> >> -        display_channel_drawable_unref(display, now);
>> >> -    } while (now != surface_last);
>> >>      surface_update_dest(surface, area);
>
> Now is always called even is surface_last was NULL.
>
>> >>  }
>> >>
>> >> @@ -1370,8 +1379,7 @@ void display_channel_draw(DisplayChannel *display,
>> >> const SpiceRect *area, int su
>> >>      Ring *ring;
>> >>      RingItem *ring_item;
>> >>      QRegion rgn;
>> >> -    Drawable *last;
>> >> -    Drawable *now;
>> >> +    Drawable *last, *now;
>>
>> Ah, one minor: this change can be dropped from this patch.
>>
>
> Yes, I can remove it. Not harmful anyway.
>
>> >>      spice_debug("surface %d: area ==>", surface_id);
>> >>      rect_debug(area);
>> >>
>> >> @@ -1397,22 +1405,8 @@ void display_channel_draw(DisplayChannel *display,
>> >> const SpiceRect *area, int su
>> >>      }
>> >>      region_destroy(&rgn);
>> >>
>> >> -    if (!last) {
>> >> -        surface_update_dest(surface, area);
>> >> -        return;
>> >> -    }
>> >> -
>
> Here (a similar function) surface_update_dest is always called...
>
>> >> -    do {
>> >> -        Container *container;
>> >> +    if (last)
>> >> +        draw_until(display, surface, last);
>> >>
>> >> -        ring_item = ring_get_tail(&surface->current_list);
>> >> -        now = SPICE_CONTAINEROF(ring_item, Drawable, surface_list_link);
>> >> -        now->refs++;
>> >> -        container = now->tree_item.base.container;
>> >> -        current_remove_drawable(display, now);
>> >> -        container_cleanup(container);
>> >> -        drawable_draw(display, now);
>> >> -        display_channel_drawable_unref(display, now);
>> >> -    } while (now != last);
>> >>      surface_update_dest(surface, area);
>
> ... even after the patch.
>
>> >>  }
>> >> --
>> >> 2.4.3
>> >
>> > Looks good.
>> > Acked-by: Fabiano Fidêncio <fidencio at redhat.com>
>>
>
> So... it is an accident or an hidden fix ??

Good catch! I would bet in an accident.
I'd change the patch to keep the original behavior.

>
> Frediano

Best Regards,