Re: [v2 2/6] d3dx9: Apply changed states only in CommitChanges.
2017-03-14 13:46 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com>
The current subject might be slightly confusing, something like "d3dx9: Don't apply unmodified states in CommitChanges." would be probably better. The patch looks generally good. Comments follow.
--- v2: - stored current_light and current_material in effect structure instead of pass; - renamed clear_dirty_all() -> clear_dirty_params(). --- +static BOOL param_clear_dirty_func(void *dummy, struct d3dx_parameter *param) +{ + *param->dirty_flag_ptr &= ~PARAMETER_FLAG_DIRTY; + return FALSE; +} + +static void clear_dirty_params(struct d3dx9_base_effect *base) +{ + unsigned int i; + + for (i = 0; i < base->parameter_count; ++i) + walk_parameter_tree(&base->parameters[i], param_clear_dirty_func, NULL); +}
The walk seems unnecessary since we know that all the children parameters share their dirty flag with their parents.
- param = param->referenced_param; - TRACE("Array index %u.\n", array_idx); - if (array_idx >= param->element_count) + if (array_idx >= ref_param->element_count) { - ERR("Computed array index %u is out of bound %u.\n", array_idx, param->element_count); + ERR("Computed array index %u is out of bound %u.\n", array_idx, ref_param->element_count); return D3DERR_INVALIDCALL; } - param = ¶m->members[array_idx]; + param = &ref_param->members[array_idx]; + *param_dirty = state->index != array_idx || is_param_dirty(param);
This looked a bit weird to me at first because of reusing "param". Maybe use a separate "selected_param" variable (or something to that effect)?
- if (FAILED(hr = d3dx9_get_param_value_ptr(effect, pass, state, ¶m_value, ¶m))) + if (FAILED(hr = d3dx9_get_param_value_ptr(effect, pass, state, ¶m_value, ¶m, + update_all, ¶m_dirty))) /* Native d3dx returns D3D_OK from BeginPass or Commit involving out of bounds array - * access and does not touch affected state. */ + * access and does not touch affected state, except for BeginPass fails with E_FAIL if + * out of bounds array index depends on dirty parameter(s) in BeginPass. The latter case + * is TODO. */
Nitpicking on the comment, it sounds better to me with something like "...except for BeginPass when the out of bounds array index depends on dirty parameters. The latter case is supposed to return E_FAIL but is currently TODO". BTW, the ERR in d3dx9_get_param_value_ptr() should probably be a WARN since it can be triggered by the application.
+ TRACE("d3dx9_get_param_value_ptr error %#x.\n", hr);
This one shouldn't be necessary, I think there should have been a more specific error / warning printed out by d3dx9_get_param_value_ptr() or one of its callees. Otherwise you could repurpose this to say that you're returning D3D_OK notwithstanding the failure.
+ return D3D_OK; + }
Actually, it's probably better to change the d3dx9_get_param_value_ptr() return on out of bounds to E_FAIL and only override the return value to D3D_OK in that specific case.
+BOOL is_param_eval_input_dirty(struct d3dx_param_eval *peval) +{ + return is_const_tab_input_dirty(&peval->pres.inputs) + || is_const_tab_input_dirty(&peval->shader_inputs); +}
If I'm not mistaken this only needs to check pres.inputs, given that it's only reasonably going to be called on FXLC and ARRAY_SELECTOR preshaders (not on actual shaders' preshaders). It doesn't matter much though.
- set_constants(rs, &pres->inputs); - if (FAILED(hr = execute_preshader(pres))) - return hr; + if (update_all || is_const_tab_input_dirty(&pres->inputs)) + { + set_constants(rs, &pres->inputs, TRUE); + if (FAILED(hr = execute_preshader(pres))) + return hr; + regstore_reset_table(rs, PRES_REGTAB_CONST); + }
This is technically a separate bugfix which would be better as a separate patch, before this one.
On 03/16/2017 01:39 AM, Matteo Bruni wrote:
- param = param->referenced_param; - TRACE("Array index %u.\n", array_idx); - if (array_idx >= param->element_count) + if (array_idx >= ref_param->element_count) { - ERR("Computed array index %u is out of bound %u.\n", array_idx, param->element_count); + ERR("Computed array index %u is out of bound %u.\n", array_idx, ref_param->element_count); return D3DERR_INVALIDCALL; } - param = ¶m->members[array_idx]; + param = &ref_param->members[array_idx]; + *param_dirty = state->index != array_idx || is_param_dirty(param); This looked a bit weird to me at first because of reusing "param". Maybe use a separate "selected_param" variable (or something to that effect)?
Yes, sure.
- if (FAILED(hr = d3dx9_get_param_value_ptr(effect, pass, state, ¶m_value, ¶m))) + if (FAILED(hr = d3dx9_get_param_value_ptr(effect, pass, state, ¶m_value, ¶m, + update_all, ¶m_dirty))) /* Native d3dx returns D3D_OK from BeginPass or Commit involving out of bounds array - * access and does not touch affected state. */ + * access and does not touch affected state, except for BeginPass fails with E_FAIL if + * out of bounds array index depends on dirty parameter(s) in BeginPass. The latter case + * is TODO. */ Nitpicking on the comment, it sounds better to me with something like "...except for BeginPass when the out of bounds array index depends on dirty parameters. The latter case is supposed to return E_FAIL but is currently TODO".
BTW, the ERR in d3dx9_get_param_value_ptr() should probably be a WARN since it can be triggered by the application.
+ TRACE("d3dx9_get_param_value_ptr error %#x.\n", hr); This one shouldn't be necessary, I think there should have been a more specific error / warning printed out by d3dx9_get_param_value_ptr() or one of its callees. Otherwise you could repurpose this to say that you're returning D3D_OK notwithstanding the failure. The reason I left ERR in d3dx9_get_param_value_ptr is now our implementation for out of bounds selectors errors handling is different from native implementation for certain cases. Should I still avoid ERR or FIXME, or maybe change TRACE("d3dx9_get_param_value_ptr error %#x.\n", hr); to ERR("d3dx9_get_param_value_ptr error %#x, returning D3D_OK.\n", hr); when returning OK status only? + return D3D_OK; + } Actually, it's probably better to change the d3dx9_get_param_value_ptr() return on out of bounds to E_FAIL and only override the return value to D3D_OK in that specific case.
+BOOL is_param_eval_input_dirty(struct d3dx_param_eval *peval) +{ + return is_const_tab_input_dirty(&peval->pres.inputs) + || is_const_tab_input_dirty(&peval->shader_inputs); +} If I'm not mistaken this only needs to check pres.inputs, given that it's only reasonably going to be called on FXLC and ARRAY_SELECTOR preshaders (not on actual shaders' preshaders). It doesn't matter much though. If it is FXLC or ARRAY_SELECTOR peval->shader_inputs is an empty table with zero count, and I thought that doing a check in is_const_tab_input_dirty() with zero count in table is not much different from explicitly checking the type of param_eval, while it is shorter/simpler and more generic. Maybe I could leave it this way?
- set_constants(rs, &pres->inputs); - if (FAILED(hr = execute_preshader(pres))) - return hr; + if (update_all || is_const_tab_input_dirty(&pres->inputs)) + { + set_constants(rs, &pres->inputs, TRUE); + if (FAILED(hr = execute_preshader(pres))) + return hr; + regstore_reset_table(rs, PRES_REGTAB_CONST); + } This is technically a separate bugfix which would be better as a separate patch, before this one.
This chunk actually avoids redundant preshader recompute. I don't quite understand how can I do it as a separate previous patch, as this patch introduce "update_all" parameter, is_const_tab_input_dirty() function and all the other logic around it. Or can it be that you were thinking of changes on light and material state update, and copy pasted different chunk?
2017-03-16 10:52 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
On 03/16/2017 01:39 AM, Matteo Bruni wrote:
The reason I left ERR in d3dx9_get_param_value_ptr is now our implementation for out of bounds selectors errors handling is different from native implementation for certain cases. Should I still avoid ERR or FIXME, or maybe change TRACE("d3dx9_get_param_value_ptr error %#x.\n", hr); to ERR("d3dx9_get_param_value_ptr error %#x, returning D3D_OK.\n", hr); when returning OK status only?
I think WARN is what you want to use in d3dx9_get_param_value_ptr(). A FIXME for the broken case only would be okay but at that point you could just fix it properly as well so I don't think you need anything else. The todo_wine in the tests are enough. You can keep the TRACE when returning D3D_OK (definitely not ERR) but I think it's better to only override the return on out-of-bounds, so the thing with using a different HRESULT for that case.
+BOOL is_param_eval_input_dirty(struct d3dx_param_eval *peval) +{ + return is_const_tab_input_dirty(&peval->pres.inputs) + || is_const_tab_input_dirty(&peval->shader_inputs); +}
If I'm not mistaken this only needs to check pres.inputs, given that it's only reasonably going to be called on FXLC and ARRAY_SELECTOR preshaders (not on actual shaders' preshaders). It doesn't matter much though.
If it is FXLC or ARRAY_SELECTOR peval->shader_inputs is an empty table with zero count, and I thought that doing a check in is_const_tab_input_dirty() with zero count in table is not much different from explicitly checking the type of param_eval, while it is shorter/simpler and more generic. Maybe I could leave it this way?
Yeah, it's fine.
- set_constants(rs, &pres->inputs); - if (FAILED(hr = execute_preshader(pres))) - return hr; + if (update_all || is_const_tab_input_dirty(&pres->inputs)) + { + set_constants(rs, &pres->inputs, TRUE); + if (FAILED(hr = execute_preshader(pres))) + return hr; + regstore_reset_table(rs, PRES_REGTAB_CONST); + }
This is technically a separate bugfix which would be better as a separate patch, before this one.
This chunk actually avoids redundant preshader recompute. I don't quite understand how can I do it as a separate previous patch, as this patch introduce "update_all" parameter, is_const_tab_input_dirty() function and all the other logic around it. Or can it be that you were thinking of changes on light and material state update, and copy pasted different chunk?
I just meant the regstore_reset_table() part.
- set_constants(rs, &pres->inputs); - if (FAILED(hr = execute_preshader(pres))) - return hr; + if (update_all || is_const_tab_input_dirty(&pres->inputs)) + { + set_constants(rs, &pres->inputs, TRUE); + if (FAILED(hr = execute_preshader(pres))) + return hr; + regstore_reset_table(rs, PRES_REGTAB_CONST); + } This is technically a separate bugfix which would be better as a separate patch, before this one.
This chunk actually avoids redundant preshader recompute. I don't quite understand how can I do it as a separate previous patch, as this patch introduce "update_all" parameter, is_const_tab_input_dirty() function and all the other logic around it. Or can it be that you were thinking of changes on light and material state update, and copy pasted different chunk? I just meant the regstore_reset_table() part.
I am sorry, I still don't understand how it is a separate bugfix. I don't see the absence of it as a bug in previous code, reset of constants is not required as they are updated anyway. Actually regstore_reset_table() call along with ultimate TRUE for update_all parameter in set_constants() call here is redundant. I added it just to make things a bit more robuts and more robust checking of unitialized input in preshader in case of some bugs in their update logic, which got more complicated with selective constants update. It should actually work (without updating constants from unchanged parameters) if change set_constants(rs, &pres->inputs, TRUE); to set_constants(rs, &pres->inputs, update_all); and remove regstore_reset_table(). Maybe I should do it like this?
2017-03-16 17:15 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
- set_constants(rs, &pres->inputs); - if (FAILED(hr = execute_preshader(pres))) - return hr; + if (update_all || is_const_tab_input_dirty(&pres->inputs)) + { + set_constants(rs, &pres->inputs, TRUE); + if (FAILED(hr = execute_preshader(pres))) + return hr; + regstore_reset_table(rs, PRES_REGTAB_CONST); + }
This is technically a separate bugfix which would be better as a separate patch, before this one.
This chunk actually avoids redundant preshader recompute. I don't quite understand how can I do it as a separate previous patch, as this patch introduce "update_all" parameter, is_const_tab_input_dirty() function and all the other logic around it. Or can it be that you were thinking of changes on light and material state update, and copy pasted different chunk?
I just meant the regstore_reset_table() part.
I am sorry, I still don't understand how it is a separate bugfix. I don't see the absence of it as a bug in previous code, reset of constants is not required as they are updated anyway.
I guess you can argue that, sure. It's mostly that set_shader_constants_device() calls regstore_reset_table() and, for symmetry if not in practice, it seems better to clear here too. Not a big deal either way you choose.
Actually regstore_reset_table() call along with ultimate TRUE for update_all parameter in set_constants() call here is redundant. I added it just to make things a bit more robuts and more robust checking of unitialized input in preshader in case of some bugs in their update logic, which got more complicated with selective constants update. It should actually work (without updating constants from unchanged parameters) if change set_constants(rs, &pres->inputs, TRUE); to set_constants(rs, &pres->inputs, update_all); and remove regstore_reset_table(). Maybe I should do it like this?
Right, that's a good idea I think. If that breaks the tests it means there is some bug somewhere in the update logic. Do you need to remove regstore_reset_table() though?
On 03/16/2017 10:12 PM, Matteo Bruni wrote:
2017-03-16 17:15 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
I am sorry, I still don't understand how it is a separate bugfix. I don't see the absence of it as a bug in previous code, reset of constants is not required as they are updated anyway.
I guess you can argue that, sure. It's mostly that set_shader_constants_device() calls regstore_reset_table() and, for symmetry if not in practice, it seems better to clear here too. Not a big deal either way you choose.
Actually regstore_reset_table() call along with ultimate TRUE for update_all parameter in set_constants() call here is redundant. I added it just to make things a bit more robuts and more robust checking of unitialized input in preshader in case of some bugs in their update logic, which got more complicated with selective constants update. It should actually work (without updating constants from unchanged parameters) if change set_constants(rs, &pres->inputs, TRUE); to set_constants(rs, &pres->inputs, update_all); and remove regstore_reset_table(). Maybe I should do it like this? Right, that's a good idea I think. If that breaks the tests it means there is some bug somewhere in the update logic. Do you need to remove regstore_reset_table() though?
No, it does not break the tests I have now. It may become more tricky in shared parameters update logic, but I don't have enough tests for that part yet so probably some possible problems there should be addressed later. But removing regstore_reset_table() is necessary in this case, because otherwise if some preshader uses both updated and non-updated parameter, the input for non-updated parameter will be zeroed after previous (full) update and not initialized by update logic (in the current version of the patch all preshaders input are initialized due to 'TRUE' ultimately passed as update_all to constant set function, so it can bear table reset).
2017-03-16 20:22 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
On 03/16/2017 10:12 PM, Matteo Bruni wrote:
Actually regstore_reset_table() call along with ultimate TRUE for update_all parameter in set_constants() call here is redundant. I added it just to make things a bit more robuts and more robust checking of unitialized input in preshader in case of some bugs in their update logic, which got more complicated with selective constants update. It should actually work (without updating constants from unchanged parameters) if change set_constants(rs, &pres->inputs, TRUE); to set_constants(rs, &pres->inputs, update_all); and remove regstore_reset_table(). Maybe I should do it like this?
Right, that's a good idea I think. If that breaks the tests it means there is some bug somewhere in the update logic. Do you need to remove regstore_reset_table() though?
No, it does not break the tests I have now. It may become more tricky in shared parameters update logic, but I don't have enough tests for that part yet so probably some possible problems there should be addressed later. But removing regstore_reset_table() is necessary in this case, because otherwise if some preshader uses both updated and non-updated parameter, the input for non-updated parameter will be zeroed after previous (full) update and not initialized by update logic (in the current version of the patch all preshaders input are initialized due to 'TRUE' ultimately passed as update_all to constant set function, so it can bear table reset).
It makes sense. Yes, I'd go with the set_constants(..., update_all) change for the time being since it looks like it should work. If later on something problematic comes up we can always go back on that. Hopefully that won't be necessary though.
participants (2)
-
Matteo Bruni -
Paul Gofman