[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)

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2839

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

[Bartosz Kosiorek]

From: Bartosz Kosiorek <gang65(a)poczta.onet.pl>

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;
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/2839

[Nikolay Sivov (＠nsivov)]

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

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2839#note_32968

[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)

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2839#note_32969

[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;
}
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2839#note_32977

[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.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2839#note_33018

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

[Esme Povirk (＠madewokherd)]

This merge request was approved by Esme Povirk.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2839