Re: [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  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.