From: Nikolay Sivov <nsivov@codeweavers.com>
Subject: Re: [PATCH 2/5] d2d1: Partially implement RegisterEffectFromStream().
Message-Id: <e6add160-d761-1675-0ac9-5e76ade56cf9@codeweavers.com>
Date: Mon, 6 Jun 2022 11:29:20 +0300
In-Reply-To: <20220606073525.1774002-2-zhui@codeweavers.com>
References: <20220606073525.1774002-1-zhui@codeweavers.com> <20220606073525.1774002-2-zhui@codeweavers.com>



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.