[PATCH 0/1] MR2839: gdiplus: Improve performance of GdipInvertMatrix.

[Bartosz Kosiorek (＠gang65)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53947

Performance tests results:
* Without optimization : 0.320s
* With current optimizations: 0.245s

Application used for testing:
[gdiplusdisplay.exe](/uploads/995f3f5d6fbe24ee7ad2513bae51a04f/gdiplusdisplay.exe)

[PATCH 1/1] gdiplus: Improve performance of GdipInvertMatrix.

[Bartosz Kosiorek]

From: Bartosz Kosiorek

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53947
---
 dlls/gdiplus/matrix.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/dlls/gdiplus/matrix.c b/dlls/gdiplus/matrix.c
index d69672bddcf..c5d5d70c648 100644
--- a/dlls/gdiplus/matrix.c
+++ b/dlls/gdiplus/matrix.c
@@ -182,38 +182,41 @@ GpStatus WINGDIPAPI GdipInvertMatrix(GpMatrix *matrix)
 {
     GpMatrix copy;
     REAL det;
-    BOOL invertible;

TRACE("(%p)\n", matrix);

if(!matrix)
         return InvalidParameter;

-    GdipIsMatrixInvertible(matrix, &invertible);
-    if(!invertible)
-        return InvalidParameter;
-
     /* optimize inverting simple scaling and translation matrices */
     if(matrix->matrix[1] == 0 && matrix->matrix[2] == 0)
     {
-        matrix->matrix[4] = -matrix->matrix[4] / matrix->matrix[0];
-        matrix->matrix[5] = -matrix->matrix[5] / matrix->matrix[3];
-        matrix->matrix[0] = 1 / matrix->matrix[0];
-        matrix->matrix[3] = 1 / matrix->matrix[3];
-
-        return Ok;
+        if (matrix->matrix[0] != 0 && matrix->matrix[3] != 0)
+        {
+            matrix->matrix[4] = -matrix->matrix[4] / matrix->matrix[0];
+            matrix->matrix[5] = -matrix->matrix[5] / matrix->matrix[3];
+            matrix->matrix[0] = 1 / matrix->matrix[0];
+            matrix->matrix[3] = 1 / matrix->matrix[3];
+
+            return Ok;
+        }
+        else
+            return InvalidParameter;
     }
-
     det = matrix_det(matrix);
+    if (!(fabs(det) >= 1e-5))
+        return InvalidParameter;
+
+    det = 1 / det;

copy = *matrix;
     /* store result */
-    matrix->matrix[0] =   copy.matrix[3] / det;
-    matrix->matrix[1] =  -copy.matrix[1] / det;
-    matrix->matrix[2] =  -copy.matrix[2] / det;
-    matrix->matrix[3] =   copy.matrix[0] / det;
-    matrix->matrix[4] =  (copy.matrix[2]*copy.matrix[5]-copy.matrix[3]*copy.matrix[4]) / det;
-    matrix->matrix[5] = -(copy.matrix[0]*copy.matrix[5]-copy.matrix[1]*copy.matrix[4]) / det;
+    matrix->matrix[0] =   copy.matrix[3] * det;
+    matrix->matrix[1] =  -copy.matrix[1] * det;
+    matrix->matrix[2] =  -copy.matrix[2] * det;
+    matrix->matrix[3] =   copy.matrix[0] * det;
+    matrix->matrix[4] =  (copy.matrix[2]*copy.matrix[5]-copy.matrix[3]*copy.matrix[4]) * det;
+    matrix->matrix[5] = -(copy.matrix[0]*copy.matrix[5]-copy.matrix[1]*copy.matrix[4]) * det;

return Ok;
 }

[Nikolay Sivov (＠nsivov)]

Instead of duplicating checks, it's better to move functional part of GdipIsMatrixInvertible() to some inlined helper.

[Bartosz Kosiorek (＠gang65)]

Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/matrix.c:
> -
> -
>    return Ok;
> -
>    if (matrix->matrix[0] != 0 && matrix->matrix[3] != 0)
> -
>    {
> -
>        matrix->matrix[4] = -matrix->matrix[4] / matrix->matrix[0];
> -
>        matrix->matrix[5] = -matrix->matrix[5] / matrix->matrix[3];
> -
>        matrix->matrix[0] = 1 / matrix->matrix[0];
> -
>        matrix->matrix[3] = 1 / matrix->matrix[3];
> -
> -
>        return Ok;
> -
>    }
> -
>    else
> -
>        return InvalidParameter;
> }
> - det = matrix_det(matrix);

We are calculating `matrix_det` only once (previously it was done also inside GdipIsMatrixInvertible)

[Bartosz (＠gang65)]

On Wed May 17 09:07:24 2023 +0000, Nikolay Sivov wrote:
> Instead of duplicating checks, it's better to move functional part of
> GdipIsMatrixInvertible() to some inlined helper.

I have tried such approach, but unfortunately I don't know how to do not calculate `matrix_det(matrix)` twice.

Maybe we could introduce function and provide calculated `det`, like:
```
static inline GpStatus check_det(GDIPCONST REAL det)
{
    if (!(fabs(det) >= 1e-5))
        return InvalidParameter;
    return Ok;
}
```

[Esme Povirk (＠madewokherd)]

On Wed May 17 09:15:05 2023 +0000, Bartosz Kosiorek wrote:
> I have tried such approach, but unfortunately I don't know how to do not
> calculate `matrix_det(matrix)` twice.
> Maybe we could introduce function and provide calculated `det`, like:
> static inline BOOL check_det(GDIPCONST REAL det)
> {
>     return (fabs(det) >= 1e-5);
> }
>
> for
> if(matrix->matrix[1] == 0 && matrix->matrix[2] == 0)
>
> check we are already using that check for optimization. So with proposed
> optimization, we have limited that check to one.

I don't see a way to do this optimization while leaving the logic in GdipIsMatrixInvertible intact. There isn't much logic in there to begin with, so to me it seems fine to duplicate.

[PATCH 0/1] MR2839: gdiplus: Improve performance of GdipInvertMatrix. - approved

[Esme Povirk (＠madewokherd)]

This merge request was approved by Esme Povirk.