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

[Nanley Chery]

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?

-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