[Mesa-dev] [PATCH] mesa/main: TexImage2DMultisample needs to pass OpenGL3.3 conformance test.

[Timothy Arceri]

On Wed, 2015-11-25 at 17:13 +0200, Tapani Pälli wrote:
> On 11/25/2015 04:00 PM, Predut, Marius wrote:
> > > -----Original Message-----
> > > From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On
> > > Behalf Of
> > > Timothy Arceri
> > > Sent: Wednesday, November 25, 2015 1:12 PM
> > > To: Palli, Tapani; Predut, Marius; mesa-dev at lists.freedesktop.org
> > > Subject: Re: [Mesa-dev] [PATCH] mesa/main: TexImage2DMultisample
> > > needs to pass
> > > OpenGL3.3 conformance test.
> > > 
> > > On Wed, 2015-11-25 at 12:47 +0200, Tapani Pälli wrote:
> > > > Hi;
> > > > 
> > > > On 11/25/2015 01:15 PM, Marius Predut wrote:
> > > > > Open GL 3.3 reference document says:
> > > > > samples must be in the range zero to GL_MAX_TEXTURE_SIZE - 1.
> > > > > Open GL.4 clearly states:
> > > > > An INVALID_VALUE error is generated if samples is zero.
> > > See my comment in bugzilla [1] I believe this is just a bug in
> > > the
> > > reference pages, we implement things in Mesa going by what the
> > > spec
> > > says and the spec says nothing about samples being 0 in the 3.2
> > > spec in
> > > fact it doen't even say anything in the 4.0 spec which you have
> > > changed
> > > the check to.
> > > 
> > > Also the 4.5 reference pages also conflict with the spec so this
> > > is
> > > even more reason I think this change is wrong.
> > > 
> > > [1] https://bugs.freedesktop.org/show_bug.cgi?id=91670
> > > 
> > I don't think it is a bug in specs because in this case also the
> > CTS and the piglit test is wrong:
> > 
> > With this patch 2 things are fixed:
> > 1.Khronos CTS conformance tests for OpenGL 3.3
> > 2. The piglit test 
> > https://bugs.freedesktop.org/show_bug.cgi?id=93100 (Or 
> > https://bugs.freedesktop.org/show_bug.cgi?id=91670)
> > 
> > The patch is based on this spec:
> > https://www.opengl.org/sdk/docs/man3/xhtml/glTexImage3DMultisample.
> > xml
> > 
> > We can't believe or "suppose" something here, the specs need to be
> > as an axioma.
> > Can someone confirm that this reference includes the wrong specs?
> 
> That's not a specification but a manual page. Timothy is pointing to 
> OpenGL specifications (available at www.opengl.org/registry). It is
> true 
> that for example 3.3 Core does not mention this error case which
> means 
> using 0 was allowed there. IMO either we should allow to use 0 (and
> bump 
> it to 1?) when running on 3.x context since it's not forbidden or
> maybe 
> just locally patch this whenever running 3.x conformance.

Hi Marius,

Please provide the test CTS test that you are trying to fix so that we
can take a look at what the CTS is trying to test.

While the OpenGL 3.2 -> 4.2 specs seem to allow zero due simple to not
mentioning it seems odd to me that the CTS would specificly test for
this. As far as I understand it a value of 0 would result in undefined
behaviour, so it doesn't seem right to allow this for these versions of
OpenGL. IMO we should be going with the later specs that fix this
oversight for all versions of GL.

One thing I can see the CTS doing in various tests is querying the GL
implementation for values, its possible we are passing a value of 0
back to the tests somewhere and its trying to used this with the
multisample functions. The only way to know whats going on is if you
tell us which test you are trying to fix.

Tim

> 
> (It seems OpenGL 4.2 is the first spec to state the INVALID_VALUE
> error 
> case for 0.)
> > > > OpenGL ES 3.1 spec also says "An INVALID_VALUE error is
> > > > generated if
> > > > samples is zero.". You'll need to change you check below to
> > > > include
> > > > also
> > > > ES 3.1.
> > > > 
> > > > > Fixing the piglit test case gl-3.2-layered-rendering
> > > > > -framebuffertexture.
> > > > > 
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93100
> > > > > 
> > > > > Signed-off-by: Marius Predut <marius.predut at intel.com>
> > > > > ---
> > > > >    src/mesa/main/teximage.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/src/mesa/main/teximage.c
> > > > > b/src/mesa/main/teximage.c
> > > > > index d9453e3..69634ff 100644
> > > > > --- a/src/mesa/main/teximage.c
> > > > > +++ b/src/mesa/main/teximage.c
> > > > > @@ -5211,7 +5211,7 @@ texture_image_multisample(struct
> > > > > gl_context
> > > > > *ctx, GLuint dims,
> > > > >          return;
> > > > >       }
> > > > > 
> > > > > -   if (samples < 1) {
> > > > > +   if (samples < 1 && ctx->API == API_OPENGL_CORE && ctx
> > > > > ->Version
> > > > > > = 40) {
> > > > >          _mesa_error(ctx, GL_INVALID_VALUE, "%s(samples <
> > > > > 1)",
> > > > > func);
> > > > >          return;
> > > > >       }
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>