From: Ziqing Hui <zhui@codeweavers.com>
Subject: Re: [PATCH 2/3] d2d1: Implement effect creation for registered effect.
Message-Id: <3fb6a54b-f482-b2a4-bad4-512cfd341458@codeweavers.com>
Date: Tue, 21 Jun 2022 17:34:01 +0800
In-Reply-To: <e31e9919-3cf7-9188-ff3b-197d5cdb29e5@codeweavers.com>
References: <20220621031708.2241872-1-zhui@codeweavers.com> <20220621031708.2241872-2-zhui@codeweavers.com> <e31e9919-3cf7-9188-ff3b-197d5cdb29e5@codeweavers.com>



On 6/21/22 4:45 PM, Nikolay Sivov wrote:
> 
> 
> On 6/21/22 06:17, Ziqing Hui wrote:
>>   HRESULT d2d_effect_init(struct d2d_effect *effect, struct d2d_effect_context *effect_context, const CLSID *effect_id)
>>   {
>> -    unsigned int i;
>> +    struct d2d_effect_registration *reg;
>> +    struct d2d_factory *factory;
>> +    HRESULT hr;
>>         effect->ID2D1Effect_iface.lpVtbl = &d2d_effect_vtbl;
>>       effect->ID2D1Image_iface.lpVtbl = &d2d_effect_image_vtbl;
>>       effect->refcount = 1;
>>   -    for (i = 0; i < ARRAY_SIZE(builtin_effects); ++i)
>> +    factory = unsafe_impl_from_ID2D1Factory3((ID2D1Factory3 *)effect_context->device_context->factory);
>> +    LIST_FOR_EACH_ENTRY(reg, &factory->effects, struct d2d_effect_registration, entry)
>>       {
>> -        if (IsEqualGUID(effect_id, &builtin_effects[i].id))
>> +        if (IsEqualGUID(effect_id, &reg->id))
>>           {
>> -            effect->min_inputs = builtin_effects[i].min_inputs;
>> -            effect->max_inputs = builtin_effects[i].max_inputs;
>> -            d2d_effect_SetInputCount(&effect->ID2D1Effect_iface, builtin_effects[i].default_input_count);
>> +            if (FAILED(hr = reg->factory((IUnknown **)&effect->impl)))
>> +                return hr;
>> +            if (FAILED(hr = ID2D1EffectImpl_Initialize(effect->impl, &effect_context->ID2D1EffectContext_iface, NULL)))
>> +            {
>> +                ID2D1EffectImpl_Release(effect->impl);
>> +                return hr;
>> +            }
>> +            effect->id = *effect_id;
>> +            effect->min_inputs = reg->min_inputs;
>> +            effect->max_inputs = reg->max_inputs;
>> +            d2d_effect_SetInputCount(&effect->ID2D1Effect_iface, reg->default_input_count);
>>               effect->effect_context = effect_context;
>>               ID2D1EffectContext_AddRef(&effect_context->ID2D1EffectContext_iface);
>> +            /* FIXME: Properties are ignored. */
>>               return S_OK;
>>           }
>>       }
> CreateEffect() is a single place where this would be used, so I'd suggest to pick correct d2d_effect_registration there, and simply pass it to this helper.
> 
> Using unsafe_* is normally reserved for externally provided pointers, that's not the case here.
> 
OK, I'll do that.

>>   static const struct d2d_builtin_effect_registration builtin_effects[] =
>>   {
>> -    {&CLSID_D2D12DAffineTransform,      NULL, 1, 1, 1},
>> -    {&CLSID_D2D13DPerspectiveTransform, NULL, 1, 1, 1},
>> -    {&CLSID_D2D1Composite,              NULL, 2, 1, 0xffffffff},
>> -    {&CLSID_D2D1Crop,                   NULL, 1, 1, 1},
>> -    {&CLSID_D2D1Shadow,                 NULL, 1, 1, 1},
>> -    {&CLSID_D2D1Grayscale,              NULL, 1, 1, 1},
>> +    {&CLSID_D2D12DAffineTransform,      d2d_effect_impl_create,              1, 1, 1},
>> +    {&CLSID_D2D13DPerspectiveTransform, d2d_effect_impl_create,              1, 1, 1},
>> +    {&CLSID_D2D1Composite,              d2d_effect_impl_create,              2, 1, 0xffffffff},
>> +    {&CLSID_D2D1Crop,                   d2d_effect_impl_create,              1, 1, 1},
>> +    {&CLSID_D2D1Shadow,                 d2d_effect_impl_create,              1, 1, 1},
>> +    {&CLSID_D2D1Grayscale,              d2d_effect_impl_create,              1, 1, 1},
>>   };
> I don't know how extensible this is, every _create would be different.


Yeah, each builtin effect would have a different _create. So the _create here in this patch is just a placeholder, which makes sure every builtin effect in the list can be at least successfully created for now.

My plan is something like, we take 2DAffineTransform as an example: we define a new struct for each effect:

struct d2d_2d_affine_transform
{
    struct d2d_effect_impl effect_impl;

    ... its own fileds ...
}

d2d_effect_impl will be included in the beginning of each effect struct. The benefit of this is that we can reuse one QueryInterface, AddRef, Release, we don't have to define a new one for each new effect.

And of course, _create will be different for each effect.