[Piglit] [Mesa-dev] [PATCH] piglit/general/texunits: fix to check the correct extension and be less pedantic

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Luca Barbieri wrote:
>> Note: I'm moving this discussion to the piglit list.
> Sorry, did not realize there was one (doesn't seem mentioned on the webpage).
> 
>> Neither version of the code is correct.  :(  I suspect the correct thing
>> is to check for either extension, since both add this query.  From the
>> GL_ARB_fragment_program spec:
> 
> Ah right the ARB file lists the non-_ARB-suffixed and the ARB-suffixed
> one separately:
> 	MAX_TEXTURE_COORDS				= 0x8871    # ARB_vertex_shader, ARB_fragment_shader
> 
> However, MAX_COMBINED_TEXTURE_IMAGE_UNITS_ARB still requires
> ARB_vertex_shader as far as I can tell.
> 
>> On which drivers do you see this?  I don't think it's correct.  If
>> completely non-programmable GPU advertises N texture units, the spec is
>> pretty clear that using units beyond N-1 is an error.  I'm not aware of
>> any ambiguity in the spec language here.  Or does this only occur when
>> GL_ARB_vertex_shader is not supported but GL_ARB_fragment_program is?
> 
> On the nv40 Gallium driver without GLSL exposed (but with
> GL_ARB_fragment_program, of course).
> 
> [this is a temporary situation, and GLSL will be introduced soon, but
> this still holds]
> 
> Does the spec really force the implementation to return an error for
> things it does not advertise, even if they are actually supported?

For ActiveTexture, page 44 (57 of the PDF) of the OpenGL 1.5 spec says:

    "Specifying an invalid texture generates the error
    INVALID_ENUM."

The definition of ClientActiveTexture has similar language.

The set of valid textures is defined on page 20 (33 of the PDF):

    "texture is a symbolic constant of the form TEXTUREi, indicating
    that texture coordinate set i is to be modified. The constants obey
    TEXTUREi = TEXTURE0 + i (i is in the range 0 to k − 1, where k is
    the implementation-dependent number of texture units defined
    by MAX_TEXTURE_UNITS)."

GL_ARB_fragment_program changes this language slightly, and the OpenGL
2.0 spec contains similar changes.  Specifically, says
GL_ARB_fragment_program:

    "ActiveTexture generates the error INVALID_ENUM if an invalid
    <texture> is specified.  <texture> is a symbolic constant of the
    form TEXTUREi, indicating that texture unit i is to be modified.
    The constants obey TEXTUREi = TEXTURE0 + i (i is in the range 0 to
    k-1, where k is the larger of the MAX_TEXTURE_COORDS_ARB and
    MAX_TEXTURE_IMAGE_UNITS_ARB).  For compatibility with old OpenGL
    specifications, the implementation-dependent constant
    MAX_TEXTURE_UNITS specifies the number of conventional texture units
    supported by the implementation.  Its value must be no larger than
    the minimum of MAX_TEXTURE_COORDS_ARB and
    MAX_TEXTURE_IMAGE_UNITS_ARB."

The difference in OpenGL 2.0 (or GL_ARB_vertex_shader or
GL_NV_vertex_program3) is that MAX_TEXTURE_IMAGE_UNITS_ARB applies only
to fragment programs / shaders, and MAX_VERTEX_TEXTURE_IMAGE_UNITS_ARB
is added for vertex programs.  MAX_COMBINED_TEXTURE_IMAGE_UNITS_ARB is
added to denote the maximum number of total usable textures.

> The issue here is that we have only 8 texcoords varyings, but 16
> fragment textures.
> The driver/state tracker correctly set the internal Mesa constants,
> which Mesa apparently honors.
> On the other hand, since the extension is not exposed, Mesa decides to
> refuse to give out the constants and raises errors instead.
> 
> Thus, the application only sees GL_MAX_TEXTURE_UNITS = 8, but you can
> use texture 8-15 as well.

Any texture image units or coordinate units beyond the limit of
MAX_TEXTURE_UNITS can't be used by fixed function, and MAX_TEXTURE_UNITS
is the fixed function query.  This is the correct behavior.  Based on
your description, an application should see:

    MAX_TEXTURE_COORDS_ARB = 8
    MAX_TEXTURE_IMAGE_UNITS_ARB = 16
    MAX_TEXTURE_UNITS = 8

> Basically leaving the test as is means forcing Mesa to not support
> textures 8-15 when GLSL is not supported, just because it cannot
> advertise them, which seems stupid.

If you can't advertise it, how can an application use it?

If it's not advertised, and the application can't use it, that's the
definition of *NOT* being supported. :)

> In general the following things seem dubious to me:
> 1. The test checking the presence of a (not very related, except for
> happening to use the functionality) extension instead of whether
> glGetInteger returns an error or not
> 2. Mesa checking the presence of the extension and, seeing it is
> absent, not giving out the constants it does in fact have and raising
> an error instead
> 3. The test checking that errors are triggered for unadvertised
> functionality, as opposed to only checking whether both the advertised
> and the non-error-raising functionality works, or better, leaving that
> to specific tests
> 
> As far as I can see, either fixing (3) (as the current patch does), or
> both (1) and (2) will solve the problem.
> Castrating Mesa to only support  8 textures in this case will too, but
> it doesn't seem great.
> 
> Ideally maybe we should fix all three points.
> 
> Perhaps you could add a third mode that only asks for
> GL_MAX_TEXTURE_COORDS in the "fp but not GLSL case", but then you
> still have the problem for vertex textures (which could be exposed via
> GL_NV_vertex_program3 or some other extension instead of GLSL).

I looked at the test in a bit more detail.  It looks like the only thing
wrong with the test is the query mechanism.  There are two ways forward,
I think.

1. Always do all of the queries.  After each query, check to see of
glGetIntegerv generated an error.  In the error case, do some sensible
fallback.

2. Since it is unlikely there will be more extensions enabling these
queries, just fix the extension tests.  I guess this would look like:

   /* Set some reasonable defaults, and increase some of the values if
    * certain extensions are available.
    */
   glGetIntegerv(GL_MAX_TEXTURE_UNITS, &MaxTextureCoordUnits);
   MaxTextureImageUnits =
   MaxTextureCombinedUnits = MaxTextureCoordUnits;

   if (GLEW_ARB_fragment_program
       || GLEW_ARB_fragment_shader
       || GLEW_VERSION_2_0) {
      glGetIntegerv(GL_MAX_TEXTURE_COORDS,
                    &MaxTextureCoordUnits);
      glGetIntegerv(GL_MAX_TEXTURE_IMAGE_UNITS,
                    &MaxTextureImageUnits);
      MaxTextureCombinedUnits = MaxTextureImageUnits;
   }

   if (GLEW_ARB_vertex_shader || GLEW_VERSION_2_0) {
      /* Strangely, NV_vertex_prrogram3 does not add this query, but it
       * seems like it should.
       */
      glGetIntegerv(GL_MAX_COMBINED_TEXTURE_IMAGE_UNITS,
                    &MaxTextureCombinedUnits);
   }

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkvHkN4ACgkQX1gOwKyEAw8PVACfRg/zAMlCI8q+pruPR7Fdf9Ni
w0AAnR3XFYNgZIfo6/D70I3LTiYnbFS6
=t1dw
-----END PGP SIGNATURE-----