[PATCH v9 0/4] MR2864: gdiplus: Improve performance of GdipDrawImagePointsRect
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53947 Improve performance of GdipDrawImagePointsRect by: 1. avoiding multiplication and use addition where it is possible. 2. avoid calculating `GdipInvertMatrix` if it is not used It gives noticible speed improvement. I divided MR to several commits, to better undestand what is going on. Application for testing (it needs logo.jpg file): [gdiplusdrawimagepoints.exe](/uploads/545ff7d8ab1d60386366f64999346825/gdiplusdrawimagepoints.exe) -- v9: gdiplus: Improve performance of DrawImagePointsRect by avoid TransformMatrixPoints gdiplus: use float increment instead of calculation to impove perf gdiplus: use iterator instead calculate pointer position every time https://gitlab.winehq.org/wine/wine/-/merge_requests/2864
From: Bartosz Kosiorek <gang65(a)poczta.onet.pl> Calculating transformation points and not using it it is a waste of CPU time. With this patch the code was moved where it is used. It is improving performance of GdipDrawImagePointsRect when resampling is not needed. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53947 --- dlls/gdiplus/graphics.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index 6831cfcb247..11dc06e6c9e 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -3152,14 +3152,10 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image GpRectF graphics_bounds; GpRect src_area; int i, x, y, src_stride, dst_stride; - GpMatrix dst_to_src; - REAL m11, m12, m21, m22, mdx, mdy; LPBYTE src_data, dst_data, dst_dyn_data=NULL; BitmapData lockeddata; InterpolationMode interpolation = graphics->interpolation; PixelOffsetMode offset_mode = graphics->pixeloffset; - GpPointF dst_to_src_points[3] = {{0.0, 0.0}, {1.0, 0.0}, {0.0, 1.0}}; - REAL x_dx, x_dy, y_dx, y_dy; static const GpImageAttributes defaultImageAttributes = {WrapModeClamp, 0, FALSE}; if (!imageAttributes) @@ -3187,18 +3183,6 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image if (IsRectEmpty(&dst_area)) return Ok; - m11 = (ptf[1].X - ptf[0].X) / srcwidth; - m21 = (ptf[2].X - ptf[0].X) / srcheight; - mdx = ptf[0].X - m11 * srcx - m21 * srcy; - m12 = (ptf[1].Y - ptf[0].Y) / srcwidth; - m22 = (ptf[2].Y - ptf[0].Y) / srcheight; - mdy = ptf[0].Y - m12 * srcx - m22 * srcy; - - GdipSetMatrixElements(&dst_to_src, m11, m12, m21, m22, mdx, mdy); - - stat = GdipInvertMatrix(&dst_to_src); - if (stat != Ok) return stat; - if (do_resampling) { get_bitmap_sample_size(interpolation, imageAttributes->wrap, @@ -3248,6 +3232,23 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image if (do_resampling) { + GpMatrix dst_to_src; + REAL m11, m12, m21, m22, mdx, mdy; + GpPointF dst_to_src_points[3] = {{0.0, 0.0}, {1.0, 0.0}, {0.0, 1.0}}; + REAL x_dx, x_dy, y_dx, y_dy; + + m11 = (ptf[1].X - ptf[0].X) / srcwidth; + m21 = (ptf[2].X - ptf[0].X) / srcheight; + m12 = (ptf[1].Y - ptf[0].Y) / srcwidth; + m22 = (ptf[2].Y - ptf[0].Y) / srcheight; + mdx = ptf[0].X - m11 * srcx - m21 * srcy; + mdy = ptf[0].Y - m12 * srcx - m22 * srcy; + + GdipSetMatrixElements(&dst_to_src, m11, m12, m21, m22, mdx, mdy); + + stat = GdipInvertMatrix(&dst_to_src); + if (stat != Ok) return stat; + /* Transform the bits as needed to the destination. */ dst_data = dst_dyn_data = heap_alloc_zero(sizeof(ARGB) * (dst_area.right - dst_area.left) * (dst_area.bottom - dst_area.top)); if (!dst_data) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2864
From: Bartosz Kosiorek <gang65(a)poczta.onet.pl> With previous implementation, every iteration pointer value was calculated by taking row and column of the image. It needs multiply calculation. With current implementation, pointer value calculation, is replaced with iterator, which takes next pixel data. It improves efficiency by using addition instead of multiplication for iterating through points data. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53947 --- dlls/gdiplus/graphics.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index 11dc06e6c9e..c7a8dcf39f3 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -3236,6 +3236,7 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image REAL m11, m12, m21, m22, mdx, mdy; GpPointF dst_to_src_points[3] = {{0.0, 0.0}, {1.0, 0.0}, {0.0, 1.0}}; REAL x_dx, x_dy, y_dx, y_dy; + ARGB *dst_color; m11 = (ptf[1].X - ptf[0].X) / srcwidth; m21 = (ptf[2].X - ptf[0].X) / srcheight; @@ -3249,14 +3250,6 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image stat = GdipInvertMatrix(&dst_to_src); if (stat != Ok) return stat; - /* Transform the bits as needed to the destination. */ - dst_data = dst_dyn_data = heap_alloc_zero(sizeof(ARGB) * (dst_area.right - dst_area.left) * (dst_area.bottom - dst_area.top)); - if (!dst_data) - { - heap_free(src_data); - return OutOfMemory; - } - dst_stride = sizeof(ARGB) * (dst_area.right - dst_area.left); GdipTransformMatrixPoints(&dst_to_src, dst_to_src_points, 3); @@ -3266,19 +3259,25 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image y_dx = dst_to_src_points[2].X - dst_to_src_points[0].X; y_dy = dst_to_src_points[2].Y - dst_to_src_points[0].Y; - for (x=dst_area.left; x<dst_area.right; x++) + /* Transform the bits as needed to the destination. */ + dst_data = dst_dyn_data = heap_alloc_zero(sizeof(ARGB) * (dst_area.right - dst_area.left) * (dst_area.bottom - dst_area.top)); + if (!dst_data) + { + heap_free(src_data); + return OutOfMemory; + } + + dst_color = (ARGB*)(dst_data); + for (y = dst_area.top; y < dst_area.bottom; y++) { - for (y=dst_area.top; y<dst_area.bottom; y++) + for (x = dst_area.left; x < dst_area.right; x++, dst_color += sizeof(ARGB)) { GpPointF src_pointf; - ARGB *dst_color; - src_pointf.X = dst_to_src_points[0].X + x * x_dx + y * y_dx; src_pointf.Y = dst_to_src_points[0].Y + x * x_dy + y * y_dy; - dst_color = (ARGB*)(dst_data + dst_stride * (y - dst_area.top) + sizeof(ARGB) * (x - dst_area.left)); - - if (src_pointf.X >= srcx && src_pointf.X < srcx + srcwidth && src_pointf.Y >= srcy && src_pointf.Y < srcy+srcheight) + if (src_pointf.X >= srcx && src_pointf.X < srcx + srcwidth && + src_pointf.Y >= srcy && src_pointf.Y < srcy + srcheight) *dst_color = resample_bitmap_pixel(&src_area, src_data, bitmap->width, bitmap->height, &src_pointf, imageAttributes, interpolation, offset_mode); else -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2864
From: Bartosz Kosiorek <gang65(a)poczta.onet.pl> Improves efficiency by using addition instead of float numbers multiplication for destination points. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53947 --- dlls/gdiplus/graphics.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index c7a8dcf39f3..d371d78ea8e 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -3237,6 +3237,7 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image GpPointF dst_to_src_points[3] = {{0.0, 0.0}, {1.0, 0.0}, {0.0, 1.0}}; REAL x_dx, x_dy, y_dx, y_dy; ARGB *dst_color; + GpPointF src_pointf_row, src_pointf; m11 = (ptf[1].X - ptf[0].X) / srcwidth; m21 = (ptf[2].X - ptf[0].X) / srcheight; @@ -3266,16 +3267,21 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image heap_free(src_data); return OutOfMemory; } - dst_color = (ARGB*)(dst_data); - for (y = dst_area.top; y < dst_area.bottom; y++) + + /* Calculate top left point of transformed image. + It would be used as reference point for adding */ + src_pointf_row.X = dst_to_src_points[0].X + + dst_area.left * x_dx + dst_area.top * y_dx; + src_pointf_row.Y = dst_to_src_points[0].Y + + dst_area.left * x_dy + dst_area.top * y_dy; + + for (y = dst_area.top; y < dst_area.bottom; + y++, src_pointf_row.X += y_dx, src_pointf_row.Y += y_dy) { - for (x = dst_area.left; x < dst_area.right; x++, dst_color += sizeof(ARGB)) + for (x = dst_area.left, src_pointf = src_pointf_row; x < dst_area.right; + x++, src_pointf.X += x_dx, src_pointf.Y += x_dy, dst_color += sizeof(ARGB)) { - GpPointF src_pointf; - src_pointf.X = dst_to_src_points[0].X + x * x_dx + y * y_dx; - src_pointf.Y = dst_to_src_points[0].Y + x * x_dy + y * y_dy; - if (src_pointf.X >= srcx && src_pointf.X < srcx + srcwidth && src_pointf.Y >= srcy && src_pointf.Y < srcy + srcheight) *dst_color = resample_bitmap_pixel(&src_area, src_data, bitmap->width, bitmap->height, &src_pointf, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2864
From: Bartosz Kosiorek <gang65(a)poczta.onet.pl> Using TransformMatrixPoints is not needed and all values could be taken from transformation matrix: - ShearX from m11, m12 - ShearY from m21, m22 - Translation mdx, mdy The result could be calculated by taking destination points values: {{0.0, 0.0}, {1.0, 0.0}, {0.0, 1.0}} and calculating it with GdipTransformMatrixPoints function: dst_to_src_points[0].X = dst_to_src.matrix[4]; dst_to_src_points[0].Y = dst_to_src.matrix[5]; dst_to_src_points[1].X = dst_to_src.matrix[0] + dst_to_src.matrix[4]; dst_to_src_points[1].Y = dst_to_src.matrix[1] + dst_to_src.matrix[5]; dst_to_src_points[2].X = dst_to_src.matrix[2] + dst_to_src.matrix[4]; dst_to_src_points[2].Y = dst_to_src.matrix[3] + dst_to_src.matrix[5]; --- dlls/gdiplus/graphics.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index d371d78ea8e..9d033af7609 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -3234,14 +3234,13 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image { GpMatrix dst_to_src; REAL m11, m12, m21, m22, mdx, mdy; - GpPointF dst_to_src_points[3] = {{0.0, 0.0}, {1.0, 0.0}, {0.0, 1.0}}; REAL x_dx, x_dy, y_dx, y_dy; ARGB *dst_color; GpPointF src_pointf_row, src_pointf; m11 = (ptf[1].X - ptf[0].X) / srcwidth; - m21 = (ptf[2].X - ptf[0].X) / srcheight; m12 = (ptf[1].Y - ptf[0].Y) / srcwidth; + m21 = (ptf[2].X - ptf[0].X) / srcheight; m22 = (ptf[2].Y - ptf[0].Y) / srcheight; mdx = ptf[0].X - m11 * srcx - m21 * srcy; mdy = ptf[0].Y - m12 * srcx - m22 * srcy; @@ -3252,13 +3251,10 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image if (stat != Ok) return stat; dst_stride = sizeof(ARGB) * (dst_area.right - dst_area.left); - - GdipTransformMatrixPoints(&dst_to_src, dst_to_src_points, 3); - - x_dx = dst_to_src_points[1].X - dst_to_src_points[0].X; - x_dy = dst_to_src_points[1].Y - dst_to_src_points[0].Y; - y_dx = dst_to_src_points[2].X - dst_to_src_points[0].X; - y_dy = dst_to_src_points[2].Y - dst_to_src_points[0].Y; + x_dx = dst_to_src.matrix[0]; + x_dy = dst_to_src.matrix[1]; + y_dx = dst_to_src.matrix[2]; + y_dy = dst_to_src.matrix[3]; /* Transform the bits as needed to the destination. */ dst_data = dst_dyn_data = heap_alloc_zero(sizeof(ARGB) * (dst_area.right - dst_area.left) * (dst_area.bottom - dst_area.top)); @@ -3271,9 +3267,9 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image /* Calculate top left point of transformed image. It would be used as reference point for adding */ - src_pointf_row.X = dst_to_src_points[0].X + + src_pointf_row.X = dst_to_src.matrix[4] + dst_area.left * x_dx + dst_area.top * y_dx; - src_pointf_row.Y = dst_to_src_points[0].Y + + src_pointf_row.Y = dst_to_src.matrix[5] + dst_area.left * x_dy + dst_area.top * y_dy; for (y = dst_area.top; y < dst_area.bottom; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2864
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=132938 Your paranoid android. === debian11 (32 bit report) === gdiplus: image.c:4831: Test failed: 2: data should match image.c:4831: Test failed: 3: data should match image.c:4831: Test failed: 4: data should match image.c:4831: Test failed: 5: data should match image.c:4831: Test failed: 6: data should match image.c:4831: Test failed: 9: data should match image.c:4831: Test failed: 10: data should match image.c:4831: Test failed: 11: data should match image.c:4831: Test failed: 12: data should match image.c:4831: Test failed: 13: data should match image.c:4831: Test failed: 18: data should match image.c:4831: Test failed: 25: data should match Report validation errors: gdiplus:image crashed (c0000005) === debian11 (32 bit zh:CN report) === gdiplus: image.c:4831: Test failed: 2: data should match image.c:4831: Test failed: 3: data should match image.c:4831: Test failed: 4: data should match image.c:4831: Test failed: 5: data should match image.c:4831: Test failed: 6: data should match image.c:4831: Test failed: 9: data should match image.c:4831: Test failed: 10: data should match image.c:4831: Test failed: 11: data should match image.c:4831: Test failed: 12: data should match image.c:4831: Test failed: 13: data should match image.c:4831: Test failed: 18: data should match image.c:4831: Test failed: 25: data should match Report validation errors: gdiplus:image crashed (c0000005) === debian11b (64 bit WoW report) === gdiplus: image.c:4831: Test failed: 2: data should match image.c:4831: Test failed: 3: data should match image.c:4831: Test failed: 4: data should match image.c:4831: Test failed: 5: data should match image.c:4831: Test failed: 6: data should match image.c:4831: Test failed: 9: data should match image.c:4831: Test failed: 10: data should match image.c:4831: Test failed: 11: data should match image.c:4831: Test failed: 12: data should match image.c:4831: Test failed: 13: data should match image.c:4831: Test failed: 18: data should match image.c:4831: Test failed: 25: data should match image: Timeout
participants (3)
-
Bartosz Kosiorek -
Bartosz Kosiorek (＠gang65)
-
Marvin