Re: [PATCH 2/5] d3dx9_36: Improved constant table handle support (try 3)
On Mon, Jul 18, 2011 at 2:41 AM, Alexandre Julliard wrote:
> Travis Athougies writes:
>
>> +static ctab_constant *is_valid_constant(ID3DXConstantTableImpl *This,
>> D3DXHANDLE parameter)
>> +{
>> + UINT i;
>> +
>> + for (i = 0; i < This->desc.Constants; i++)
>> + if ((ctab_constant *)parameter == &This->constants[i])
>> + return (ctab_constant *)parameter;
>
> This doesn't look like an improvement.
>
While I understand that this is not the most efficient way to do
things, it is the same method used in the effects framework (see
is_valid_parameter in dlls/d3dx9_36/effect.c). Currently, in the
constant table implementation, we distinguish D3DXHANDLE's from char
pointers by forcing D3DXHANDLE's to be less than 65536, which is
rather arbitrary, and could be wrong. This, on the other hand, is
accurate, even though it may not necessarily offer the best
performance.
Native D3DX distinguishes D3DXHANDLEs from char pointers by negating
the address of all D3DXHANDLE's. Thus, if the most significant bit is
set, we know we are dealing with a D3DXHANDLE. Unfortunately, this
assumes that userspace ends at 2gb, which is not necessarily the case
on linux, so we can't make this assumption. Thus, we need a way to
distinguish d3dxhandles from char pointers, without using this trick.
Below, I've listed some ideas I've had. Which one(s) would be acceptable to you?
1. Using magic numbers to determine if a pointer is a D3DXHANDLE
2. Using a hash table to implement a set of D3DXHANDLEs and then
checking to see if a pointer is in this set
3. Allocating all handles in a contiguous block of memory and then
using range checking. (This could slow down loading somewhat, since
you might have to do a lot of HeapReAlloc's)
One final thing to note is that D3DXHANDLEs are not constant table
specific. If I get a D3DXHANDLE from constant table A, I can still use
it to set constants in constant table B. In this case, constant table
B will forward the call to constant table A. However, I don't think
any game makes use of this fact.
Travis.
> --
> Alexandre Julliard
> julli...@winehq.org
>
--
Travis Athougies
Fwd: [PATCH 2/5] d3dx9_36: Implemented ID3DXConstantTable_SetIntArray and ID3DXConstantTable_SetInt (Try 5)
>> Ints and floats are represented differently and so, if we want to put >> an integer array into floating point registers, we need to have casts. >> If you were to simply call SetFloatArray, you would get nonsense >> values in the floating point registers. > > Even using casts will fall foul of the C aliasing rules - which > mean that the compiler can assume that a read of type will not be > modified by a write to a different type so can be reorderd (unless > one of the types is 'char'). > Exactly. Which is why I am not ever casting int pointers to float pointers as Rico suggested. Instead I'm looping through the array and casting each int to a float, which should invoke the machinery necessary to convert from the bit representation of an int to that of a float. > Not only that, but there are systems that will fault is an invalid > FP bit pattern is loaded into an FP register. > If I'm casting ints to floats (and not int pointers to float pointers) I don't see how this could be an issue. Travis -- Travis Athougies
Re: [PATCH 2/5] d3dx9_36: Implemented ID3DXConstantTable_SetIntArray and ID3DXConstantTable_SetInt (Try 5)
2011/3/28 Rico Schüller :
> Hi,
>
> this and the previous patch (1/5) are doing the same thing? Is there a
> reason why they could not use the same code base?
>
> e.g.
> ID3DXConstantTable_SetIntArray(iface, device, constant, n, count)
> {
> return ID3DXConstantTable_SetFloatArray(iface, device, constant, n,
> count);
> }
Ints and floats are represented differently and so, if we want to put
an integer array into floating point registers, we need to have casts.
If you were to simply call SetFloatArray, you would get nonsense
values in the floating point registers.
>
> Also I'd prefer if there would be TRACEs in each function.
>
> It might be useful to add the RegisterSet in the FIXME for the default case,
> to see which one is missing in the special case.
Okay.
Travis.
>
> Cheers
> Rico
>
> Am 27.03.2011 22:22, schrieb Travis Athougies:
>>
>> ---
>> dlls/d3dx9_36/shader.c | 37 ++---
>> 1 files changed, 30 insertions(+), 7 deletions(-)
>>
>> diff --git a/dlls/d3dx9_36/shader.c b/dlls/d3dx9_36/shader.c
>> index 4a3caab..874d916 100644
>> --- a/dlls/d3dx9_36/shader.c
>> +++ b/dlls/d3dx9_36/shader.c
>> @@ -847,11 +847,7 @@ static HRESULT WINAPI
>> ID3DXConstantTableImpl_SetBoolArray(ID3DXConstantTable* if
>>
>> static HRESULT WINAPI ID3DXConstantTableImpl_SetInt(ID3DXConstantTable*
>> iface, LPDIRECT3DDEVICE9 device, D3DXHANDLE constant, INT n)
>> {
>> - ID3DXConstantTableImpl *This = impl_from_ID3DXConstantTable(iface);
>> -
>> - FIXME("(%p)->(%p, %p, %d): stub\n", This, device, constant, n);
>> -
>> - return E_NOTIMPL;
>> + return ID3DXConstantTable_SetIntArray(iface, device, constant,&n, 1);
>> }
>>
>> static HRESULT WINAPI
>> ID3DXConstantTableImpl_SetIntArray(ID3DXConstantTable* iface,
>> LPDIRECT3DDEVICE9 device,
>> @@ -859,9 +855,36 @@ static HRESULT WINAPI
>> ID3DXConstantTableImpl_SetIntArray(ID3DXConstantTable* ifa
>> {
>> ID3DXConstantTableImpl *This = impl_from_ID3DXConstantTable(iface);
>>
>> - FIXME("(%p)->(%p, %p, %p, %d): stub\n", This, device, constant, n,
>> count);
>> + D3DXCONSTANT_DESC desc;
>> + HRESULT hr;
>> + UINT i, desc_count = 1;
>> + float row[4] = {0.0f, 0.0f, 0.0f, 0.0f};
>> +
>> + hr = ID3DXConstantTable_GetConstantDesc(iface,
>> constant,&desc,&desc_count);
>> + if (FAILED(hr))
>> + {
>> + TRACE("ID3DXConstantTable_GetConstantDesc failed: %08x", hr);
>> + return D3DERR_INVALIDCALL;
>> + }
>>
>> - return E_NOTIMPL;
>> + switch (desc.RegisterSet)
>> + {
>> + case D3DXRS_FLOAT4:
>> + for (i = 0; i< count&& i< desc.RegisterCount; i++)
>> + {
>> + row[0] = n[i];
>> + if (is_vertex_shader(This->desc.Version))
>> + IDirect3DDevice9_SetVertexShaderConstantF(device,
>> desc.RegisterIndex + i, row, 1);
>> + else
>> + IDirect3DDevice9_SetPixelShaderConstantF(device,
>> desc.RegisterIndex + i, row, 1);
>> + }
>> + break;
>> + default:
>> + FIXME("Can't handle RegisterSets other than D3DXRS_FlOAT4\n");
>> + return E_NOTIMPL;
>> + }
>> +
>> + return D3D_OK;
>> }
>>
>> static HRESULT WINAPI ID3DXConstantTableImpl_SetFloat(ID3DXConstantTable*
>> iface, LPDIRECT3DDEVICE9 device,
>
>
--
Travis Athougies
Re: [PATCH 2/5] d3dx9_36: Implemented ID3DXConstantTable_SetIntArray and ID3DXConstantTable_SetInt
On Feb 8, 2011, at 4:10 AM, Matteo Bruni
wrote:
+ /* We need the for loop, since we need to convert the
integer to a float */
+for (i = 0; i < count && i < desc.RegisterCount; i++)
+{
+row[0] = (float)n[i];
The cast is unneeded, and the comment above is too.
If it's being passed into set*shaderconstantf, shouldn't it need to be
cast to a float? Or am I mishnderstaning you?
Travis
Re: [PATCH 5/5] d3dx9_36/tests: Added tests for ID3DXConstantTable_Set* functions
Regardless, I've modified the tests a so that they should pass on
windows. Can you verify that they do?
Travis.
On Fri, Jan 21, 2011 at 9:22 PM, Henri Verbeet wrote:
> On 22 January 2011 06:11, Travis Athougies wrote:
>> The failed tests worry me since it passes on linux with a native
>> d3dx9_36 dll. Have you tried a native d3dx9_36 with wine? Do the tests
>> still fail?
>>
> That's not a valid way to verify tests.
>
--
Travis Athougies
From 81cd52cea8e0f9b9b669856046b369a57249006b Mon Sep 17 00:00:00 2001
From: Travis Athougies
Date: Wed, 19 Jan 2011 16:00:28 -0800
Subject: [PATCH 5/5] d3dx9_36/tests: Added tests for ID3DXConstantTable_Set* functions
---
dlls/d3dx9_36/tests/shader.c | 219 +-
1 files changed, 217 insertions(+), 2 deletions(-)
diff --git a/dlls/d3dx9_36/tests/shader.c b/dlls/d3dx9_36/tests/shader.c
index a4bbc22..d075740 100644
--- a/dlls/d3dx9_36/tests/shader.c
+++ b/dlls/d3dx9_36/tests/shader.c
@@ -142,7 +142,7 @@ static const DWORD ctab_arrays[] = { /* 0123456789ABCDEF */
0xabab0079, 0x00010001, 0x00030001, 0x0003, /* y... */
0x, 0x72726166, 0xab007961, 0x0003, /* farray.. */
0x00010001, 0x0004, 0x, 0x78746d66, /* fmtx */
-0x61727261, 0xabab0079, 0x00030003, 0x00040004, /* array... */
+0x61727261, 0xabab0079, 0x00030002, 0x00040004, /* array... */
0x0002, 0x, 0x63657666, 0x61727261, /* fvecarra */
0xabab0079, 0x00030001, 0x00040001, 0x0002, /* y... */
0x, 0x63657669, 0x61727261, 0xabab0079, /* ivecarray... */
@@ -153,7 +153,7 @@ static const DWORD ctab_arrays[] = { /* 0123456789ABCDEF */
0x332e3235, 0x00313131, 0x}; /* 52.3111. END*/
static const D3DXCONSTANT_DESC ctab_arrays_expected[] = {
-{"fmtxarray", D3DXRS_FLOAT4, 0, 8, D3DXPC_MATRIX_COLUMNS, D3DXPT_FLOAT, 4, 4, 2, 0, 128, 0},
+{"fmtxarray", D3DXRS_FLOAT4, 0, 8, D3DXPC_MATRIX_ROWS, D3DXPT_FLOAT, 4, 4, 2, 0, 128, 0},
{"farray", D3DXRS_FLOAT4, 8, 4, D3DXPC_SCALAR, D3DXPT_FLOAT, 1, 1, 4, 0, 16, 0},
{"fvecarray", D3DXRS_FLOAT4, 12, 2, D3DXPC_VECTOR, D3DXPT_FLOAT, 1, 4, 2, 0, 32, 0},
{"barray", D3DXRS_FLOAT4, 14, 2, D3DXPC_SCALAR, D3DXPT_BOOL, 1, 1, 2, 0, 8, 0},
@@ -423,6 +423,220 @@ static void test_constant_tables(void)
sizeof(ctab_arrays_expected)/sizeof(*ctab_arrays_expected));
}
+static void test_setting_basic_table(IDirect3DDevice9 *device)
+{
+static const D3DXMATRIX mvp = {
+0.514f, 0.626f, 0.804f, 0.786f,
+0.238f, 0.956f, 0.374f, 0.483f,
+0.109f, 0.586f, 0.900f, 0.255f,
+0.898f, 0.411f, 0.932f, 0.275f};
+static const D3DXVECTOR4 f4 = {0.350f, 0.526f, 0.925f, 0.021f};
+static const float f = 0.12543f;
+static const int i = 321;
+
+ID3DXConstantTable *ctable;
+
+HRESULT res;
+float out[16];
+
+/* Get the constant table from the shader itself */
+res = D3DXGetShaderConstantTable(ctab_basic, &ctable);
+ok(res == D3D_OK, "D3DXGetShaderConstantTable failed: got 0x%08x\n", res);
+
+/* Set constants */
+res = ID3DXConstantTable_SetMatrix(ctable, device, "mvp", &mvp);
+ok(res == D3D_OK, "ID3DXConstantTable_SetMatrix failed on variable mvp: got 0x%08x\n", res);
+
+res = ID3DXConstantTable_SetInt(ctable, device, "i", i);
+ok(res == D3D_OK, "ID3DXConstantTable_SetInt failed on variable i: got 0x%08x\n", res);
+
+res = ID3DXConstantTable_SetFloat(ctable, device, "f", f);
+ok(res == D3D_OK, "ID3DXConstantTable_SetFloat failed on variable f: got 0x%08x\n", res);
+
+res = ID3DXConstantTable_SetVector(ctable, device, "f4", &f4);
+ok(res == D3D_OK, "ID3DXConstantTable_SetVector failed on variable f4: got 0x%08x\n", res);
+
+/* Get constants back and validate */
+IDirect3DDevice9_GetVertexShaderConstantF(device, 0, out, 4);
+ok(out[0] == mvp._11 && out[4] == mvp._12 && out[8] == mvp._13 && out[12] == mvp._14,
+"The first row of mvp was not set correctly, got {%f, %f, %f, %f}, should be {%f, %f, %f, %f}\n",
+out[0], out[4], out[8], out[12], mvp._11, mvp._12, mvp._13, mvp._14);
+ok(out[1] == mvp._21 && out[5] == mvp._22 && out[9] == mvp._23 && out[13] == mvp._24,
+"The second row of mvp was not set correctly, got {%f, %f, %f, %f}, should be {%f, %f, %f, %f}\n",
+out[1], out[5], out[9], out[13], mvp._21, mvp._22, mvp._23, mvp._24);
+ok(out[2] == mvp._31 && out[6] == mvp._32 && out[10] == mvp._33 && out[14] == mvp._34,
+"The third row of mvp was not
Re: [PATCH 5/5] d3dx9_36/tests: Added tests for ID3DXConstantTable_Set* functions
The failed tests worry me since it passes on linux with a native
d3dx9_36 dll. Have you tried a native d3dx9_36 with wine? Do the tests
still fail?
Travis.
On Thu, Jan 20, 2011 at 8:48 AM, Matteo Bruni wrote:
> 2011/1/20 Travis Athougies :
>> +static void test_setting_basic_table(IDirect3DDevice9 *device)
>> +{
>> + static const D3DXMATRIX mvp = {
>> + 0.514f, 0.626f, 0.804f, 0.786f,
>> + 0.238f, 0.956f, 0.374f, 0.483f,
>> + 0.109f, 0.586f, 0.900f, 0.255f,
>> + 0.898f, 0.411f, 0.932f, 0.275f};
>
> This throws some warnings here. Apparently you have to put some
> additional braces around that initializer.
>
> + ok(out[0] == (float)iarray[0] && out[4] == (float)iarray[1] &&
> out[8] == (float)iarray[2] &&
> + out[12] == (float)iarray[3], "SetIntArray did not
> properly set a float array: out={%f, %f, %f, %f}, should"
> + " be {%d, %d, %d, %d}\n", out[0], out[4], out[8],
> out[12], iarray[0], iarray[1], iarray[2], iarray[3]);
>
> The float casts are unneeded.
>
> The tests look generally OK to me otherwise, albeit they are somewhat
> wordy. Oh, please notice that some tests fail on Windows. The testbot
> can't create a d3d device so it skips these tests, but on my Win7 box
> I get this:
>
> shader.c:462: Test failed: The first row of mvp was not set correctly, got
> {0.51
> 4000, 0.238000, 0.109000, 0.898000}, should be {0.514000, 0.626000, 0.804000,
> 0.
> 786000}
> shader.c:465: Test failed: The second row of mvp was not set correctly, got
> {0.6
> 26000, 0.956000, 0.586000, 0.411000}, should be {0.238000, 0.956000,
> 0.374000, 0
> .483000}
> shader.c:468: Test failed: The third row of mvp was not set correctly, got
> {0.80
> 4000, 0.374000, 0.90, 0.932000}, should be {0.109000, 0.586000, 0.90,
> 0.
> 255000}
> shader.c:471: Test failed: The fourth row of mvp was not set correctly, got
> {0.7
> 86000, 0.483000, 0.255000, 0.275000}, should be {0.898000, 0.411000,
> 0.932000, 0
> .275000}
> shader.c:530: Test failed: The excess elements of the array were not set
> correct
> ly, out={0.00, 0.00, 0.00, 0.00}, should be {0.01,
> 0.02,
> 0.03, 0.04}
> shader: 248 tests executed (0 marked as todo, 5 failures), 0 skipped.
>
> The matrix seems to have its rows and columns switched (probably this
> has to do with row-major vs column-major matrix storage -
> http://msdn.microsoft.com/en-us/library/bb509706%28v=VS.85%29.aspx).
> So you have to fix your SetMatrixArray implementation to account for
> that (and update the tests accordingly). The other failing test shows
> that actually the excess elements are not set, apparently.
>
> A last thing. I noticed that you never set the vertex shader in the
> d3d9 device. I'd say: don't change that. This shows that
> ID3DXConstantTable doesn't pay attention to the vertex shader
> currently set and it is responsibility of the application programmer
> to call these functions with the correct shader program.
>
--
Travis Athougies
Re: [PATCH 4/5] d3dx9_36: Implemented ID3DXConstantTable_SetMatrix and ID3DXConstantTable_SetMatrixArray
Oh I'm sorry. I now see what you mean, you were talking about the register index parameter to Set*ShaderConstantF? Yes, that should be changed. I will submit updated patches shortly. Travis. On Fri, Jan 21, 2011 at 3:22 PM, Travis Athougies wrote: > matrix + i will advance by the size of 1 D3DXMATRIX, since matrix is a > pointer to a D3DXMATRIX. Michael is right, I thought the code was > clearer with the pointer arithmetic, but I now see more people are > familiar with the array indexing style. > > Travis. > > On Fri, Jan 21, 2011 at 8:47 AM, Matteo Bruni > wrote: >> 2011/1/21 Michael Stefaniuc : >>> On 01/21/2011 06:56 AM, Travis Athougies wrote: >>>> On Thu, Jan 20, 2011 at 7:27 AM, Matteo Bruni >>>> wrote: >>>>> >>>>> 2011/1/20 Travis Athougies: >>>>>> >>>>>> + /* D3DXMATRIX is a union, one of whose elements is an >>>>>> array, so it can be cast to a float pointer */ >>>>>> + if (is_vertex_shader(This->desc.Version)) >>>>>> + IDirect3DDevice9_SetVertexShaderConstantF(device, >>>>>> desc.RegisterIndex + i, (float *)(matrix + i), >>>>>> + desc.RegisterCount); >>>>>> + else >>>>>> + IDirect3DDevice9_SetPixelShaderConstantF(device, >>>>>> desc.RegisterIndex + i, (float *)(matrix + i), >>>>>> + desc.RegisterCount); >>>>> >>>>> Can't you just pass matrix->m[i] as parameter, instead of that cast? >>>>> >>>> >>>> Uh no. If I were to do that, matrix->m[1] would be the second row, not >>>> the second matrix. I'm trying to get at the second matrix. To >>>> illustrate this, suppose the matrix were at 0x8000 (not going to >>>> happen, but just pretend). matrix->m[1] would be at 0x8010, since >>>> floats are 4 bytes. However, the next matrix is actually at 0x8040 >>>> (since sizeof(float) * 16 [the number of elements in the matrix] = >>>> 64). >>> >>> So you want a pointer to the first element of the "i"th matrix, right? >>> &matrix[i]._11 >>> No cast, no explicit pointer arithmetic. >>> >> >> Oh, right, I misread that (and Michael's suggestion is good). But then >> I would expect the register index to be incremented by >> desc.RegisterCount at each iteration. RegisterCount is supposedly 4, >> but what if it is not? This calls for some SetMatrixArray tests. >> > > > > -- > Travis Athougies > -- Travis Athougies
Re: [PATCH 4/5] d3dx9_36: Implemented ID3DXConstantTable_SetMatrix and ID3DXConstantTable_SetMatrixArray
matrix + i will advance by the size of 1 D3DXMATRIX, since matrix is a pointer to a D3DXMATRIX. Michael is right, I thought the code was clearer with the pointer arithmetic, but I now see more people are familiar with the array indexing style. Travis. On Fri, Jan 21, 2011 at 8:47 AM, Matteo Bruni wrote: > 2011/1/21 Michael Stefaniuc : >> On 01/21/2011 06:56 AM, Travis Athougies wrote: >>> On Thu, Jan 20, 2011 at 7:27 AM, Matteo Bruni >>> wrote: >>>> >>>> 2011/1/20 Travis Athougies: >>>>> >>>>> + /* D3DXMATRIX is a union, one of whose elements is an >>>>> array, so it can be cast to a float pointer */ >>>>> + if (is_vertex_shader(This->desc.Version)) >>>>> + IDirect3DDevice9_SetVertexShaderConstantF(device, >>>>> desc.RegisterIndex + i, (float *)(matrix + i), >>>>> + desc.RegisterCount); >>>>> + else >>>>> + IDirect3DDevice9_SetPixelShaderConstantF(device, >>>>> desc.RegisterIndex + i, (float *)(matrix + i), >>>>> + desc.RegisterCount); >>>> >>>> Can't you just pass matrix->m[i] as parameter, instead of that cast? >>>> >>> >>> Uh no. If I were to do that, matrix->m[1] would be the second row, not >>> the second matrix. I'm trying to get at the second matrix. To >>> illustrate this, suppose the matrix were at 0x8000 (not going to >>> happen, but just pretend). matrix->m[1] would be at 0x8010, since >>> floats are 4 bytes. However, the next matrix is actually at 0x8040 >>> (since sizeof(float) * 16 [the number of elements in the matrix] = >>> 64). >> >> So you want a pointer to the first element of the "i"th matrix, right? >> &matrix[i]._11 >> No cast, no explicit pointer arithmetic. >> > > Oh, right, I misread that (and Michael's suggestion is good). But then > I would expect the register index to be incremented by > desc.RegisterCount at each iteration. RegisterCount is supposedly 4, > but what if it is not? This calls for some SetMatrixArray tests. > -- Travis Athougies
Re: [PATCH 4/5] d3dx9_36: Implemented ID3DXConstantTable_SetMatrix and ID3DXConstantTable_SetMatrixArray
Uh no. If I were to do that, matrix->m[1] would be the second row, not the second matrix. I'm trying to get at the second matrix. To illustrate this, suppose the matrix were at 0x8000 (not going to happen, but just pretend). matrix->m[1] would be at 0x8010, since floats are 4 bytes. However, the next matrix is actually at 0x8040 (since sizeof(float) * 16 [the number of elements in the matrix] = 64). Travis. On Thu, Jan 20, 2011 at 7:27 AM, Matteo Bruni wrote: > 2011/1/20 Travis Athougies : >> + /* D3DXMATRIX is a union, one of whose elements is an array, so >> it can be cast to a float pointer */ >> + if (is_vertex_shader(This->desc.Version)) >> + IDirect3DDevice9_SetVertexShaderConstantF(device, >> desc.RegisterIndex + i, (float *)(matrix + i), >> + desc.RegisterCount); >> + else >> + IDirect3DDevice9_SetPixelShaderConstantF(device, >> desc.RegisterIndex + i, (float *)(matrix + i), >> + desc.RegisterCount); > > Can't you just pass matrix->m[i] as parameter, instead of that cast? > -- Travis Athougies
Testbot not accepting patch
Hi there, I've been trying to submit this patch to the testbot, and it keeps refusing it with the error "Unrecognized file typea" (that's not a typo, that's the exact error). The testbot has also refused to accept the patch when I sent it in on wine-patches (the error on the patch status page is apply failure). As far as I can tell, the patch looks good, it was generated by git format-patch from the latest git and I did not touch it after that, so it should apply, but it doesn't. Can you look into this? Thanks, Travis. -- Travis Athougies 0001-d3dx9_36-Implemented-ID3DXConstantTableImpl_SetFloat.patch Description: Binary data
Re: [PATCH] d3dcompiler_43/tests: Added error tests to HLSL test suite
To address, the C string literals issue, this is the same format I
have used for the other test strings. It would seem inconsistent to
break this now. Maybe a later patch can address this.
I've used IUnknown_Release in other places to free ID3D10Blobs, so it
also seems inconsistent to use ID3D10Blob_Release here. However, I did
not cast it those other times, so I will remove the casts in my next
attempt
And yes, compiled is NULL if the compile failed.
Travis.
2010/12/23 Rico Schüller :
> Am 23.12.2010 00:37, schrieb Michael Stefaniuc:
>>
>> Hello Travis,
>>
>> On 12/22/2010 10:31 PM, Travis Athougies wrote:
>>>
>>> Tests to ensure the HLSL compiler won't crash on malformed input.
>>>
>>> ---
>>> dlls/d3dcompiler_43/tests/hlsl.c | 87
>>> ++
>>> 1 files changed, 87 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/dlls/d3dcompiler_43/tests/hlsl.c
>>> b/dlls/d3dcompiler_43/tests/hlsl.c
>>> index 1f8e31c..a6a6099 100644
>>> --- a/dlls/d3dcompiler_43/tests/hlsl.c
>>> +++ b/dlls/d3dcompiler_43/tests/hlsl.c
>>> @@ -556,6 +556,92 @@ static void test_trig(IDirect3DDevice9 *device,
>>> IDirect3DVertexBuffer9 *quad_geo
>>> }
>>> }
>>>
>>> +static void test_fail(IDirect3DDevice9 *device, IDirect3DVertexBuffer9
>>> *qquad_geometry,
>>> + IDirect3DVertexShader9 *vshader_passthru)
>>> +{
>>> + static const char *undefined_variable_shader =
>>> + "float4 test(float2 pos: TEXCOORD0) : COLOR \
>>> + { \
>>> + return y; \
>>> + }";
>>
>> please use proper C string literals. Escaping the newline is a gcc
>> extension. You can use string literal concatenation to split the string
>> on multiple lines. Something like this would do:
>> "float4 test(float2 pos: TEXCOORD0) : COLOR"
>> "{"
>> " return y;"
>> "}";
>>
>> bye
>> michael
>>
>>
>>
> You probably may use ID3D10Blob_Release() instead of IUnknown_Release().
> This way you could drop the cast.
>
> What happens to the ID3D10Blob *compiled? Is it still NULL after each call
> to D3DCompile or is it an valid blob which has to be released?
>
> Cheers
> Rico
>
--
Travis Athougies
Re: [PATCH 5/5] d3dcompiler_43/tests: Added trigonometric function tests to HLSL test suite
It definitely could. I just thought that would result in a lot of ugly probe array definitions. Nevertheless, if you think it would be cleaner, I could definitely migrate it over, without too much effort. Travis. On Tue, Nov 2, 2010 at 4:18 AM, Henri Verbeet wrote: > On 2 November 2010 07:20, Travis Athougies wrote: >> +/* Repeatedly call a shader 32 times with linearly varying values and test >> the results against >> + an array of floats using the given epsilon */ > Why can't this just use the existing framework? > > > -- Travis Athougies
Re: [PATCH 1/5] d3dcompiler_43/tests: Added HLSL test suite
On Mon, Sep 27, 2010 at 3:57 AM, Henri Verbeet wrote: > On 27 September 2010 02:22, Travis Athougies wrote: >>>> + /* The Direct3D 9 docs state that we cannot lock a render target >>>> surface, >>>> + instead we must copy the render target onto this surface to lock >>>> it */ >>> I think you can, if you create it with D3DUSAGE_DYNAMIC | >>> D3DUSAGE_RENDERTARGET. If you want the backbuffer to be lockable you'll >>> need some device creation flag whose name I forgot. >> >> There is the lockable boolean argument that I think might work. I will >> look into using this >> > Please don't. I think GetRenderTargetData() is the correct way to do > this. Generally you don't want lockable render targets at all, but the > case where they're "useful" is for writing data to the render target, > not reading. This is what I read somewhere, some time ago, so I'll just keep it the same. > > The problem with your current setup is that you do the readback after > doing a Present(). Present() invalidates the contents of the > backbuffer. > >> + ok(data[0] == D3DCOLOR_ARGB(0, 0, 255, 255), >> + "swizzle_test: Got color %08x (should be 0x)\n", >> data[0]); > While I doubt you're going to care a lot for this specific test, I > think it makes sense to integrate something like color_match() from > the d3d9 tests right from the start. In fact, I think the way you want > to do this is to keep the actual testing inside > compile_pixel_shader9(). E.g., you could pass it an array with things > like probe locations, expected values, allowed deviations and failure > messages. I'm wondering... Inside compile_pixel_shader9? or compute_pixel_shader9? I'm thinking that this might make the argument list of compile_pixel_shader9 really really ugly. I'm unsure of exactly what you're telling me here? Could you be more explicit? > >> + if (caps.PixelShaderVersion >= 0x0200) > You can just use D3DPS_VERSION(2, 0) here. > Uhmm Didn't know that.. thanks! -- Travis Athougies
Re: [PATCH 1/5] d3dcompiler_43/tests: Added HLSL test suite
On Sun, Sep 26, 2010 at 12:41 PM, Stefan Dösinger wrote: > > Am 26.09.2010 um 19:51 schrieb Travis Athougies: > >> +data = compute_shader_fullscreen9(device, vshader_passthru, >> pshader, quad_geometry, >> +D3DFMT_A8R8G8B8, 1, 1); >> + >> +ok(data[0] == D3DCOLOR_ARGB(0, 0, 255, 255), >> +"swizzle_test: Got color %08x (should be 0x)\n", >> data[0]); > You leak the returned data here. > Ok woops. Can't believe I missed that > On the big picture of this, I don't like the void * blob returned from > compute_shader_fullscreen. As far as I can see the only position dependent > aspect of HLSL is vpos(or rather, the HLSL equivalent). For all others it > would be OK to return a ARGB color. > The reason why I don't return just one value here is (a) because the data might either be in DWORD or float format and (b) some tests use more than just 1 pixel, so they need access to all their pixels. >> +/* The Direct3D 9 docs state that we cannot lock a render target >> surface, >> + instead we must copy the render target onto this surface to lock it >> */ > I think you can, if you create it with D3DUSAGE_DYNAMIC | > D3DUSAGE_RENDERTARGET. If you want the backbuffer to be lockable you'll need > some device creation flag whose name I forgot. There is the lockable boolean argument that I think might work. I will look into using this On Sun, Sep 26, 2010 at 12:41 PM, Stefan Dösinger wrote: > > Am 26.09.2010 um 19:51 schrieb Travis Athougies: > >> + data = compute_shader_fullscreen9(device, vshader_passthru, >> pshader, quad_geometry, >> + D3DFMT_A8R8G8B8, 1, 1); >> + >> + ok(data[0] == D3DCOLOR_ARGB(0, 0, 255, 255), >> + "swizzle_test: Got color %08x (should be 0x)\n", >> data[0]); > You leak the returned data here. > > On the big picture of this, I don't like the void * blob returned from > compute_shader_fullscreen. As far as I can see the only position dependent > aspect of HLSL is vpos(or rather, the HLSL equivalent). For all others it > would be OK to return a ARGB color. > >> + /* The Direct3D 9 docs state that we cannot lock a render target >> surface, >> + instead we must copy the render target onto this surface to lock it >> */ > I think you can, if you create it with D3DUSAGE_DYNAMIC | > D3DUSAGE_RENDERTARGET. If you want the backbuffer to be lockable you'll need > some device creation flag whose name I forgot. > > > > -- Travis Athougies
Re: Wine test bot failure?
Still, I would like for it to be able to be buillt.. On Fri, Sep 24, 2010 at 1:20 AM, Austin English wrote: > On Fri, Sep 24, 2010 at 12:30 AM, Travis Athougies wrote: >> Hi, >> >> I was trying to run my HLSL test suite patch through wine test bot and >> the build failed. However, it's not a compilation failure. Instead it >> complains about not being able to copy some executable file. Also, the >> build fails almost instantaneously. It also complains about a missing >> command. > > Keep in mind that these tests are running in a virtual machine, so any > graphics tests may be skipped/broken/fail in unexpected ways, compared > to 'real' hardware. > > -- > -Austin > -- Travis Athougies
Wine test bot failure?
Hi, I was trying to run my HLSL test suite patch through wine test bot and the build failed. However, it's not a compilation failure. Instead it complains about not being able to copy some executable file. Also, the build fails almost instantaneously. It also complains about a missing command. The results are at https://testbot.winehq.org/JobDetails.pl?Key=5449&log_101=1#k101 How can I get this to builid and run? Travis Athougies. -- Travis Athougies
Re: [PATCH 2/7] d3dx9_36/tests: Added HLSL test suite
Henri, You said to move all assembler, shader and compiler tests over to the d3dcompiler dll. When you said this did you mean only grammar tests, or did you also mean visual tests as well? I've hit sort of a roadblock, where I don't know exactly what to place where. Right now all the tests are visual, so I need to use either the d3d9 or d3d10 API. However, the d3dcompiler dll is version agnostic, so I'm unsure of which version of d3d I should use. As far as I can tell, the wine d3d 10 implementation is much less mature than its d3d9 one, which makes me guess I should put it in one of the d3d9 dlls. D3dx9 wouldn't be the right place for it, though, since I'm using the d3dcompiler calls instead of D3DXCompileShader. So should I move the visual tests to d3d9? Or somewhere else? To complicate things, the next thing on my list is implementing grammar tests. These should definitely go in d3dcompiler, but that still doesn't answer the question of where to put the visual tests? Travis. On Wed, Aug 25, 2010 at 9:11 AM, Dan Kegel wrote: > Henri wrote: >> You still have basic formatting errors like trailing spaces, mixing >> tabs and spaces, inconsistent "*" placement, etc. >> ... unnecessary cast... >> wine-patches reviews are supposed to catch non-trivial interactions with >> other parts of the code, not trivial whitespace errors. > > Looking at http://wiki.winehq.org/SubmittingPatches, I didn't see a nice, > concise checklist of things to check for, so I added one at the top, and > linked to Henri's post. Improvements welcome. > - Dan > > > -- Travis Athougies
RFC: draft HLSL test suite
Henri,
Thanks for the quick reply.
So I got rid of the maze of structures and instead called the helper functions
directly for each test. The tests are now much easier to understand.
Is there anything
else that should be incorporated into these tests? Any other ideas or
suggestions?
Travis Athougies
On Thu, Jul 22, 2010 at 3:16 PM, Henri Verbeet wrote:
> On 21 July 2010 23:46, Travis Athougies wrote:
>> an HLSL test suite might be a good place to start, so here's a draft
>> of a test framework and a few tests that use it. It's not complete, I
>> know (for one, I need to write tests for vertex shaders), but before I
>> go any farther, I'd like to know get peoples' opinion on the design.
>> Is this the way a wine HLSL test suite should look? Is there something
>> else I should incorporate into these tests? I'm absolutely open to
>> suggestions.
> Leaving the (fairly significant) specific issues with the
> implementation aside, the basic idea will work, but it seems to me
> you're making it more complicated than it needs to be. This really
> doesn't need to be much more complex than the tests in
> dlls/d3d9/tests/visual.c, though a bit more generic would be nice.
>
>> http://wiki.winehq.org/HLSLTestSuite
> That page is pretty awful.
>
--
Travis Athougies
--
Travis Athougies
diff --git a/dlls/d3dx9_36/tests/Makefile.in b/dlls/d3dx9_36/tests/Makefile.in
index b9ad40d..ed7857a 100644
--- a/dlls/d3dx9_36/tests/Makefile.in
+++ b/dlls/d3dx9_36/tests/Makefile.in
@@ -13,7 +13,8 @@ C_SRCS = \
math.c \
mesh.c \
shader.c \
- surface.c
+ surface.c \
+ hlsl.c
RC_SRCS = rsrc.rc
diff --git a/dlls/d3dx9_36/tests/hlsl.c b/dlls/d3dx9_36/tests/hlsl.c
new file mode 100644
index 000..9c91294
--- /dev/null
+++ b/dlls/d3dx9_36/tests/hlsl.c
@@ -0,0 +1,703 @@
+/*
+ * Copyright (C) 2010 - Travis Athougies
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+ */
+#define CINTERFACE
+#define COBJMACROS
+#include "wine/test.h"
+
+#include
+#include
+#include
+#include
+#include
+
+#include "hlsl.h"
+
+/* if GEN_EXPECTED is #define'd, then we will dump the values generated by the
+ PIXEL_SHADER_TEST_LINEAR's tests. You can use this to generate the expected
+ values tables.
+*/
+
+/* Global variables, etc */
+static LPDIRECT3DVERTEXBUFFER9 geometry = 0;
+static LPDIRECT3DVERTEXSHADER9 vshader = 0;
+static LPDIRECT3DVERTEXDECLARATION9 vdeclaration = 0;
+static LPDIRECT3DDEVICE9 device = 0;
+
+#ifdef GEN_EXPECTED
+static HANDLE output = 0;
+#endif
+
+const struct vertex
+{
+float x, y, z;
+} geometry_vertices[4] = {
+{-1, -1, 0},
+{-1, 1, 0},
+{1, -1, 0},
+{1, 1, 0}
+};
+
+const D3DVERTEXELEMENT9 vdeclelements[] = {
+{0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0},
+D3DDECL_END()
+};
+
+/* For ps_3_0 shaders, we need a vertex shader */
+const char* vshader_hlsl =
+"float4 vshader(float4 pos: POSITION): POSITION \
+{ \
+ return pos; \
+} \
+";
+
+/* Generic functions to create windows, initialize Direct3D, etc. */
+static HWND create_window(void)
+{
+WNDCLASS wc = {0};
+wc.lpfnWndProc = DefWindowProc;
+wc.lpszClassName = "d3d9_test_wc";
+RegisterClass(&wc);
+
+return CreateWindow("d3d9_test_wc", "d3d9_test",
+0, 0, 0, 0, 0, 0, 0, 0, 0);
+}
+
+static IDirect3DDevice9 *init_d3d9(HMODULE d3d9_handle)
+{
+IDirect3D9 * (__stdcall * d3d9_create)(UINT SDKVersion) = 0;
+IDirect3D9 *d3d9_ptr = 0;
+IDirect3DDevice9 *device_ptr = 0;
+D3DPRESENT_PARAMETERS present_parameters;
+D3DXMATRIX projection_matrix;
+
+void* temp_geometry_vertices;
+
+LPD3DXBUFFER compiled = NULL;
+LPD3DXBUFFER errors;
+
+HRESULT hres;
+
+d3d9_create = (void *)GetProcAddress(d3d9_handle, "Direct3DCreate9");
+ok(d3d9_create != NULL, "Failed to get address of Direct3DCreate9\n");
+if (!d3d9_create) return NULL;
+
+d3d9_ptr = d3d9_create(D3D_SDK_VERSION);
+if (!d3d9_ptr)
+{
+skip("could not create D3D9\n");
+return NULL;
+}
+
+ZeroMemory(&
RFC: draft HLSL test suite
Hi,
First of all, let me introduce myself. I'm a young software developer
entering college this fall. I've always been interested in helping out
the WINE project, especially in the areas of DirectX and Direct3D. Dan
Kegel suggested in http://wiki.winehq.org/HLSLTestSuite that writing
an HLSL test suite might be a good place to start, so here's a draft
of a test framework and a few tests that use it. It's not complete, I
know (for one, I need to write tests for vertex shaders), but before I
go any farther, I'd like to know get peoples' opinion on the design.
Is this the way a wine HLSL test suite should look? Is there something
else I should incorporate into these tests? I'm absolutely open to
suggestions.
And lastly, I'd like to thank the wine developers for producing such a
wonderful piece of software. We really appreciate you all!
Travis Athougies.
diff --git a/dlls/d3dx9_36/tests/Makefile.in b/dlls/d3dx9_36/tests/Makefile.in
index b9ad40d..ed7857a 100644
--- a/dlls/d3dx9_36/tests/Makefile.in
+++ b/dlls/d3dx9_36/tests/Makefile.in
@@ -13,7 +13,8 @@ C_SRCS = \
math.c \
mesh.c \
shader.c \
- surface.c
+ surface.c \
+ hlsl.c
RC_SRCS = rsrc.rc
diff --git a/dlls/d3dx9_36/tests/hlsl.c b/dlls/d3dx9_36/tests/hlsl.c
new file mode 100644
index 000..9c91294
--- /dev/null
+++ b/dlls/d3dx9_36/tests/hlsl.c
@@ -0,0 +1,703 @@
+/*
+ * Copyright (C) 2010 - Travis Athougies
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+ */
+#define CINTERFACE
+#define COBJMACROS
+#include "wine/test.h"
+
+#include
+#include
+#include
+#include
+#include
+
+#include "hlsl.h"
+
+/* if GEN_EXPECTED is #define'd, then we will dump the values generated by the
+ PIXEL_SHADER_TEST_LINEAR's tests. You can use this to generate the expected
+ values tables.
+*/
+
+/* Global variables, etc */
+static LPDIRECT3DVERTEXBUFFER9 geometry = 0;
+static LPDIRECT3DVERTEXSHADER9 vshader = 0;
+static LPDIRECT3DVERTEXDECLARATION9 vdeclaration = 0;
+static LPDIRECT3DDEVICE9 device = 0;
+
+#ifdef GEN_EXPECTED
+static HANDLE output = 0;
+#endif
+
+const struct vertex
+{
+float x, y, z;
+} geometry_vertices[4] = {
+{-1, -1, 0},
+{-1, 1, 0},
+{1, -1, 0},
+{1, 1, 0}
+};
+
+const D3DVERTEXELEMENT9 vdeclelements[] = {
+{0, 0, D3DDECLTYPE_FLOAT3, D3DDECLMETHOD_DEFAULT, D3DDECLUSAGE_POSITION, 0},
+D3DDECL_END()
+};
+
+/* For ps_3_0 shaders, we need a vertex shader */
+const char* vshader_hlsl =
+"float4 vshader(float4 pos: POSITION): POSITION \
+{ \
+ return pos; \
+} \
+";
+
+/* Generic functions to create windows, initialize Direct3D, etc. */
+static HWND create_window(void)
+{
+WNDCLASS wc = {0};
+wc.lpfnWndProc = DefWindowProc;
+wc.lpszClassName = "d3d9_test_wc";
+RegisterClass(&wc);
+
+return CreateWindow("d3d9_test_wc", "d3d9_test",
+0, 0, 0, 0, 0, 0, 0, 0, 0);
+}
+
+static IDirect3DDevice9 *init_d3d9(HMODULE d3d9_handle)
+{
+IDirect3D9 * (__stdcall * d3d9_create)(UINT SDKVersion) = 0;
+IDirect3D9 *d3d9_ptr = 0;
+IDirect3DDevice9 *device_ptr = 0;
+D3DPRESENT_PARAMETERS present_parameters;
+D3DXMATRIX projection_matrix;
+
+void* temp_geometry_vertices;
+
+LPD3DXBUFFER compiled = NULL;
+LPD3DXBUFFER errors;
+
+HRESULT hres;
+
+d3d9_create = (void *)GetProcAddress(d3d9_handle, "Direct3DCreate9");
+ok(d3d9_create != NULL, "Failed to get address of Direct3DCreate9\n");
+if (!d3d9_create) return NULL;
+
+d3d9_ptr = d3d9_create(D3D_SDK_VERSION);
+if (!d3d9_ptr)
+{
+skip("could not create D3D9\n");
+return NULL;
+}
+
+ZeroMemory(&present_parameters, sizeof(present_parameters));
+present_parameters.Windowed = TRUE;
+present_parameters.hDeviceWindow = create_window();
+present_parameters.SwapEffect = D3DSWAPEFFECT_DISCARD;
+
+hres = IDirect3D9_CreateDevice(d3d9_ptr, D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL, NULL, D3DCREATE_SOFTWARE_VERTEXPROCESSING, &present_parameters, &device_ptr);
+ok(SUCCEEDED(hres), "Could not create device, IDirect3D9_CreateDevice returned: %08x", hres);
+
+/*
Contributing
Hi, I would like to help contribute to WINE because I use it so much yet I've never actually contributed to it. I was wondering if there are any simple bugs or other tasks that someone new to WINE development could undertake. Thanks. -- From: Travis Athougies 2 + 2 = 4
