899855 - GLContext parse GL_VERSION

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Guillaume Abadie]

Parse GL_VERSION till mVersion's patch is landed.

Comment 1

[Guillaume Abadie]

Attachment: patch revision 1

https://tbpl.mozilla.org/?tree=Try&rev=c35ea3affc56

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 787098 [details] [diff] [review]
patch revision 1

Review of attachment 787098 [details] [diff] [review]:
-----------------------------------------------------------------

I recommend writing this such that we use something like:
bool GLContext::ParseGLVersion(int* major, int* minor);
or
static bool ParseGLVersion(GLContext* gl, int* major, int* minor);

Adding more InitFoo() functions tends to be a bit of an antipattern, particularly when this huge function only ends up init-ing two member vars.

::: gfx/gl/GLContext.cpp
@@ +291,5 @@
>  
>      mInitialized = LoadSymbols(&symbols[0], trygl, prefix);
>  
> +    if (mInitialized) {
> +        InitVersion();

Why does it return a bool if we never check it?

::: gfx/gl/GLContextInit.cpp
@@ +11,5 @@
> +namespace mozilla {
> +namespace gl {
> +
> +bool
> +GLContext::InitVersion()

Why do we want a new file for this?
What other functions to do you plan on moving here?

@@ +14,5 @@
> +bool
> +GLContext::InitVersion()
> +{
> +    MOZ_ASSERT(mVersion >= 200,
> +               "GLContext require an OpenGL (ES or not) 2.0 at least.");

This is a little strange, since this is only asserting that our initial guesses were not insane.

@@ +23,5 @@
> +    }
> +
> +    unsigned int version = 0;
> +
> +    {

Please leave a short comment as to why we're using an anonymous scope. ("Anon scope to allow identifier reuse")

@@ +47,5 @@
> +            majorVersion = 0;
> +            minorVersion = 0;
> +        }
> +
> +        version = majorVersion * 100 + minorVersion * 10;

I don't like that we propagate the error status by setting two variables to zero, then setting another variable to false using those two.
I think this would be better as separate function, so we can just abandon that approach if the QueryGLVersion function returns false.

@@ +50,5 @@
> +
> +        version = majorVersion * 100 + minorVersion * 10;
> +    }
> +
> +    if (version == 0) {

!version

@@ +79,5 @@
> +         *
> +         * Note:
> +         *  We don't care about release_number.
> +         */
> +        const char* versionString = (const char*) fGetString(LOCAL_GL_VERSION);

No space between cast and value.
Really this should be reinterpret_cast<>, though. (Maybe const_cast? I forget)

@@ +83,5 @@
> +        const char* versionString = (const char*) fGetString(LOCAL_GL_VERSION);
> +
> +        error = fGetError();
> +        if (error != LOCAL_GL_NO_ERROR) {
> +            NS_ERROR("glGetString(GL_VERSION) has generated an error");

Is this ever allowed to happen? Under which conditions might this generate an error?
It seems like we should just assert that there is no error.

@@ +86,5 @@
> +        if (error != LOCAL_GL_NO_ERROR) {
> +            NS_ERROR("glGetString(GL_VERSION) has generated an error");
> +            return false;
> +        } else if (!versionString) {
> +            NS_ERROR("glGetString(GL_VERSION) has returned 0");

We should assert this isn't true, unless we know of an implementation which does this.
I'm not confident enough to assume all implementations do this, but we should assert this, so we don't get bitten by it locally.

@@ +91,5 @@
> +            return false;
> +        }
> +
> +#ifdef DEBUG
> +        printf_stderr("parsing context's GL_VERSION = %s\n", versionString);

Either remove this DEBUG spew, or put it behind DebugMode().

@@ +94,5 @@
> +#ifdef DEBUG
> +        printf_stderr("parsing context's GL_VERSION = %s\n", versionString);
> +#endif
> +
> +        static const char* kGLESVersionPrefix = "OpenGL ES ";

Just do `const char kGLESVersionPrefix[] = `

@@ +95,5 @@
> +        printf_stderr("parsing context's GL_VERSION = %s\n", versionString);
> +#endif
> +
> +        static const char* kGLESVersionPrefix = "OpenGL ES ";
> +        bool isGLES = memcmp(versionString, kGLESVersionPrefix, strlen(kGLESVersionPrefix)) == 0;

It's probably unlikely, but `versionString` could be shorter than `kGLESVersionPrefix`. We rely on `versionString` being a proper c-string already, so we should just use `strncmp`.

@@ +98,5 @@
> +        static const char* kGLESVersionPrefix = "OpenGL ES ";
> +        bool isGLES = memcmp(versionString, kGLESVersionPrefix, strlen(kGLESVersionPrefix)) == 0;
> +
> +        if ((mProfile == ContextProfile::OpenGLES) != isGLES) {
> +            NS_ERROR("mismatched OpenGL context profile");

Basically, all these NS_ERRORS should be asserts. If we start finding that we're hitting these asserts under reasonable conditions, *then* we should reduce them to NS_ERRORs or NS_WARNINGs.

@@ +105,5 @@
> +            /**
> +             * We skip the "OpenGL ES " prefix to directly go on "N.M [...]"
> +             * like in OpenGL
> +             */
> +            versionString += strlen(kGLESVersionPrefix);

I would like this better if it were immediately after when we check whether the string is for GLES.

@@ +110,5 @@
> +        }
> +
> +        /**
> +         * We don't care about the release number.
> +         */

Why does this comment take three lines?

@@ +122,5 @@
> +                /**
> +                 * This character is a digit.
> +                 *  => We update numberArr[numberId]
> +                 */
> +                numberArr[numberId] = numberArr[numberId] * 10 + (unsigned int) (c - '0');

It's probably much easier and cleaner to use this: (which I only actually just found while looking at `atoi`)
http://www.cplusplus.com/reference/cstdlib/strtol/ (Though I guess we might actually want `strtoul`)

const char* itr = versionString;
char* end = nullptr;
int major = strtol(itr, &end, 10);
if (!end) {
  MOZ_ASSERT(false, "Failed to parse the GL major version number.");
  return false;
}
if (*end != '.') {
  MOZ_ASSERT(false, "Failed to parse GL's major-minor version number separator.");
  return false;
}

itr = end + 1;
end = nullptr;
int minor = strtol(itr, &end, 10);
if (!end) {
  MOZ_ASSERT(false, "Failed to parse GL's minor version number.");
  return false;
}

version = 100*major + 10*minor;


We also want to print the version string we failed to parse, when we do fail.
It would be nice if we parsed the release version number if it's present, though we can do that in a follow-up.

@@ +144,5 @@
> +    }
> +
> +#ifdef DEBUG
> +    printf_stderr("GLContext::mVersion == %u\n", mVersion);
> +    printf_stderr("context version     == %u\n", version);

Remove debug spew.
Also, I believe we generally do "Foo: %d" or similar for printing the value of variables. IMO, otherwise, "foo == %d" looks like a comparison check, and "foo = %d" might be an assignment. Colon is less ambiguous.

@@ +148,5 @@
> +    printf_stderr("context version     == %u\n", version);
> +#endif
> +
> +    if (version < mVersion) {
> +        NS_ERROR("The GLContext version.");

MOZ_ASSERT(false, "Parsed GL version is less than our minimal estimate.");

@@ +154,5 @@
> +    }
> +
> +    /**
> +     * We update mVersion if we are lucky enought to get a more recent OpenGL
> +     * context than expected.

Rather, more recent, or equally recent.
We really update it whenever we didn't get something *less* than our estimate.

Comment 3

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 787098 [details] [diff] [review]
> patch revision 1
> 
> Review of attachment 787098 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I recommend writing this such that we use something like:
> bool GLContext::ParseGLVersion(int* major, int* minor);
> or
> static bool ParseGLVersion(GLContext* gl, int* major, int* minor);
> 
> Adding more InitFoo() functions tends to be a bit of an antipattern,
> particularly when this huge function only ends up init-ing two member vars.
ParseGLVersion's caller would also have some work to do, while InitVersion is doing everything needed about the OpenGL context version, and just say : I succeed, or I failed
> 
> ::: gfx/gl/GLContext.cpp
> @@ +291,5 @@
> >  
> >      mInitialized = LoadSymbols(&symbols[0], trygl, prefix);
> >  
> > +    if (mInitialized) {
> > +        InitVersion();
> 
> Why does it return a bool if we never check it?
Oups... I've just forgotten the most important...
> 
> ::: gfx/gl/GLContextInit.cpp
> @@ +11,5 @@
> > +namespace mozilla {
> > +namespace gl {
> > +
> > +bool
> > +GLContext::InitVersion()
> 
> Why do we want a new file for this?
> What other functions to do you plan on moving here?
GLContext.cpp is to big, And I was thinking about moving GLContext::InitWithPrefix (that might be rename LoadSymbols by the sameway) into it.
> 
> @@ +14,5 @@
> > +bool
> > +GLContext::InitVersion()
> > +{
> > +    MOZ_ASSERT(mVersion >= 200,
> > +               "GLContext require an OpenGL (ES or not) 2.0 at least.");
> 
> This is a little strange, since this is only asserting that our initial
> guesses were not insane.
Why not? I don't understand where the matter is.
> 
> @@ +23,5 @@
> > +    }
> > +
> > +    unsigned int version = 0;
> > +
> > +    {
> 
> Please leave a short comment as to why we're using an anonymous scope.
> ("Anon scope to allow identifier reuse")
Oh...
> 
> @@ +47,5 @@
> > +            majorVersion = 0;
> > +            minorVersion = 0;
> > +        }
> > +
> > +        version = majorVersion * 100 + minorVersion * 10;
> 
> I don't like that we propagate the error status by setting two variables to
> zero, then setting another variable to false using those two.
> I think this would be better as separate function, so we can just abandon
> that approach if the QueryGLVersion function returns false.
Why? While this is the only place that it's done.
> 
> @@ +50,5 @@
> > +
> > +        version = majorVersion * 100 + minorVersion * 10;
> > +    }
> > +
> > +    if (version == 0) {
> 
> !version
Oups...
> 
> @@ +79,5 @@
> > +         *
> > +         * Note:
> > +         *  We don't care about release_number.
> > +         */
> > +        const char* versionString = (const char*) fGetString(LOCAL_GL_VERSION);
> 
> No space between cast and value.
> Really this should be reinterpret_cast<>, though. (Maybe const_cast? I
> forget)
They would do exactly same thing in this case.
> 
> @@ +83,5 @@
> > +        const char* versionString = (const char*) fGetString(LOCAL_GL_VERSION);
> > +
> > +        error = fGetError();
> > +        if (error != LOCAL_GL_NO_ERROR) {
> > +            NS_ERROR("glGetString(GL_VERSION) has generated an error");
> 
> Is this ever allowed to happen? Under which conditions might this generate
> an error?
> It seems like we should just assert that there is no error.
The assert would not be in release code. I want this parser to prevent any driver bugs.
> 
> @@ +86,5 @@
> > +        if (error != LOCAL_GL_NO_ERROR) {
> > +            NS_ERROR("glGetString(GL_VERSION) has generated an error");
> > +            return false;
> > +        } else if (!versionString) {
> > +            NS_ERROR("glGetString(GL_VERSION) has returned 0");
> 
> We should assert this isn't true, unless we know of an implementation which
> does this.
> I'm not confident enough to assume all implementations do this, but we
> should assert this, so we don't get bitten by it locally.
No, the assertion would be remove in release code, and it's much better to fail the parser instead of a crash by accessing value at address 0 by followed operations.
> 
> @@ +91,5 @@
> > +            return false;
> > +        }
> > +
> > +#ifdef DEBUG
> > +        printf_stderr("parsing context's GL_VERSION = %s\n", versionString);
> 
> Either remove this DEBUG spew, or put it behind DebugMode().
Sorry, this is temporary code for debugging on push to try. That will be removed in the landed patch for sure.
> 
> @@ +94,5 @@
> > +#ifdef DEBUG
> > +        printf_stderr("parsing context's GL_VERSION = %s\n", versionString);
> > +#endif
> > +
> > +        static const char* kGLESVersionPrefix = "OpenGL ES ";
> 
> Just do `const char kGLESVersionPrefix[] = `
Ok.
> 
> @@ +95,5 @@
> > +        printf_stderr("parsing context's GL_VERSION = %s\n", versionString);
> > +#endif
> > +
> > +        static const char* kGLESVersionPrefix = "OpenGL ES ";
> > +        bool isGLES = memcmp(versionString, kGLESVersionPrefix, strlen(kGLESVersionPrefix)) == 0;
> 
> It's probably unlikely, but `versionString` could be shorter than
> `kGLESVersionPrefix`. We rely on `versionString` being a proper c-string
> already, so we should just use `strncmp`.
True, but if versionString is shorter, memcmp would find at least the '\0' that is not supposed to be there. Fixed.
> 
> @@ +98,5 @@
> > +        static const char* kGLESVersionPrefix = "OpenGL ES ";
> > +        bool isGLES = memcmp(versionString, kGLESVersionPrefix, strlen(kGLESVersionPrefix)) == 0;
> > +
> > +        if ((mProfile == ContextProfile::OpenGLES) != isGLES) {
> > +            NS_ERROR("mismatched OpenGL context profile");
> 
> Basically, all these NS_ERRORS should be asserts. If we start finding that
> we're hitting these asserts under reasonable conditions, *then* we should
> reduce them to NS_ERRORs or NS_WARNINGs.
No, in case of an OpenGL ES driver would not have the exact "OpenGL ES " prefix.
> 
> @@ +105,5 @@
> > +            /**
> > +             * We skip the "OpenGL ES " prefix to directly go on "N.M [...]"
> > +             * like in OpenGL
> > +             */
> > +            versionString += strlen(kGLESVersionPrefix);
> 
> I would like this better if it were immediately after when we check whether
> the string is for GLES.
I don't understand... It is!
> 
> @@ +110,5 @@
> > +        }
> > +
> > +        /**
> > +         * We don't care about the release number.
> > +         */
> 
> Why does this comment take three lines?
Oups...
> 
> @@ +122,5 @@
> > +                /**
> > +                 * This character is a digit.
> > +                 *  => We update numberArr[numberId]
> > +                 */
> > +                numberArr[numberId] = numberArr[numberId] * 10 + (unsigned int) (c - '0');
> 
> It's probably much easier and cleaner to use this: (which I only actually
> just found while looking at `atoi`)
> http://www.cplusplus.com/reference/cstdlib/strtol/ (Though I guess we might
> actually want `strtoul`)
> 
> const char* itr = versionString;
> char* end = nullptr;
> int major = strtol(itr, &end, 10);
> if (!end) {
>   MOZ_ASSERT(false, "Failed to parse the GL major version number.");
>   return false;
> }
> if (*end != '.') {
>   MOZ_ASSERT(false, "Failed to parse GL's major-minor version number
> separator.");
>   return false;
> }
> 
> itr = end + 1;
> end = nullptr;
> int minor = strtol(itr, &end, 10);
> if (!end) {
>   MOZ_ASSERT(false, "Failed to parse GL's minor version number.");
>   return false;
> }
> 
> version = 100*major + 10*minor;
> 
> 
> We also want to print the version string we failed to parse, when we do fail.
> It would be nice if we parsed the release version number if it's present,
> though we can do that in a follow-up.
True, but I prefer my way, because you don't need any extra information about how it works. And your code is failing if you don't get the minor version, where mine can handle that case.
> 
> @@ +144,5 @@
> > +    }
> > +
> > +#ifdef DEBUG
> > +    printf_stderr("GLContext::mVersion == %u\n", mVersion);
> > +    printf_stderr("context version     == %u\n", version);
> 
> Remove debug spew.
> Also, I believe we generally do "Foo: %d" or similar for printing the value
> of variables. IMO, otherwise, "foo == %d" looks like a comparison check, and
> "foo = %d" might be an assignment. Colon is less ambiguous.
This only for debugging on push to try.
> 
> @@ +148,5 @@
> > +    printf_stderr("context version     == %u\n", version);
> > +#endif
> > +
> > +    if (version < mVersion) {
> > +        NS_ERROR("The GLContext version.");
> 
> MOZ_ASSERT(false, "Parsed GL version is less than our minimal estimate.");
Fixed.
> 
> @@ +154,5 @@
> > +    }
> > +
> > +    /**
> > +     * We update mVersion if we are lucky enought to get a more recent OpenGL
> > +     * context than expected.
> 
> Rather, more recent, or equally recent.
> We really update it whenever we didn't get something *less* than our
> estimate.
Fixed.

Comment 4

[Guillaume Abadie]

Attachment: patch revision 2

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Guillaume Abadie from comment #3)
> (In reply to Jeff Gilbert [:jgilbert] from comment #2)
> > Comment on attachment 787098 [details] [diff] [review]
> > patch revision 1
> > 
> > Review of attachment 787098 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I recommend writing this such that we use something like:
> > bool GLContext::ParseGLVersion(int* major, int* minor);
> > or
> > static bool ParseGLVersion(GLContext* gl, int* major, int* minor);
> > 
> > Adding more InitFoo() functions tends to be a bit of an antipattern,
> > particularly when this huge function only ends up init-ing two member vars.
> ParseGLVersion's caller would also have some work to do, while InitVersion
> is doing everything needed about the OpenGL context version, and just say :
> I succeed, or I failed
That's the thing. Functions are better if they do fewer things. That's why it would be better to have a function that *just* tries to parse the string, and then use the result of that function to set our version, if we want to.

We'd be separating parse-the-string from update-version-maybe. It's easier to maintain functions which don't do as much. Function decomposition is useful even if it doesn't make things smaller.

With ParseGLVersion, we could have a very obvious snippet of code in GL's main Init function:
int version = 0;
if (ParseGLVersion(this, &version)) {
  if (version >= mVersion) {
    MOZ_ASSERT(false, "Parsed version less than expected.");
    mVersion = version;
  }
}

This way, we separate the logic of how we deal with the parsed version number from the mechanics of actually parsing it.
    
> > 
> > ::: gfx/gl/GLContext.cpp
> > @@ +291,5 @@
> > >  
> > >      mInitialized = LoadSymbols(&symbols[0], trygl, prefix);
> > >  
> > > +    if (mInitialized) {
> > > +        InitVersion();
> > 
> > Why does it return a bool if we never check it?
> Oups... I've just forgotten the most important...
> > 
> > ::: gfx/gl/GLContextInit.cpp
> > @@ +11,5 @@
> > > +namespace mozilla {
> > > +namespace gl {
> > > +
> > > +bool
> > > +GLContext::InitVersion()
> > 
> > Why do we want a new file for this?
> > What other functions to do you plan on moving here?
> GLContext.cpp is to big, And I was thinking about moving
> GLContext::InitWithPrefix (that might be rename LoadSymbols by the sameway)
> into it.
A decent idea, but save it for later patches, otherwise we'll end up with things all over the place for half-forgotten reasons.
> > 
> > @@ +47,5 @@
> > > +            majorVersion = 0;
> > > +            minorVersion = 0;
> > > +        }
> > > +
> > > +        version = majorVersion * 100 + minorVersion * 10;
> > 
> > I don't like that we propagate the error status by setting two variables to
> > zero, then setting another variable to false using those two.
> > I think this would be better as separate function, so we can just abandon
> > that approach if the QueryGLVersion function returns false.
> Why? While this is the only place that it's done.
Just because we only do something once doesn't mean it wouldn't benefit from function decomposition. Especially given the use of an anon-scope here, this is practically asking to be its own function. It would also improve readability, and narrow the scope of the two functions.
> > 
> > @@ +83,5 @@
> > > +        const char* versionString = (const char*) fGetString(LOCAL_GL_VERSION);
> > > +
> > > +        error = fGetError();
> > > +        if (error != LOCAL_GL_NO_ERROR) {
> > > +            NS_ERROR("glGetString(GL_VERSION) has generated an error");
> > 
> > Is this ever allowed to happen? Under which conditions might this generate
> > an error?
> > It seems like we should just assert that there is no error.
> The assert would not be in release code. I want this parser to prevent any
> driver bugs.
Do that, but use asserts instead of errors. No one sees errors, so we'd end up ignoring them.
We can both be safe in release code, and assert in debug code.
> > 
> > @@ +86,5 @@
> > > +        if (error != LOCAL_GL_NO_ERROR) {
> > > +            NS_ERROR("glGetString(GL_VERSION) has generated an error");
> > > +            return false;
> > > +        } else if (!versionString) {
> > > +            NS_ERROR("glGetString(GL_VERSION) has returned 0");
> > 
> > We should assert this isn't true, unless we know of an implementation which
> > does this.
> > I'm not confident enough to assume all implementations do this, but we
> > should assert this, so we don't get bitten by it locally.
> No, the assertion would be remove in release code, and it's much better to
> fail the parser instead of a crash by accessing value at address 0 by
> followed operations.
I want an assert to be added, not to replace this with an assert. :)
> > 
> > @@ +98,5 @@
> > > +        static const char* kGLESVersionPrefix = "OpenGL ES ";
> > > +        bool isGLES = memcmp(versionString, kGLESVersionPrefix, strlen(kGLESVersionPrefix)) == 0;
> > > +
> > > +        if ((mProfile == ContextProfile::OpenGLES) != isGLES) {
> > > +            NS_ERROR("mismatched OpenGL context profile");
> > 
> > Basically, all these NS_ERRORS should be asserts. If we start finding that
> > we're hitting these asserts under reasonable conditions, *then* we should
> > reduce them to NS_ERRORs or NS_WARNINGs.
> No, in case of an OpenGL ES driver would not have the exact "OpenGL ES "
> prefix.
Have you encountered this yet? If not, let's assert until we do.
> > 
> > @@ +105,5 @@
> > > +            /**
> > > +             * We skip the "OpenGL ES " prefix to directly go on "N.M [...]"
> > > +             * like in OpenGL
> > > +             */
> > > +            versionString += strlen(kGLESVersionPrefix);
> > 
> > I would like this better if it were immediately after when we check whether
> > the string is for GLES.
> I don't understand... It is!
It's about 10 lines and two branches away! That's hardly immediate. :)
What would be better, in my opinion, is:
bool isGLES = false;
if (strncmp(versionString, kGLESVersionPrefix, strlen(kGLESVersionPrefix)) == 0) {
  versionString += strlen(kGLESVersionPrefix;
  isGLES = true;
}

if (isGLES) {
  //etc.
}
> > 
> > @@ +110,5 @@
> > > +        }
> > > +
> > > +        /**
> > > +         * We don't care about the release number.
> > > +         */
> > 
> > Why does this comment take three lines?
> Oups...
> > 
> > @@ +122,5 @@
> > > +                /**
> > > +                 * This character is a digit.
> > > +                 *  => We update numberArr[numberId]
> > > +                 */
> > > +                numberArr[numberId] = numberArr[numberId] * 10 + (unsigned int) (c - '0');
> > 
> > It's probably much easier and cleaner to use this: (which I only actually
> > just found while looking at `atoi`)
> > http://www.cplusplus.com/reference/cstdlib/strtol/ (Though I guess we might
> > actually want `strtoul`)
> > 
> > const char* itr = versionString;
> > char* end = nullptr;
> > int major = strtol(itr, &end, 10);
> > if (!end) {
> >   MOZ_ASSERT(false, "Failed to parse the GL major version number.");
> >   return false;
> > }
> > if (*end != '.') {
> >   MOZ_ASSERT(false, "Failed to parse GL's major-minor version number
> > separator.");
> >   return false;
> > }
> > 
> > itr = end + 1;
> > end = nullptr;
> > int minor = strtol(itr, &end, 10);
> > if (!end) {
> >   MOZ_ASSERT(false, "Failed to parse GL's minor version number.");
> >   return false;
> > }
> > 
> > version = 100*major + 10*minor;
> > 
> > 
> > We also want to print the version string we failed to parse, when we do fail.
> > It would be nice if we parsed the release version number if it's present,
> > though we can do that in a follow-up.
> True, but I prefer my way, because you don't need any extra information
> about how it works. And your code is failing if you don't get the minor
> version, where mine can handle that case.
To fix the 'extra information' thing, just add a short comment on what strtoul does, since it's not that complicated. We could also always pull out strtoul into `bool ParseUInt(const char* src, uint* result)` and make it really obvious. Really though, it's in <cstring>, so just let people check the docs.

Avoiding using existing functions (especially std:: functions) should not be a goal. Rewriting `strcmp` only takes two or three lines, but in rewriting these lines, we increase the chance of bugs, and require each reader to understand what we're doing. (instead of just checking the docs for the well-defined behavior of these functions)

I'm not putting my suggestion out there as 'do this', but rather 'look at how this could also be done'. It's trivial to make either support optional minor versions, or even release versions, but the point wasn't correctness so much as style. The important differences are not how they differ (or not) functionally, but readable and robust they each are.  By decomposing something into parts, it's easier to say with confidence 'this will work', instead of the just 'well, it looks like this should work'.

Anyways, these are increasingly fine-grained nits, but code is read hundreds of times for each time its written, so it's worth the time spent to make functional code look and read as well as it performs, and to ask questions like 'can this be made simpler?'. Sometimes it can't, but this block seems like a good candidate. I can still review your version, I just wanted to put this alternative form out there.

Comment 6

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> (In reply to Guillaume Abadie from comment #3)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #2)
> > > Comment on attachment 787098 [details] [diff] [review]
> > > patch revision 1
> > > 
> > > Review of attachment 787098 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I recommend writing this such that we use something like:
> > > bool GLContext::ParseGLVersion(int* major, int* minor);
> > > or
> > > static bool ParseGLVersion(GLContext* gl, int* major, int* minor);
> > > 
> > > Adding more InitFoo() functions tends to be a bit of an antipattern,
> > > particularly when this huge function only ends up init-ing two member vars.
> > ParseGLVersion's caller would also have some work to do, while InitVersion
> > is doing everything needed about the OpenGL context version, and just say :
> > I succeed, or I failed
> That's the thing. Functions are better if they do fewer things. That's why
> it would be better to have a function that *just* tries to parse the string,
> and then use the result of that function to set our version, if we want to.
> 
> We'd be separating parse-the-string from update-version-maybe. It's easier
> to maintain functions which don't do as much. Function decomposition is
> useful even if it doesn't make things smaller.
> 
> With ParseGLVersion, we could have a very obvious snippet of code in GL's
> main Init function:
> int version = 0;
> if (ParseGLVersion(this, &version)) {
>   if (version >= mVersion) {
>     MOZ_ASSERT(false, "Parsed version less than expected.");
>     mVersion = version;
>   }
> }
> 
> This way, we separate the logic of how we deal with the parsed version
> number from the mechanics of actually parsing it.
Fair enough, I will do it.
>     
> > > 
> > > ::: gfx/gl/GLContext.cpp
> > > @@ +291,5 @@
> > > >  
> > > >      mInitialized = LoadSymbols(&symbols[0], trygl, prefix);
> > > >  
> > > > +    if (mInitialized) {
> > > > +        InitVersion();
> > > 
> > > Why does it return a bool if we never check it?
> > Oups... I've just forgotten the most important...
> > > 
> > > ::: gfx/gl/GLContextInit.cpp
> > > @@ +11,5 @@
> > > > +namespace mozilla {
> > > > +namespace gl {
> > > > +
> > > > +bool
> > > > +GLContext::InitVersion()
> > > 
> > > Why do we want a new file for this?
> > > What other functions to do you plan on moving here?
> > GLContext.cpp is to big, And I was thinking about moving
> > GLContext::InitWithPrefix (that might be rename LoadSymbols by the sameway)
> > into it.
> A decent idea, but save it for later patches, otherwise we'll end up with
> things all over the place for half-forgotten reasons.
Ok.
> > > 
> > > @@ +47,5 @@
> > > > +            majorVersion = 0;
> > > > +            minorVersion = 0;
> > > > +        }
> > > > +
> > > > +        version = majorVersion * 100 + minorVersion * 10;
> > > 
> > > I don't like that we propagate the error status by setting two variables to
> > > zero, then setting another variable to false using those two.
> > > I think this would be better as separate function, so we can just abandon
> > > that approach if the QueryGLVersion function returns false.
> > Why? While this is the only place that it's done.
> Just because we only do something once doesn't mean it wouldn't benefit from
> function decomposition. Especially given the use of an anon-scope here, this
> is practically asking to be its own function. It would also improve
> readability, and narrow the scope of the two functions.
> > > 
> > > @@ +83,5 @@
> > > > +        const char* versionString = (const char*) fGetString(LOCAL_GL_VERSION);
> > > > +
> > > > +        error = fGetError();
> > > > +        if (error != LOCAL_GL_NO_ERROR) {
> > > > +            NS_ERROR("glGetString(GL_VERSION) has generated an error");
> > > 
> > > Is this ever allowed to happen? Under which conditions might this generate
> > > an error?
> > > It seems like we should just assert that there is no error.
> > The assert would not be in release code. I want this parser to prevent any
> > driver bugs.
> Do that, but use asserts instead of errors. No one sees errors, so we'd end
> up ignoring them.
> We can both be safe in release code, and assert in debug code.
> > > 
> > > @@ +86,5 @@
> > > > +        if (error != LOCAL_GL_NO_ERROR) {
> > > > +            NS_ERROR("glGetString(GL_VERSION) has generated an error");
> > > > +            return false;
> > > > +        } else if (!versionString) {
> > > > +            NS_ERROR("glGetString(GL_VERSION) has returned 0");
> > > 
> > > We should assert this isn't true, unless we know of an implementation which
> > > does this.
> > > I'm not confident enough to assume all implementations do this, but we
> > > should assert this, so we don't get bitten by it locally.
> > No, the assertion would be remove in release code, and it's much better to
> > fail the parser instead of a crash by accessing value at address 0 by
> > followed operations.
> I want an assert to be added, not to replace this with an assert. :)
Oh ok! I agree so.
> > > 
> > > @@ +98,5 @@
> > > > +        static const char* kGLESVersionPrefix = "OpenGL ES ";
> > > > +        bool isGLES = memcmp(versionString, kGLESVersionPrefix, strlen(kGLESVersionPrefix)) == 0;
> > > > +
> > > > +        if ((mProfile == ContextProfile::OpenGLES) != isGLES) {
> > > > +            NS_ERROR("mismatched OpenGL context profile");
> > > 
> > > Basically, all these NS_ERRORS should be asserts. If we start finding that
> > > we're hitting these asserts under reasonable conditions, *then* we should
> > > reduce them to NS_ERRORs or NS_WARNINGs.
> > No, in case of an OpenGL ES driver would not have the exact "OpenGL ES "
> > prefix.
> Have you encountered this yet? If not, let's assert until we do.
If you insist, ok! But the assert would be removed from the release code...
> > > 
> > > @@ +105,5 @@
> > > > +            /**
> > > > +             * We skip the "OpenGL ES " prefix to directly go on "N.M [...]"
> > > > +             * like in OpenGL
> > > > +             */
> > > > +            versionString += strlen(kGLESVersionPrefix);
> > > 
> > > I would like this better if it were immediately after when we check whether
> > > the string is for GLES.
> > I don't understand... It is!
> It's about 10 lines and two branches away! That's hardly immediate. :)
> What would be better, in my opinion, is:
> bool isGLES = false;
> if (strncmp(versionString, kGLESVersionPrefix, strlen(kGLESVersionPrefix))
> == 0) {
>   versionString += strlen(kGLESVersionPrefix;
>   isGLES = true;
> }
> 
> if (isGLES) {
>   //etc.
> }
Oh I see.
> > > 
> > > @@ +110,5 @@
> > > > +        }
> > > > +
> > > > +        /**
> > > > +         * We don't care about the release number.
> > > > +         */
> > > 
> > > Why does this comment take three lines?
> > Oups...
> > > 
> > > @@ +122,5 @@
> > > > +                /**
> > > > +                 * This character is a digit.
> > > > +                 *  => We update numberArr[numberId]
> > > > +                 */
> > > > +                numberArr[numberId] = numberArr[numberId] * 10 + (unsigned int) (c - '0');
> > > 
> > > It's probably much easier and cleaner to use this: (which I only actually
> > > just found while looking at `atoi`)
> > > http://www.cplusplus.com/reference/cstdlib/strtol/ (Though I guess we might
> > > actually want `strtoul`)
> > > 
> > > const char* itr = versionString;
> > > char* end = nullptr;
> > > int major = strtol(itr, &end, 10);
> > > if (!end) {
> > >   MOZ_ASSERT(false, "Failed to parse the GL major version number.");
> > >   return false;
> > > }
> > > if (*end != '.') {
> > >   MOZ_ASSERT(false, "Failed to parse GL's major-minor version number
> > > separator.");
> > >   return false;
> > > }
> > > 
> > > itr = end + 1;
> > > end = nullptr;
> > > int minor = strtol(itr, &end, 10);
> > > if (!end) {
> > >   MOZ_ASSERT(false, "Failed to parse GL's minor version number.");
> > >   return false;
> > > }
> > > 
> > > version = 100*major + 10*minor;
> > > 
> > > 
> > > We also want to print the version string we failed to parse, when we do fail.
> > > It would be nice if we parsed the release version number if it's present,
> > > though we can do that in a follow-up.
> > True, but I prefer my way, because you don't need any extra information
> > about how it works. And your code is failing if you don't get the minor
> > version, where mine can handle that case.
> To fix the 'extra information' thing, just add a short comment on what
> strtoul does, since it's not that complicated. We could also always pull out
> strtoul into `bool ParseUInt(const char* src, uint* result)` and make it
> really obvious. Really though, it's in <cstring>, so just let people check
> the docs.
> 
> Avoiding using existing functions (especially std:: functions) should not be
> a goal. Rewriting `strcmp` only takes two or three lines, but in rewriting
> these lines, we increase the chance of bugs, and require each reader to
> understand what we're doing. (instead of just checking the docs for the
> well-defined behavior of these functions)
> 
> I'm not putting my suggestion out there as 'do this', but rather 'look at
> how this could also be done'. It's trivial to make either support optional
> minor versions, or even release versions, but the point wasn't correctness
> so much as style. The important differences are not how they differ (or not)
> functionally, but readable and robust they each are.  By decomposing
> something into parts, it's easier to say with confidence 'this will work',
> instead of the just 'well, it looks like this should work'.
> 
> Anyways, these are increasingly fine-grained nits, but code is read hundreds
> of times for each time its written, so it's worth the time spent to make
> functional code look and read as well as it performs, and to ask questions
> like 'can this be made simpler?'. Sometimes it can't, but this block seems
> like a good candidate. I can still review your version, I just wanted to put
> this alternative form out there.
Ok ok... My mind is we don't care about "release versions" in the parser, because we would not have it if it's an OpenGL 3.1 or OpenGL ES 3.0 anyway because using GL_{MAJOR,MINOR)_VERSION. So doesn't make sense to parse it "sometimes".

Comment 7

[Guillaume Abadie]

Attachment: patch revision 3

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 787823 [details] [diff] [review]
patch revision 3

Review of attachment 787823 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContext.cpp
@@ +134,5 @@
> +ParseGLVersion(GLContext* gl, unsigned int* version)
> +{
> +    GLenum error = gl->fGetError();
> +    if (error != LOCAL_GL_NO_ERROR) {
> +        return false;

Also assert here, since this means we've *really* messed up, previous to this.

@@ +192,5 @@
> +    const char* versionString = (const char*)gl->fGetString(LOCAL_GL_VERSION);
> +
> +    error = gl->fGetError();
> +    if (error != LOCAL_GL_NO_ERROR) {
> +        MOZ_CRASH("glGetString(GL_VERSION) has generated an error");

MOZ_CRASH is both debug and release builds. You want MOZ_ASSERT(false, _).

@@ +195,5 @@
> +    if (error != LOCAL_GL_NO_ERROR) {
> +        MOZ_CRASH("glGetString(GL_VERSION) has generated an error");
> +        return false;
> +    } else if (!versionString) {
> +        MOZ_CRASH("glGetString(GL_VERSION) has returned 0");

s/MOZ_CRASH(/MOZ_ASSERT(false, /
More cases below.

@@ +204,5 @@
> +    // DO NOT LAND WITH THAT, ONLY FOR DEBUGGING ON PUSH TO TRY
> +    printf_stderr("parsing context's GL_VERSION = %s\n", versionString);
> +#endif
> +
> +    static const char kGLESVersionPrefix[] = "OpenGL ES ";

Drop `static` since it's not necessary, and we want to keep the number of static vars to a minimum.

@@ +206,5 @@
> +#endif
> +
> +    static const char kGLESVersionPrefix[] = "OpenGL ES ";
> +    bool isGLES = false;
> +

I would not include this newline, since the final value of `isGLES` is actually determined by whether or not we hit the next branch.

@@ +214,5 @@
> +    }
> +
> +    const char* itr = versionString;
> +    char* end = nullptr;
> +    long int majorVersion = strtol(itr, &end, 10);

I would probably just do:
int majorVersion = (int)strtol...

@@ +234,5 @@
> +        return false;
> +    }
> +
> +    if (majorVersion <= 0 || majorVersion >= 100) {
> +        MOZ_CRASH("Wrong major version.");

s/Wrong/Invalid/

@@ +236,5 @@
> +
> +    if (majorVersion <= 0 || majorVersion >= 100) {
> +        MOZ_CRASH("Wrong major version.");
> +        return false;
> +    } else if (majorVersion < 0 || majorVersion >= 10) {

s/major/minor/

@@ +407,5 @@
>  
> +    if (mInitialized) {
> +        unsigned int version = 0;
> +
> +        mInitialized &= ParseGLVersion(this, &version);

&&=

@@ +416,5 @@
> +#endif
> +
> +        if (version < mVersion) {
> +            MOZ_CRASH("Parsed version less than expected.");
> +            mInitialized = false;

I would not fail us in this case, but rather continue and rely our guess for the version, and on the extensions we have available. Otherwise, this will cause all GL to fail on all hardware with invalid version strings. I'm pretty sure this is rare, but also not confident it won't happen on at least one device in the wild.

Comment 9

[Guillaume Abadie]

Attachment: patch revision 4

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 788462 [details] [diff] [review]
patch revision 4

Review of attachment 788462 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContext.cpp
@@ +224,5 @@
> +        MOZ_ASSERT(false, "Failed to parse GL's major-minor version number separator.");
> +        return false;
> +    }
> +
> +    itr = end + 1;

Comment as to why we do this. (to advance past the decimal)

@@ +407,5 @@
>  
> +    if (mInitialized) {
> +        unsigned int version = 0;
> +
> +        mInitialized &&= ParseGLVersion(this, &version);

We don't actually want to abort GLContext init if parsing the version fails. We should just assume our guesses are reasonable.

We should probably assert that parsing succeeded, though.

@@ +417,5 @@
> +
> +        if (version >= mVersion) {
> +            mVersion = version;
> +        } else {
> +            MOZ_ASSERT(false, "Parsed version less than expected.");

Only assert this if parsing succeeded.

@@ -776,5 @@
>          mSymbols.Zero();
>          NS_WARNING("InitWithPrefix failed!");
>      }
>  
> -    mVersionString = nsPrintfCString("%u.%u.%u", mVersion / 100, (mVersion / 10) % 10, mVersion % 10);

Why has this been deleted. Is it unneeded now?

Comment 11

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> Comment on attachment 788462 [details] [diff] [review]
> patch revision 4
> 
> Review of attachment 788462 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.cpp
> @@ +224,5 @@
> > +        MOZ_ASSERT(false, "Failed to parse GL's major-minor version number separator.");
> > +        return false;
> > +    }
> > +
> > +    itr = end + 1;
> 
> Comment as to why we do this. (to advance past the decimal)
Fixed.
> 
> @@ +407,5 @@
> >  
> > +    if (mInitialized) {
> > +        unsigned int version = 0;
> > +
> > +        mInitialized &&= ParseGLVersion(this, &version);
> 
> We don't actually want to abort GLContext init if parsing the version fails.
> We should just assume our guesses are reasonable.
> 
> We should probably assert that parsing succeeded, though.
Fixed.
> 
> @@ +417,5 @@
> > +
> > +        if (version >= mVersion) {
> > +            mVersion = version;
> > +        } else {
> > +            MOZ_ASSERT(false, "Parsed version less than expected.");
> 
> Only assert this if parsing succeeded.
Fixed.
> 
> @@ -776,5 @@
> >          mSymbols.Zero();
> >          NS_WARNING("InitWithPrefix failed!");
> >      }
> >  
> > -    mVersionString = nsPrintfCString("%u.%u.%u", mVersion / 100, (mVersion / 10) % 10, mVersion % 10);
> 
> Why has this been deleted. Is it unneeded now?
Oups, that was done in the function member before. Fixed.

Comment 12

[Guillaume Abadie]

(In reply to Guillaume Abadie from comment #11)
> (In reply to Jeff Gilbert [:jgilbert] from comment #10)
> > 
> > @@ +407,5 @@
> > >  
> > > +    if (mInitialized) {
> > > +        unsigned int version = 0;
> > > +
> > > +        mInitialized &&= ParseGLVersion(this, &version);
> > 
> > We don't actually want to abort GLContext init if parsing the version fails.
> > We should just assume our guesses are reasonable.
> > 
> > We should probably assert that parsing succeeded, though.
> Fixed.
Actually, no need because ParseGLVersion already has all assertion in case of.

Comment 13

[Guillaume Abadie]

Attachment: patch revision 5

Patch for landing? :D

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 789289 [details] [diff] [review]
patch revision 5

Review of attachment 789289 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

::: gfx/gl/GLContext.cpp
@@ +129,5 @@
>      KIND_OTHER,
>      UNITS_BYTES,
>      GetTextureMemoryUsage,
>      "Memory used for storing GL textures.")
> +    

There are some errant spaces here.

Comment 15

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> Comment on attachment 789289 [details] [diff] [review]
> patch revision 5
> 
> Review of attachment 789289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> \o/
> 
> ::: gfx/gl/GLContext.cpp
> @@ +129,5 @@
> >      KIND_OTHER,
> >      UNITS_BYTES,
> >      GetTextureMemoryUsage,
> >      "Memory used for storing GL textures.")
> > +    
> 
> There are some errant spaces here.
Oups... Fixed.

Thanks!

Comment 16

[Guillaume Abadie]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5432f96b6ba0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G test bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/355dcff578b6

https://tbpl.mozilla.org/php/getParsedLog.php?id=26544854&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=26543797&tree=Mozilla-Inbound

Comment 18

[Guillaume Abadie]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> Backed out for B2G test bustage.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/355dcff578b6
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26544854&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26543797&tree=Mozilla-
> Inbound
I don't understand... It is green as an apple regardless frequent oranges that everybody is having.
https://tbpl.mozilla.org/?tree=Try&rev=9bd664108081

Comment 19

[Guillaume Abadie]

Attachment: patch revision 6: B2G emulator bug work around

Has discussed offline with bjacob, We use GL_{MAJOR,MINOR}_VERSION only on non OpenGL ES context.

Comment 20

[Guillaume Abadie]

https://tbpl.mozilla.org/?tree=Try&rev=2518f938107d
https://tbpl.mozilla.org/?tree=Try&rev=26222ca61be2

Comment 21

[Guillaume Abadie]

Attachment: patch revision 7: B2G emulator bug work around

Nits fix.

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 796867 [details] [diff] [review]
patch revision 7: B2G emulator bug work around

Review of attachment 796867 [details] [diff] [review]:
-----------------------------------------------------------------

(I understand that I only had to review the !IsGLES() hunk at the top, and the rest (the actual parser) is already r+'d by Jeff.)

Comment 23

[Guillaume Abadie]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e3879c8516c

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e3879c8516c