[Piglit] [PATCH] ext_render_snorm-render: fix read back assumptions

[Tapani Pälli]

On 08/29/2018 08:57 PM, Nanley Chery wrote:
> On Wed, Aug 29, 2018 at 10:15:41AM +0300, Tapani Pälli wrote:
>>
>>
>> On 29.08.2018 00:23, Nanley Chery wrote:
>>> On Tue, Aug 28, 2018 at 09:58:14AM +0300, Tapani Pälli wrote:
>>>> Test assumed that we can read back directly from R8, RG8 fbo but
>>>> this is not actually enabled by the extension. Patch introduces
>>>> additional read from window to sanity check fbo contents.
>>>>
>>>
>>> I think we're allowed to read from R8 and RG8 SNORM FBOs. Page 340 of
>>> the GLES 3.1 spec says that apps can query the driver to find out which
>>> formats are supported for ReadPixels:
>>>
>>>      The second is an implementation-chosen format from among those
>>>      defined in table 8.2, excluding formats DEPTH_COMPONENT and
>>>      DEPTH_STENCIL. The values of format and type for this format may be
>>>      determined by calling GetIntegerv with the symbolic constants
>>>      IMPLEMENTATION_COLOR_READ_FORMAT and IMPLEMENTATION_COLOR_READ_TYPE,
>>>      respectively. The implementation-chosen format may vary depending on
>>>      the format of the selected read buffer of the currently bound read
>>>      framebuffer.
>>>
>>> Table 8.2 has entries for R8 and RG8 SNORM.
>>
>> Maybe the issue is that reading should be allowed only to matching number of
>> components with GL_BYTE? I'm currently reading every format as GL_RGBA but I
>> could change to use GL_RED, GL_RG, GL_RGBA depending on the base format. If
>> I enforce such rules for GL_BYTE formats in Mesa the rest of failing dEQP
>> tests start to pass. So for example, reading fbo with internalFormat of
>> GL_R8_SNORM would require:
>>
>> glReadPixels(0, 0, w, h, GL_RED, GL_BYTE, pix);
>>
>> Does this make sense?
>>
> 
> I think we're also allowed to have mismatching component numbers when
> the format is RGBA. In section 16.1.3 of the GLES3.1 spec:
> 
>     If G, B, or A values are not present in the internal format, they are
>     taken to be zero, zero, and one respectively.
> 
> I think this agrees with the extension spec which allows any 8bit SNORM
> format to be read with RGBA:
> 
>     For 8bit signed normalized fixed-point rendering surfaces, the
>     combination format RGBA and type BYTE is accepted. For a 16bit signed
>     normalized fixed point buffer, the combination RGBA and SHORT is
>     accepted.
> 
> Maybe there's a dEQP bug?


Yeah it sure starts to look like that :/ I haven't found such 
restriction for GL_BYTE reads but that seems to be what the issue is 
about. Will try to find and fix.


> -Nanley
> 
>>
>>
>>> -Nanley
>>>
>>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>>> ---
>>>>    tests/spec/ext_render_snorm/render.c | 32 ++++++++++++++++++++++++++++----
>>>>    1 file changed, 28 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/spec/ext_render_snorm/render.c b/tests/spec/ext_render_snorm/render.c
>>>> index 1d4a69c2d..ad309f17c 100644
>>>> --- a/tests/spec/ext_render_snorm/render.c
>>>> +++ b/tests/spec/ext_render_snorm/render.c
>>>> @@ -82,10 +82,11 @@ static const struct fmt_test {
>>>>    	GLenum iformat;
>>>>    	GLenum base_format;
>>>>    	unsigned bpp;
>>>> +	bool can_read;
>>>>    } tests[] = {
>>>> -	{ GL_R8_SNORM,		GL_RED,		1, },
>>>> -	{ GL_RG8_SNORM,		GL_RG,		2, },
>>>> -	{ GL_RGBA8_SNORM,	GL_RGBA,	4, },
>>>> +	{ GL_R8_SNORM,		GL_RED,		1,	false },
>>>> +	{ GL_RG8_SNORM,		GL_RG,		2,	false },
>>>> +	{ GL_RGBA8_SNORM,	GL_RGBA,	4, 	true  },
>>>>    };
>>>>    static GLuint prog;
>>>> @@ -229,6 +230,25 @@ verify_contents(const struct fmt_test *test)
>>>>    	return result;
>>>>    }
>>>> +static bool
>>>> +verify_contents_float(const struct fmt_test *test)
>>>> +{
>>>> +	/* Setup expected value, alpha is always max in the test. */
>>>> +	char value[4] = { 0, 0, 0, SCHAR_MAX };
>>>> +	value_for_format(test, value);
>>>> +
>>>> +	const float expected[4] = {
>>>> +		value[0] / SCHAR_MAX,
>>>> +		value[1] / SCHAR_MAX,
>>>> +		value[2] / SCHAR_MAX,
>>>> +		value[3] / SCHAR_MAX,
>>>> +	};
>>>> +
>>>> +	bool res = piglit_probe_rect_rgba(0, 0, piglit_width, piglit_height,
>>>> +					  expected);
>>>> +	return res;
>>>> +}
>>>> +
>>>>    static bool
>>>>    test_format(const struct fmt_test *test)
>>>>    {
>>>> @@ -282,13 +302,17 @@ test_format(const struct fmt_test *test)
>>>>    	glDeleteTextures(1, &tmp_tex);
>>>>    	/* Verify contents. */
>>>> -	pass &= verify_contents(test);
>>>> +	if (test->can_read)
>>>> +		pass &= verify_contents(test);
>>>>    	glDeleteFramebuffers(1, &fbo);
>>>>    	/* Render fbo contents to window. */
>>>>    	render_texture(fbo_tex, GL_TEXTURE_2D, 0);
>>>> +	/* Verify window contents. */
>>>> +	pass &= verify_contents_float(test);
>>>> +
>>>>    	piglit_present_results();
>>>>    	glDeleteTextures(1, &fbo_tex);
>>>> -- 
>>>> 2.14.4
>>>>
>>>> _______________________________________________
>>>> Piglit mailing list
>>>> Piglit at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/piglit