[Cogl] [PATCH] Initialize the inverse matrix in invert_matrix_3d()

[Robert Bragg]

This looks like a good catch!

Since I think calculating the inverse matrix can be quite a common
critical path for performance I wonder if we could just fixup the last
row of the matrix which doesn't seem to get touched by
invert_matrix_3d or invert_matrix_3d_general, something like this?:

diff --git a/cogl/cogl-matrix.c b/cogl/cogl-matrix.c
index 3753efb..2368bee 100644
--- a/cogl/cogl-matrix.c
+++ b/cogl/cogl-matrix.c
@@ -643,6 +643,9 @@ invert_matrix_3d_general (CoglMatrix *matrix)
                     MAT (in, 1, 3) * MAT (out, 2, 1) +
                     MAT (in, 2, 3) * MAT (out, 2, 2) );

+  MAT (out, 3, 0) = MAT (out, 3, 1) = MAT (out, 3, 2) = 0;
+  MAT (out, 3, 3) = 1;
+
   return TRUE;
 }

@@ -729,6 +732,9 @@ invert_matrix_3d (CoglMatrix *matrix)
   else
     MAT (out, 0, 3) = MAT (out, 1, 3) = MAT (out, 2, 3) = 0.0;

+  MAT (out, 3, 0) = MAT (out, 3, 1) = MAT (out, 3, 2) = 0;
+  MAT (out, 3, 3) = 1;
+
   return TRUE;
 }

I haven't tested this though and I'm not sure how much difference that
might make so I don't really mind if we just stick with your patch as
is.

At some point it would be good to create some micro benchmarks for
inverting different types of matrices so we'd be able to quantify if
optimizing this matters much.

Please feel free to land your patch as is or alternatively test and
land a fixup based patch instead.

Reviewed-by: Robert Bragg <robert at linux.intel.com>

kind regards,
- Robert

On Wed, Jun 6, 2012 at 5:34 PM, Damien Lespiau <damien.lespiau at gmail.com> wrote:
> From: Damien Lespiau <damien.lespiau at intel.com>
>
> Contrary to the other inversion functions, invert_matrix_3d() does not
> initialize the inverse to the identity and then only touches the
> elements it cares about. Problem is the ww component is left alone,
> which makes everything go to a black hole when using an inverse matrix
> as the transform matrix of a framebuffer.
>
> This is how cameras are typically implemented, they have a transform
> and the framebuffer matrix stack is initialized with the inverse
> of that transform. A gdb session gives away what happens:
>
> The camera model view matrix, slightly rotation around the X axis:
> camera mv       1.000000 0.000000 0.000000 0.000000
> camera mv       0.000000 0.984808 -0.173648 0.000000
> camera mv       0.000000 0.173648 0.984808 10.000000
> camera mv       0.000000 0.000000 0.000000 1.000000
>
> Breakpoint 5, invert_matrix_3d (matrix=0x8056b58) at ./cogl-matrix.c:671
> 671       const float *in = (float *)matrix;
> (gdb) p *matrix
> $1 = {xx = 1, yx = 0, zx = 0, wx = 0, xy = 0, yy = 0.98480773,
>  zy = 0.173648164, wy = 0, xz = 0, yz = -0.173648164, zz = 0.98480773,
>  wz = 0, xw = 0, yw = 0, zw = 10, ww = 1, inv = {0 <repeats 16 times>},
>  type = 6, flags = 1030, _padding3 = 0}
> (gdb) finish
> Run till exit from #0  invert_matrix_3d (matrix=0x8056b58)
>    at ./cogl-matrix.c:671
> 0x00141ced in _cogl_matrix_update_inverse (matrix=0x8056b58)
>    at ./cogl-matrix.c:1123
> 1123          if (inv_mat_tab[matrix->type](matrix))
> Value returned is $2 = 1
> (gdb) p *matrix
> $3 = {xx = 1, yx = 0, zx = 0, wx = 0, xy = 0, yy = 0.98480773,
>  zy = 0.173648164, wy = 0, xz = 0, yz = -0.173648164, zz = 0.98480773,
>  wz = 0, xw = 0, yw = 0, zw = 10, ww = 1, inv = {1, 0, 0, 0, 0, 0.98480773,
>    -0.173648164, 0, 0, 0.173648164, 0.98480773, 0, -0, -1.73648167,
>    -9.84807777, 0}, type = 6, flags = 1030, _padding3 = 0}
> ---
>  cogl/cogl-matrix.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/cogl/cogl-matrix.c b/cogl/cogl-matrix.c
> index 3f0deb5..6c7118b 100644
> --- a/cogl/cogl-matrix.c
> +++ b/cogl/cogl-matrix.c
> @@ -665,6 +665,8 @@ invert_matrix_3d (CoglMatrix *matrix)
>   const float *in = (float *)matrix;
>   float *out = matrix->inv;
>
> +  memcpy (out, identity, 16 * sizeof (float));
> +
>   if (!TEST_MAT_FLAGS(matrix, MAT_FLAGS_ANGLE_PRESERVING))
>     return invert_matrix_3d_general (matrix);
>
> --
> 1.7.7.5
>
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl