[PATCH weston 11/11] simple-damage: Add --use-buffer-damage flag

[Derek Foreman]

On 30/11/15 01:47 AM, Pekka Paalanen wrote:
> On Fri, 27 Nov 2015 14:55:13 -0600
> Derek Foreman <derekf at osg.samsung.com> wrote:
> 
>> On 27/11/15 07:47 AM, Pekka Paalanen wrote:
>>> On Wed, 18 Nov 2015 16:32:34 -0600
>>> Derek Foreman <derekf at osg.samsung.com> wrote:
>>>   
>>>> Add a new flag for testing damage in buffer co-ordinates
>>>>
>>>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>>>> ---
>>>>  clients/simple-damage.c | 62 +++++++++++++++++++++++++++++++++++++------------
>>>>  1 file changed, 47 insertions(+), 15 deletions(-)
>>>>  
>>>
>>> Hi Derek,
>>>
>>> even without your series, running simple-damage --use-viewport
>>> --scale=2 will leave trails. It's not a damage issue, because
>>> compositor repaints do not clear it. It seems to be a rendering issue
>>> in the app. Using viewport with any scale > 1 will trigger that.  
>>
>> Yes, I was seeing this too.  Doesn't happen with the pixman renderer.
>>
>> Figuring it out is on my TODO list somewhere, right next to sorting out
>> the damage issues when zoomed and when changing transforms between
>> commits. :)
>>
>> If I mess up the gl renderer to do a full texture upload with ever
>> damage commit the problem goes away.
> 
> Ha! You're right, I forgot the compositor has a copy of the surface
> contents and uses damage to update that. D'oh.
> 
>> Checking the damage co-ordinates in gl_renderer_flush_damage(),
>> sometimes thin areas are getting shrunk to 0 (when they look like they
>> should be getting expanded by the transform)
>>
>> I guess the thing damage regions come from the two (erase, replace)
>> damage regions being bashed together and overlapping when the ball is
>> moving slowly...
>>
>> weston_surface_to_buffer_rect() looks like it could be part of the
>> problem, since it looks like it does the viewport stuff first, then
>> truncates to integer, then transforms the resulting rect.  Could be
>> losing thing rectangles in the middle?
> 
> You are on to something there. On quick look, I'd blame the rounding of
> x2,y2 being in the wrong direction. But then users need to ensure not
> crossing over image boundaries.

Yup, that "fixes" it.  It's all fine if someone's just using these
functions for damage tracking, since a little extra will by fine (the
buffer is expected to be all there anyway)

That seems to be all it's used for right now.

>> Appears to not happen with my transforms branch that replaced all sorts
>> of this stuff with matrix mults with no conversion to integer in
>> between.  Likely because it doesn't have to round off stuff to ints
>> between steps?
> 
> Quite likely, and maybe the final rounding is also done towards
> enlarging the damage region there?

Ahh yes, maybe that too.

>> I think that's as far as I'm going to go in debugging this one.
> 
> If simply fixing the rounding in weston_surface_to_buffer_rect() fixes
> it, maybe it could be landed fast?

Well, I'll send a patch - it does seem to clear it up here.

There are a few caveats with weston_surface_to_buffer_rect() as it is
(hello non-axis aligned rectangles).

Perhaps a comment is in order too.

>>> Anyway, I didn't see this patch changing any behaviour that I tried.
> 
>>> Otherwise this patch looks fine, but I have hard time understanding how
>>> everything works here. Especially the rotations made my brain fall off.
>>> That's why just a  
>>
>> We're only really rotating a circle, and it's orientation independent,
>> right? ;)
> 
> Yeeeah... except when you are rounding to integer pixels and if the
> rounding direction makes the circle not centered and...
> 
> Actually I was more trying to understand the code that was already in
> redraw(), that you clarified to do things like off_x = bwidth - tx; and
> then it does paint_box(buffer->shm_data, bpitch, off_x, off_y, bwidth,
> bborder, 0xffffffff);, so would that not paint up to off_x + bwidth =
> bwidth - tx + bwidth which will be greater than bwidth, i.e. drawing
> beyond the image border? But when I looked at the rendering, I didn't
> see any evidence of that, so there is something I don't understand.
> After all, tx > bwidth cannot be, right? Ooh, but bwidth is only half
> of the actual bwidth... that's probably the thing I missed. :-P
> 
> Yeah, hard time following all that.

Ouch, I didn't mean to goad you into reading that code again.  Or maybe
I did. ;)

I'll probably wait until a week or so after we land the damage_buffer()
protocol, then land this with just your ACK if nobody has taken the time
for a full review at that point.

Thanks,
Derek

> 
> Thanks,
> pq
> 
>>
>>> Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>>
>>> Whether to use damage_buffer by default when available, I don't think
>>> it matters. It depends on your toolkit which one is more
>>> straightforward to use, otherwise they are equally good.  
>>
>> Ok, I'll leave it alone out of sheer laziness then.
>>
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel