[PATCH 2/5] d3d10/effect: Partially implement D3D10CreateEffectPoolFromMemory().

[Nikolay Sivov]

On 9/19/21 9:03 PM, Matteo Bruni wrote:
> On Thu, Sep 16, 2021 at 8:48 AM Nikolay Sivov <nsivov at codeweavers.com> wrote:
>> Signed-off-by: Nikolay Sivov <nsivov at codeweavers.com>
>> ---
>>  dlls/d3d10/d3d10_private.h |   2 +-
>>  dlls/d3d10/effect.c        | 148 +++++++++++++++++++++++++++++++++----
>>  dlls/d3d10/tests/effect.c  | 122 ++++++++++++++++++++++++------
>>  3 files changed, 235 insertions(+), 37 deletions(-)
>> diff --git a/dlls/d3d10/tests/effect.c b/dlls/d3d10/tests/effect.c
>> index 9223fdf51ef..97331c2135e 100644
>> --- a/dlls/d3d10/tests/effect.c
>> +++ b/dlls/d3d10/tests/effect.c
>> @@ -6377,6 +6377,7 @@ static void test_effect_pool(void)
>>      D3D10_EFFECT_DESC desc;
>>      ID3D10Buffer *buffer;
>>      ULONG refcount;
>> +    IUnknown *unk;
>>      HRESULT hr;
>>      BOOL ret;
>>
>> @@ -6394,13 +6395,7 @@ todo_wine
>>      ok(hr == E_INVALIDARG, "Unexpected hr %#x.\n", hr);
>>
>>      hr = create_effect_pool(fx_test_pool, device, &pool);
>> -todo_wine
>>      ok(hr == S_OK, "Failed to create effect pool, hr %#x.\n", hr);
>> -    if (FAILED(hr))
>> -    {
>> -        ID3D10Device_Release(device);
>> -        return;
>> -    }
>>
>>      refcount = get_refcount(pool);
>>      ok(refcount == 1, "Unexpected refcount %u.\n", refcount);
>> @@ -6412,6 +6407,27 @@ todo_wine
>>      ok(refcount == 2, "Unexpected refcount %u.\n", refcount);
>>      effect->lpVtbl->Release(effect);
>>
>> +    hr = pool->lpVtbl->QueryInterface(pool, &IID_IUnknown, (void **)&unk);
>> +todo_wine
>> +    ok(hr == E_NOINTERFACE, "Unexpected hr %#x.\n", hr);
>> +    if (SUCCEEDED(hr)) IUnknown_Release(unk);
>> +
>> +    hr = pool->lpVtbl->QueryInterface(pool, &IID_ID3D10Effect, (void **)&unk);
>> +    ok(hr == E_NOINTERFACE, "Unexpected hr %#x.\n", hr);
>> +
>> +    hr = pool->lpVtbl->QueryInterface(pool, &IID_ID3D10EffectPool, (void **)&unk);
>> +    ok(hr == S_OK, "Unexpected hr %#x.\n", hr);
>> +    ok(unk == (IUnknown *)pool, "Unexpected pointer.\n");
>> +    IUnknown_Release(unk);
>> +
>> +    hr = effect->lpVtbl->QueryInterface(effect, &IID_IUnknown, (void **)&unk);
>> +todo_wine
>> +    ok(hr == E_NOINTERFACE, "Unexpected hr %#x.\n", hr);
>> +    if (SUCCEEDED(hr)) IUnknown_Release(unk);
>> +
>> +    hr = effect->lpVtbl->QueryInterface(effect, &IID_ID3D10Effect, (void **)&unk);
>> +    ok(hr == E_NOINTERFACE, "Unexpected hr %#x.\n", hr);
>> +
> I guess we could fix those two todo_wine by not accepting IID_IUnknown
> in d3d10_effect_pool_QueryInterface(). Any reason not to?
>
> Especially given the last test above, this isn't standard COM by any means...
Yes, it is entirely broken. I wasn't planning to break ours too, it's
just something that I found while testing.
>
>>      hr = effect->lpVtbl->QueryInterface(effect, &IID_ID3D10EffectPool, (void **)&pool2);
>>      ok(hr == S_OK, "Unexpected hr %#x.\n", hr);
>>      ok(pool2 == pool, "Unexpected pool pointer.\n");
>> @@ -6434,24 +6450,28 @@ todo_wine
>>      hr = cb->lpVtbl->GetDesc(cb, &var_desc);
>>      ok(hr == S_OK, "Unexpected hr %#x.\n", hr);
>>      ok(!strcmp(var_desc.Name, "s_cb"), "Unexpected name %s.\n", var_desc.Name);
>> +todo_wine
>>      ok(var_desc.Flags == D3D10_EFFECT_VARIABLE_POOLED, "Unexpected flags %#x.\n", var_desc.Flags);
>>
>>      v = effect->lpVtbl->GetVariableByName(effect, "s_blendstate");
>>      hr = v->lpVtbl->GetDesc(v, &var_desc);
>>      ok(hr == S_OK, "Unexpected hr %#x.\n", hr);
>>      ok(!strcmp(var_desc.Name, "s_blendstate"), "Unexpected name %s.\n", var_desc.Name);
>> +todo_wine
>>      ok(var_desc.Flags == D3D10_EFFECT_VARIABLE_POOLED, "Unexpected flags %#x.\n", var_desc.Flags);
>>
>>      v = effect->lpVtbl->GetVariableByName(effect, "s_texture");
>>      hr = v->lpVtbl->GetDesc(v, &var_desc);
>>      ok(hr == S_OK, "Unexpected hr %#x.\n", hr);
>>      ok(!strcmp(var_desc.Name, "s_texture"), "Unexpected name %s.\n", var_desc.Name);
>> +todo_wine
>>      ok(var_desc.Flags == D3D10_EFFECT_VARIABLE_POOLED, "Unexpected flags %#x.\n", var_desc.Flags);
>>
>>      v = effect->lpVtbl->GetVariableByName(effect, "ps");
>>      hr = v->lpVtbl->GetDesc(v, &var_desc);
>>      ok(hr == S_OK, "Unexpected hr %#x.\n", hr);
>>      ok(!strcmp(var_desc.Name, "ps"), "Unexpected name %s.\n", var_desc.Name);
>> +todo_wine
>>      ok(var_desc.Flags == D3D10_EFFECT_VARIABLE_POOLED, "Unexpected flags %#x.\n", var_desc.Flags);
>>
>>      t = effect->lpVtbl->GetTechniqueByIndex(effect, 0);
>> @@ -6466,11 +6486,31 @@ todo_wine
>>
>>      /* Create standalone effect from the same blob used for pool,  */
>>      hr = create_effect(fx_test_pool, D3D10_EFFECT_COMPILE_CHILD_EFFECT, device, NULL, &child_effect);
>> +todo_wine
>>      ok(hr == E_INVALIDARG, "Unexpected hr %#x.\n", hr);
>> +    if (SUCCEEDED(hr)) child_effect->lpVtbl->Release(child_effect);
>>
>>      hr = create_effect(fx_test_pool, 0, device, NULL, &effect2);
>>      ok(hr == S_OK, "Unexpected hr %#x.\n", hr);
>>
>> +    hr = effect2->lpVtbl->QueryInterface(effect2, &IID_IUnknown, (void **)&unk);
>> +todo_wine
>> +    ok(hr == E_NOINTERFACE, "Unexpected hr %#x.\n", hr);
>> +    if (SUCCEEDED(hr)) IUnknown_Release(unk);
>> +
>> +    hr = effect2->lpVtbl->QueryInterface(effect2, &IID_ID3D10Effect, (void **)&unk);
>> +todo_wine
>> +    ok(hr == E_NOINTERFACE, "Unexpected hr %#x.\n", hr);
>> +    if (SUCCEEDED(hr)) IUnknown_Release(unk);
>> +
>> +    /* For regular effects querying for ID3D10EffectPool is broken */
>> +    hr = effect2->lpVtbl->QueryInterface(effect2, &IID_ID3D10EffectPool, (void **)&unk);
>> +todo_wine {
>> +    ok(hr == S_OK, "Unexpected hr %#x.\n", hr);
>> +    ok(unk == (IUnknown *)effect2, "Unexpected pointer.\n");
>> +}
>> +    if (SUCCEEDED(hr)) IUnknown_Release(unk);
>> +
> I guess this one can remain todo_wine. Not that I see any trouble in
> just making d3d10_effect_QueryInterface() match this behavior.
> Somewhat relatedly, we should probably implement IsValid(), IsPool(),
> IsOptimized() sooner rather than later.
I agree about keeping it as todo, I was thinking doing the same for
broken QI cases above too, but if you want that in, sure.