On Tue, Nov 3, 2015 at 6:08 PM, Fabiano Fidêncio <fidencio@xxxxxxxxxx> wrote:
> On Tue, Nov 3, 2015 at 5:28 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>>
>>> On Tue, Nov 3, 2015 at 11:20 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
>>> >
>>> > ---
>>> > 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@xxxxxxxxxxxxxxxxxxxxx
>>> > 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 still didn't get exactly the problems you found wrt delta
computation on these patches. The patches moved it in order to
directly use it from the drawable, which is okay IMHO.
>
>> I would nack it and consider all these strangely related patches.
>
What's your proposal? Squash the patches?
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel