Re: GSoC-2011: Implement Missing Mesh Functions in Wine’s D3DX9

[Michael Mc Donnell]

On Tue, Jun 14, 2011 at 1:43 PM, Stefan Dösinger <stefandoesinger at gmx.at> wrote:
> On Tuesday 14 June 2011 12:57:35 Michael Mc Donnell wrote:
>> I cache the vertex declaration (D3DVERTEXELEMENT9 array) and the size
>> of the vertex declaration, so that they can be used by
>> GetNumBytesPerVertex and GetDeclaration when an invalid declaration
>> has been passed.
>
> In GetDeclaration:
>
>> +    memcpy(declaration, This->cached_declaration, sizeof(This-
>>cached_declaration));
> You should probably test if native really writes the full MAX_FVF_DECL_SIZE
> declaration elements, or only up to the D3DDECL_END() marker in the real
> declaration. A similar consideration applies to UpdateSemantics, but that is
> harder to test. E.g.:
>
> +    D3DVERTEXELEMENT9 declaration[MAX_FVF_DECL_SIZE];
> +    D3DVERTEXELEMENT9 declaration0[] =
> +    {
> ...
>
> memset(declaration, 0xaa, sizeof(declaration));
> memcpy(declaration, declaration0, sizeof(declaration0));
> UpdateSemantics(declaration)
> memset(declaration, 0xbb, sizeof(declaration));
> GetDeclaration(declaration);
>
> Now look at the bytes in 'declaration' after the D3DDECL_END() marker up to
> the end of the array. They could be 0xaa(UpdateSemantics reads everything,
> GetDeclaration writes everything), 0xbb(UpdateSemantics is unknown,
> GetDeclaration writes only the defined part), or something else(e.g.
> UpdateSemantics and/or GetDeclaration clear set the extra bytes to 0).

I've added the test you outlined. It shows you're correct that
GetDeclaration only writes up to D3DDECL_END().  I've changed the
implementation so that it caches the number of elements and only
writes up to D3DDECL_END().

> In the way your implementation and tests are currently set up UpdateSemantics
> will read undefined memory because your test declarations like declaration0
> don't have the full MAX_FVF_DECL_SIZE array size. Otoh I am not quite sure
> about the semantics of passing an array on the stack like this. Maybe the
> compiler allocates a new array with the full size and copies the content
> around.

You're right it could potentially read undefined memory depending on
the compiler semantics. I think the safest thing is just to read up to
D3DDECL_END() in the passed in declaration, then it will never read
undefined memory (except if the programmer makes a mistake). I've
changed the implementation to count the number of elements in the new
declaration, cache the number, and then copy the contents of new
declaration into the cached declaration. Counting the elements doesn't
add any extra overhead as I also needed it to check for non-zero
stream values.

> Also I think this method has been pretty thoroughly tested and debated by now.
> If we implement all the functions this way we'd probably be pretty bug-free
> :-)

Yeah it turned out to be a lot harder than I had expected to get all
the details correct. I have also added a check for non-zero stream
values that Dylan Smith wanted me to add.

> Also, there's the obligatory style nitpick:
>> +    IDirect3DDevice9 *device;
>> ...
>> +static struct test_context* new_test_context(void)
> For consistency this should probably be static struct test_context
> *new_test_context(void).

Check :-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-d3dx9-test-Add-UpdateSemantics-test.patch
Type: text/x-patch
Size: 19521 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110616/a015e968/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-d3dx9-Implement-UpdateSemantics-mesh-method.patch
Type: text/x-patch
Size: 16634 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20110616/a015e968/attachment-0003.bin>