[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 at 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.