Signed-off-by: Ziqing Hui zhui@codeweavers.com ---
v2: Add case tests and min/max inputs tests.
dlls/d2d1/tests/d2d1.c | 120 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 112 insertions(+), 8 deletions(-)
diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c index bc662b4f8ae..970706bc569 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -99,6 +99,17 @@ L"<?xml version='1.0'?> \ </Effect> \ ";
+static const WCHAR *effect_xml_inputs_attribute = +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'/> \ + <Inputs min='1' max='10' deadbeef='abcdef'/> \ + </Effect> \ +"; + static const WCHAR *effect_xml_without_version = L"<Effect> \ <Property name='DisplayName' type='string'/> \ @@ -170,6 +181,90 @@ 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 WCHAR *effect_xml_lower_case_effect = +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'/> \ + <Inputs/> \ + </effect> \ +"; + +static const WCHAR *effect_xml_lower_case_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'/> \ + <Inputs/> \ + </Effect> \ +"; + +static const WCHAR *effect_xml_lower_case_inputs = +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'/> \ + <inputs/> \ + </Effect> \ +"; + +static const WCHAR *effect_xml_min_max_inputs_a = +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='MinInputs' type='uint32' value='1'/> \ + <Property name='MaxInputs' type='uint32' value='2'/> \ + <Inputs/> \ + </Effect> \ +"; + +static const WCHAR *effect_xml_min_max_inputs_b = +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'/> \ + <Inputs> \ + <Property name='Min' type='uint32' value='1'/> \ + <Property name='Max' type='uint32' value='2'/> \ + </Inputs> \ + </Effect> \ +"; + + static const DWORD test_vs[] = { #if 0 @@ -10604,10 +10699,11 @@ static void test_effect_register(BOOL d3d11) } xml_tests[] = { - {effect_xml_a, S_OK}, - {effect_xml_b, S_OK}, - {effect_xml_c, S_OK}, - {effect_xml_minimum, S_OK}, + {effect_xml_a, S_OK}, + {effect_xml_b, S_OK}, + {effect_xml_c, S_OK}, + {effect_xml_minimum, S_OK}, + {effect_xml_inputs_attribute, S_OK}, {effect_xml_without_version, HRESULT_FROM_WIN32(ERROR_NOT_FOUND)}, {effect_xml_without_inputs, E_INVALIDARG}, {effect_xml_without_name, E_INVALIDARG}, @@ -10615,6 +10711,13 @@ 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}, + {effect_xml_lower_case_effect, HRESULT_FROM_WIN32(ERROR_NOT_FOUND)}, + {effect_xml_lower_case_property, HRESULT_FROM_WIN32(ERROR_NOT_FOUND)}, + {effect_xml_lower_case_inputs, HRESULT_FROM_WIN32(ERROR_NOT_FOUND)}, + {effect_xml_min_max_inputs_a, E_INVALIDARG}, + {effect_xml_min_max_inputs_b, HRESULT_FROM_WIN32(ERROR_NOT_FOUND)}, };
const D2D1_PROPERTY_BINDING binding[] = @@ -10926,10 +11029,11 @@ static void test_effect_properties(BOOL d3d11) } system_property_tests[] = { - {effect_xml_a, L"TestEffectA", effect_author, effect_category, effect_description, 1, 1}, - {effect_xml_b, L"TestEffectB", effect_author, effect_category, effect_description, 1, 1}, - {effect_xml_c, L"TestEffectC", effect_author, effect_category, effect_description, 1, 1}, - {effect_xml_minimum, L"", L"" ,L"", L"", 0, 0}, + {effect_xml_a, L"TestEffectA", effect_author, effect_category, effect_description, 1, 1}, + {effect_xml_b, L"TestEffectB", effect_author, effect_category, effect_description, 1, 1}, + {effect_xml_c, L"TestEffectC", effect_author, effect_category, effect_description, 1, 1}, + {effect_xml_minimum, L"", L"" ,L"", L"", 0, 0}, + {effect_xml_inputs_attribute, L"", L"" ,L"", L"", 0, 0}, };
if (!init_test_context(&ctx, d3d11))
Signed-off-by: Ziqing Hui zhui@codeweavers.com ---
v2: Rework.
dlls/d2d1/Makefile.in | 2 +- dlls/d2d1/factory.c | 165 +++++++++++++++++++++++++++++++++++++++++- system_property_count | 0 3 files changed, 164 insertions(+), 3 deletions(-) create mode 100644 system_property_count
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/factory.c b/dlls/d2d1/factory.c index 01dac8051c0..38b5c59e879 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,17 @@ struct d2d_settings d2d_settings = ~0u, /* No ID2D1Factory version limit by default. */ };
+struct d2d_effect_reg +{ + CLSID id; + PD2D1_EFFECT_FACTORY factory; + UINT32 count; + + UINT32 input_count; + + struct list entry; +}; + struct d2d_factory { ID2D1Factory3 ID2D1Factory3_iface; @@ -39,6 +52,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 +66,14 @@ 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); +} + 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,148 @@ static HRESULT STDMETHODCALLTYPE d2d_factory_CreateGdiMetafile(ID2D1Factory3 *if return E_NOTIMPL; }
+static BOOL next_xml_node(IXmlReader *xml_reader, XmlNodeType *node_type, const WCHAR **node_name) +{ + while (IXmlReader_Read(xml_reader, node_type) == S_OK) + { + if (*node_type == XmlNodeType_Whitespace) + continue; + if (FAILED(IXmlReader_GetLocalName(xml_reader, node_name, NULL))) + return E_INVALIDARG; + return S_OK; + } + return S_FALSE; +} + +static HRESULT parse_property(IXmlReader *xml_reader, struct d2d_effect_reg *reg) +{ + /* FIXME: Property parsing is not implemented. */ + return S_OK; +} + +static HRESULT parse_inputs(IXmlReader *xml_reader, struct d2d_effect_reg *reg) +{ + const WCHAR *node_name; + XmlNodeType node_type; + + if (IXmlReader_IsEmptyElement(xml_reader)) + return S_OK; + + while (next_xml_node(xml_reader, &node_type, &node_name) == S_OK) + { + if (node_type == XmlNodeType_Element && !wcscmp(node_name, L"Input")) + { + ++reg->input_count; + /* FIXME: Name attribute of input is ignored. */ + } + else if (node_type == XmlNodeType_EndElement && !wcscmp(node_name, L"Inputs")) + { + return S_OK; + } + else + { + return HRESULT_FROM_WIN32(ERROR_NOT_FOUND); + } + } + + return E_INVALIDARG; +} + +static HRESULT parse_effect_xml(IStream *property_xml, struct d2d_effect_reg *reg) +{ + IXmlReader *xml_reader; + const WCHAR *node_name; + XmlNodeType node_type; + BOOL end_node_found; + 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; + + /* Read version node */ + if (next_xml_node(xml_reader, &node_type, &node_name) != S_OK || node_type != XmlNodeType_XmlDeclaration) + { + hr = HRESULT_FROM_WIN32(ERROR_NOT_FOUND); + goto done; + } + + /* Read effect node */ + if (next_xml_node(xml_reader, &node_type, &node_name) != S_OK + || node_type != XmlNodeType_Element || wcscmp(node_name, L"Effect") != 0) + { + hr = HRESULT_FROM_WIN32(ERROR_NOT_FOUND); + goto done; + } + + /* Loop inside effect node */ + end_node_found = FALSE; + while ((hr = next_xml_node(xml_reader, &node_type, &node_name)) == S_OK) + { + if (node_type == XmlNodeType_Element) + { + if (!wcscmp(node_name, L"Property")) + hr = parse_property(xml_reader, reg); + else if (!wcscmp(node_name, L"Inputs")) + hr = parse_inputs(xml_reader, reg); + else + hr = HRESULT_FROM_WIN32(ERROR_NOT_FOUND); + + if (FAILED(hr)) + goto done; + } + else if (node_type == XmlNodeType_EndElement && !wcscmp(node_name, L"Effect")) + { + end_node_found = TRUE; + break; + } + } + hr = (SUCCEEDED(hr) && end_node_found) ? S_OK : E_INVALIDARG; + +done: + 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->id)) + { + ++iter->count; + return S_OK; + } + } + + if (!(entry = heap_alloc_zero(sizeof(*entry)))) + { + hr = E_OUTOFMEMORY; + goto done; + } + + if (FAILED(hr = parse_effect_xml(property_xml, entry))) + goto done; + + entry->count = 1; + entry->id = *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 +903,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, diff --git a/system_property_count b/system_property_count new file mode 100644 index 00000000000..e69de29bb2d
On 6/12/22 08:31, Ziqing Hui wrote:
- /* Loop inside effect node */
- end_node_found = FALSE;
- while ((hr = next_xml_node(xml_reader, &node_type, &node_name)) == S_OK)
- {
if (node_type == XmlNodeType_Element){if (!wcscmp(node_name, L"Property"))hr = parse_property(xml_reader, reg);else if (!wcscmp(node_name, L"Inputs"))hr = parse_inputs(xml_reader, reg);elsehr = HRESULT_FROM_WIN32(ERROR_NOT_FOUND);if (FAILED(hr))goto done;}else if (node_type == XmlNodeType_EndElement && !wcscmp(node_name, L"Effect")){end_node_found = TRUE;break;}- }
- hr = (SUCCEEDED(hr) && end_node_found) ? S_OK : E_INVALIDARG;
I don't think it's necessary to check EndElement name. The structure is always N elements, followed by EndElement. Each element helper should do the same - skip EndElement on return, and skip elements you're not interested in entirely.
Error handling could be better, without SUCCEEDED -> S_OK fixups, or goto's.
On 6/12/22 4:30 PM, Nikolay Sivov wrote:
On 6/12/22 08:31, Ziqing Hui wrote:
+ /* Loop inside effect node */ + end_node_found = FALSE; + while ((hr = next_xml_node(xml_reader, &node_type, &node_name)) == S_OK) + { + if (node_type == XmlNodeType_Element) + { + if (!wcscmp(node_name, L"Property")) + hr = parse_property(xml_reader, reg); + else if (!wcscmp(node_name, L"Inputs")) + hr = parse_inputs(xml_reader, reg); + else + hr = HRESULT_FROM_WIN32(ERROR_NOT_FOUND);
+ if (FAILED(hr)) + goto done; + } + else if (node_type == XmlNodeType_EndElement && !wcscmp(node_name, L"Effect")) + { + end_node_found = TRUE; + break; + } + } + hr = (SUCCEEDED(hr) && end_node_found) ? S_OK : E_INVALIDARG;
I don't think it's necessary to check EndElement name. The structure is always N elements, followed by EndElement. Each element helper should do the same - skip EndElement on return, and skip elements you're not interested in entirely.
Error handling could be better, without SUCCEEDED -> S_OK fixups, or goto's.
I'm thinking of failure case. The tests in the PATCH 1 show that if we meet element that are not expected, the function will return a error code (mostly HRESULT_FROM_WIN32(ERROR_NOT_FOUND)). So I think we should not skip elements we don't interested in, we should return error for them.
Checking EndElement name is for the same reason.
Nikolay, what do you think?
On 6/13/22 09:46, Ziqing Hui wrote:
On 6/12/22 4:30 PM, Nikolay Sivov wrote:
On 6/12/22 08:31, Ziqing Hui wrote:
+ /* Loop inside effect node */ + end_node_found = FALSE; + while ((hr = next_xml_node(xml_reader, &node_type, &node_name)) == S_OK) + { + if (node_type == XmlNodeType_Element) + { + if (!wcscmp(node_name, L"Property")) + hr = parse_property(xml_reader, reg); + else if (!wcscmp(node_name, L"Inputs")) + hr = parse_inputs(xml_reader, reg); + else + hr = HRESULT_FROM_WIN32(ERROR_NOT_FOUND);
+ if (FAILED(hr)) + goto done; + } + else if (node_type == XmlNodeType_EndElement && !wcscmp(node_name, L"Effect")) + { + end_node_found = TRUE; + break; + } + } + hr = (SUCCEEDED(hr) && end_node_found) ? S_OK : E_INVALIDARG;
I don't think it's necessary to check EndElement name. The structure is always N elements, followed by EndElement. Each element helper should do the same - skip EndElement on return, and skip elements you're not interested in entirely.
Error handling could be better, without SUCCEEDED -> S_OK fixups, or goto's.
I'm thinking of failure case. The tests in the PATCH 1 show that if we meet element that are not expected, the function will return a error code (mostly HRESULT_FROM_WIN32(ERROR_NOT_FOUND)). So I think we should not skip elements we don't interested in, we should return error for them.
Checking EndElement name is for the same reason.
Nikolay, what do you think?
What I mean is that xmllite will check that for you, if closing tag does not match last open tag it will be a parser error. My suggestion was to make helpers like parse_property() consume entire <Property>...</Property> element, including end node, so the only possibility for EndElement at top level would be </Effect> one.
Regarding errors, does that mean the order of elements is important too? E.g. can you have some property elements before Inputs and some after, or it's stricter than that? I don't think we should care too much about that, we can introduce additional checks later, but for now it's fine to handle that as a general xml document.
On 6/13/22 3:38 PM, Nikolay Sivov wrote:
On 6/13/22 09:46, Ziqing Hui wrote:
On 6/12/22 4:30 PM, Nikolay Sivov wrote:
On 6/12/22 08:31, Ziqing Hui wrote:
+ /* Loop inside effect node */ + end_node_found = FALSE; + while ((hr = next_xml_node(xml_reader, &node_type, &node_name)) == S_OK) + { + if (node_type == XmlNodeType_Element) + { + if (!wcscmp(node_name, L"Property")) + hr = parse_property(xml_reader, reg); + else if (!wcscmp(node_name, L"Inputs")) + hr = parse_inputs(xml_reader, reg); + else + hr = HRESULT_FROM_WIN32(ERROR_NOT_FOUND);
+ if (FAILED(hr)) + goto done; + } + else if (node_type == XmlNodeType_EndElement && !wcscmp(node_name, L"Effect")) + { + end_node_found = TRUE; + break; + } + } + hr = (SUCCEEDED(hr) && end_node_found) ? S_OK : E_INVALIDARG;
I don't think it's necessary to check EndElement name. The structure is always N elements, followed by EndElement. Each element helper should do the same - skip EndElement on return, and skip elements you're not interested in entirely.
Error handling could be better, without SUCCEEDED -> S_OK fixups, or goto's.
I'm thinking of failure case. The tests in the PATCH 1 show that if we meet element that are not expected, the function will return a error code (mostly HRESULT_FROM_WIN32(ERROR_NOT_FOUND)). So I think we should not skip elements we don't interested in, we should return error for them.
Checking EndElement name is for the same reason.
Nikolay, what do you think?
What I mean is that xmllite will check that for you, if closing tag does not match last open tag it will be a parser error. My suggestion was to make helpers like parse_property() consume entire <Property>...</Property> element, including end node, so the only possibility for EndElement at top level would be </Effect> one.
Regarding errors, does that mean the order of elements is important too? E.g. can you have some property elements before Inputs and some after, or it's stricter than that? I don't think we should care too much about that, we can introduce additional checks later, but for now it's fine to handle that as a general xml document.
According to my test, order of elements is not important.
OK, I think we can ignore these error check now and focus on the important part.
Signed-off-by: Ziqing Hui zhui@codeweavers.com ---
v2: Rework.
dlls/d2d1/d2d1_private.h | 25 +++++ dlls/d2d1/factory.c | 203 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 226 insertions(+), 2 deletions(-)
diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h index efc9247a822..b57c971a2ce 100644 --- a/dlls/d2d1/d2d1_private.h +++ b/dlls/d2d1/d2d1_private.h @@ -603,6 +603,15 @@ 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; + BYTE *value; + PD2D1_PROPERTY_SET_FUNCTION set_function; + PD2D1_PROPERTY_GET_FUNCTION get_function; +}; + struct d2d_effect_info { const CLSID *clsid; @@ -768,4 +777,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 38b5c59e879..0bffae15ff6 100644 --- a/dlls/d2d1/factory.c +++ b/dlls/d2d1/factory.c @@ -37,6 +37,11 @@ struct d2d_effect_reg
UINT32 input_count;
+ struct d2d_effect_property *properties; + size_t property_size; + size_t property_count; + size_t system_property_count; + struct list entry; };
@@ -68,9 +73,18 @@ 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;
+ for (i = 0; i < reg->property_count; ++i) + { + heap_free(reg->properties[i].name); + heap_free(reg->properties[i].value); + } + heap_free(reg->properties); + heap_free(reg); }
@@ -603,10 +617,168 @@ static BOOL next_xml_node(IXmlReader *xml_reader, XmlNodeType *node_type, const return S_FALSE; }
+static HRESULT heap_get_attr(IXmlReader *xml_reader, const WCHAR *name, WCHAR **ptr) +{ + const WCHAR *attr_value; + + *ptr = NULL; + + if (IXmlReader_MoveToAttributeByName(xml_reader, name, NULL) != S_OK) + return E_INVALIDARG; + if (IXmlReader_GetValue(xml_reader, &attr_value, NULL) != S_OK) + return E_INVALIDARG; + if (!(*ptr = heap_strdupW(attr_value))) + return E_OUTOFMEMORY; + + return S_OK; +} + +static HRESULT get_property_type(IXmlReader *xml_reader, D2D1_PROPERTY_TYPE *type) +{ + const WCHAR *str; + unsigned int i; + + 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 */ + L"int32", /* D2D1_PROPERTY_TYPE_INT32 */ + L"float", /* D2D1_PROPERTY_TYPE_FLOAT */ + L"vector2", /* D2D1_PROPERTY_TYPE_VECTOR2 */ + L"vector3", /* D2D1_PROPERTY_TYPE_VECTOR3 */ + L"vector4", /* D2D1_PROPERTY_TYPE_VECTOR4 */ + L"blob", /* D2D1_PROPERTY_TYPE_BLOB */ + L"iunknown", /* D2D1_PROPERTY_TYPE_IUNKNOWN */ + L"enum", /* D2D1_PROPERTY_TYPE_ENUM */ + L"array", /* D2D1_PROPERTY_TYPE_ARRAY */ + L"clsid", /* D2D1_PROPERTY_TYPE_CLSID */ + L"matrix3x2", /* D2D1_PROPERTY_TYPE_MATRIX_3X2 */ + L"matrix4x3", /* D2D1_PROPERTY_TYPE_MATRIX_4X3 */ + L"matrix4x4", /* D2D1_PROPERTY_TYPE_MATRIX_4X4 */ + L"matrix5x4", /* D2D1_PROPERTY_TYPE_MATRIX_5X4 */ + L"colorcontext", /* D2D1_PROPERTY_TYPE_COLOR_CONTEXT */ + }; + + if (IXmlReader_MoveToAttributeByName(xml_reader, L"type", NULL) != S_OK) + return E_INVALIDARG; + if (IXmlReader_GetValue(xml_reader, &str, NULL) != S_OK) + return E_INVALIDARG; + + for (i = 0; i < ARRAY_SIZE(type_str); ++i) + { + if (!wcscmp(str, type_str[i])) + { + *type = i; + return S_OK; + } + } + + return E_INVALIDARG; +} + +static HRESULT add_property(struct d2d_effect_reg *reg, WCHAR *name, D2D1_PROPERTY_TYPE type, BYTE *value) +{ + struct d2d_effect_property *entry; + + if (!d2d_array_reserve((void **)®->properties, ®->property_size, reg->property_count + 1, sizeof(*reg->properties))) + { + ERR("Failed to resize properties array.\n"); + return E_OUTOFMEMORY; + } + + if (!wcscmp(name, L"DisplayName") + || !wcscmp(name, L"Author") + || !wcscmp(name, L"Category") + || !wcscmp(name, L"Description")) + { + if (type != D2D1_PROPERTY_TYPE_STRING) + return E_INVALIDARG; + ++reg->system_property_count; + } + + entry = ®->properties[reg->property_count++]; + entry->name = name; + entry->type = type; + entry->value = value; + entry->set_function = NULL; + entry->get_function = NULL; + + return S_OK; +} + +static HRESULT parse_sub_property(IXmlReader *xml_reader, struct d2d_effect_reg *reg) +{ + /* FIXME: Sub property is ignored. */ + return S_OK; +} + static HRESULT parse_property(IXmlReader *xml_reader, struct d2d_effect_reg *reg) { - /* FIXME: Property parsing is not implemented. */ - return S_OK; + WCHAR *name = NULL, *value = NULL; + D2D1_PROPERTY_TYPE type; + const WCHAR *node_name; + XmlNodeType node_type; + BOOL end_node_found; + unsigned int i; + HRESULT hr; + + /* Get property name, type, value */ + if (FAILED(hr = heap_get_attr(xml_reader, L"name", &name))) + return hr; + if (FAILED(hr = get_property_type(xml_reader, &type))) + goto done; + heap_get_attr(xml_reader, L"value", &value); + + /* Check if property is already set */ + for (i = 0; i < reg->property_count; ++i) + { + if (!wcscmp(name, reg->properties[i].name)) + { + hr = E_INVALIDARG; + goto done; + } + } + + /* Parse sub properties */ + if (FAILED(hr = IXmlReader_MoveToElement(xml_reader))) + goto done; + if (!IXmlReader_IsEmptyElement(xml_reader)) + { + end_node_found = FALSE; + while ((hr = next_xml_node(xml_reader, &node_type, &node_name)) == S_OK) + { + if (node_type == XmlNodeType_Element && !wcscmp(node_name, L"Property")) + { + if (FAILED(hr = parse_sub_property(xml_reader, reg))) + goto done; + } + else if (node_type == XmlNodeType_EndElement && !wcscmp(node_name, L"Property")) + { + end_node_found = TRUE; + break; + } + } + + if (FAILED(hr) || !end_node_found) + { + hr = E_INVALIDARG; + goto done; + } + } + + /* Add property to array */ + hr = add_property(reg, name, type, (BYTE *)value); + +done: + if (hr != S_OK) + { + heap_free(value); + heap_free(name); + } + + return hr; }
static HRESULT parse_inputs(IXmlReader *xml_reader, struct d2d_effect_reg *reg) @@ -700,6 +872,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", @@ -723,6 +896,32 @@ static HRESULT STDMETHODCALLTYPE d2d_factory_RegisterEffectFromStream(ID2D1Facto if (FAILED(hr = parse_effect_xml(property_xml, entry))) goto done;
+ /* System properties: DisplayName, Author, Category, Description are required */ + if (entry->system_property_count != 4) + { + hr = E_INVALIDARG; + 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->id = *effect_id; entry->factory = effect_factory;
On 6/12/22 08:31, Ziqing Hui wrote:
- if (!wcscmp(name, L"DisplayName")
|| !wcscmp(name, L"Author")|| !wcscmp(name, L"Category")|| !wcscmp(name, L"Description"))- {
if (type != D2D1_PROPERTY_TYPE_STRING)return E_INVALIDARG;++reg->system_property_count;- }
That's not going to work for nested properties, that likely can have same names as system ones.
if (j > entry->property_count){hr = D2DERR_INVALID_PROPERTY;goto done;}
Should it be j == entry->property_count ?
On 6/12/22 4:34 PM, Nikolay Sivov wrote:
On 6/12/22 08:31, Ziqing Hui wrote:
+ if (!wcscmp(name, L"DisplayName") + || !wcscmp(name, L"Author") + || !wcscmp(name, L"Category") + || !wcscmp(name, L"Description")) + { + if (type != D2D1_PROPERTY_TYPE_STRING) + return E_INVALIDARG; + ++reg->system_property_count; + }
That's not going to work for nested properties, that likely can have same names as system ones.
Does "nested properties" means sub properties? If it does, I think it's OK because sub properties is ignored. My plan is to ignore sub properties unless we really need them in effect implementation.
So at least for now add_property() will only be called for top level properties.
+ if (j > entry->property_count) + { + hr = D2DERR_INVALID_PROPERTY; + goto done; + }
Should it be j == entry->property_count ?
On 6/13/22 09:55, Ziqing Hui wrote:
On 6/12/22 4:34 PM, Nikolay Sivov wrote:
On 6/12/22 08:31, Ziqing Hui wrote:
+ if (!wcscmp(name, L"DisplayName") + || !wcscmp(name, L"Author") + || !wcscmp(name, L"Category") + || !wcscmp(name, L"Description")) + { + if (type != D2D1_PROPERTY_TYPE_STRING) + return E_INVALIDARG; + ++reg->system_property_count; + }
That's not going to work for nested properties, that likely can have same names as system ones.
Does "nested properties" means sub properties? If it does, I think it's OK because sub properties is ignored. My plan is to ignore sub properties unless we really need them in effect implementation.
So at least for now add_property() will only be called for top level properties.
I understand that, and it's fine to ignore them. But alternative way to test for required properties is simply to check if they were added once you parsed everything, instead of counting them. By checking I mean to look them up by names in registration structure after xml document was processed.
+ if (j > entry->property_count) + { + hr = D2DERR_INVALID_PROPERTY; + goto done; + }
Should it be j == entry->property_count ?
Signed-off-by: Ziqing Hui zhui@codeweavers.com ---
v2: Write then seek.
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 0bffae15ff6..31bad2cdb63 100644 --- a/dlls/d2d1/factory.c +++ b/dlls/d2d1/factory.c @@ -937,10 +937,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); + static const LARGE_INTEGER zero; + IStream *stream; + ULONG size; + 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); + + if (FAILED(hr = CreateStreamOnHGlobal(NULL, TRUE, &stream))) + return hr; + + size = sizeof(*property_xml) * (wcslen(property_xml) + 1); + if (FAILED(hr = IStream_Write(stream, property_xml, size, NULL))) + goto done; + if (FAILED(hr = IStream_Seek(stream, zero, SEEK_SET, NULL))) + goto done; + + hr = ID2D1Factory3_RegisterEffectFromStream(iface, effect_id, stream, bindings, binding_count, effect_factory); + +done: + IStream_Release(stream); + return hr; }
static HRESULT STDMETHODCALLTYPE d2d_factory_UnregisterEffect(ID2D1Factory3 *iface, REFCLSID effect_id)
On 6/12/22 08:31, Ziqing Hui wrote:
- if (FAILED(hr = CreateStreamOnHGlobal(NULL, TRUE, &stream)))
return hr;- size = sizeof(*property_xml) * (wcslen(property_xml) + 1);
- if (FAILED(hr = IStream_Write(stream, property_xml, size, NULL)))
goto done;- if (FAILED(hr = IStream_Seek(stream, zero, SEEK_SET, NULL)))
goto done;- hr = ID2D1Factory3_RegisterEffectFromStream(iface, effect_id, stream, bindings, binding_count, effect_factory);
+done:
- IStream_Release(stream);
- return hr; }
This is short enough, it doesn't have to use gotos.
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 31bad2cdb63..2adcd1bd575 100644 --- a/dlls/d2d1/factory.c +++ b/dlls/d2d1/factory.c @@ -963,9 +963,26 @@ done:
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->id)) + { + --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 970706bc569..76b10f3339e 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -10772,7 +10772,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_min_max_inputs_a + || test->xml == effect_xml_without_inputs) ok(hr == test->hr, "Got unexpected hr %#lx, expected %#lx.\n", hr, test->hr); if (hr == S_OK) { @@ -10780,9 +10781,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); } @@ -10805,11 +10806,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(); } @@ -10844,9 +10845,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) @@ -10905,13 +10906,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); } @@ -10989,7 +10990,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); } @@ -11147,7 +11147,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(); }
@@ -11240,7 +11240,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); }
-
Nikolay Sivov -
Ziqing Hui