[Piglit] [PATCH] piglit-util-gl-common: fix ES1 / ES2 build error

[Chad Versace]

On 10/10/2012 12:00 PM, Brian Paul wrote:
> On 10/10/2012 12:27 PM, Chad Versace wrote:
>> On 10/08/2012 06:23 PM, Brian Paul wrote:
>>> On Mon, Oct 8, 2012 at 6:17 PM, Jordan Justen<jordan.l.justen at intel.com>  wrote:
>>>> Signed-off-by: Jordan Justen<jordan.l.justen at intel.com>
>>>> Cc: Brian Paul<brianp at vmware.com>
>>>> Cc: Chad Versace<chad.versace at linux.intel.com>
>>>> ---
>>>>   tests/util/piglit-util-gl-common.c |    7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/tests/util/piglit-util-gl-common.c
>>>> b/tests/util/piglit-util-gl-common.c
>>>> index 62b5312..4f9fe5f 100644
>>>> --- a/tests/util/piglit-util-gl-common.c
>>>> +++ b/tests/util/piglit-util-gl-common.c
>>>> @@ -447,8 +447,14 @@ piglit_get_compressed_block_size(GLenum format,
>>>>                                   unsigned *bw, unsigned *bh, unsigned *bytes)
>>>>   {
>>>>          switch (format) {
>>>> +#if defined(USE_OPENGL) || defined(USE_OPENGL_ES2)
>>>>          case GL_COMPRESSED_RGB_S3TC_DXT1_EXT:
>>>>          case GL_COMPRESSED_RGBA_S3TC_DXT1_EXT:
>>>> +               *bw = *bh = 4;
>>>> +               *bytes = 8;
>>>> +               return true;
>>>> +#endif
>>>> +#if defined(USE_OPENGL)
>>>>          case GL_COMPRESSED_SRGB_S3TC_DXT1_EXT:
>>>>          case GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT1_EXT:
>>>>          case GL_COMPRESSED_RED_RGTC1:
>>>> @@ -475,6 +481,7 @@ piglit_get_compressed_block_size(GLenum format,
>>>>                  *bh = 4;
>>>>                  *bytes = 16;
>>>>                  return true;
>>>> +#endif
>>>>          default:
>>>>                  /* return something rather than uninitialized values */
>>>>                  *bw = *bh = *bytes = 1;
>>>
>>> Actually, it would probably be better to test the extension #define,
>>> such as GL_EXT_texture_compression_s3tc because off-hand I don't know
>>> which compressed formats are supported by which APIs.
>>
>>
>> Brian,
>>
>> The GL headers on my system don't define the GL_EXT_texture_compression_s3tc
>> feature macro.
>>
>> Since the Piglit build is currently broken, this patch fixes it, and the
>> suggested alternative doesn't work on my system, I've gone ahead and committed
>> the patch. If we later find a better method to filter the enums in this switch,
>> we can apply the better method then.
> 
> That's fine but I think I'm missing something.
> 
> Aren't all the GL_(s3tc)_EXT tokens coming from the generated_dispatch.h file? 
> It looks like that header also has a whole mess of "#define GL_EXT_foo_bar"
> lines.  Shouldn't there be one for GL_EXT_texture_compression_s3tc also?
> 
> Wouldn't everyone have the s3tc-related tokens defined in generated_dispatch.h
> like I do?
> 
> AFAICT, generated_dispatch.[ch] are generated from the glapi.json file.  And
> glapi.json is generated from enumext.spec and that file defines the extension
> tokens above.  So I don't see where the variation in #defined tokens is coming
> from.

I might know where the confusion is coming from.

Support for ES1 and ES2 hasn't been added to piglit-dispatch yet. So,
generated_dispatch.h is only included when building for OpenGL. When building
for ES2, <GLES2/gl2.h> is directly included. Likewise for ES1. So, when building
the ES tests, many of these s3tc tokens are undefined.

Regarding the lack of the `#define GL_EXT_texture_compression_s3tc`...

I've checked both /usr/include/glext.h and generated_dispatch.h, and grep is
unable to find `#define GL_EXT_texture_compression_s3tc` in either. The only
s3tc tokens in those headers are for the texture formats.

I don't understand why generated_dispatch.h contains a whole mess of `#define
$EXT_FEATURE_MACRO` but lacks one for s3tc. Perhaps it special-cased this macro
in order to follow the precedent of glext.h?

Paul, ^^^, is there an explanation for this?

Given that there really is no feature macro for s3tc, then maybe the switch
statement should look something like this:
    #ifdef GL_COMPRESSED_SRGB_S3TC_DXT1_EXT
        case GL_COMPRESSED_SRGB_S3TC_DXT1_EXT:
    #endif
    #ifdef GL_COMPRESSED_RGBA_S3TC_DXT1_EXT:
        case GL_COMPRESSED_SRGBA_S3TC_DXT1_EXT:
    #endif
It's ugly, but it works around the problem of needing to know which formats are
supported by which API's. What do you think?