Re: [PATCH 1/3] d2d1: Support registering builtin effect.

[Ziqing Hui]

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?

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?
> > +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()?