[PATCH weston 1/3] compositor: Update input region when updating surface transfomations

[Pekka Paalanen]

On Thu, 15 Nov 2012 10:36:00 +0100
Jonas Ådahl <jadahl at gmail.com> wrote:

> On Thu, Nov 15, 2012 at 8:52 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Wed, 14 Nov 2012 23:39:51 +0100
> > Jonas Ådahl <jadahl at gmail.com> wrote:
> >
> >> If the compositor updated the geometry of a surface, the input region
> >> would still represent the original geometry. By intersecting the input
> >> region specified by the client, if any, by the new geometry when
> >> updating surface transformations the active input region can be kept up
> >> to date.
> >
> > Did you hit this problem when a surface radically changes size in a
> > single step, leaving the input region obviously wrong? Or something
> > more subtle? That would be nice to mention.
> 
> The problem happens if the compositor (in particular in event-test.c)
> calls weston_surface_configure() and then tries to repick a surface.
> When repicking, the input region will be the same as it was before the
> reconfiguration, which potentially could be the rect 0x0, resulting it
> wont be repicked by the compositor.

Ok. I was trying to figure out how to trigger the problem, and couldn't
really see how. When wl_surface.commit is handled, surface's configure
is called first, and then the input region is clipped with it.

I don't understand event-test.c at all, but it just occurred to me that
it might be faulty now that we have commit. Or do we expect the server
to change the size of a surface on its own?

Actually, the server may change the transformation, which could scale
the surface, but that still won't affect the input region, since input
region is always in the surface-local coordinates, and hit-testing
converts before testing.

> >> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> >> ---
> >>  src/compositor.c |   23 +++++++++++++++++------
> >>  src/compositor.h |    1 +
> >>  2 files changed, 18 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/compositor.c b/src/compositor.c
> >> index 83bd244..75c66ab 100644
> >> --- a/src/compositor.c
> >> +++ b/src/compositor.c
> >> @@ -249,6 +249,7 @@ weston_surface_create(struct weston_compositor *compositor)
> >>       pixman_region32_init(&surface->opaque);
> >>       pixman_region32_init(&surface->clip);
> >>       region_init_infinite(&surface->input);
> >> +     region_init_infinite(&surface->full_input);
> >>       pixman_region32_init(&surface->transform.opaque);
> >>       wl_list_init(&surface->frame_callback_list);
> >>
> >> @@ -508,6 +509,17 @@ weston_surface_update_transform_enable(struct weston_surface *surface)
> >>       return 0;
> >>  }
> >>
> >> +static void
> >> +weston_surface_update_input_region(struct weston_surface *surface)
> >> +{
> >> +     pixman_region32_fini(&surface->input);
> >> +     pixman_region32_init_rect(&surface->input, 0, 0,
> >> +                               surface->geometry.width,
> >> +                               surface->geometry.height);
> >> +     pixman_region32_intersect(&surface->input,
> >> +                               &surface->input, &surface->full_input);
> >> +}
> >> +
> >>  WL_EXPORT void
> >>  weston_surface_update_transform(struct weston_surface *surface)
> >>  {
> >> @@ -522,6 +534,8 @@ weston_surface_update_transform(struct weston_surface *surface)
> >>       pixman_region32_fini(&surface->transform.opaque);
> >>       pixman_region32_init(&surface->transform.opaque);
> >>
> >> +     weston_surface_update_input_region(surface);
> >> +
> >>       /* transform.position is always in transformation_list */
> >>       if (surface->geometry.transformation_list.next ==
> >>           &surface->transform.position.link &&
> >> @@ -791,6 +805,7 @@ destroy_surface(struct wl_resource *resource)
> >>       pixman_region32_fini(&surface->opaque);
> >>       pixman_region32_fini(&surface->clip);
> >>       pixman_region32_fini(&surface->input);
> >> +     pixman_region32_fini(&surface->full_input);
> >>
> >>       wl_list_for_each_safe(cb, next, &surface->frame_callback_list, link)
> >>               wl_resource_destroy(&cb->resource);
> >> @@ -1286,12 +1301,8 @@ surface_commit(struct wl_client *client, struct wl_resource *resource)
> >>       pixman_region32_fini(&opaque);
> >>
> >>       /* wl_surface.set_input_region */
> >> -     pixman_region32_fini(&surface->input);
> >> -     pixman_region32_init_rect(&surface->input, 0, 0,
> >> -                               surface->geometry.width,
> >> -                               surface->geometry.height);
> >> -     pixman_region32_intersect(&surface->input,
> >> -                               &surface->input, &surface->pending.input);
> >> +     pixman_region32_copy(&surface->full_input, &surface->pending.input);
> >> +     weston_surface_update_input_region(surface);
> >
> > Hi,
> >
> > just wondering, do we actually want to call
> > weston_surface_update_input_region() here?
> >
> > If we think about what's on screen, that won't change until repaint, so
> > wouldn't it be logical to actually update the input region only at
> > repaint? Yeah, this is a change from how it was before, but I think
> > this could be more correct.
> 
> This would work, I suppose. Practically, the test cases would need to
> be changed to require a redraw before continuing after resizing
> surface so that later motion events have the expected effect.
> 
> >
> > If the server sends input to the client surface after
> > wl_surface.commit, but before repaint, then from user perspective the
> > input corresponds to the old surface contents. Hence, also clients
> > should be expecting input relative to the old contents until they
> > actually get the frame callback.
> 
> With this patch, if a client sends input to the surface after commit,
> the previously set input will still be the one in use, until next
> wl_surface.commit even if the compositor changes the dimensions. If a
> client never set any input region, the coordinates it receives might
> be confusing, though, as it'll be whatever the compositor thinks is
> the surface size. If a client cares about this, maybe this is good
> enough to have it have control on what it receives by setting input
> regions when creating?
> 
> Looking in shell.c where clients are notified of surface changes via
> wl_shell_surface_configure() I can't see any obvious correlation with
> what has been painted or not. Do you think we should change this as
> well, so that clients will be notified of dimension changes after the
> frame callback?

Sorry, I really meant input events. Configure events are unrelated.

An example:

	client				server

prepare a new buffer
wl_surface.commit	->
				input from devices
			<-	input events
input processing
				repaint
			<-	frame callback triggered

Now, there are old and new contents for a surface. For the user, the
content changes to new at "repaint" step.

Logically, the "input processing" in client before it receives the
frame callback should work with the old surface contents, since that is
what is still showing to the user. This is what I meant earlier.


Thanks,
pq

> > Or should we just not care, since even pointer motion triggers a
> > repaint, and it's practically impossible for a user to tell the
> > difference between (to react to the change, actually) whether the input
> > was for this or previous content?
> >
> > Well... like I said, just wondering.
> 
> I think either way would practically work, but would be good to
> document what the client can expect and how server implementers should
> behave.