Re: [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  wrote:
> > Signed-off-by: Nikolay Sivov
> >
> > 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.