[Spice-devel] [PATCH 11/15] worker: minor simplification

[Fabiano Fidêncio]

On Tue, Nov 3, 2015 at 5:28 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
>>
>> On Tue, Nov 3, 2015 at 11:20 AM, Frediano Ziglio <fziglio at redhat.com> wrote:
>> > From: Marc-André Lureau <marcandre.lureau at gmail.com>
>> >
>> > ---
>> >  server/red_worker.c | 42 +++++++++++++++++++++++-------------------
>> >  server/tree.h       |  1 +
>> >  2 files changed, 24 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/server/red_worker.c b/server/red_worker.c
>> > index 1849cc8..9f42f87 100644
>> > --- a/server/red_worker.c
>> > +++ b/server/red_worker.c
>> > @@ -3056,6 +3056,7 @@ static inline int
>> > red_current_add_with_shadow(RedWorker *worker, Ring *ring, Dra
>> >  {
>> >  #ifdef RED_WORKER_STAT
>> >      stat_time_t start_time = stat_now(worker);
>> > +    ++worker->add_with_shadow_count;
>> >  #endif
>> >
>> >      Shadow *shadow = __new_shadow(worker, item, delta);
>> > @@ -3131,24 +3132,8 @@ static inline void red_update_streamable(RedWorker
>> > *worker, Drawable *drawable,
>> >      drawable->streamable = TRUE;
>> >  }
>> >
>> > -static inline int red_current_add_qxl(RedWorker *worker, Ring *ring,
>> > Drawable *drawable,
>> > -                                      RedDrawable *red_drawable)
>> > +static void red_print_stats(RedWorker *worker)
>> >  {
>> > -    int ret;
>> > -
>> > -    if (has_shadow(red_drawable)) {
>> > -        SpicePoint delta;
>> > -
>> > -#ifdef RED_WORKER_STAT
>> > -        ++worker->add_with_shadow_count;
>> > -#endif
>> > -        delta.x = red_drawable->u.copy_bits.src_pos.x -
>> > red_drawable->bbox.left;
>> > -        delta.y = red_drawable->u.copy_bits.src_pos.y -
>> > red_drawable->bbox.top;
>> > -        ret = red_current_add_with_shadow(worker, ring, drawable, &delta);
>> > -    } else {
>> > -        red_update_streamable(worker, drawable, red_drawable);
>> > -        ret = red_current_add(worker, ring, drawable);
>> > -    }
>> >  #ifdef RED_WORKER_STAT
>> >      if ((++worker->add_count % 100) == 0) {
>> >          stat_time_t total = worker->add_stat.total;
>> > @@ -3173,6 +3158,26 @@ static inline int red_current_add_qxl(RedWorker
>> > *worker, Ring *ring, Drawable *d
>> >          stat_reset(&worker->__exclude_stat);
>> >      }
>> >  #endif
>> > +}
>> > +
>> > +static int red_add_drawable(RedWorker *worker, Drawable *drawable)
>> > +{
>> > +    int ret = FALSE, surface_id = drawable->surface_id;
>> > +    RedDrawable *red_drawable = drawable->red_drawable;
>> > +    Ring *ring = &worker->surfaces[surface_id].current;
>> > +
>> > +    if (has_shadow(red_drawable)) {
>> > +        SpicePoint delta = {
>> > +            .x = red_drawable->u.copy_bits.src_pos.x -
>> > red_drawable->bbox.left,
>> > +            .y = red_drawable->u.copy_bits.src_pos.y -
>> > red_drawable->bbox.top
>> > +        };
>> > +        ret = red_current_add_with_shadow(worker, ring, drawable, &delta);
>> > +    } else {
>> > +        red_update_streamable(worker, drawable, red_drawable);
>> > +        ret = red_current_add(worker, ring, drawable);
>> > +    }
>> > +
>> > +    red_print_stats(worker);
>> >      return ret;
>> >  }
>> >
>> > @@ -3484,8 +3489,7 @@ static gboolean red_process_draw(RedWorker *worker,
>> > QXLCommandExt *ext_cmd)
>> >          goto end;
>> >      }
>> >
>> > -    if (red_current_add_qxl(worker, &worker->surfaces[surface_id].current,
>> > drawable,
>> > -                            red_drawable)) {
>> > +    if (red_add_drawable(worker, drawable)) {
>> >          if (drawable->tree_item.effect != QXL_EFFECT_OPAQUE) {
>> >              worker->transparent_count++;
>> >          }
>> > diff --git a/server/tree.h b/server/tree.h
>> > index 77f1fbf..e10fa1c 100644
>> > --- a/server/tree.h
>> > +++ b/server/tree.h
>> > @@ -45,6 +45,7 @@ struct TreeItem {
>> >      QRegion rgn;
>> >  };
>> >
>> > +/* A region "below" a copy, or the src region of the copy */
>>
>> This comment is nice, but doesn't belong to this commit IMHO.
>>
>> >  struct Shadow {
>> >      TreeItem base;
>> >      QRegion on_hold;
>> > --
>> > 2.4.3
>> >
>> > _______________________________________________
>> > Spice-devel mailing list
>> > Spice-devel at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>> ack!
>>
>
> There is an ack and a comment. Do you want to split the patch or commit
> as it is?

Well, I meant: "would be better to have it in another commit, but
ignore if you want".

> Viewing the 11+12+13 patches looks they move multiple times delta computation.

It didn't come to my attention. I will re-review the patches now with
a more focused view on the delta computation.

> I would nack it and consider all these strangely related patches.

Mainly for a NACK, I do believe that the opinion of the people that
are more used to the project is stronger than the opinion of the
people that just jumped in not so long time ago.

Best Regards,