Fix snorm #2033
Fix snorm #2033
Conversation
When comparing scanlines for SNORM images, take into account that -1.0 can be exactly represented in two different ways.
test_common/harness/imageHelpers.cpp
Outdated
| break; | ||
|
|
||
| case CL_SNORM_INT16: { | ||
| cl_short aPixel = *(cl_short *)aPtr; |
There was a problem hiding this comment.
This needs to be ushort
Does this only apply to the Embedded Profile or also to the Full Profile? |
|
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: 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! |
|
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. |
|
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. |
| // -1.0 is defined as 0x80 and 0x81 | ||
| if ((aPixel != bPixel) && ((aPixel | bPixel) != 0x81)) |
There was a problem hiding this comment.
This might be a bit too clever, since as-written it will pass the test if aPixel is 0x81 and bPixel is 0, for example.
Maybe it'd be better to just pick one of the -1.0 representations instead, something like:
| // -1.0 is defined as 0x80 and 0x81 | |
| if ((aPixel != bPixel) && ((aPixel | bPixel) != 0x81)) | |
| // -1.0 is defined as 0x80 and 0x81 | |
| aPixel = (aPixel == 0x80) ? 0x81 : aPixel; | |
| bPixel = (bPixel == 0x80) ? 0x81 : bPixel; | |
| if (aPixel != bPixel) |
| // -1.0 is defined as 0x8000 and 0x8001 | ||
| if ((aPixel != bPixel) && ((aPixel | bPixel) != 0x8001)) |
There was a problem hiding this comment.
See above, and apply whatever fix we make for CL_SNORM_INT8 to CL_SNORM_INT16 here.
When comparing scanlines for SNORM images, take into account that -1.0 can be exactly represented in two different ways.