[PATCH v26 0/4] MR4661: gdiplus: Implement Gdiplus effects

[Vijay Kiran Kamuju (＠infyquest)]

Implement few effect related functions in Gdiplus.

--
  v26: gdiplus: Add GdipSetEffectsParameters implementation.
       gdiplus: Add GdipGetEffectParameterSize implementation.
       gdiplus: Add GdipCreateEffect implementation.
       include: Add gdiplus effect parameter structs.

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

[PATCH v26 1/4] include: Add gdiplus effect parameter structs.

[Vijay Kiran Kamuju]

From: Vijay Kiran Kamuju

---
 include/gdipluscolormatrix.h |  1 +
 include/gdipluseffects.h     | 74 ++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/include/gdipluscolormatrix.h b/include/gdipluscolormatrix.h
index 44016ff3451..391095ed8fb 100644
--- a/include/gdipluscolormatrix.h
+++ b/include/gdipluscolormatrix.h
@@ -62,6 +62,7 @@ enum HistogramFormat

#ifndef __cplusplus

+typedef BYTE ColorChannelLUT[256];
 typedef enum ColorAdjustType ColorAdjustType;
 typedef enum ColorMatrixFlags ColorMatrixFlags;
 typedef enum HistogramFormat HistogramFormat;
diff --git a/include/gdipluseffects.h b/include/gdipluseffects.h
index d2d6a0500da..4100647f7a1 100644
--- a/include/gdipluseffects.h
+++ b/include/gdipluseffects.h
@@ -31,6 +31,80 @@ DEFINE_GUID(ColorBalanceEffectGuid,           0x537e597d, 0x251e, 0x48da, 0x96,
 DEFINE_GUID(RedEyeCorrectionEffectGuid,       0x74d29d05, 0x69a4, 0x4266, 0x95, 0x49, 0x3c, 0xc5, 0x28, 0x36, 0xb6, 0x32);
 DEFINE_GUID(ColorCurveEffectGuid,             0xdd6a0022, 0x58e4, 0x4a67, 0x9d, 0x9b, 0xd4, 0x8e, 0xb8, 0x81, 0xa5, 0x3d);

+struct BlurParams {
+    float radius;
+    BOOL expandEdge;
+};
+
+struct SharpenParams {
+    float radius;
+    float amount;
+};
+
+struct TintParams {
+    INT hue;
+    INT amount;
+};
+
+struct RedEyeCorrectionParams {
+    UINT numberOfAreas;
+    RECT *areas;
+};
+
+struct ColorLUTParams {
+   ColorChannelLUT lutB;
+   ColorChannelLUT lutG;
+   ColorChannelLUT lutR;
+   ColorChannelLUT lutA;
+};
+
+struct BrightnessContrastParams {
+    INT brightnessLevel;
+    INT contrastLevel;
+};
+
+struct HueSaturationLightnessParams {
+    INT hueLevel;
+    INT saturationLevel;
+    INT lightnessLevel;
+};
+
+struct ColorBalanceParams {
+    INT cyanRed;
+    INT magentaGreen;
+    INT yellowBlue;
+};
+
+struct LevelsParams {
+    INT highlight;
+    INT midtone;
+    INT shadow;
+};
+
+typedef enum CurveAdjustments {
+   AdjustExposure,
+   AdjustDensity,
+   AdjustContrast,
+   AdjustHighlight,
+   AdjustShadow,
+   AdjustMidtone,
+   AdjustWhiteSaturation,
+   AdjustBlackSaturation
+} CurveAdjustments;
+
+typedef enum CurveChannel {
+  CurveChannelAll,
+  CurveChannelRed,
+  CurveChannelGreen,
+  CurveChannelBlue
+} CurveChannel;
+
+struct ColorCurveParams {
+    CurveAdjustments adjustment;
+    CurveChannel channel;
+    INT adjustValue;
+};
+
 #ifdef __cplusplus
 extern "C" {
 #endif

[PATCH v26 2/4] gdiplus: Add GdipCreateEffect implementation.

[Vijay Kiran Kamuju]

From: Vijay Kiran Kamuju

---
 dlls/gdiplus/gdiplus_private.h | 19 +++++++++
 dlls/gdiplus/image.c           | 71 ++++++++++++++++++++++++++++++----
 dlls/gdiplus/tests/image.c     |  7 ++--
 3 files changed, 86 insertions(+), 11 deletions(-)

diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h
index 2b5dbee43e9..778a2ef5298 100644
--- a/dlls/gdiplus/gdiplus_private.h
+++ b/dlls/gdiplus/gdiplus_private.h
@@ -368,6 +368,25 @@ struct GpAdjustableArrowCap{
     REAL width;
 };

+typedef enum EffectType {
+    NoneEffect,
+    BlurEffect,
+    SharpenEffect,
+    TintEffect,
+    RedEyeCorrectionEffect,
+    ColorMatrixEffect,
+    ColorLUTEffect,
+    BrightnessContrastEffect,
+    HueSaturationLightnessEffect,
+    ColorBalanceEffect,
+    LevelsEffect,
+    ColorCurveEffect,
+} EffectType;
+
+typedef struct CGpEffect{
+    EffectType type;
+} CGpEffect;
+
 struct GpImage{
     IWICBitmapDecoder *decoder;
     IWICBitmapEncoder *encoder;
diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c
index 806da0cb696..47f7e92195f 100644
--- a/dlls/gdiplus/image.c
+++ b/dlls/gdiplus/image.c
@@ -5576,14 +5576,69 @@ GpStatus WINGDIPAPI GdipCreateBitmapFromHBITMAP(HBITMAP hbm, HPALETTE hpal, GpBi
  */
 GpStatus WINGDIPAPI GdipCreateEffect(const GUID guid, CGpEffect **effect)
 {
-    FIXME("(%s, %p): stub\n", debugstr_guid(&guid), effect);
+    CGpEffect *ef = NULL;
+    EffectType type;

-    if(!effect)
+    TRACE("(%s, %p)\n", debugstr_guid(&guid), effect);
+
+    if (!effect)
         return InvalidParameter;

-    *effect = NULL;
+    if (IsEqualGUID(&guid, &BlurEffectGuid))
+    {
+        type = BlurEffect;
+    }
+    else if (IsEqualGUID(&guid, &SharpenEffectGuid))
+    {
+        type = SharpenEffect;
+    }
+    else if (IsEqualGUID(&guid, &TintEffectGuid))
+    {
+        type = TintEffect;
+    }
+    else if (IsEqualGUID(&guid, &RedEyeCorrectionEffectGuid))
+    {
+        type = RedEyeCorrectionEffect;
+    }
+    else if (IsEqualGUID(&guid, &ColorMatrixEffectGuid))
+    {
+        type = ColorMatrixEffect;
+    }
+    else if (IsEqualGUID(&guid, &ColorLUTEffectGuid))
+    {
+        type = ColorLUTEffect;
+    }
+    else if (IsEqualGUID(&guid, &BrightnessContrastEffectGuid))
+    {
+        type = BrightnessContrastEffect;
+    }
+    else if (IsEqualGUID(&guid, &HueSaturationLightnessEffectGuid))
+    {
+        type = HueSaturationLightnessEffect;
+    }
+    else if (IsEqualGUID(&guid, &ColorBalanceEffectGuid))
+    {
+        type = ColorBalanceEffect;
+    }
+    else if (IsEqualGUID(&guid, &LevelsEffectGuid))
+    {
+        type = LevelsEffect;
+    }
+    else if (IsEqualGUID(&guid, &ColorCurveEffectGuid))
+    {
+        type = ColorCurveEffect;
+    }
+    else
+    {
+        *effect = NULL;
+        return Win32Error;
+    }

-    return NotImplemented;
+    ef = malloc(sizeof(CGpEffect));
+    ef->type = type;
+    *effect = ef;
+
+    return Ok;
 }

/*****************************************************************************
@@ -5591,10 +5646,10 @@ GpStatus WINGDIPAPI GdipCreateEffect(const GUID guid, CGpEffect **effect)
  */
 GpStatus WINGDIPAPI GdipDeleteEffect(CGpEffect *effect)
 {
-    FIXME("(%p): stub\n", effect);
-    /* note: According to Jose Roca's GDI+ Docs, this is not implemented
-     * in Windows's gdiplus */
-    return NotImplemented;
+    TRACE("(%p)\n", effect);
+
+    free(effect);
+    return Ok;
 }

/*****************************************************************************
diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c
index 8fc9aa78225..d0ca8f2e409 100644
--- a/dlls/gdiplus/tests/image.c
+++ b/dlls/gdiplus/tests/image.c
@@ -5408,7 +5408,7 @@ static void test_createeffect(void)
     GpStatus (WINAPI *pGdipCreateEffect)( const GUID guid, CGpEffect **effect);
     GpStatus (WINAPI *pGdipDeleteEffect)( CGpEffect *effect);
     GpStatus stat;
-    CGpEffect *effect;
+    CGpEffect *effect = NULL;
     HMODULE mod = GetModuleHandleA("gdiplus.dll");
     int i;
     const GUID * const effectlist[] =
@@ -5429,12 +5429,13 @@ static void test_createeffect(void)
     expect(InvalidParameter, stat);

stat = pGdipCreateEffect(noneffect, &effect);
-    todo_wine expect(Win32Error, stat);
+    expect(Win32Error, stat);
+    ok(effect == NULL, "Expected effect to be NULL\n");

for(i=0; i < ARRAY_SIZE(effectlist); i++)
     {
         stat = pGdipCreateEffect(*effectlist[i], &effect);
-        todo_wine expect(Ok, stat);
+        expect(Ok, stat);
         if(stat == Ok)
         {
             stat = pGdipDeleteEffect(effect);

[PATCH v26 3/4] gdiplus: Add GdipGetEffectParameterSize implementation.

[Vijay Kiran Kamuju]

From: Vijay Kiran Kamuju

---
 dlls/gdiplus/gdiplus.spec  |  2 +-
 dlls/gdiplus/image.c       | 57 ++++++++++++++++++++++++++++++++++++++
 dlls/gdiplus/tests/image.c | 46 ++++++++++++++++++++++++++----
 include/gdipluseffects.h   |  1 +
 4 files changed, 99 insertions(+), 7 deletions(-)

diff --git a/dlls/gdiplus/gdiplus.spec b/dlls/gdiplus/gdiplus.spec
index 178592e35d9..0f17bb02d4d 100644
--- a/dlls/gdiplus/gdiplus.spec
+++ b/dlls/gdiplus/gdiplus.spec
@@ -612,7 +612,7 @@
 612 stdcall GdipGetImageItemData(ptr ptr)
 613 stdcall GdipCreateEffect(int128 ptr)
 614 stdcall GdipDeleteEffect(ptr)
-615 stub GdipGetEffectParameterSize
+615 stdcall GdipGetEffectParameterSize(ptr ptr)
 616 stub GdipGetEffectParameters
 617 stdcall GdipSetEffectParameters(ptr ptr long)
 618 stdcall GdipInitializePalette(ptr long long long ptr)
diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c
index 47f7e92195f..f3cf246bf77 100644
--- a/dlls/gdiplus/image.c
+++ b/dlls/gdiplus/image.c
@@ -5652,6 +5652,63 @@ GpStatus WINGDIPAPI GdipDeleteEffect(CGpEffect *effect)
     return Ok;
 }

+/*****************************************************************************
+ * GdipGetEffectParameterSize [GDIPLUS.@]
+ */
+GpStatus WINGDIPAPI GdipGetEffectParameterSize(CGpEffect *effect, UINT *size)
+{
+    UINT sz = 0;
+    GpStatus status = Ok;
+
+    TRACE("(%p %p)\n", effect, size);
+
+    if (!effect || !size)
+         return InvalidParameter;
+
+    switch (effect->type)
+    {
+    case BlurEffect:
+        sz = sizeof(struct BlurParams);
+        break;
+    case SharpenEffect:
+        sz = sizeof(struct SharpenParams);
+        break;
+    case TintEffect:
+        sz = sizeof(struct TintParams);
+        break;
+    case RedEyeCorrectionEffect:
+        sz = sizeof(struct RedEyeCorrectionParams);
+        break;
+    case ColorMatrixEffect:
+        sz = sizeof(ColorMatrix);
+        break;
+    case ColorLUTEffect:
+        sz = sizeof(struct ColorLUTParams);
+        break;
+    case BrightnessContrastEffect:
+        sz = sizeof(struct BrightnessContrastParams);
+        break;
+    case HueSaturationLightnessEffect:
+        sz = sizeof(struct HueSaturationLightnessParams);
+        break;
+    case ColorBalanceEffect:
+        sz = sizeof(struct ColorBalanceParams);
+        break;
+    case LevelsEffect:
+        sz = sizeof(struct LevelsParams);
+        break;
+    case ColorCurveEffect:
+        sz = sizeof(struct ColorCurveParams);
+        break;
+    default:
+        status = InvalidParameter;
+        break;
+    }
+
+    *size = sz;
+    return status;
+}
+
 /*****************************************************************************
  * GdipSetEffectParameters [GDIPLUS.@]
  */
diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c
index d0ca8f2e409..471f9909c80 100644
--- a/dlls/gdiplus/tests/image.c
+++ b/dlls/gdiplus/tests/image.c
@@ -5407,17 +5407,38 @@ static void test_createeffect(void)
     static const GUID noneffect = { 0xcd0c3d4b, 0xe15e, 0x4cf2, { 0x9e, 0xa8, 0x6e, 0x1d, 0x65, 0x48, 0xc5, 0xa5 } };
     GpStatus (WINAPI *pGdipCreateEffect)( const GUID guid, CGpEffect **effect);
     GpStatus (WINAPI *pGdipDeleteEffect)( CGpEffect *effect);
+    GpStatus (WINAPI *pGdipGetEffectParameterSize)( CGpEffect *effect, UINT *size);
     GpStatus stat;
     CGpEffect *effect = NULL;
+    UINT size;
     HMODULE mod = GetModuleHandleA("gdiplus.dll");
     int i;
-    const GUID * const effectlist[] =
-               {&BlurEffectGuid, &SharpenEffectGuid, &ColorMatrixEffectGuid, &ColorLUTEffectGuid,
-                &BrightnessContrastEffectGuid, &HueSaturationLightnessEffectGuid, &LevelsEffectGuid,
-                &TintEffectGuid, &ColorBalanceEffectGuid, &RedEyeCorrectionEffectGuid, &ColorCurveEffectGuid};
+
+    static const struct test_data {
+        const GUID *guid;
+        UINT size;
+    } td[] =
+    {
+        { &BlurEffectGuid, 8 },
+        { &SharpenEffectGuid, 8 },
+        { &ColorMatrixEffectGuid, 100 },
+        { &ColorLUTEffectGuid, 1024 },
+        { &BrightnessContrastEffectGuid, 8 },
+        { &HueSaturationLightnessEffectGuid, 12 },
+        { &LevelsEffectGuid, 12 },
+        { &TintEffectGuid, 8 },
+        { &ColorBalanceEffectGuid, 12 },
+#ifdef _WIN64
+        { &RedEyeCorrectionEffectGuid, 16 },
+#else
+        { &RedEyeCorrectionEffectGuid, 8 },
+#endif
+        { &ColorCurveEffectGuid, 12 }
+    };

pGdipCreateEffect = (void*)GetProcAddress( mod, "GdipCreateEffect");
     pGdipDeleteEffect = (void*)GetProcAddress( mod, "GdipDeleteEffect");
+    pGdipGetEffectParameterSize = (void*)GetProcAddress( mod, "GdipGetEffectParameterSize");
     if(!pGdipCreateEffect || !pGdipDeleteEffect)
     {
         /* GdipCreateEffect/GdipDeleteEffect was introduced in Windows Vista. */
@@ -5428,16 +5449,29 @@ static void test_createeffect(void)
     stat = pGdipCreateEffect(BlurEffectGuid, NULL);
     expect(InvalidParameter, stat);

+    stat = pGdipGetEffectParameterSize(NULL, NULL);
+    expect(InvalidParameter, stat);
+
+    stat = pGdipGetEffectParameterSize(effect, NULL);
+    expect(InvalidParameter, stat);
+
+    effect = (CGpEffect *)0xdeadbeef;
     stat = pGdipCreateEffect(noneffect, &effect);
     expect(Win32Error, stat);
     ok(effect == NULL, "Expected effect to be NULL\n");

-    for(i=0; i < ARRAY_SIZE(effectlist); i++)
+    for(i=0; i < ARRAY_SIZE(td); i++)
     {
-        stat = pGdipCreateEffect(*effectlist[i], &effect);
+        stat = pGdipCreateEffect(*td[i].guid, &effect);
         expect(Ok, stat);
+
         if(stat == Ok)
         {
+            size = 0;
+            stat = pGdipGetEffectParameterSize(effect, &size);
+            expect(Ok, stat);
+            expect(td[i].size, size);
+
             stat = pGdipDeleteEffect(effect);
             expect(Ok, stat);
         }
diff --git a/include/gdipluseffects.h b/include/gdipluseffects.h
index 4100647f7a1..a58d25cb0e3 100644
--- a/include/gdipluseffects.h
+++ b/include/gdipluseffects.h
@@ -111,6 +111,7 @@ extern "C" {

GpStatus WINGDIPAPI GdipCreateEffect(const GUID guid, CGpEffect **effect);
 GpStatus WINGDIPAPI GdipDeleteEffect(CGpEffect *effect);
+GpStatus WINGDIPAPI GdipGetEffectParameterSize(CGpEffect *effect, UINT *size);

#ifdef __cplusplus
 }

[PATCH v26 4/4] gdiplus: Add GdipSetEffectsParameters implementation.

[Vijay Kiran Kamuju]

From: Vijay Kiran Kamuju

---
 dlls/gdiplus/gdiplus_private.h |  1 +
 dlls/gdiplus/image.c           | 39 ++++++++++++--
 dlls/gdiplus/tests/image.c     | 92 ++++++++++++++++++++++++++++------
 include/gdipluseffects.h       |  1 +
 4 files changed, 114 insertions(+), 19 deletions(-)

diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h
index 778a2ef5298..16ecdf010b9 100644
--- a/dlls/gdiplus/gdiplus_private.h
+++ b/dlls/gdiplus/gdiplus_private.h
@@ -384,6 +384,7 @@ typedef enum EffectType {
 } EffectType;

typedef struct CGpEffect{
+    void *params;
     EffectType type;
 } CGpEffect;

diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c
index f3cf246bf77..5f11f61bd0c 100644
--- a/dlls/gdiplus/image.c
+++ b/dlls/gdiplus/image.c
@@ -5635,6 +5635,7 @@ GpStatus WINGDIPAPI GdipCreateEffect(const GUID guid, CGpEffect **effect)
     }

ef = malloc(sizeof(CGpEffect));
+    ef->params = NULL;
     ef->type = type;
     *effect = ef;

@@ -5648,6 +5649,7 @@ GpStatus WINGDIPAPI GdipDeleteEffect(CGpEffect *effect)
 {
     TRACE("(%p)\n", effect);

+    free(effect->params);
     free(effect);
     return Ok;
 }
@@ -5678,6 +5680,8 @@ GpStatus WINGDIPAPI GdipGetEffectParameterSize(CGpEffect *effect, UINT *size)
         break;
     case RedEyeCorrectionEffect:
         sz = sizeof(struct RedEyeCorrectionParams);
+        if (effect->params)
+            sz += (((struct RedEyeCorrectionParams *)effect->params)->numberOfAreas)*sizeof(RECT);
         break;
     case ColorMatrixEffect:
         sz = sizeof(ColorMatrix);
@@ -5704,8 +5708,8 @@ GpStatus WINGDIPAPI GdipGetEffectParameterSize(CGpEffect *effect, UINT *size)
         status = InvalidParameter;
         break;
     }
-
     *size = sz;
+
     return status;
 }

@@ -5715,14 +5719,39 @@ GpStatus WINGDIPAPI GdipGetEffectParameterSize(CGpEffect *effect, UINT *size)
 GpStatus WINGDIPAPI GdipSetEffectParameters(CGpEffect *effect,
     const VOID *params, const UINT size)
 {
-    static int calls;
+    UINT paramsize = 0, num = 0;

TRACE("(%p,%p,%u)\n", effect, params, size);

-    if(!(calls++))
-        FIXME("not implemented\n");
+    if (!effect || !params || !size)
+        return InvalidParameter;

-    return NotImplemented;
+    if (GdipGetEffectParameterSize(effect, &paramsize) != Ok)
+        return InvalidParameter;
+
+    if (effect->type == RedEyeCorrectionEffect)
+    {
+        if ((paramsize-size > 0) || (((size-paramsize)%sizeof(RECT)) != 0))
+            return InvalidParameter;
+    }
+    else
+    {
+        if (paramsize != size)
+            return InvalidParameter;
+    }
+
+    effect->params = realloc(effect->params, size);
+    if (effect->type == RedEyeCorrectionEffect)
+    {
+        num = (size-paramsize)/sizeof(RECT);
+        ((struct RedEyeCorrectionParams *)effect->params)->numberOfAreas = num;
+        ((struct RedEyeCorrectionParams *)effect->params)->areas = malloc(num*sizeof(RECT));
+        memcpy(((struct RedEyeCorrectionParams *)params)->areas, ((struct RedEyeCorrectionParams *)effect->params)->areas, num*sizeof(RECT));
+    }
+    else
+        memcpy(effect->params, params, size);
+
+    return Ok;
 }

/*****************************************************************************
diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c
index 471f9909c80..747cefd9fad 100644
--- a/dlls/gdiplus/tests/image.c
+++ b/dlls/gdiplus/tests/image.c
@@ -5408,37 +5408,95 @@ static void test_createeffect(void)
     GpStatus (WINAPI *pGdipCreateEffect)( const GUID guid, CGpEffect **effect);
     GpStatus (WINAPI *pGdipDeleteEffect)( CGpEffect *effect);
     GpStatus (WINAPI *pGdipGetEffectParameterSize)( CGpEffect *effect, UINT *size);
+    GpStatus (WINAPI *pGdipSetEffectParameters)( CGpEffect *effect, const VOID *params, const UINT size);
     GpStatus stat;
     CGpEffect *effect = NULL;
     UINT size;
     HMODULE mod = GetModuleHandleA("gdiplus.dll");
     int i;

-    static const struct test_data {
+    RECT rects[2] = { { 0, 0, 75, 75 }, { -32, -32, 32, 32 } };
+    struct BlurParams blur = { 2.0, TRUE };
+    struct SharpenParams sharpen = { 5.0, 2 };
+    ColorMatrix matrix =
+    {
+        {
+            { 1.0, 1.0, 1.0, 1.0, 1.0 },
+            { 2.0, 2.0, 2.0, 2.0, 2.0 },
+            { 3.0, 3.0, 3.0, 3.0, 3.0 },
+            { 4.0, 4.0, 4.0, 4.0, 4.0 },
+            { 5.0, 5.0, 5.0, 5.0, 5.0 }
+        }
+    };
+    struct ColorLUTParams lut =
+    {
+        { 0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          4,4,4,4,4,4,4,4,5,5,5,5,5,5,5,5,6,6,6,6,6,6,6,6,7,7,7,7,7,7,7,7,
+          0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          4,4,4,4,4,4,4,4,5,5,5,5,5,5,5,5,6,6,6,6,6,6,6,6,7,7,7,7,7,7,7,7,
+          0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          4,4,4,4,4,4,4,4,5,5,5,5,5,5,5,5,6,6,6,6,6,6,6,6,7,7,7,7,7,7,7,7,
+          0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          4,4,4,4,4,4,4,4,5,5,5,5,5,5,5,5,6,6,6,6,6,6,6,6,7,7,7,7,7,7,7,7 },
+        { 0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          4,4,4,4,4,4,4,4,5,5,5,5,5,5,5,5,6,6,6,6,6,6,6,6,7,7,7,7,7,7,7,7,
+          8,8,8,8,8,8,8,8,9,9,9,9,9,9,9,9,10,10,10,10,10,10,10,10,11,11,11,11,11,11,11,11,
+          12,12,12,12,12,12,12,12,13,13,13,13,13,13,13,13,14,14,14,14,14,14,14,14,15,15,15,15,15,15,15,15,
+          0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          4,4,4,4,4,4,4,4,5,5,5,5,5,5,5,5,6,6,6,6,6,6,6,6,7,7,7,7,7,7,7,7,
+          8,8,8,8,8,8,8,8,9,9,9,9,9,9,9,9,10,10,10,10,10,10,10,10,11,11,11,11,11,11,11,11,
+          12,12,12,12,12,12,12,12,13,13,13,13,13,13,13,13,14,14,14,14,14,14,14,14,15,15,15,15,15,15,15,15 },
+        { 0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3,
+          0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,2,2,2,2,2,2,2,2,3,3,3,3,3,3,3,3 },
+        { 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+          0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+          0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+          0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+          0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+          0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+          0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+          0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 }
+    };
+    struct BrightnessContrastParams bc = { 75, 50 };
+    struct HueSaturationLightnessParams hsl = { 50, 50, 50 };
+    struct LevelsParams lvl = { 50, 50, 5 };
+    struct TintParams tint = { 20, 5 };
+    struct ColorBalanceParams cbal = { 50, 50, 50 };
+    struct ColorCurveParams ccurve = { AdjustExposure, CurveChannelAll, 5 };
+    struct RedEyeCorrectionParams redi = { 2, rects };
+    const struct test_data {
         const GUID *guid;
         UINT size;
+        void *params;
     } td[] =
     {
-        { &BlurEffectGuid, 8 },
-        { &SharpenEffectGuid, 8 },
-        { &ColorMatrixEffectGuid, 100 },
-        { &ColorLUTEffectGuid, 1024 },
-        { &BrightnessContrastEffectGuid, 8 },
-        { &HueSaturationLightnessEffectGuid, 12 },
-        { &LevelsEffectGuid, 12 },
-        { &TintEffectGuid, 8 },
-        { &ColorBalanceEffectGuid, 12 },
+        { &BlurEffectGuid, 8, &blur },
+        { &SharpenEffectGuid, 8, &sharpen },
+        { &ColorMatrixEffectGuid, 100, &matrix },
+        { &ColorLUTEffectGuid, 1024, &lut },
+        { &BrightnessContrastEffectGuid, 8, &bc },
+        { &HueSaturationLightnessEffectGuid, 12, &hsl },
+        { &LevelsEffectGuid, 12, &lvl },
+        { &TintEffectGuid, 8, &tint },
+        { &ColorBalanceEffectGuid, 12, &cbal },
 #ifdef _WIN64
-        { &RedEyeCorrectionEffectGuid, 16 },
+        { &RedEyeCorrectionEffectGuid, 16, &redi },
 #else
-        { &RedEyeCorrectionEffectGuid, 8 },
+        { &RedEyeCorrectionEffectGuid, 8, &redi },
 #endif
-        { &ColorCurveEffectGuid, 12 }
+        { &ColorCurveEffectGuid, 12, &ccurve }
     };

pGdipCreateEffect = (void*)GetProcAddress( mod, "GdipCreateEffect");
     pGdipDeleteEffect = (void*)GetProcAddress( mod, "GdipDeleteEffect");
     pGdipGetEffectParameterSize = (void*)GetProcAddress( mod, "GdipGetEffectParameterSize");
+    pGdipSetEffectParameters = (void*)GetProcAddress( mod, "GdipSetEffectParameters");
     if(!pGdipCreateEffect || !pGdipDeleteEffect)
     {
         /* GdipCreateEffect/GdipDeleteEffect was introduced in Windows Vista. */
@@ -5452,7 +5510,7 @@ static void test_createeffect(void)
     stat = pGdipGetEffectParameterSize(NULL, NULL);
     expect(InvalidParameter, stat);

-    stat = pGdipGetEffectParameterSize(effect, NULL);
+    stat = pGdipSetEffectParameters(NULL, NULL, 0);
     expect(InvalidParameter, stat);

effect = (CGpEffect *)0xdeadbeef;
@@ -5472,6 +5530,12 @@ static void test_createeffect(void)
             expect(Ok, stat);
             expect(td[i].size, size);

+            stat = pGdipSetEffectParameters(effect, td[i].params, td[i].size);
+            expect(Ok, stat);
+            size = 0;
+            stat = pGdipGetEffectParameterSize(effect, &size);
+            expect(td[i].size, size);
+
             stat = pGdipDeleteEffect(effect);
             expect(Ok, stat);
         }
diff --git a/include/gdipluseffects.h b/include/gdipluseffects.h
index a58d25cb0e3..2b94479f7f5 100644
--- a/include/gdipluseffects.h
+++ b/include/gdipluseffects.h
@@ -112,6 +112,7 @@ extern "C" {
 GpStatus WINGDIPAPI GdipCreateEffect(const GUID guid, CGpEffect **effect);
 GpStatus WINGDIPAPI GdipDeleteEffect(CGpEffect *effect);
 GpStatus WINGDIPAPI GdipGetEffectParameterSize(CGpEffect *effect, UINT *size);
+GpStatus WINGDIPAPI GdipSetEffectParameters(CGpEffect *effect, const VOID *params, const UINT size);

#ifdef __cplusplus
 }

[PATCH v26 4/4] gdiplus: Add GdipSetEffectsParameters implementation.

[Marvin]

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=143204

Your paranoid android.

=== w7u_2qxl (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w7u_adm (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w7u_el (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w8 (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w8adm (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w864 (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w1064v1507 (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w1064v1809 (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w1064_tsign (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w10pro64 (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w10pro64_en_AE_u8 (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w11pro64 (32 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w7pro64 (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w864 (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w1064v1507 (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w1064v1809 (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w1064_2qxl (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w1064_adm (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w1064_tsign (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w10pro64 (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w10pro64_ar (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w10pro64_ja (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w10pro64_zh_CN (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

=== w11pro64_amd (64 bit report) ===

gdiplus:
image.c:5534: Test failed: Expected 0, got 2

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/gdiplus_private.h:
> +typedef enum EffectType {
> - NoneEffect,
> - BlurEffect,
> - SharpenEffect,
> - TintEffect,
> - RedEyeCorrectionEffect,
> - ColorMatrixEffect,
> - ColorLUTEffect,
> - BrightnessContrastEffect,
> - HueSaturationLightnessEffect,
> - ColorBalanceEffect,
> - LevelsEffect,
> - ColorCurveEffect,
>
> +} EffectType;
> -
>
> +typedef struct CGpEffect{

Based on looking at the surrounding code...
1. I believe the convention is to just use `struct` rather than `typedef struct`
2. `GpEffect` seems more consistent here than `CGpEffect`.

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/image.c:
> - else if (IsEqualGUID(&guid, &LevelsEffectGuid))
> - {
> -
>    type = LevelsEffect;
> - }
> - else if (IsEqualGUID(&guid, &ColorCurveEffectGuid))
> - {
> -
>    type = ColorCurveEffect;
> - }
> - else
> - {
> -
>    *effect = NULL;
> -
>    return Win32Error;
> - }
> - return NotImplemented;
> - ef = malloc(sizeof(CGpEffect));

Should check for NULL result (out of memory).

[Jeffrey Smith (＠whydoubt)]

On Tue Jan 23 17:28:18 2024 +0000, Esme Povirk wrote:
> Don't check for NULL if you're only calling `free`.

Wouldn't it still be good to check that `effect` is non-NULL though, to avoid possible NULL dereference?

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/image.c:
> - return NotImplemented;
> - if (GdipGetEffectParameterSize(effect, &paramsize) != Ok)
> -
>    return InvalidParameter;
> -
> - if (effect->type == RedEyeCorrectionEffect)
> - {
> -
>    if ((paramsize-size > 0) || (((size-paramsize)%sizeof(RECT)) != 0))
> -
>        return InvalidParameter;
> - }
> - else
> - {
> -
>    if (paramsize != size)
> -
>        return InvalidParameter;
> - }
> -
> - effect->params = realloc(effect->params, size);

The effects of realloc failing should be accounted for.
1. `realloc` can return NULL, in which case you'll want to `return OutOfMemory`.
2. When `realloc` does return NULL, the pointer passed to realloc has not been freed, and the current code will leave the location unreachable.

Consider https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/gdiplus/stringformat.... for reference.

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/image.c:
> -
>    if ((paramsize-size > 0) || (((size-paramsize)%sizeof(RECT)) != 0))
> -
>        return InvalidParameter;
> - }
> - else
> - {
> -
>    if (paramsize != size)
> -
>        return InvalidParameter;
> - }
> -
> - effect->params = realloc(effect->params, size);
> - if (effect->type == RedEyeCorrectionEffect)
> - {
> -
>    num = (size-paramsize)/sizeof(RECT);
> -
>    ((struct RedEyeCorrectionParams *)effect->params)->numberOfAreas = num;
> -
>    ((struct RedEyeCorrectionParams *)effect->params)->areas = malloc(num*sizeof(RECT));
> -
>    memcpy(((struct RedEyeCorrectionParams *)params)->areas, ((struct RedEyeCorrectionParams *)effect->params)->areas, num*sizeof(RECT));

Another. Need check for NULL result (out of memory).

[Jeffrey Smith (＠whydoubt)]

Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/image.c:
> -
>    if ((paramsize-size > 0) || (((size-paramsize)%sizeof(RECT)) != 0))
> -
>        return InvalidParameter;
> - }
> - else
> - {
> -
>    if (paramsize != size)
> -
>        return InvalidParameter;
> - }
> -
> - effect->params = realloc(effect->params, size);
> - if (effect->type == RedEyeCorrectionEffect)
> - {
> -
>    num = (size-paramsize)/sizeof(RECT);
> -
>    ((struct RedEyeCorrectionParams *)effect->params)->numberOfAreas = num;
> -
>    ((struct RedEyeCorrectionParams *)effect->params)->areas = malloc(num*sizeof(RECT));
> -
>    memcpy(((struct RedEyeCorrectionParams *)params)->areas, ((struct RedEyeCorrectionParams *)effect->params)->areas, num*sizeof(RECT));

`((struct RedEyeCorrectionParams *)effect->params)` appears in all three of these lines.  It might improve readability here to make a variable out of this.

[Jeffrey Smith (＠whydoubt)]

On Mon Feb 26 19:46:21 2024 +0000, Jeffrey Smith wrote:
> Another. Need check for NULL result (out of memory).

Marked the wrong line, I meant for the `malloc` in the previous line.

[Nikolay Sivov (＠nsivov)]

This does not look like the most readable and maintainable way to do it, structurally. The main problem I see is that effect type is checked everywhere, instead some static configuration could be used with an index to it, or something similar.

[Jeffrey Smith (＠whydoubt)]

On Mon Feb 26 19:04:00 2024 +0000, Jeffrey Smith wrote:
> Based on looking at the surrounding code...
> - I believe the convention is to just use `struct` rather than `typedef struct`
> - `GpEffect` seems more consistent here than `CGpEffect`.

- With (1), the typedef is handled in `gdiplusgpstubs.h`, so it's not needed here.
- Nevermind on (2).