Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix snorm #2033

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix snorm #2033

wants to merge 2 commits into from

Conversation

joshqti
Copy link
Contributor

@joshqti joshqti commented Aug 5, 2024

When comparing scanlines for SNORM images, take into account that -1.0 can be exactly represented in two different ways.

When comparing scanlines for SNORM images,
take into account that -1.0 can be exactly
represented in two different ways.
break;

case CL_SNORM_INT16: {
cl_short aPixel = *(cl_short *)aPtr;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be ushort

@svenvh
Copy link
Member

svenvh commented Aug 12, 2024

take into account that -1.0 can be exactly represented in two different ways.

Does this only apply to the Embedded Profile or also to the Full Profile?

@bashbaug
Copy link
Contributor

Can you please explain a bit more exactly what scenario this change is trying to fix? Does the problem happen when copying SNORM data from one image to the other? If so, does the multiple representations for -1.0 cause problems for the source data, or the destination data, or both?

The reason why I ask is because the spec is pretty clear how the conversions between SNORM <-> float should occur:

SNORM to float
float to SNORM

I think I have an idea how this still could cause a problem (specifically with the two source representations for -1?) but I'd like to be sure I understand it - thanks!

@joshqti
Copy link
Contributor Author

joshqti commented Oct 14, 2024

Correct, there are two valid representations on -1.0, it is possible a valid image copy to cause a false failure in the test as-is.

@joshqti
Copy link
Contributor Author

joshqti commented Oct 29, 2024

Snorm has two representations of -1.0. Lets look at CL_R, SNORM_INT8 for example.

Lets sat a source image has the pixel 0x80, this is one of two valid -1.0 representation. One way to copy this pixel is to first read, say using "x = read_imagef(source image)". Variable x will be -1.0. Lets say the device then writes the pixel to a destination image using the forumal from the spec "convert_char_sat_rte(f * 127.0f)". It will convert -127.0f to -127, which is 0x81.

The CTS test will notice 0x80 != 0x81. This change makes CTS aware of snorm's multiple representation of -1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants