[PATCH 0/1] MR3613: gdiplus: improve performance by switching loops and fix size
The performance improvement was verified with following code: ``` #include <stdio.h> #include <stdlib.h> #include <time.h> /* for(y) for(x) 9.05 */ /* for(x) for(y) 39.69 */ int main() { long width, height; long has_alpha; long x, y; unsigned int * src; width= 5000; height = 999999; src = (unsigned int*)malloc(width*height*sizeof(unsigned int)); has_alpha = 0; clock_t t; t = clock(); for (x=0; x<width && !has_alpha; x++) for (y=0; y<height && !has_alpha; y++) { //printf(" %i", *src); if ((src[y*width + x] & 0xff000000) != 0) has_alpha = 1; } t = clock() - t; double time_taken = ((double)t)/CLOCKS_PER_SEC; // in seconds printf("fun() took %f seconds to execute \n", time_taken); return 0; } ``` It could be verified with Online compilers: https://godbolt.org/ https://www.programiz.com/c-programming/online-compiler/#google_vignette -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3613
From: Bartosz Kosiorek <gang65(a)poczta.onet.pl> --- dlls/gdiplus/graphics.c | 20 ++++++++++---------- dlls/gdiplus/image.c | 8 ++++---- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index e3b5661fd67..65b33cdc960 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -771,8 +771,8 @@ PixelFormat apply_image_attributes(const GpImageAttributes *attributes, LPBYTE d max_green = (key->high>>8)&0xff; max_red = (key->high>>16)&0xff; - for (x=0; x<width; x++) - for (y=0; y<height; y++) + for (y=0; y<height; y++) + for (x=0; x<width; x++) { ARGB *src_color; BYTE blue, green, red; @@ -799,8 +799,8 @@ PixelFormat apply_image_attributes(const GpImageAttributes *attributes, LPBYTE d else table = &attributes->colorremaptables[ColorAdjustTypeDefault]; - for (x=0; x<width; x++) - for (y=0; y<height; y++) + for (y=0; y<height; y++) + for (x=0; x<width; x++) { ARGB *src_color; src_color = (ARGB*)(data + stride * y + sizeof(ARGB) * x); @@ -838,9 +838,9 @@ PixelFormat apply_image_attributes(const GpImageAttributes *attributes, LPBYTE d if (!identity) { - for (x=0; x<width; x++) + for (y=0; y<height; y++) { - for (y=0; y<height; y++) + for (x=0; x<width; x++) { ARGB *src_color; src_color = (ARGB*)(data + stride * y + sizeof(ARGB) * x); @@ -872,8 +872,8 @@ PixelFormat apply_image_attributes(const GpImageAttributes *attributes, LPBYTE d else gamma = attributes->gamma[ColorAdjustTypeDefault]; - for (x=0; x<width; x++) - for (y=0; y<height; y++) + for (y=0; y<height; y++) + for (x=0; x<width; x++) { ARGB *src_color; BYTE blue, green, red; @@ -1201,8 +1201,8 @@ static GpStatus brush_fill_pixels(GpGraphics *graphics, GpBrush *brush, { int x, y; GpSolidFill *fill = (GpSolidFill*)brush; - for (x=0; x<fill_area->Width; x++) - for (y=0; y<fill_area->Height; y++) + for (y=0; y<fill_area->Height; y++) + for (x=0; x<fill_area->Width; x++) argb_pixels[x + y*cdwStride] = fill->color; return Ok; } diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c index 3dfc2fec763..1aa5f59a4e7 100644 --- a/dlls/gdiplus/image.c +++ b/dlls/gdiplus/image.c @@ -1693,8 +1693,8 @@ GpStatus WINGDIPAPI GdipCreateBitmapFromHICON(HICON hicon, GpBitmap** bitmap) /* If any pixel has a non-zero alpha, ignore hbmMask */ src = (DWORD*)lockeddata.Scan0; - for (x=0; x<width && !has_alpha; x++) - for (y=0; y<height && !has_alpha; y++) + for (y=0; y<height && !has_alpha; y++) + for (x=0; x<width && !has_alpha; x++) if ((*src++ & 0xff000000) != 0) has_alpha = TRUE; } @@ -1723,7 +1723,7 @@ GpStatus WINGDIPAPI GdipCreateBitmapFromHICON(HICON hicon, GpBitmap** bitmap) for (y=0; y<height; y++) { dst = (DWORD*)dst_row; - for (x=0; x<height; x++) + for (x=0; x<width; x++) { DWORD src_value = *src++; if (src_value) @@ -1743,7 +1743,7 @@ GpStatus WINGDIPAPI GdipCreateBitmapFromHICON(HICON hicon, GpBitmap** bitmap) for (y=0; y<height; y++) { dst = (DWORD*)dst_row; - for (x=0; x<height; x++) + for (x=0; x<width; x++) *dst++ |= 0xff000000; dst_row += lockeddata.Stride; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3613
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/image.c:
/* If any pixel has a non-zero alpha, ignore hbmMask */ src = (DWORD*)lockeddata.Scan0; - for (x=0; x<width && !has_alpha; x++) - for (y=0; y<height && !has_alpha; y++) + for (y=0; y<height && !has_alpha; y++) + for (x=0; x<width && !has_alpha; x++)
It desn't give performance improvement, but I would like to align it with the rest of the code. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3613#note_42783
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/image.c:
for (y=0; y<height; y++) { dst = (DWORD*)dst_row; - for (x=0; x<height; x++) + for (x=0; x<width; x++)
Thas it really strange, so I fixed that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3613#note_42784
Funny, over the last few days I've been preparing some patches for `GdipCreateBitmapFromHICON`. They address the changes you made in `dlls/gdiplus/image.c` but are more comprehensive and include tests. I don't have a problem with the changes you made there, but if you took those out it could avoid one of us having to deal with merge conflicts. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3613#note_42807
The changes to `dlls/gdiplus/graphics.c` do seem obviously correct to me, as it makes array access more contiguous. Also note, the commit message could be read as saying that the 'fix size' changes are part of the performance improvement. I've already suggested that you remove the `image.c` changes, but if you do keep them I would at least recommend putting those in a separate commit, since as you said they were separate from the actual performance improvements. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3613#note_42812
This merge request was approved by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3613
On Sun Aug 20 06:45:08 2023 +0000, Bartosz Kosiorek wrote:
It desn't give performance improvement, but I would like to align it with the rest of the code. Maybe there will be difference with single loop? Since the inner loop isn't referencing x or y, it technically doesn't matter, but this more accurately reflects what's happening.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3613#note_42902
participants (4)
-
Bartosz Kosiorek -
Bartosz Kosiorek (＠gang65)
-
Esme Povirk (＠madewokherd)
-
Jeffrey Smith (＠whydoubt)