[PATCH 2/2] compositor: Remove surface->transform.enabled
Pekka Paalanen
ppaalanen at gmail.com
Thu Feb 28 02:04:24 PST 2013
On Wed, 27 Feb 2013 17:17:55 -0500
Kristian Høgsberg <hoegsberg at gmail.com> wrote:
> On Thu, Feb 21, 2013 at 11:05:41AM +0200, Pekka Paalanen wrote:
> > On Wed, 20 Feb 2013 11:52:18 -0500
> > Kristian Høgsberg <krh at bitplanet.net> wrote:
> >
> > > We have matrix.type now and can rely on that for optimizing transformations
> > > and choosing fast-paths.
> > > ---
> > > shared/matrix.c | 15 +++++++++++++++
> > > src/compositor-drm.c | 5 ++---
> > > src/compositor-rpi.c | 2 +-
> > > src/compositor.c | 50 +++++++++-----------------------------------------
> > > src/compositor.h | 4 ----
> > > src/gl-renderer.c | 5 +++--
> > > src/pixman-renderer.c | 5 ++---
> > > src/shell.c | 14 +-------------
> > > 8 files changed, 33 insertions(+), 67 deletions(-)
> > >
> > > diff --git a/shared/matrix.c b/shared/matrix.c
> > > index 3ff4089..310fae4 100644
> > > --- a/shared/matrix.c
> > > +++ b/shared/matrix.c
> > > @@ -115,12 +115,27 @@ weston_matrix_transform(struct weston_matrix *matrix, struct weston_vector *v)
> > > int i, j;
> > > struct weston_vector t;
> > >
> > > + if (matrix->type == WESTON_MATRIX_TRANSFORM_TRANSLATE) {
> > > + v->f[0] += matrix->d[12];
> > > + v->f[1] += matrix->d[13];
> > > + v->f[2] += matrix->d[14];
> > > + return;
> > > + }
> > > +
> >
> > This would actually be conditional on v->f[3] == 1.0f, which I think we
> > can just compare literally by equality, since the 1.0f is always
> > assigned, never a computed result, and in case we do get a wrong result
> > from the comparison, it still does not produce wrong results, just
> > slower.
> >
> > > for (i = 0; i < 4; i++) {
> > > t.f[i] = 0;
> > > for (j = 0; j < 4; j++)
> > > t.f[i] += v->f[j] * matrix->d[i + j * 4];
> > > }
> > >
> > > + if (fabsf(t.f[3]) < 1e-6) {
> > > + t.f[0] = 0;
> > > + t.f[1] = 0;
> > > + } else {
> > > + t.f[0] /= t.f[3];
> > > + t.f[1] /= t.f[3];
> > > + }
> > > +
> >
> > Missing element [2]? That would be a surprise if we started using
> > z-coordinate.
> >
> > There's also a bit of a question, how generic (and unsurprising) we
> > want this matrix operations code to be. By normalizing here to w=1.0,
> > you explicitly exclude the possiblity to produce points at infinity, or
> > to transform vectors rather than points (in the sense that vectors are
> > not affected by translation, hence are of the form [x, y, z, 0]).
> >
> > Given that we do not use the z-coordinate, points never lie in
> > infinity, and never transform vectors, this is ok in practice, for now.
> >
> > I wonder if we could actually benefit from transforming vectors...
> > handling the wl_surface.attach dx, dy parameters would benefit from
> > handling it as a vector, but that also assumes our transformations will
> > be linear, which is something I'm not sure we want to pose in the
> > weston code outside of weston_surface_{to,from}_global*(). Hmm, or do
> > we actually do that already... ah, can't think that far.
> >
> > So, this is mostly a question of how generic do we want matrix.c to be.
> >
> > Personally, I would prefer it to be generic (hey, I wrote the code for
> > inverting arbitrary 4x4 matrices, when in reality we only ever have
> > 2D-affine in XY!) and unsurprising, until performance profiling raises
> > issues with it.
> > > for (i = 0; i < surf.n; i++) {
> > > ex[i] = clip(surf.x[i], ctx.clip.x1, ctx.clip.x2);
> > > ey[i] = clip(surf.y[i], ctx.clip.y1, ctx.clip.y2);
> > > @@ -772,7 +772,8 @@ draw_surface(struct weston_surface *es, struct weston_output *output,
> > > use_shader(gr, gs->shader);
> > > shader_uniforms(gs->shader, es, output);
> > >
> > > - if (es->transform.enabled || output->zoom.active)
> > > + if (es->transform.matrix.type != WESTON_MATRIX_TRANSFORM_TRANSLATE ||
> > > + output->zoom.active)
> >
> > Inequality comparison. Oh btw, !es->transform.enabled also used to
> > mean, that the translation is in full integer pixels. Do we care about
> > fractional translations anymore?
>
> Yea true... I think enabling filtering for translation animations make
> sense, we'll be able to animate the surface to sub-pixel positions
> which I believe makes a difference for slow-moving surfaces (eg
> towards the end of a slide as the surface slows down). I don't know
> if we can deduce it from the matrix though... we'd have to either add
> a surface flag to trigger GL_LINEAR or have this code look at
> transformation_list to see if there's only the position transform.
>
> Hmm...
Or maybe inspect the fractional part of the translation elements in the
final matrix?
>
> > > filter = GL_LINEAR;
> > > else
> > > filter = GL_NEAREST;
> > > diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c
> > > index 9571f6f..24961fa 100644
> > > --- a/src/pixman-renderer.c
> > > +++ b/src/pixman-renderer.c
> > > @@ -203,7 +203,7 @@ repaint_region_simple(struct weston_surface *es, struct weston_output *output,
> > > pixman_image_set_filter(ps->image, PIXMAN_FILTER_NEAREST, NULL, 0);
> > > pixman_image_set_transform(ps->image, NULL);
> > >
> > > - if (!es->transform.enabled) {
> > > + if (es->transform.matrix.type == WESTON_MATRIX_TRANSFORM_TRANSLATE) {
> >
> > (In)equality comparison.
> >
> > > pixman_region32_translate(&final_region, es->geometry.x, es->geometry.y);
> > > } else {
> > > weston_surface_to_global_float(es, 0, 0, &surface_x, &surface_y);
> > > @@ -271,8 +271,7 @@ draw_surface(struct weston_surface *es, struct weston_output *output,
> > > }
> > >
> > > /* TODO: Implement repaint_region_complex() using pixman_composite_trapezoids() */
> > > - if (es->transform.enabled &&
> > > - es->transform.matrix.type != WESTON_MATRIX_TRANSFORM_TRANSLATE) {
> > > + if (es->transform.matrix.type != WESTON_MATRIX_TRANSFORM_TRANSLATE) {
> >
> > Inequality comparison.
> >
> > > repaint_region_complex(es, output, &repaint);
> > > } else {
> > > /* blended region is whole surface minus opaque region: */
> > > diff --git a/src/shell.c b/src/shell.c
> > > index af802a5..7643421 100644
> > > --- a/src/shell.c
> > > +++ b/src/shell.c
> > > @@ -1912,19 +1912,7 @@ shell_map_popup(struct shell_surface *shsurf)
> > > shsurf->popup.grab.interface = &popup_grab_interface;
> > >
> > > weston_surface_update_transform(parent);
> > > - if (parent->transform.enabled) {
> > > - shsurf->popup.parent_transform.matrix =
> > > - parent->transform.matrix;
> > > - } else {
> > > - /* construct x, y translation matrix */
> > > - weston_matrix_init(&shsurf->popup.parent_transform.matrix);
> > > - shsurf->popup.parent_transform.matrix.type =
> > > - WESTON_MATRIX_TRANSFORM_TRANSLATE;
> > > - shsurf->popup.parent_transform.matrix.d[12] =
> > > - parent->geometry.x;
> > > - shsurf->popup.parent_transform.matrix.d[13] =
> > > - parent->geometry.y;
> > > - }
> > > + shsurf->popup.parent_transform.matrix = parent->transform.matrix;
> >
> > Ok.
> >
> > > wl_list_insert(es->geometry.transformation_list.prev,
> > > &shsurf->popup.parent_transform.link);
> > >
> >
> > There is one more thing: now that you have removed
> > surface->transform.enabled flag, you can no longer use
> > surface->geometry.{x,y} fields at all in code outside of
> > weston_surface_transform_update() or weston_surface_set_position(). You
> > must use the translation elements from surface->transform.matrix.
> >
> > The example are popup surfaces. Their surface->transform.matrix.type is
> > usually TRANSLATE, but geometry.{x,y} is only the translation relative
> > to the parent surface, not the final global coordinates.
> >
> > Before this patch, maybe almost all the reads of geometry.{x,y} should
> > be converted to use
> >
> > static inline void
> > weston_matrix_get_translation_xy(struct weston_matrix *matrix, float *x, float *y)
> > {
> > assert(matrix->type <= WESTON_MATRIX_TRANSFORM_TRANSLATE);
> > *x = matrix->f[12];
> > *y = matrix->f[13];
> > }
> >
> > (weston_surface_get_position() would not be symmetric to
> > weston_surface_set_position(), since that is specifically just
> > about geometry.{x,y}. Hm, weston_surface_get_global_position?)
> >
> > Actually, after that it would be just a small step to remove
> > geometry.{x,y} fields completely, in favor of transform.position.matrix
> > which would then move to geometry.position.matrix.
>
> Removing geometry.x/y is the next step (and I did mostly write that
> patch), but I don't see how this patch changes the validity of them?
> In other words, why can't we do this first and then remove x/y?
The backends are probably the best example. I think they first
guarantee, that surface->transform.enabled is false, and then use
geometry.{x,y} to position the surface on a plane. That assumes, that
geometry.{x,y} is the total translation from global to surface
coordinates. This is a popular fast path in Weston code base IIRC. For
popup surfaces that won't work.
If you simply replaced surface->transform.enabled with
surface->transform.matrix.type > WESTON_MATRIX_TRANSFORM_TRANSLATE,
the code will continue using geometry.{x,y} even if there are more
matrices in the transformation_list, provided that all matrices only
apply translation. So, those places will only get a part of the
translation, not all of it.
The old meaning of surface->transform.enabled was:
- there are no other matrices in the list, therefore
- geometry.{x,y} can be used directly instead of the final matrix, and
- the final translation is also in integers
The condition surface->transform.matrix.type >
WESTON_MATRIX_TRANSFORM_TRANSLATE does not guarantee any of those, it
simply means that the final transformation is a pure translation.
That is why we should remove most of the geometry.{x,y} uses before
changing the meaning of the transform.enabled conditionals.
Hmm, actually it looks like the DRM backend is broken with this respect
already.
Thanks,
pq
More information about the wayland-devel
mailing list