[PATCH 0/1] MR2847: gdiplus: Improve performance of gdip_transform_points.
From: Bartosz Kosiorek <gang65(a)poczta.onet.pl> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53947 --- dlls/gdiplus/graphics.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index fca66c8654b..eb208f63fd9 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -6887,13 +6887,15 @@ GpStatus get_graphics_transform(GpGraphics *graphics, GpCoordinateSpace dst_spac if (dst_space != src_space) { - scale_x = units_to_pixels(1.0, graphics->unit, graphics->xres, graphics->printer_display); - scale_y = units_to_pixels(1.0, graphics->unit, graphics->yres, graphics->printer_display); - if(graphics->unit != UnitDisplay) { - scale_x *= graphics->scale; - scale_y *= graphics->scale; + scale_x = units_to_pixels(graphics->scale, graphics->unit, graphics->xres, graphics->printer_display); + scale_y = units_to_pixels(graphics->scale, graphics->unit, graphics->yres, graphics->printer_display); + } + else + { + scale_x = units_to_pixels(1.0, graphics->unit, graphics->xres, graphics->printer_display); + scale_y = units_to_pixels(1.0, graphics->unit, graphics->yres, graphics->printer_display); } if (dst_space < src_space) @@ -6903,12 +6905,11 @@ GpStatus get_graphics_transform(GpGraphics *graphics, GpCoordinateSpace dst_spac { case WineCoordinateSpaceGdiDevice: { - GpMatrix gdixform; - gdixform = graphics->gdi_transform; + GpMatrix gdixform = graphics->gdi_transform; stat = GdipInvertMatrix(&gdixform); if (stat != Ok) break; - GdipMultiplyMatrix(matrix, &gdixform, MatrixOrderAppend); + memcpy(matrix->matrix, gdixform.matrix, 6 * sizeof(REAL)); if (dst_space == CoordinateSpaceDevice) break; /* else fall-through */ @@ -6934,7 +6935,7 @@ GpStatus get_graphics_transform(GpGraphics *graphics, GpCoordinateSpace dst_spac switch ((int)src_space) { case CoordinateSpaceWorld: - GdipMultiplyMatrix(matrix, &graphics->worldtrans, MatrixOrderAppend); + memcpy(matrix->matrix, &graphics->worldtrans, 6 * sizeof(REAL)); if (dst_space == CoordinateSpacePage) break; /* else fall-through */ @@ -6954,7 +6955,7 @@ GpStatus get_graphics_transform(GpGraphics *graphics, GpCoordinateSpace dst_spac return stat; } -GpStatus gdip_transform_points(GpGraphics *graphics, GpCoordinateSpace dst_space, +GpStatus inline gdip_transform_points(GpGraphics *graphics, GpCoordinateSpace dst_space, GpCoordinateSpace src_space, GpPointF *points, INT count) { GpMatrix matrix; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2847
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/graphics.c:
if (dst_space != src_space) { - scale_x = units_to_pixels(1.0, graphics->unit, graphics->xres, graphics->printer_display); - scale_y = units_to_pixels(1.0, graphics->unit, graphics->yres, graphics->printer_display); - if(graphics->unit != UnitDisplay) { - scale_x *= graphics->scale; - scale_y *= graphics->scale;
Don't waste time for multiply floating numbers if it is not needed. Instead invoke `units_to_pixels` functions with `1.0` or `graphics->scale` values -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2847#note_33023
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/graphics.c:
{ case WineCoordinateSpaceGdiDevice: { - GpMatrix gdixform; - gdixform = graphics->gdi_transform; + GpMatrix gdixform = graphics->gdi_transform; stat = GdipInvertMatrix(&gdixform); if (stat != Ok) break; - GdipMultiplyMatrix(matrix, &gdixform, MatrixOrderAppend); + memcpy(matrix->matrix, gdixform.matrix, 6 * sizeof(REAL));
In previous implementation we have multiplied [Identity Matrix](https://en.wikipedia.org/wiki/Identity_matrix) with GDIMatrix, and the result was always the same (GDIMatrix). Instead of expensive Matrix Multiplication, just copy matrix, and boost perfmance. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2847#note_33025
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/graphics.c:
switch ((int)src_space) { case CoordinateSpaceWorld: - GdipMultiplyMatrix(matrix, &graphics->worldtrans, MatrixOrderAppend); + memcpy(matrix->matrix, &graphics->worldtrans, 6 * sizeof(REAL));
Instead of expensive Matrix Multiplication by [Identity Matrix](https://en.wikipedia.org/wiki/Identity_matrix), just copy matrix, and boost performance. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2847#note_33026
On Wed May 17 17:43:45 2023 +0000, Bartosz Kosiorek wrote:
In previous implementation we have multiplied [Identity Matrix](https://en.wikipedia.org/wiki/Identity_matrix) with GDIMatrix, and the result was always the same (GDIMatrix). Instead of expensive Matrix Multiplication, just copy matrix, and boost perfmance. I'd suggest `sizeof(matrix->matrix)` rather than hard-coding the size calculation.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2847#note_33029
Esme Povirk (@madewokherd) commented about dlls/gdiplus/graphics.c:
return stat; }
-GpStatus gdip_transform_points(GpGraphics *graphics, GpCoordinateSpace dst_space, +GpStatus inline gdip_transform_points(GpGraphics *graphics, GpCoordinateSpace dst_space,
I'm not sure if there are any implications to marking a non-static function as inline. I couldn't find another example of it in Wine code. But, the function isn't used in any other file so it's probably fine to make it static inline. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2847#note_33038
In the future, I'd prefer that you don't use gitlab's comment feature to explain your code. Either I should be able to figure out what the code is doing and why by reading it, or the code needs a comment. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2847#note_33039
While I'm not too familiar with the GDI+ code in Wine, I don't see any glaring issues with these changes. I agree that `6 * sizeof(REAL)` should be replaced with `sizeof(matrix->matrix)`, but either way this is reviewed-by me. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2847#note_33040
participants (4)
-
Bartosz Kosiorek -
Bartosz Kosiorek (＠gang65)
-
Esme Povirk (＠madewokherd)
-
Torge Matthies (＠tmatthies)