[PATCH v2 1/3] d2d1/tests: Add failure tests for ID2D1Effect_GetValue().

[Ziqing Hui]

Signed-off-by: Ziqing Hui <zhui(a)codeweavers.com>
---
 dlls/d2d1/tests/d2d1.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c
index 72487692abf..629bbd65523 100644
--- a/dlls/d2d1/tests/d2d1.c
+++ b/dlls/d2d1/tests/d2d1.c
@@ -9680,13 +9680,14 @@ static void test_mt_factory(BOOL d3d11)
 
 static void test_effect(BOOL d3d11)
 {
-    unsigned int i, min_inputs, max_inputs;
+    unsigned int i, min_inputs, max_inputs, str_size;
     D2D1_BUFFER_PRECISION precision;
     ID2D1Image *image_a, *image_b;
     struct d2d1_test_context ctx;
     ID2D1DeviceContext *context;
     ID2D1Factory1 *factory;
     ID2D1Effect *effect;
+    BYTE buffer[64];
     BOOL cached;
     CLSID clsid;
     HRESULT hr;
@@ -9735,6 +9736,33 @@ static void test_effect(BOOL d3d11)
 
         todo_wine
         {
+        hr = ID2D1Effect_GetValue(effect, 0xdeadbeef, D2D1_PROPERTY_TYPE_CLSID, (BYTE *)&clsid, sizeof(clsid));
+        ok(hr == D2DERR_INVALID_PROPERTY, "Got unexpected hr %#x.\n", hr);
+
+        hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, buffer, sizeof(clsid) + 1);
+        ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr);
+        hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, buffer, sizeof(clsid) - 1);
+        ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr);
+        hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, buffer, sizeof(clsid));
+        ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
+
+        hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_DISPLAYNAME, D2D1_PROPERTY_TYPE_STRING, buffer, sizeof(buffer));
+        ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
+        str_size = (wcslen((WCHAR *)buffer) + 1) * sizeof(WCHAR);
+        hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_DISPLAYNAME, D2D1_PROPERTY_TYPE_STRING, buffer, str_size);
+        ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
+        hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_DISPLAYNAME, D2D1_PROPERTY_TYPE_STRING, buffer, str_size - 1);
+        ok(hr == D2DERR_INSUFFICIENT_BUFFER, "Got unexpected hr %#x.\n", hr);
+
+        hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, 0xdeadbeef, (BYTE *)&clsid, sizeof(clsid));
+        ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr);
+        hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_UNKNOWN, (BYTE *)&clsid, sizeof(clsid));
+        ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
+        hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_VECTOR4, (BYTE *)&clsid, sizeof(clsid));
+        ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr);
+        hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_VECTOR4, buffer, sizeof(buffer));
+        ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr);
+
         hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID,
                 D2D1_PROPERTY_TYPE_CLSID, (BYTE *)&clsid, sizeof(clsid));
         ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
-- 
2.25.1

[PATCH v2 2/3] d2d1/tests: Add tests for effect inputs.

[Ziqing Hui]

Signed-off-by: Ziqing Hui <zhui(a)codeweavers.com>
---
 dlls/d2d1/tests/d2d1.c | 59 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c
index 629bbd65523..7971d1396bc 100644
--- a/dlls/d2d1/tests/d2d1.c
+++ b/dlls/d2d1/tests/d2d1.c
@@ -9680,13 +9680,16 @@ static void test_mt_factory(BOOL d3d11)
 
 static void test_effect(BOOL d3d11)
 {
-    unsigned int i, min_inputs, max_inputs, str_size;
+    unsigned int i, j, min_inputs, max_inputs, str_size, input_count, off_limit_tests = 4;
+    D2D1_BITMAP_PROPERTIES bitmap_desc;
     D2D1_BUFFER_PRECISION precision;
     ID2D1Image *image_a, *image_b;
     struct d2d1_test_context ctx;
     ID2D1DeviceContext *context;
     ID2D1Factory1 *factory;
+    ID2D1Bitmap *bitmap;
     ID2D1Effect *effect;
+    D2D1_SIZE_U size;
     BYTE buffer[64];
     BOOL cached;
     CLSID clsid;
@@ -9797,6 +9800,60 @@ static void test_effect(BOOL d3d11)
                     max_inputs, test->max_inputs);
         }
 
+        todo_wine
+        {
+        input_count = ID2D1Effect_GetInputCount(effect);
+        ok (input_count == 1 || input_count == 2, "Got unexpected input count %u.\n", input_count);
+
+        input_count = (test->max_inputs < 16 ? test->max_inputs : 16);
+        for (j = 0; j < input_count + off_limit_tests; ++j)
+        {
+            hr = ID2D1Effect_SetInputCount(effect, j);
+            if (j < test->min_inputs || j > test->max_inputs)
+                ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr);
+            else
+                ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
+        }
+
+        input_count = ID2D1Effect_GetInputCount(effect);
+        for (j = 0; j < input_count + off_limit_tests; ++j)
+        {
+            ID2D1Effect_GetInput(effect, j, &image_a);
+            ok(image_a == NULL, "Got unexpected image_a %p.\n", image_a);
+        }
+        }
+
+        set_size_u(&size, 1, 1);
+        bitmap_desc.pixelFormat.format = DXGI_FORMAT_B8G8R8A8_UNORM;
+        bitmap_desc.pixelFormat.alphaMode = D2D1_ALPHA_MODE_IGNORE;
+        bitmap_desc.dpiX = 96.0f;
+        bitmap_desc.dpiY = 96.0f;
+        hr = ID2D1RenderTarget_CreateBitmap(ctx.rt, size, NULL, 4, &bitmap_desc, &bitmap);
+        ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
+
+        todo_wine
+        {
+        ID2D1Effect_SetInput(effect, 0, (ID2D1Image *)bitmap, FALSE);
+        for (j = 0; j < input_count + off_limit_tests; ++j)
+        {
+            image_a = (ID2D1Image *)0xdeadbeef;
+            if (j >= input_count)
+                ID2D1Effect_SetInput(effect, j, (ID2D1Image *)bitmap, FALSE);
+            ID2D1Effect_GetInput(effect, j, &image_a);
+            if (j == 0)
+            {
+                ok(image_a == (ID2D1Image *)bitmap, "Got unexpected image_a %p.\n", image_a);
+                if (image_a == (ID2D1Image *)bitmap)
+                    ID2D1Image_Release(image_a);
+            }
+            else
+            {
+                ok(image_a == NULL, "Got unexpected image_a %p.\n", image_a);
+            }
+        }
+        ID2D1Bitmap_Release(bitmap);
+        }
+
         ID2D1Effect_Release(effect);
         winetest_pop_context();
     }
-- 
2.25.1

[PATCH v2 2/3] d2d1/tests: Add tests for effect inputs.

[Henri Verbeet]

On Sat, 31 Jul 2021 at 06:25, Ziqing Hui <zhui(a)codeweavers.com> wrote:
> @@ -9797,6 +9800,60 @@ static void test_effect(BOOL d3d11)
>                      max_inputs, test->max_inputs);
>          }
>
> +        todo_wine
> +        {
> +        input_count = ID2D1Effect_GetInputCount(effect);
> +        ok (input_count == 1 || input_count == 2, "Got unexpected input count %u.\n", input_count);
> +
> What determines whether this returns 1 or 2?

As an aside, I'd much prefer putting "todo_wine" before individual
ok() calls over using block-todo_wine.
> +        input_count = (test->max_inputs < 16 ? test->max_inputs : 16);
> +        for (j = 0; j < input_count + off_limit_tests; ++j)
> +        {
> What is the meaning of the "off_limit_tests" variable? It's not
> obvious to me from the variable name.
> +            hr = ID2D1Effect_SetInputCount(effect, j);
> +            if (j < test->min_inputs || j > test->max_inputs)
> +                ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr);
> +            else
> +                ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
> +        }
> If these were to fail, it would not be obvious from the failure
> message for which input/"j" that was.
> +        ID2D1Effect_SetInput(effect, 0, (ID2D1Image *)bitmap, FALSE);
> +        for (j = 0; j < input_count + off_limit_tests; ++j)
> +        {
> +            image_a = (ID2D1Image *)0xdeadbeef;
> +            if (j >= input_count)
> +                ID2D1Effect_SetInput(effect, j, (ID2D1Image *)bitmap, FALSE);
> +            ID2D1Effect_GetInput(effect, j, &image_a);
> +            if (j == 0)
> +            {
> +                ok(image_a == (ID2D1Image *)bitmap, "Got unexpected image_a %p.\n", image_a);
> +                if (image_a == (ID2D1Image *)bitmap)
> +                    ID2D1Image_Release(image_a);
> +            }
> +            else
> +            {
> +                ok(image_a == NULL, "Got unexpected image_a %p.\n", image_a);
> +            }
> +        }
> It may end up being nicer to use two separate loops for this.

[PATCH v2 2/3] d2d1/tests: Add tests for effect inputs.

[Ziqing Hui]

On 8/3/21 11:52 PM, Henri Verbeet wrote:
> On Sat, 31 Jul 2021 at 06:25, Ziqing Hui <zhui(a)codeweavers.com> wrote:
> > @@ -9797,6 +9800,60 @@ static void test_effect(BOOL d3d11)
> >                      max_inputs, test->max_inputs);
> >          }
> >
> > +        todo_wine
> > +        {
> > +        input_count = ID2D1Effect_GetInputCount(effect);
> > +        ok (input_count == 1 || input_count == 2, "Got unexpected input count %u.\n", input_count);
> > +
> > What determines whether this returns 1 or 2?

It would return 2 for the Composite effect, and 1 for the other two effects.

I'm not sure if there would be other value for builtin effects that we have not tested.
> As an aside, I'd much prefer putting "todo_wine" before individual
> ok() calls over using block-todo_wine.
> > +        input_count = (test->max_inputs < 16 ? test->max_inputs : 16);
> > +        for (j = 0; j < input_count + off_limit_tests; ++j)
> > +        {
> > What is the meaning of the "off_limit_tests" variable? It's not
> > obvious to me from the variable name.

It means the count of input index which is out of bound (index which is greater than, or equal to max_inputs).

[PATCH v2 2/3] d2d1/tests: Add tests for effect inputs.

[Ziqing Hui]

On 8/4/21 9:27 AM, Ziqing Hui wrote:
> > As an aside, I'd much prefer putting "todo_wine" before individual
> > ok() calls over using block-todo_wine.
> > > +        input_count = (test->max_inputs < 16 ? test->max_inputs : 16);
> > > +        for (j = 0; j < input_count + off_limit_tests; ++j)
> > > +        {
> > > What is the meaning of the "off_limit_tests" variable? It's not
> > > obvious to me from the variable name.
> > > It means the count of input index which is out of bound (index which is greater than, or equal to max_inputs).

Also, the bound can beinput_count.

Generally, this variable means the count of input index which is out of bound.

The bound can be max_inputs or input_count.

[PATCH v2 2/3] d2d1/tests: Add tests for effect inputs.

[Henri Verbeet]

On Wed, 4 Aug 2021 at 03:27, Ziqing Hui <zhui(a)codeweavers.com> wrote:
> On 8/3/21 11:52 PM, Henri Verbeet wrote:
> > On Sat, 31 Jul 2021 at 06:25, Ziqing Hui <zhui(a)codeweavers.com> wrote:
> > > @@ -9797,6 +9800,60 @@ static void test_effect(BOOL d3d11)
> > >                      max_inputs, test->max_inputs);
> > >          }
> > >
> > > +        todo_wine
> > > +        {
> > > +        input_count = ID2D1Effect_GetInputCount(effect);
> > > +        ok (input_count == 1 || input_count == 2, "Got unexpected input count %u.\n", input_count);
> > > +
> > > What determines whether this returns 1 or 2?
>
> It would return 2 for the Composite effect, and 1 for the other two effects.

We might as well store that in effect_tests[] then.

[PATCH v2 3/3] d2d1: Implement d2d_effect_{Get,Set}Input{,Count}().

[Ziqing Hui]

Signed-off-by: Ziqing Hui <zhui(a)codeweavers.com>
---
 dlls/d2d1/d2d1_private.h |  4 +++
 dlls/d2d1/effect.c       | 61 ++++++++++++++++++++++++++++++++++++----
 dlls/d2d1/tests/d2d1.c   | 10 ++-----
 3 files changed, 61 insertions(+), 14 deletions(-)

diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h
index ce99e7c3432..a80e1b6a18d 100644
--- a/dlls/d2d1/d2d1_private.h
+++ b/dlls/d2d1/d2d1_private.h
@@ -572,6 +572,10 @@ struct d2d_effect
     LONG refcount;
 
     ID2D1Factory *factory;
+    UINT32 min_inputs;
+    UINT32 max_inputs;
+    ID2D1Image **inputs;
+    UINT32 inputs_count;
 };
 
 void d2d_effect_init(struct d2d_effect *effect, ID2D1Factory *factory) DECLSPEC_HIDDEN;
diff --git a/dlls/d2d1/effect.c b/dlls/d2d1/effect.c
index 40dd2187953..50b1c5917f4 100644
--- a/dlls/d2d1/effect.c
+++ b/dlls/d2d1/effect.c
@@ -67,12 +67,19 @@ static ULONG STDMETHODCALLTYPE d2d_effect_Release(ID2D1Effect *iface)
 {
     struct d2d_effect *effect = impl_from_ID2D1Effect(iface);
     ULONG refcount = InterlockedDecrement(&effect->refcount);
+    unsigned int i;
 
     TRACE("%p decreasing refcount to %u.\n", iface, refcount);
 
     if (!refcount)
     {
         ID2D1Factory_Release(effect->factory);
+        for (i = 0; i < effect->inputs_count; ++i)
+        {
+            if (effect->inputs[i])
+                ID2D1Image_Release(effect->inputs[i]);
+        }
+        heap_free(effect->inputs);
         heap_free(effect);
     }
 
@@ -166,26 +173,64 @@ static HRESULT STDMETHODCALLTYPE d2d_effect_GetSubProperties(ID2D1Effect *iface,
 
 static void STDMETHODCALLTYPE d2d_effect_SetInput(ID2D1Effect *iface, UINT32 index, ID2D1Image *input, BOOL invalidate)
 {
-    FIXME("iface %p, index %u, input %p, invalidate %d stub!\n", iface, index, input, invalidate);
+    struct d2d_effect *effect = impl_from_ID2D1Effect(iface);
+
+    TRACE("iface %p, index %u, input %p, invalidate %d.\n", iface, index, input, invalidate);
+
+    if (index < effect->inputs_count)
+    {
+        if (effect->inputs[index])
+            ID2D1Image_Release(effect->inputs[index]);
+        ID2D1Image_AddRef(effect->inputs[index] = input);
+    }
 }
 
 static HRESULT STDMETHODCALLTYPE d2d_effect_SetInputCount(ID2D1Effect *iface, UINT32 count)
 {
-    FIXME("iface %p, count %u stub!\n", iface, count);
+    struct d2d_effect *effect = impl_from_ID2D1Effect(iface);
+    unsigned int i;
 
-    return E_NOTIMPL;
+    TRACE("iface %p, count %u.\n", iface, count);
+
+    if (count < effect->min_inputs || count > effect->max_inputs)
+        return E_INVALIDARG;
+
+    if (count != effect->inputs_count)
+    {
+        if (count < effect->inputs_count)
+        {
+            for (i = count; i < effect->inputs_count; ++i)
+            {
+                if (effect->inputs[i])
+                    ID2D1Image_Release(effect->inputs[i]);
+            }
+        }
+        effect->inputs_count = count;
+        HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, effect->inputs, sizeof(*effect->inputs) * count);
+    }
+
+    return S_OK;
 }
 
 static void STDMETHODCALLTYPE d2d_effect_GetInput(ID2D1Effect *iface, UINT32 index, ID2D1Image **input)
 {
-    FIXME("iface %p, index %u, input %p stub!\n", iface, index, input);
+    struct d2d_effect *effect = impl_from_ID2D1Effect(iface);
+
+    TRACE("iface %p, index %u, input %p.\n", iface, index, input);
+
+    if (index < effect->inputs_count && effect->inputs[index])
+        ID2D1Image_AddRef(*input = effect->inputs[index]);
+    else
+        *input = NULL;
 }
 
 static UINT32 STDMETHODCALLTYPE d2d_effect_GetInputCount(ID2D1Effect *iface)
 {
-    FIXME("iface %p stub!\n", iface);
+    struct d2d_effect *effect = impl_from_ID2D1Effect(iface);
 
-    return 0;
+    TRACE("iface %p.\n", iface);
+
+    return effect->inputs_count;
 }
 
 static void STDMETHODCALLTYPE d2d_effect_GetOutput(ID2D1Effect *iface, ID2D1Image **output)
@@ -274,5 +319,9 @@ void d2d_effect_init(struct d2d_effect *effect, ID2D1Factory *factory)
     effect->ID2D1Effect_iface.lpVtbl = &d2d_effect_vtbl;
     effect->ID2D1Image_iface.lpVtbl = &d2d_effect_image_vtbl;
     effect->refcount = 1;
+    effect->min_inputs = 1;
+    effect->max_inputs = 1;
+    effect->inputs_count = 1;
+    effect->inputs = heap_alloc_zero(sizeof(*effect->inputs));
     ID2D1Factory_AddRef(effect->factory = factory);
 }
diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c
index 7971d1396bc..f265b88796b 100644
--- a/dlls/d2d1/tests/d2d1.c
+++ b/dlls/d2d1/tests/d2d1.c
@@ -9800,8 +9800,6 @@ static void test_effect(BOOL d3d11)
                     max_inputs, test->max_inputs);
         }
 
-        todo_wine
-        {
         input_count = ID2D1Effect_GetInputCount(effect);
         ok (input_count == 1 || input_count == 2, "Got unexpected input count %u.\n", input_count);
 
@@ -9812,6 +9810,7 @@ static void test_effect(BOOL d3d11)
             if (j < test->min_inputs || j > test->max_inputs)
                 ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr);
             else
+                todo_wine_if(test->max_inputs > 1 && j > 1)
                 ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
         }
 
@@ -9821,7 +9820,6 @@ static void test_effect(BOOL d3d11)
             ID2D1Effect_GetInput(effect, j, &image_a);
             ok(image_a == NULL, "Got unexpected image_a %p.\n", image_a);
         }
-        }
 
         set_size_u(&size, 1, 1);
         bitmap_desc.pixelFormat.format = DXGI_FORMAT_B8G8R8A8_UNORM;
@@ -9831,8 +9829,6 @@ static void test_effect(BOOL d3d11)
         hr = ID2D1RenderTarget_CreateBitmap(ctx.rt, size, NULL, 4, &bitmap_desc, &bitmap);
         ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
 
-        todo_wine
-        {
         ID2D1Effect_SetInput(effect, 0, (ID2D1Image *)bitmap, FALSE);
         for (j = 0; j < input_count + off_limit_tests; ++j)
         {
@@ -9843,8 +9839,7 @@ static void test_effect(BOOL d3d11)
             if (j == 0)
             {
                 ok(image_a == (ID2D1Image *)bitmap, "Got unexpected image_a %p.\n", image_a);
-                if (image_a == (ID2D1Image *)bitmap)
-                    ID2D1Image_Release(image_a);
+                ID2D1Image_Release(image_a);
             }
             else
             {
@@ -9852,7 +9847,6 @@ static void test_effect(BOOL d3d11)
             }
         }
         ID2D1Bitmap_Release(bitmap);
-        }
 
         ID2D1Effect_Release(effect);
         winetest_pop_context();
-- 
2.25.1

[PATCH v2 3/3] d2d1: Implement d2d_effect_{Get, Set}Input{, Count}().

[Henri Verbeet]

The patch subject suggests it shouldn't be too hard to split this patch up.

On Sat, 31 Jul 2021 at 06:25, Ziqing Hui <zhui(a)codeweavers.com> wrote:
> @@ -572,6 +572,10 @@ struct d2d_effect
>      LONG refcount;
>
> ID2D1Factory *factory;
> +    UINT32 min_inputs;
> +    UINT32 max_inputs;
> +    ID2D1Image **inputs;
> +    UINT32 inputs_count;
>  };

"input_count"
> @@ -67,12 +67,19 @@ static ULONG STDMETHODCALLTYPE d2d_effect_Release(ID2D1Effect *iface)
>  {
>      struct d2d_effect *effect = impl_from_ID2D1Effect(iface);
>      ULONG refcount = InterlockedDecrement(&effect->refcount);
> +    unsigned int i;
>
> TRACE("%p decreasing refcount to %u.\n", iface, refcount);
>
> if (!refcount)
>      {
>          ID2D1Factory_Release(effect->factory);
> +        for (i = 0; i < effect->inputs_count; ++i)
> +        {
> +            if (effect->inputs[i])
> +                ID2D1Image_Release(effect->inputs[i]);
> +        }
> +        heap_free(effect->inputs);
>          heap_free(effect);
>      }
> We typically clean up in reverse initialisation order, so factory after inputs.
> @@ -166,26 +173,64 @@ static HRESULT STDMETHODCALLTYPE d2d_effect_GetSubProperties(ID2D1Effect *iface,
>
> static void STDMETHODCALLTYPE d2d_effect_SetInput(ID2D1Effect *iface, UINT32 index, ID2D1Image *input, BOOL invalidate)
>  {
> -    FIXME("iface %p, index %u, input %p, invalidate %d stub!\n", iface, index, input, invalidate);
> +    struct d2d_effect *effect = impl_from_ID2D1Effect(iface);
> +
> +    TRACE("iface %p, index %u, input %p, invalidate %d.\n", iface, index, input, invalidate);
> +
> +    if (index < effect->inputs_count)
> +    {
> +        if (effect->inputs[index])
> +            ID2D1Image_Release(effect->inputs[index]);
> +        ID2D1Image_AddRef(effect->inputs[index] = input);
> +    }
>  }
> If we invert the condition above, we can return early and slightly
> reduce the indentation level.
> Release() before AddRef() doesn't seem entirely safe above; consider
> the case that "effect->inputs[index]" holds the only reference to the
> image, and we're setting the same input image again.
> static HRESULT STDMETHODCALLTYPE d2d_effect_SetInputCount(ID2D1Effect *iface, UINT32 count)
>  {
> -    FIXME("iface %p, count %u stub!\n", iface, count);
> +    struct d2d_effect *effect = impl_from_ID2D1Effect(iface);
> +    unsigned int i;
>
> -    return E_NOTIMPL;
> +    TRACE("iface %p, count %u.\n", iface, count);
> +
> +    if (count < effect->min_inputs || count > effect->max_inputs)
> +        return E_INVALIDARG;
> +
> +    if (count != effect->inputs_count)
> +    {
> +        if (count < effect->inputs_count)
> +        {
> +            for (i = count; i < effect->inputs_count; ++i)
> +            {
> +                if (effect->inputs[i])
> +                    ID2D1Image_Release(effect->inputs[i]);
> +            }
> +        }
> +        effect->inputs_count = count;
> +        HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, effect->inputs, sizeof(*effect->inputs) * count);
> +    }
> +
> +    return S_OK;
>  }

That doesn't generally work; the memory returned by HeapReAlloc() may
not be the same as what was originally passed in. More broadly, it's
not clear to me that we gain much from avoiding d2d_array_reserve()
here.

[PATCH v2 3/3] d2d1: Implement d2d_effect_{Get,Set}Input{,Count}().

[Ziqing Hui]

On 8/3/21 11:53 PM, Henri Verbeet wrote:
> > static HRESULT STDMETHODCALLTYPE d2d_effect_SetInputCount(ID2D1Effect *iface, UINT32 count)
> >  {
> > -    FIXME("iface %p, count %u stub!\n", iface, count);
> > +    struct d2d_effect *effect = impl_from_ID2D1Effect(iface);
> > +    unsigned int i;
> >
> > -    return E_NOTIMPL;
> > +    TRACE("iface %p, count %u.\n", iface, count);
> > +
> > +    if (count < effect->min_inputs || count > effect->max_inputs)
> > +        return E_INVALIDARG;
> > +
> > +    if (count != effect->inputs_count)
> > +    {
> > +        if (count < effect->inputs_count)
> > +        {
> > +            for (i = count; i < effect->inputs_count; ++i)
> > +            {
> > +                if (effect->inputs[i])
> > +                    ID2D1Image_Release(effect->inputs[i]);
> > +            }
> > +        }
> > +        effect->inputs_count = count;
> > +        HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, effect->inputs, sizeof(*effect->inputs) * count);
> > +    }
> > +
> > +    return S_OK;
> >  }
>
> That doesn't generally work; the memory returned by HeapReAlloc() may
> not be the same as what was originally passed in. More broadly, it's
> not clear to me that we gain much from avoiding d2d_array_reserve()
> here.

I need to use HeapReAlloc(HEAP_ZERO_MEMORY) to initialize the newly allocated memory to zero.

Which way is better? HeapReAlloc(HEAP_ZERO_MEMORY) or d2d_array_reserve() + memset(0)?

[PATCH v2 3/3] d2d1: Implement d2d_effect_{Get, Set}Input{, Count}().

[Henri Verbeet]

On Wed, 4 Aug 2021 at 04:26, Ziqing Hui <zhui(a)codeweavers.com> wrote:
> On 8/3/21 11:53 PM, Henri Verbeet wrote:
> > > static HRESULT STDMETHODCALLTYPE d2d_effect_SetInputCount(ID2D1Effect *iface, UINT32 count)
> > >  {
> > > -    FIXME("iface %p, count %u stub!\n", iface, count);
> > > +    struct d2d_effect *effect = impl_from_ID2D1Effect(iface);
> > > +    unsigned int i;
> > >
> > > -    return E_NOTIMPL;
> > > +    TRACE("iface %p, count %u.\n", iface, count);
> > > +
> > > +    if (count < effect->min_inputs || count > effect->max_inputs)
> > > +        return E_INVALIDARG;
> > > +
> > > +    if (count != effect->inputs_count)
> > > +    {
> > > +        if (count < effect->inputs_count)
> > > +        {
> > > +            for (i = count; i < effect->inputs_count; ++i)
> > > +            {
> > > +                if (effect->inputs[i])
> > > +                    ID2D1Image_Release(effect->inputs[i]);
> > > +            }
> > > +        }
> > > +        effect->inputs_count = count;
> > > +        HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, effect->inputs, sizeof(*effect->inputs) * count);
> > > +    }
> > > +
> > > +    return S_OK;
> > >  }
> >
> > That doesn't generally work; the memory returned by HeapReAlloc() may
> > not be the same as what was originally passed in. More broadly, it's
> > not clear to me that we gain much from avoiding d2d_array_reserve()
> > here.
>
> I need to use HeapReAlloc(HEAP_ZERO_MEMORY) to initialize the newly allocated memory to zero.
>
> Which way is better? HeapReAlloc(HEAP_ZERO_MEMORY) or d2d_array_reserve() + memset(0)?

I'd use memset().

[Henri Verbeet]

Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>