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

[Derek Foreman]

Pushed this one (changed the commit log to say --use-damage-buffer)


On 30/11/15 01:11 PM, Derek Foreman wrote:
> 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
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>