[PATCH 0/1] MR3613: gdiplus: improve performance by switching loops and fix size

[Bartosz Kosiorek (＠gang65)]

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

[PATCH 1/1] gdiplus: improve performance by switching loops and fix size

[Bartosz Kosiorek]

From: Bartosz Kosiorek

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

[Bartosz Kosiorek (＠gang65)]

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.

[Bartosz Kosiorek (＠gang65)]

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.

[Jeffrey Smith (＠whydoubt)]

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.

[Jeffrey Smith (＠whydoubt)]

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.

[PATCH 0/1] MR3613: gdiplus: improve performance by switching loops and fix size - approved

[Esme Povirk (＠madewokherd)]

This merge request was approved by Esme Povirk.

[Esme Povirk (＠madewokherd)]

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.