Re: [PATCH v6 0/2] MR979: d3d9: Implement d3dx_effect_SetRawValue

[Matteo Bruni (＠Mystral)]

On Wed Oct 19 11:23:25 2022 +0000, Patrick Hibbs wrote:
> Sorry about the unexpected push, I'll be more careful next time.
> With regards to the bool fixups, are we sure we want to just write the
> raw data given to us? I ask because I've also noticed that SetRawValue()
> has a tendency to promote datatypes when given a single value and a
> vector / matrix paramater. I.e. float and int become a float4 and int4
> respectively for both vectors and matrices. (MSDN calls this behavior a
> "cast.") This results in the next 3 values in the paramater's buffer
> being set to zero.
> Assuming SetRawValue() shouldn't fix the bool values itself, should it
> also avoid fixing the ints and floats? If so, how should we indicate the
> need for a fixup to other functions? There doesn't seem to be any
> infrastructure to support that in wine currently. (We could abuse the
> paramater block support for that, but see the speculation below.)
> The float and int promotions were the reason I didn't clear the
> paramater after each test. As doing so would cause us to miss the writes
> to the next 3 values in the paramater. As they would already be set to
> zero before the call. Should I still go ahead and clear the paramaters
> after each test anyway?
> As a dirty hack, I added a call to zero out the paramaters after each
> test with a 4x4 matrix of zero ints in v3 of the test. That call is
> commented out in the push, but if you enable it it will succeed for most
> of the tests under win10. (I disabled it before I fixed the string test
> crashes. So it may work fine now, I haven't checked.) I haven't tried
> checking the paramaters after the current one for alterations. I was
> going to put that in it's own set of tests.
> This part is speculation but, if it is a valid use of SetRawValue() to
> set adjacent paramaters regardless of type with one call, then the
> entire set of paramaters for a given effect needs to be in the same
> memory block. (Which seems to be not the case in wine, as there are
> plenty of calls to heap_alloc() / heap_realloc() using the param->data
> pointer in effect.c.) That would also imply that there is a specific
> ordering of paramaters in memory as well. Which would need to be tested
> for. If this behavior is combined with the bool assumption above, then
> we'd also need to test for order of operations too.
> Assuming all of that needs to be done, how do we split this up? Because
> I have a feeling that this patchset is going to be rather large in
> scope. (If all of the assumptions and speculation are true that is.)
> Unfortunately I keep being unable to reply promptly. Anyway...
> With regards to the bool fixups, are we sure we want to just write the raw data given to us? I ask because I've also noticed that SetRawValue() has a tendency to promote datatypes when given a single value and a vector / matrix paramater. I.e. float and int become a float4 and int4 respectively for both vectors and matrices. (MSDN calls this behavior a "cast.") This results in the next 3 values in the paramater's buffer being set to zero.

Is that what happens or is just the rest of the parameter being set to 0?
> Assuming SetRawValue() shouldn't fix the bool values itself, should it also avoid fixing the ints and floats?

How should they be fixed? I'm still assuming that SetRawValue() more or less does what it says i.e. it just sets the value of parameters by mere byte-for-byte copy, without any kind of conversion or fixup. Do we have evidence that's not the case?
> The float and int promotions were the reason I didn't clear the paramater after each test. As doing so would cause us to miss the writes to the next 3 values in the paramater. As they would already be set to zero before the call. Should I still go ahead and clear the paramaters after each test anyway?

It might make sense to clear them to a non-zero value (e.g. some variation of the usual 0xdeadbeef marker) to make those results more evident.
> This part is speculation but, if it is a valid use of SetRawValue() to set adjacent paramaters regardless of type with one call, then the entire set of paramaters for a given effect needs to be in the same memory block. (Which seems to be not the case in wine, as there are plenty of calls to heap_alloc() / heap_realloc() using the param->data pointer in effect.c.) That would also imply that there is a specific ordering of paramaters in memory as well. Which would need to be tested for. If this behavior is combined with the bool assumption above, then we'd also need to test for order of operations too.

Yes, that wouldn't quite work right away. It's fine to just add a FIXME() for straddling parameters for the time being, if it turns out that native actually supports that. Similarly for the test.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/979#note_12210