Signed-off-by: Ziqing Hui zhui@codeweavers.com --- dlls/d2d1/tests/d2d1.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c index bc662b4f8ae..3e385b2b05f 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -170,6 +170,29 @@ L"<?xml version='1.0'?> \ </Effect> \ ";
+static const WCHAR *effect_xml_wrong_type = +L"<?xml version='1.0'?> \ + <Effect> \ + <Property name='DisplayName' type='string' value='TestEffect'/> \ + <Property name='Author' type='uint32' value='32'/> \ + <Property name='Category' type='string' value='Test'/> \ + <Property name='Description' type='string' value='Test effect.'/> \ + <Inputs/> \ + </Effect> \ +"; + +static const WCHAR *effect_xml_duplicate_property = +L"<?xml version='1.0'?> \ + <Effect> \ + <Property name='DisplayName' type='string'/> \ + <Property name='Author' type='string'/> \ + <Property name='Category' type='string'/> \ + <Property name='Description' type='string'/> \ + <Property name='Description' type='string'/> \ + <Inputs/> \ + </Effect> \ +"; + static const DWORD test_vs[] = { #if 0 @@ -10615,6 +10638,8 @@ static void test_effect_register(BOOL d3d11) {effect_xml_without_category, E_INVALIDARG}, {effect_xml_without_description, E_INVALIDARG}, {effect_xml_without_type, E_INVALIDARG}, + {effect_xml_wrong_type, E_INVALIDARG}, + {effect_xml_duplicate_property, E_INVALIDARG}, };
const D2D1_PROPERTY_BINDING binding[] =
Signed-off-by: Ziqing Hui zhui@codeweavers.com ---
List is used here rather than array. Because we will remove node in UnregisterEffect().
dlls/d2d1/Makefile.in | 2 +- dlls/d2d1/d2d1_private.h | 16 +++++ dlls/d2d1/factory.c | 134 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 149 insertions(+), 3 deletions(-)
diff --git a/dlls/d2d1/Makefile.in b/dlls/d2d1/Makefile.in index 413571338ba..2456c63c336 100644 --- a/dlls/d2d1/Makefile.in +++ b/dlls/d2d1/Makefile.in @@ -1,6 +1,6 @@ MODULE = d2d1.dll IMPORTLIB = d2d1 -IMPORTS = d3d10_1 dxguid uuid gdi32 user32 advapi32 +IMPORTS = d3d10_1 dxguid uuid gdi32 user32 advapi32 ole32 xmllite DELAYIMPORTS = dwrite
C_SRCS = \ diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h index efc9247a822..cc625ff5719 100644 --- a/dlls/d2d1/d2d1_private.h +++ b/dlls/d2d1/d2d1_private.h @@ -768,4 +768,20 @@ static inline const char *debug_d2d_ellipse(const D2D1_ELLIPSE *ellipse) ellipse->point.x, ellipse->point.y, ellipse->radiusX, ellipse->radiusY); }
+static inline WCHAR *heap_strdupW(const WCHAR *str) +{ + WCHAR *ret = NULL; + size_t size; + + if(!str) + return ret; + + size = (wcslen(str) + 1) * sizeof(*str); + if(!(ret = heap_alloc(size))) + return ret; + memcpy(ret, str, size); + + return ret; +} + #endif /* __WINE_D2D1_PRIVATE_H */ diff --git a/dlls/d2d1/factory.c b/dlls/d2d1/factory.c index 01dac8051c0..e8aa1fab012 100644 --- a/dlls/d2d1/factory.c +++ b/dlls/d2d1/factory.c @@ -18,6 +18,8 @@
#define D2D1_INIT_GUID #include "d2d1_private.h" +#include "xmllite.h" +#include "wine/list.h"
WINE_DECLARE_DEBUG_CHANNEL(winediag); WINE_DEFAULT_DEBUG_CHANNEL(d2d); @@ -27,6 +29,16 @@ struct d2d_settings d2d_settings = ~0u, /* No ID2D1Factory version limit by default. */ };
+struct d2d_effect_reg +{ + PD2D1_EFFECT_FACTORY factory; + UINT32 count; + + struct d2d_effect_info *info; + + struct list entry; +}; + struct d2d_factory { ID2D1Factory3 ID2D1Factory3_iface; @@ -39,6 +51,8 @@ struct d2d_factory float dpi_y;
CRITICAL_SECTION cs; + + struct list effect_regs; };
static inline struct d2d_factory *impl_from_ID2D1Factory3(ID2D1Factory3 *iface) @@ -51,6 +65,15 @@ static inline struct d2d_factory *impl_from_ID2D1Multithread(ID2D1Multithread *i return CONTAINING_RECORD(iface, struct d2d_factory, ID2D1Multithread_iface); }
+static void d2d_effect_reg_cleanup(struct d2d_effect_reg *reg) +{ + if (!reg) + return; + + heap_free(reg->info); + heap_free(reg); +} + static HRESULT d2d_factory_reload_sysmetrics(struct d2d_factory *factory) { HDC hdc; @@ -112,6 +135,7 @@ static ULONG STDMETHODCALLTYPE d2d_factory_Release(ID2D1Factory3 *iface) { struct d2d_factory *factory = impl_from_ID2D1Factory3(iface); ULONG refcount = InterlockedDecrement(&factory->refcount); + struct d2d_effect_reg *iter, *iter2;
TRACE("%p decreasing refcount to %lu.\n", iface, refcount);
@@ -120,6 +144,8 @@ static ULONG STDMETHODCALLTYPE d2d_factory_Release(ID2D1Factory3 *iface) if (factory->device) ID3D10Device1_Release(factory->device); DeleteCriticalSection(&factory->cs); + LIST_FOR_EACH_ENTRY_SAFE(iter, iter2, &factory->effect_regs, struct d2d_effect_reg, entry) + d2d_effect_reg_cleanup(iter); heap_free(factory); }
@@ -564,14 +590,117 @@ static HRESULT STDMETHODCALLTYPE d2d_factory_CreateGdiMetafile(ID2D1Factory3 *if return E_NOTIMPL; }
+static HRESULT parse_effect_xml(IStream *property_xml, struct d2d_effect_reg *reg) +{ + unsigned int i, len, depth, input_count = 0; + IXmlReader *xml_reader; + WCHAR *node_name[3]; + const WCHAR *value; + XmlNodeType type; + HRESULT hr; + + if (FAILED(hr = CreateXmlReader(&IID_IXmlReader, (void **)&xml_reader, NULL))) + return hr; + if (FAILED(hr = IXmlReader_SetInput(xml_reader, (IUnknown *)property_xml))) + goto done; + + memset(node_name, 0, sizeof(node_name)); + while (IXmlReader_Read(xml_reader, &type) == S_OK) + { + if (FAILED(hr = IXmlReader_GetDepth(xml_reader, &depth))) + goto done; + if (depth >= 3) + continue; + if (FAILED(hr = IXmlReader_GetLocalName(xml_reader, &value, &len))) + goto done; + if (node_name[depth] && wcslen(node_name[depth]) <= len) + { + wcscpy(node_name[depth], value); + } + else + { + heap_free(node_name[depth]); + node_name[depth] = heap_strdupW(value); + } + + if (type != XmlNodeType_Element) + continue; + + switch (depth) + { + case 1: + if (!wcscmp(node_name[depth - 1], L"Effect") + && !wcscmp(node_name[depth], L"Property")) + { + FIXME("Property is ignored.\n"); + } + break; + case 2: + if (!wcscmp(node_name[depth - 1], L"Inputs") + && !wcscmp(node_name[depth], L"Input")) + { + ++input_count; + } + else if (!wcscmp(node_name[depth - 1], L"Property") + && !wcscmp(node_name[depth], L"Property")) + { + FIXME("Sub property is ignored.\n"); + } + break; + default: + break; + } + } + + reg->info->default_input_count = input_count; + reg->info->min_inputs = input_count; + reg->info->max_inputs = input_count; + +done: + for (i = 0; i < ARRAY_SIZE(node_name); ++i) + heap_free(node_name[i]); + IXmlReader_Release(xml_reader); + return hr; +} + static HRESULT STDMETHODCALLTYPE d2d_factory_RegisterEffectFromStream(ID2D1Factory3 *iface, REFCLSID effect_id, IStream *property_xml, const D2D1_PROPERTY_BINDING *bindings, UINT32 binding_count, PD2D1_EFFECT_FACTORY effect_factory) { - FIXME("iface %p, effect_id %s, property_xml %p, bindings %p, binding_count %u, effect_factory %p stub!\n", + struct d2d_factory *factory = impl_from_ID2D1Factory3(iface); + struct d2d_effect_reg *iter, *entry = NULL; + HRESULT hr; + + TRACE("iface %p, effect_id %s, property_xml %p, bindings %p, binding_count %u, effect_factory %p.\n", iface, debugstr_guid(effect_id), property_xml, bindings, binding_count, effect_factory);
- return E_NOTIMPL; + LIST_FOR_EACH_ENTRY(iter, &factory->effect_regs, struct d2d_effect_reg, entry) + { + if (IsEqualGUID(effect_id, iter->info->clsid)) + { + ++iter->count; + return S_OK; + } + } + + if (!(entry = heap_alloc_zero(sizeof(*entry))) || !(entry->info = heap_alloc_zero(sizeof(*entry->info)))) + { + hr = E_OUTOFMEMORY; + goto done; + } + + if (FAILED(hr = parse_effect_xml(property_xml, entry))) + goto done; + + entry->count = 1; + entry->info->clsid = effect_id; + entry->factory = effect_factory; + list_add_tail(&factory->effect_regs, &entry->entry); + +done: + if (hr != S_OK) + d2d_effect_reg_cleanup(entry); + return hr; }
static HRESULT STDMETHODCALLTYPE d2d_factory_RegisterEffectFromString(ID2D1Factory3 *iface, @@ -743,6 +872,7 @@ static void d2d_factory_init(struct d2d_factory *factory, D2D1_FACTORY_TYPE fact factory->refcount = 1; d2d_factory_reload_sysmetrics(factory); InitializeCriticalSection(&factory->cs); + list_init(&factory->effect_regs); }
HRESULT WINAPI D2D1CreateFactory(D2D1_FACTORY_TYPE factory_type, REFIID iid,
On 6/6/22 10:35, Ziqing Hui wrote:
+struct d2d_effect_reg +{
- PD2D1_EFFECT_FACTORY factory;
- UINT32 count;
- struct d2d_effect_info *info;
- struct list entry;
+};
You don't need to allocate 'info' separately.
- memset(node_name, 0, sizeof(node_name));
- while (IXmlReader_Read(xml_reader, &type) == S_OK)
- {
if (FAILED(hr = IXmlReader_GetDepth(xml_reader, &depth)))goto done;if (depth >= 3)continue;if (FAILED(hr = IXmlReader_GetLocalName(xml_reader, &value, &len)))goto done;if (node_name[depth] && wcslen(node_name[depth]) <= len){wcscpy(node_name[depth], value);}else{heap_free(node_name[depth]);node_name[depth] = heap_strdupW(value);}if (type != XmlNodeType_Element)continue;
Depth should not be used for this. What you need is to first find "Effect" element. After that loop within it, until you run out of nested elements. You can then have helpers to process specific elements like Property or Inputs.
if (!wcscmp(node_name[depth - 1], L"Effect")&& !wcscmp(node_name[depth], L"Property"))
Is it really case-sensitive?
- reg->info->default_input_count = input_count;
- reg->info->min_inputs = input_count;
- reg->info->max_inputs = input_count;
Is this supposed to be subproperties of an Input? Maybe we should have basic property support first, and store it there. Is min/max/default expressible in effect xml? <Input> attributes maybe?
- entry->count = 1;
- entry->info->clsid = effect_id;
- entry->factory = effect_factory;
- list_add_tail(&factory->effect_regs, &entry->entry);
This should store GUID itself, not a pointer.
On 6/6/22 4:29 PM, Nikolay Sivov wrote:
On 6/6/22 10:35, Ziqing Hui wrote:
+struct d2d_effect_reg +{ + PD2D1_EFFECT_FACTORY factory; + UINT32 count;
+ struct d2d_effect_info *info;
+ struct list entry; +};
You don't need to allocate 'info' separately.
+ memset(node_name, 0, sizeof(node_name)); + while (IXmlReader_Read(xml_reader, &type) == S_OK) + { + if (FAILED(hr = IXmlReader_GetDepth(xml_reader, &depth))) + goto done; + if (depth >= 3) + continue; + if (FAILED(hr = IXmlReader_GetLocalName(xml_reader, &value, &len))) + goto done; + if (node_name[depth] && wcslen(node_name[depth]) <= len) + { + wcscpy(node_name[depth], value); + } + else + { + heap_free(node_name[depth]); + node_name[depth] = heap_strdupW(value); + }
+ if (type != XmlNodeType_Element) + continue;
Depth should not be used for this. What you need is to first find "Effect" element. After that loop within it, until you run out of nested elements. You can then have helpers to process specific elements like Property or Inputs.
+ if (!wcscmp(node_name[depth - 1], L"Effect") + && !wcscmp(node_name[depth], L"Property"))
Is it really case-sensitive?
I'm not sure. I'll add a test for it.
+ reg->info->default_input_count = input_count; + reg->info->min_inputs = input_count; + reg->info->max_inputs = input_count;
Is this supposed to be subproperties of an Input? Maybe we should have basic property support first, and store it there. Is min/max/default expressible in effect xml? <Input> attributes maybe?
It is likely that they can be expressed in xml as properties of <Effect> like "DisplayName"/"Author", tests are still needed. And according to the current test, if I don't explicitly express them in xml, they will be equal to the number of <Input> nodes inside <Inputs>.
+ entry->count = 1; + entry->info->clsid = effect_id; + entry->factory = effect_factory; + list_add_tail(&factory->effect_regs, &entry->entry);
This should store GUID itself, not a pointer.
+ reg->info->default_input_count = input_count; + reg->info->min_inputs = input_count; + reg->info->max_inputs = input_count;
Is this supposed to be subproperties of an Input? Maybe we should have basic property support first, and store it there. Is min/max/default expressible in effect xml? <Input> attributes maybe?
It is likely that they can be expressed in xml as properties of <Effect> like "DisplayName"/"Author", tests are still needed. And according to the current test, if I don't explicitly express them in xml, they will be equal to the number of <Input> nodes inside <Inputs>.
I did some tests today. I didn't find a way to express min/max/default input count in XML.
I tried these ways:
* Property of <Effect>:
<Effect> ... <Property name='MinInputs' type='uint32' value='1'/> </Effect>
* Sub property of <Inputs>:
<Effect> ... <Inputs> <Property name='Min' type='uint32' value='1'/> </Inputs> <Effect>
* Attribute of <Inputs>:
<Effect> ... <Inputs min='1'/> <Effect/>
Unfortunately, none of them work.
On 6/7/22 11:43 AM, Ziqing Hui wrote:
+ reg->info->default_input_count = input_count; + reg->info->min_inputs = input_count; + reg->info->max_inputs = input_count;
Is this supposed to be subproperties of an Input? Maybe we should have basic property support first, and store it there. Is min/max/default expressible in effect xml? <Input> attributes maybe?
It is likely that they can be expressed in xml as properties of <Effect> like "DisplayName"/"Author", tests are still needed. And according to the current test, if I don't explicitly express them in xml, they will be equal to the number of <Input> nodes inside <Inputs>.
I did some tests today. I didn't find a way to express min/max/default input count in XML.
I tried these ways:
Property of <Effect>:
<Effect> ... <Property name='MinInputs' type='uint32' value='1'/> </Effect>
Sub property of <Inputs>:
<Effect> ... <Inputs> <Property name='Min' type='uint32' value='1'/> </Inputs> <Effect>
Attribute of <Inputs>:
<Effect> ... <Inputs min='1'/> <Effect/>
Unfortunately, none of them work.
Some typo here in the mail... the second effect nodes are all </Effect>.
On 6/7/22 06:43, Ziqing Hui wrote:
+ reg->info->default_input_count = input_count; + reg->info->min_inputs = input_count; + reg->info->max_inputs = input_count;
Is this supposed to be subproperties of an Input? Maybe we should have basic property support first, and store it there. Is min/max/default expressible in effect xml? <Input> attributes maybe?
It is likely that they can be expressed in xml as properties of <Effect> like "DisplayName"/"Author", tests are still needed. And according to the current test, if I don't explicitly express them in xml, they will be equal to the number of <Input> nodes inside <Inputs>.
I did some tests today. I didn't find a way to express min/max/default input count in XML.
I tried these ways:
Property of <Effect>:
<Effect> ... <Property name='MinInputs' type='uint32' value='1'/> </Effect>
Sub property of <Inputs>:
<Effect> ... <Inputs> <Property name='Min' type='uint32' value='1'/> </Inputs> <Effect>
Attribute of <Inputs>:
<Effect> ... <Inputs min='1'/> <Effect/>
Unfortunately, none of them work.
I see. It's easier to assume it's consistent with returned name, so it should be using MinInputs name. If it doesn't work, let's ignore that for now.
Structurally, I was thinking that each <Property/> would trigger some add_property() helper that would simply append to some array. That could use some structure of { index, name, type, value }, for system properties name could be empty. It doesn't have to be exactly like that, but the point is that it should be easy to use when parsing description, and when instantiating effects objects. I haven't looked that closely, but I imaging a new instance will use initial values from 'value' attributes + set getter/setter functions, which means that instance properties are essentially cloned from the description.
Signed-off-by: Ziqing Hui zhui@codeweavers.com --- dlls/d2d1/d2d1_private.h | 12 +++ dlls/d2d1/factory.c | 155 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 164 insertions(+), 3 deletions(-)
diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h index cc625ff5719..07f07c3a087 100644 --- a/dlls/d2d1/d2d1_private.h +++ b/dlls/d2d1/d2d1_private.h @@ -603,12 +603,24 @@ struct d2d_effect_context void d2d_effect_context_init(struct d2d_effect_context *effect_context, struct d2d_device_context *device_context) DECLSPEC_HIDDEN;
+struct d2d_effect_property +{ + WCHAR *name; + D2D1_PROPERTY_TYPE type; + PD2D1_PROPERTY_SET_FUNCTION set_function; + PD2D1_PROPERTY_GET_FUNCTION get_function; +}; + struct d2d_effect_info { const CLSID *clsid; UINT32 default_input_count; UINT32 min_inputs; UINT32 max_inputs; + WCHAR *display_name; + WCHAR *author; + WCHAR *category; + WCHAR *description; };
struct d2d_effect diff --git a/dlls/d2d1/factory.c b/dlls/d2d1/factory.c index e8aa1fab012..b3b65b0cb92 100644 --- a/dlls/d2d1/factory.c +++ b/dlls/d2d1/factory.c @@ -36,6 +36,10 @@ struct d2d_effect_reg
struct d2d_effect_info *info;
+ struct d2d_effect_property *properties; + size_t property_size; + size_t property_count; + struct list entry; };
@@ -67,10 +71,24 @@ static inline struct d2d_factory *impl_from_ID2D1Multithread(ID2D1Multithread *i
static void d2d_effect_reg_cleanup(struct d2d_effect_reg *reg) { + size_t i; + if (!reg) return;
- heap_free(reg->info); + if (reg->info) + { + heap_free(reg->info->display_name); + heap_free(reg->info->author); + heap_free(reg->info->category); + heap_free(reg->info->description); + heap_free(reg->info); + } + + for (i = 0; i < reg->property_count; ++i) + heap_free(reg->properties[i].name); + heap_free(reg->properties); + heap_free(reg); }
@@ -590,6 +608,110 @@ static HRESULT STDMETHODCALLTYPE d2d_factory_CreateGdiMetafile(ID2D1Factory3 *if return E_NOTIMPL; }
+static HRESULT heap_get_attribute(IXmlReader *xml_reader, const WCHAR *name, WCHAR **ptr) +{ + const WCHAR *value; + + if (IXmlReader_MoveToAttributeByName(xml_reader, name, NULL) != S_OK) + return E_INVALIDARG; + if (IXmlReader_GetValue(xml_reader, &value, NULL) != S_OK) + return E_INVALIDARG; + if (!(*ptr = heap_strdupW(value))) + return E_OUTOFMEMORY; + return S_OK; +} + +static D2D1_PROPERTY_TYPE str_to_property_type(const WCHAR *str) +{ + unsigned int i; + + static const WCHAR *type_str[] = + { + L"", L"string", L"bool", L"uint32", L"int32", L"float", L"vector2", L"vector3", + L"vector4", L"blob", L"iunknown", L"enum", L"array", L"clsid", L"matrix3x2", + L"matrix4x3", L"matrix4x4", L"matrix5x4", L"colorcontext", + }; + + for (i = 1; i < ARRAY_SIZE(type_str); ++i) + { + if (!wcscmp(str, type_str[i])) + return i; + } + + return D2D1_PROPERTY_TYPE_UNKNOWN; +} + +static HRESULT parse_property(IXmlReader *xml_reader, struct d2d_effect_reg *reg) +{ + struct d2d_effect_property *entry; + D2D1_PROPERTY_TYPE property_type; + WCHAR **ptr = NULL, *name = NULL; + const WCHAR *type; + unsigned int i; + HRESULT hr; + + /* get property name and type */ + if (FAILED(hr = heap_get_attribute(xml_reader, L"name", &name))) + goto done; + if ((hr = IXmlReader_MoveToAttributeByName(xml_reader, L"type", NULL)) != S_OK) + goto done; + if ((hr = IXmlReader_GetValue(xml_reader, &type, NULL)) != S_OK) + goto done; + property_type = str_to_property_type(type); + if (property_type == D2D1_PROPERTY_TYPE_UNKNOWN) + { + hr = E_INVALIDARG; + goto done; + } + + /* is system property */ + if (!wcscmp(name, L"DisplayName")) + ptr = ®->info->display_name; + else if(!wcscmp(name, L"Author")) + ptr = ®->info->author; + else if(!wcscmp(name, L"Category")) + ptr = ®->info->category; + else if(!wcscmp(name, L"Description")) + ptr = ®->info->description; + if (ptr) + { + heap_free(name); + if (*ptr || property_type != D2D1_PROPERTY_TYPE_STRING) + return E_INVALIDARG; + if (FAILED(heap_get_attribute(xml_reader, L"value", ptr))) + { + if (!(*ptr = heap_strdupW(L""))) + hr = E_OUTOFMEMORY; + } + return hr; + } + + /* is custom property */ + for (i = 0; i < reg->property_count; ++i) + { + if (!wcscmp(name, reg->properties[i].name)) + { + hr = E_INVALIDARG; + goto done; + } + } + if (!d2d_array_reserve((void **)®->properties, ®->property_size, reg->property_count + 1, sizeof(*reg->properties))) + { + ERR("Failed to resize properties array.\n"); + goto done; + } + entry = ®->properties[reg->property_count++]; + entry->name = name; + entry->type = property_type; + entry->get_function = NULL; + entry->set_function = NULL; + +done: + if (hr != S_OK) + heap_free(name); + return hr; +} + static HRESULT parse_effect_xml(IStream *property_xml, struct d2d_effect_reg *reg) { unsigned int i, len, depth, input_count = 0; @@ -632,7 +754,8 @@ static HRESULT parse_effect_xml(IStream *property_xml, struct d2d_effect_reg *re if (!wcscmp(node_name[depth - 1], L"Effect") && !wcscmp(node_name[depth], L"Property")) { - FIXME("Property is ignored.\n"); + if (FAILED(hr = parse_property(xml_reader, reg))) + goto done; } break; case 2: @@ -644,7 +767,7 @@ static HRESULT parse_effect_xml(IStream *property_xml, struct d2d_effect_reg *re else if (!wcscmp(node_name[depth - 1], L"Property") && !wcscmp(node_name[depth], L"Property")) { - FIXME("Sub property is ignored.\n"); + FIXME("SubProperty is ignored.\n"); } break; default: @@ -652,6 +775,12 @@ static HRESULT parse_effect_xml(IStream *property_xml, struct d2d_effect_reg *re } }
+ if (!reg->info->display_name || !reg->info->author || !reg->info->category || !reg->info->description) + { + hr = E_INVALIDARG; + goto done; + } + reg->info->default_input_count = input_count; reg->info->min_inputs = input_count; reg->info->max_inputs = input_count; @@ -669,6 +798,7 @@ static HRESULT STDMETHODCALLTYPE d2d_factory_RegisterEffectFromStream(ID2D1Facto { struct d2d_factory *factory = impl_from_ID2D1Factory3(iface); struct d2d_effect_reg *iter, *entry = NULL; + unsigned int i, j; HRESULT hr;
TRACE("iface %p, effect_id %s, property_xml %p, bindings %p, binding_count %u, effect_factory %p.\n", @@ -692,6 +822,25 @@ static HRESULT STDMETHODCALLTYPE d2d_factory_RegisterEffectFromStream(ID2D1Facto if (FAILED(hr = parse_effect_xml(property_xml, entry))) goto done;
+ /* bind getter and setter to properties */ + for (i = 0; i < binding_count; ++i) + { + for (j = 0; j < entry->property_count; ++j) + { + if (!wcscmp(bindings[i].propertyName, entry->properties[j].name)) + { + entry->properties[j].get_function = bindings[i].getFunction; + entry->properties[j].set_function = bindings[i].setFunction; + break; + } + } + if (j > entry->property_count) + { + hr = D2DERR_INVALID_PROPERTY; + goto done; + } + } + entry->count = 1; entry->info->clsid = effect_id; entry->factory = effect_factory;
On 6/6/22 10:35, Ziqing Hui wrote:
struct d2d_effect_info { const CLSID *clsid; UINT32 default_input_count; UINT32 min_inputs; UINT32 max_inputs;
- WCHAR *display_name;
- WCHAR *author;
- WCHAR *category;
- WCHAR *description; };
Shouldn't these also be expressed as properties?
+static D2D1_PROPERTY_TYPE str_to_property_type(const WCHAR *str) +{
- unsigned int i;
- static const WCHAR *type_str[] =
- {
L"", L"string", L"bool", L"uint32", L"int32", L"float", L"vector2", L"vector3",L"vector4", L"blob", L"iunknown", L"enum", L"array", L"clsid", L"matrix3x2",L"matrix4x3", L"matrix4x4", L"matrix5x4", L"colorcontext",- };
- for (i = 1; i < ARRAY_SIZE(type_str); ++i)
- {
if (!wcscmp(str, type_str[i]))return i;- }
- return D2D1_PROPERTY_TYPE_UNKNOWN;
+}
This uses numeric values of D2D1_PROPERTY_TYPE, that deserves at least a comment.
On 6/6/22 4:36 PM, Nikolay Sivov wrote:
On 6/6/22 10:35, Ziqing Hui wrote:
struct d2d_effect_info { const CLSID *clsid; UINT32 default_input_count; UINT32 min_inputs; UINT32 max_inputs; + WCHAR *display_name; + WCHAR *author; + WCHAR *category; + WCHAR *description; };
Shouldn't these also be expressed as properties?
Yeah, these are properties in XML.
However, there are two kinds of effect properties, one is system property, the other is custom property.
System property always exists in every effect, not matter if we express them in effect XML or not. So my thought is, since they always exists, we don't need to dynamically store them in properties array. We can use one struct to store all system properties. We store custom properties as d2d_effect_property, while handle system properties separately.
Currently, we are storing system properties in struct d2d_effect_info. Each field in info is a system property. So I just put them there like before.
Also, system properties don't have custom setter and getter, while custom properties have.
Do you think my thought is ok or maybe we should take a different way?
+static D2D1_PROPERTY_TYPE str_to_property_type(const WCHAR *str) +{ + unsigned int i;
+ static const WCHAR *type_str[] = + { + L"", L"string", L"bool", L"uint32", L"int32", L"float", L"vector2", L"vector3", + L"vector4", L"blob", L"iunknown", L"enum", L"array", L"clsid", L"matrix3x2", + L"matrix4x3", L"matrix4x4", L"matrix5x4", L"colorcontext", + };
+ for (i = 1; i < ARRAY_SIZE(type_str); ++i) + { + if (!wcscmp(str, type_str[i])) + return i; + }
+ return D2D1_PROPERTY_TYPE_UNKNOWN; +}
This uses numeric values of D2D1_PROPERTY_TYPE, that deserves at least a comment.>
Would it be ok like this: ?
static const WCHAR *type_str[] = { L"", /* D2D1_PROPERTY_TYPE_UNKNOWN */ L"string", /* D2D1_PROPERTY_TYPE_STRING */ L"bool", /* D2D1_PROPERTY_TYPE_BOOL */ L"uint32", /* D2D1_PROPERTY_TYPE_UINT32 */ ... };
On 6/6/22 13:13, Ziqing Hui wrote:
On 6/6/22 4:36 PM, Nikolay Sivov wrote:
On 6/6/22 10:35, Ziqing Hui wrote:
struct d2d_effect_info { const CLSID *clsid; UINT32 default_input_count; UINT32 min_inputs; UINT32 max_inputs; + WCHAR *display_name; + WCHAR *author; + WCHAR *category; + WCHAR *description; };
Shouldn't these also be expressed as properties?
Yeah, these are properties in XML.
However, there are two kinds of effect properties, one is system property, the other is custom property.
System property always exists in every effect, not matter if we express them in effect XML or not. So my thought is, since they always exists, we don't need to dynamically store them in properties array. We can use one struct to store all system properties. We store custom properties as d2d_effect_property, while handle system properties separately.
Currently, we are storing system properties in struct d2d_effect_info. Each field in info is a system property. So I just put them there like before.
Also, system properties don't have custom setter and getter, while custom properties have.
Do you think my thought is ok or maybe we should take a different way?
Hard to tell before it's done. Even we don't have to store them along with custom properties, it would be better to have some organized way for them, and not separate fields. I guess for now we could even ignore them to some extent, the only important ones are input/output configuration. Effect parameters will be all custom properties, right?
+static D2D1_PROPERTY_TYPE str_to_property_type(const WCHAR *str) +{ + unsigned int i;
+ static const WCHAR *type_str[] = + { + L"", L"string", L"bool", L"uint32", L"int32", L"float", L"vector2", L"vector3", + L"vector4", L"blob", L"iunknown", L"enum", L"array", L"clsid", L"matrix3x2", + L"matrix4x3", L"matrix4x4", L"matrix5x4", L"colorcontext", + };
+ for (i = 1; i < ARRAY_SIZE(type_str); ++i) + { + if (!wcscmp(str, type_str[i])) + return i; + }
+ return D2D1_PROPERTY_TYPE_UNKNOWN; +}
This uses numeric values of D2D1_PROPERTY_TYPE, that deserves at least a comment.>
Would it be ok like this: ?
static const WCHAR *type_str[] = { L"", /* D2D1_PROPERTY_TYPE_UNKNOWN */ L"string", /* D2D1_PROPERTY_TYPE_STRING */ L"bool", /* D2D1_PROPERTY_TYPE_BOOL */ L"uint32", /* D2D1_PROPERTY_TYPE_UINT32 */ ... };
Yes.
On 6/6/22 7:00 PM, Nikolay Sivov wrote:
On 6/6/22 13:13, Ziqing Hui wrote:
On 6/6/22 4:36 PM, Nikolay Sivov wrote:
On 6/6/22 10:35, Ziqing Hui wrote:
struct d2d_effect_info { const CLSID *clsid; UINT32 default_input_count; UINT32 min_inputs; UINT32 max_inputs; + WCHAR *display_name; + WCHAR *author; + WCHAR *category; + WCHAR *description; };
Shouldn't these also be expressed as properties?
Yeah, these are properties in XML.
However, there are two kinds of effect properties, one is system property, the other is custom property.
System property always exists in every effect, not matter if we express them in effect XML or not. So my thought is, since they always exists, we don't need to dynamically store them in properties array. We can use one struct to store all system properties. We store custom properties as d2d_effect_property, while handle system properties separately.
Currently, we are storing system properties in struct d2d_effect_info. Each field in info is a system property. So I just put them there like before.
Also, system properties don't have custom setter and getter, while custom properties have.
Do you think my thought is ok or maybe we should take a different way?
Hard to tell before it's done. Even we don't have to store them along with custom properties, it would be better to have some organized way for them, and not separate fields. I guess for now we could even ignore them to some extent, the only important ones are input/output configuration. Effect parameters will be all custom properties, right?
Yeah, effect parameters are all custom properties.
+static D2D1_PROPERTY_TYPE str_to_property_type(const WCHAR *str) +{ + unsigned int i;
+ static const WCHAR *type_str[] = + { + L"", L"string", L"bool", L"uint32", L"int32", L"float", L"vector2", L"vector3", + L"vector4", L"blob", L"iunknown", L"enum", L"array", L"clsid", L"matrix3x2", + L"matrix4x3", L"matrix4x4", L"matrix5x4", L"colorcontext", + };
+ for (i = 1; i < ARRAY_SIZE(type_str); ++i) + { + if (!wcscmp(str, type_str[i])) + return i; + }
+ return D2D1_PROPERTY_TYPE_UNKNOWN; +}
This uses numeric values of D2D1_PROPERTY_TYPE, that deserves at least a comment.>
Would it be ok like this: ?
static const WCHAR *type_str[] = { L"", /* D2D1_PROPERTY_TYPE_UNKNOWN */ L"string", /* D2D1_PROPERTY_TYPE_STRING */ L"bool", /* D2D1_PROPERTY_TYPE_BOOL */ L"uint32", /* D2D1_PROPERTY_TYPE_UINT32 */ ... };
Yes.
Signed-off-by: Ziqing Hui zhui@codeweavers.com --- dlls/d2d1/factory.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/dlls/d2d1/factory.c b/dlls/d2d1/factory.c index b3b65b0cb92..1d7c8325077 100644 --- a/dlls/d2d1/factory.c +++ b/dlls/d2d1/factory.c @@ -856,10 +856,28 @@ static HRESULT STDMETHODCALLTYPE d2d_factory_RegisterEffectFromString(ID2D1Facto REFCLSID effect_id, const WCHAR *property_xml, const D2D1_PROPERTY_BINDING *bindings, UINT32 binding_count, PD2D1_EFFECT_FACTORY effect_factory) { - FIXME("iface %p, effect_id %s, property_xml %s, bindings %p, binding_count %u, effect_factory %p stub!\n", - iface, debugstr_guid(effect_id), debugstr_w(property_xml), bindings, binding_count, effect_factory); + HGLOBAL hglobal; + IStream *stream; + size_t size; + BYTE *data; + HRESULT hr;
- return S_OK; + TRACE("iface %p, effect_id %s, property_xml %s, bindings %p, binding_count %u, effect_factory %p.\n", + iface, debugstr_guid(effect_id), debugstr_w(property_xml), bindings, binding_count, effect_factory); + + size = sizeof(*property_xml) * (wcslen(property_xml) + 1); + hglobal = GlobalAlloc(0, size); + data = GlobalLock(hglobal); + memcpy(data, property_xml, size); + GlobalUnlock(hglobal); + + if (FAILED(hr = CreateStreamOnHGlobal(hglobal, TRUE, &stream))) + return hr; + + hr = ID2D1Factory3_RegisterEffectFromStream(iface, effect_id, stream, bindings, binding_count, effect_factory); + + IStream_Release(stream); + return hr; }
static HRESULT STDMETHODCALLTYPE d2d_factory_UnregisterEffect(ID2D1Factory3 *iface, REFCLSID effect_id)
On 6/6/22 10:35, Ziqing Hui wrote:
- size = sizeof(*property_xml) * (wcslen(property_xml) + 1);
- hglobal = GlobalAlloc(0, size);
- data = GlobalLock(hglobal);
- memcpy(data, property_xml, size);
- GlobalUnlock(hglobal);
- if (FAILED(hr = CreateStreamOnHGlobal(hglobal, TRUE, &stream)))
return hr;
Seems easier to write whole string in a stream, and seek back.
On 6/6/22 4:37 PM, Nikolay Sivov wrote:
On 6/6/22 10:35, Ziqing Hui wrote:
+ size = sizeof(*property_xml) * (wcslen(property_xml) + 1); + hglobal = GlobalAlloc(0, size); + data = GlobalLock(hglobal); + memcpy(data, property_xml, size); + GlobalUnlock(hglobal);
+ if (FAILED(hr = CreateStreamOnHGlobal(hglobal, TRUE, &stream))) + return hr;
Seems easier to write whole string in a stream, and seek back.
I'm not familiar with IStream interface... I'm not pretty sure how to do this. Do I still use CreateStreamOnHGlobal() to create a stream? Do you mean I should create a stream first. And then use IStream_Write() to write the string to it, then seek back?
On 6/6/22 13:31, Ziqing Hui wrote:
On 6/6/22 4:37 PM, Nikolay Sivov wrote:
On 6/6/22 10:35, Ziqing Hui wrote:
+ size = sizeof(*property_xml) * (wcslen(property_xml) + 1); + hglobal = GlobalAlloc(0, size); + data = GlobalLock(hglobal); + memcpy(data, property_xml, size); + GlobalUnlock(hglobal);
+ if (FAILED(hr = CreateStreamOnHGlobal(hglobal, TRUE, &stream))) + return hr;
Seems easier to write whole string in a stream, and seek back.
I'm not familiar with IStream interface... I'm not pretty sure how to do this. Do I still use CreateStreamOnHGlobal() to create a stream? Do you mean I should create a stream first. And then use IStream_Write() to write the string to it, then seek back?
Yes, create it with a null memory handle, Write, and Seek() back to the beginning. Another option is to have some wrapper that implements Read() and save on additional allocations, but it's more code that we don't need.
Signed-off-by: Ziqing Hui zhui@codeweavers.com --- dlls/d2d1/factory.c | 21 +++++++++++++++++++-- dlls/d2d1/tests/d2d1.c | 28 ++++++++++++++-------------- 2 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/dlls/d2d1/factory.c b/dlls/d2d1/factory.c index 1d7c8325077..aac00a50a2a 100644 --- a/dlls/d2d1/factory.c +++ b/dlls/d2d1/factory.c @@ -882,9 +882,26 @@ static HRESULT STDMETHODCALLTYPE d2d_factory_RegisterEffectFromString(ID2D1Facto
static HRESULT STDMETHODCALLTYPE d2d_factory_UnregisterEffect(ID2D1Factory3 *iface, REFCLSID effect_id) { - FIXME("iface %p, effect_id %s stub!\n", iface, debugstr_guid(effect_id)); + struct d2d_factory *factory = impl_from_ID2D1Factory3(iface); + struct d2d_effect_reg *iter, *iter2;
- return E_NOTIMPL; + TRACE("iface %p, effect_id %s.\n", iface, debugstr_guid(effect_id)); + + LIST_FOR_EACH_ENTRY_SAFE(iter, iter2, &factory->effect_regs, struct d2d_effect_reg, entry) + { + if (IsEqualGUID(effect_id, iter->info->clsid)) + { + --iter->count; + if (!iter->count) + { + list_remove(&iter->entry); + d2d_effect_reg_cleanup(iter); + } + return S_OK; + } + } + + return D2DERR_EFFECT_IS_NOT_REGISTERED; }
static HRESULT STDMETHODCALLTYPE d2d_factory_GetRegisteredEffects(ID2D1Factory3 *iface, diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c index 3e385b2b05f..15b106288a8 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -10694,7 +10694,8 @@ static void test_effect_register(BOOL d3d11) winetest_push_context("Test %u", i);
hr = ID2D1Factory1_RegisterEffectFromString(factory, &CLSID_TestEffect, test->xml, NULL, 0, effect_impl_create); - todo_wine_if(test->hr != S_OK) + todo_wine_if(test->xml == effect_xml_without_version + || test->xml == effect_xml_without_inputs) ok(hr == test->hr, "Got unexpected hr %#lx, expected %#lx.\n", hr, test->hr); if (hr == S_OK) { @@ -10702,9 +10703,9 @@ static void test_effect_register(BOOL d3d11) hr = ID2D1DeviceContext_CreateEffect(device_context, &CLSID_TestEffect, &effect); todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == D2DERR_EFFECT_IS_NOT_REGISTERED, "Got unexpected hr %#lx.\n", hr); + ok(hr == D2DERR_EFFECT_IS_NOT_REGISTERED, "Got unexpected hr %#lx.\n", hr); if (effect) ID2D1Effect_Release(effect); } @@ -10727,11 +10728,11 @@ static void test_effect_register(BOOL d3d11) hr = ID2D1Factory1_RegisterEffectFromString(factory, &CLSID_TestEffect, test->xml, NULL, 0, effect_impl_create); ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == D2DERR_EFFECT_IS_NOT_REGISTERED, "Got unexpected hr %#lx.\n", hr); + ok(hr == D2DERR_EFFECT_IS_NOT_REGISTERED, "Got unexpected hr %#lx.\n", hr);
winetest_pop_context(); } @@ -10766,9 +10767,9 @@ static void test_effect_register(BOOL d3d11) }
hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
/* Register effect with property binding */ for (i = 0; i < ARRAY_SIZE(binding_tests); ++i) @@ -10827,13 +10828,13 @@ static void test_effect_register(BOOL d3d11) }
hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
/* Unregister builtin effect */ hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_D2D1Composite); - todo_wine ok(hr == D2DERR_EFFECT_IS_NOT_REGISTERED, "Got unexpected hr %#lx.\n", hr); + ok(hr == D2DERR_EFFECT_IS_NOT_REGISTERED, "Got unexpected hr %#lx.\n", hr);
release_test_context(&ctx); } @@ -10911,7 +10912,6 @@ done: if (effect) ID2D1Effect_Release(effect); hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); release_test_context(&ctx); } @@ -11068,7 +11068,7 @@ static void test_effect_properties(BOOL d3d11) if (effect) ID2D1Effect_Release(effect); hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); winetest_pop_context(); }
@@ -11161,7 +11161,7 @@ done: if (effect) ID2D1Effect_Release(effect); hr = ID2D1Factory1_UnregisterEffect(factory, &CLSID_TestEffect); - todo_wine ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr); + ok(hr == S_OK, "Got unexpected hr %#lx.\n", hr);
release_test_context(&ctx); }
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=116285
Your paranoid android.
=== debian11 (64 bit WoW report) ===
d2d1: d2d1: Timeout
-
Marvin -
Nikolay Sivov -
Ziqing Hui