[PATCH 2/2] compositor: Remove surface->transform.enabled
Pekka Paalanen
ppaalanen at gmail.com
Thu Feb 21 01:05:41 PST 2013
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.
> *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.
> 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.
> }
>
> 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.
> }
>
> 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?
> 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.
What do you think?
Thanks,
pq
More information about the wayland-devel
mailing list