[PATCH v2 3/3] d3d10+d3dcompiler: Move d3d10 reflection into d3dcompiler

[Matteo Bruni]

On Thu, Oct 24, 2019 at 9:50 PM Connor McAdams <conmanx360 at gmail.com> wrote:
>
> This patch moves the implementation of d3d10 shader reflection into the
> existing d3dcompiler reflection code, and uses ifdefs to make the
> interface methods conform to the size of d3d10 structures. In the case
> of the methods GetResourceBindingDesc and GetConstantBuffer, no changes
> are needed, as the data structures and interfaces are the same between
> d3d10 and d3d11.
>
> Signed-off-by: Connor McAdams <conmanx360 at gmail.com>
> ---
>  dlls/d3d10/d3d10_main.c          |  19 ++----
>  dlls/d3d10/d3d10_private.h       |   8 ---
>  dlls/d3d10/shader.c              | 112 -------------------------------
>  dlls/d3dcompiler_43/reflection.c | 102 ++++++++++++++++++++++------
>  include/d3dcompiler.h            |   4 ++
>  5 files changed, 89 insertions(+), 156 deletions(-)
>
> diff --git a/dlls/d3d10/d3d10_main.c b/dlls/d3d10/d3d10_main.c
> index e3d1c57e44..7c7500e045 100644
> --- a/dlls/d3d10/d3d10_main.c
> +++ b/dlls/d3d10/d3d10_main.c
> @@ -300,22 +300,11 @@ const char * WINAPI D3D10GetPixelShaderProfile(ID3D10Device *device)
>
>  HRESULT WINAPI D3D10ReflectShader(const void *data, SIZE_T data_size, ID3D10ShaderReflection **reflector)
>  {
> -    struct d3d10_shader_reflection *object;
> -
> -    FIXME("data %p, data_size %lu, reflector %p stub!\n", data, data_size, reflector);
> -
> -    if (!(object = heap_alloc_zero(sizeof(*object))))
> -    {
> -        ERR("Failed to allocate D3D10 shader reflection object memory\n");
> -        return E_OUTOFMEMORY;
> -    }
> -
> -    object->ID3D10ShaderReflection_iface.lpVtbl = &d3d10_shader_reflection_vtbl;
> -    object->refcount = 1;
> +    HRESULT hr;
>
> -    *reflector = &object->ID3D10ShaderReflection_iface;
> +    TRACE("data %p, data_size %lu, reflector %p stub!\n", data, data_size, reflector);
>
> -    TRACE("Created ID3D10ShaderReflection %p\n", object);
> +    hr = d3dcompiler_d3d10_reflection_init(data, data_size, (void **)reflector);

Hiding away the interface difference here is NOT okay. Also there is
no need to, you should have a d3dcompiler_shader_reflection_init()
function that takes a struct d3dcompiler_shader_reflection * and is
called by both D3D10ReflectShader and D3DReflect(). More on this
below.
Also it seems nicer to just move the function to d3dcompiler_43/reflection.c.

> diff --git a/dlls/d3dcompiler_43/reflection.c b/dlls/d3dcompiler_43/reflection.c
> index 5faea4c8e6..696b842dfc 100644
> --- a/dlls/d3dcompiler_43/reflection.c
> +++ b/dlls/d3dcompiler_43/reflection.c
> @@ -307,7 +307,7 @@ static void reflection_cleanup(struct d3dcompiler_shader_reflection *ref)
>
>  /* IUnknown methods */
>
> -static inline struct d3dcompiler_shader_reflection *impl_from_ID3D11ShaderReflection(ID3D11ShaderReflection *iface)
> +static inline struct d3dcompiler_shader_reflection *impl_from_ID3DShaderReflection(ID3D11ShaderReflection *iface)
>  {
>      return CONTAINING_RECORD(iface, struct d3dcompiler_shader_reflection, ID3DShaderReflection_iface);
>  }

I don't like this. There is no reason to change the function name and
I'd still use the full name for the interface field in the struct
definition (as mentioned in the previous patch).

> @@ -315,9 +315,13 @@ static inline struct d3dcompiler_shader_reflection *impl_from_ID3D11ShaderReflec
>  static HRESULT STDMETHODCALLTYPE d3dcompiler_shader_reflection_QueryInterface(ID3D11ShaderReflection *iface, REFIID riid, void **object)
>  {
>      TRACE("iface %p, riid %s, object %p\n", iface, debugstr_guid(riid), object);
> -
> +#ifndef D3D10REFLECT
>      if (IsEqualGUID(riid, &IID_ID3D11ShaderReflection)
>              || IsEqualGUID(riid, &IID_IUnknown))
> +#else
> +    if (IsEqualGUID(riid, &IID_ID3D10ShaderReflection)
> +            || IsEqualGUID(riid, &IID_IUnknown))
> +#endif
>      {
>          IUnknown_AddRef(iface);
>          *object = iface;

I had mentioned it on IRC but it was probably missed in the midst of
the discussion: in general I'd prefer full "duplicate" functions
rather than this kind of #ifdefing inside functions. This also doesn't
work when exposing proper vtables.

> @@ -398,6 +402,7 @@ static HRESULT STDMETHODCALLTYPE d3dcompiler_shader_reflection_GetDesc(ID3D11Sha
>      desc->EmitInstructionCount = This->emit_instruction_count;
>      desc->GSOutputTopology = This->gs_output_topology;
>      desc->GSMaxOutputVertexCount = This->gs_max_output_vertex_count;
> +#ifndef D3D10REFLECT
>      desc->InputPrimitive = This->input_primitive;
>      desc->PatchConstantParameters = This->pcsg ? This->pcsg->element_count : 0;
>      desc->cGSInstanceCount = 0;
> @@ -408,14 +413,14 @@ static HRESULT STDMETHODCALLTYPE d3dcompiler_shader_reflection_GetDesc(ID3D11Sha
>      desc->cBarrierInstructions = 0;
>      desc->cInterlockedInstructions = 0;
>      desc->cTextureStoreInstructions = 0;
> -
> +#endif
>      return S_OK;
>  }

Taking this function as an example, what I had in mind was to
effectively change this function to
d3dcompiler_shader_reflection_get_desc(struct
d3dcompiler_shader_reflection *, ...) and have separate
ID3D10ShaderReflection / ID3D11ShaderReflection methods that simply
recover the object via impl_from_*() and call into the new helper.
Most of the methods are trivial and it should be fine to simply
duplicate them (i.e. without need of a common helper).

>  static struct ID3D11ShaderReflectionConstantBuffer * STDMETHODCALLTYPE d3dcompiler_shader_reflection_G
> @@ -489,7 +494,11 @@ static HRESULT STDMETHODCALLTYPE d3dcompiler_shader_reflection_GetInputParameter
>          return E_INVALIDARG;
>      }
>
> +#ifndef D3D10REFLECT
>      *desc = This->isgn->elements[index];
> +#else
> +    memcpy(desc, &This->isgn->elements[index], sizeof(D3D10_SIGNATURE_PARAMETER_DESC));
> +#endif

With separate implementations you can write this as
"...sizeof(*desc));" (and you won't need an #ifdef).

> +    object->ID3DShaderReflection_iface.lpVtbl = (const struct ID3D10ShaderReflectionVtbl *)&d3dcompiler_shader_reflection_vtbl;

This is not okay. You have to have a proper vtbl for
ID3D10ShaderReflection, with correct method signatures.

> diff --git a/include/d3dcompiler.h b/include/d3dcompiler.h
> index 5151f94510..c602d05e76 100644
> --- a/include/d3dcompiler.h
> +++ b/include/d3dcompiler.h
> @@ -148,6 +148,10 @@ typedef HRESULT (WINAPI *pD3DPreprocess)(const void *data, SIZE_T size, const ch
>
>  HRESULT WINAPI D3DLoadModule(const void *data, SIZE_T size, ID3D11Module **module);
>
> +#ifdef D3D10REFLECT
> +HRESULT d3dcompiler_d3d10_reflection_init(const void *data, SIZE_T data_size, void **reflector);
> +#endif

You can't add Wine-only functions to public headers. This won't be
necessary once you move D3D10ReflectShader() to reflection.c.

In general, I was expecting this to be structured differently, like I
tried to explain here. Let me know if I haven't been clear enough.