[PATCH weston 11/11] simple-damage: Add --use-buffer-damage flag
Derek Foreman
derekf at osg.samsung.com
Fri Nov 27 12:55:13 PST 2015
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.
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?
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?
I think that's as far as I'm going to go in debugging this one.
> Anyway, I didn't see this patch changing any behaviour that I tried.
>
>> diff --git a/clients/simple-damage.c b/clients/simple-damage.c
>> index 0551b9d..8929071 100644
>> --- a/clients/simple-damage.c
>> +++ b/clients/simple-damage.c
>> @@ -64,6 +64,7 @@ struct buffer {
>> enum window_flags {
>> WINDOW_FLAG_USE_VIEWPORT = 0x1,
>> WINDOW_FLAG_ROTATING_TRANSFORM = 0x2,
>> + WINDOW_FLAG_USE_DAMAGE_BUFFER = 0x4,
>> };
>>
>> struct window {
>> @@ -261,6 +262,14 @@ create_window(struct display *display, int width, int height,
>> exit(1);
>> }
>>
>> + if (display->compositor_version < 4 &&
>> + (flags & WINDOW_FLAG_USE_DAMAGE_BUFFER)) {
>> + fprintf(stderr, "wl_surface.damage_buffer unsupported in "
>> + "wl_surface version %d\n",
>> + display->compositor_version);
>> + exit(1);
>> + }
>> +
>> window = calloc(1, sizeof *window);
>> if (!window)
>> return NULL;
>> @@ -303,8 +312,12 @@ create_window(struct display *display, int width, int height,
>> }
>>
>> /* Initialise damage to full surface, so the padding gets painted */
>> - wl_surface_damage(window->surface, 0, 0, INT32_MAX, INT32_MAX);
>> -
>> + if (window->flags & WINDOW_FLAG_USE_DAMAGE_BUFFER) {
>> + wl_surface_damage_buffer(window->surface, 0, 0,
>> + INT32_MAX, INT32_MAX);
>> + } else {
>> + wl_surface_damage(window->surface, 0, 0, INT32_MAX, INT32_MAX);
>> + }
>> return window;
>> }
>>
>> @@ -567,12 +580,20 @@ redraw(void *data, struct wl_callback *callback, uint32_t time)
>> bwidth - 2 * bborder, bheight - 2 * bborder, 0x80000000);
>>
>> /* Damage where the ball was */
>> - wl_surface_damage(window->surface,
>> - window->ball.x - window->ball.radius,
>> - window->ball.y - window->ball.radius,
>> - window->ball.radius * 2 + 1,
>> - window->ball.radius * 2 + 1);
>> -
>> + if (window->flags & WINDOW_FLAG_USE_DAMAGE_BUFFER) {
>> + window_get_transformed_ball(window, &bx, &by);
>> + wl_surface_damage_buffer(window->surface,
>> + bx - bradius + off_x,
>> + by - bradius + off_y,
>> + bradius * 2 + 1,
>> + bradius * 2 + 1);
>
> Should the +1 be buffer-transformed too?
>
> The loop in paint_circle() goes from x-r to x+r inclusive, so was the
> +1 to account for the center pixel, or is it just a rounding?
>
> Hmm, but these are buffer coordinates, so +1 is correct here. For
> surface coordinates it might be too much, but won't hurt. It's probably
> fine afterall.
It is possible that it's 1 too far. It was present in the --debug print
already, and I copied that without thinking about it too much.
If paint_circle() colored every pixel it iterated over, the +1 would
make sense due to the center pixel... I'm not 100% certain
paint_circle() can touch that rightmost pixel though... :)
> I'm a bit uneasy with all the float to int conversions. Feels like it
> would be too easy to round in the wrong direction.
Yeah, fair enough. As long as we screw up what we render in the same
way we screw up the damage accounting we're all good though, right? ;)
>> + } else {
>> + wl_surface_damage(window->surface,
>> + window->ball.x - window->ball.radius,
>> + window->ball.y - window->ball.radius,
>> + window->ball.radius * 2 + 1,
>> + window->ball.radius * 2 + 1);
>> + }
>> window_advance_game(window, time);
>>
>> window_get_transformed_ball(window, &bx, &by);
>> @@ -595,12 +616,19 @@ redraw(void *data, struct wl_callback *callback, uint32_t time)
>> }
>>
>> /* Damage where the ball is now */
>> - wl_surface_damage(window->surface,
>> - window->ball.x - window->ball.radius,
>> - window->ball.y - window->ball.radius,
>> - window->ball.radius * 2 + 1,
>> - window->ball.radius * 2 + 1);
>> -
>> + if (window->flags & WINDOW_FLAG_USE_DAMAGE_BUFFER) {
>> + wl_surface_damage_buffer(window->surface,
>> + bx - bradius + off_x,
>> + by - bradius + off_y,
>> + bradius * 2 + 1,
>> + bradius * 2 + 1);
>> + } else {
>> + wl_surface_damage(window->surface,
>> + window->ball.x - window->ball.radius,
>> + window->ball.y - window->ball.radius,
>> + window->ball.radius * 2 + 1,
>> + window->ball.radius * 2 + 1);
>> + }
>> wl_surface_attach(window->surface, buffer->buffer, 0, 0);
>>
>> if (window->display->compositor_version >= 2 &&
>> @@ -787,6 +815,7 @@ print_usage(int retval)
>> " --transform=TRANSFORM\tTransform for the surface\n"
>> " --rotating-transform\tUse a different buffer_transform for each frame\n"
>> " --use-viewport\tUse wl_viewport\n"
>> + " --use-damage-buffer\tUse damage_buffer to post damage\n"
>> );
>>
>> exit(retval);
>> @@ -837,7 +866,7 @@ main(int argc, char **argv)
>> strcmp(argv[i], "-h") == 0) {
>> print_usage(0);
>> } else if (sscanf(argv[i], "--version=%d", &version) > 0) {
>> - if (version < 1 || version > 3) {
>> + if (version < 1 || version > 4) {
>> fprintf(stderr, "Unsupported wl_surface version: %d\n",
>> version);
>> return 1;
>> @@ -861,6 +890,9 @@ main(int argc, char **argv)
>> } else if (strcmp(argv[i], "--use-viewport") == 0) {
>> flags |= WINDOW_FLAG_USE_VIEWPORT;
>> continue;
>> + } else if (strcmp(argv[i], "--use-damage-buffer") == 0) {
>> + flags |= WINDOW_FLAG_USE_DAMAGE_BUFFER;
>> + continue;
>> } else {
>> printf("Invalid option: %s\n", argv[i]);
>> print_usage(255);
>
> 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? ;)
> 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.
>
> Thanks,
> pq
>
Thanks,
Derek
More information about the wayland-devel
mailing list