[PATCH weston 1/2] simple-touch: Use damage_buffer if available
Bryce Harrington
bryce at osg.samsung.com
Mon Feb 22 22:17:17 UTC 2016
On Sat, Feb 20, 2016 at 08:52:10AM -0600, Derek Foreman wrote:
> 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. :)
That's fine. Like I said initially I'm not opposed to the code, but if
we want to keep both in there, then there needs to be some comments
explaining why. Just document in the code comments what you've
explained above, and it's fine with me and I'll gladly R-b it.
Bryce
> 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, ®istry_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
More information about the wayland-devel
mailing list