From: Nikolay Sivov <nsivov@codeweavers.com>
Subject: Re: [PATCH 1/3] d2d1: Support registering builtin effect.
Message-Id: <baa486ca-b451-491f-46bf-cbc2f4917301@codeweavers.com>
Date: Wed, 22 Jun 2022 13:57:00 +0300
In-Reply-To: <acc40eeb-efb0-1a64-a891-3edab6a4ad8b@codeweavers.com>
References: <20220621031708.2241872-1-zhui@codeweavers.com> <9284cf85-4f5e-a385-0dbb-285eb13baaad@codeweavers.com> <acc40eeb-efb0-1a64-a891-3edab6a4ad8b@codeweavers.com>



On 6/21/22 12:24, Ziqing Hui wrote:
>
> On 6/21/22 4:35 PM, Nikolay Sivov wrote:
>>
>> On 6/21/22 06:17, Ziqing Hui wrote:
>>> @@ -616,7 +651,9 @@ struct d2d_effect
>>>        ID2D1Image ID2D1Image_iface;
>>>        LONG refcount;
>>>    -    const struct d2d_effect_info *info;
>>> +    CLSID id;
>>> +    UINT32 min_inputs;
>>> +    UINT32 max_inputs;
>>>          struct d2d_effect_context *effect_context;
>>>        ID2D1Image **inputs;
>> This should be a part of property system, not exposed like that.
>>
>>> @@ -554,21 +563,21 @@ static HRESULT STDMETHODCALLTYPE d2d_effect_GetValue(ID2D1Effect *iface, UINT32
>>>        {
>>>            case D2D1_PROPERTY_CLSID:
>>>                if ((type != D2D1_PROPERTY_TYPE_UNKNOWN && type != D2D1_PROPERTY_TYPE_CLSID)
>>> -                    || value_size != sizeof(*effect->info->clsid))
>>> +                    || value_size != sizeof(effect->id))
>>>                    return E_INVALIDARG;
>>> -            src = effect->info->clsid;
>>> +            src = &effect->id;
>>>                break;
>>>            case D2D1_PROPERTY_MIN_INPUTS:
>>>                if ((type != D2D1_PROPERTY_TYPE_UNKNOWN && type != D2D1_PROPERTY_TYPE_UINT32)
>>> -                    || value_size != sizeof(effect->info->min_inputs))
>>> +                    || value_size != sizeof(effect->min_inputs))
>>>                    return E_INVALIDARG;
>>> -            src = &effect->info->min_inputs;
>>> +            src = &effect->min_inputs;
>>>                break;
>>>            case D2D1_PROPERTY_MAX_INPUTS:
>>>                if ((type != D2D1_PROPERTY_TYPE_UNKNOWN && type != D2D1_PROPERTY_TYPE_UINT32)
>>> -                    || value_size != sizeof(effect->info->max_inputs))
>>> +                    || value_size != sizeof(effect->max_inputs))
>>>                    return E_INVALIDARG;
>>> -            src = &effect->info->max_inputs;
>>> +            src = &effect->max_inputs;
>>>                break;
>>>            default:
>>>                if (index < D2D1_PROPERTY_CLSID)
>> Similarly, this needs rework.
>>
> OK, I'll rework the property system first.
>
>
>>> -static const struct d2d_effect_info builtin_effects[] =
>>> +struct d2d_builtin_effect_registration
>>>    {
>>> -    {&CLSID_D2D12DAffineTransform,      1, 1, 1},
>>> -    {&CLSID_D2D13DPerspectiveTransform, 1, 1, 1},
>>> -    {&CLSID_D2D1Composite,              2, 1, 0xffffffff},
>>> -    {&CLSID_D2D1Crop,                   1, 1, 1},
>>> -    {&CLSID_D2D1Shadow,                 1, 1, 1},
>>> -    {&CLSID_D2D1Grayscale,              1, 1, 1},
>>> +    const CLSID *id;
>>> +    PD2D1_EFFECT_FACTORY factory;
>>> +    UINT32 default_input_count;
>>> +    UINT32 min_inputs;
>>> +    UINT32 max_inputs;
>>> +};
>> It should be possible to reuse same structure for builtin effects.
>>
> Does it means that we should reuse d2d_effect_registration for the builtin effect data here? Or we keep d2d_effect_info and ignore factory field in this patch?

I think single structure is better, but there are options of course. You 
could have some lighter array of essential configuration for builtin 
effects, and later turn that into _effect_registration. But later that 
will need to have properties for builtin effects too, so it won't stay 
that light.

>
> If use d2d_effect_registration, one problem is that CLSID is stored by itself in d2d_effect_registration, not a const pointer. So we are not able to set it statically.
> Then we can have something like:
>
> struct d2d_builtin_effects
> {
>      const CLSID *id;
>      struct d2d_effect_registration reg;
> }
> builtin_effects[] = {....};
>
> Does it work?

Or you can initialize it once dynamically, keeping same structure.

>
>
>>> +HRESULT d2d_register_builtin_effects(struct d2d_factory *factory)
>>> +{
>>> +    struct d2d_effect_registration *reg;
>>> +    unsigned int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(builtin_effects); ++i)
>>> +    {
>>> +        const struct d2d_builtin_effect_registration *builtin_reg = &builtin_effects[i];
>>> +
>>> +        if (!(reg = calloc(1, sizeof(*reg))))
>>> +            return E_OUTOFMEMORY;
>>> +
>>> +        reg->is_builtin = TRUE;
>>> +        reg->factory = builtin_reg->factory;
>>> +        reg->registration_count = 1;
>>> +        reg->id = *builtin_reg->id;
>>> +        reg->default_input_count = builtin_reg->default_input_count;
>>> +        reg->min_inputs = builtin_reg->min_inputs;
>>> +        reg->max_inputs = builtin_reg->max_inputs;
>>> +        list_add_tail(&factory->effects, &reg->entry);
>>> +    }
>>> +
>>> +    return S_OK;
>>> +}
>> I'm not sure if we want that. It should be enough to check if CLSID is for builtin in Register* call, and redirect to builtin data in CreateEffect().
>>
> It means that, we don't register builtin effect to factory's registered effects list, and access builtin data directly in CreateEffect()?
>
>
Yes, the only value of getting it registered is for CreateEffect() to 
pick them up. But, first you can't unregister builtin ones, and can't 
register them manually, it's an error, not a refcount increase like for 
custom effects.

So you'll need some helper to get builtin effect config structure for 
CreateEffect(), and you can use same helper for RegisterEffect, to 
return error.