[Piglit] [PATCH v2 1/2] Add a piglit_require_GL_version() function.

[Paul Berry]

On 8 October 2011 06:49, Brian Paul <brian.e.paul at gmail.com> wrote:

> On Fri, Oct 7, 2011 at 6:28 PM, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > ---
> >  tests/util/piglit-util.c |   12 ++++++++++++
> >  tests/util/piglit-util.h |    1 +
> >  2 files changed, 13 insertions(+), 0 deletions(-)
> >
> > diff --git a/tests/util/piglit-util.c b/tests/util/piglit-util.c
> > index 358b0dc..b8a0219 100644
> > --- a/tests/util/piglit-util.c
> > +++ b/tests/util/piglit-util.c
> > @@ -108,6 +108,18 @@ bool piglit_is_extension_supported(const char *name)
> >        return found;
> >  }
> >
> > +void piglit_require_GL_version(double required_version)
> > +{
> > +       double gl_version = strtod((char *) glGetString(GL_VERSION),
> NULL);
>
> I don't think we can use strtod().  The OpenGL version string always
> uses '.' while strtod()'s decimal point character is local-dependent.
>
> I think we could get by with something like
>
> const GLUbyte *verStr =  glGetString(GL_VERSION);
> int version = verStr[0] * 10 + verStr[2];
>

Hmm, I did not think of this.  (...goes and learns a lot about locales...)

It turns out that my version works ok even if the computer's default locale
uses ',' as the decimal separator, because the computer's default locale
doesn't take effect until you call setlocale(LC_ALL, ""), which Piglit
doesn't do.  Until then you're always in the "C" locale, which uses '.' as
the decimal separator (see
http://www.chemie.fu-berlin.de/chemnet/use/info/libc/libc_19.html#SEC323).

However, I still like your version better, because (a) you don't have to
research locales to see why it works, and (b) it neatly avoids any issues
with rounding errors.  So I'm going to go with it.

Incidentally, while investigating this stuff I discovered that a lot of the
glean tests don't use ">=" for their version comparisons where they should.
For example, in tshaderapi.cpp, we have this:

    const char *version = (const char *) glGetString(GL_VERSION);
    if (strncmp(version, "2.0", 3) == 0 ||
        strncmp(version, "2.1", 3) == 0 ||
        strncmp(version, "3.0", 3) == 0) {
        return true;
    }
    else {
        env->log << name
                 << ":  skipped.  Requires GL 2.0, 2.1 or 3.0.\n";
        return false;
    }

Which means that the "shaderapi" test gets skipped on a GL implementation
that advertises support for GL version 3.1 or newer.  Worse yet, since the
test doesn't use the standard piglit mechanism to report that it's skipping,
this shows up in the Piglit results as a "pass".  Really this code should be
replaced with:

    piglit_require_GL_version(20);

(taking your suggestion of using an integer to represent ten times the
version rather than a floating-point value).


>
> BTW, I think a pigilt_get_gl_version() function would be useful too.
>
>
It turns out that Piglit already has such a function (I didn't realize this
either until this morning):

    void piglit_get_gl_version(bool *es, int* major, int* minor);

But this type signature is horrible because it means if you want to compare
version numbers you have to jump through hoops:

    /* Are we using GL with a version of at least 3.1? */
    bool es;
    int major;
    int minor;
    piglit_get_gl_version(&es, &major, &minor);
    if (es && (major > 3 || (major == 3 && minor >= 1)))

I would far rather change this to:

    bool piglit_is_gles();
    int piglit_get_gl_version(); /* returns 10*major + minor */

So we could do this:

    if (piglit_is_gles() && piglit_get_gl_version() >= 31)

Fortunately piglit_get_gl_version() is only called from one place right now,
so this should be easy to change.



I'll go ahead and fold your suggestions on piglit_require_GL_version() into
this patch.  I'll submit a separate patch series to fix the problems with
piglit_get_gl_version() and the glean tests.

Paul
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20111008/72516de4/attachment.html>