On Tue, Aug 31, 2021 at 3:57 PM Nikolay Sivov nsivov@codeweavers.com wrote:
On 8/31/21 4:32 PM, Matteo Bruni wrote:
On Mon, Aug 30, 2021 at 7:07 AM Nikolay Sivov nsivov@codeweavers.com wrote:
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com
dlls/d3d10/effect.c | 172 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 166 insertions(+), 6 deletions(-)
Patch is okay, a few notes below.
diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c index 113125e9cbd..c3c98a0da31 100644 --- a/dlls/d3d10/effect.c +++ b/dlls/d3d10/effect.c @@ -566,6 +592,127 @@ static HRESULT get_fx10_shader_resources(struct d3d10_effect_variable *v, const return S_OK; }
+struct d3d10_effect_so_decl +{
- D3D10_SO_DECLARATION_ENTRY *entries;
- SIZE_T capacity;
- SIZE_T count;
- unsigned int stride;
- char *decl;
+};
+static void d3d10_effect_cleanup_so_decl(struct d3d10_effect_so_decl *so_decl) +{
- heap_free(so_decl->entries);
- heap_free(so_decl->decl);
- memset(so_decl, 0, sizeof(*so_decl));
+}
+static HRESULT d3d10_effect_parse_stream_output_declaration(const char *decl,
struct d3d10_effect_so_decl *so_decl)+{
- static const char * allmask = "xyzw";
Usually in HLSL you can use "rgba" in place of "xyzw" for swizzles / writemasks, it might be worth checking if those also work (and are passed through to the effect bytecode unchanged).
Yes, "rgba" also works apparently. Compiler preserves string as is, does not convert to coordinate mask names. I'll add a test for that.
- char *p, *ptr, *end, *next, *mask, *m, *slot;
- unsigned int len = strlen(decl);
- D3D10_SO_DECLARATION_ENTRY e;
- memset(so_decl, 0, sizeof(*so_decl));
- if (!(so_decl->decl = heap_alloc(len + 1)))
return E_OUTOFMEMORY;- memcpy(so_decl->decl, decl, len + 1);
- p = so_decl->decl;
I think it would be nice to have a small comment showing the expected syntax of a stream out declaration (entry) somewhere around here. Just something to quickly glance at while reading the following code.
Okay.
- while (p && *p)
- {
memset(&e, 0, sizeof(e));end = strchr(p, ';');next = end ? end + 1 : p + strlen(p);len = next - p;if (end) len--;/* Remove leading and trailing spaces. */while (len && isspace(*p)) { len--; p++; }while (len && isspace(p[len - 1])) len--;p[len] = 0;/* Output slot */if ((slot = strchr(p, ':'))){*slot = 0;ptr = p;while (*ptr){if (!isdigit(*ptr)){WARN("Invalid output slot %s.\n", debugstr_a(p));goto failed;}ptr++;}e.OutputSlot = atoi(p);p = slot + 1;}/* Mask */if ((mask = strchr(p, '.'))){*mask = 0; mask++;if (!(m = strstr(allmask, mask))){WARN("Invalid component mask %s.\n", debugstr_a(mask));goto failed;}e.StartComponent = m - allmask;e.ComponentCount = strlen(mask);Did you verify that it's illegal to have a wrongly ordered mask (e.g. something like .wzyx)? I expect that to be invalid but better to be sure. Likewise for other assumptions involved here, like repeating the same component twice.
Yes, it is disallowed to break the order. API manifestation is that you specify start and count, which assumes it's always forward looking. Regarding error, it won't pass compiler check first, complaining about "invalid mask declaration". As far as I can tell that's the only thing that compiler checks in this string, you can have non-existent semantics as much as you like. That will break effect creation later though, presumably because shader object can't be created with invalid declaration.
Excellent. That should already do the right thing then.
In general, some more tests with interesting stream out declarations would be nice. I guess you have more planned after the test in patch 5/5.
I can think of adding more variations with spaces around, and maybe with multiple entries. Do you have other ideas?
Yes, that sounds good. I guess generally trying to exercise all the edge cases of the parser.
I just noticed that there is probably one case that needs special handling. From the MS docs:
'There is one special Semantic, labeled "$SKIP" which indicates an empty semantic, leaving the corresponding memory in the stream out buffer untouched. The $SKIP semantic cannot have a SemanticIndex, but can have a Mask.'
It should probably be translated into a declaration entry with NULL semantic name. See test_stream_output() in d3d10core/tests/d3d10core.c for a few examples (both valid and not) if you haven't yet.
}else{e.StartComponent = 0;e.ComponentCount = 4;}/* Semantic index and name */len = strlen(p);while (isdigit(p[len - 1]))len--;if (p[len]){e.SemanticIndex = atoi(&p[len]);p[len] = 0;}e.SemanticName = p;if (!d3d_array_reserve((void **)&so_decl->entries, &so_decl->capacity, so_decl->count + 1,sizeof(*so_decl->entries)))goto failed;so_decl->entries[so_decl->count++] = e;if (e.OutputSlot == 0)so_decl->stride += e.ComponentCount * sizeof(float);This looks weird, but that's probably because the API for ID3D10Device_CreateGeometryShaderWithStreamOutput() is confusing (just one stride for up to 4 output buffers? What does it even mean?) and neither the documentation nor our tests seem to clarify that. Any clue what's supposed to happen with multiple output buffers?
FWIW that function's signature makes a lot more sense in d3d11.
My understanding is that in d3d10 you can specify stride only when you use one SO buffer. And if you use multiple buffers runtime will use default strides for all. For d3d11 you can set each explicitly.
Somehow I missed the relevant tests in d3d10core.c. It is still weird but I guess not as mysterious now.
After playing with this a bit, it looks like it's actually more strict than that: if there is just one buffer you need to pass a non-zero stride; OTOH when there are multiple buffers you need to pass 0 as stride. Please double check that it's the case (as I still don't trust myself a lot on this) and consider sending a fix if necessary.