From: Nikolay Sivov Subject: Re: [PATCH 2/3] d2d1: Implement effect creation for registered effect. Message-Id: Date: Tue, 21 Jun 2022 11:45:54 +0300 In-Reply-To: <20220621031708.2241872-2-zhui@codeweavers.com> References: <20220621031708.2241872-1-zhui@codeweavers.com> <20220621031708.2241872-2-zhui@codeweavers.com> 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, ®->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. > 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.

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.

 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.