[PATCH weston 1/2] simple-touch: Use damage_buffer if available

[Derek Foreman]

On 20/02/16 02:53 AM, Giulio Camuffo wrote:
> 2016-02-20 5:20 GMT+02:00 Bryce Harrington <bryce at osg.samsung.com>:
>> On Fri, Feb 12, 2016 at 09:03:16AM +0200, Giulio Camuffo wrote:
>>> 2016-02-12 6:26 GMT+02:00 Bryce Harrington <bryce at osg.samsung.com>:
>>>> On Fri, Jan 08, 2016 at 03:00:55PM -0600, Derek Foreman wrote:
>>>>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>>>>> ---
>>>>>  clients/simple-touch.c | 18 +++++++++++++++---
>>>>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>>>
>>>> Given that the demo clients exist to show the functionality of the
>>>> weston they're included with, damage_buffer is always going to be
>>>> available, isn't it?
>>>
>>> But it can be used to test other compositors too. I think demo clients
>>> should strive to be compatible with as many compositors as possible.
>>
>> Yes, they can be used to test other compositors, but I would argue their
>> ultimate purpose is more educational oriented, and in service of that
>> aim, keeping the code concise is beneficial.

As an educational item, the code as written makes it clear how to write
code that works on both new and old compositors efficiently.  Anyone
interested in compatibility should probably write it this way.

>> On the topic of testing, if you were in fact using this to test your
>> compositor implementation, and your compositor lacks the damage_buffer
>> feature, the error generated in that case would be a valid testing
>> result, I should think.

Well, we'd lose the ability to run the test against compositors that
don't support the latest version of wl_surface.  That's not really
testing anything in the case where a compositor just doesn't claim to
support it.

If the compositor doesn't actually have an implementation at all but
reports the version, then it's got some very serious bugs that probably
don't need advanced tests to find, as it's missing a function pointer a
client can make it try to dereference.

> I don't quite agree. While damage_buffer is better damage didn't go
> anywere and compositors are supposed to implement it correctly. We can

Yes, we talked briefly about trying to deprecate wl_surface.damage, but
ultimately it was decided that we shouldn't.  Enlightenment will
probably eventually implement damage as "damage the world" and
damage_buffer efficiently.

As it stands anyone implementing the latest version of wl_surface needs
to implement both functionally.

> maybe make it complain if buffer_damage is not available, if we want
> to hint at compositors they should add that.
> And i don't think the code this patch adds makes it difficult to
> understand the code, it remains pretty straightforward. I would also
> argue that if we want the code to be educational it should show how to
> to remain backward compatible when new features are being used.

I'm of course, inclined to agree with Giulio here, as he's saying pretty
much everything I was thinking when I wrote the code. :)

Thanks,
Derek

> 
> 
> Thanks,
> Giulio
> 
>>
>> Bryce
>>
>>>> Maybe just keep the client concise and just use the new API?
>>>>
>>>> If we do want to demonstrate both routines, then IMHO the code should
>>>> either show or explain why one would prefer one over the other.  I
>>>> imagine a new wayland coder would be looking at simple-touch as a simple
>>>> example of how to do touch, and might get confused seeing two ways to do
>>>> it.
>>>>
>>>> Bryce
>>>>
>>>>> diff --git a/clients/simple-touch.c b/clients/simple-touch.c
>>>>> index 446f2ca..383d802 100644
>>>>> --- a/clients/simple-touch.c
>>>>> +++ b/clients/simple-touch.c
>>>>> @@ -56,6 +56,7 @@ struct touch {
>>>>>       int has_argb;
>>>>>       int width, height;
>>>>>       void *data;
>>>>> +     bool use_damage_buffer;
>>>>>  };
>>>>>
>>>>>  static void
>>>>> @@ -148,7 +149,10 @@ touch_paint(struct touch *touch, int32_t x, int32_t y, int32_t id)
>>>>>       p[2] = c;
>>>>>
>>>>>       wl_surface_attach(touch->surface, touch->buffer, 0, 0);
>>>>> -     wl_surface_damage(touch->surface, x - 2, y - 2, 5, 5);
>>>>> +     if (touch->use_damage_buffer)
>>>>> +             wl_surface_damage_buffer(touch->surface, x - 2, y - 2, 5, 5);
>>>>> +     else
>>>>> +             wl_surface_damage(touch->surface, x - 2, y - 2, 5, 5);
>>>>>       /* todo: We could queue up more damage before committing, if there
>>>>>        * are more input events to handle.
>>>>>        */
>>>>> @@ -269,9 +273,13 @@ handle_global(void *data, struct wl_registry *registry,
>>>>>       struct touch *touch = data;
>>>>>
>>>>>       if (strcmp(interface, "wl_compositor") == 0) {
>>>>> +             int cv = MIN(WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION, version);
>>>>> +
>>>>>               touch->compositor =
>>>>>                       wl_registry_bind(registry, name,
>>>>> -                                      &wl_compositor_interface, 1);
>>>>> +                                      &wl_compositor_interface, cv);
>>>>> +             if (cv >= WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION)
>>>>> +                     touch->use_damage_buffer = true;
>>>>>       } else if (strcmp(interface, "wl_shell") == 0) {
>>>>>               touch->shell =
>>>>>                       wl_registry_bind(registry, name,
>>>>> @@ -308,6 +316,7 @@ touch_create(int width, int height)
>>>>>       touch->display = wl_display_connect(NULL);
>>>>>       assert(touch->display);
>>>>>
>>>>> +     touch->use_damage_buffer = false;
>>>>>       touch->has_argb = 0;
>>>>>       touch->registry = wl_display_get_registry(touch->display);
>>>>>       wl_registry_add_listener(touch->registry, &registry_listener, touch);
>>>>> @@ -337,7 +346,10 @@ touch_create(int width, int height)
>>>>>
>>>>>       memset(touch->data, 64, width * height * 4);
>>>>>       wl_surface_attach(touch->surface, touch->buffer, 0, 0);
>>>>> -     wl_surface_damage(touch->surface, 0, 0, width, height);
>>>>> +     if (touch->use_damage_buffer)
>>>>> +             wl_surface_damage_buffer(touch->surface, 0, 0, width, height);
>>>>> +     else
>>>>> +             wl_surface_damage(touch->surface, 0, 0, width, height);
>>>>>       wl_surface_commit(touch->surface);
>>>>>
>>>>>       return touch;
>>>>> --
>>>>> 2.6.4
>>>>>
>>>>> _______________________________________________
>>>>> 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
>>>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel