[PATCH 2/3] d2d1: Implement effect creation for registered effect.

[Nikolay Sivov]

On 6/21/22 12:34, Ziqing Hui wrote:
>
> 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.
>
>
Single stub is fine. Regarding nesting structures, I don't think it buys 
a lot in this case. My understanding is that structures for specific 
effects like d2d_2d_affine_transform will have to hold only current 
property values, updated through setter function. So yes, you will be 
able to reuse QI, AddRef, and maybe Release, unless some effects will 
have properties that need to be released. The rest of the methods will 
be very different between effects. Maybe it's easier to pick some simple 
effect and make that work first, without adding stubs that will have to 
be removed? From existing list Grayscale one is probably the easiest.