Igalia / vkrunner

A shader script tester for Vulkan. Moved to https://gitlab.freedesktop.org/mesa/vkrunner
Other
43 stars 14 forks source link

Allow error bounds for expected output #24

Closed jaebaek closed 6 years ago

jaebaek commented 6 years ago

Add "tolerance" command to set up the error bound and support fuzzy equality operator "~=". The fuzzy equality can only be used for comparisions of float values.

EXAMPLE: examples/tolerance.shader_test RELATED ISSUE: #23

jaebaek commented 6 years ago

Thank you for the suggestion. When I wrote this code, I also thought that we need more discussions for the design. To doublecheck my understanding, it should be:

F1. tolerance value0 value1 value2 value3 --> framebuffer, vec4 and dvec4 type F2. tolerance value0 value1 value2 --> vec3 and dvec3 type F3. tolerance value0 value1 --> vec2 and dvec2 type F4. tolerance value0 --> float and double type

F5. If some values are missing, use default value e.g., 0.01

What if the type is one of mat2, mat2x2, mat2x3, mat3x2, or other matrix types?

If it is matNxM type, using M values would be reasonable? For example, we can use tolerance value0 value1 value2 for mat2x3 type.

bpeel commented 6 years ago

Yes, I think that sounds like a reasonable approach if you agree too.

Yes, your explanation of matrix types sounds good to me. To clarify, I think we can treat a matNxM type as an array of N vecM’s. Eg, this command:

probe ssbo mat2x3 0 0 ~= 1 2 3 4 5 6

would be equivalent to:

probe ssbo vec3 0 0 ~= 1 2 3
probe ssbo vec3 0 16 ~= 4 5 6

so it will use the tolerance values tol1, tol2, tol3, tol1, tol2, tol3 in that order. This assumes std140 and col-major layout. (I think having a way to tell the probing code and data uploading code to use a different layout could be interesting but is a separate issue).

bpeel commented 6 years ago

F5. If some values are missing, use default value e.g., 0.01

Hm, I think if someone specifies a single tolerance value maybe it would be more useful if it duplicated it across all of the components. It’s probably only rare cases where you want a different tolerance value for each component. I’m not sure what to do with the unspecified values if there are only two or three components. Maybe we should just disallow that? Ie, you can only specify either a single value or all four. What do you think?

jaebaek commented 6 years ago

I agree with you. Most cases might need only a single tolerance. Let's just allow users to specify a single value or all four. I will send an updated PR soon including the description in README.md file.

bpeel commented 6 years ago

I made a patch here with some further suggested changes that I thought would be easier to just change rather than describing. If you are happy with them then please feel free to squash them into your patch.

https://github.com/Igalia/vkrunner/commit/34dbd94d778e0cbf3af7186734b714155084e980

There is a further problem with the parsing of the tolerance command that I haven’t fixed in that patch. It can’t just return is_end(p) at the end of processing because if that fails it won’t print any error message and the script will just silently fail. I think this section of code could do with some reorganising to avoid repeating the check for negative values. Perhaps it could be moved into its own function (or even split into several functions).

jaebaek commented 6 years ago

It looks good to me. Thanks. After I squashed your commit with mine, I created one more commit to solve the issue you mentioned. Feel free to update it if it is still not readable.

bpeel commented 6 years ago

This looks good to me. If you want to squash it I will merge it.

One last thing, I noticed that the validation layers complain that the tolerance test is missing the fragmentStoresAndAtomics capability. Please add it.

jaebaek commented 6 years ago

Thank you for checking it with the validation layers. I squashed it.