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

[Brian Paul]

On 10/10/2012 01:26 PM, Chad Versace wrote:
> 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.

Huh, I think we just found a bug in glext.h.  I'll double-check a few 
things and report the issue to the ARB.


> 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?

I'm happy with the original patch for the time being.  I was just 
suggesting doing the feature check in the regular GL way.

-Brian