[PATCH 2/2] compositor: Remove surface->transform.enabled
Kristian Høgsberg
hoegsberg at gmail.com
Wed Feb 27 14:17:55 PST 2013
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.
Ok, let's just change the surface->transform.enabled test to a
type > translation check then.
> > *v = t;
> > }
> >
> > diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> > index c170f97..3135cd9 100644
> > --- a/src/compositor-drm.c
> > +++ b/src/compositor-drm.c
> > @@ -444,7 +444,7 @@ drm_output_prepare_scanout_surface(struct weston_output *_output,
> > buffer->width != output->base.current->width ||
> > buffer->height != output->base.current->height ||
> > output->base.transform != es->buffer_transform ||
> > - es->transform.enabled)
> > + es->transform.matrix.type != WESTON_MATRIX_TRANSFORM_TRANSLATE)
>
> Instead of inequality comparison, I would probably prefer
> es->transform.matrix.type > WESTON_MATRIX_TRANSFORM_TRANSLATE
> so that we can handle the identity matrix, too, if that happens to have
> matrix.type = 0.
Yeah, good point.
> > return NULL;
> >
> > bo = gbm_bo_import(c->gbm, GBM_BO_IMPORT_WL_BUFFER,
> > @@ -692,8 +692,7 @@ drm_output_check_sprite_format(struct drm_sprite *s,
> > static int
> > drm_surface_transform_supported(struct weston_surface *es)
> > {
> > - return !es->transform.enabled ||
> > - (es->transform.matrix.type < WESTON_MATRIX_TRANSFORM_ROTATE);
> > + return es->transform.matrix.type < WESTON_MATRIX_TRANSFORM_ROTATE;
>
> Ok.
Ok.
> > }
> >
> > static struct weston_plane *
> > diff --git a/src/compositor-rpi.c b/src/compositor-rpi.c
> > index 208271f..1ff6ca7 100644
> > --- a/src/compositor-rpi.c
> > +++ b/src/compositor-rpi.c
> > @@ -736,7 +736,7 @@ rpi_assign_plane(struct weston_surface *surface, struct rpi_output *output)
> > struct rpi_element *element;
> >
> > /* dispmanx elements cannot transform */
> > - if (surface->transform.enabled) {
> > + if (surface->transform.matrix.type != WESTON_MATRIX_TRANSFORM_TRANSLATE) {
>
> Inequality comparison, see earlier.
>
> > /* XXX: inspect the transformation matrix, we might still
> > * be able to put it into an element; scaling, additional
> > * translation (window titlebar context menus?)
> > diff --git a/src/compositor.c b/src/compositor.c
> > index 07b95a9..e3787e8 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -326,26 +326,12 @@ WL_EXPORT void
> > weston_surface_to_global_float(struct weston_surface *surface,
> > float sx, float sy, float *x, float *y)
> > {
> > - if (surface->transform.enabled) {
> > - struct weston_vector v = { { sx, sy, 0.0f, 1.0f } };
> > + struct weston_vector v = { { sx, sy, 0.0f, 1.0f } };
> >
> > - weston_matrix_transform(&surface->transform.matrix, &v);
> > + weston_matrix_transform(&surface->transform.matrix, &v);
> >
> > - if (fabsf(v.f[3]) < 1e-6) {
> > - weston_log("warning: numerical instability in "
> > - "%s(), divisor = %g\n", __func__,
> > - v.f[3]);
> > - *x = 0;
> > - *y = 0;
> > - return;
> > - }
> > -
> > - *x = v.f[0] / v.f[3];
> > - *y = v.f[1] / v.f[3];
> > - } else {
> > - *x = sx + surface->geometry.x;
> > - *y = sy + surface->geometry.y;
> > - }
> > + *x = v.f[0];
> > + *y = v.f[1];
>
> Continuing on the matrix.c discussion above,
> weston_surface_{to,from}_global_float() are defined to work on finite 2D
> points only. If you want to limit weston_matrix_transform() to work on
> finite points only, this is fine.
>
> I only wish we would keep the warning about numerical instability,
> since I would like to see it logged instead of seeing hard-to-catch
> glitches on screen, should a bug ever cause it to happen.
>
> weston_surface_{to,from}_global_float() is also the place that actually
> can skip the z-coordinate division without causing any surprise.
Yup, we'll keep all this in place.
> > }
> >
> > WL_EXPORT void
> > @@ -514,8 +500,6 @@ surface_compute_bbox(struct weston_surface *surface, int32_t sx, int32_t sy,
> > static void
> > weston_surface_update_transform_disable(struct weston_surface *surface)
> > {
> > - surface->transform.enabled = 0;
> > -
> > /* round off fractions when not transformed */
> > surface->geometry.x = roundf(surface->geometry.x);
> > surface->geometry.y = roundf(surface->geometry.y);
> > @@ -553,8 +537,6 @@ weston_surface_update_transform_enable(struct weston_surface *surface)
> > struct weston_matrix *inverse = &surface->transform.inverse;
> > struct weston_transform *tform;
> >
> > - surface->transform.enabled = 1;
> > -
> > /* Otherwise identity matrix, but with x and y translation. */
> > surface->transform.position.matrix.type = WESTON_MATRIX_TRANSFORM_TRANSLATE;
> > surface->transform.position.matrix.d[12] = surface->geometry.x;
> > @@ -627,26 +609,12 @@ WL_EXPORT void
> > weston_surface_from_global_float(struct weston_surface *surface,
> > float x, float y, float *sx, float *sy)
> > {
> > - if (surface->transform.enabled) {
> > - struct weston_vector v = { { x, y, 0.0f, 1.0f } };
> > -
> > - weston_matrix_transform(&surface->transform.inverse, &v);
> > + struct weston_vector v = { { x, y, 0.0f, 1.0f } };
> >
> > - if (fabsf(v.f[3]) < 1e-6) {
> > - weston_log("warning: numerical instability in "
> > - "weston_surface_from_global(), divisor = %g\n",
> > - v.f[3]);
> > - *sx = 0;
> > - *sy = 0;
> > - return;
> > - }
> > + weston_matrix_transform(&surface->transform.inverse, &v);
> >
> > - *sx = v.f[0] / v.f[3];
> > - *sy = v.f[1] / v.f[3];
> > - } else {
> > - *sx = x - surface->geometry.x;
> > - *sy = y - surface->geometry.y;
> > - }
> > + *sx = v.f[0];
> > + *sy = v.f[1];
>
> Same comments as above.
>
> > }
> >
> > WL_EXPORT void
> > @@ -1030,7 +998,7 @@ surface_accumulate_damage(struct weston_surface *surface,
> > wl_buffer_is_shm(surface->buffer_ref.buffer))
> > surface->compositor->renderer->flush_damage(surface);
> >
> > - if (surface->transform.enabled) {
> > + if (surface->transform.matrix.type != WESTON_MATRIX_TRANSFORM_TRANSLATE) {
>
> Inequality comparison.
>
> > pixman_box32_t *extents;
> >
> > extents = pixman_region32_extents(&surface->damage);
> > diff --git a/src/compositor.h b/src/compositor.h
> > index fdde762..409ef15 100644
> > --- a/src/compositor.h
> > +++ b/src/compositor.h
> > @@ -428,10 +428,6 @@ struct weston_surface {
> > pixman_region32_t boundingbox;
> > pixman_region32_t opaque;
> >
> > - /* matrix and inverse are used only if enabled = 1.
> > - * If enabled = 0, use x, y, width, height directly.
> > - */
> > - int enabled;
> > struct weston_matrix matrix;
> > struct weston_matrix inverse;
> >
> > diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> > index a5dc2f3..4a67cae 100644
> > --- a/src/gl-renderer.c
> > +++ b/src/gl-renderer.c
> > @@ -480,7 +480,7 @@ calculate_edges(struct weston_surface *es, pixman_box32_t *rect,
> > * there will be only four edges. We just need to clip the surface
> > * vertices to the clip rect bounds:
> > */
> > - if (!es->transform.enabled) {
> > + if (es->transform.matrix.type == WESTON_MATRIX_TRANSFORM_TRANSLATE) {
>
> (In)equality comparison.
>
> > 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...
> > 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?
Kristian
More information about the wayland-devel
mailing list