[PATCH 1/4] d2d1: Add a properties list for builtin effects.
Signed-off-by: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/d2d1/d2d1_private.h | 2 +- dlls/d2d1/device.c | 2 +- dlls/d2d1/effect.c | 45 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h index ce99e7c3432..1cca943122e 100644 --- a/dlls/d2d1/d2d1_private.h +++ b/dlls/d2d1/d2d1_private.h @@ -574,7 +574,7 @@ struct d2d_effect ID2D1Factory *factory; }; -void d2d_effect_init(struct d2d_effect *effect, ID2D1Factory *factory) DECLSPEC_HIDDEN; +void d2d_effect_init(struct d2d_effect *effect, ID2D1Factory *factory, const CLSID *effect_id) DECLSPEC_HIDDEN; static inline BOOL d2d_array_reserve(void **elements, size_t *capacity, size_t count, size_t size) { diff --git a/dlls/d2d1/device.c b/dlls/d2d1/device.c index 0647bc57fc3..cc86c6332f9 100644 --- a/dlls/d2d1/device.c +++ b/dlls/d2d1/device.c @@ -1893,7 +1893,7 @@ static HRESULT STDMETHODCALLTYPE d2d_device_context_CreateEffect(ID2D1DeviceCont if (!(object = heap_alloc_zero(sizeof(*object)))) return E_OUTOFMEMORY; - d2d_effect_init(object, context->factory); + d2d_effect_init(object, context->factory, effect_id); TRACE("Created effect %p.\n", object); *effect = &object->ID2D1Effect_iface; diff --git a/dlls/d2d1/effect.c b/dlls/d2d1/effect.c index 40dd2187953..c9d8085f81a 100644 --- a/dlls/d2d1/effect.c +++ b/dlls/d2d1/effect.c @@ -20,6 +20,31 @@ WINE_DEFAULT_DEBUG_CHANNEL(d2d); +struct d2d_effect_info +{ + const CLSID *clsid; + BOOL cached; + D2D1_BUFFER_PRECISION precision; + UINT32 min_inputs; + UINT32 max_inputs; +}; + +static const struct d2d_effect_info builtin_effects[] = +{ + { + &CLSID_D2D12DAffineTransform, + FALSE, D2D1_BUFFER_PRECISION_UNKNOWN, 1, 1 + }, + { + &CLSID_D2D13DPerspectiveTransform, + FALSE, D2D1_BUFFER_PRECISION_UNKNOWN, 1, 1 + }, + { + &CLSID_D2D1Composite, + FALSE, D2D1_BUFFER_PRECISION_UNKNOWN, 1, 0xffffffff + } +}; + static inline struct d2d_effect *impl_from_ID2D1Effect(ID2D1Effect *iface) { return CONTAINING_RECORD(iface, struct d2d_effect, ID2D1Effect_iface); @@ -269,10 +294,28 @@ static const ID2D1ImageVtbl d2d_effect_image_vtbl = d2d_effect_image_GetFactory, }; -void d2d_effect_init(struct d2d_effect *effect, ID2D1Factory *factory) +static void d2d_effect_init_standard_properties(struct d2d_effect *effect, const struct d2d_effect_info *info) { + /* TODO */ +} + +void d2d_effect_init(struct d2d_effect *effect, ID2D1Factory *factory, const CLSID *effect_id) +{ + unsigned int i; + effect->ID2D1Effect_iface.lpVtbl = &d2d_effect_vtbl; effect->ID2D1Image_iface.lpVtbl = &d2d_effect_image_vtbl; effect->refcount = 1; ID2D1Factory_AddRef(effect->factory = factory); + + for (i = 0; i < ARRAY_SIZE(builtin_effects); ++i) + { + if (IsEqualGUID(effect_id, builtin_effects[i].clsid)) + { + d2d_effect_init_standard_properties(effect, builtin_effects + i); + return; + } + } + + WARN("Unsupported effect clsid %s.\n", debugstr_guid(effect_id)); } -- 2.25.1
Signed-off-by: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/d2d1/d2d1_private.h | 13 +++++++++++++ dlls/d2d1/effect.c | 24 +++++++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h index 1cca943122e..f4bc1ab2011 100644 --- a/dlls/d2d1/d2d1_private.h +++ b/dlls/d2d1/d2d1_private.h @@ -565,6 +565,13 @@ struct d2d_device void d2d_device_init(struct d2d_device *device, ID2D1Factory1 *factory, IDXGIDevice *dxgi_device) DECLSPEC_HIDDEN; +struct d2d_property +{ + D2D1_PROPERTY_TYPE type; + void *value; + UINT32 value_size; +}; + struct d2d_effect { ID2D1Effect ID2D1Effect_iface; @@ -572,6 +579,12 @@ struct d2d_effect LONG refcount; ID2D1Factory *factory; + CLSID clsid; + BOOL cached; + UINT min_inputs; + UINT max_inputs; + D2D1_BUFFER_PRECISION precision; + struct d2d_property standard_properties[10]; }; void d2d_effect_init(struct d2d_effect *effect, ID2D1Factory *factory, const CLSID *effect_id) DECLSPEC_HIDDEN; diff --git a/dlls/d2d1/effect.c b/dlls/d2d1/effect.c index c9d8085f81a..2d70ec84f6a 100644 --- a/dlls/d2d1/effect.c +++ b/dlls/d2d1/effect.c @@ -294,9 +294,31 @@ static const ID2D1ImageVtbl d2d_effect_image_vtbl = d2d_effect_image_GetFactory, }; +static void d2d_property_set(struct d2d_property *property, D2D1_PROPERTY_TYPE type, void *value, UINT32 value_size) +{ + property->type = type; + property->value = value; + property->value_size = value_size; +} + static void d2d_effect_init_standard_properties(struct d2d_effect *effect, const struct d2d_effect_info *info) { - /* TODO */ + effect->clsid = *info->clsid; + effect->cached = info->cached; + effect->precision = info->precision; + effect->min_inputs = info->min_inputs; + effect->max_inputs = info->max_inputs; + + d2d_property_set(effect->standard_properties + D2D1_PROPERTY_CLSID - D2D1_PROPERTY_CLSID, + D2D1_PROPERTY_TYPE_CLSID, &effect->clsid, sizeof(effect->clsid)); + d2d_property_set(effect->standard_properties + D2D1_PROPERTY_CACHED - D2D1_PROPERTY_CLSID, + D2D1_PROPERTY_TYPE_BOOL, &effect->cached, sizeof(effect->cached)); + d2d_property_set(effect->standard_properties + D2D1_PROPERTY_PRECISION - D2D1_PROPERTY_CLSID, + D2D1_PROPERTY_TYPE_ENUM, &effect->precision, sizeof(effect->precision)); + d2d_property_set(effect->standard_properties + D2D1_PROPERTY_MIN_INPUTS - D2D1_PROPERTY_CLSID, + D2D1_PROPERTY_TYPE_UINT32, &effect->min_inputs, sizeof(effect->min_inputs)); + d2d_property_set(effect->standard_properties + D2D1_PROPERTY_MAX_INPUTS - D2D1_PROPERTY_CLSID, + D2D1_PROPERTY_TYPE_UINT32, &effect->max_inputs, sizeof(effect->max_inputs)); } void d2d_effect_init(struct d2d_effect *effect, ID2D1Factory *factory, const CLSID *effect_id) -- 2.25.1
On Tue, 27 Jul 2021 at 13:11, Ziqing Hui <zhui(a)codeweavers.com> wrote:
@@ -572,6 +579,12 @@ struct d2d_effect LONG refcount;
ID2D1Factory *factory; + CLSID clsid; + BOOL cached; + UINT min_inputs; + UINT max_inputs; + D2D1_BUFFER_PRECISION precision; + struct d2d_property standard_properties[10]; };
I don't think the fixed-size array of an arbitrary size is great. More, if these are static properties, there's really not much of a point in storing them like that; we could just as well store a static const table, not entirely unlike the builtin_effects[] table from patch 1/4. Like patch 1/4, most of this patch is dead code.
Hi Henri, On 7/29/21 10:50 PM, Henri Verbeet wrote:
On Tue, 27 Jul 2021 at 13:11, Ziqing Hui <zhui(a)codeweavers.com> wrote:
@@ -572,6 +579,12 @@ struct d2d_effect LONG refcount;
ID2D1Factory *factory; + CLSID clsid; + BOOL cached; + UINT min_inputs; + UINT max_inputs; + D2D1_BUFFER_PRECISION precision; + struct d2d_property standard_properties[10]; };
I don't think the fixed-size array of an arbitrary size is great.
Should we allocate the standard_properties field by heap_alloc() or use "struct d2d_property standard_properties[D2D1_PROPERTY_MAX_INPUTS - D2D1_PROPERTY_CLSID + 1];" or just remove this field?
More, if these are static properties, there's really not much of a point in storing them like that; we could just as well store a static const table, not entirely unlike the builtin_effects[] table from patch 1/4.
Does it means like we should use something like this: struct d2d_effect { .... const struct d2d_info *info; }
Like patch 1/4, most of this patch is dead code.
I submit a new patch set which makes min_inputs and max_inputs "alive". I am planning to submit a V2 version of this patch set after that patch set is accepted.
On Fri, 30 Jul 2021 at 06:13, Ziqing Hui <zhui(a)codeweavers.com> wrote:
On 7/29/21 10:50 PM, Henri Verbeet wrote:
On Tue, 27 Jul 2021 at 13:11, Ziqing Hui <zhui(a)codeweavers.com> wrote:
@@ -572,6 +579,12 @@ struct d2d_effect LONG refcount;
ID2D1Factory *factory; + CLSID clsid; + BOOL cached; + UINT min_inputs; + UINT max_inputs; + D2D1_BUFFER_PRECISION precision; + struct d2d_property standard_properties[10]; };
I don't think the fixed-size array of an arbitrary size is great.
Should we allocate the standard_properties field by heap_alloc()
or use "struct d2d_property standard_properties[D2D1_PROPERTY_MAX_INPUTS - D2D1_PROPERTY_CLSID + 1];"
or just remove this field?
More, if these are static properties, there's really not much of a point in storing them like that; we could just as well store a static const table, not entirely unlike the builtin_effects[] table from patch 1/4.
Does it means like we should use something like this:
struct d2d_effect { .... const struct d2d_info *info; }
Potentially, yes.
Signed-off-by: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/d2d1/tests/d2d1.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c index 72487692abf..629bbd65523 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -9680,13 +9680,14 @@ static void test_mt_factory(BOOL d3d11) static void test_effect(BOOL d3d11) { - unsigned int i, min_inputs, max_inputs; + unsigned int i, min_inputs, max_inputs, str_size; D2D1_BUFFER_PRECISION precision; ID2D1Image *image_a, *image_b; struct d2d1_test_context ctx; ID2D1DeviceContext *context; ID2D1Factory1 *factory; ID2D1Effect *effect; + BYTE buffer[64]; BOOL cached; CLSID clsid; HRESULT hr; @@ -9735,6 +9736,33 @@ static void test_effect(BOOL d3d11) todo_wine { + hr = ID2D1Effect_GetValue(effect, 0xdeadbeef, D2D1_PROPERTY_TYPE_CLSID, (BYTE *)&clsid, sizeof(clsid)); + ok(hr == D2DERR_INVALID_PROPERTY, "Got unexpected hr %#x.\n", hr); + + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, buffer, sizeof(clsid) + 1); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, buffer, sizeof(clsid) - 1); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, buffer, sizeof(clsid)); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_DISPLAYNAME, D2D1_PROPERTY_TYPE_STRING, buffer, sizeof(buffer)); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + str_size = (wcslen((WCHAR *)buffer) + 1) * sizeof(WCHAR); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_DISPLAYNAME, D2D1_PROPERTY_TYPE_STRING, buffer, str_size); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_DISPLAYNAME, D2D1_PROPERTY_TYPE_STRING, buffer, str_size - 1); + ok(hr == D2DERR_INSUFFICIENT_BUFFER, "Got unexpected hr %#x.\n", hr); + + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, 0xdeadbeef, (BYTE *)&clsid, sizeof(clsid)); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_UNKNOWN, (BYTE *)&clsid, sizeof(clsid)); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_VECTOR4, (BYTE *)&clsid, sizeof(clsid)); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_VECTOR4, buffer, sizeof(buffer)); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, (BYTE *)&clsid, sizeof(clsid)); ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); -- 2.25.1
Signed-off-by: Giovanni Mascellani <gmascellani(a)codeweavers.com> Il 27/07/21 13:10, Ziqing Hui ha scritto:
Signed-off-by: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/d2d1/tests/d2d1.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c index 72487692abf..629bbd65523 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -9680,13 +9680,14 @@ static void test_mt_factory(BOOL d3d11)
static void test_effect(BOOL d3d11) { - unsigned int i, min_inputs, max_inputs; + unsigned int i, min_inputs, max_inputs, str_size; D2D1_BUFFER_PRECISION precision; ID2D1Image *image_a, *image_b; struct d2d1_test_context ctx; ID2D1DeviceContext *context; ID2D1Factory1 *factory; ID2D1Effect *effect; + BYTE buffer[64]; BOOL cached; CLSID clsid; HRESULT hr; @@ -9735,6 +9736,33 @@ static void test_effect(BOOL d3d11)
todo_wine { + hr = ID2D1Effect_GetValue(effect, 0xdeadbeef, D2D1_PROPERTY_TYPE_CLSID, (BYTE *)&clsid, sizeof(clsid)); + ok(hr == D2DERR_INVALID_PROPERTY, "Got unexpected hr %#x.\n", hr); + + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, buffer, sizeof(clsid) + 1); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, buffer, sizeof(clsid) - 1); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, buffer, sizeof(clsid)); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_DISPLAYNAME, D2D1_PROPERTY_TYPE_STRING, buffer, sizeof(buffer)); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + str_size = (wcslen((WCHAR *)buffer) + 1) * sizeof(WCHAR); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_DISPLAYNAME, D2D1_PROPERTY_TYPE_STRING, buffer, str_size); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_DISPLAYNAME, D2D1_PROPERTY_TYPE_STRING, buffer, str_size - 1); + ok(hr == D2DERR_INSUFFICIENT_BUFFER, "Got unexpected hr %#x.\n", hr); + + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, 0xdeadbeef, (BYTE *)&clsid, sizeof(clsid)); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_UNKNOWN, (BYTE *)&clsid, sizeof(clsid)); + ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_VECTOR4, (BYTE *)&clsid, sizeof(clsid)); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_VECTOR4, buffer, sizeof(buffer)); + ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); + hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, (BYTE *)&clsid, sizeof(clsid)); ok(hr == S_OK, "Got unexpected hr %#x.\n", hr);
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
Signed-off-by: Ziqing Hui <zhui(a)codeweavers.com> --- dlls/d2d1/effect.c | 28 +++++++++++++++++++++++++--- dlls/d2d1/tests/d2d1.c | 27 +++++++++++---------------- 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/dlls/d2d1/effect.c b/dlls/d2d1/effect.c index 2d70ec84f6a..02e81b86973 100644 --- a/dlls/d2d1/effect.c +++ b/dlls/d2d1/effect.c @@ -169,10 +169,32 @@ static HRESULT STDMETHODCALLTYPE d2d_effect_GetValueByName(ID2D1Effect *iface, c static HRESULT STDMETHODCALLTYPE d2d_effect_GetValue(ID2D1Effect *iface, UINT32 index, D2D1_PROPERTY_TYPE type, BYTE *value, UINT32 value_size) { - FIXME("iface %p, index %u, type %#x, value %p, value_size %u stub!\n", iface, index, type, - value, value_size); + struct d2d_effect *effect = impl_from_ID2D1Effect(iface); + struct d2d_property *property; - return E_NOTIMPL; + TRACE("iface %p, index %u, type %#x, value %p, value_size %u.\n", iface, index, type, value, value_size); + + if (index < D2D1_PROPERTY_CLSID) + { + FIXME("Custom properties are not supported.\n"); + return D2DERR_INVALID_PROPERTY; + } + + if (index > D2D1_PROPERTY_MAX_INPUTS) + return D2DERR_INVALID_PROPERTY; + + property = effect->standard_properties + index - D2D1_PROPERTY_CLSID; + + if ((type > D2D1_PROPERTY_TYPE_COLOR_CONTEXT) + || (type != D2D1_PROPERTY_TYPE_UNKNOWN && type != property->type) + || (type != D2D1_PROPERTY_TYPE_STRING && value_size != property->value_size)) + return E_INVALIDARG; + if (type == D2D1_PROPERTY_TYPE_STRING && value_size < property->value_size) + return D2DERR_INSUFFICIENT_BUFFER; + + memcpy(value, property->value, property->value_size); + + return S_OK; } static UINT32 STDMETHODCALLTYPE d2d_effect_GetValueSize(ID2D1Effect *iface, UINT32 index) diff --git a/dlls/d2d1/tests/d2d1.c b/dlls/d2d1/tests/d2d1.c index 629bbd65523..dde41912068 100644 --- a/dlls/d2d1/tests/d2d1.c +++ b/dlls/d2d1/tests/d2d1.c @@ -9734,8 +9734,6 @@ static void test_effect(BOOL d3d11) ID2D1Image_Release(image_b); ID2D1Image_Release(image_a); - todo_wine - { hr = ID2D1Effect_GetValue(effect, 0xdeadbeef, D2D1_PROPERTY_TYPE_CLSID, (BYTE *)&clsid, sizeof(clsid)); ok(hr == D2DERR_INVALID_PROPERTY, "Got unexpected hr %#x.\n", hr); @@ -9746,6 +9744,8 @@ static void test_effect(BOOL d3d11) hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, buffer, sizeof(clsid)); ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); + todo_wine + { hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_DISPLAYNAME, D2D1_PROPERTY_TYPE_STRING, buffer, sizeof(buffer)); ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); str_size = (wcslen((WCHAR *)buffer) + 1) * sizeof(WCHAR); @@ -9753,6 +9753,7 @@ static void test_effect(BOOL d3d11) ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_DISPLAYNAME, D2D1_PROPERTY_TYPE_STRING, buffer, str_size - 1); ok(hr == D2DERR_INSUFFICIENT_BUFFER, "Got unexpected hr %#x.\n", hr); + } hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, 0xdeadbeef, (BYTE *)&clsid, sizeof(clsid)); ok(hr == E_INVALIDARG, "Got unexpected hr %#x.\n", hr); @@ -9766,36 +9767,30 @@ static void test_effect(BOOL d3d11) hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CLSID, D2D1_PROPERTY_TYPE_CLSID, (BYTE *)&clsid, sizeof(clsid)); ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); - if (hr == S_OK) - ok(IsEqualGUID(&clsid, test->clsid), "Got unexpected clsid %s, expected %s.\n", - debugstr_guid(&clsid), debugstr_guid(test->clsid)); + ok(IsEqualGUID(&clsid, test->clsid), "Got unexpected clsid %s, expected %s.\n", + debugstr_guid(&clsid), debugstr_guid(test->clsid)); hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_CACHED, D2D1_PROPERTY_TYPE_BOOL, (BYTE *)&cached, sizeof(cached)); ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); - if (hr == S_OK) - ok(cached == FALSE, "Got unexpected cached %d.\n", cached); + ok(cached == FALSE, "Got unexpected cached %d.\n", cached); hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_PRECISION, D2D1_PROPERTY_TYPE_ENUM, (BYTE *)&precision, sizeof(precision)); ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); - if (hr == S_OK) - ok(precision == D2D1_BUFFER_PRECISION_UNKNOWN, "Got unexpected precision %u.\n", precision); + ok(precision == D2D1_BUFFER_PRECISION_UNKNOWN, "Got unexpected precision %u.\n", precision); hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_MIN_INPUTS, D2D1_PROPERTY_TYPE_UINT32, (BYTE *)&min_inputs, sizeof(min_inputs)); ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); - if (hr == S_OK) - ok(min_inputs == test->min_inputs, "Got unexpected min inputs %u, expected %u.\n", - min_inputs, test->min_inputs); + ok(min_inputs == test->min_inputs, "Got unexpected min inputs %u, expected %u.\n", + min_inputs, test->min_inputs); hr = ID2D1Effect_GetValue(effect, D2D1_PROPERTY_MAX_INPUTS, D2D1_PROPERTY_TYPE_UINT32, (BYTE *)&max_inputs, sizeof(max_inputs)); ok(hr == S_OK, "Got unexpected hr %#x.\n", hr); - if (hr == S_OK) - ok(max_inputs == test->max_inputs, "Got unexpected max inputs %u, expected %u.\n", - max_inputs, test->max_inputs); - } + ok(max_inputs == test->max_inputs, "Got unexpected max inputs %u, expected %u.\n", + max_inputs, test->max_inputs); ID2D1Effect_Release(effect); winetest_pop_context(); -- 2.25.1
Hi, Il 27/07/21 13:10, Ziqing Hui ha scritto:
+void d2d_effect_init(struct d2d_effect *effect, ID2D1Factory *factory, const CLSID *effect_id) +{ + unsigned int i; + effect->ID2D1Effect_iface.lpVtbl = &d2d_effect_vtbl; effect->ID2D1Image_iface.lpVtbl = &d2d_effect_image_vtbl; effect->refcount = 1; ID2D1Factory_AddRef(effect->factory = factory); + + for (i = 0; i < ARRAY_SIZE(builtin_effects); ++i) + { + if (IsEqualGUID(effect_id, builtin_effects[i].clsid)) + { + d2d_effect_init_standard_properties(effect, builtin_effects + i); + return; + } + } + + WARN("Unsupported effect clsid %s.\n", debugstr_guid(effect_id)); }
My understanding is that you want to implement three different effects using the same class, which I suppose will dynamically change its behavior depending on the stored clsid. Personally I would rather write three different classes, each of which implements one effect (I would even put them in three different files, but I guess this is not really in Wine's philosophy). They can share code, if needed, but having three different classes (with three virtual tables) means that you don't have to do some kind of switch on the clsid each time the implementations differ. Also, each class can hardcode the values you're putting in builtin_effects without having to lookup a shared table, so you can keep the implementations more self contained. Just my two cents, though; given that my Signed-off-by has basically no relevance, you might want to check how the big bosses want stuff to be done. :-) Giovanni.
Hi Giovanni, On 7/27/21 9:55 PM, Giovanni Mascellani wrote:
Hi,
Il 27/07/21 13:10, Ziqing Hui ha scritto:
+void d2d_effect_init(struct d2d_effect *effect, ID2D1Factory *factory, const CLSID *effect_id) +{ + unsigned int i; + effect->ID2D1Effect_iface.lpVtbl = &d2d_effect_vtbl; effect->ID2D1Image_iface.lpVtbl = &d2d_effect_image_vtbl; effect->refcount = 1; ID2D1Factory_AddRef(effect->factory = factory); + + for (i = 0; i < ARRAY_SIZE(builtin_effects); ++i) + { + if (IsEqualGUID(effect_id, builtin_effects[i].clsid)) + { + d2d_effect_init_standard_properties(effect, builtin_effects + i); + return; + } + } + + WARN("Unsupported effect clsid %s.\n", debugstr_guid(effect_id)); }
My understanding is that you want to implement three different effects using the same class, which I suppose will dynamically change its behavior depending on the stored clsid. Personally I would rather write three different classes, each of which implements one effect (I would even put them in three different files, but I guess this is not really in Wine's philosophy). They can share code, if needed, but having three different classes (with three virtual tables) means that you don't have to do some kind of switch on the clsid each time the implementations differ. Also, each class can hardcode the values you're putting in builtin_effects without having to lookup a shared table, so you can keep the implementations more self contained.
Just my two cents, though; given that my Signed-off-by has basically no relevance, you might want to check how the big bosses want stuff to be done. :-)
Giovanni.
Thanks for your reply. My plan is to add something like " struct d2d_effect_ops", which is a vtable. And each effect class needs to implement this vtable. It may look like this: struct d2d_effect_ops { void (*op1)(); void (*op2)(); .... } struct d2d_effect { .... struct d2d_effect_ops *ops; } Assuming that we are trying to implement 2D Affine Effect: void 2d_affine_op1(){ ....} void 2d_affine_op2(){ ....} static struct d2d_effect_ops 2d_affine_ops = { 2d_affine_op1, 2d_affine_op2, .... } And this op table will be initialized in d2d_effect_init() according to clsid.
Hi Ziqing, Il 29/07/21 03:30, Ziqing Hui ha scritto:
Thanks for your reply. My plan is to add something like " struct d2d_effect_ops", which is a vtable. And each effect class needs to implement this vtable. It may look like this: I think this strategy makes the code less idiomatic, therefore harder to read and maintain, and it requires a double virtual dispatching, for which I see no use. The COM virtual dispatching mechanism is already good enough to implement all the dynamic behavior you need, so I would use that one, putting each effect in its own class.
This would mean something like: struct d2d_affine_effect { ID2D1Effect ID2D1Effect_iface; ... }; static const ID2D1EffectVtbl d2d_affine_effect_vtbl = { d2d_affine_effect_QueryInterface, d2d_affine_effect_AddRef, d2d_affine_effect_Release, d2d_affine_effect_GetPropertyCount, ... }; void d2d_effect_init(ID2D1Effect **effect, ID2D1Factory *factory, const CLSID *effect_id) { if (IsEqualGUID(effect_id, &CLSID_D2D12DAffineTransform)) { struct d2d_affine_effect *object = heap_alloc_zero(sizeof(*object)); object->ID2D1Effect_iface.lpVtbl = &d2d_affine_effect; ... *effect = object->ID2D1Effect_iface.lpVtbl; return S_OK; } else if (IsEqualGUID(effect_id, &CLSID_D2D12DPerspectiveTransform)) ... FIXME("Effect not implemented: %p\n", wine_dbgstr_guid(effect_id)); return D2DERR_EFFECT_IS_NOT_REGISTERED; } (coding out of my mind, might require some tweaks, of course) The implementations of the methods in the three different classes can very well share code by mean of helper functions, if that's useful. Again, just my 2 cents. Giovanni.
It may be a little early to worry about virtual calls at this point; I think there's enough time to worry about those when we need them. (And we may not; it's not clear to me how we intend to implement the actual effects, but there seems to be a good chance that the differences between the different effects would largely be expressed as static data.) The more immediate issue I have with this patch is that much of it amounts to being dead code; other than the "clsid" field, none of the information in builtin_effects[] is actually used. On Tue, 27 Jul 2021 at 13:11, Ziqing Hui <zhui(a)codeweavers.com> wrote:
+ for (i = 0; i < ARRAY_SIZE(builtin_effects); ++i) + { + if (IsEqualGUID(effect_id, builtin_effects[i].clsid)) + { + d2d_effect_init_standard_properties(effect, builtin_effects + i); + return; + } + }
By convention, "&builtin_effects[i]" instead of "builtin_effects + i".
participants (4)
-
Giovanni Mascellani -
Henri Verbeet -
Henri Verbeet -
Ziqing Hui