900101 - let GLContext extension group queries XXX_* use mVersion and mProfile

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Guillaume Abadie]

Attachment: patch revision 1

Comment 2

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

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

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

::: gfx/gl/GLContext.cpp
@@ +724,5 @@
>      }
>  
> +    mVersionString.SetLength(16);
> +    sprintf(mVersionString.Elements(), "%u.%u.%u", mVersion / 100, (mVersion / 10) % 10, mVersion % 10);
> +

mVersionString = nsPrintfCString(format, ...);

::: gfx/gl/GLContext.h
@@ +287,5 @@
>       * the context is an OpenGL 2.1 context, mVersion value will be 210.
>       */
>      unsigned int mVersion;
>      ContextProfile mProfile;
> +    nsTArray<char> mVersionString;

nsCString

@@ +490,5 @@
> +     * 
> +     * @return
> +     *  - true if that group is not supposed to be natively supported by given
> +     *    version and profile.
> +     *  - false elsewhere.

Returns false if marking this extension group as unsupported contradicts the OpenGL version and profile. Returns true otherwise.

::: gfx/gl/GLContextExtensionGroupQueries.cpp
@@ +8,5 @@
> +
> +using namespace mozilla::gl;
> +
> +namespace
> +{

namespace mozilla { namespace gl

@@ +9,5 @@
> +using namespace mozilla::gl;
> +
> +namespace
> +{
> +const uint32_t kMAX_EXTENSION_GROUP_DEPENDENCIES = 5;

size_t

@@ +19,5 @@
> +    uint32_t mOpenGLESVersion;
> +    GLContext::GLExtensions mDependencies[kMAX_EXTENSION_GROUP_DEPENDENCIES];
> +} ExtensionGroupInfos;
> +
> +const ExtensionGroupInfos sExtensionGroups [] = {

sExtensionGroupInfos

@@ +26,5 @@
> +        300, // OpenGL ES version
> +        {
> +            GLContext::ARB_draw_buffers,
> +            GLContext::EXT_draw_buffers,
> +            GLContext::Extensions_Max

add a "end" symbolic constant

@@ +110,5 @@
> +
> +inline const ExtensionGroupInfos&
> +GetExtensionGroupInfo(GLContext::GLExtensionGroup extensionGroup)
> +{
> +    MOZ_ASSERT((sizeof(sExtensionGroups) / sizeof(ExtensionGroupInfos) - 1) == GLsizei(GLContext::ExtensionGroup_Max),

static_assert

@@ +113,5 @@
> +{
> +    MOZ_ASSERT((sizeof(sExtensionGroups) / sizeof(ExtensionGroupInfos) - 1) == GLsizei(GLContext::ExtensionGroup_Max),
> +               "sExtensionGroups corrupted");
> +
> +    MOZ_ASSERT(uint32_t(extensionGroup) < uint32_t(GLContext::ExtensionGroup_Max),

no need to cast to uint32_t

@@ +116,5 @@
> +
> +    MOZ_ASSERT(uint32_t(extensionGroup) < uint32_t(GLContext::ExtensionGroup_Max),
> +               "GLContext::GetExtensionGroupInfo : unknown <extensionGroup>");
> +
> +    return sExtensionGroups[uint32_t(extensionGroup)];

no need to cast

@@ +120,5 @@
> +    return sExtensionGroups[uint32_t(extensionGroup)];
> +}
> +
> +inline uint32_t
> +NativeVersion(GLContext::GLExtensionGroup extensionGroup, ContextProfile profile)

ProfileVersionForExtensionGroup

@@ +134,5 @@
> +    return groupInfos.mOpenGLVersion;
> +}
> +
> +inline bool
> +IsSupposeToBeNativelySupported(GLContext::GLExtensionGroup extensionGroup,

IsExtensionGroupPartOfProfileVersion

@@ +144,5 @@
> +}
> +}
> +
> +namespace mozilla {
> +namespace gl {

move that to the top (as said above)

@@ +161,5 @@
> +    }
> +
> +    const ExtensionGroupInfos& groupInfos = GetExtensionGroupInfo(extensionGroup);
> +
> +    for (uint32_t i = 0; i < kMAX_EXTENSION_GROUP_DEPENDENCIES; i++)

size_t

@@ +163,5 @@
> +    const ExtensionGroupInfos& groupInfos = GetExtensionGroupInfo(extensionGroup);
> +
> +    for (uint32_t i = 0; i < kMAX_EXTENSION_GROUP_DEPENDENCIES; i++)
> +    {
> +        if (groupInfos.mDependencies[i] == GLContext::Extensions_Max) {

Adjust that to compare to the new 'end' symbolic constant!

@@ +179,5 @@
> +GLContext::MarkExtensionGroupUnsupported(GLExtensionGroup extensionGroup)
> +{
> +    const ExtensionGroupInfos& groupInfos = GetExtensionGroupInfo(extensionGroup);
> +
> +    for (uint32_t i = 0; i < kMAX_EXTENSION_GROUP_DEPENDENCIES; i++)

size_t

@@ +193,5 @@
> +                      GetExtensionGroupName(extensionGroup),
> +                      GetProfileName(mProfile),
> +                      VersionString());
> +        return false;
> +    }

Move that check to the top.

@@ +195,5 @@
> +                      VersionString());
> +        return false;
> +    }
> +
> +    MOZ_ASSERT(!IsExtensionSupported(extensionGroup), "GLContext::MarkExtensionGroupUnsupported is corrupted!");

"failed"

Comment 3

[Guillaume Abadie]

Attachment: patch revision 2

Has discussed offline with bjacob, here is the revision 2.

Comment 4

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

Comment on attachment 785130 [details] [diff] [review]
patch revision 2

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

r=me with these nits:

::: gfx/gl/GLContextExtensionGroupQueries.cpp
@@ +20,5 @@
> +};
> +
> +static const ExtensionGroupInfo sExtensionGroupInfos [] = {
> +    { "XXX_draw_buffers",
> +        200, // OpenGL version

funky coding style

@@ +137,5 @@
> +static inline bool
> +IsExtensionGroupIsPartOfProfileVersion(GLContext::GLExtensionGroup extensionGroup,
> +                                       ContextProfile profile, unsigned int version)
> +{
> +    unsigned int nativeVersion = ProfileVersionForExtensionGroup(extensionGroup, profile);

nativeVersion is not a great name

@@ +139,5 @@
> +                                       ContextProfile profile, unsigned int version)
> +{
> +    unsigned int nativeVersion = ProfileVersionForExtensionGroup(extensionGroup, profile);
> +
> +    return (nativeVersion && version >= nativeVersion);

() is unneeded

Comment 5

[Guillaume Abadie]

Attachment: patch revision 3 (r+ by bjacob)

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 785135 [details] [diff] [review]
patch revision 3 (r+ by bjacob)

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

No printfs in non-DEBUG code.

::: gfx/gl/GLContextExtensionGroupQueries.cpp
@@ +18,5 @@
> +    unsigned int mOpenGLESVersion;
> +    GLContext::GLExtensions mExtensions[kMAX_EXTENSION_GROUP_SIZE];
> +};
> +
> +static const ExtensionGroupInfo sExtensionGroupInfos [] = {

FooT{List,Arr,Map} is better than FooTs.

@@ +112,5 @@
> +            GLContext::APPLE_vertex_array_object,
> +            GLContext::Extensions_End
> +        }
> +    },
> +    { nullptr, 0, 0, { GLContext::Extensions_End } }

Why bother to pseudo-null-terminate this?
Just assert that the index is < ExtensionGroup_Max everywhere. (Which it looks like we do anyways)
It would also mean we can drop the messy `- 1` in the array size static_assert below.

@@ +119,5 @@
> +static inline const ExtensionGroupInfo&
> +GetExtensionGroupInfo(GLContext::GLExtensionGroup extensionGroup)
> +{
> +    static_assert((sizeof(sExtensionGroupInfos) / sizeof(ExtensionGroupInfo) - 1) == GLsizei(GLContext::ExtensionGroup_Max),
> +                  "sExtensionGroupInfos corrupted");

"Mismatched lengths for sExtensionGroupInfos and ExtensionGroup enums"
Also, I believe the preferred way to check the length of an arr is `sizeof(arr)/sizeof(*arr)`. If mfbt/Util.h's ArrayLength works, we should use that instead.

If we end up needing the `- 1` because we don't drop the psuedo-null-terminator above, we should probably make this: (Adding 1 to a 0-based max is common, whereas subtracting 1 from a size is somewhat strange)
sizeof(arr)/sizeof(*arr) == Max+1

@@ +152,5 @@
> +    return profileVersion && version >= profileVersion;
> +}
> +
> +const char*
> +GLContext::GetExtensionGroupName(GLExtensionGroup extensionGroup)

Can't this func be const?

@@ +158,5 @@
> +    return GetExtensionGroupInfo(extensionGroup).mName;
> +}
> +
> +bool
> +GLContext::IsExtensionSupported(GLExtensionGroup extensionGroup) const

Can't this func be const?

@@ +167,5 @@
> +
> +    const ExtensionGroupInfo& groupInfo = GetExtensionGroupInfo(extensionGroup);
> +
> +    for (size_t i = 0; i < kMAX_EXTENSION_GROUP_SIZE; i++)
> +    {

We should actually do: ([1])
while (true) {
  MOZ_ASSERT(i < kMAX_SIZE);

That way, it will crash if we ever forget to update the MAX.
If we want to be really safe, we could do:
  if (i >= kMAX) {
    MOZ_ASSERT(false, "kMAX too small.");
    break;
  }

This would be a really nice time for the for...else pattern. :(

@@ +185,5 @@
> +{
> +    MOZ_ASSERT(IsExtensionSupported(extensionGroup), "extension group is already unsupported!");
> +
> +    if (IsExtensionGroupIsPartOfProfileVersion(extensionGroup, mProfile, mVersion)) {
> +        printf_stderr("%s marked as unsupported, but it's supposed to be supported by %s %s\n",

NS_WARNING

@@ +194,5 @@
> +    }
> +
> +    const ExtensionGroupInfo& groupInfo = GetExtensionGroupInfo(extensionGroup);
> +
> +    for (size_t i = 0; i < kMAX_EXTENSION_GROUP_SIZE; i++)

[1]

@@ +205,5 @@
> +
> +    MOZ_ASSERT(!IsExtensionSupported(extensionGroup), "GLContext::MarkExtensionGroupUnsupported has failed!");
> +
> +    printf_stderr("%s marked as unsupported\n",
> +                  GetExtensionGroupName(extensionGroup));

NS_WARNING(nsPrintfCString("...", ...).get())

Comment 7

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Comment on attachment 785135 [details] [diff] [review]
> patch revision 3 (r+ by bjacob)
> 
> Review of attachment 785135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> No printfs in non-DEBUG code.
> 
> ::: gfx/gl/GLContextExtensionGroupQueries.cpp
> @@ +18,5 @@
> > +    unsigned int mOpenGLESVersion;
> > +    GLContext::GLExtensions mExtensions[kMAX_EXTENSION_GROUP_SIZE];
> > +};
> > +
> > +static const ExtensionGroupInfo sExtensionGroupInfos [] = {
> 
> FooT{List,Arr,Map} is better than FooTs.
Changed by sExtensionGroupInfoArr
> 
> @@ +112,5 @@
> > +            GLContext::APPLE_vertex_array_object,
> > +            GLContext::Extensions_End
> > +        }
> > +    },
> > +    { nullptr, 0, 0, { GLContext::Extensions_End } }
> 
> Why bother to pseudo-null-terminate this?
> Just assert that the index is < ExtensionGroup_Max everywhere. (Which it
> looks like we do anyways)
> It would also mean we can drop the messy `- 1` in the array size
> static_assert below.
Fixed.
> 
> @@ +119,5 @@
> > +static inline const ExtensionGroupInfo&
> > +GetExtensionGroupInfo(GLContext::GLExtensionGroup extensionGroup)
> > +{
> > +    static_assert((sizeof(sExtensionGroupInfos) / sizeof(ExtensionGroupInfo) - 1) == GLsizei(GLContext::ExtensionGroup_Max),
> > +                  "sExtensionGroupInfos corrupted");
> 
> "Mismatched lengths for sExtensionGroupInfos and ExtensionGroup enums"
> Also, I believe the preferred way to check the length of an arr is
> `sizeof(arr)/sizeof(*arr)`. If mfbt/Util.h's ArrayLength works, we should
> use that instead.
Fixed.
> 
> If we end up needing the `- 1` because we don't drop the
> psuedo-null-terminator above, we should probably make this: (Adding 1 to a
> 0-based max is common, whereas subtracting 1 from a size is somewhat strange)
> sizeof(arr)/sizeof(*arr) == Max+1
> 
> @@ +152,5 @@
> > +    return profileVersion && version >= profileVersion;
> > +}
> > +
> > +const char*
> > +GLContext::GetExtensionGroupName(GLExtensionGroup extensionGroup)
> 
> Can't this func be const?
This is a static function in the GLContext.h
> 
> @@ +158,5 @@
> > +    return GetExtensionGroupInfo(extensionGroup).mName;
> > +}
> > +
> > +bool
> > +GLContext::IsExtensionSupported(GLExtensionGroup extensionGroup) const
> 
> Can't this func be const?
It's already an const ...
> 
> @@ +167,5 @@
> > +
> > +    const ExtensionGroupInfo& groupInfo = GetExtensionGroupInfo(extensionGroup);
> > +
> > +    for (size_t i = 0; i < kMAX_EXTENSION_GROUP_SIZE; i++)
> > +    {
> 
> We should actually do: ([1])
> while (true) {
>   MOZ_ASSERT(i < kMAX_SIZE);
> 
> That way, it will crash if we ever forget to update the MAX.
> If we want to be really safe, we could do:
>   if (i >= kMAX) {
>     MOZ_ASSERT(false, "kMAX too small.");
>     break;
>   }
Ok.
> 
> This would be a really nice time for the for...else pattern. :(
Oups...
> 
> @@ +185,5 @@
> > +{
> > +    MOZ_ASSERT(IsExtensionSupported(extensionGroup), "extension group is already unsupported!");
> > +
> > +    if (IsExtensionGroupIsPartOfProfileVersion(extensionGroup, mProfile, mVersion)) {
> > +        printf_stderr("%s marked as unsupported, but it's supposed to be supported by %s %s\n",
> 
> NS_WARNING
> 
> @@ +194,5 @@
> > +    }
> > +
> > +    const ExtensionGroupInfo& groupInfo = GetExtensionGroupInfo(extensionGroup);
> > +
> > +    for (size_t i = 0; i < kMAX_EXTENSION_GROUP_SIZE; i++)
> 
> [1]
> 
> @@ +205,5 @@
> > +
> > +    MOZ_ASSERT(!IsExtensionSupported(extensionGroup), "GLContext::MarkExtensionGroupUnsupported has failed!");
> > +
> > +    printf_stderr("%s marked as unsupported\n",
> > +                  GetExtensionGroupName(extensionGroup));
> 
> NS_WARNING(nsPrintfCString("...", ...).get())

Comment 8

[Guillaume Abadie]

Attachment: patch revision 4 (r+ by bjacob)

Nits fixed.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 785193 [details] [diff] [review]
patch revision 4 (r+ by bjacob)

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

NS_WARNING handles its own line termination.

::: gfx/gl/GLContext.cpp
@@ +15,5 @@
>  #include "GLContextProvider.h"
>  #include "GLTextureImage.h"
>  #include "nsIMemoryReporter.h"
>  #include "nsThreadUtils.h"
> +#include "nsPrintfCString.h"

nsP is before nsT, but after nsI.

::: gfx/gl/GLContext.h
@@ +199,5 @@
>  
>      /**
> +     * Return true if we are running on a OpenGL core profile context
> +     */
> +    inline const char* ProfileString() const {

`inline` is redundant in class definitions.

::: gfx/gl/GLContextExtensionGroupQueries.cpp
@@ +19,5 @@
> +    unsigned int mOpenGLESVersion;
> +    GLContext::GLExtensions mExtensions[kMAX_EXTENSION_GROUP_SIZE];
> +};
> +
> +static const ExtensionGroupInfo sExtensionGroupInfoArr [] = {

no space between identifier and []

@@ +191,5 @@
> +    if (IsExtensionGroupIsPartOfProfileVersion(extensionGroup, mProfile, mVersion)) {
> +        NS_WARNING(nsPrintfCString("%s marked as unsupported, but it's supposed to be supported by %s %s\n",
> +                                   GetExtensionGroupName(extensionGroup),
> +                                   ProfileString(),
> +                                   VersionString()).get());

No trailing \n in NS_WARNING.

@@ +210,5 @@
> +    }
> +
> +    MOZ_ASSERT(!IsExtensionSupported(extensionGroup), "GLContext::MarkExtensionGroupUnsupported has failed!");
> +
> +    NS_WARNING(nsPrintfCString("%s marked as unsupported\n", GetExtensionGroupName(extensionGroup)).get());

No trailing \n in NS_WARNING.

Comment 10

[Guillaume Abadie]

(In reply to Jeff Gilbert [:jgilbert] from comment #9)
> Comment on attachment 785193 [details] [diff] [review]
> patch revision 4 (r+ by bjacob)
> 
> Review of attachment 785193 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> NS_WARNING handles its own line termination.
> 
> ::: gfx/gl/GLContext.cpp
> @@ +15,5 @@
> >  #include "GLContextProvider.h"
> >  #include "GLTextureImage.h"
> >  #include "nsIMemoryReporter.h"
> >  #include "nsThreadUtils.h"
> > +#include "nsPrintfCString.h"
> 
> nsP is before nsT, but after nsI.
> 
> ::: gfx/gl/GLContext.h
> @@ +199,5 @@
> >  
> >      /**
> > +     * Return true if we are running on a OpenGL core profile context
> > +     */
> > +    inline const char* ProfileString() const {
> 
> `inline` is redundant in class definitions.
> 
> ::: gfx/gl/GLContextExtensionGroupQueries.cpp
> @@ +19,5 @@
> > +    unsigned int mOpenGLESVersion;
> > +    GLContext::GLExtensions mExtensions[kMAX_EXTENSION_GROUP_SIZE];
> > +};
> > +
> > +static const ExtensionGroupInfo sExtensionGroupInfoArr [] = {
> 
> no space between identifier and []
> 
> @@ +191,5 @@
> > +    if (IsExtensionGroupIsPartOfProfileVersion(extensionGroup, mProfile, mVersion)) {
> > +        NS_WARNING(nsPrintfCString("%s marked as unsupported, but it's supposed to be supported by %s %s\n",
> > +                                   GetExtensionGroupName(extensionGroup),
> > +                                   ProfileString(),
> > +                                   VersionString()).get());
> 
> No trailing \n in NS_WARNING.
> 
> @@ +210,5 @@
> > +    }
> > +
> > +    MOZ_ASSERT(!IsExtensionSupported(extensionGroup), "GLContext::MarkExtensionGroupUnsupported has failed!");
> > +
> > +    NS_WARNING(nsPrintfCString("%s marked as unsupported\n", GetExtensionGroupName(extensionGroup)).get());
> 
> No trailing \n in NS_WARNING.
All fixed. Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/7052c5bee580

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7052c5bee580
https://hg.mozilla.org/mozilla-central/rev/d93fe7628f85

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

> https://hg.mozilla.org/mozilla-central/rev/d93fe7628f85

Better as static_assert(MOZ_ARRAY_LENGTH(... Can you do that?

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to :Ms2ger from comment #12)
> > https://hg.mozilla.org/mozilla-central/rev/d93fe7628f85
> 
> Better as static_assert(MOZ_ARRAY_LENGTH(... Can you do that?

I found bug 901131.